-
-
Notifications
You must be signed in to change notification settings - Fork 432
Refitem refactor, fix refs cache, handling of annotated tags, and switching to buggy versions #2868
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
- Add detailed logging for install, reinstall, and switch-ref operations - Display RefItem from/to information with proper formatting - Include operation type suffix: (install), (reinstall), (switch-ref) - Use direct UUID lookup for old metadata instead of plugin iteration - Change git ops logging from info to debug to avoid redundancy
- Capture old metadata before git operation in switch_ref - Use stored ref name instead of git HEAD for accurate logging - Add helper method to eliminate duplicated logging code - Use plugin.uuid directly instead of PluginValidation.get_plugin_uuid
- Add ref_item field to store current git ref information - Add sync_ref_item_from_git method to sync with git repository state - Add basic unit tests for RefItem functionality - Use TYPE_CHECKING import for RefItem type hint
- Add cache_ref_items_for_commit() to store RefItems by commit ID - Add get_ref_items_for_commit() to retrieve RefItems for specific commits - Add add_ref_item_to_commit() to add single RefItems to cache - Support multiple RefItems per commit (tags and branches) - Add comprehensive unit tests for new functionality
- Add _get_ref_item_from_git_state() to resolve RefItem from git repository - Add _resolve_annotated_tag_info() to handle annotated tags correctly - Add _get_ref_item_from_commit() for cache-based RefItem lookup - Add _normalize_ref_parameter() to support RefItem|str|None parameters - Add preference methods for selecting best RefItem from multiple options - Add comprehensive unit tests for all new methods
- Add to_dict() method to RefItem for serialization - Add from_dict() classmethod to RefItem for deserialization - Update refs_cache to use RefItem serialization methods - Simplify cache code by removing manual dict construction - Add comprehensive tests for RefItem serialization roundtrip
…ntic versioning - Add RefItem equality, hashing, and sorting methods for better data handling - Implement RefItem validation methods (is_valid, is_commit_only, get_ref_type) - Add RefItem copy method with field overrides - Enhance serialization with validation and ref_type field - Add RefItem.create_from_ref_name factory method with automatic type detection - Implement semantic version sorting in PluginManager with prerelease handling - Add advanced refs_cache methods: sorting, searching, and cleanup functionality - Enhance error handling for corrupted cache data - Add comprehensive tests for all new RefItem functionality - Update existing tests to accommodate enhanced serialization format
Update test expectation to reflect alphabetical sorting of branches when no tags are available in _default_ref_preference method.
- Enhance switch_ref method to accept RefItem objects via _normalize_ref_parameter - Add RefItem synchronization after ref operations in both methods - Improve logging to use actual plugin.ref_item when available - Add comprehensive tests for RefItem integration in manager methods - Maintain backward compatibility with string ref parameters - Ensure plugin.ref_item stays synchronized with git repository state
- Update plugin list widget to pass RefItem objects directly to switch_ref - Update install plugin dialog to pass RefItem objects to install_plugin - Remove unnecessary .name extraction since manager methods now accept RefItem - Add integration tests to verify RefItem usage in selector context - Maintain full RefItem information flow from UI to manager methods - Ensure consistent RefItem handling across install, reinstall, and switch operations
Performance improvements: - Consolidate RefItem imports in manager.py (6 imports → 1 import) - Remove redundant ref_type field from serialization (reduces cache size) - Add RefItem.for_logging() factory method for efficient logging - Optimize logging calls to avoid unnecessary RefItem creation - Improve null handling in logging with conditional formatting Simplifications: - Cleaner import structure reduces module loading overhead - Minimal RefItem creation for logging-only purposes - Streamlined serialization format - Enhanced test coverage for new factory method All existing functionality preserved with improved efficiency.
Plugin Metadata Manager: - Use existing plugin.ref_item when available for efficiency - Avoid redundant git operations when RefItem data is current - Fallback to direct git detection when RefItem unavailable Async Operations Manager: - Update docstrings to document RefItem support in install_plugin - Update docstrings to document RefItem support in switch_ref - Maintain backward compatibility with string refs Integration Tests: - Verify RefItem.for_logging efficiency and null handling - Confirm async manager docstring updates - Validate plugin metadata RefItem integration exists These integrations improve performance by reusing existing RefItem data and provide better documentation of RefItem support throughout the system.
Problem: Cache was being saved after every single plugin operation, causing 5+ separate disk writes during plugin initialization. Solution: - Add RefsCacheBatch context manager for batching cache updates - Implement _dirty flag tracking to defer writes until batch completion - Add batch_updates() method to RefsCache for grouping operations - Update init_plugins() to use batching during plugin initialization Performance Impact: - Reduces disk writes from N operations to 1 write per batch - Eliminates redundant JSON serialization during bulk operations - Maintains data consistency with proper dirty flag tracking - Preserves existing behavior for single operations Testing: - Comprehensive tests verify batching reduces disk writes - Context manager properly enters/exits batch mode - Dirty flag tracking works correctly - All existing functionality preserved
- Move RefItem sync after plugin enable attempts in all methods - Add _enable_plugin_and_sync_ref_item() helper with enable parameter - Eliminate duplicate cache flushes during plugin operations - Ensure RefItem display always matches actual repo state
- Auto-select current ref in appropriate list widget - Switch to correct tab containing current ref - Prioritize tags over branches for display - Ensure current ref is always visible and highlighted
- Add InstallResult class to track both install and enable status - Update helper method to return enable success/failure with error details - Add exc_info=True to error logging for full stack traces - Show warning dialogs when plugins install/update but fail to enable - Update UI dialogs to display specific enable failure messages - Move PyQt6 imports to top of files - Ensure users are always informed of plugin operation problems
- Add cache batching to prevent excessive cache flushes during operations - Fix switch_ref completion handler type checking to prevent crashes - Prevent network calls during options dialog initialization - Update plugin display refresh to show new version after ref switch - Improve enable failure error handling across all operations
- Log selected ref for debugging plugin operations - Shows exactly which ref user selected in ref selector dialog
- Add helper methods to create RefItem from target ref (reduce duplication) - Preserve user-selected ref names during install and switch operations - Prevent RefItem sync from overwriting correctly set values - Prioritize live RefItem over stored metadata for display consistency - Improve switch_ref logging to show tag names instead of commit hashes - Remove nested cache batching to prevent duplicate flushes
- Fix undefined variable bug in get_plugin_update_status - Add update status check after plugin installation - Improve _get_new_version to show actual versions from refs cache - Simplify plugin list widget by removing redundant update status cache - Use manager's get_plugin_update_status directly for cleaner architecture - Add debug logging for update column setup and status checking
- Create unified _set_plugin_ref_item_from_operation method - Fix update_plugin to preserve tag names instead of showing main branch - Remove redundant _set_plugin_ref_item_after_install wrapper method - Eliminate code duplication between install/switch/update operations - Ensure consistent RefItem handling across all plugin management operations
- Add common _handle_enable_failure() method for consistent error dialogs across all plugin operations (install, reinstall, update, switch ref) - Fix async manager switch_ref to return enable status information - Fix _refresh_plugin_display to properly update plugin enabled state using setCheckState - Use UUID comparison instead of plugin_id for plugin matching - Fix update column checkbox visibility using proper Qt setData() method with empty QVariant - Clear version cache after ref switch to ensure update status is refreshed - Ensure update checkboxes only appear when updates are available
- Update tests to handle InstallResult return type instead of plugin_id string - Fix switch_ref tests to handle new dictionary return format - Update RefItem integration tests to match new behavior - Fix variable name issues in test code - Update reinstall test to reflect current behavior (doesn't preserve ref)
- Fix install_plugin CLI to handle InstallResult objects instead of strings - Fix switch_ref CLI to extract tuple values from dictionary return - Extend switch_ref manager method to return both dict and tuple data - Ensure plugin_ref_switched signal is emitted before return - Maintain colorized CLI output for ref transitions - Add cleanup step to test script to prevent UUID conflicts
- Replace get_ref_type usage in manager.py with RefItem.is_tag property - Remove unused get_ref_type import from manager.py - Keep get_ref_type in ref_utils.py for repository-level ref resolution - RefItem.get_ref_type() and ref_utils.get_ref_type() serve different purposes
- Update test_switch_ref_cli to expect dictionary return from switch_ref - Update test_install_command_execution to expect InstallResult from install_plugin - Update test_install_by_plugin_id to expect InstallResult from install_plugin - Tests now compatible with new return types while maintaining coverage
- Create minimal resolve_ref utility function for reference resolution - Replace all duplicated git reference resolution code with resolve_ref calls - Use repo.get_references() and repo.revparse_single() directly - Eliminate unnecessary abstraction layer while reducing code duplication - Keep RefItem.get_ref_type() method for object-level operations
- Move common plugin enable failure handling to picard.ui.util - Replace identical methods in plugins3.py and pluginlistwidget.py - Reduce code duplication while maintaining same functionality
- Move GitResetMode and GitObjectType imports to top of plugin.py - Remove lazy import from handle_plugin_enable_failure function - Improve code organization by following import conventions
- Handle both 'branch' and 'origin/branch' formats in resolve_ref - Fixes warnings about failing to resolve 'origin/v1.1.1' references - Improves compatibility with existing ref resolution patterns
- Replace complex manual ref resolution logic with resolve_ref calls - Fixes 'Failed to resolve reference origin/vX.X.X' warnings - Ensures consistent reference handling across all plugin operations - Eliminates duplicate reference resolution code
- Move RefItem import from lazy imports to top level - Remove three duplicate lazy imports within functions - Improve code organization following import conventions
- Break down 200+ line sync() method into 8 helper methods - Each method now has single responsibility - Improves maintainability and testability - Preserves all original functionality
- Break down 60+ line update() method into 4 helper methods - Each method now has single responsibility - Improves maintainability and testability - Preserves all original functionality
- Break down 173-line install_plugin() into 11 smaller methods - Each method now has single responsibility - Improves readability, testability, and maintainability - No functional changes, only structural refactoring
- Break down 130-line method into 6 focused methods - Reuse existing methods from git URL installation - Eliminate code duplication between installation paths - Improve maintainability and consistency
- Break down 130-line method into 9 focused methods - Reuse existing validation and enabling logic - Improve maintainability and testability - Follow same patterns as installation methods - Fix ref handling to capture updated ref from source
- Break down 90-line method into 6 focused methods - Centralize error handling in _check_single_plugin_update - Improve maintainability and testability - Follow same patterns as other refactored methods
- Break down 95-line method into 6 focused methods - Reuse existing methods from check_updates refactoring - Separate caching logic from live checking logic - Eliminate code duplication between update checking methods
- Return RefItem objects for branches and tags instead of dictionaries - Update all callers to work with RefItem properties (.name, .commit) - Add helper method to convert cached dicts to RefItem objects - Maintain backward compatibility with caching layer - Improve type safety and consistency across codebase
- Use RefItem.to_dict() for serializing to cache instead of manual dict creation - Use RefItem.from_dict() for deserializing from cache instead of manual construction - Simplifies code and ensures all RefItem properties are properly preserved - Leverages built-in validation in RefItem.from_dict()
…ties - Return single list of RefItem objects instead of separate branches/tags dicts - Use RefItem.is_tag and RefItem.is_branch properties for filtering - Maintain backward compatibility with legacy cache format - Eliminate redundant data structure separation - Simplify all callers to filter by RefItem properties
- Remove legacy cache format support (branches/tags dict) - Handle cache format/read failures gracefully by returning None - Continue with fresh fetch when cache conversion fails - Simplify cache conversion logic with proper error handling
- Create RefItem objects directly instead of intermediate dictionaries - Use RefItem properties for annotated tag handling - Simplify sorting logic with RefItem-aware comparisons - Remove redundant data structure conversions - Cleaner, more direct code flow
- Eliminate duplicate if/elif branches for cache checking - Use single conditional with allow_expired parameter - Reduce code duplication and improve readability
- Use proper two-step process for annotated tags
- First pass: collect all refs and dereferenced commits
- Second pass: update annotated tags with actual commits
- Fixes race condition where ^{} refs might appear before tag objects
- Update cache_all_refs to expect list of RefItem dicts instead of dict with branches/tags - Update get_cached_all_refs to return list and validate format - Reject old dict format gracefully by invalidating cache - Fix logging to count refs by type from list
…refs - Use _find_newer_version_tag() for plugins with versioning_scheme (like semver) - Fallback to basic _find_latest_tag() for plugins without versioning_scheme - Fixes update detection for plugins like XQAF that use semantic versioning - Ensures newer versions (0.3.1, 0.3.2) are detected when on 0.3.0
- it will ensure we have Plugin.uuid properly set - it fixes missing updates (because uuid = None)
|
@zas Something is not working right on update checks. I made new commits with a new tag to my playground repo (local install). When checking with the picard master branch and using "Refresh all" it shows as update available. When I then switch picard to the refitem_refactor branch in this state upon opening settings Picard will now also show the tag version and commit ref in the update column. But when then doing "Refresh all" it will now no longer detect the update and show the plugin as no update available. Switching back to Picard master the refresh all will show the update again. |
Summary
Problem
The old code had many issues in the way it was handling references.
Especially with annotated tags, and when switching from a valid version to a buggy one.
Solution
Rethink and refactor so we make more use of RefItem which was heavily extended.
Better handling of enabling failures (install/reinstall/switch-ref/update), so the user gets a message, the error is properly logged, and the plugin disabled.
Also redesign the refs cache so it is much more efficient (less I/O by batching, instead of having one cache flush per plugin), and cache much more ref infos to avoid disk or network operations.
Get rid of hacky get_ref_type() and replace it with a resolve_ref() method which should be more reliable.
Add more unit tests.
Action
Additional actions required: