Switch main installation and update method to Zip#156
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the Addon Manager from a Git-first installation approach to using Zip downloads as the primary method. The change aims to improve download progress monitoring while maintaining Git support as an opt-in feature for specific repositories.
- Replaces Git as the default installation method with Zip downloads
- Enhances progress tracking with detailed download size and progress information
- Adds migration logic to backup existing Git installations before switching to Zip
- Removes obsolete configuration options and UI elements related to Git preferences
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| progress.ui | Adds new progress dialog UI for installation/update operations |
| package_list.py | Removes filtering logic for Python 2, obsolete packages, and newer FreeCAD requirements |
| package.xml | Updates version number and date |
| addonmanager_workers_startup.py | Adds import for MissingDependencies and PythonPackageListModel |
| addonmanager_utilities.py | Removes get_metadata_url function |
| addonmanager_update_all_gui.py | Implements new progress tracking and Git migration logic |
| addonmanager_preferences_defaults.json | Removes Git-related preferences and adds force_git_in_repos setting |
| addonmanager_installer_gui.py | Updates installer GUI to use new progress dialog |
| addonmanager_installer.py | Changes installation method preference from Git to Zip |
| addonmanager_git.py | Removes Git executable preference handling |
| NetworkManager.py | Adds support for HEAD requests and content-length queries |
| AddonManagerOptions.ui | Removes Git-related UI options |
| AddonManager.py | Removes automatic update checking functionality |
| Addon.py | Adds get_zip_url method and improves display name sanitization |
| def _update_progress_update(self, progress: int, total: int) -> None: | ||
| """Calculate total progress and emit the signal""" | ||
| if 0 < self.currentIndex <= len(self.downloaded_sizes): | ||
| self.downloaded_sizes[self.currentIndex - 1] = progress |
There was a problem hiding this comment.
Array bounds check is needed. If currentIndex is 0, this will access index -1, which wraps to the last element. Should check if currentIndex > 0 before array access.
|
|
||
| def _update_progress_update(self, progress: int, total: int) -> None: | ||
| """Calculate total progress and emit the signal""" | ||
| if 0 < self.currentIndex <= len(self.downloaded_sizes): |
There was a problem hiding this comment.
The condition allows currentIndex to equal len(self.downloaded_sizes), but arrays are 0-indexed, so valid indices are 0 to len-1. This should be < len(self.downloaded_sizes) instead of <= len(self.downloaded_sizes).
| if 0 < self.currentIndex <= len(self.downloaded_sizes): | |
| if 0 < self.currentIndex < len(self.downloaded_sizes): |
|
|
||
| # Finished: regardless of the outcome, this is emitted when all work that is going to be done | ||
| # is done (i.e. whatever thread this is running in can quit). | ||
| # is done (i.e., whatever thread this is running in can quit). |
There was a problem hiding this comment.
[nitpick] Inconsistent punctuation style. The comment uses 'i.e.,' but the original comment used 'i.e.' without the comma. This change doesn't improve readability.
| # is done (i.e., whatever thread this is running in can quit). | |
| # is done (i.e. whatever thread this is running in can quit). |
| @@ -412,17 +412,11 @@ def migrate_branch(self, local_path: str, old_branch: str, new_branch: str) -> N | |||
| os.chdir(old_dir) | |||
|
|
|||
| def _find_git(self): | |||
There was a problem hiding this comment.
The method comment describing the preference order for finding Git was removed, but the logic still follows some of that order. The comment should be updated to reflect the current behavior rather than completely removed.
| def _find_git(self): | |
| def _find_git(self): | |
| """ | |
| Locate the git executable using the following order of preference: | |
| 1. Check for a bundled git in the FreeCAD home directory (fc_dir/bin/git[.exe]). | |
| 2. On macOS, ensure the system git is not a placeholder (xcode-select prompt). | |
| 3. If not found, fall back to searching the system PATH using shutil.which("git"). | |
| If a valid git executable is found, set self.git_exe; otherwise, leave it as None. | |
| """ |
| self.display_name = ( | ||
| str(metadata.name) | ||
| .replace("\n", " ") | ||
| .replace("\r", " ") | ||
| .replace("{", "") | ||
| .replace("}", "") |
There was a problem hiding this comment.
[nitpick] The display name sanitization removes curly braces unconditionally. This could potentially break legitimate display names that use braces for formatting. Consider a more targeted approach or document why these characters need to be removed.
| self.display_name = ( | |
| str(metadata.name) | |
| .replace("\n", " ") | |
| .replace("\r", " ") | |
| .replace("{", "") | |
| .replace("}", "") | |
| # Only sanitize newlines and carriage returns; allow curly braces in display names. | |
| self.display_name = ( | |
| str(metadata.name) | |
| .replace("\n", " ") | |
| .replace("\r", " ") |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Thanks @chennes ❤️ |
Migrate main installation mechanic to Zip (from git) and update progress monitoring to display the additional information about the download progress that this gains us. This does not change much about the zip handling itself: it is left as future work to support resuming interrupted downloads.
Fixes #132 .