Compare commits
No commits in common. "2874fe35d13358658d35a675b6bad198e4f7ff93" and "e4156ca0960945bc9eaf46be0b6686f5d32395d7" have entirely different histories.
2874fe35d1
...
e4156ca096
359
CODE_ANALYSIS.md
Normal file
359
CODE_ANALYSIS.md
Normal file
@ -0,0 +1,359 @@
|
|||||||
|
# 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.
|
||||||
|
|
||||||
2551
MyTimeline.py
2551
MyTimeline.py
File diff suppressed because it is too large
Load Diff
Loading…
x
Reference in New Issue
Block a user