seat: allow clients to bind to seat multiple times #1031

martinetd · 2018-06-03T23:40:47Z

This lets clients bind to a seat multiple times by re-using the existing
wlr_seat_client whenever a duplicate request happens.
Previously, an independant wlr_seat_client would be created and only
events from one would be processed.

Fixes #1023.

~~FWIW I'm not 100% happy with the style (new_seat_client bool and the need for a new function in the API, and that function's name), but at least it seems to work.~~
Cc @Ongy @Timidger as API changes, even if I didn't need to change anything to rootston/sway, it's just a ~~new helper function~~ change in the wlr_seat_client struct

emersion · 2018-06-04T07:42:20Z

types/seat/wlr_seat.c

@@ -282,6 +307,20 @@ struct wlr_seat_client *wlr_seat_client_for_wl_client(struct wlr_seat *wlr_seat,
 	return NULL;
 }

+struct wl_client *wl_client_for_wlr_seat_client(


This is already wlr_seat_client->client

Oh. That removes one thorn, I totally missed that. Thanks.

(force pushed)

emersion · 2018-06-04T07:47:06Z

include/wlr/types/wlr_seat.h

 	struct wl_client *client;
 	struct wlr_seat *seat;

 	// lists of wl_resource
+	struct wl_list binds;


I don't like a lot that there are two levels of lists. Why not create one wlr_seat_client per bind?

What do you call two levels of lists? There already are plenty of lists in wlr_seat_client.
On the other hand, creating one wlr_seat_client per bind will make us iterate over all the binds everywhere we want to notify the client about something, that's a much bigger code change that would break compositor compat (that is fine) and feels less practical to use.
Unless there's any reason to treat one bind differently from the other I'd rather keep it as simple as possible from an interface point of view

I'll think about it. In the meantime, can you rename it to resources? We don't use the term "bind" anywhere.

Hmm, looked around and we're not consistent at all. The one that comes up most is wl_resources but we have resources, client_resources...

I personally don't like to say just resources as everything is resources, a few lines below (thanks github for showing context!!!) are "resources" to pointers, keyboards etc.

Anyway, renaming for now, but this might need a separate discussion.

BTW I think you're thinking too much, this is basically just what I said in #1023 - or did you understand something else in my comment?

Yeah, read it too quickly.

emersion · 2018-06-04T08:54:05Z

Can you confirm it fixes firefox on wayland?

I personally don't like to say just resources as everything is resources, a few lines below (thanks github for showing context!!!) are "resources" to pointers, keyboards etc.

Yeah, but the struct is named wlr_seat_client, so it makes sense that a resource field contains a wl_seat.

martinetd · 2018-06-04T08:59:56Z

Can you confirm it fixes firefox on wayland?

Yes it does. Well, it doesn't work great because of no popup, but there's input.

Yeah, but the struct is named wlr_seat_client, so it makes sense that a resource field contains a wl_seat.

Well, sure, until it's created as struct wlr_seat_client *client and then client->resource is a wl_seat (thanksfully nobody tried to access that field with that naming until now, but we do have some of that naming)
Anyway, I'm just nitpicking at this point, it's more annoying to me that we have some wl_resources some resources and some others, I'll open an issue for that later

emersion · 2018-06-04T09:02:11Z

Yes it does. Well, it doesn't work great because of no popup, but there's input.

Good. Is the popup issue a firefox issue or a wlroots issue?

Anyway, I'm just nitpicking at this point, it's more annoying to me that we have some wl_resources some resources and some others, I'll open an issue for that later

+1

emersion · 2018-06-04T09:04:01Z

types/seat/wlr_seat.c

 	}
-	seat_client->wl_resource =
+
+	struct wl_resource *wl_resource =


Maybe we can move this block before the previous one and remove new_seat_client?

Good idea.
How can we cleanly undo a wl_resource_create if the calloc fails, though? just destroy it?

Yeah, wl_resource_destroy should do it.

okay, force pushed.

martinetd

Good. Is the popup issue a firefox issue or a wlroots issue?

Unsure yet, I tried to test just after you posted and got rootston stuck in a loop in wlr_surface_get_root_surface because subsurface->surface == surface and it gets stuck, so either the logic is wrong or we need an anti-looping check (and then, is one level enough e.g. A->B->A or would we need something more complex for A->B->C->D->B->... that would suck)
The first time it didn't get stuck though, but it looks the the subsurface appeared below the firefox window (z-axis below, e.g. making firefox small with a large popup I'd see the bottom of the popup but the top would be hidden by FF)

But for either of these problems, I didn't check if what FF does is correct.

martinetd · 2018-06-04T10:55:47Z

types/seat/wlr_seat.c

 	}
-	seat_client->wl_resource =
+
+	struct wl_resource *wl_resource =


Good idea.
How can we cleanly undo a wl_resource_create if the calloc fails, though? just destroy it?

agx · 2018-06-04T12:29:05Z

Ah...I've seen something similar in phosh when binding to a seat multiple times

https://source.puri.sm/Librem5/phosh/blob/master/src/phosh.c#L634

I can try the patches later this week.

This lets clients bind to a seat multiple times by re-using the existing wlr_seat_client whenever a duplicate request happens. Previously, an independant wlr_seat_client would be created and only events from one would be processed. Fixes #1023.

martinetd · 2018-06-04T13:37:06Z

@agx: Cool! I'm sure the other problems with firefox-wayland are different anyway so should be separate, but this can probably wait until you've had time to test to be sure.

emersion · 2018-06-04T15:44:30Z

Thanks!

agx · 2018-06-04T16:38:23Z

Hi,

On Mon, Jun 04, 2018 at 06:37:08AM -0700, Dominique Martinet wrote: ***@***.***: Cool! I'm sure the other problems with firefox-wayland are different anyway so should be separate, but this can probably wait until you've had time to test to be sure.

I did not look at the code but your patch breaks binding wl_seat multiple times in phosh. Without your patch there's simply no input at all, works fine with your patch applied.

emersion · 2018-06-04T16:39:59Z

I did not look at the code but your patch breaks binding wl_seat multiple times in phosh.

Can you elaborate?

agx · 2018-06-05T05:26:24Z

Hi,

On Mon, Jun 04, 2018 at 04:40:01PM +0000, emersion wrote: I did not look at the code but your patch breaks binding wl_seat multiple times in phosh. Can you elaborate?

This meant to read: I did not look at the code but your patch *un*breaks binding wl_seat multiple times in phosh.

emersion · 2018-06-05T07:35:40Z

Ahah, got it, thanks for the feedback!

The corresponding issue in wlroots was fixed by swaywm/wlroots#1031

emersion reviewed Jun 4, 2018

View reviewed changes

martinetd commented Jun 4, 2018

View reviewed changes

emersion approved these changes Jun 4, 2018

View reviewed changes

emersion merged commit 7896641 into swaywm:master Jun 4, 2018

martinetd deleted the multibind-seat branch June 5, 2018 00:26

agx added a commit to agx/phosh that referenced this pull request Sep 30, 2018

Remove wl_seats workaround

cf4b783

The corresponding issue in wlroots was fixed by swaywm/wlroots#1031

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

seat: allow clients to bind to seat multiple times #1031

seat: allow clients to bind to seat multiple times #1031

martinetd commented Jun 3, 2018 •

edited

emersion Jun 4, 2018

martinetd Jun 4, 2018

martinetd Jun 4, 2018

emersion Jun 4, 2018

martinetd Jun 4, 2018

emersion Jun 4, 2018

martinetd Jun 4, 2018

emersion Jun 4, 2018

emersion commented Jun 4, 2018

martinetd commented Jun 4, 2018

emersion commented Jun 4, 2018

emersion Jun 4, 2018

martinetd Jun 4, 2018

emersion Jun 4, 2018

martinetd Jun 4, 2018

martinetd left a comment

martinetd Jun 4, 2018

agx commented Jun 4, 2018

martinetd commented Jun 4, 2018

emersion commented Jun 4, 2018

agx commented Jun 4, 2018 via email

emersion commented Jun 4, 2018

agx commented Jun 5, 2018 via email

emersion commented Jun 5, 2018

seat: allow clients to bind to seat multiple times #1031

seat: allow clients to bind to seat multiple times #1031

Conversation

martinetd commented Jun 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emersion commented Jun 4, 2018

martinetd commented Jun 4, 2018

emersion commented Jun 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinetd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agx commented Jun 4, 2018

martinetd commented Jun 4, 2018

emersion commented Jun 4, 2018

agx commented Jun 4, 2018 via email

emersion commented Jun 4, 2018

agx commented Jun 5, 2018 via email

emersion commented Jun 5, 2018

martinetd commented Jun 3, 2018 •

edited