Merge pull request #19444 from chennes/addonManagerChangeBranchCleanup

Addon manager: change branch cleanup

src/Mod/AddonManager/AddonManagerTest/gui/test_change_branch.py

@@ -0,0 +1,234 @@
+# SPDX-License-Identifier: LGPL-2.1-or-later
+# ***************************************************************************
+# *                                                                         *
+# *   Copyright (c) 2025 The FreeCAD Project Association AISBL              *
+# *                                                                         *
+# *   This file is part of FreeCAD.                                         *
+# *                                                                         *
+# *   FreeCAD is free software: you can redistribute it and/or modify it    *
+# *   under the terms of the GNU Lesser General Public License as           *
+# *   published by the Free Software Foundation, either version 2.1 of the  *
+# *   License, or (at your option) any later version.                       *
+# *                                                                         *
+# *   FreeCAD is distributed in the hope that it will be useful, but        *
+# *   WITHOUT ANY WARRANTY; without even the implied warranty of            *
+# *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU      *
+# *   Lesser General Public License for more details.                       *
+# *                                                                         *
+# *   You should have received a copy of the GNU Lesser General Public      *
+# *   License along with FreeCAD. If not, see                               *
+# *   <https://www.gnu.org/licenses/>.                                      *
+# *                                                                         *
+# ***************************************************************************
+
+"""Test the Change Branch GUI code"""
+
+# pylint: disable=wrong-import-position, deprecated-module, too-many-return-statements
+
+import sys
+import unittest
+from unittest.mock import patch, Mock, MagicMock
+
+# So that when run standalone, the Addon Manager classes imported below are available
+sys.path.append("../..")
+
+from AddonManagerTest.gui.gui_mocks import DialogWatcher, AsynchronousMonitor
+
+from change_branch import ChangeBranchDialog
+
+from addonmanager_freecad_interface import translate
+from addonmanager_git import GitFailed
+
+try:
+    from PySide import QtCore, QtWidgets
+except ImportError:
+    try:
+        from PySide6 import QtCore, QtWidgets
+    except ImportError:
+        from PySide2 import QtCore, QtWidgets
+
+
+class MockFilter(QtCore.QSortFilterProxyModel):
+    """Replaces a filter with a non-filter that simply always returns whatever it's given"""
+
+    def mapToSource(self, something):
+        return something
+
+
+class MockChangeBranchDialogModel(QtCore.QAbstractTableModel):
+    """Replace a data-connected model with a static one for testing"""
+
+    branches = [
+        {"ref_name": "ref1", "upstream": "us1"},
+        {"ref_name": "ref2", "upstream": "us2"},
+        {"ref_name": "ref3", "upstream": "us3"},
+    ]
+    current_branch = "ref1"
+    DataSortRole = QtCore.Qt.UserRole
+    RefAccessRole = QtCore.Qt.UserRole + 1
+
+    def __init__(self, _: str, parent=None) -> None:
+        super().__init__(parent)
+
+    def rowCount(self, parent: QtCore.QModelIndex = QtCore.QModelIndex()) -> int:
+        """Number of rows: should always return 3"""
+        if parent.isValid():
+            return 0
+        return len(self.branches)
+
+    def columnCount(self, parent: QtCore.QModelIndex = QtCore.QModelIndex()) -> int:
+        """Number of columns (identical to non-mocked version)"""
+        if parent.isValid():
+            return 0
+        return 3  # Local name, remote name, date
+
+    def data(self, index: QtCore.QModelIndex, role: int = QtCore.Qt.DisplayRole):
+        """Mock returns static untranslated strings for DisplayRole, no tooltips at all, and
+        otherwise matches the non-mock version"""
+        if not index.isValid():
+            return None
+        row = index.row()
+        column = index.column()
+        if role == QtCore.Qt.DisplayRole:
+            if column == 2:
+                return "date"
+            if column == 0:
+                return "ref_name"
+            if column == 1:
+                return "upstream"
+            return None
+        if role == MockChangeBranchDialogModel.DataSortRole:
+            return None
+        if role == MockChangeBranchDialogModel.RefAccessRole:
+            return self.branches[row]
+        return None
+
+    def headerData(
+        self,
+        section: int,
+        orientation: QtCore.Qt.Orientation,
+        role: int = QtCore.Qt.DisplayRole,
+    ):
+        """Mock returns untranslated strings for DisplayRole, and no tooltips at all"""
+        if orientation == QtCore.Qt.Vertical:
+            return None
+        if role != QtCore.Qt.DisplayRole:
+            return None
+        if section == 0:
+            return "Local"
+        if section == 1:
+            return "Remote tracking"
+        if section == 2:
+            return "Last Updated"
+        return None
+
+    def currentBranch(self) -> str:
+        """Mock returns a static string stored in the class: that string could be modified to
+        return something else by tests that require it."""
+        return self.current_branch
+
+
+class TestChangeBranchGui(unittest.TestCase):
+    """Tests for the ChangeBranch GUI code"""
+
+    MODULE = "test_change_branch"  # file name without extension
+
+    def setUp(self):
+        pass
+
+    def tearDown(self):
+        pass
+
+    @patch("change_branch.ChangeBranchDialogModel", new=MockChangeBranchDialogModel)
+    @patch("change_branch.initialize_git", new=Mock(return_value=None))
+    def test_no_git(self):
+        """If git is not present, a dialog saying so is presented"""
+        # Arrange
+        gui = ChangeBranchDialog("/some/path")
+        ref = {"ref_name": "foo/bar", "upstream": "us1"}
+        dialog_watcher = DialogWatcher(
+            translate("AddonsInstaller", "Cannot find git"),
+            QtWidgets.QDialogButtonBox.Ok,
+        )
+
+        # Act
+        gui.change_branch("/foo/bar/baz", ref)
+
+        # Assert
+        self.assertTrue(dialog_watcher.dialog_found, "Failed to find the expected dialog box")
+
+    @patch("change_branch.ChangeBranchDialogModel", new=MockChangeBranchDialogModel)
+    @patch("change_branch.initialize_git")
+    def test_git_failed(self, init_git: MagicMock):
+        """If git fails when attempting to change branches, a dialog saying so is presented"""
+        # Arrange
+        git_manager = MagicMock()
+        git_manager.checkout = MagicMock()
+        git_manager.checkout.side_effect = GitFailed()
+        init_git.return_value = git_manager
+        gui = ChangeBranchDialog("/some/path")
+        ref = {"ref_name": "foo/bar", "upstream": "us1"}
+        dialog_watcher = DialogWatcher(
+            translate("AddonsInstaller", "git operation failed"),
+            QtWidgets.QDialogButtonBox.Ok,
+        )
+
+        # Act
+        gui.change_branch("/foo/bar/baz", ref)
+
+        # Assert
+        self.assertTrue(dialog_watcher.dialog_found, "Failed to find the expected dialog box")
+
+    @patch("change_branch.ChangeBranchDialogModel", new=MockChangeBranchDialogModel)
+    @patch("change_branch.initialize_git", new=MagicMock)
+    def test_branch_change_succeeded(self):
+        """If nothing gets thrown, then the process is assumed to have worked, and the appropriate
+        signal is emitted."""
+
+        # Arrange
+        gui = ChangeBranchDialog("/some/path")
+        ref = {"ref_name": "foo/bar", "upstream": "us1"}
+        monitor = AsynchronousMonitor(gui.branch_changed)
+
+        # Act
+        gui.change_branch("/foo/bar/baz", ref)
+
+        # Assert
+        monitor.wait_for_at_most(10)  # Should be effectively instantaneous
+        self.assertTrue(monitor.good())
+
+    @patch("change_branch.ChangeBranchDialogFilter", new=MockFilter)
+    @patch("change_branch.ChangeBranchDialogModel", new=MockChangeBranchDialogModel)
+    @patch("change_branch.initialize_git", new=MagicMock)
+    def test_warning_is_shown_when_dialog_is_accepted(self):
+        """If the dialog is accepted (e.g. a branch change is requested) then a warning dialog is
+        displayed, and gives the opportunity to cancel. If cancelled, no signal is emitted."""
+        # Arrange
+        gui = ChangeBranchDialog("/some/path")
+        gui.ui.exec = MagicMock()
+        gui.ui.exec.return_value = QtWidgets.QDialog.Accepted
+        gui.ui.tableView.selectedIndexes = MagicMock()
+        gui.ui.tableView.selectedIndexes.return_value = [MagicMock()]
+        gui.ui.tableView.selectedIndexes.return_value[0].isValid = MagicMock()
+        gui.ui.tableView.selectedIndexes.return_value[0].isValid.return_value = True
+        dialog_watcher = DialogWatcher(
+            translate("AddonsInstaller", "DANGER: Developer feature"),
+            QtWidgets.QDialogButtonBox.Cancel,
+        )
+        monitor = AsynchronousMonitor(gui.branch_changed)
+
+        # Act
+        gui.exec()
+
+        # Assert
+        self.assertTrue(dialog_watcher.dialog_found, "Failed to find the expected dialog box")
+        self.assertFalse(monitor.good())  # The watcher cancelled the op, so no signal is emitted
+
+
+if __name__ == "__main__":
+    app = QtWidgets.QApplication(sys.argv)
+    QtCore.QTimer.singleShot(0, unittest.main)
+    if hasattr(app, "exec"):
+        app.exec()  # PySide6
+    else:
+        app.exec_()  # PySide2

src/Mod/AddonManager/addonmanager_freecad_interface.py

@@ -46,9 +46,13 @@ try:
     getUserMacroDir = FreeCAD.getUserMacroDir
     getUserCachePath = FreeCAD.getUserCachePath
     translate = FreeCAD.Qt.translate
+    loadUi = None
 
     if FreeCAD.GuiUp:
         import FreeCADGui
+
+        if hasattr(FreeCADGui, "PySideUic"):
+            loadUi = FreeCADGui.PySideUic.loadUi
     else:
         FreeCADGui = None
 
@@ -63,7 +67,7 @@ except ImportError:
         return string
 
     def Version():
-        return 0, 22, 0, "dev"
+        return 1, 1, 0, "dev"
 
     class ConsoleReplacement:
         """If FreeCAD's Console is not available, create a replacement by redirecting FreeCAD

src/Mod/AddonManager/addonmanager_package_details_controller.py

@@ -248,18 +248,6 @@ class PackageDetailsController(QtCore.QObject):
     def branch_changed(self, old_branch: str, name: str) -> None:
         """Displays a dialog confirming the branch changed, and tries to access the
         metadata file from that branch."""
-        QtWidgets.QMessageBox.information(
-            self.ui,
-            translate("AddonsInstaller", "Success"),
-            translate(
-                "AddonsInstaller",
-                "Branch change succeeded.\n"
-                "Moved\n"
-                "from: {}\n"
-                "to: {}\n"
-                "Please restart to use the new version.",
-            ).format(old_branch, name),
-        )
         # See if this branch has a package.xml file:
         basedir = fci.getUserAppDataDir()
         path_to_metadata = os.path.join(basedir, "Mod", self.addon.name, "package.xml")
@@ -275,6 +263,18 @@ class PackageDetailsController(QtCore.QObject):
         self.addon.set_status(Addon.Status.PENDING_RESTART)
         self.ui.set_new_branch(name)
         self.update_status.emit(self.addon)
+        QtWidgets.QMessageBox.information(
+            self.ui,
+            translate("AddonsInstaller", "Success"),
+            translate(
+                "AddonsInstaller",
+                "Branch change succeeded.\n"
+                "Moved\n"
+                "from: {}\n"
+                "to: {}\n"
+                "Please restart to use the new version.",
+            ).format(old_branch, name),
+        )
 
     def display_repo_status(self, addon):
         self.update_status.emit(self.addon)

src/Mod/AddonManager/addonmanager_utilities.py

@@ -24,6 +24,8 @@
 
 """Utilities to work across different platforms, providers and python versions"""
 
+# pylint: disable=deprecated-module, ungrouped-imports
+
 from datetime import datetime
 from typing import Optional, Any, List
 import os
@@ -52,6 +54,7 @@ try:
 except ImportError:
 
     def get_python_exe():
+        """Use shutil.which to find python executable"""
         return shutil.which("python")
 
 
@@ -61,14 +64,52 @@ if fci.FreeCADGui:
     # loop running this is not possible, so fall back to requests (if available), or the native
     # Python urllib.request (if requests is not available).
     import NetworkManager  # Requires an event loop, so is only available with the GUI
+
+    requests = None
+    ssl = None
+    urllib = None
 else:
+    NetworkManager = None
     try:
         import requests
+
+        ssl = None
+        urllib = None
     except ImportError:
         requests = None
         import urllib.request
         import ssl
 
+if fci.FreeCADGui:
+    loadUi = fci.loadUi
+else:
+    has_loader = False
+    try:
+        from PySide6.QtUiTools import QUiLoader
+
+        has_loader = True
+    except ImportError:
+        try:
+            from PySide2.QtUiTools import QUiLoader
+
+            has_loader = True
+        except ImportError:
+
+            def loadUi(ui_file: str):
+                """If there are no available versions of QtUiTools, then raise an error if this
+                method is used."""
+                raise RuntimeError("Cannot use QUiLoader without PySide or FreeCAD")
+
+    if has_loader:
+
+        def loadUi(ui_file: str) -> QtWidgets.QWidget:
+            """Load a Qt UI from an on-disk file."""
+            q_ui_file = QtCore.QFile(ui_file)
+            q_ui_file.open(QtCore.QFile.OpenModeFlag.ReadOnly)
+            loader = QUiLoader()
+            return loader.load(ui_file)
+
+
 #  @package AddonManager_utilities
 #  \ingroup ADDONMANAGER
 #  \brief Utilities to work across different platforms, providers and python versions
@@ -108,10 +149,13 @@ def symlink(source, link_name):
 
 
 def rmdir(path: str) -> bool:
+    """Remove a directory or symlink, even if it is read-only."""
     try:
         if os.path.islink(path):
             os.unlink(path)  # Remove symlink
         else:
+            # NOTE: the onerror argument was deprecated in Python 3.12, replaced by onexc -- replace
+            # when earlier versions are no longer supported.
             shutil.rmtree(path, onerror=remove_readonly)
     except (WindowsError, PermissionError, OSError):
         return False
@@ -186,7 +230,7 @@ def get_zip_url(repo):
 
 
 def recognized_git_location(repo) -> bool:
-    """Returns whether this repo is based at a known git repo location: works with github, gitlab,
+    """Returns whether this repo is based at a known git repo location: works with GitHub, gitlab,
     framagit, and salsa.debian.org"""
 
     parsed_url = urlparse(repo.url)
@@ -368,7 +412,7 @@ def is_float(element: Any) -> bool:
 
 
 def get_pip_target_directory():
-    # Get the default location to install new pip packages
+    """Get the default location to install new pip packages"""
     major, minor, _ = platform.python_version_tuple()
     vendor_path = os.path.join(
         fci.DataPaths().mod_dir, "..", "AdditionalPythonPackages", f"py{major}{minor}"
@@ -390,7 +434,12 @@ def blocking_get(url: str, method=None) -> bytes:
     succeeded, or an empty string if it failed, or returned no data. The method argument is
     provided mainly for testing purposes."""
     p = b""
-    if fci.FreeCADGui and method is None or method == "networkmanager":
+    if (
+        fci.FreeCADGui
+        and method is None
+        or method == "networkmanager"
+        and NetworkManager is not None
+    ):
         NetworkManager.InitializeNetworkManager()
         p = NetworkManager.AM_NETWORK_MANAGER.blocking_get(url, 10000)  # 10 second timeout
         if p:
@@ -435,13 +484,13 @@ def run_interruptable_subprocess(args, timeout_secs: int = 10) -> subprocess.Com
             # one second timeout allows interrupting the run once per second
             stdout, stderr = p.communicate(timeout=1)
             return_code = p.returncode
-        except subprocess.TimeoutExpired:
+        except subprocess.TimeoutExpired as timeout_exception:
             if (
                 hasattr(QtCore, "QThread")
                 and QtCore.QThread.currentThread().isInterruptionRequested()
             ):
                 p.kill()
-                raise ProcessInterrupted()
+                raise ProcessInterrupted() from timeout_exception
             if time.time() - start_time >= timeout_secs:  # The real timeout
                 p.kill()
                 stdout, stderr = p.communicate()
@@ -454,9 +503,10 @@ def run_interruptable_subprocess(args, timeout_secs: int = 10) -> subprocess.Com
 
 
 def process_date_string_to_python_datetime(date_string: str) -> datetime:
-    """For modern macros the expected date format is ISO 8601, YYYY-MM-DD. For older macros this standard was not always
-    used, and various orderings and separators were used. This function tries to match the majority of those older
-    macros. Commonly-used separators are periods, slashes, and dashes."""
+    """For modern macros the expected date format is ISO 8601, YYYY-MM-DD. For older macros this
+    standard was not always used, and various orderings and separators were used. This function
+    tries to match the majority of those older macros. Commonly-used separators are periods,
+    slashes, and dashes."""
 
     def raise_error(bad_string: str, root_cause: Exception = None):
         raise ValueError(
@@ -472,19 +522,19 @@ def process_date_string_to_python_datetime(date_string: str) -> datetime:
         # The earliest possible year an addon can be created or edited is 2001:
         if split_result[0] > 2000:
             return datetime(split_result[0], split_result[1], split_result[2])
-        elif split_result[2] > 2000:
-            # Generally speaking it's not possible to distinguish between DD-MM and MM-DD, so try the first, and
-            # only if that fails try the second
+        if split_result[2] > 2000:
+            # Generally speaking it's not possible to distinguish between DD-MM and MM-DD, so try
+            # the first, and only if that fails try the second
             if split_result[1] <= 12:
                 return datetime(split_result[2], split_result[1], split_result[0])
             return datetime(split_result[2], split_result[0], split_result[1])
-        else:
-            raise ValueError(f"Invalid year in date string '{date_string}'")
+        raise ValueError(f"Invalid year in date string '{date_string}'")
     except ValueError as exception:
         raise_error(date_string, exception)
 
 
 def get_main_am_window():
+    """Find the Addon Manager's main window in the Qt widget hierarchy."""
     windows = QtWidgets.QApplication.topLevelWidgets()
     for widget in windows:
         if widget.objectName() == "AddonManager_Main_Window":
@@ -513,7 +563,7 @@ def create_pip_call(args: List[str]) -> List[str]:
     else:
         python_exe = get_python_exe()
         if not python_exe:
-            raise (RuntimeError("Could not locate Python executable on this system"))
+            raise RuntimeError("Could not locate Python executable on this system")
         call_args = [python_exe, "-m", "pip", "--disable-pip-version-check"]
         call_args.extend(args)
     return call_args

src/Mod/AddonManager/change_branch.py

@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: LGPL-2.1-or-later
 # ***************************************************************************
 # *                                                                         *
-# *   Copyright (c) 2022-2023 FreeCAD Project Association                   *
+# *   Copyright (c) 2022-2025 The FreeCAD Project Association AISBL         *
 # *                                                                         *
 # *   This file is part of FreeCAD.                                         *
 # *                                                                         *
@@ -21,27 +21,38 @@
 # *                                                                         *
 # ***************************************************************************
 
+"""The Change Branch dialog and utility classes and methods"""
+
 import os
+from typing import Dict
 
-import FreeCAD
-import FreeCADGui
-from addonmanager_git import initialize_git
+import addonmanager_freecad_interface as fci
+import addonmanager_utilities as utils
 
-from PySide import QtWidgets, QtCore
+from addonmanager_git import initialize_git, GitFailed
 
-translate = FreeCAD.Qt.translate
+try:
+    from PySide import QtWidgets, QtCore
+except ImportError:
+    try:
+        from PySide6 import QtWidgets, QtCore
+    except ImportError:
+        from PySide2 import QtWidgets, QtCore  # pylint: disable=deprecated-module
+
+translate = fci.translate
 
 
 class ChangeBranchDialog(QtWidgets.QWidget):
+    """A dialog that displays available git branches and allows the user to select one to change
+    to. Includes code that does that change, as well as some modal dialogs to warn them of the
+    possible consequences and display various error messages."""
 
     branch_changed = QtCore.Signal(str, str)
 
     def __init__(self, path: str, parent=None):
         super().__init__(parent)
 
-        self.ui = FreeCADGui.PySideUic.loadUi(
-            os.path.join(os.path.dirname(__file__), "change_branch.ui")
-        )
+        self.ui = utils.loadUi(os.path.join(os.path.dirname(__file__), "change_branch.ui"))
 
         self.item_filter = ChangeBranchDialogFilter()
         self.ui.tableView.setModel(self.item_filter)
@@ -54,6 +65,9 @@ class ChangeBranchDialog(QtWidgets.QWidget):
 
         # Figure out what row gets selected:
         git_manager = initialize_git()
+        if git_manager is None:
+            return
+
         row = 0
         self.current_ref = git_manager.current_branch(path)
         selection_model = self.ui.tableView.selectionModel()
@@ -71,6 +85,9 @@ class ChangeBranchDialog(QtWidgets.QWidget):
         header.setSectionResizeMode(QtWidgets.QHeaderView.ResizeToContents)
 
     def exec(self):
+        """Run the Change Branch dialog and its various sub-dialogs. May result in the branch
+        being changed. Code that cares if that happens should connect to the branch_changed
+        signal."""
         if self.ui.exec() == QtWidgets.QDialog.Accepted:
 
             selection = self.ui.tableView.selectedIndexes()
@@ -111,17 +128,58 @@ class ChangeBranchDialog(QtWidgets.QWidget):
                 if result == QtWidgets.QMessageBox.Cancel:
                     return
 
-            gm = initialize_git()
-            remote_name = ref["ref_name"]
-            _, _, local_name = ref["ref_name"].rpartition("/")
+            self.change_branch(self.item_model.path, ref)
+
+    def change_branch(self, path: str, ref: Dict[str, str]) -> None:
+        """Change the git clone in `path` to git ref `ref`. Emits the branch_changed signal
+        on success."""
+        remote_name = ref["ref_name"]
+        _, _, local_name = ref["ref_name"].rpartition("/")
+        gm = initialize_git()
+        if gm is None:
+            self._show_no_git_dialog()
+            return
+
+        try:
             if ref["upstream"]:
-                gm.checkout(self.item_model.path, remote_name)
+                gm.checkout(path, remote_name)
             else:
-                gm.checkout(self.item_model.path, remote_name, args=["-b", local_name])
+                gm.checkout(path, remote_name, args=["-b", local_name])
             self.branch_changed.emit(self.current_ref, local_name)
+        except GitFailed:
+            self._show_git_failed_dialog()
+
+    def _show_no_git_dialog(self):
+        QtWidgets.QMessageBox.critical(
+            self,
+            translate("AddonsInstaller", "Cannot find git"),
+            translate(
+                "AddonsInstaller",
+                "Could not find git executable: cannot change branch",
+            ),
+            QtWidgets.QMessageBox.Ok,
+            QtWidgets.QMessageBox.Ok,
+        )
+
+    def _show_git_failed_dialog(self):
+        QtWidgets.QMessageBox.critical(
+            self,
+            translate("AddonsInstaller", "git operation failed"),
+            translate(
+                "AddonsInstaller",
+                "Git returned an error code when attempting to change branch. There may be "
+                "more details in the Report View.",
+            ),
+            QtWidgets.QMessageBox.Ok,
+            QtWidgets.QMessageBox.Ok,
+        )
 
 
 class ChangeBranchDialogModel(QtCore.QAbstractTableModel):
+    """The data for the dialog comes from git: this model handles the git interactions and
+    returns branch information as its rows. Use user data in the RefAccessRole to get information
+    about the git refs. RefAccessRole data is a dictionary defined by the GitManager class as the
+    results of a `get_branches_with_info()` call."""
 
     branches = []
     DataSortRole = QtCore.Qt.UserRole
@@ -138,54 +196,61 @@ class ChangeBranchDialogModel(QtCore.QAbstractTableModel):
         self._remove_tracking_duplicates()
 
     def rowCount(self, parent: QtCore.QModelIndex = QtCore.QModelIndex()) -> int:
+        """Returns the number of rows in the model, e.g. the number of branches."""
         if parent.isValid():
             return 0
         return len(self.branches)
 
     def columnCount(self, parent: QtCore.QModelIndex = QtCore.QModelIndex()) -> int:
+        """Returns the number of columns in the model, e.g. the number of entries in the git ref
+        structure (currently 3, 'ref_name', 'upstream', and 'date')."""
         if parent.isValid():
             return 0
         return 3  # Local name, remote name, date
 
     def data(self, index: QtCore.QModelIndex, role: int = QtCore.Qt.DisplayRole):
+        """The data access method for this model. Supports four roles: ToolTipRole, DisplayRole,
+        DataSortRole, and RefAccessRole."""
         if not index.isValid():
             return None
         row = index.row()
         column = index.column()
         if role == QtCore.Qt.ToolTipRole:
-            tooltip = self.branches[row]["author"] + ": " + self.branches[row]["subject"]
-            return tooltip
-        elif role == QtCore.Qt.DisplayRole:
-            dd = self.branches[row]
-            if column == 2:
-                if dd["date"] is not None:
-                    q_date = QtCore.QDateTime.fromString(
-                        dd["date"], QtCore.Qt.DateFormat.RFC2822Date
-                    )
-                    return QtCore.QLocale().toString(q_date, QtCore.QLocale.ShortFormat)
-                return None
-            elif column == 0:
-                return dd["ref_name"]
-            elif column == 1:
-                return dd["upstream"]
-            else:
-                return None
-        elif role == ChangeBranchDialogModel.DataSortRole:
-            if column == 2:
-                if self.branches[row]["date"] is not None:
-                    q_date = QtCore.QDateTime.fromString(
-                        self.branches[row]["date"], QtCore.Qt.DateFormat.RFC2822Date
-                    )
-                    return q_date
-                return None
-            elif column == 0:
-                return self.branches[row]["ref_name"]
-            elif column == 1:
-                return self.branches[row]["upstream"]
-            else:
-                return None
-        elif role == ChangeBranchDialogModel.RefAccessRole:
+            return self.branches[row]["author"] + ": " + self.branches[row]["subject"]
+        if role == QtCore.Qt.DisplayRole:
+            return self._data_display_role(column, row)
+        if role == ChangeBranchDialogModel.DataSortRole:
+            return self._data_sort_role(column, row)
+        if role == ChangeBranchDialogModel.RefAccessRole:
             return self.branches[row]
+        return None
+
+    def _data_display_role(self, column, row):
+        dd = self.branches[row]
+        if column == 2:
+            if dd["date"] is not None:
+                q_date = QtCore.QDateTime.fromString(dd["date"], QtCore.Qt.DateFormat.RFC2822Date)
+                return QtCore.QLocale().toString(q_date, QtCore.QLocale.ShortFormat)
+            return None
+        if column == 0:
+            return dd["ref_name"]
+        if column == 1:
+            return dd["upstream"]
+        return None
+
+    def _data_sort_role(self, column, row):
+        if column == 2:
+            if self.branches[row]["date"] is not None:
+                q_date = QtCore.QDateTime.fromString(
+                    self.branches[row]["date"], QtCore.Qt.DateFormat.RFC2822Date
+                )
+                return q_date
+            return None
+        if column == 0:
+            return self.branches[row]["ref_name"]
+        if column == 1:
+            return self.branches[row]["upstream"]
+        return None
 
     def headerData(
         self,
@@ -193,6 +258,7 @@ class ChangeBranchDialogModel(QtCore.QAbstractTableModel):
         orientation: QtCore.Qt.Orientation,
         role: int = QtCore.Qt.DisplayRole,
     ):
+        """Returns the header information for the data in this model."""
         if orientation == QtCore.Qt.Vertical:
             return None
         if role != QtCore.Qt.DisplayRole:
@@ -209,14 +275,13 @@ class ChangeBranchDialogModel(QtCore.QAbstractTableModel):
                 "Remote tracking",
                 "Table header for git remote tracking branch name",
             )
-        elif section == 2:
+        if section == 2:
             return translate(
                 "AddonsInstaller",
                 "Last Updated",
                 "Table header for git update date",
             )
-        else:
-            return None
+        return None
 
     def _remove_tracking_duplicates(self):
         remote_tracking_branches = []
@@ -234,12 +299,12 @@ class ChangeBranchDialogModel(QtCore.QAbstractTableModel):
 
 
 class ChangeBranchDialogFilter(QtCore.QSortFilterProxyModel):
+    """Uses the DataSortRole in the model to provide a comparison method to sort the data."""
+
     def lessThan(self, left: QtCore.QModelIndex, right: QtCore.QModelIndex):
-        leftData = self.sourceModel().data(left, ChangeBranchDialogModel.DataSortRole)
-        rightData = self.sourceModel().data(right, ChangeBranchDialogModel.DataSortRole)
-        if leftData is None or rightData is None:
-            if rightData is not None:
-                return True
-            else:
-                return False
-        return leftData < rightData
+        """Compare two git refs according to the DataSortRole in the model."""
+        left_data = self.sourceModel().data(left, ChangeBranchDialogModel.DataSortRole)
+        right_data = self.sourceModel().data(right, ChangeBranchDialogModel.DataSortRole)
+        if left_data is None or right_data is None:
+            return right_data is not None
+        return left_data < right_data