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

Added Alt/Option key binding to lines to extend/pivot from center #1298

Closed
wants to merge 3 commits into from

Conversation

Tiomat85
Copy link
Contributor

@Tiomat85 Tiomat85 commented Jan 2, 2025

Issue #961

Alt/Option mode when dragging the ends of a line control. Added a defined pivot point in the code to use as the rotation centre and refactored accordingly.

Alt/Option mode when dragging the ends of a line control.
Added a defined pivot point in the code to use as the rotation centre and refactored accordingly.
@Ion-e
Copy link

Ion-e commented Jan 2, 2025

#961

@cmeyer
Copy link
Collaborator

cmeyer commented Jan 2, 2025

Can you add a test case for this in test_dragging_regions in Graphics_text.py? This is a big messy method, but should provide a good starting point.

Added explicit unit tests for line graphic dragging.
Test 1.1 verifies dragging start point works.
Test 1.2 verifies dragging end point works.
Test 2 verifies dragging with Alt pressed causes the other end to mirror the delta, to stretch/move around the central point of the line.
Test 3 verifies the Shift modifier works by dragging 'close' to the vertical and see the line snaps to vertical.
@Tiomat85
Copy link
Contributor Author

Tiomat85 commented Jan 2, 2025

Can you add a test case for this in test_dragging_regions in Graphics_text.py? This is a big messy method, but should provide a good starting point.

I was in the middle of writing those test cases while waiting for Kev/Luke to get a chance to check over the code. I have done them in a new method instead, test_line_dragging, as the existing methods looked quite complex and long already. Currently it has 4 tests added, dragging both ends with no modifiers and then just dragging the start with the alt and shift modifiers.

Copy link
Contributor

@KRLango KRLango left a comment

Choose a reason for hiding this comment

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

The changes look good to me, but as a more general comment, there is a lot of duplicated code between the part[0] == "start" and part[0] == "end" cases - primarily the shift modifier handling - that existed prior to this work. This could be pulled out into a function to remove this duplication.

@Tiomat85 Tiomat85 marked this pull request as ready for review January 3, 2025 14:58
Copy link
Collaborator

@cmeyer cmeyer left a comment

Choose a reason for hiding this comment

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

This overall looks nice - thanks!

However, I'd request one change to make it behave more consistently: the end point / center point should not change after the first click. Right now if you start dragging with the alt-key, then let go of the alt-key, it uses the "start" point at the time you let go of the alt-key rather than the original start point. I think it should use the original. Right now, if you press/release the alt key while dragging, you can make the line walk around the image and it's impossible to know even where it started.

Making it remember the first start point and first center point will cause some jumping around when you press and release the alt key, but I think this is a better way to do it since the user may want to try the alt-key and see what happens.

Making this change would make it work like the rectangle resizing works, too. You can see how it works by experimenting with the rectangle tool.

As discussed in PR changed behaviour to return to previous 'pivot' position when alt is released to match Rectangle behaviour.
Also refactored to remove duplicated code for the different ends.
@Tiomat85
Copy link
Contributor Author

Tiomat85 commented Jan 6, 2025

Updated it based on that. It feels a little weird behaviourally to have the line 'jump' when you release alt but it does match the rectangle and some other pieces of software so it seems that weirdness is the standard. Also used this as an excuse to address Kev's point on the almost-identical sections for the different ends of the line.

@cmeyer
Copy link
Collaborator

cmeyer commented Jan 13, 2025

Squashed, added changes.json entry, and merged here 3f900f9

Additional modification here b74b80f

@cmeyer cmeyer closed this Jan 13, 2025
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.

5 participants