Skip to content
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

Add Path tool support for the Tab key swapping to dragging the opposite handle #2058

Merged
merged 9 commits into from
Oct 30, 2024

Conversation

DaraghD
Copy link
Contributor

@DaraghD DaraghD commented Oct 20, 2024

Unsure of naming of the function

Partially closes #1870

Copy link

📦 Build Complete for 0fe4c29
https://b9a70f6c.graphite.pages.dev

Copy link
Member

@0HyperCube 0HyperCube left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good so far and works well.

Currently if both the handles from an anchor are selected, tab switches to just having a random one selected. Is this the desired behaviour? I would have thought it would be more intuitive just to keep both handles selected (since they are both opposite to a formerly selected handle). However it probably doesn't matter too much.

Also please can you add a hint for this. These are the text at the bottom of the viewport:
screenshot with hint in

Thanks for your work on this contribution so far.

@DaraghD
Copy link
Contributor Author

DaraghD commented Oct 20, 2024

Currently if both the handles from an anchor are selected, tab switches to just having a random one selected. Is this the desired behaviour? I would have thought it would be more intuitive just to keep both handles selected (since they are both opposite to a formerly selected handle). However it probably doesn't matter too much.

Switched it to be that way, I think that makes more sense

Also please can you add a hint for this. These are the text at the bottom of the viewport:

Added one, again not sure of the naming

Copy link
Member

@0HyperCube 0HyperCube left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well and the code looks good. Thanks again for working on this.

@DaraghD
Copy link
Contributor Author

DaraghD commented Oct 21, 2024

Clippy failed on a ton of stuff that I didn't touch, is that normal?

@0HyperCube
Copy link
Member

@DaraghD the clippy lints are fixed by a395fbf.

@0HyperCube 0HyperCube enabled auto-merge (squash) October 25, 2024 21:09
@Keavon Keavon disabled auto-merge October 25, 2024 21:12
@Keavon
Copy link
Member

Keavon commented Oct 25, 2024

I'll do a QA review on this before we merge it. It's in my queue that I'm working through presently.

@Keavon Keavon force-pushed the switch_handle_shortcut branch from ae3523d to 700b323 Compare October 26, 2024 07:08
@Keavon
Copy link
Member

Keavon commented Oct 26, 2024

@DaraghD some QA feedback:

  1. Upon testing this, I think the best near-term solution for the question of cursor alignment is to enable cursor lock so the user's cursor disappears, but don't yet attempt to render a fake cursor. (Since unfortunately, I'm finding that the visibility of the misaligned cursor is too confusing.) Could you please add that part? You can reference how we do this in the NumberInput.svelte slider drag, for example. That's all frontend though, and this'll need to plumbed in from the backend. You could also look at the code paths which let the backend plumb to the frontend which cursor style to display, since this would be similar and could potentially be included in those code paths.
  2. When dragging an anchor around, but with no handles selected, it looks like hitting Tab still makes it swap by setting one of the handles to active. We don't want that happening, where an anchor makes a handle become selected. Only swapping should happen. Could you please fix that? And the hints bar input should also be responsive to that situation where hitting Tab would do nothing because no handles are selected (only anchors). But if handles and anchors are mixed, then Tab does do something (swaps the handles, not affecting anchors with no selected handles) so the hint should appear in that case. Please also fix the typo in the hint (three p's).
  3. You didn't cause this one but it's related to the selection aspect and I'd appreciate if you can fix it: when aborting the drag of a point or points with a right click or Escape, it causes the points that were selected at the beginning of the drag to all get deselected. We want to restore the point selection so it has the same set as when the drag began.

Thanks!

@Keavon Keavon marked this pull request as draft October 26, 2024 18:54
@DaraghD DaraghD closed this Oct 27, 2024
@DaraghD DaraghD force-pushed the switch_handle_shortcut branch from 700b323 to 9eeefaa Compare October 27, 2024 16:48
@DaraghD DaraghD reopened this Oct 27, 2024
@DaraghD
Copy link
Contributor Author

DaraghD commented Oct 28, 2024

@DaraghD some QA feedback:

1. Upon testing this, I think the best near-term solution for the question of cursor alignment is to enable cursor lock so the user's cursor disappears, but don't yet attempt to render a fake cursor. (Since unfortunately, I'm finding that the visibility of the misaligned cursor is too confusing.) Could you please add that part? You can reference how we do this in the NumberInput.svelte slider drag, for example. That's all frontend though, and this'll need to plumbed in from the backend. You could also look at the code paths which let the backend plumb to the frontend which cursor style to display, since this would be similar and could potentially be included in those code paths.

2. When dragging an anchor around, but with no handles selected, it looks like hitting Tab still makes it swap by setting one of the handles to active. We don't want that happening, where an anchor makes a handle become selected. Only swapping should happen. Could you please fix that? And the hints bar input should also be responsive to that situation where hitting Tab would do nothing because no handles are selected (only anchors). But if handles and anchors are mixed, then Tab does do something (swaps the handles, not affecting anchors with no selected handles) so the hint should appear in that case. Please also fix the typo in the hint (three `p`'s).

3. You didn't cause this one but it's related to the selection aspect and I'd appreciate if you can fix it: when aborting the drag of a point or points with a right click or Escape, it causes the points that were selected at the beginning of the drag to all get deselected. We want to restore the point selection so it has the same set as when the drag began.

All these are now done, with small exception of the pointer lock as discussed- for now its just hidden through changing the cursor to none.
I took care of some of the TODOs aswell as they were similar to what I was doing with the responsive hints for swapping handles. I probably broke a few things so ill try test as much as I can in the meantime.

@DaraghD DaraghD marked this pull request as ready for review October 28, 2024 21:11
@DaraghD DaraghD requested a review from 0HyperCube October 28, 2024 22:19
Added specific handle hints,
Can no longer switch to handle if just anchor is selected
typo fix
A pointerlock implementation would be ideal in the future to keep the screen from panning,
@Keavon Keavon force-pushed the switch_handle_shortcut branch from 5aaae31 to e1d7dc3 Compare October 30, 2024 02:19
@Keavon
Copy link
Member

Keavon commented Oct 30, 2024

!build

Copy link

📦 Build Complete for e1d7dc3
https://0f95cd1d.graphite.pages.dev

@Keavon
Copy link
Member

Keavon commented Oct 30, 2024

This will be ready to merge once GitHub fixes their CI outage and I can make it re-run to pass CI, which should be when I wake up.

@Keavon Keavon changed the title Tab swaps to dragging the opposite handle Add Path tool support for the Tab key swapping to dragging the opposite handle Oct 30, 2024
@Keavon Keavon merged commit 018e983 into GraphiteEditor:master Oct 30, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking Issue: Pen and Path tool improvements
3 participants