dv/mygramps
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Implement all code analysis recommendations

Browse Source 
Phase 1 - Quick Wins:
- Extract date range calculation to _calculate_date_range() with caching
- Extract Y position calculation to _calculate_y_position()
- Add remaining constants (TOOLTIP_MAX_WIDTH, LABEL_BACKGROUND_PADDING, LABEL_BACKGROUND_RADIUS)

Phase 2 - Refactoring:
- Replace tuple indexing with TimelineEvent dataclass for type safety
- Break down collect_person_events into smaller methods:
  * _collect_person_event_refs()
  * _process_event_ref()
  * _collect_person_events()
- Break down show_tooltip into helper methods:
  * _format_person_tooltip()
  * _format_single_event_tooltip()
  * _get_or_create_tooltip_window()
- Break down draw_event_marker into helper methods:
  * _draw_marker_shadow()
  * _draw_marker_gradient()
- Break down on_draw into helper methods:
  * _draw_background()
  * _draw_no_events_message()
  * _draw_timeline_axis()
  * _draw_events()

Phase 3 - Enhancements:
- Add comprehensive type hints to all methods
- Improve error handling with specific exceptions (AttributeError, KeyError, ValueError)
- Add logging module with appropriate log levels
- Improve all docstrings with parameter and return documentation
- Reuse tooltip window instead of recreating each time
- Improve cache management with hash-based keys instead of context comparison

Code Quality Improvements:
- Type safety: TimelineEvent dataclass replaces error-prone tuple indexing
- Maintainability: Methods are shorter and more focused (28 → 40+ methods)
- Performance: Better caching with hash-based keys
- Readability: Clear method names and comprehensive docstrings
- Debugging: Logging for error tracking
- IDE support: Type hints improve autocomplete and error detection

All linter errors resolved. Code compiles successfully.
This commit is contained in:
Daniel Viegas 2025-11-29 00:17:58 +01:00
parent e4156ca096
commit e6c6afc4e4
2 changed files with 1178 additions and 323 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

394
CODE_ANALYSIS_V2.md Normal file
View File

@ -0,0 +1,394 @@
# 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.

1107
MyTimeline.py
View File

File diff suppressed because it is too large Load Diff