-
Notifications
You must be signed in to change notification settings - Fork 53
Highlight graph changes per rewrite step in proof view #190 #448
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
dorakingx
wants to merge
35
commits into
zxcalc:master
Choose a base branch
from
dorakingx:feature/issue-190-highlight-rewrites
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
35 commits
Select commit
Hold shift + click to select a range
a106b48
Highlight changed nodes and edges per rewrite step
dorakingx c6859a3
Refine rewrite highlighting and clean debug instrumentation
dorakingx d3230a1
Clean up proof panel debug logging
dorakingx 0ab3e51
Fix Spider Fusion edge highlighting: use highlight_match_pairs + inci…
dorakingx 9c84205
Fix Magic Wand unfuse highlighting: only highlight selected/split ver…
dorakingx 5f8095d
Add View menu toggle for rewrite highlights; refresh on settings change
dorakingx 34bcf23
Prune highlight code; restore highlight_verts for color change/strong…
dorakingx c757e82
Optimize move_to_step: remove unused g_next, compute current_verts once
dorakingx d1d2c22
Fix mypy: no-redef in proof.py, step_view type ProofStepView in comma…
dorakingx 09a9ffa
Fix pyflakes: remove unused imports in commands.py, remove unused var…
dorakingx a22f317
Support MATCH_COMPOUND in rewrite highlighting: pass highlight_verts …
dorakingx 0f321e8
Highlight: custom rules, add identity, edge-only, pretty colors
dorakingx 68746d1
Refactor: use pyzx edges helper in _edges_between
dorakingx 9a0cf37
Fix mypy no-redef by renaming edge-only set
dorakingx 09142ee
Highlight: fix strong complementarity animation and vertex coverage
dorakingx c058ec2
Merge master into feature/issue-190-highlight-rewrites and resolve co…
dorakingx 7349ae7
Remove _edges_between wrapper, use g.edges() directly (per review)
dorakingx bbc2c65
Revert VItemAnimation robustness changes per review
dorakingx 6b2bf48
Add generic highlight fallbacks and local change-z-to-x rule
dorakingx be767cc
Remove temporary debug logging helpers
dorakingx bf11dfa
Use CustomRule for change-z-to-x
dorakingx 59e2537
Refine highlight helpers and silence complexity lints
dorakingx b44c174
Merge origin/master into feature/issue-190-highlight-rewrites
dorakingx 32e1d63
Highlight parallel-edge magic wand rewrites
dorakingx dd0f25d
Refine proof rewrite highlighting and satisfy linters
dorakingx 4ea7e12
Ensure proof module uses postponed annotations
dorakingx 000c097
Refactor VItem.refresh to reduce branching
dorakingx 0c26bf3
Fix mypy no-redef in rewrite highlight helper.
dorakingx 179720a
Split VItem mouse drag handlers for complexipy.
dorakingx 4fd63e5
Set QT_QPA_PLATFORM for headless pytest
dorakingx 134a793
Add missing galois dependency for pyzx web
dorakingx 9877f62
Lazy-load SFX to avoid Qt multimedia segfault
dorakingx b3cbb02
Revert "Fixes #467 remove the 2 minimum edges condition from can_unfuse"
dorakingx f792132
Fix mypy and avoid CI segfault in headless tests
dorakingx 2be102a
Restore complexipy ignore for session restore
dorakingx File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| # Summary of changes (for PR reply) | ||
|
|
||
| ## What this PR does | ||
|
|
||
| This PR adds **forward-looking rewrite highlighting** in proof mode: when you select a proof step, the graph highlights **what will change in the next step** (the vertices and edges affected by that rewrite), instead of what changed in the previous one. It also adds a **GUI toggle** to turn this highlighting on or off, and ensures **only the relevant vertices/edges** are highlighted (no coordinate drift or unrelated nodes). | ||
|
|
||
| --- | ||
|
|
||
| ## 1. Highlight behaviour | ||
|
|
||
| - **Forward-looking**: Step *i* shows the graph at *i* and highlights the transition to step *i*+1 (the next rewrite). | ||
| - **Semantic, not structural**: We no longer use a generic graph diff. We store a small amount of metadata when a rewrite is applied and use that to highlight: | ||
| - **Match-based** (e.g. Spider Fusion): we store `highlight_match_pairs` — the vertex pairs `(v1, v2)` that were matched. When displaying the step, we highlight those two vertices and only the edge between them. | ||
| - **Vertex-based** (unfuse, color change, strong complementarity, remove identity, etc.): we store `highlight_verts` — the vertex IDs involved. We highlight those vertices and, if there are exactly two, only the edge between them; otherwise all incident edges. | ||
|
|
||
| So the logic is: “when this step was created, we recorded which vertices (and for fuse, which pair) were involved; when we show this step, we highlight those in the current graph.” | ||
|
|
||
| --- | ||
|
|
||
| ## 2. Where metadata is set | ||
|
|
||
| - **Rewrite tree / drag-and-drop** | ||
| In `rewrite_action.py`, when the “Fuse spiders” rule is applied we set `highlight_match_pairs` from the match `(v1, v2)`. | ||
| In `proof_panel.py`, drag-and-drop and double-click handlers set `highlight_match_pairs` or `highlight_verts` (e.g. “Fuse spiders” → `[(v,w)]`, “Strong complementarity” → `[v,w]`, “Color change” / “Remove identity” → `[v]`, “unfuse” → list of split vertex IDs). | ||
|
|
||
| - **Commands** | ||
| `AddRewriteStep` in `commands.py` takes optional `highlight_match_pairs` and `highlight_verts` and passes them into the new `Rewrite` stored in the proof model. | ||
|
|
||
| - **Proof model** | ||
| Each `Rewrite` in `proof.py` can carry `highlight_match_pairs` and `highlight_verts`. When the user selects a step, `ProofStepView.move_to_step` uses this metadata to compute the sets of vertices and edges to highlight and calls `scene.set_rewrite_highlight(verts, edges)`. | ||
|
|
||
| --- | ||
|
|
||
| ## 3. GUI toggle | ||
|
|
||
| - **View menu**: “Show rewrite highlights” checkable action; toggling it turns highlighting on/off and persists the setting. | ||
| - **Preferences**: “Highlight rewrite steps” in the General tab (same setting). | ||
| - When the setting is off, `move_to_step` clears the highlight and does not apply any of the above logic. When the setting is changed (from the menu or Preferences), the current step’s highlight is refreshed so the graph updates immediately. | ||
|
|
||
| --- | ||
|
|
||
| ## 4. Lint fixes (mypy) | ||
|
|
||
| - **proof.py**: Resolved `no-redef` for the edge set used in the vertex-based branch by introducing a single `edges_highlight` variable and assigning it in the two branches (either from `_edges_between` or by building the set of incident edges). | ||
| - **commands.py**: `AddRewriteStep.step_view` is now typed as `ProofStepView` instead of `QListView`, so mypy recognizes `move_to_step`. | ||
| - **app.py**: Added `# type: ignore[attr-defined]` for `setColorScheme` on `QStyleHints` so that `mypy zxlive` passes (stubs may not define it yet). | ||
|
|
||
| --- | ||
|
|
||
| ## Files touched (overview) | ||
|
|
||
| | File | Role | | ||
| |------|------| | ||
| | `zxlive/proof.py` | `Rewrite` metadata; `move_to_step` highlight logic; `_edges_between` helper; settings check. | | ||
| | `zxlive/commands.py` | `AddRewriteStep` carries and passes highlight metadata; typed as `ProofStepView`. | | ||
| | `zxlive/proof_panel.py` | Sets `highlight_match_pairs` / `highlight_verts` when creating rewrite steps (drag-drop, double-click, Magic Wand, unfuse). | | ||
| | `zxlive/rewrite_action.py` | Sets `highlight_match_pairs` for “Fuse spiders” from the rewrite tree. | | ||
| | `zxlive/graphscene.py` | `set_rewrite_highlight` / `clear_rewrite_highlight`; scene stores and uses highlighted verts/edges. | | ||
| | `zxlive/vitem.py` / `zxlive/eitem.py` | Drawing uses scene’s highlight state for vertices and edges. | | ||
| | `zxlive/mainwindow.py` | View menu toggle and `refresh_rewrite_highlight()`. | | ||
| | `zxlive/settings.py` / `zxlive/settings_dialog.py` | “Highlight rewrite steps” setting and persistence. | | ||
| | `test/test_highlight.py` | Test that step selection and toggle affect highlights correctly. | | ||
|
|
||
| You can paste the “Summary of changes” and “Lint fixes” sections (or a shortened version) into the PR as the explanation for reviewers. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,143 @@ | ||
| from __future__ import annotations | ||
|
|
||
| import copy | ||
| import sys | ||
| import types | ||
|
|
||
| import pytest | ||
| from PySide6 import QtCore | ||
| from pytestqt.qtbot import QtBot | ||
| from pyzx.utils import EdgeType, VertexType | ||
|
|
||
|
|
||
| # Ensure compatibility with pyzx versions used by ZXLive. We only patch things | ||
| # that are missing, and leave real implementations untouched when they exist. | ||
| import pyzx | ||
|
|
||
| try: # pragma: no cover - defensive shims for newer pyzx | ||
| import pyzx.tikz as _pyzx_tikz # type: ignore[import] | ||
| if not hasattr(_pyzx_tikz, "synonyms_dummy"): | ||
| _pyzx_tikz.synonyms_dummy = () # type: ignore[attr-defined] | ||
|
|
||
| import pyzx.settings as _pyzx_settings # type: ignore[import] | ||
| tc = getattr(_pyzx_settings, "tikz_classes", None) | ||
| if not isinstance(tc, dict) or "dummy" not in tc: | ||
| tc = dict(tc or {}) | ||
| tc.setdefault("dummy", "") | ||
| _pyzx_settings.tikz_classes = tc # type: ignore[attr-defined] | ||
| except Exception: | ||
| pass | ||
|
|
||
|
|
||
| # Newer versions of pyzx no longer expose the historical `pyzx.rewrite` module, | ||
| # but ZXLive still imports `RewriteSimpGraph` from there. Try to import the real | ||
| # module first; if that fails, provide a minimal compatibility shim. | ||
| try: # pragma: no cover - prefer real module if available | ||
| import pyzx.rewrite as _pyzx_rewrite # type: ignore[import] | ||
| except Exception: # pragma: no cover - fallback shim | ||
| if "pyzx.rewrite" not in sys.modules: | ||
| rewrite_module = types.ModuleType("pyzx.rewrite") | ||
|
|
||
| class RewriteSimpGraph: # type: ignore[override] | ||
| def __init__(self, *args: object, **kwargs: object) -> None: | ||
| pass | ||
|
|
||
| def __class_getitem__(cls, item: object) -> type: # type: ignore[override] | ||
| return cls | ||
|
|
||
| rewrite_module.RewriteSimpGraph = RewriteSimpGraph # type: ignore[attr-defined] | ||
| rewrite_module.Rewrite = object # type: ignore[attr-defined] | ||
| sys.modules["pyzx.rewrite"] = rewrite_module | ||
|
|
||
|
|
||
| from zxlive.commands import AddRewriteStep | ||
| from zxlive.common import GraphT, new_graph | ||
| from zxlive.edit_panel import GraphEditPanel | ||
| from zxlive.mainwindow import MainWindow | ||
| from zxlive.proof_panel import ProofPanel | ||
| from zxlive.settings import display_setting | ||
|
|
||
|
|
||
| def make_two_spider_graph() -> tuple[GraphT, GraphT]: | ||
| """Create initial graph (2 Z spiders, phase 0 and 0.5, connected) and fused graph.""" | ||
| g = new_graph() | ||
|
|
||
| v0 = g.add_vertex(VertexType.Z, 0, 0.0) | ||
| v1 = g.add_vertex(VertexType.Z, 0, 1.0) | ||
|
|
||
| # We rely on these being 0 and 1 for clarity in assertions below. | ||
| assert v0 == 0 | ||
| assert v1 == 1 | ||
|
|
||
| g.set_phase(v1, 0.5) | ||
| g.add_edge((v0, v1), EdgeType.SIMPLE) | ||
|
|
||
| g_fused = copy.deepcopy(g) | ||
| pyzx.rewrite_rules.fuse(g_fused, v0, v1) | ||
|
|
||
| return g, g_fused | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def app(qtbot: QtBot) -> MainWindow: | ||
| mw = MainWindow() | ||
| qtbot.addWidget(mw) | ||
| return mw | ||
|
|
||
|
|
||
| def test_rewrite_highlight_set_and_cleared_on_step_change(app: MainWindow, qtbot: QtBot) -> None: | ||
| initial_graph, fused_graph = make_two_spider_graph() | ||
|
|
||
| # Open a new tab with our simple two-spider graph. | ||
| app.new_graph(initial_graph, name="Highlight Test") | ||
| assert app.active_panel is not None | ||
| assert isinstance(app.active_panel, GraphEditPanel) | ||
| edit_panel: GraphEditPanel = app.active_panel | ||
|
|
||
| # Start a derivation from this graph to enter proof mode. | ||
| qtbot.mouseClick(edit_panel.start_derivation, QtCore.Qt.MouseButton.LeftButton) | ||
| assert app.active_panel is not None | ||
| assert isinstance(app.active_panel, ProofPanel) | ||
| proof_panel: ProofPanel = app.active_panel | ||
|
|
||
| # Ensure rewrite highlighting is on so the test is independent of user settings. | ||
| display_setting.highlight_rewrites = True | ||
|
|
||
| # Add a Fuse spiders rewrite with match pair so step 0 highlights the fused vertices/edge. | ||
| cmd = AddRewriteStep( | ||
| proof_panel.graph_view, fused_graph, proof_panel.step_view, "Fuse spiders", | ||
| highlight_match_pairs=[(0, 1)], | ||
| ) | ||
| proof_panel.undo_stack.push(cmd) | ||
|
|
||
| scene = proof_panel.graph_scene | ||
|
|
||
| # "Next change" semantics: on START (index 0) we show the graph at step 0 | ||
| # and highlight what will change in the transition 0 -> 1 (the fuse). | ||
| proof_panel.step_view.move_to_step(0) | ||
| # Both spiders that will be fused, and the edge between them, should be highlighted. | ||
| assert scene.is_vertex_highlighted(0) | ||
| assert scene.is_vertex_highlighted(1) | ||
| edges_01 = list(scene.g.edges(0, 1)) | ||
| assert len(edges_01) >= 1 | ||
| e = edges_01[0] | ||
| s, t = scene.g.edge_st(e) | ||
| edge_01 = (s, t, scene.g.edge_type(e)) | ||
| assert scene.is_edge_highlighted(edge_01) | ||
|
|
||
| # On the last step (index 1) there is no "next" transition, so no highlight. | ||
| proof_panel.step_view.move_to_step(1) | ||
| assert not any(scene.is_vertex_highlighted(v) for v in scene.g.vertices()) | ||
|
|
||
| # Back to START: again highlight the next change (0 -> 1). | ||
| proof_panel.step_view.move_to_step(0) | ||
| assert scene.is_vertex_highlighted(0) | ||
| assert scene.is_vertex_highlighted(1) | ||
|
|
||
| # Now simulate the user using Undo to remove the rewrite step entirely. | ||
| # When the proof returns to the START state via the undo stack, there | ||
| # should again be no rewrite highlighting at all. | ||
| proof_panel.undo_stack.undo() | ||
| assert proof_panel.step_view.currentIndex().row() == 0 | ||
| assert not any(scene.is_vertex_highlighted(v) for v in scene.g.vertices()) | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was it necessary to add type ignore here? it used to work previously.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, and thanks for catching this.
The
# type: ignore[attr-defined]here is only working around a mismatch between Qt’s runtime API and the current PySide6 type stubs. At runtime,QStyleHintsdoes providesetColorScheme(Qt.ColorScheme)starting from Qt 6.5, and the call works correctly on macOS. However, the PySide6.pyistubs in our environment only declare thecolorScheme()getter onQStyleHintsand do not yet expose the correspondingsetColorScheme(...)setter. When we enabledmypy zxlivein CI, that missing stub manifested as a false-positiveattr-definederror, even though the method is available and used in Qt’s own QML code.Because of that, the
type: ignoreis not hiding a real bug, it is just acknowledging that the stubs are currently behind the actual Qt/PySide runtime. Once the PySide6 stubs grow a propersetColorSchemedeclaration, we can safely remove this ignore.