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

Implement pointer-constraints-unstable-v1 protocol #852

Merged
merged 17 commits into from Sep 27, 2018

Conversation

Projects
None yet
10 participants
@Laaas
Copy link
Contributor

Laaas commented Apr 8, 2018

Fixes #686

@Laaas

This comment has been minimized.

Copy link
Contributor

Laaas commented Apr 8, 2018

Easy test case: https://github.com/Laaas/hello_wayland

Also: How would I go about matching on wl_pointer, i.e. get the wl_pointer corresponding to a wlr_cursor?

@ddevault

This comment has been minimized.

Copy link
Member

ddevault commented Apr 8, 2018

Easy test case: https://github.com/Laaas/hello_wayland

Can you add a test client to examples/ instead?

@ddevault

This comment has been minimized.

Copy link
Member

ddevault commented Apr 8, 2018

Also: How would I go about matching on wl_pointer, i.e. get the wl_pointer corresponding to a wlr_cursor?

You cannot, you have to get more creative with it. Where specifically do you need it?

@Laaas

This comment has been minimized.

Copy link
Contributor

Laaas commented Apr 8, 2018

I need it @ rootston/cursor.c:329.

Right now if there are multiple cursors, and the client tries to lock just one of them, both cursors will be locked (if they are both in the region). Preferably, just the cursor that was requested to be locked should be locked.

Although I may have misunderstood the whole wl_pointer concept, but as I see it:
On one client each wlr_cursor corresponds to a wl_pointer.

.gitmodules Outdated
@@ -0,0 +1,3 @@
[submodule "examples/pointer-constraints-demo-client"]
path = examples/pointer-constraints-demo-client
url = https://github.com/Laaas/hello_wayland.git

This comment has been minimized.

@ddevault

ddevault Apr 8, 2018

Member

Haha, this isn't going to cut it. Look at some of the other example clients, like idle or layer-shell.

@@ -290,18 +294,128 @@ static void roots_cursor_press_button(struct roots_cursor *cursor,
}
}

void roots_cursor_handle_motion(struct roots_cursor *cursor,
// TODO: Match on wl_pointer
static void cursor_constrain(struct roots_cursor *roots_cur, void *data,

This comment has been minimized.

@ddevault

ddevault Apr 8, 2018

Member

80 column limit

This comment has been minimized.

@ddevault

ddevault Apr 8, 2018

Member

What is the purpose of this function?

// used when there is no specific region in the constraints
struct wlr_box sbox = {
.x = 0,
.y = 0,

This comment has been minimized.

@ddevault

ddevault Apr 8, 2018

Member

Don't align things like this (or at all)

wl_list_for_each(view, &desktop->views, link) {
if (view->wlr_surface == wlr_surface) break;
}
if (!view) return;

This comment has been minimized.

@ddevault

ddevault Apr 8, 2018

Member

braces are mandatory

} else if (!wlr_box_contains_point(box, sx + deltas[0], sy + deltas[1])) {
deltas[0] = 0, deltas[1] = 0;
}
}

This comment has been minimized.

@ddevault

ddevault Apr 8, 2018

Member

It would be better to have wlr_cursor support taking a constraint - it already supports this via wlr_cursor_map_to_region.

||
wl_resource_instance_of(resource, &zwp_locked_pointer_v1_interface,
&locked_pointer_impl)
);

This comment has been minimized.

@ddevault

ddevault Apr 8, 2018

Member

Not fond of this indentation here

&b->link != head;
b = tmp,
tmp = wl_container_of(b->link.next, tmp, link)
) {

This comment has been minimized.

@ddevault

ddevault Apr 8, 2018

Member

Dude just use wl_list_for_each_safe

This comment has been minimized.

@Laaas

Laaas Apr 8, 2018

Contributor

I actually can't do that here, since I don't iterate the entire list. wl_list_for_each_safe always iterates the entire list.

This comment has been minimized.

@ddevault

ddevault Apr 8, 2018

Member

You can break out of a wl_list_for_each (continue too)

struct wl_resource *surface,
struct wl_resource *pointer,
struct wl_resource *region,
uint32_t lifetime) {

This comment has been minimized.

@ddevault

ddevault Apr 8, 2018

Member

This can be done in fewer lines


static void pointer_constraints_destroy(struct wl_client *client,
struct wl_resource *resource) {
// TODO: Destroy all relevant locked pointers

This comment has been minimized.

@ddevault

ddevault Apr 8, 2018

Member

Need this done

@ddevault
Copy link
Member

ddevault left a comment

Needs some work

@Laaas Laaas force-pushed the Laaas:master branch 2 times, most recently from bc2f669 to 7ccc1d7 Apr 8, 2018

@Laaas

This comment has been minimized.

Copy link
Contributor

Laaas commented Apr 15, 2018

Please review this again

@Laaas Laaas changed the title Implement pointer-constraints-unstable-v1 protocol (partially) Implement pointer-constraints-unstable-v1 protocol Apr 15, 2018

threads = dependency('threads')
wayland_cursor = dependency('wayland-cursor')
rt = cc.find_library('rt')

This comment has been minimized.

@ddevault

ddevault Apr 15, 2018

Member

rt is already defined at the top-level meson.build

This comment has been minimized.

@Laaas

Laaas Apr 15, 2018

Contributor

So I tried removing this but it didn't work without it, saying rt isn't defined.

This comment has been minimized.

@ddevault

ddevault Apr 15, 2018

Member

Sorry, I was thinking of a different project that already had it at the top-level meson.build. In any case, the top-level meson.build is where this needs to go. We don't resolve dependencies in subprojects.

if (strcmp(interface, interface_wanted.name) == 0) { \
printf("got %s\n", interface); \
output = wl_registry_bind(registry, name, &interface_wanted, version); \
}

This comment has been minimized.

@ddevault

ddevault Apr 15, 2018

Member

Just because this is an example doesn't mean it's exempt from our style guide

const uint64_t size = 4L * 1024L * 1024L;
int fd = shm_open("pointer-constraints-demo-wlroots", O_CREAT | O_RDWR, S_IRUSR | S_IWUSR);
assert(ftruncate(fd, size) == 0);
struct wl_shm_pool *pool = wl_shm_create_pool(shm, fd, size); // 4 MiBs, how many are even needed?

This comment has been minimized.

@ddevault

ddevault Apr 15, 2018

Member

We typically just use EGL surfaces for demos

This comment has been minimized.

@Laaas

Laaas Apr 15, 2018

Contributor

can you explain how to do that? there's pretty much no tutorials or so for wayland.

This comment has been minimized.

@ddevault

ddevault Apr 15, 2018

Member

Just check out some of the other examples, like idle-inhibit.c

wl_surface_commit(surface);

while (true) {
if (wl_display_dispatch(display) < 0) {

This comment has been minimized.

@ddevault

ddevault Apr 15, 2018

Member

You can move this conditional into the loop condition

struct wl_surface *surface = wl_compositor_create_surface(compositor);
wl_surface_attach(surface, buffer, 0, 0);

struct wl_shell_surface *shell_surface = wl_shell_get_shell_surface(shell, surface);

This comment has been minimized.

@ddevault

ddevault Apr 15, 2018

Member

Use xdg-shell

}

return 0;
}

This comment has been minimized.

@ddevault

ddevault Apr 15, 2018

Member

Make these the same example with a getopt flag to control its behavior

@@ -14,6 +14,8 @@ struct roots_cursor {
struct roots_seat *seat;
struct wlr_cursor *cursor;

struct wlr_box *mapped_box;

This comment has been minimized.

@ddevault

ddevault Apr 15, 2018

Member

mapped seems weird, not confined? constrained?

This comment has been minimized.

@Laaas

Laaas Apr 15, 2018

Contributor

well it's what it's called in wlr_cursor, don't ask me

This comment has been minimized.

@emersion

emersion Apr 15, 2018

Member

I don't think it's the same thing: mapped defines the range of e.g. tablet devices, constrained defines a box outside of which the cursor cannot go? (One is "the whole tablet extends maps to this region", the other is "don't move outside of this box")

This comment has been minimized.

@Laaas

Laaas Apr 16, 2018

Contributor

I did notice this when I was using a touch pad with pointer constraints: my cursor was moving much slower than usual, depending on the size of the constraint.

I don't know whether this is necessarily undesirable, but I leave that up to you guys to decide.

This comment has been minimized.

@emersion

emersion Apr 18, 2018

Member

I think the protocol intends constraints, not mappings.

This comment has been minimized.

@ddevault
@@ -35,4 +36,11 @@ void wlr_box_rotated_bounds(const struct wlr_box *box, float rotation,

struct wlr_box wlr_box_from_pixman_box32(const pixman_box32_t box);

static const struct wlr_box WLR_BOX_MAX = {

This comment has been minimized.

@ddevault

ddevault Apr 15, 2018

Member

Move this to wlr_box.c and make this extern

@@ -153,6 +153,15 @@ void wlr_cursor_map_to_region(struct wlr_cursor *cur, struct wlr_box *box);
void wlr_cursor_map_input_to_region(struct wlr_cursor *cur,
struct wlr_input_device *dev, struct wlr_box *box);

/**
* Locks the cursor to its current position
*/

This comment has been minimized.

@ddevault

ddevault Apr 15, 2018

Member

I'm not sure wlr_cursor needs to handle this, the compositor can instead just choose to stop processing input events. The cursor does not move itself when input events happen on attached devices, it just fires the signals and the compositor has to do the actual moving.

This comment has been minimized.

@Laaas

Laaas Apr 16, 2018

Contributor

I think having it as a part of the wlr_cursor would be better in the context of relative-pointer, because you still have to process the input from those devices anyway.

This comment has been minimized.

@ddevault

ddevault Apr 16, 2018

Member

I'm not sure that justification makes any sense. wlr_cursor does not move itself in response to input events. The compositor has to explicitly ask it to do so, and can just stop doing so when it wants to be locked. But it can still process input events in that situation...

This comment has been minimized.

@Laaas

Laaas Apr 16, 2018

Contributor

That's true

This comment has been minimized.

@ddevault

ddevault Apr 19, 2018

Member

This is still here?

@@ -4,6 +4,10 @@
#include <stdint.h>
#include <wayland-server.h>
#include <wlr/types/wlr_box.h>
#include <wlr/types/wlr_seat.h>

// needed because of a circular dependency...

This comment has been minimized.

@ddevault

ddevault Apr 15, 2018

Member

This doesn't need a comment

This comment has been minimized.

@ddevault

ddevault Apr 15, 2018

Member

But now that you mention it... I wonder if at some point we can find a way to transfer responsibility for its resources away from the seat and into the resources themselves. This isn't a problem unique to your patch.

void* data;
};

struct wlr_pointer_constraints_v1 *wlr_pointer_constraints_v1_create(struct wl_display *display);
struct wlr_pointer_constraints_v1_interface {

This comment has been minimized.

@ddevault

ddevault Apr 15, 2018

Member

Seems more appropriate to use wl_signal here

This comment has been minimized.

@Laaas

Laaas Apr 15, 2018

Contributor

Yeah I could probably do that. It's an interface because I initially had non-void return values.

wl_resource_instance_of(resource, &zwp_locked_pointer_v1_interface,
&locked_pointer_impl)
);
wl_resource_instance_of(resource, &zwp_confined_pointer_v1_interface,

This comment has been minimized.

@ddevault

ddevault Apr 15, 2018

Member

80 column limit

@arandomhuman

This comment has been minimized.

Copy link
Contributor

arandomhuman commented Sep 18, 2018

@Laaas Maybe by wrapping a pixman_region inside a wlr_region/box which is aware of rotations (and other transforms)? Or just treating the no-rotation/with-rotation cases separately as you're doing right now.

Check out -Wconversion too, but lots of noise; set werror to false.

@Laaas

This comment has been minimized.

Copy link
Contributor

Laaas commented Sep 18, 2018

I just did a grep for pixman_region32_contains_point, and this implicit conversion of floats to integers is also a problem elsewhere. E.g. in wlr_surface_point_accepts_input, if sx or sy is just underneath the minimum extents of the region, it will count it as inside the region. I really think we should enable -Wconversion, shouldn't be too hard to go through each case and fix it.

Fix implicit conversion of floats to ints in calls to pixman_region32…
…_contains_point

I do not think the conversion is specifically defined, but on my system and SirCmpwn's
the floats are rounded instead of floored, which is incorrect in this case, since
for a range from 0 to 256, any value greater or equal to 0 and less than 256 is valid.
I.e. [0;256[, or 0 <= x < 256, but if x is e.g. -0.1, then it will be rounded to 0, which
is invalid. The correct behavior would be to floor to -1.

@Laaas Laaas force-pushed the Laaas:master branch from 3b92961 to afa2e39 Sep 18, 2018

@Laaas

This comment has been minimized.

Copy link
Contributor

Laaas commented Sep 18, 2018

@SirCmpwn it has been fixed.

@arandomhuman

This comment has been minimized.

Copy link
Contributor

arandomhuman commented Sep 18, 2018

Well, sadly, compiling with -Wconversion currently gives all of 672 warnings on master, and 823 on your branch. I did it blind so the numbers may be off a bit, and probably has duplicates, but that's still quite a lot.

@Laaas

This comment has been minimized.

Copy link
Contributor

Laaas commented Sep 21, 2018

@emersion any objections?


struct wlr_pointer_constraints_v1 {
struct wl_list wl_resources; // wl_resource_get_link
struct wl_global *wl_global;

This comment has been minimized.

@emersion

emersion Sep 26, 2018

Member

These should be named resources and global, we renamed those recently

locked_pointer = zwp_pointer_constraints_v1_lock_pointer(
pointer_constraints, surface, pointer, regions[region_type], lifetime);

zwp_locked_pointer_v1_set_cursor_position_hint(locked_pointer, wl_fixed_from_int(128), wl_fixed_from_int(128));

This comment has been minimized.

@emersion

emersion Sep 26, 2018

Member

80 char limit


if (lock) {
locked_pointer = zwp_pointer_constraints_v1_lock_pointer(
pointer_constraints, surface, pointer, regions[region_type], lifetime);

This comment has been minimized.

@emersion

emersion Sep 26, 2018

Member

80 char limit


return EXIT_SUCCESS;

invalid_args: {

This comment has been minimized.

@emersion

emersion Sep 26, 2018

Member

Style:

  • No braces
  • Label without indentation
free(constraint->current.region);
}
if (constraint->pending.region &&
constraint->current.region != constraint->pending.region) {

This comment has been minimized.

@emersion

emersion Sep 26, 2018

Member

This is a use-after-free

emersion added some commits Sep 26, 2018

pointer-constraints: refactoring
* Rename the constraint_create signal to new_constraint for
  consistency
* Move the constraint_destroy signal to the constraint itself
* Use rotate_child_position instead of duplicating logic
* Fix inert constraint resource handling
* Style fixes
dx = sx2_confined - sx1;
dy = sy2_confined - sy1;
} else {
assert(false);

This comment has been minimized.

@emersion

emersion Sep 27, 2018

Member

If it isn't working, we should remove this

This comment has been minimized.

@Laaas

Laaas Sep 27, 2018

Contributor

It does work though? Can't see the entire context but I presume that is an assumption derived logically, and not a "unimplemented just fail" assertion. It asserts that that should never happen.

This comment has been minimized.

@Laaas

Laaas Sep 27, 2018

Contributor

Hmm, perhaps I forgot to put a FIXME in there. Seems odd that I've put that there. Good catch.

@emersion
Copy link
Member

emersion left a comment

OK for me

Follow-up issue: doesn't work yet with rotated views

@ddevault ddevault merged commit 5e9959d into swaywm:master Sep 27, 2018

1 check passed

builds.sr.ht builds.sr.ht job completed successfully
Details
@ddevault

This comment has been minimized.

Copy link
Member

ddevault commented Sep 27, 2018

Thanks!

...seems a bit too little gratitude for such an arduous pull request. Seriously, nice work @Laaas and thanks for putting a cherry on top @emersion

@Laaas

This comment has been minimized.

Copy link
Contributor

Laaas commented Sep 27, 2018

thanks @SirCmpwn but I could also have done it better. My inexperience was obvious, and there were times where I did not get much work done.

@synaptiko

This comment has been minimized.

Copy link

synaptiko commented Sep 28, 2018

Is it possible that this PR broke swayidle? Last working commit of wlroots for me is: e47b8cd. I'm on the latest master of sway.

This is what I see in the log:

2018-09-28 20:38:08 - [sway/sway/commands.c:286] Handling command 'exec swayidle 		timeout 300 'swaylock -c 000000' 		timeout 360 'swaymsg "output * dpms off"' 		resume 'swaymsg "output * dpms on"' 		before-sleep 'swaylock -c 000000''
2018-09-28 20:38:08 - [sway/sway/commands.c:165] find_handler(exec)
2018-09-28 20:38:08 - [sway/sway/commands/exec_always.c:44] Executing swayidle timeout 300 'swaylock -c 000000' timeout 360 'swaymsg "output * dpms off"' resume 'swaymsg "output * dpms on"' before-sleep 'swaylock -c 000000'
2018-09-28 20:38:08 - [sway/sway/commands/exec_always.c:85] Child process created with pid 11524
2018-09-28 20:38:08 - [sway/sway/tree/root.c:208] Recording workspace for process 11524
2018-09-28 20:38:08 - [sway/sway/server.c:164] Running compositor on wayland display 'wayland-0'
2018-09-28 20:38:08 - [types/wlr_surface.c:626] New wlr_surface 0x5599d82554e0 (res 0x5599d8257d60)
2018-09-28 20:38:08 - [types/wlr_layer_shell_v1.c:388] new layer_surface 0x5599d8255800 (res 0x5599d822df00)
2018-09-28 20:38:08 - [sway/sway/desktop/layer_shell.c:332] new layer surface: namespace wallpaper layer 0 anchor 0 size 0x0 margin 0,0,0,0
2018-09-28 20:38:08 - [sway/swayidle/main.c:401] Display doesn't support idle protocol
@klardotsh

This comment has been minimized.

Copy link

klardotsh commented Sep 30, 2018

This is seriously awesome to see merged. Thanks a ton @Laaas, "could have done it or better" or not, you got it done and it's a huge boost to the project 🎉

@ddevault ddevault referenced this pull request Dec 9, 2018

Closed

Mouse capture support? #1430

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