Added Crumble Zoom+pan & scroll#1051
Conversation
+ Added vertical scroll for timeline (right) view + changed some "magic numbers" to consts (QUBIT_HIGHLIGHT_SIZE, MAX_QUBIT_COORDINATE)
DeDuckProject
changed the title
Strilanc
left a comment
Strilanc
left a comment
There was a problem hiding this comment.
I really like this functionality, but want to tweak how it's triggered.
| } finally { | ||
| ctx.restore(); | ||
| } | ||
| return maxTimelineScrollY; |
There was a problem hiding this comment.
It's super counter-intuitive that the draw command would return maxTimelineScrollY. Provide an alternate route to get at it, like a getMaxTimelineScrollY method, and don't return it here.
Alternatively, declare a DrawSummary struct with a maxTimelineScrollY and return that. And update the docstring of this method to say it's returning that value.
There was a problem hiding this comment.
Done. added DrawSummary.
| * @param {!number} curMouseScreenY | ||
| */ | ||
| constructor(circuit, curLayer, focusedSet, timelineSet, curMouseX, curMouseY, mouseDownX, mouseDownY, boxHighlightPreview) { | ||
| constructor(circuit, curLayer, focusedSet, timelineSet, curMouseX, curMouseY, mouseDownX, mouseDownY, boxHighlightPreview, viewportX=0, viewportY=0, viewportZoom=1, timelineScrollY=0, curMouseScreenX=undefined, curMouseScreenY=undefined) { |
There was a problem hiding this comment.
Why are these new parameters defaulting to values, when the old ones weren't?
There was a problem hiding this comment.
Removed default assignments
| } | ||
|
|
||
| function handleQubitGridZoomPan(ev) { | ||
| if (ev.ctrlKey || ev.metaKey) { |
There was a problem hiding this comment.
I think the control scheme should be the following:
-
When you middle-click+drag on either of the views, dragging the mouse should pan. For the timeline view it might be best to keep the X panning locked... but there would be value in the user being able to indicate they want the current layer to be more to the left so they see further into the circuit.
-
When you scroll the mouse wheel while hovering over the timeslice view, it should zoom around the mouse pointer. (Note you can efficiently pan by zooming out with the mouse in one position then zooming in with the mouse in a new position. This, plus the middle-dragging for panning, obsoletes the need for the modifier keys.)
-
When you scroll the mouse wheel while hovering over the timeline view, it should scroll the list of qubits ...or zoom the list. I'm not sure. Scrolling makes sense from a "this is a list" perspective and zooming makes sense from "behave the same as the thing right next to you" perspective. Only way to tell would be to try it and see which felt better.
There was a problem hiding this comment.
Sure, as for (1), using the middle click+drag does makes sense. Regarding whether the timeline X panning should be locked - the implementation is definitely simpler if we keep it locked, but I agree that allowing for X scroll is something that a user might want. I think I'll give it a go and see how it feels.
(2) that makes sense for the mouse. However, I think it would make using the touchpad (which is what I tested on) unintuitive. I'd argue that using the modifier key for zoom is a better compromise, but I'll give it some more thought tomorrow. What do you think?
(3) if we do decide to go for "scroll is zoom for timeslice" in (2) - then indeed it's not clear whether scrolling for scroll would feel natural here. I will say that with current implementation (scroll is pan) it does feel natural even though it only scrolls the Y axis at the moment.
There was a problem hiding this comment.
Hm, I didn't consider using a touchpad. It would also make sense to consider pinch actions etc for users with touch screens on their laptops.
I'm fine with timeslice wheeling meaning to scroll rather than to zoom.
There was a problem hiding this comment.
Currently pinch works on my touchpad, so I assume it should work similarly on a touch screen - but I'll check.
So to summarize, I think the approach would be:
- add middle click + drag to pan both panels; As for timeline - ideally we'll add X panning as well (if it's simple enough. We'll probably want to reset X when user moves between layers for focus.
- Leaving the modifier+scroll for zoom for mouse users; touchpad users can use modifier/pinch for zoom; regular scrolling just pans.
- since scroll is pan - timeline behaviour will stay the same and feel natural; There will be no zoom in timeline panel.
I would like to suggest keeping the axes anchored to the top and left edges and adjusting the tick intervals while zooming in and out, like what I did #1050. I stole the hotkeys from Blender, though, so the interaction is different from what you had in mind. |
|
@Raycosine lol I hadn't realized I was getting notifications about two separate PRs. Keeping the ticks marks anchored does seem like the clearly better behavior. |
|
@Raycosine @Strilanc Yes, freezing the tick marks does make sense. I'll add that after the other stuff. |
+ Add X Scroll for timeline panel; Draw all layers always (instead of restricting, like before); X scroll resets on next/prev layer. + Introduce DrawSummary class to include min/max offsets for drawing timeline
…e/crumble-scroll-zoom-pan
|
@Strilanc Ok, I've completed the changes we discussed:
See updated gif in PR description for demo. |
3 participants
Added Crumble zoom,pan and scroll behaviour:
Closes #1040