Fix TikZ copy/paste to preserve parameter metadata across windows#433
Fix TikZ copy/paste to preserve parameter metadata across windows#433Bumsparkle wants to merge 13 commits intozxcalc:masterfrom
Conversation
|
Thanks for the implementation! |
|
Fixed my implementation. Yes, Tikzit copy/paste still works. The clipboard still contains plain TikZ text (mime.setText(...)), so Tikzit continues to recognize it. Ctrl+Shift+C / Ctrl+Shift+V are still meaningful and kept as TikZ-only copy/paste paths. Also, I noticed someone else has opened a related PR sooner, so happy to close this one. |
|
Hi @Bumsparkle, thanks for fixing the 2nd issue Razin pointed out in his most recent comment above, which is that the variable viewer is no longer empty when pasting between tabs of the same window. |
|
Thanks for testing and for the detailed report. I’ve now fixed the remaining issue: boolean variable types are preserved on copy/paste between windows and between tabs (normal Ctrl+C / Ctrl+V), and no longer get converted to parametric. |
|
I’ve resolved the merge conflicts and re-tested the boolean variable type issue: Ctrl+C/Ctrl+V preserves Boolean vs Parametric types across windows and tabs. |
|
Hi @Bumsparkle can you please clean up the PR? There are many changes that are not relevant to the issue; I believe they come from merging incorrectly from the main branch. Can you also make the checks pass? |
|
I've tested that in ZXLive, copy paste works both with and without Shift, on both different tabs and different windows, preserving the parameter metadata. The tikz copy (Ctrl/Command + Shift + C in ZXLive) didn't work pasting (Ctrl/Command + V) into Tikzit as-is, which thankfully has a one-line fix in the above commit. Tikzit doesn't preserve the metadata, but that's a feature that could be added to tikzit's parser that I would say isn't necessary for this unitary design bounty. |
|
I’ve finished cleaning up this PR and rebased it so it now only contains the copy/paste variable-type fix (removed unrelated merge-noise files and resolved conflicts). Could someone with write access please re-run the failed [test (3.11)] job? The failure is in Setup Audio (apt-get install pulseaudio) with Ubuntu mirror 404s, so I think it looks infrastructure-related rather than a code/test failure. |
|
Thanks for cleaning up. We are looking into why setup audio is failing; this is happening across all PRs. |
|
I am looking at the code now and I am a bit confused by the |
|
Good point, I’m cautious about over-claiming this as purely a PyZX bug. From what I found, ZXLive’s copy/paste path (subgraph_from_vertices + merge/update), boolean type info was not reliably preserved, which caused variables to appear as parametric after paste. copy_variable_types() is then a defensive fix in ZXLive to keep behaviour stable for users right now. I agree the cleaner long-term fix is likely to preserve this metadata in PyZX for those graph operations, then simplify/remove the ZXLive workaround. |
|
I see, thanks. Can you document this somewhere in the code so that we can remove the workaround once it has been fixed in PyZX? And do you mind creating an issue in the PyZX repo with a minimal example? |
|
I’ve added a TODO in ZXLive that its a temporary workaround and opened PyZX issue zxcalc/pyzx#408 with a minimal repro. |
|
Hello, it looks like the PyZX issue is already closed (so fast, within 4 hours after you created the issue😅). Do you mind having a look at your PR again and see if the temporary workaround can now be removed? Sorry, I was thinking of leaving the workaround but since PyZX is already fixed, it might be worth cleaning it up before the PR is merged. |
|
Hey, no worries, I've removed the temporary ZXLive-side variable type workaround |
zxlive/common.py
Outdated
| def get_variable_types(graph: GraphT) -> dict[str, bool]: | ||
| """Returns variable type metadata, falling back to symbolic phase vars.""" | ||
| variable_types = { | ||
| str(name): bool(graph.var_registry.get_type(name, default=False)) | ||
| for name in graph.var_registry.vars() | ||
| } | ||
|
|
||
| for v in graph.vertices(): | ||
| phase = graph.phase(v) | ||
| if not hasattr(phase, "free_vars"): | ||
| continue | ||
| try: | ||
| for var in phase.free_vars(): | ||
| name = str(var) | ||
| inferred = bool(getattr(var, "is_bool", False)) | ||
| if name not in variable_types: | ||
| variable_types[name] = inferred | ||
| else: | ||
| variable_types[name] = variable_types[name] or inferred | ||
| except Exception: | ||
| continue | ||
|
|
||
| return variable_types |
There was a problem hiding this comment.
Is this method necessary now that this is fixed in PyZX? Would something break if you simplified to this?
| def get_variable_types(graph: GraphT) -> dict[str, bool]: | |
| """Returns variable type metadata, falling back to symbolic phase vars.""" | |
| variable_types = { | |
| str(name): bool(graph.var_registry.get_type(name, default=False)) | |
| for name in graph.var_registry.vars() | |
| } | |
| for v in graph.vertices(): | |
| phase = graph.phase(v) | |
| if not hasattr(phase, "free_vars"): | |
| continue | |
| try: | |
| for var in phase.free_vars(): | |
| name = str(var) | |
| inferred = bool(getattr(var, "is_bool", False)) | |
| if name not in variable_types: | |
| variable_types[name] = inferred | |
| else: | |
| variable_types[name] = variable_types[name] or inferred | |
| except Exception: | |
| continue | |
| return variable_types | |
| def get_variable_types(graph: GraphT) -> dict[str, bool]: | |
| """Returns variable type metadata.""" | |
| variable_types = { | |
| str(name): bool(graph.var_registry.get_type(name, default=False)) | |
| for name in graph.var_registry.vars() | |
| } | |
| return variable_types |
There was a problem hiding this comment.
Don't think anything breaks. Variable types are now taken only from var_registry, which matches PyZX.
zxlive/common.py
Outdated
| def _coerce_bool(value: object) -> bool: | ||
| if isinstance(value, bool): | ||
| return value | ||
| if isinstance(value, str): | ||
| return value.strip().lower() in ("true", "1", "yes", "on") | ||
| return bool(value) |
There was a problem hiding this comment.
is this method necessary because the from_tikz method will only interact with the metadata encoded by you in _encode_tikz_metadata method? Couldn't you have just done bool(is_bool) instead of _coerce_bool(is_bool)?
There was a problem hiding this comment.
You’re right _coerce_bool was just defensive, but as _from_tikz only ever sees data produced by _encode_tikz_metadata, I’ll simplify this to bool(is_bool).
zxlive/common.py
Outdated
| def normalize_symbolic_phase_types(graph: GraphT) -> None: | ||
| """Reparse symbolic phases so Var.is_bool matches graph.var_registry.""" | ||
| for v in graph.vertices(): | ||
| if graph.type(v) not in (VertexType.Z, VertexType.X): | ||
| continue | ||
| phase = graph.phase(v) | ||
| if not hasattr(phase, "free_vars"): | ||
| continue | ||
| try: | ||
| if not phase.free_vars(): | ||
| continue | ||
| graph.set_phase(v, string_to_phase(str(phase), graph)) | ||
| except Exception: | ||
| continue |
There was a problem hiding this comment.
Why do we need to parse again and which part of the code makes sure that Var.is_bool matches graph.var_registry? Would graph.rebind_variables_to_registry not suffice instead of normalize_symbolic_phase_types?
There was a problem hiding this comment.
Yes graph.rebind_variables_to_registry would work.
zxlive/mainwindow.py
Outdated
| def _parse_bool(value: object) -> bool: | ||
| if isinstance(value, bool): | ||
| return value | ||
| if isinstance(value, str): | ||
| return value.strip().lower() in ("true", "1", "yes", "on") | ||
| return bool(value) |
There was a problem hiding this comment.
This is the same code is _coerce_bool and as I mentioned there, I am not sure if this is really necessary?
There was a problem hiding this comment.
Yep, same story as _coerce_bool, just defensive, I’ve simplified it to bool(v) and removed _parse_bool.
zxlive/mainwindow.py
Outdated
| if include_internal: | ||
| payload = json.dumps({ | ||
| "graph_json": graph.to_json(), | ||
| "variable_types": get_variable_types(graph), |
There was a problem hiding this comment.
is it necessary to store variable types seperately for json? I thought json from pyzx already includes the correct variable types
There was a problem hiding this comment.
Yes. PyZX’s graph_to_dict already puts variable_types in the graph JSON, and from_json restores the registry. Storing it again in the clipboard was redundant.
| def normalize_symbolic_phase_types(graph: GraphT) -> None: | ||
| """Ensure phase variables are consistent with the graph's var_registry.""" | ||
| # Prefer the library helper if it's available. | ||
| rebind = getattr(graph, "rebind_variables_to_registry", None) | ||
| if callable(rebind): | ||
| rebind() |
There was a problem hiding this comment.
This method just calls rebind_variables_to_registry in a complicated way. Can you delete this method and replace the instances of normalize_symbolic_phase_types(g) with g.rebind_variables_to_registry?
|
@Bumsparkle Thanks for the changes. Looking at the code now, I realized that the changes made in this PR are of two types:
The first one should really be in PyZX rather than ZXLive and ZXLive should expect the PyZX methods to preserve metadata appropriately. It turns out that similar issues were open in PyZX already but I didn't realize that they are related. I think it would be better to move your code related to the first point to PyZX and that would also end up fixing the existing PyZX issue zxcalc/pyzx#367 |
|
@RazinShaikh That makes a lot of sense. I’m going to be away for today and tomorrow morning, but I’ll get started on splitting these changes out this Sunday late afternoon if that's okay? |
|
No worries, I believe the issues need to be closed by Monday end of day for the hackathon |


Issue summary:
Copying/pasting ZX diagrams would drop parameter metadata.
Root cause:
Fix summary:
Manual verification:
Issue link
Fixes #363