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

input/cursor: refactor cursor logic for reuse in tablet/touch #5254

Merged
merged 3 commits into from
May 2, 2020

Conversation

Xyene
Copy link
Member

@Xyene Xyene commented Apr 25, 2020

Fixes #5232 and hopefully other tablet bugs.

Will need rebasing after #5222 is merged.

struct sway_node *node) {
if (node && node->type == N_CONTAINER) {
// Try a node's resize edge
enum wlr_edges edge = find_resize_edge(node->sway_container, NULL, cursor);
Copy link
Member Author

Choose a reason for hiding this comment

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

Note for reviewer: the original code had

enum wlr_edges edge = find_resize_edge(node->sway_container, surface, cursor);

...but following the control flow, surface was guaranteed to always be NULL at that point.

@Xyene Xyene force-pushed the refactor-cursor-events branch 9 times, most recently from ecde737 to f5b0011 Compare April 26, 2020 01:33
@@ -220,6 +223,9 @@ bool seat_is_input_allowed(struct sway_seat *seat, struct wlr_surface *surface);

void drag_icon_update_position(struct sway_drag_icon *icon);

enum wlr_edges find_resize_edge(struct sway_container *cont,
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not super big on the semantics of having this function exported from here; if anyone has a better home for it I'm happy to move it.

@Xyene Xyene changed the title [WIP] input/cursor: refactor cursor logic for reuse in tablet/touch input/cursor: refactor cursor logic for reuse in tablet/touch Apr 26, 2020
@Xyene Xyene marked this pull request as ready for review April 26, 2020 03:31
@Xyene
Copy link
Member Author

Xyene commented Apr 26, 2020

I think this should be ready for review, but maybe not merging without a bit more testing. The commits are probably best viewed in order, rather than through the full-PR diff.

I'm running the following patches on my system, based on the latest master (which has the rest of the tablet fixes merged now):

...and hopefully if there are issues I'll run into them in the next few days. That said, these patches fix most of the issues I've personally encountered with tablets. @David96 or @imyxh, if either of you are feeling particularly brave, some more tablet user testing of these patches would go a long way 😄

Copy-pasteable commands if you choose to do so:

$ cd /path/to/sway
$ curl -L https://github.com/swaywm/sway/pull/5254.diff | patch -p1
$ curl -L https://github.com/swaywm/sway/pull/5259.diff | patch -p1

Currently, this only adds a seatop handler for tablet motion. I think we ought to have one more at least for seatop_down, so that you can move a floating tablet v2 surface by tablet pen (currently, you can't, even without this patchset) — but I think that would best be done in a separate PR.

I would also like to rename the existing button, motion, and axis handlers to pointer_button, pointer_motion etc. to be unambiguous, but the resulting diff will be quite large. @emersion if you're OK with that, I can go ahead and do it.

I think there are definitely some opportunities for reducing code duplication here, but I think it'd be best to figure out whether the current logic is buggy or not before adding abstractions to it.

@emersion
Copy link
Member

emersion commented May 1, 2020

I would also like to rename the existing button, motion, and axis handlers to pointer_button, pointer_motion etc. to be unambiguous, but the resulting diff will be quite large. @emersion if you're OK with that, I can go ahead and do it.

Yes, please do in a separate commit.

include/sway/input/seat.h Outdated Show resolved Hide resolved
sway/input/cursor.c Outdated Show resolved Hide resolved
Copy link
Member

@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.

Looks pretty good apart from these comments

This commit refactors `cursor_rebase` into `cursor_update_image`, and
moves sending pointer events to the two existing call sites. This will
enable this code to be reused for tablets.

Refs swaywm#5232
This commit moves tablet motion logic into a seatop handler.

As a side-effect of seatop implementations being able to receive
tablet motion events, fixes swaywm#5232.
@David96
Copy link
Contributor

David96 commented May 2, 2020

Is there any specific operations you would like us to test?

I just pulled your changes and am running sway with it right now - so far resizing windows appears to be smooth and from a superficial test, using the pen buttons seems to work better than ever (I have one eraser button and one that I bound to a select tool in my note taking app, the 2nd one never quite seemed to work right, now it does).

Only thing I'm not sure about: that one button seems to be supposed to work as right click? At least in FF it does. I also was able to at one point use it together with my modifier key to resize a window without grabbing its border (a 1px one in my case, quite hard to grab with the pen ^^) - but in this case the resizing wasn't quite working. As soon as the pen touched the screen it stopped and in general it was quite sluggish (maybe the offset between cursor and border was added to the window size upon resize start? Felt like something like that). And after playing around with it a bit I'm now no longer able to resize a window with this method at all. The window I was using is alacritty so I think without native tablet tool support. But this might be a general thing and not related to this pull request - I never played around with resizing windows so far, as I usually use my keyboard for that.

Also, I don't seem to be able to move windows supporting tablet tool events (tried with nemo and FF) using Mod+Pen. The events are just "eaten" by the client acting as if no modifier key was pressed.

Copy link
Member

@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.

LGTM, thanks!

@emersion emersion merged commit ae3ec74 into swaywm:master May 2, 2020
@Xyene
Copy link
Member Author

Xyene commented May 2, 2020

Thanks for testing @David96, I didn't really have much of a test plan other than "it seems to work better" :)

I just pulled your changes and am running sway with it right now - so far resizing windows appears to be smooth and from a superficial test, using the pen buttons seems to work better than ever

Great!

Only thing I'm not sure about: that one button seems to be supposed to work as right click?

I don't actually use the buttons on my pen so I've never noticed this, but I can confirm there's something weird going on here. My pen has two buttons. If I start gedit (by default, it binds tablet input) and press the "bottom" one, the right-click context menu opens. If I start GDK_BACKEND=x11 gedit, which doesn't bind tablet input, then I need to press the "top" button on my pen to trigger the same. So, there's some inconsistency there.

Also, I don't seem to be able to move windows supporting tablet tool events (tried with nemo and FF) using Mod+Pen. The events are just "eaten" by the client acting as if no modifier key was pressed.

This is since we map tool tip down to left button press, and only enter the "move" state on left button press. On a v2 surface, we don't do this mapping, so never enter the move move. I've opened #5293 to track this.

@Xyene Xyene deleted the refactor-cursor-events branch October 18, 2020 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Resizing windows with tablet pen is extremely jerky
3 participants