Implement multi-branch handling#165
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| self.fs.create_dir(os.path.join(mod_dir, "OrphanAddon")) | ||
|
|
||
| InstallationManifest.path_to_manifest_file = "" # Reset shared class var | ||
| manifest = InstallationManifest(catalog=None) |
Check notice
Code scanning / CodeQL
Unused local variable Note test
chennes
force-pushed
the
multiBranchHandling
branch
2 times, most recently
from
c89f8b8 to
e149dc8
Compare
chennes
force-pushed
the
multiBranchHandling
branch
from
e149dc8 to
391edf6
Compare
chennes
force-pushed
the
multiBranchHandling
branch
2 times, most recently
from
8b8a328 to
17dd760
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR implements multi-branch handling functionality to allow users to switch between different branches of addons (e.g., main vs development branches). The implementation includes a new installation manifest system to track which branch is currently installed and UI components to support branch switching.
Key changes:
- Added
InstallationManifestclass to track addon installations and branches - Enhanced catalog processing to support multiple branches per addon
- Modified UI components to display branch selection options
- Updated installation/uninstallation processes to maintain manifest state
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| addonmanager_installation_manifest.py | New module implementing installation tracking and manifest management |
| addonmanager_workers_startup.py | Enhanced catalog processing to handle multiple branches and integrate with manifest |
| Widgets/addonmanager_widget_addon_buttons.py | Modified button widget to support branch selection menu |
| addonmanager_package_details_controller.py | Updated controller to handle branch switching logic |
| Addon.py | Added branch display name support and sub-addon management |
| AddonCatalog.py | Modified to return branch display names instead of tuples |
| AddonManager.py | Added backup/restore functionality and branch cycling logic |
| package_list.py | Added remove_item method for model updates |
| NetworkManager.py | Fixed HEAD request implementation |
| composite_view.py | Removed debug print statements |
| addonmanager_installer.py | Updated to record installations in manifest |
| addonmanager_uninstaller.py | Updated to remove entries from manifest |
| addonmanager_update_all_gui.py | Added missing signal connection |
| package.xml | Version bump to 2025.08.08 |
| AddonManagerTest/* | Updated test files to mock new manifest functionality |
| ).isoformat(), | ||
| "branch_display_name": branch_display_name, | ||
| "extra_files": [], | ||
| "freecad_version": "", |
There was a problem hiding this comment.
The FreeCAD version is being set to an empty string during migration, but line 145 shows it should use fci.Version()[0:3] to get the actual version. This inconsistency means migrated addons won't have proper version tracking.
| "freecad_version": "", | |
| "freecad_version": fci.Version()[0:3], |
| f"Failed to load the addon {addon_id} from the addon catalog, skipping it.\n" | ||
| ) | ||
| fci.Console.PrintError(str(e) + "\n") | ||
| continue |
There was a problem hiding this comment.
This code attempts to access addon_instances[primary_branch_name] but primary_branch_name could be None if installed_branch is None and name_of_first_entry is None. This will cause a KeyError.
| self.update_status.emit(self.addon) | ||
|
|
||
| def install_branch(self, branch: str): | ||
| if self.addon.branch_display_name == branch: |
There was a problem hiding this comment.
The comparison uses == but should handle the case where self.addon.branch_display_name might be None. This could cause unexpected behavior when comparing with a string branch name.
| if self.addon.branch_display_name == branch: | |
| if self.addon.branch_display_name is not None and self.addon.branch_display_name == branch: |
| reply = self.QNAM.sendCustomRequest(request, b"HEAD") | ||
| reply = self.QNAM.head(request) | ||
| else: | ||
| raise NotImplementedError(f"Unknown operation {operation}") |
There was a problem hiding this comment.
The error message uses an f-string to display the operation, but operation is likely an enum value that may not have a meaningful string representation. Consider using operation.name or a more descriptive error message.
| raise NotImplementedError(f"Unknown operation {operation}") | |
| raise NotImplementedError(f"Unknown operation {operation.name}") |
| fci.Console.PrintWarning( | ||
| f"Could not load branch '{branch_display_name}' for addon {addon_id}: {str(e)}\n" | ||
| ) | ||
| continue |
There was a problem hiding this comment.
If all branches fail to load (all raise exceptions in the try-catch block), name_of_first_entry will remain None, making primary_branch_name None as well. The subsequent code assumes primary_branch_name is valid.
| continue | |
| continue | |
| if name_of_first_entry is None: | |
| fci.Console.PrintError( | |
| f"Failed to load the addon {addon_id} from the addon catalog, skipping it.\n" | |
| ) | |
| continue |
| if os.path.exists(original): | ||
| shutil.copytree(original, original + ".pre_update_backup", dirs_exist_ok=True) | ||
|
|
||
|
|
There was a problem hiding this comment.
The dirs_exist_ok=True parameter allows overwriting existing backup directories without warning. This could silently overwrite a previous backup, potentially losing important recovery data.
| backup_dir = original + ".pre_update_backup" | |
| if os.path.exists(original): | |
| if os.path.exists(backup_dir): | |
| # Warn the user and skip backup to avoid overwriting previous backup | |
| fci.Console.PrintError( | |
| f"Backup directory '{backup_dir}' already exists. Skipping backup to avoid overwriting previous backup." | |
| ) | |
| return | |
| shutil.copytree(original, backup_dir) |
Required so that the AM knows what branch is installed, since zip files don't have that info.
chennes
force-pushed
the
multiBranchHandling
branch
from
17dd760 to
68a1ef2
Compare
Adds an installation manifest to that the AM knows what branch is installed, and the implements switching between branches (for addons whose authors have chosen to present multiple branches to the end user -- currently only the Addon Manager itself and Assembly 4.1, for testing).
To test:
Remove any Addon Manager you have installed in your USER_APP_DATA_DIR/Mod directory and then clone this PR there.
Test installing an addon (anything).
Test updating your addons (consider making a backup before doing so :) ).
Test installing Assembly 4.1 and switching between the main and development branches.
Fixes #148 (because now you cannot "disable" the Addon Manager, the UI indicates that you are "reverting to built-in")
Fixes #113
Fixes #42
Fixes #40