Conversation
nabbelbabbel
requested review from
billti,
idavis,
minestarks and
swernli
as code owners
source/widgets/js/render_svg.mjs
Outdated
| // ---- Circuit (imperative DOM via qviz) ---- | ||
| case "Circuit": { | ||
| // qviz.draw() needs a real DOM. Use jsdom. | ||
| const { JSDOM } = await import("jsdom"); |
There was a problem hiding this comment.
This feature assume a) Node is on the users PATH (and at least it gives an error message if not), and b) That jsdom is available as a package to import in node (which it isn't by default, the user would need to install it into the global (or local) node environment. This seems fragile and likely to fail unless users have followed some specific setup instructions first. (We also don't want to bundle JSDOM into our package as it's huge).
Not sure what to do about this, but worth thinking about.
There was a problem hiding this comment.
Agree, an initial version had JSDOM as dependency but I removed it for the reason you stated. I dont think I can get rid of the Node requirement if I want to allow rendering without actually displaying the widget. But I can shim/mock the few parts of JSDOM we need.
There was a problem hiding this comment.
I added the shim, but its 500 lines of extra stuff, thoughts?
source/widgets/js/render_svg.mjs
Outdated
| } | ||
|
|
||
| // ---- Circuit (imperative DOM via qviz) ---- | ||
| case "Circuit": { |
There was a problem hiding this comment.
Did you try this at all? Thinking through how this works, you wouldn't get what's displayed on screen (unless you've not interacted with it). You'll just get the default new rendering with a circuit, which now has all blocks/calls/operations collapsed. If the user has drilled into operations to expand them and show the gates etc., that's not going to show up here, as it's re-rendering the circuit from a clean state, not the current UX state.
There was a problem hiding this comment.
Yes that was the intended behavior (for now).
The point of these functions was not so save the user-altered state of the widget into an SVG. (Though that would be even better!)
The idea was to be able to make the widget class, call the SVG dump and never need to look at it to extract an SVG to later view/use.
Will see if I can get it to work using the actual state of the widget.
There was a problem hiding this comment.
I have now added functionality to use the actual state of the widgets but still allowing a different path to render without the widget being displayed to the user.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| /** | ||
| * Detect whether the host background is dark or light by sampling the | ||
| * computed background-color of the nearest ancestor with one. | ||
| * Returns a high-contrast colour for selection outlines. |
There was a problem hiding this comment.
Why not use CSS values to match the current theme like the other widgets?
There was a problem hiding this comment.
The current version should fix this but have generic fall backs in case there is no theme.
source/widgets/js/index.tsx
Outdated
| clone.querySelectorAll(stripSelector).forEach((n) => n.remove()); | ||
| } | ||
| // Remove interactive-only elements common across widgets | ||
| clone |
There was a problem hiding this comment.
I think this is an interesting approach (cloning and serializing the SVG), but this implementation seems hard to maintain and fragile. Below you are baking knowledge of the internals of various widgets into this one central location outside of the widgets. This would mean if we ever touch those widgets (e.g. rename the .menu-icon class or #menu id or some of the other classes or styles hard-coded below, we'd need to remember to come here and change it also. That would be easy to miss and introduce bugs we wouldn't find for a while (being automated testing for this is hard)
I'm also not a big fan of automatically watching for changes and cloning the svg on mutation, being the SVG can be really huge (esp. for circuits, and likely for chord diagrams), and the vast majority of time people aren't going to use the 'save the svg feature'.
Is there a way to push the 'cleaning' of the SVG/DOM to the owning widget (so it's easy to see when changing the widget itself), and only clone & serialize the SVG when the user actually wants to save the SVG?
There was a problem hiding this comment.
Fully agree, this is all an artifact of the idea that I outlined below with your question about the second svg export route.
source/widgets/js/render_svg.mjs
Outdated
| import circuitCss from "../../npm/qsharp/ux/qsharp-circuit.css"; | ||
|
|
||
| const input = readFileSync(0, "utf-8"); // stdin | ||
| const { component, props } = JSON.parse(input); |
There was a problem hiding this comment.
If you're cloning and serializing the SVG above now, why do you need to shell out to node and render the component via the prop at all? What's the use-case for this file now?
There was a problem hiding this comment.
The use case I have in mind is not having a browser/notebook/vscode instance at all.
Essentially thinking about someone running qdk-chemistry or any other component on a remote HPC system and wanting to generate a picture to quickly see if a calculation did what it was supposed to.
If I understand correctly, anywidget uses the DOM of whatever the widget is shown in, meaning that if there is not VSCode/jupyterlab/etc, there also is no DOM to render the SVG with.
3 participants
Note: This PR is a lot of AI generated infra to achieve an outcome for testing purposes. While the features work, the way they are done needs some serious work and rethinking.
Adds:
Widget with active space selection (gold):
Widget while hovering individual orbital/entangled node:
