Conversation
- Collect all dirty vertices and edges into sets before refreshing, so each VItem/EItem is refreshed exactly once regardless of how many diff categories it appears in. - Add _bulk_updating flag to GraphScene to suppress the VItem.itemChange cascade (triggered by setPos/setSelected) during update_graph, since the deduplication pass already handles all necessary refreshes. - Disable view updates for the duration of update_graph to prevent per-item repaint scheduling and BSP tree rebuilds. - Add cascade_edges parameter to VItem.refresh() so update_graph can skip per-vertex edge cascades and handle edges in a single batch. - Use vertex_map for O(1) lookups in select_vertices instead of iterating all scene items.
- Track _last_type to skip setPath() when vertex type is unchanged (phase-only or position-only changes no longer rebuild the path) - Remove redundant setPen from update_shape (overwritten by refresh) - Remove redundant QPen copy constructor in EItem.refresh - Inline update_shape logic into refresh and remove dead method
There was a problem hiding this comment.
Pull request overview
Improves UI performance by reducing redundant refresh work during graph updates and by caching computed display properties.
Changes:
- Deduplicates vertex/edge refreshes during
GraphScene.update_graph()and suppresses per-item cascades during bulk updates. - Optimizes
VItem.refresh()to only rebuild geometry when the vertex type changes and optionally cascade edge refreshes. - Adds caching for
DisplaySettings.dark_modeandeffective_colors.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| zxlive/vitem.py | Adds conditional/cascaded refresh logic and bulk-update refresh suppression. |
| zxlive/settings.py | Introduces caching for dark mode detection and effective color computation. |
| zxlive/graphscene.py | Implements bulk update mode, disables view repaints during updates, and deduplicates refresh calls. |
| zxlive/eitem.py | Avoids unnecessary QPen copy when setting the pen. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def effective_colors(self) -> dict[str, QColor]: | ||
| if self._cached_effective_colors is not None: | ||
| return self._cached_effective_colors |
There was a problem hiding this comment.
Caching and returning the same mutable dict instance changes the property’s behavior: callers that mutate the returned dict will now affect subsequent reads (and all consumers). Consider caching an immutable representation (or returning a copy, e.g. return self._cached_effective_colors.copy()), or changing the return type to a read-only mapping to prevent accidental mutation.
| self._cached_effective_colors = base | ||
| return base |
There was a problem hiding this comment.
Caching and returning the same mutable dict instance changes the property’s behavior: callers that mutate the returned dict will now affect subsequent reads (and all consumers). Consider caching an immutable representation (or returning a copy, e.g. return self._cached_effective_colors.copy()), or changing the return type to a read-only mapping to prevent accidental mutation.
| def dark_mode(self) -> bool: | ||
| if self._cached_dark_mode is not None: | ||
| return self._cached_dark_mode | ||
| dark_mode_setting = str(settings.value("dark-mode", "system")) | ||
| if dark_mode_setting == "system": |
There was a problem hiding this comment.
With caching, dark_mode == 'system' will no longer reflect runtime OS theme changes (previously it could, since it recomputed each access). If live theme updates are expected, invalidate this cache on the relevant Qt change events (e.g. application palette/style hint changes) or avoid caching when the setting is system.
| self.set_vitem_rotation() | ||
|
|
||
| def shape(self) -> QPainterPath: | ||
| return self._make_shape_path() |
There was a problem hiding this comment.
shape() recomputes the path on every call, which can be hot during hit-testing. Since refresh() now caches the geometry via setPath(...), consider returning the existing path (e.g. return self.path()) or caching the computed shape path so the new optimization isn’t undermined by frequent shape() calls.
| return self._make_shape_path() | |
| # Use the cached path set via setPath(...) in refresh() for hit-testing. | |
| return self.path() |
| # Skip refresh when the scene is performing a bulk graph update | ||
| # (it will refresh all affected items in a single pass afterwards). | ||
| if not self.is_animated and not self.graph_scene._bulk_updating: | ||
| self.refresh() |
There was a problem hiding this comment.
VItem directly depends on GraphScene’s private _bulk_updating flag. To reduce cross-class coupling, consider exposing a public read-only property/method on GraphScene (e.g. is_bulk_updating) and using that here, keeping _bulk_updating as an internal implementation detail.
No description provided.