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

[WIP] Implement relative-pointer #1274

Closed
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@arandomhuman
Copy link
Contributor

arandomhuman commented Oct 1, 2018

This is mostly a feature complete implementation of the relative-pointer protocol, but since this is my first try, I've marked it WIP for now. Suggestions on how to correct/improve code, structure, whatever, are welcome.

What works:

  • weston-resizor (mostly) works. Some eccentricity is because of issue #613; specifically (lack of) this behavior. This makes the cursor release the lock as soon as the surface is sufficiently small (i.e., the pointer location is out of bounds). The x11 backend is generating absolute events for pointer motion for me (why?), so the scaling is out of whack.
  • Neverball on Xwayland

PR Todo:

  • Add an example/append to the pointer-constraints example

Otherwise:

  • Maintain event timestamps in usecs instead of converting to msecs?

arandomhuman added some commits Sep 27, 2018

relative_pointer: create skeleton and build
Add protocol, header and type files to build. Create skeleton structs,
creator and destroyer, and define implementations.
relative_pointer: implement protocol requests
Flesh out the details of the structs, signals, callback functions, and
so on. weston-resizer silently works at this point (no events sent).
relative_pointer: implement protocol events
Implement zwp_relative_pointer_v1.relative_motion event, along with some
glue code in wlr_seat_pointer and rootston.
@@ -304,6 +305,18 @@ void wlr_seat_pointer_clear_focus(struct wlr_seat *wlr_seat);
void wlr_seat_pointer_send_motion(struct wlr_seat *wlr_seat, uint32_t time,
double sx, double sy);

/** Send relative motion events to the surface with pointer focus. Coordinates

This comment has been minimized.

@ddevault

ddevault Oct 1, 2018

Member

nit: for this and the other doc comments, move the text to the next line

/**
  * Send relative motion...
  * ...32 bits.
  */
@@ -310,6 +310,9 @@ void roots_cursor_handle_motion(struct roots_cursor *cursor,
double dx = event->delta_x;
double dy = event->delta_y;

wlr_seat_pointer_notify_relative_motion(cursor->seat->seat, (uint64_t)

This comment has been minimized.

@ddevault

ddevault Oct 1, 2018

Member

nit: put the cast next to the statement it's casting, don't split the line here

This comment has been minimized.

@ddevault

ddevault Oct 1, 2018

Member

I wonder if these calls could live in wlr_cursor?

This comment has been minimized.

@arandomhuman

arandomhuman Oct 1, 2018

Contributor

Can't opine on that yet; not too familiar with the pipeline. It has to be done after pointer focus is calculated, at least.

This comment has been minimized.

@arandomhuman

arandomhuman Oct 3, 2018

Contributor

I checked; the analogous wlr_seat_pointer_notify_motion is called only in rootston. Same treatment for this one.

*/

static void relative_pointer_manager_v1_handle_resource_destroy(struct wl_resource *resource)
{

This comment has been minimized.

@ddevault

ddevault Oct 1, 2018

Member

Opening braces go on the same line


wlr_log(WLR_DEBUG,
"relative_pointer_v1 0x%" PRIXPTR " created for client 0x%" PRIXPTR,
(uintptr_t) relative_pointer, (uintptr_t) client);

This comment has been minimized.

@ddevault

ddevault Oct 1, 2018

Member

Just use %p

assert(wl_resource_instance_of(resource, &zwp_relative_pointer_v1_interface,
&relative_pointer_v1_impl));
zwp_relative_pointer_v1_send_relative_motion(resource,
(uint32_t) (time >> 32), (uint32_t) time, wl_fixed_from_double(dx),

This comment has been minimized.

@ddevault

ddevault Oct 1, 2018

Member

nit: remove the extra space between cast and statement


void wlr_relative_pointer_v1_send_relative_motion(struct wl_resource *resource,
uint64_t time, double dx, double dy, double dx_unaccel, double
dy_unaccel)

This comment has been minimized.

@ddevault

ddevault Oct 1, 2018

Member

Don't split lines along a parameter definition

@ddevault

This comment has been minimized.

Copy link
Member

ddevault commented Oct 1, 2018

Overall this looks pretty good

wlr_seat_pointer_notify_relative_motion(cursor->seat->seat, (uint64_t)
event->time_msec, dx, dy, dx, dy);
wlr_seat_pointer_notify_relative_motion(cursor->seat->seat,
(uint64_t) event->time_msec, dx, dy, dx, dy);

This comment has been minimized.

@ddevault

ddevault Oct 1, 2018

Member

Extra space here

This comment has been minimized.

@arandomhuman

arandomhuman Oct 1, 2018

Contributor

I kinda find them dirty, but eh, your call. Formatting's all done. Let me add an example before merging.

This comment has been minimized.

@emersion

emersion Oct 1, 2018

Member

There's no need to add an example if there's already a weston example that does the same thing.

(uint32_t) (time >> 32), (uint32_t) time, wl_fixed_from_double(dx),
wl_fixed_from_double(dy), wl_fixed_from_double(dx_unaccel),
wl_fixed_from_double(dy_unaccel));
(uint32_t) (time >> 32), (uint32_t) time,

This comment has been minimized.

@ddevault

ddevault Oct 1, 2018

Member

still the extra spaces here

@arandomhuman arandomhuman force-pushed the arandomhuman:relative-pointers branch from 37069aa to b11bd7e Oct 1, 2018

Show resolved Hide resolved rootston/cursor.c Outdated
Show resolved Hide resolved types/seat/wlr_seat_pointer.c Outdated
}


void wlr_relative_pointer_v1_destroy(struct wlr_relative_pointer_manager_v1 *relative_pointer_manager) {

This comment has been minimized.

@emersion

emersion Oct 1, 2018

Member

Style: 80 char limit

This comment has been minimized.

@arandomhuman

arandomhuman Oct 3, 2018

Contributor

A lot of lines go above 80 chars because of the long variable/interface names. I'll implement a 100 char hard limit; would that do? Breaking at 80 looks awkward in a lot of places.

This comment has been minimized.

@emersion

emersion Oct 3, 2018

Member

I think @SirCmpwn wants to have a hard 80-char limit (please confirm). It looks like this:

void wlr_relative_pointer_manager_v1_destroy(
		struct wlr_relative_pointer_manager_v1 *manager) {
	…
}
@emersion
Copy link
Member

emersion left a comment

Looks pretty good.

I'm not a big fan of the list of resources in the seat (type safety? what happens if relative-pointer-v2 is introduced?), but we have the same issue with primary-selection.

arandomhuman added some commits Oct 1, 2018

relative_pointer: add relative-pointer example
On left mouse button click, locks the cursor and renders relative motion
events.
@arandomhuman

This comment has been minimized.

Copy link
Contributor

arandomhuman commented Oct 3, 2018

@emersion, I think I've finished with most of your comments. Will push changes plus an example in a bit.
Re: resources in the seat, would you rather it be named relative_pointers_v1?

@emersion

This comment has been minimized.

Copy link
Member

emersion commented Oct 3, 2018

Will push changes plus an example in a bit.

Only push an example if there's no matching Weston example.

Re: resources in the seat, would you rather it be named relative_pointers_v1?

Yeah, that seems like a good idea.

relative_pointer: implementation and code fixes
In particular, modified public creator and destructor function names,
added a display destroy listener, safely extract user data from
resources, send correct time (in usecs) in rootston, etc.

@arandomhuman arandomhuman force-pushed the arandomhuman:relative-pointers branch from b11bd7e to f2479d2 Oct 3, 2018

@arandomhuman

This comment has been minimized.

Copy link
Contributor

arandomhuman commented Oct 3, 2018

I've pushed an example nevertheless because the weston example isn't working satisfactorily (see top post). I'll remove it if required. Column limits are left; I'll get to that then squash my commits.

@emersion

This comment has been minimized.

Copy link
Member

emersion commented Oct 23, 2018

This PR needs a rebase.

@ddevault

This comment has been minimized.

Copy link
Member

ddevault commented Oct 23, 2018

Would you consider sending a patch to fix the weston example?

@clarcharr

This comment has been minimized.

Copy link

clarcharr commented Dec 9, 2018

Any updates on this? Would love to see this merged so the corresponding functionality in sway can be added.

@ddevault

This comment has been minimized.

Copy link
Member

ddevault commented Dec 9, 2018

Anyone is welcome to adopt this patch to put the finishing touches on.

@ddevault ddevault referenced this pull request Dec 9, 2018

Closed

Mouse capture support? #1430

@ForTheReallys

This comment has been minimized.

Copy link
Contributor

ForTheReallys commented Dec 9, 2018

I can pick this up. Do I just create a separate pull request?

@ddevault

This comment has been minimized.

Copy link
Member

ddevault commented Dec 9, 2018

Yes.

@emersion

This comment has been minimized.

Copy link
Member

emersion commented Jan 4, 2019

Superseded by #1432

@emersion emersion closed this Jan 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment