Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix click and drag functionality overlap in path tool in extended select #2395

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

singhutsav5502
Copy link
Contributor

@singhutsav5502 singhutsav5502 commented Mar 7, 2025

Resolves https://discord.com/channels/731730685944922173/881073965047636018/1347408498211815434

Send Path_tool FSM to Dragging state on MouseDown if clicking an already selected point.

@Keavon
Copy link
Member

Keavon commented Mar 7, 2025

!build

Copy link

github-actions bot commented Mar 7, 2025

📦 Build Complete for 773ce48
https://a76df4e9.graphite.pages.dev

@Keavon Keavon requested a review from 0HyperCube March 7, 2025 21:37
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.

This removes the functionality of removing a point from the selection by shift clicking on it.

@Keavon Keavon marked this pull request as draft March 8, 2025 05:53
@0HyperCube
Copy link
Member

Thanks for the fix @singhutsav5502. However now you can't shift select points in the first place (they are selected on mouse down and deselected on mouse up).

shift_select_issue.mp4

@singhutsav5502
Copy link
Contributor Author

Hi, it seemed to be working on my local build at first but broke when i tried it on a new document. I have pushed some changes and tried running it on a new document, seems to be working fine now. @0HyperCube
Though the storing of previously selected points could negatively affect performance, do tell me if there's anything i overlooked.

@Keavon
Copy link
Member

Keavon commented Mar 9, 2025

My intuition is that this issue doesn't need to involve storing any new state such as the list of originally selected points just to filter out a Shift-click-and-drag, but do you agree @0HyperCube?

@singhutsav5502
Copy link
Contributor Author

Honestly i feel the same, will give it a try. This can be treated as a temporary solution at best.

…lready selected, when DragStop state is reached instead of inside the mouse_down function.
@singhutsav5502
Copy link
Contributor Author

Hi, @Keavon @0HyperCube latest code push uses a boolean value instead of storing a state snapshot of the previous state selections.

@0HyperCube
Copy link
Member

My intuition is that this issue doesn't need to involve storing any new state such as the list of originally selected points just to filter out a Shift-click-and-drag, but do you agree @0HyperCube?

In the current main branch, the shift click to deselect happens on pointer down. At this point we obviously don't know if the user is going to drag or not.

I would probably just move the deselect behaviour to the mouse up and only run it if there was no dragging. However there are many possible ways to solve the problem.

@singhutsav5502
Copy link
Contributor Author

I would probably just move the deselect behaviour to the mouse up and only run it if there was no dragging. However there are many possible ways to solve the problem.

That's similar to what i have done in the last code push, while there doesn't seem to be a mouse_up, there is a DragStop event, that's where i have decided to put the logic of deselect when drag didn't occur. ( though i did end up using a boolean variable to make sure we didn't end up deselecting the points that was just selected ).

@Keavon
Copy link
Member

Keavon commented Mar 12, 2025

I would probably just move the deselect behaviour to the mouse up and only run it if there was no dragging.

I suggest going with this. Thanks both of you!

@Keavon
Copy link
Member

Keavon commented Mar 17, 2025

Checking in on the status.

@singhutsav5502
Copy link
Contributor Author

singhutsav5502 commented Mar 17, 2025

Hi, @Keavon i was planning to hold of on any new commits until i am done coming up with a decent GSOC proposal. ( I do plan to make some draft PRs related to the areas of the proposals to get an idea of the expected code implementations. )
Same for #2423

@singhutsav5502
Copy link
Contributor Author

singhutsav5502 commented Mar 17, 2025

I would probably just move the deselect behaviour to the mouse up and only run it if there was no dragging. However there are many possible ways to solve the problem.

Also, regarding this @Keavon @0HyperCube ,
The current implementation i have pushed is logically the same as this.
The only difference is that i had to include a boolean value since otherwise it would be impossible to differentiate between a node that was Shift Clicked for selection ( drag didn't occur ) and an Unselected node that was Clicked-> got selected on mouseDown -> Not Dragged -> Hence Unselected on Mouse Up.
This would create a situation where Newly selected nodes will get immediately Unselected if they are not dragged.
This is why i have used the boolean last_clicked_point_was_selected to make sure that the last clicked point was already selected Prior to the Mouse Down and hence needs to be unselected if a drag didn't occur on Mouse up ( in extended select mode )

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants