From 1efe97872800937211a3a8406fb4125d21db1d16 Mon Sep 17 00:00:00 2001 From: Daniel Viegas Date: Sat, 29 Nov 2025 00:23:36 +0100 Subject: [PATCH] Install plugin files to Gramps plugins directory - Plugin files are now installed and ready for use - All code improvements are included in the installed version --- CODE_ANALYSIS.md | 359 ---------------------------------------- CODE_ANALYSIS_V2.md | 394 -------------------------------------------- 2 files changed, 753 deletions(-) delete mode 100644 CODE_ANALYSIS.md delete mode 100644 CODE_ANALYSIS_V2.md diff --git a/CODE_ANALYSIS.md b/CODE_ANALYSIS.md deleted file mode 100644 index 69d9fce..0000000 --- a/CODE_ANALYSIS.md +++ /dev/null @@ -1,359 +0,0 @@ -# MyTimeline Plugin - Code Analysis & Enhancement Recommendations - -## Executive Summary - -The MyTimeline plugin is well-structured and functional, but there are several opportunities for improvement in code quality, performance, maintainability, and user experience. - -**Total Lines:** 1,312 -**Methods:** 31 -**Complexity:** Medium to High (some methods exceed 100 lines) - ---- - -## 1. Code Duplication Issues - -### 1.1 Collision Detection Logic (HIGH PRIORITY) -**Location:** `detect_label_overlaps()` (lines 516-561) and `find_event_at_position()` (lines 719-761) - -**Problem:** The same collision detection algorithm is duplicated in two places, making maintenance difficult. - -**Impact:** -- Bug fixes need to be applied twice -- Inconsistencies can occur between click detection and visual rendering -- ~45 lines of duplicated code - -**Recommendation:** -```python -def _calculate_adjusted_positions(self, events_with_y_pos, timeline_y_start, timeline_y_end, context=None): - """Calculate adjusted Y positions with collision detection. Reusable by both drawing and click detection.""" - # Extract common logic here - pass -``` - -### 1.2 Label Text Generation (MEDIUM PRIORITY) -**Location:** Multiple places (lines 532-540, 734-742, 1096-1143) - -**Problem:** Label text generation logic is repeated in several methods. - -**Recommendation:** -```python -def _get_event_label_text(self, event, person, event_type, is_expanded=False): - """Generate label text for an event. Centralized logic.""" - date_str = get_date(event) - event_type_str = str(event_type) - if person: - person_name = name_displayer.display(person) - if is_expanded: - # Expanded format - else: - return f"{date_str} - {event_type_str} - {person_name}" - else: - # Family event format -``` - ---- - -## 2. Performance Issues - -### 2.1 Recalculating Positions on Every Mouse Event (HIGH PRIORITY) -**Location:** `find_event_at_position()` (line 686) - -**Problem:** Every mouse move/click recalculates all Y positions and collision detection, which is expensive. - -**Impact:** -- Laggy mouse interaction with many events -- Unnecessary CPU usage -- Poor user experience - -**Recommendation:** -- Cache adjusted positions after `collect_events()` or `on_draw()` -- Only recalculate when events change or zoom changes -- Store `self.adjusted_events_cache` and invalidate on updates - -```python -def collect_events(self): - # ... existing code ... - self._adjusted_events_cache = None # Invalidate cache - -def _get_adjusted_events(self, context, timeline_y_start, timeline_y_end): - """Get adjusted events, using cache if available.""" - if self._adjusted_events_cache is None: - # Calculate and cache - return self._adjusted_events_cache -``` - -### 2.2 Temporary Cairo Surface Creation (MEDIUM PRIORITY) -**Location:** `find_event_at_position()` (lines 721-723) - -**Problem:** Creates a new Cairo surface on every call, which is expensive. - -**Recommendation:** -- Reuse a cached surface or context -- Or use Pango layout without Cairo context for text measurement - -### 2.3 Redundant Event Collection (LOW PRIORITY) -**Location:** `on_zoom_in()`, `on_zoom_out()`, `on_zoom_reset()` (lines 563-587) - -**Problem:** `collect_events()` is called on every zoom change, but events don't change, only layout does. - -**Recommendation:** -- Only recalculate timeline height, not re-collect events -- Separate height calculation from event collection - ---- - -## 3. Code Organization & Maintainability - -### 3.1 Long Methods (MEDIUM PRIORITY) -**Methods exceeding 50 lines:** -- `on_draw()`: 108 lines -- `draw_event_label()`: 106 lines -- `show_tooltip()`: 98 lines -- `find_event_at_position()`: 94 lines - -**Recommendation:** Break down into smaller, focused methods: -```python -def on_draw(self, widget, context): - """Main draw method - delegates to helpers.""" - self._draw_background(context) - self._draw_timeline_axis(context) - events_with_y_pos = self._calculate_and_draw_events(context) - self._draw_connections(context, events_with_y_pos) - self._draw_year_markers(context) -``` - -### 3.2 Magic Numbers (MEDIUM PRIORITY) -**Location:** Throughout codebase - -**Problem:** Hard-coded values make code harder to maintain: -- `600` (clickable width) -- `30` (min spacing) -- `16` (padding) -- `25` (label offset) -- `10` (marker area padding) - -**Recommendation:** Move to constants: -```python -CLICKABLE_AREA_WIDTH = 600 -MIN_LABEL_SPACING = 30 -LABEL_PADDING = 16 -LABEL_X_OFFSET = 25 -MARKER_CLICK_PADDING = 10 -``` - -### 3.3 Tuple-Based Data Structure (LOW PRIORITY) -**Location:** `self.events` (line 231) - -**Problem:** Using tuples with index access (`prev_data[6]`) is error-prone and hard to read. - -**Recommendation:** Use dataclass or named tuple: -```python -from dataclasses import dataclass - -@dataclass -class TimelineEvent: - date_sort: int - date_obj: Date - event: Event - person: Person | None - event_type: EventType - expanded: bool - y_pos: float -``` - ---- - -## 4. Error Handling - -### 4.1 Bare Exception Handlers (MEDIUM PRIORITY) -**Location:** 26 instances of bare `except:` clauses - -**Problem:** Catching all exceptions without logging makes debugging difficult. - -**Recommendation:** -```python -# Instead of: -except: - pass - -# Use: -except Exception as e: - # Log error for debugging - import logging - logging.warning(f"Error processing event: {e}", exc_info=True) - pass -``` - -### 4.2 Database Access Errors (LOW PRIORITY) -**Location:** `collect_events()`, `person_updated()`, etc. - -**Problem:** Database errors are silently ignored. - -**Recommendation:** Add specific exception handling for database errors. - ---- - -## 5. Code Quality Improvements - -### 5.1 Type Hints (LOW PRIORITY) -**Recommendation:** Add type hints for better IDE support and documentation: -```python -def collect_events(self) -> None: -def find_event_at_position(self, x: float, y: float) -> int | None: -def draw_event_marker(self, context: cairo.Context, x: float, y: float, - event_type: EventType, is_hovered: bool = False, - is_selected: bool = False) -> None: -``` - -### 5.2 Docstrings (LOW PRIORITY) -**Status:** Most methods have docstrings, but some could be more detailed. - -**Recommendation:** Add parameter and return value documentation. - -### 5.3 Constants Organization (LOW PRIORITY) -**Location:** Lines 63-72 - -**Recommendation:** Group related constants: -```python -# Timeline Layout -TIMELINE_MARGIN_LEFT = 150 -TIMELINE_MARGIN_RIGHT = 50 -TIMELINE_MARGIN_TOP = 50 -TIMELINE_MARGIN_BOTTOM = 50 -TIMELINE_LINE_WIDTH = 3 - -# Event Display -EVENT_MARKER_SIZE = 10 -EVENT_SPACING = 80 -LABEL_X_OFFSET = 25 -MIN_LABEL_SPACING = 30 -``` - ---- - -## 6. User Experience Enhancements - -### 6.1 Keyboard Navigation (NEW FEATURE) -**Recommendation:** Add keyboard shortcuts: -- Arrow keys to navigate between events -- Enter to select person -- Space to expand/collapse event - -### 6.2 Event Filtering (NEW FEATURE) -**Recommendation:** Add filter options: -- Filter by event type -- Filter by person -- Date range filtering - -### 6.3 Export Functionality (NEW FEATURE) -**Recommendation:** Add export options: -- Export timeline as image (PNG/SVG) -- Export event list as CSV - -### 6.4 Configuration Options (NEW FEATURE) -**Recommendation:** Add user preferences: -- Customizable colors for event types -- Adjustable spacing and margins -- Font size preferences - ---- - -## 7. Testing & Reliability - -### 7.1 Unit Tests (NEW) -**Recommendation:** Add unit tests for: -- Event collection logic -- Collision detection algorithm -- Date calculations -- Coordinate transformations - -### 7.2 Edge Case Handling (MEDIUM PRIORITY) -**Issues to address:** -- Very large families (100+ events) -- Events with no dates -- Events spanning very long time periods -- Zoom at extreme levels - ---- - -## 8. Memory Optimization - -### 8.1 Event Data Caching (LOW PRIORITY) -**Recommendation:** Cache formatted strings and calculated values to avoid repeated computation. - -### 8.2 Tooltip Window Management (LOW PRIORITY) -**Location:** `show_tooltip()` (line 780) - -**Recommendation:** Reuse tooltip window instead of creating new one each time. - ---- - -## Priority Summary - -### High Priority (Do First) -1. ✅ Extract collision detection to shared method -2. ✅ Cache adjusted positions to improve mouse interaction performance -3. ✅ Replace bare `except:` with specific exception handling - -### Medium Priority (Do Soon) -1. Extract label text generation to shared method -2. Break down long methods into smaller functions -3. Replace magic numbers with named constants -4. Optimize zoom operations (don't re-collect events) - -### Low Priority (Nice to Have) -1. Add type hints -2. Use dataclasses for event data structure -3. Add keyboard navigation -4. Add event filtering -5. Add unit tests - ---- - -## Estimated Impact - -**Performance Improvements:** -- Mouse interaction: 50-70% faster with caching -- Zoom operations: 30-40% faster by avoiding event re-collection - -**Code Quality:** -- Reduced duplication: ~100 lines of code -- Improved maintainability: Easier to modify and extend -- Better error handling: Easier debugging - -**User Experience:** -- Smoother interactions -- More features (keyboard nav, filtering, export) -- Better customization options - ---- - -## Implementation Order - -1. **Phase 1 (Quick Wins):** - - Extract collision detection method - - Add constants for magic numbers - - Cache adjusted positions - -2. **Phase 2 (Refactoring):** - - Extract label text generation - - Break down long methods - - Improve error handling - -3. **Phase 3 (Enhancements):** - - Add type hints - - Use dataclasses - - Add new features (keyboard nav, filtering) - ---- - -## Conclusion - -The MyTimeline plugin is functional and well-designed, but would benefit significantly from: -- Reducing code duplication -- Improving performance through caching -- Better code organization -- Enhanced error handling - -These improvements would make the codebase more maintainable, performant, and extensible. - diff --git a/CODE_ANALYSIS_V2.md b/CODE_ANALYSIS_V2.md deleted file mode 100644 index 9fe4f9d..0000000 --- a/CODE_ANALYSIS_V2.md +++ /dev/null @@ -1,394 +0,0 @@ -# MyTimeline Plugin - Additional Code Analysis & Improvements - -## Executive Summary - -After removing expand/collapse functionality and implementing previous enhancements, the codebase is in good shape. However, there are still several opportunities for improvement in code quality, maintainability, and robustness. - -**Current State:** -- Total Lines: 1,269 -- Methods: 28 -- Average Method Length: 35.6 lines -- Some methods exceed 100 lines (complexity concern) - ---- - -## 1. Tuple Indexing Issues (MEDIUM PRIORITY) - -### 1.1 Numeric Index Access -**Location:** Throughout codebase, especially `_calculate_adjusted_positions()` (line 573) - -**Problem:** Using numeric indices like `prev_data[5]` is error-prone and hard to maintain. - -**Current Code:** -```python -prev_y_pos = prev_data[5] # Get y_pos from previous event (index 5 now) -``` - -**Impact:** -- Easy to introduce bugs when tuple structure changes -- Code is hard to read and understand -- No type safety - -**Recommendation:** Use named tuples or dataclasses: -```python -from dataclasses import dataclass - -@dataclass -class TimelineEvent: - date_sort: int - date_obj: Date - event: Event - person: Person | None - event_type: EventType - y_pos: float - -# Then use: -prev_y_pos = prev_data.y_pos # Much clearer! -``` - -**Benefits:** -- Type safety -- Better IDE support -- Self-documenting code -- Easier refactoring - ---- - -## 2. Long Methods (MEDIUM PRIORITY) - -### 2.1 `collect_person_events` - 130 lines -**Location:** Lines 457-586 - -**Problem:** This nested function is very long and handles multiple responsibilities. - -**Recommendation:** Break into smaller functions: -```python -def _collect_person_event_refs(self, person): - """Collect event references from a person.""" - # Extract event reference collection logic - -def _process_event_ref(self, event_ref, person_obj): - """Process a single event reference.""" - # Extract event processing logic -``` - -### 2.2 `show_tooltip` - 100 lines -**Location:** Lines 804-903 - -**Problem:** Handles both person events and single events with complex formatting. - -**Recommendation:** Split into helper methods: -```python -def _format_person_tooltip(self, person, person_events): - """Format tooltip for person with multiple events.""" - -def _format_single_event_tooltip(self, event, event_type): - """Format tooltip for single event.""" -``` - -### 2.3 `draw_event_marker` - 94 lines -**Location:** Lines 994-1087 - -**Problem:** Handles multiple shape types and styling logic. - -**Recommendation:** Extract shape-specific logic: -```python -def _draw_marker_shadow(self, context, x, y, size): - """Draw shadow for marker.""" - -def _draw_marker_gradient(self, context, x, y, size, color): - """Draw gradient fill for marker.""" -``` - -### 2.4 `on_draw` - 90 lines -**Location:** Lines 904-993 - -**Problem:** Handles multiple drawing responsibilities. - -**Recommendation:** Break into helper methods: -```python -def _draw_background(self, context, width, height): - """Draw background gradient.""" - -def _draw_timeline_axis(self, context, timeline_x, y_start, y_end): - """Draw timeline axis with shadow.""" - -def _draw_events(self, context, events_with_y_pos, timeline_x): - """Draw all events.""" -``` - ---- - -## 3. Cache Management (LOW PRIORITY) - -### 3.1 Cache Invalidation Logic -**Location:** `_get_adjusted_events()` (line 726) - -**Problem:** Cache invalidation checks context object identity, which may not be reliable. - -**Current Code:** -```python -if (self._adjusted_events_cache is not None and - self._cache_context == context and - self._cache_timeline_y_start == timeline_y_start and - self._cache_timeline_y_end == timeline_y_end): -``` - -**Recommendation:** Use a hash-based cache key: -```python -def _get_cache_key(self, timeline_y_start, timeline_y_end): - """Generate cache key for adjusted events.""" - return hash((len(self.events), timeline_y_start, timeline_y_end)) -``` - -**Benefits:** -- More reliable cache invalidation -- Don't need to store context object -- Better performance - ---- - -## 4. Code Organization (LOW PRIORITY) - -### 4.1 Method Grouping -**Recommendation:** Group related methods together: -- Drawing methods: `on_draw`, `draw_event_marker`, `draw_event_label`, `draw_year_markers`, `draw_person_connections` -- Event handling: `collect_events`, `collect_person_events`, `_get_adjusted_events` -- Interaction: `on_button_press`, `on_motion_notify`, `on_leave_notify`, `find_event_at_position` -- UI: `build_widget`, `build_tree`, `goto_handle` - -### 4.2 Constants Organization -**Status:** Already well-organized, but could add more: -- Add constants for colors used in drawing -- Add constants for font sizes -- Add constants for padding values - ---- - -## 5. Error Handling Improvements (LOW PRIORITY) - -### 5.1 More Specific Exceptions -**Current:** Most handlers use generic `Exception` - -**Recommendation:** Use more specific exceptions where possible: -```python -except (AttributeError, KeyError) as e: - # Handle specific database access errors -except ValueError as e: - # Handle date parsing errors -``` - -### 5.2 Logging -**Recommendation:** Add logging for debugging: -```python -import logging -logger = logging.getLogger(__name__) - -try: - # code -except Exception as e: - logger.warning(f"Error processing event: {e}", exc_info=True) -``` - ---- - -## 6. Performance Optimizations (LOW PRIORITY) - -### 6.1 Repeated Calculations -**Location:** `on_draw()`, `find_event_at_position()` - -**Problem:** Date range calculation is repeated. - -**Recommendation:** Cache date range: -```python -@property -def date_range(self): - """Calculate and cache date range.""" - if not hasattr(self, '_cached_date_range') or self._cached_date_range is None: - if self.events: - min_date = min(event[0] for event in self.events) - max_date = max(event[0] for event in self.events) - self._cached_date_range = max_date - min_date if max_date != min_date else 1 - else: - self._cached_date_range = 1 - return self._cached_date_range -``` - -### 6.2 Tooltip Window Reuse -**Location:** `show_tooltip()` (line 804) - -**Problem:** Creates new tooltip window each time. - -**Recommendation:** Reuse tooltip window: -```python -if not hasattr(self, '_tooltip_window') or self._tooltip_window is None: - self._tooltip_window = Gtk.Window(type=Gtk.WindowType.POPUP) - # ... setup -# Reuse existing window -``` - ---- - -## 7. Type Safety (LOW PRIORITY) - -### 7.1 Type Hints -**Recommendation:** Add type hints for better IDE support: -```python -from typing import Optional, List, Tuple - -def collect_events(self) -> None: -def find_event_at_position(self, x: float, y: float) -> Optional[int]: -def draw_event_marker(self, context: cairo.Context, x: float, y: float, - event_type: EventType, is_hovered: bool = False, - is_selected: bool = False) -> None: -``` - -**Benefits:** -- Better IDE autocomplete -- Catch type errors early -- Self-documenting code - ---- - -## 8. Code Duplication (LOW PRIORITY) - -### 8.1 Date Range Calculation -**Location:** `on_draw()`, `_get_adjusted_events()`, `find_event_at_position()` - -**Problem:** Date range calculation is duplicated. - -**Recommendation:** Extract to method: -```python -def _calculate_date_range(self): - """Calculate date range from events.""" - if not self.events: - return 1 - min_date = min(event[0] for event in self.events) - max_date = max(event[0] for event in self.events) - return max_date - min_date if max_date != min_date else 1 -``` - -### 8.2 Y Position Calculation -**Location:** Multiple places - -**Problem:** Y position calculation formula is repeated. - -**Recommendation:** Extract to method: -```python -def _calculate_y_position(self, date_sort, min_date, date_range, - timeline_y_start, timeline_y_end): - """Calculate Y position for an event based on date.""" - return timeline_y_start + ( - (date_sort - min_date) / date_range - ) * (timeline_y_end - timeline_y_start) -``` - ---- - -## 9. Magic Numbers (LOW PRIORITY) - -### 9.1 Remaining Magic Numbers -**Locations:** -- Line 567: `text_height + LABEL_PADDING` (already using constant, good!) -- Line 800: `500` (tooltip width?) -- Line 1125: `padding = 8` (could be constant) -- Line 1133: `radius = 5` (could be constant) - -**Recommendation:** Add constants: -```python -TOOLTIP_MAX_WIDTH = 500 -LABEL_BACKGROUND_PADDING = 8 -LABEL_BACKGROUND_RADIUS = 5 -``` - ---- - -## 10. Documentation (LOW PRIORITY) - -### 10.1 Docstring Improvements -**Recommendation:** Add parameter and return value documentation: -```python -def draw_event_marker(self, context, x, y, event_type, is_hovered=False, is_selected=False): - """ - Draw a marker for an event with modern styling. - - Args: - context: Cairo drawing context - x: X coordinate for marker center - y: Y coordinate for marker center - event_type: Type of event (determines color and shape) - is_hovered: Whether marker is currently hovered - is_selected: Whether marker belongs to selected person - - Returns: - None - """ -``` - ---- - -## Priority Summary - -### Medium Priority (Consider Soon) -1. ✅ Replace tuple indexing with named tuples/dataclasses -2. ✅ Break down long methods (collect_person_events, show_tooltip, draw_event_marker, on_draw) -3. ✅ Extract repeated calculations (date range, Y position) - -### Low Priority (Nice to Have) -1. Improve cache management -2. Add type hints -3. Add more specific exception handling -4. Add logging -5. Extract remaining magic numbers -6. Improve docstrings -7. Reuse tooltip window - ---- - -## Estimated Impact - -**Code Quality:** -- Better maintainability with named tuples -- Easier to understand with shorter methods -- Better type safety with type hints - -**Performance:** -- Slight improvement from caching date range -- Better cache management - -**Developer Experience:** -- Better IDE support -- Easier debugging with logging -- Self-documenting code - ---- - -## Implementation Order - -1. **Phase 1 (Quick Wins):** - - Extract date range calculation - - Extract Y position calculation - - Add remaining constants - -2. **Phase 2 (Refactoring):** - - Break down long methods - - Replace tuple indexing with named tuples - -3. **Phase 3 (Enhancements):** - - Add type hints - - Improve error handling - - Add logging - - Improve docstrings - ---- - -## Conclusion - -The codebase is in good shape after recent improvements. The main areas for further enhancement are: -- Using named tuples/dataclasses instead of numeric indexing -- Breaking down long methods for better maintainability -- Extracting repeated calculations -- Adding type hints for better IDE support - -These improvements would make the code more maintainable, type-safe, and easier to understand. -