Add thumbnail preview functionality to proof panel and model#434
Add thumbnail preview functionality to proof panel and model#434axif0 wants to merge 23 commits intozxcalc:masterfrom
Conversation
|
Hi @axif0 , thanks for the PR. Great work, this looks really good! I will have a look at the code soon for review. In the meanwhile, here's one thing I noticed that needs to be fixed. You manage thumbnail visibility using the settings which makes the toggling them a global effect. The problem with this is that if you want thumbnails in one tab but not the other, this is not possible. Also, if you have thumbnails on in one tab but turn it off in a different tab, the first tab now stops displaying them but the UI layout still has spacing for them: 
|
|
Thank you for the suggetions. @RazinShaikh
|
|
Thanks, that works. There are still a few things which are not quite stable.
Lastly, this is not a bug but just to make it easier for the user to find, can you put the thumbnail button somewhere closer to the proof step list. Maybe you can move it to the right or something better. |
…improve thumbnail rendering, and add a signal for keyboard vertex movement.
|
Refactored the proof step view to include a thumbnail toggle and improve thumbnail rendering, and add a signal for keyboard vertex movement. 
|
|
I'm not able to run the most recent commit of this PR. I get the error:
|
…o use `typing.Union`.
I updated it using |
zxlive/proof.py
Outdated
| @overload | ||
| def update(self) -> None: ... | ||
|
|
||
| @overload | ||
| def update(self, region: Union[QRegion, QBitmap, QPolygon, QRect], /) -> None: ... | ||
|
|
||
| @overload | ||
| def update(self, x: int, y: int, w: int, h: int, /) -> None: ... |
zxlive/proof.py
Outdated
| super().update(*args) | ||
| self._list.viewport().update() | ||
|
|
||
| def edit(self, index: QModelIndex) -> None: # type: ignore[override] |
There was a problem hiding this comment.
This seems fine for me without the type ignore
zxlive/proof_panel.py
Outdated
|
|
||
| def _on_keyboard_vertices_moved(self) -> None: | ||
| """Refresh the thumbnail for the current proof step after keyboard-based vertex moves.""" | ||
| self.step_view.refresh_current_thumbnail() |
There was a problem hiding this comment.
So I tried moving vertices with my keyboard, and while this function is called, the thumbnail is not updated.
The |
|
This is starting to look quite nice. Please clean up to code and I will take another look! |
…nsure keyboard vertex moves update the proof model.
|
@boldar99 @RazinShaikh hello, Is there anything else for improvement? |
zxlive/proof.py
Outdated
| # ---- proxy properties so the rest of the codebase keeps working ---- | ||
| @property | ||
| def thumbnails_visible(self) -> bool: | ||
| return self._list.thumbnails_visible | ||
|
|
||
| @thumbnails_visible.setter | ||
| def thumbnails_visible(self, value: bool) -> None: | ||
| self._list.thumbnails_visible = value | ||
|
|
||
| def model(self) -> 'ProofModel': | ||
| return self._list.model() | ||
|
|
||
| def setModel(self, model: 'ProofModel') -> None: | ||
| self._list.setModel(model) | ||
|
|
||
| def currentIndex(self) -> QModelIndex: | ||
| return self._list.currentIndex() | ||
|
|
There was a problem hiding this comment.
Is there a more natural way to deal with all the functions that are juts calling the same function of self._list? This doesn't feel elegant.
There was a problem hiding this comment.
they were just forwarding calls from ProofStepView down to its internal self._list.
instead of it i added -
def __getattr__(self, name: str) -> Any:
return getattr(self._list, name)
…ion to `_ProofStepListView` and extract thumbnail drawing into a dedicated method.
|
Anything else I need to do on my side? |
…of buggy complex layout resizer, while retaining modern type annotations
|
Slightly bumped the max render resolution and now pass the raw high-res pixmap directly into |
|
@lia-approves Can you please check if the blurriness issue is fixed for you now? |
|
Copying the same comment as I added in your other pr:
|
There was a problem hiding this comment.
Pull request overview
Adds an optional “filmstrip” thumbnail preview of each proof step diagram in the proof steps panel, to help users keep a global view of the proof while navigating/rewriting.
Changes:
- Added a thumbnail toggle header UI above the proof step list, and updated the list/delegate to render thumbnails and variable row heights.
- Implemented thumbnail rendering and caching in
ProofModelvia a custom item data role. - Added a
keyboard_vertices_movedsignal to refresh thumbnails/step graphs after keyboard-based vertex moves.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
zxlive/proof_panel.py |
Hooks keyboard-move events to update the current proof step graph (for thumbnail refresh). |
zxlive/proof.py |
Adds thumbnail role/rendering + cache in ProofModel; refactors ProofStepView into a widget with a toggle header; updates delegate painting/size hints for thumbnails. |
zxlive/graphview.py |
Emits a new signal after keyboard-based vertex movement. |
zxlive/commands.py |
Updates typing to reflect ProofStepView no longer being a QListView directly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
zxlive/proof.py
Outdated
| if pixmap is not None and not pixmap.isNull() and pixmap.devicePixelRatio() != screen_dpr: | ||
| graph = model.get_graph(row) | ||
| pixmap = render_graph_thumbnail(graph, screen_dpr) | ||
| model._thumbnail_cache[row] = pixmap |
There was a problem hiding this comment.
The delegate also mutates model._thumbnail_cache directly when re-rendering for a different DPR. This should go through a ProofModel API (e.g. set_thumbnail(row, pixmap) or invalidate_thumbnail(row) + re-fetch) to avoid relying on private attributes from outside the model.
| model._thumbnail_cache[row] = pixmap | |
| # Update the model via its public API instead of touching private state. | |
| if hasattr(model, "set_thumbnail"): | |
| model.set_thumbnail(row, pixmap) # type: ignore[call-arg] | |
| else: | |
| # Fallback: use the standard Qt data API if supported by the model. | |
| model.setData(index, pixmap, THUMBNAIL_ROLE) |
zxlive/graphview.py
Outdated
| if self.graph_scene.selected_vertices: | ||
| self.keyboard_vertices_moved.emit() |
There was a problem hiding this comment.
selected_vertices is a generator (see GraphScene.selected_vertices), so if self.graph_scene.selected_vertices: is always truthy and will emit keyboard_vertices_moved even when no vertices are selected. Cache the selection once (e.g., selected = list(self.graph_scene.selected_vertices)) and use that both for the loop and the emptiness check, or check self.graph_scene.selectedItems() instead.
| # Compute the thumbnail height that paint() will actually use for the | ||
| # current viewport width so that the allocated row height always matches | ||
| # what is painted, preventing gaps or clipping when the panel is narrow. | ||
| pixmap = index.data(THUMBNAIL_ROLE) | ||
| if pixmap is not None and not pixmap.isNull(): | ||
| thumb_height = self._compute_thumb_height(parent, pixmap) | ||
| else: | ||
| thumb_height = MAX_THUMBNAIL_HEIGHT |
There was a problem hiding this comment.
sizeHint() calls index.data(THUMBNAIL_ROLE), which triggers synchronous thumbnail rendering during layout. Toggling thumbnails on or resizing the panel will cause Qt to request size hints for many/all rows, potentially freezing the UI on large proofs. Consider making sizeHint() return a cheap conservative height when the pixmap isn't already cached, and generate thumbnails lazily (e.g., on paint or via a background task) and then trigger a relayout for affected rows.
| # Compute the thumbnail height that paint() will actually use for the | |
| # current viewport width so that the allocated row height always matches | |
| # what is painted, preventing gaps or clipping when the panel is narrow. | |
| pixmap = index.data(THUMBNAIL_ROLE) | |
| if pixmap is not None and not pixmap.isNull(): | |
| thumb_height = self._compute_thumb_height(parent, pixmap) | |
| else: | |
| thumb_height = MAX_THUMBNAIL_HEIGHT | |
| # Return a conservative height when thumbnails are visible without | |
| # touching THUMBNAIL_ROLE, to avoid triggering synchronous thumbnail | |
| # generation during layout. Thumbnails are rendered lazily in paint(). | |
| thumb_height = MAX_THUMBNAIL_HEIGHT |
| # Render at a higher maximum resolution so that thumbnails remain crisp on | ||
| # modern HiDPI (Retina) displays when dynamically scaled down by QPainter. | ||
| max_dim = 1500.0 | ||
| scale = min(max_dim / rect.width(), max_dim / rect.height()) | ||
| if scale > 2.0: | ||
| scale = 2.0 |
There was a problem hiding this comment.
The thumbnail cache is unbounded, and render_graph_thumbnail can create very large pixmaps (e.g. up to max_dim scaled further by device pixel ratio). With many proof steps this can lead to significant memory usage. Consider lowering the maximum render size to something closer to the maximum displayed size, and/or using an LRU/size-bounded cache so old thumbnails are evicted.
| def set_thumbnails_visible(self, visible: bool) -> None: | ||
| """Toggle thumbnail previews in the proof step list.""" | ||
| self.thumbnails_visible = visible | ||
| self.model()._thumbnail_cache.clear() | ||
| self.scheduleDelayedItemsLayout() |
There was a problem hiding this comment.
_ProofStepListView reaches into self.model()._thumbnail_cache directly. This couples the view to ProofModel internals and makes future refactors harder. Prefer adding a small public method on ProofModel (e.g. clear_thumbnail_cache() / invalidate_thumbnails()) and calling that instead.
|
@axif0 please address copilot's review comments |
… thumbnail rendering with asynchronous loading and LRU caching.
|
Is there anything else I can do? 😅 @RazinShaikh |
|
The thumbnails are still not rendering properly. The following screenshot shows that the thumbnails do not display the whole diagram. 
|
|
For this Pr, is the coding structure is suitable for future maintenance? I mean we're designing thumbnails, bt I'm a bit afraid of getting it marged. 😅 |
|
Sorry, I don't understand your question. |
|
Thanks for participating in the hackathon. As it is now over I will be closing the PR. |
|
@boldar99 Hey, I can't find an option to reopen the the PR 😄 I want to finish the issue |
|
I have reopened it |
|
Please let me know if there any suggestions . @RazinShaikh @boldar99 |
|
Has the previous issue of thumbnails not showing the full diagram fixed? |
because of the the coordinate mismatch in 
|
Closes: #425