From 051d86ec938cc7c354fd4759ff8013f6891229d2 Mon Sep 17 00:00:00 2001 From: Chris Hennes Date: Mon, 31 Aug 2026 10:11:41 -0500 Subject: [PATCH] Fix git installation bugs and UX --- AddonManagerTest/app/test_installer.py | 39 +++++-- AddonManagerTest/gui/test_installer_gui.py | 128 +++++++++++++++++---- addonmanager_installer.py | 38 +++--- addonmanager_installer_gui.py | 76 +++++++++--- 4 files changed, 220 insertions(+), 61 deletions(-) diff --git a/AddonManagerTest/app/test_installer.py b/AddonManagerTest/app/test_installer.py index 53f69b06..95ae1cfc 100644 --- a/AddonManagerTest/app/test_installer.py +++ b/AddonManagerTest/app/test_installer.py @@ -283,14 +283,24 @@ def test_will_use_git_for_a_normal_addon(self): self.assertFalse(installer.will_use_git()) + def test_stored_install_method_answers_for_a_signal_started_run(self): + """A run started through a Qt signal cannot be passed the installation method, so it is + stored on the installer beforehand, and questions about the run use the stored value.""" + self.real_addon.prefer_git = True + installer = AddonInstaller(self.real_addon, []) + installer.git_manager = MockGitManager() + self.assertTrue(installer.will_use_git()) + + installer.install_method = InstallationMethod.ZIP + + self.assertFalse(installer.will_use_git()) + def test_report_git_progress(self): - """Each line of git's progress report is passed on as git worded it, with the percentage - it contains, so that a long clone can drive a progress bar.""" + """Each line of git's progress report is passed on as git worded it. Git works in stages, + each counting its own percentage up to 100, so no single progress figure is extracted.""" installer = AddonInstaller(self.real_addon, []) reported = [] - installer.progress_message.connect( - lambda message, percent: reported.append((message, percent)) - ) + installer.progress_message.connect(reported.append) installer._report_git_progress("Receiving objects: 42% (5218/12345), 120.50 MiB\n") installer._report_git_progress("Cloning into 'FreeCAD-library'...\n") @@ -298,12 +308,27 @@ def test_report_git_progress(self): self.assertEqual( [ - ("Receiving objects: 42% (5218/12345), 120.50 MiB", 42), - ("Cloning into 'FreeCAD-library'...", -1), + "Receiving objects: 42% (5218/12345), 120.50 MiB", + "Cloning into 'FreeCAD-library'...", ], reported, ) + def test_unknown_zip_download_size_is_normalized(self): + """Qt reports an unknown download size as -1, but the progress_update signal documents + zero as the unknown-size marker, so that is what listeners receive.""" + installer = AddonInstaller(self.real_addon, []) + installer.zip_download_index = 7 + reported = [] + installer.progress_update.connect( + lambda bytes_read, size: reported.append((bytes_read, size)) + ) + + installer._update_zip_status(7, 150_000_000, -1) + installer._update_zip_status(8, 1, 100) # A different download's report, not ours + + self.assertEqual([(150_000_000, 0)], reported) + def test_determine_install_method_for_a_large_addon(self): """An Addon that is too large to cache in full is installed with git, when git is available, so that later updates only have to fetch what changed.""" diff --git a/AddonManagerTest/gui/test_installer_gui.py b/AddonManagerTest/gui/test_installer_gui.py index 12a53573..0e1c3450 100644 --- a/AddonManagerTest/gui/test_installer_gui.py +++ b/AddonManagerTest/gui/test_installer_gui.py @@ -92,7 +92,7 @@ def moveToThread(self, thread): class MockInstaller(QtCore.QObject): progress_update = QtCore.Signal(int, int) - progress_message = QtCore.Signal(str, int) + progress_message = QtCore.Signal(str) success = QtCore.Signal(object) failure = QtCore.Signal(object, str) finished = QtCore.Signal() @@ -164,7 +164,7 @@ def test_dialog_says_when_git_is_being_used(self): """Installing with git is slower than downloading a zip, so the dialog explains why it is worth the wait.""" gui = AddonInstallerGUI(Addon("Test Addon")) - gui.installer.will_use_git = lambda: True + gui.installer.will_use_git = lambda *args: True gui.create_installing_dialog() self.addCleanup(gui.installing_dialog.close) @@ -173,7 +173,7 @@ def test_dialog_says_when_git_is_being_used(self): def test_dialog_does_not_mention_git_for_a_zip_install(self): gui = AddonInstallerGUI(Addon("Test Addon")) - gui.installer.will_use_git = lambda: False + gui.installer.will_use_git = lambda *args: False gui.create_installing_dialog() self.addCleanup(gui.installing_dialog.close) @@ -183,7 +183,7 @@ def test_a_failed_git_installation_can_be_tried_another_way(self): """Cloning a large Addon fails for reasons a second attempt gets past, so the failure is not the end of the conversation.""" gui = AddonInstallerGUI(Addon("Test Addon")) - gui.installer.will_use_git = lambda: True + gui.installer.will_use_git = lambda *args: True gui.installation_method = InstallationMethod.ANY @@ -192,14 +192,14 @@ def test_a_failed_git_installation_can_be_tried_another_way(self): def test_a_failed_zip_installation_is_only_reported(self): """The zip download is the fallback, so there is nothing left to fall back to.""" gui = AddonInstallerGUI(Addon("Test Addon")) - gui.installer.will_use_git = lambda: True + gui.installer.will_use_git = lambda *args: True gui.installation_method = InstallationMethod.ZIP self.assertFalse(gui._can_try_another_way()) def test_a_failed_installation_that_did_not_use_git_is_only_reported(self): gui = AddonInstallerGUI(Addon("Test Addon")) - gui.installer.will_use_git = lambda: False + gui.installer.will_use_git = lambda *args: False self.assertFalse(gui._can_try_another_way()) @@ -266,6 +266,7 @@ def test_progress_update_shows_how_much_has_been_downloaded(self): gui = self._installer_gui_with_dialog() gui._progress_update(150_000_000, 2_100_000_000) + gui._apply_pending_detail() # Qt formats the sizes themselves, in the units and the notation of the user's locale locale = QtCore.QLocale() @@ -276,36 +277,123 @@ def test_progress_update_shows_how_much_has_been_downloaded(self): self.assertEqual(150_000_000, gui.installing_dialog.progressBar.value()) def test_progress_update_of_an_unknown_download_size(self): - """When the server does not say how large the download is, the amount received so far is - still shown.""" + """When the server does not say how large the download is, a bar cannot show honest + progress: it is hidden, and the amount received so far is shown with an explanation of + why there is no total.""" gui = self._installer_gui_with_dialog() gui._progress_update(150_000_000, 0) + gui._apply_pending_detail() received = QtCore.QLocale().formattedDataSize(150_000_000) self.assertIn(received, gui.installing_dialog.label.text()) - self.assertNotIn(" of ", gui.installing_dialog.label.text()) + self.assertIn("unknown total", gui.installing_dialog.label.text()) + self.assertTrue(gui.installing_dialog.progressBar.isHidden()) + + def test_progress_update_of_a_known_download_size_keeps_the_bar(self): + gui = self._installer_gui_with_dialog() + + gui._progress_update(150_000_000, 2_100_000_000) + + self.assertFalse(gui.installing_dialog.progressBar.isHidden()) def test_progress_message_shows_what_git_is_doing(self): - """A git clone reports its progress as text, which is shown as git worded it, with its - percentage driving the bar.""" + """A git clone reports its progress as text, which is shown as git worded it.""" gui = self._installer_gui_with_dialog() - gui._progress_message("Receiving objects: 42% (5218/12345)", 42) + gui._progress_message("Receiving objects: 42% (5218/12345)") + gui._apply_pending_detail() self.assertIn("Receiving objects", gui.installing_dialog.label.text()) - self.assertEqual(100, gui.installing_dialog.progressBar.maximum()) - self.assertEqual(42, gui.installing_dialog.progressBar.value()) - def test_progress_message_without_a_percentage(self): - """A git report with no percentage in it leaves the bar alone rather than resetting it.""" + def test_progress_reports_are_coalesced(self): + """Reports can arrive faster than a label can be re-laid-out, so an arrival only records + what to show and a timer applies the newest recorded report a few times per second.""" + gui = self._installer_gui_with_dialog() + label_before = gui.installing_dialog.label.text() + + gui._progress_message("Receiving objects: 41% (5100/12345)") + gui._progress_message("Receiving objects: 42% (5218/12345)") + + self.assertEqual(label_before, gui.installing_dialog.label.text()) + self.assertTrue(gui.detail_update_timer.isActive()) + gui._apply_pending_detail() + self.assertIn("42%", gui.installing_dialog.label.text()) + + def test_no_progress_bar_when_git_reports_progress(self): + """Git works in stages, each counting its own percentage up to 100: a bar following those + numbers would fill and reset several times, so no bar is shown at all.""" + gui = AddonInstallerGUI(Addon("Test Addon")) + gui.installer.will_use_git = lambda *args: True + gui.create_installing_dialog() + self.addCleanup(gui.installing_dialog.close) + + self.assertTrue(gui.installing_dialog.progressBar.isHidden()) + + def test_progress_bar_is_kept_for_a_zip_download(self): + """A zip download reports how many bytes have arrived, which a bar can show honestly.""" + gui = self._installer_gui_with_dialog() + + self.assertFalse(gui.installing_dialog.progressBar.isHidden()) + + def test_detail_line_does_not_change_the_dialog_size(self): + """The dialog is laid out with room for the detail line from the start: growing the label + after the dialog is shown does not reliably make the dialog taller.""" gui = self._installer_gui_with_dialog() - gui._progress_message("Receiving objects: 42% (5218/12345)", 42) + height_before = gui.installing_dialog.sizeHint().height() - gui._progress_message("Resolving deltas", -1) + gui._progress_message("Receiving objects: 42% (5218/12345), 120.50 MiB | 5.20 MiB/s") + gui._apply_pending_detail() + + self.assertEqual(height_before, gui.installing_dialog.sizeHint().height()) + + def test_install_stores_the_method_on_the_installer(self): + """The worker thread starts run() through a signal, which cannot carry the method + argument, so install() stores the method on the installer and connects run directly: + wrapping the connection in a partial or lambda to pass the argument makes PySide run the + whole installation on the GUI thread, freezing the interface.""" + gui = AddonInstallerGUI(Addon("Test Addon")) + with patch.object(QtCore.QThread, "start"): + gui.install(InstallationMethod.ZIP) + self.addCleanup(gui.installing_dialog.close) + self.addCleanup(gui.shutdown) + + self.assertEqual(InstallationMethod.ZIP, gui.installer.install_method) + + def test_suggests_git_for_a_large_addon_when_git_is_missing(self): + """Without git, a large Addon is downloaded in full both now and for every update, so the + dialog mentions that installing git would improve matters.""" + addon = Addon("Test Addon") + addon.prefer_git = True + gui = AddonInstallerGUI(addon) + gui.installer.git_manager = None + gui.create_installing_dialog() + self.addCleanup(gui.installing_dialog.close) + + note = gui.installing_dialog.findChild(QtWidgets.QLabel, "gitSuggestionLabel") + self.assertIsNotNone(note, "No git suggestion was added to the dialog") + self.assertIn("git", note.text()) + + def test_no_git_suggestion_when_git_is_available(self): + addon = Addon("Test Addon") + addon.prefer_git = True + gui = AddonInstallerGUI(addon) + gui.installer.git_manager = MagicMock() + gui.installer.will_use_git = lambda *args: True + gui.create_installing_dialog() + self.addCleanup(gui.installing_dialog.close) + + self.assertIsNone(gui.installing_dialog.findChild(QtWidgets.QLabel, "gitSuggestionLabel")) + + def test_no_git_suggestion_for_an_ordinary_addon(self): + """An Addon that is not flagged as large downloads quickly as a zip: there is nothing to + suggest git for.""" + gui = AddonInstallerGUI(Addon("Test Addon")) + gui.installer.git_manager = None + gui.create_installing_dialog() + self.addCleanup(gui.installing_dialog.close) - self.assertIn("Resolving deltas", gui.installing_dialog.label.text()) - self.assertEqual(42, gui.installing_dialog.progressBar.value()) + self.assertIsNone(gui.installing_dialog.findChild(QtWidgets.QLabel, "gitSuggestionLabel")) @patch("addonmanager_installer_gui.AddonDependencyInstallerGUI") @patch("addonmanager_installer_gui.MissingDependencies") diff --git a/addonmanager_installer.py b/addonmanager_installer.py index 60c48683..9cd1cac9 100644 --- a/addonmanager_installer.py +++ b/addonmanager_installer.py @@ -27,7 +27,6 @@ from datetime import datetime, timezone from enum import IntEnum, auto import os -import re import shutil from typing import List, Optional import tempfile @@ -121,9 +120,8 @@ class AddonInstaller(QtCore.QObject): # Signal: progress_message # In GUI mode this signal is emitted during an installation whose progress is reported as text # rather than as a byte count, which is how git reports what it is doing. The string is a - # human-readable description of the work in progress, and the integer is how far through that - # work we are, as a percentage, or -1 when the report did not include one. - progress_message = QtCore.Signal(str, int) + # human-readable description of the work in progress. + progress_message = QtCore.Signal(str) # Signals: success and failure # Emitted when the installation process is complete. The object emitted is the object that the @@ -161,13 +159,19 @@ def __init__(self, addon: Addon, allow_list: List[str] = None): self.macro_installation_path = fci.DataPaths().macro_dir self.zip_download_index = None - def run(self, install_method: InstallationMethod = InstallationMethod.ANY) -> bool: + # The method the next run() uses when run() is given no argument. + self.install_method = InstallationMethod.ANY + + def run(self, install_method: Optional[InstallationMethod] = None) -> bool: """Install an addon. Returns True if the addon was installed, or False if not. Emits - either success or failure prior to returning.""" + either success or failure prior to returning. The installation method may be passed in + here, or set on install_method beforehand by callers that start the run via a signal.""" + if install_method is not None: + self.install_method = install_method success = False try: addon_url = self.addon_to_install.url.replace(os.path.sep, "/") - method_to_use = self._determine_install_method(addon_url, install_method) + method_to_use = self._determine_install_method(addon_url, self.install_method) fci.Console.PrintMessage( f"Installing addon {self.addon_to_install.name} using {method_to_use}\n" ) @@ -189,6 +193,7 @@ def run(self, install_method: InstallationMethod = InstallationMethod.ANY) -> bo pass except Exception as e: fci.Console.PrintLog(str(e) + "\n") + self.failure.emit(self.addon_to_install, str(e)) success = False if success: if ( @@ -201,12 +206,13 @@ def run(self, install_method: InstallationMethod = InstallationMethod.ANY) -> bo self.finished.emit() return success - def will_use_git(self, install_method: InstallationMethod = InstallationMethod.ANY) -> bool: + def will_use_git(self, install_method: Optional[InstallationMethod] = None) -> bool: """Whether running this installer will use git, so that callers can say so before the - installation starts.""" + installation starts. With no argument, answers for the stored install_method.""" + method = install_method if install_method is not None else self.install_method addon_url = self.addon_to_install.url.replace(os.path.sep, "/") - return self._determine_install_method(addon_url, install_method) == InstallationMethod.GIT + return self._determine_install_method(addon_url, method) == InstallationMethod.GIT def _determine_install_method( self, addon_url: str, install_method: InstallationMethod @@ -317,14 +323,10 @@ def _install_by_git(self) -> bool: return True def _report_git_progress(self, line: str) -> None: - """Pass a line of git's progress report on to whatever is displaying it. This is basically - all we can do to not appear stalled out when using git to install, there's no way of giving - a "real" progress bar. The percentage is sort of a lie here, but it's all we've got.""" + """Pass a line of git's progress report on to whatever is displaying it.""" line = line.strip() - if not line: - return - percentage = re.search(r"(\d{1,3})%", line) - self.progress_message.emit(line, int(percentage.group(1)) if percentage else -1) + if line: + self.progress_message.emit(line) def _install_by_zip(self) -> bool: """Installs the specified url by downloading the file (if it is remote) and unzipping it @@ -366,7 +368,7 @@ def _update_zip_status(self, index: int, bytes_read: int, data_size: int): """Called periodically when downloading a zip file, emits a signal to display the download progress.""" if index == self.zip_download_index: - self.progress_update.emit(bytes_read, data_size) + self.progress_update.emit(bytes_read, max(data_size, 0)) def _finish_zip(self, index: int, response_code: int, filename: str): """Once the zip download is finished, unzip it into the correct location. Only called if diff --git a/addonmanager_installer_gui.py b/addonmanager_installer_gui.py index 2bdade92..0f3e08d2 100644 --- a/addonmanager_installer_gui.py +++ b/addonmanager_installer_gui.py @@ -25,7 +25,6 @@ import os import sys -from functools import partial from typing import List import addonmanager_freecad_interface as fci @@ -87,6 +86,8 @@ def __init__(self, addon: Addon, addons: List[Addon] = None): self.cancelling_dialog = None self.installation_message = "" self.installation_method = InstallationMethod.ANY + self.pending_detail = None + self.detail_update_timer = None self.worker_thread = None # Set up the installer connections @@ -141,10 +142,12 @@ def install(self, install_method: InstallationMethod = InstallationMethod.ANY) - self.installer.finished.connect(self.worker_thread.quit) self.installer.progress_update.connect(self._progress_update) self.installer.progress_message.connect(self._progress_message) - self.worker_thread.started.connect(partial(self.installer.run, install_method)) + self.installer.install_method = install_method + self.worker_thread.started.connect(self.installer.run) self.create_installing_dialog() self.installer.finished.connect(self.installing_dialog.hide) + self.installer.finished.connect(self.detail_update_timer.stop) self.installing_dialog.show() self.worker_thread.start() # Returns immediately @@ -159,11 +162,12 @@ def create_installing_dialog(self) -> None: self.installing_dialog = fci.loadUi(os.path.join(os.path.dirname(__file__), "progress.ui")) self.installing_dialog.setObjectName("AddonManager_InstallingDialog") name = self.addon_to_install.display_name + uses_git = self.installer.will_use_git(self.installation_method) if self._is_an_update(): self.installing_dialog.setWindowTitle( translate("AddonsInstaller", "Updating Addon", "Window title") ) - if self.installer.will_use_git(): + if uses_git: self.installation_message = translate( "AddonsInstaller", "Updating '{}' with git, so only the changes are downloaded" ).format(name) @@ -175,7 +179,7 @@ def create_installing_dialog(self) -> None: self.installing_dialog.setWindowTitle( translate("AddonsInstaller", "Installing Addon", "Window title") ) - if self.installer.will_use_git(): + if uses_git: self.installation_message = translate( "AddonsInstaller", "Installing '{}' with git (for more efficient updating)" ).format(name) @@ -183,33 +187,71 @@ def create_installing_dialog(self) -> None: self.installation_message = translate("AddonsInstaller", "Installing '{}'").format( name ) - self.installing_dialog.label.setText(self.installation_message) # Git's progress reports are long: give the label enough room to show both the activity # one names and the transfer rate it ends with self.installing_dialog.label.setMinimumWidth(560) - self.installing_dialog.progressBar.setRange(0, 0) # Start in indeterminate mode + # The label carries the detail line from the start, so the dialog is laid out with room + # for it: making the label taller once the dialog is shown does not reliably make the + # dialog taller + self._set_installation_detail("") + self.pending_detail = None + self.detail_update_timer = QtCore.QTimer(self) + self.detail_update_timer.setInterval(100) + self.detail_update_timer.timeout.connect(self._apply_pending_detail) + self.detail_update_timer.start() + if uses_git: + self.installing_dialog.progressBar.hide() + else: + self.installing_dialog.progressBar.setRange(0, 0) # Start in indeterminate mode + if utils.should_use_git(self.addon_to_install) and self.installer.git_manager is None: + self._suggest_installing_git() self.installing_dialog.rejected.connect(self._cancel_addon_installation) def _progress_update(self, bytes_read: int, data_size: int) -> None: """Show how much of a download has arrived. A data_size of zero means the server did not - say how large the download is, so only the amount received so far can be shown.""" - self.installing_dialog.progressBar.setMaximum(data_size) - self.installing_dialog.progressBar.setValue(bytes_read) + say how large the download is: a bar cannot show honest progress then, so it is hidden + and the amount received so far is shown as text instead.""" locale = QtCore.QLocale() if data_size > 0: + self.installing_dialog.progressBar.setMaximum(data_size) + self.installing_dialog.progressBar.setValue(bytes_read) amount = translate("AddonsInstaller", "{} of {}").format( locale.formattedDataSize(bytes_read), locale.formattedDataSize(data_size) ) else: - amount = locale.formattedDataSize(bytes_read) - self._set_installation_detail(amount) + self.installing_dialog.progressBar.hide() + amount = translate("AddonsInstaller", "{} of an unknown total download size").format( + locale.formattedDataSize(bytes_read) + ) + self.pending_detail = amount - def _progress_message(self, message: str, percentage: int) -> None: + def _progress_message(self, message: str) -> None: """Show what an installation that reports its progress as text, as git does, is doing.""" - if percentage >= 0: - self.installing_dialog.progressBar.setMaximum(100) - self.installing_dialog.progressBar.setValue(percentage) - self._set_installation_detail(message) + self.pending_detail = message + + def _apply_pending_detail(self) -> None: + """Show the newest recorded progress report, if one arrived since the last application.""" + if self.pending_detail is None: + return + detail = self.pending_detail + self.pending_detail = None + self._set_installation_detail(detail) + + def _suggest_installing_git(self) -> None: + """Add a note to the installing dialog suggesting git for an Addon that the catalog flags + as large.""" + note = QtWidgets.QLabel( + translate( + "AddonsInstaller", + "Note: this large addon must be downloaded in full, both now and for each future " + "update. If git is installed, the Addon Manager uses it to download only what " + "changed instead.", + ) + ) + note.setObjectName("gitSuggestionLabel") + note.setWordWrap(True) + layout = self.installing_dialog.layout() + layout.insertWidget(layout.indexOf(self.installing_dialog.buttonBox), note) def _set_installation_detail(self, detail: str) -> None: """Show what the installation is doing right now, on a line of its own below the name of @@ -248,6 +290,8 @@ def create_cancelling_dialog(self) -> None: ) def _cancel_addon_installation(self): + if self.detail_update_timer is not None: + self.detail_update_timer.stop() self.create_cancelling_dialog() self.cancelling_dialog.show() QtCore.QCoreApplication.processEvents(QtCore.QEventLoop.AllEvents)