Fix git installation bugs and UX

AddonManagerTest/app/test_installer.py

@@ -283,27 +283,52 @@ 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")
         installer._report_git_progress("   \n")
 
         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."""

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")

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