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.

Sign up for GitHub

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

Jump to bottom

Move selection on space keypress #35

Merged
merged 1 commit into from
Aug 1, 2019
Merged

Move selection on space keypress #35

merged 1 commit into from
Aug 1, 2019

Conversation

LeonardDrs
Copy link

@LeonardDrs LeonardDrs commented Apr 7, 2019 •
edited
Loading

Hi,

This is a feature add.

When dragging the selection, you can use the modifier space key to move your selection around

Video:
output
Result on second screen (not from the same recording and my screens aren't aligned):
output2

LeonardDrs
LeonardDrs commented Apr 7, 2019
View reviewed changes
LeonardDrs
LeonardDrs commented Apr 7, 2019
View reviewed changes
@emersion
Copy link
Owner

Can you explain what this feature is? I'm not familiar with macOS.

@LeonardDrs
Copy link
Author

LeonardDrs commented Apr 12, 2019 •
edited
Loading

Oh yeah my bad.

When taking a screenshot, you can select a part of the screen as well. During this selection, you can also use the modifier space key to move your selection around whilst the mouse button is pressed.

Video:
output
Result on second screen (not from the same recording):
output2

Also on mac, if you press space without holding the mouse button, it highlights the app you are currently hovering, and you can screen it with a click.
I left that part out since the PR #28 is around, and I don't know how to gather information on the app/panel coordinates under the cursor. I could give a try but I have no idea how.

@LeonardDrs
Copy link
Author

LeonardDrs commented Jun 25, 2019 •
edited
Loading

Hey @emersion!
Just saw that the PR I had conflict with was merged, that's nice 😄

I still have a couple of comments, let me know if you need anything else!

LeonardDrs
LeonardDrs commented Jun 25, 2019
View reviewed changes
LeonardDrs
LeonardDrs commented Jun 26, 2019
View reviewed changes
main.c Outdated
seat->selection.height = abs(seat->y - seat->pressed_y);
break;
case WL_POINTER_BUTTON_STATE_PRESSED: {
int32_t anchor_x = max(0, seat->anchor_x);
Copy link
Author

@LeonardDrs LeonardDrs Jun 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I'm doing the max here and not l.100 because we don't want the selection to be cropped if the user goes out of bounds

Copy link
Owner

Choose a reason for hiding this comment

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

You mean you want to prevent the selection to go out of bounds?

The same needs to be done for the upper bound, I suppose.

Copy link
Author

@LeonardDrs LeonardDrs Jul 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I want the selection to go out of bound. I just don't want it to be cropped.
Example: I'm doing a 100x100 selection and I want to move it to the 0,0 position.
If I go out of bounds atop of the screen by 10px, I don't want it to become a 100x90 because I feel it frustrating having to go through the resizing part of the selection again.

And if you don't max you could end up with a wrong anchor.

Copy link
Author

Choose a reason for hiding this comment

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

An image is worth a thousand words:

No max:
output-no-max

max-ing when setting the anchor:
output-crop

@emersion
Copy link
Owner

Hmm, sorry for the delay. I'll have a look at this PR soon.

@LeonardDrs
Copy link
Author

Haha don't worry I'm just happy to contribute ☺️

emersion
emersion requested changes Jul 25, 2019
View reviewed changes
Copy link
Owner

@emersion emersion left a comment

Choose a reason for hiding this comment

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

Hi again! Thanks for your patience, here are a few comments :)

main.c Outdated
result->y = min(seat->pressed_y, seat->y);
result->width = abs(seat->x - seat->pressed_x);
result->height = abs(seat->y - seat->pressed_y);
int32_t anchor_x = max(0, seat->anchor_x);
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, why is this required?

Copy link
Author

@LeonardDrs LeonardDrs Jul 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

If you release the selection while out of bound - because you want to start at position 0,0 for example - you don't want to select the out of bound part of your screen.

main.c Outdated
seat->selection.height = abs(seat->y - seat->pressed_y);
break;
case WL_POINTER_BUTTON_STATE_PRESSED: {
int32_t anchor_x = max(0, seat->anchor_x);
Copy link
Owner

Choose a reason for hiding this comment

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

You mean you want to prevent the selection to go out of bounds?

The same needs to be done for the upper bound, I suppose.

@LeonardDrs
Copy link
Author

LeonardDrs commented Jul 25, 2019 •
edited
Loading

@emersion 😱 I got an issue with the following commit:

Use delta instead of multiples variables

Somehow, if I move around my selection too fast between two monitors, it gets resized 😱
output
(End of first monitor on the left)

Tried moving fast on the same monitor, or out of bound and back, it's fine.

I cannot reproduce this bug with the previous version of the build I did.

Any idea?

@emersion
Copy link
Owner

emersion commented Aug 1, 2019

Thanks for the update, the code looks much better now :D

I've tried to debug this, and after figuring out that the size of the box doesn't change between the beginning and the end of pointer_handle_motion (which is what we want), I figured out there is another place where we set seat->x and y: in pointer_handle_enter. This is the source of the bug: the function sets seat->{x,y} without updating the anchor.

I'd suggest adding a move_pointer function that updates the coords and the anchor. What do you think?

emersion
emersion reviewed Aug 1, 2019
View reviewed changes
main.c Outdated
int y = wl_fixed_to_int(surface_y) + seat->current_output->logical_geometry.y;

if (seat->state->edit_anchor) {
seat->anchor_x = seat->anchor_x + (x - seat->x);
Copy link
Owner

Choose a reason for hiding this comment

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

This can be simplified to

seat->anchor_x += x - seat->x;

Copy link
Author

Choose a reason for hiding this comment

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

Yep forgot it when I was trying to debug 😅 Thanks

@emersion
Copy link
Owner

emersion commented Aug 1, 2019

After these changes (and a rebase), this PR will be good to merge.

@LeonardDrs
Copy link
Author

Now that you gave me the answer, It seems pretty obvious.
I feel dummy, should have debugged better!

Thanks a lot 😄

move_pointer is neat indeed, hope I didn't made any style mistake...

emersion
emersion approved these changes Aug 1, 2019
View reviewed changes
Copy link
Owner

@emersion emersion left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, this is good to go! Again, I'm sorry it took so long.

@emersion emersion merged commit db6bc53 into emersion:master Aug 1, 2019
@LeonardDrs
Copy link
Author

No issue whatsoever! I love using sway and its ecosystem 😄

@LeonardDrs LeonardDrs deleted the edit-anchor-position branch August 1, 2019 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emersion emersion emersion approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@LeonardDrs @emersion