From d4ea028edfd28af3383afea6470f034b2374fe25 Mon Sep 17 00:00:00 2001 From: Chris Hennes Date: Thu, 6 Feb 2025 11:03:00 -0600 Subject: [PATCH 1/4] Addon Manager: improve git branch changing --- .../gui/test_change_branch.py | 0 .../addonmanager_freecad_interface.py | 4 +- .../AddonManager/addonmanager_utilities.py | 15 ++++ src/Mod/AddonManager/change_branch.py | 84 ++++++++++++++----- 4 files changed, 83 insertions(+), 20 deletions(-) create mode 100644 src/Mod/AddonManager/AddonManagerTest/gui/test_change_branch.py diff --git a/src/Mod/AddonManager/AddonManagerTest/gui/test_change_branch.py b/src/Mod/AddonManager/AddonManagerTest/gui/test_change_branch.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/src/Mod/AddonManager/addonmanager_freecad_interface.py b/src/Mod/AddonManager/addonmanager_freecad_interface.py index ac66a100ca..42839f6802 100644 --- a/src/Mod/AddonManager/addonmanager_freecad_interface.py +++ b/src/Mod/AddonManager/addonmanager_freecad_interface.py @@ -49,6 +49,8 @@ try: if FreeCAD.GuiUp: import FreeCADGui + + loadUi = FreeCADGui.PySideUic.loadUi else: FreeCADGui = None @@ -63,7 +65,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 diff --git a/src/Mod/AddonManager/addonmanager_utilities.py b/src/Mod/AddonManager/addonmanager_utilities.py index 5a95d60229..2d878e1676 100644 --- a/src/Mod/AddonManager/addonmanager_utilities.py +++ b/src/Mod/AddonManager/addonmanager_utilities.py @@ -69,6 +69,21 @@ else: import urllib.request import ssl +if fci.FreeCADGui: + loadUi = fci.loadUi +else: + try: + from PySide6.QtUiTools import QUiLoader + except ImportError: + from PySide2.QtUiTools import QUiLoader + + def loadUi(ui_file: str) -> QtWidgets.QWidget: + 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 diff --git a/src/Mod/AddonManager/change_branch.py b/src/Mod/AddonManager/change_branch.py index 3fb73358de..478a012378 100644 --- a/src/Mod/AddonManager/change_branch.py +++ b/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. * # * * @@ -22,14 +22,22 @@ # *************************************************************************** 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 + +translate = fci.translate class ChangeBranchDialog(QtWidgets.QWidget): @@ -39,9 +47,7 @@ class ChangeBranchDialog(QtWidgets.QWidget): 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 +60,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() @@ -111,14 +120,51 @@ 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): @@ -235,11 +281,11 @@ class ChangeBranchDialogModel(QtCore.QAbstractTableModel): class ChangeBranchDialogFilter(QtCore.QSortFilterProxyModel): 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: + 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: + if right_data is not None: return True else: return False - return leftData < rightData + return left_data < right_data From 2dd2381c707529583c3c4403b5dddc52c3c897f8 Mon Sep 17 00:00:00 2001 From: Chris Hennes Date: Thu, 6 Feb 2025 11:03:38 -0600 Subject: [PATCH 2/4] Addon Manager: Add GUI tests for branch change dialog Further protect utils gui code during testing so the same utils file can be used for CLI and GUI tests. --- .../gui/test_change_branch.py | 214 ++++++++++++++++++ .../addonmanager_freecad_interface.py | 4 +- .../AddonManager/addonmanager_utilities.py | 26 ++- 3 files changed, 236 insertions(+), 8 deletions(-) diff --git a/src/Mod/AddonManager/AddonManagerTest/gui/test_change_branch.py b/src/Mod/AddonManager/AddonManagerTest/gui/test_change_branch.py index e69de29bb2..da53828329 100644 --- a/src/Mod/AddonManager/AddonManagerTest/gui/test_change_branch.py +++ b/src/Mod/AddonManager/AddonManagerTest/gui/test_change_branch.py @@ -0,0 +1,214 @@ +# 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 * +# * . * +# * * +# *************************************************************************** + +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, DialogInteractor, 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): + def mapToSource(self, something): + return something + + +class MockChangeBranchDialogModel(QtCore.QAbstractTableModel): + + 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: + if parent.isValid(): + return 0 + return len(self.branches) + + def columnCount(self, parent: QtCore.QModelIndex = QtCore.QModelIndex()) -> int: + if parent.isValid(): + return 0 + return 3 # Local name, remote name, date + + def data(self, index: QtCore.QModelIndex, role: int = QtCore.Qt.DisplayRole): + if not index.isValid(): + return None + row = index.row() + column = index.column() + if role == QtCore.Qt.DisplayRole: + if column == 2: + return "date" + elif column == 0: + return "ref_name" + elif column == 1: + return "upstream" + else: + return None + elif role == MockChangeBranchDialogModel.DataSortRole: + return None + elif role == MockChangeBranchDialogModel.RefAccessRole: + return self.branches[row] + + def headerData( + self, + section: int, + orientation: QtCore.Qt.Orientation, + role: int = QtCore.Qt.DisplayRole, + ): + 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" + elif section == 2: + return "Last Updated" + else: + return None + + def currentBranch(self) -> str: + return self.current_branch + + +class TestChangeBranchGui(unittest.TestCase): + + 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): + # 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): + # 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): + # 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, + ) + + # Act + gui.exec() + + # Assert + self.assertTrue(dialog_watcher.dialog_found, "Failed to find the expected dialog box") + + +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 diff --git a/src/Mod/AddonManager/addonmanager_freecad_interface.py b/src/Mod/AddonManager/addonmanager_freecad_interface.py index 42839f6802..5a087a4efe 100644 --- a/src/Mod/AddonManager/addonmanager_freecad_interface.py +++ b/src/Mod/AddonManager/addonmanager_freecad_interface.py @@ -46,11 +46,13 @@ try: getUserMacroDir = FreeCAD.getUserMacroDir getUserCachePath = FreeCAD.getUserCachePath translate = FreeCAD.Qt.translate + loadUi = None if FreeCAD.GuiUp: import FreeCADGui - loadUi = FreeCADGui.PySideUic.loadUi + if hasattr(FreeCADGui, "PySideUic"): + loadUi = FreeCADGui.PySideUic.loadUi else: FreeCADGui = None diff --git a/src/Mod/AddonManager/addonmanager_utilities.py b/src/Mod/AddonManager/addonmanager_utilities.py index 2d878e1676..c7405379b0 100644 --- a/src/Mod/AddonManager/addonmanager_utilities.py +++ b/src/Mod/AddonManager/addonmanager_utilities.py @@ -72,16 +72,28 @@ else: if fci.FreeCADGui: loadUi = fci.loadUi else: + has_loader = False try: from PySide6.QtUiTools import QUiLoader - except ImportError: - from PySide2.QtUiTools import QUiLoader - def loadUi(ui_file: str) -> QtWidgets.QWidget: - q_ui_file = QtCore.QFile(ui_file) - q_ui_file.open(QtCore.QFile.OpenModeFlag.ReadOnly) - loader = QUiLoader() - return loader.load(ui_file) + has_loader = True + except ImportError: + try: + from PySide2.QtUiTools import QUiLoader + + has_loader = True + except ImportError: + + def loadUi(ui_file: str): + raise RuntimeError("Cannot use QUiLoader without PySide or FreeCAD") + + if has_loader: + + def loadUi(ui_file: str) -> QtWidgets.QWidget: + 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 From 5dbc5c95ca0202e9ff69f381b3b31776e2b66f68 Mon Sep 17 00:00:00 2001 From: Chris Hennes Date: Thu, 6 Feb 2025 17:55:06 -0600 Subject: [PATCH 3/4] Addon Manager: Linter cleanup --- .../gui/test_change_branch.py | 50 ++++++--- .../AddonManager/addonmanager_utilities.py | 51 ++++++--- src/Mod/AddonManager/change_branch.py | 103 +++++++++++------- 3 files changed, 133 insertions(+), 71 deletions(-) diff --git a/src/Mod/AddonManager/AddonManagerTest/gui/test_change_branch.py b/src/Mod/AddonManager/AddonManagerTest/gui/test_change_branch.py index da53828329..b70689d0c4 100644 --- a/src/Mod/AddonManager/AddonManagerTest/gui/test_change_branch.py +++ b/src/Mod/AddonManager/AddonManagerTest/gui/test_change_branch.py @@ -21,6 +21,10 @@ # * * # *************************************************************************** +"""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 @@ -28,7 +32,7 @@ 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, DialogInteractor, AsynchronousMonitor +from AddonManagerTest.gui.gui_mocks import DialogWatcher, AsynchronousMonitor from change_branch import ChangeBranchDialog @@ -45,11 +49,14 @@ except ImportError: 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"}, @@ -64,16 +71,20 @@ class MockChangeBranchDialogModel(QtCore.QAbstractTableModel): 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() @@ -81,16 +92,16 @@ class MockChangeBranchDialogModel(QtCore.QAbstractTableModel): if role == QtCore.Qt.DisplayRole: if column == 2: return "date" - elif column == 0: + if column == 0: return "ref_name" - elif column == 1: + if column == 1: return "upstream" - else: - return None - elif role == MockChangeBranchDialogModel.DataSortRole: return None - elif role == MockChangeBranchDialogModel.RefAccessRole: + if role == MockChangeBranchDialogModel.DataSortRole: + return None + if role == MockChangeBranchDialogModel.RefAccessRole: return self.branches[row] + return None def headerData( self, @@ -98,6 +109,7 @@ class MockChangeBranchDialogModel(QtCore.QAbstractTableModel): 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: @@ -106,16 +118,18 @@ class MockChangeBranchDialogModel(QtCore.QAbstractTableModel): return "Local" if section == 1: return "Remote tracking" - elif section == 2: + if section == 2: return "Last Updated" - else: - return None + 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 @@ -128,6 +142,7 @@ class TestChangeBranchGui(unittest.TestCase): @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"} @@ -137,7 +152,7 @@ class TestChangeBranchGui(unittest.TestCase): ) # Act - gui._change_branch("/foo/bar/baz", ref) + gui.change_branch("/foo/bar/baz", ref) # Assert self.assertTrue(dialog_watcher.dialog_found, "Failed to find the expected dialog box") @@ -145,6 +160,7 @@ class TestChangeBranchGui(unittest.TestCase): @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() @@ -158,7 +174,7 @@ class TestChangeBranchGui(unittest.TestCase): ) # Act - gui._change_branch("/foo/bar/baz", ref) + gui.change_branch("/foo/bar/baz", ref) # Assert self.assertTrue(dialog_watcher.dialog_found, "Failed to find the expected dialog box") @@ -166,8 +182,8 @@ class TestChangeBranchGui(unittest.TestCase): @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. + """If nothing gets thrown, then the process is assumed to have worked, and the appropriate + signal is emitted.""" # Arrange gui = ChangeBranchDialog("/some/path") @@ -175,7 +191,7 @@ class TestChangeBranchGui(unittest.TestCase): monitor = AsynchronousMonitor(gui.branch_changed) # Act - gui._change_branch("/foo/bar/baz", ref) + gui.change_branch("/foo/bar/baz", ref) # Assert monitor.wait_for_at_most(10) # Should be effectively instantaneous @@ -185,6 +201,8 @@ class TestChangeBranchGui(unittest.TestCase): @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() @@ -197,12 +215,14 @@ class TestChangeBranchGui(unittest.TestCase): 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__": diff --git a/src/Mod/AddonManager/addonmanager_utilities.py b/src/Mod/AddonManager/addonmanager_utilities.py index c7405379b0..1e7b0296b8 100644 --- a/src/Mod/AddonManager/addonmanager_utilities.py +++ b/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,9 +64,17 @@ 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 @@ -85,11 +96,14 @@ else: 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() @@ -135,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 @@ -213,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) @@ -395,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}" @@ -417,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: @@ -462,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() @@ -481,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( @@ -499,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": @@ -540,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 diff --git a/src/Mod/AddonManager/change_branch.py b/src/Mod/AddonManager/change_branch.py index 478a012378..1880ab95be 100644 --- a/src/Mod/AddonManager/change_branch.py +++ b/src/Mod/AddonManager/change_branch.py @@ -21,6 +21,8 @@ # * * # *************************************************************************** +"""The Change Branch dialog and utility classes and methods""" + import os from typing import Dict @@ -35,12 +37,15 @@ except ImportError: try: from PySide6 import QtWidgets, QtCore except ImportError: - from PySide2 import QtWidgets, QtCore + 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) @@ -80,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() @@ -120,9 +128,9 @@ class ChangeBranchDialog(QtWidgets.QWidget): if result == QtWidgets.QMessageBox.Cancel: return - self._change_branch(self.item_model.path, ref) + self.change_branch(self.item_model.path, ref) - def _change_branch(self, path: str, ref: Dict[str, str]) -> None: + 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"] @@ -168,6 +176,10 @@ class ChangeBranchDialog(QtWidgets.QWidget): 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 @@ -184,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, @@ -239,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: @@ -255,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 = [] @@ -280,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): + """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: - if right_data is not None: - return True - else: - return False + return right_data is not None return left_data < right_data From a3e436d211c33378db42a195dfd641f2b622863f Mon Sep 17 00:00:00 2001 From: Chris Hennes Date: Fri, 7 Feb 2025 10:17:16 -0600 Subject: [PATCH 4/4] Addon Manager: Update background UI earlier The modal confirmation dialog blocks, so update the UI before showing it. --- ...addonmanager_package_details_controller.py | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/Mod/AddonManager/addonmanager_package_details_controller.py b/src/Mod/AddonManager/addonmanager_package_details_controller.py index 9d84e8055d..ccd97a2e7f 100644 --- a/src/Mod/AddonManager/addonmanager_package_details_controller.py +++ b/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)