| Summary: | Document how serial should work | ||
|---|---|---|---|
| Product: | Wayland | Reporter: | Pekka Paalanen <ppaalanen> |
| Component: | wayland | Assignee: | Daniel Stone <daniel> |
| Status: | RESOLVED MOVED | QA Contact: | Wayland bug list <wayland-bugs> |
| Severity: | enhancement | ||
| Priority: | medium | CC: | jadahl, mchqwerty |
| Version: | unspecified | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Bug Depends on: | |||
| Bug Blocks: | 83429 | ||
Hi,
I noticed this bug few days ago. Before I read this I sent a patch for bumping serials in disply_sync. After reading this, I got unsure whether the patch is correct.. but, yes, I think we should use different serial for each callback. Problem is that it could break things.
> < pq> one of the presumably latest additions is xdg_send_configure(), which
> wants to use a totally unrelated serial to match configure to ack_configure
>
> < daniels> ugh, yes
>
> < daniels> that's the main one i can see which is objectively broken
>
> < pq> we only have one serial source counter in wl_display, so I think we
> should be wl_display_get_serial()'ing a lot less than we do now
>
> < daniels> that really needs to be in a separate namespace
>
> < pq> well, wouldn't we need one serial generator per wl_seat?
>
> < pq> I mean namespace
>
> < daniels> bumping serial on focus change is definitely broken too, though i
> guess probably inadvertently gets us the behaviour we want :P
>
> < daniels> i.e. if you click on the titlebar of an unfocused window, you
> don't want to start a move
>
> < daniels> so it might be that clients are trying to start moves with the
> button-down serial, which then gets rejected because focus is newer :P
>
> < daniels> namespace/generator no, comparator yes
>
> < daniels> i.e. store last_serial in wl_seat rather than wl_display, and use
> that to compare against
>
> < pq> http://pastie.org/pastes/9526493/text for get_serial usage
>
> < daniels> keymap bumping serial is a little questionable too, but trying to
> relate events across keymap changes is ... ugh
>
> < pq> daniels, serials are already stored separately to be compared against,
> but the serials sent to clients are retrieved from wl_display, which may
> have advanced for unrelated reasons
>
> < daniels> hmmm, yes
>
> < daniels> right, so get_serial should also be retrieving from wl_seat where
> it relates to that
What about adding serial counter into wl_resource and relevant objects could have get_serial/next_serial for this (i.e. wl_keyboard_get/next_serial etc.)? If the serial would be specific to each object then AFAIK it wouldn't break anything in the current code. Actually, that is what I expect from the serials. When I get a pointer event with a serial, say A, and then get another pointer event with some other serial, say B, I would expect that the B serial is related only to A serial and is not affected by any other object.
(In reply to Marek Chalupa from comment #1) > What about adding serial counter into wl_resource and relevant objects could > have get_serial/next_serial for this (i.e. wl_keyboard_get/next_serial > etc.)? If the serial would be specific to each object then AFAIK it wouldn't > break anything in the current code. Actually, that is what I expect from the > serials. When I get a pointer event with a serial, say A, and then get > another pointer event with some other serial, say B, I would expect that the > B serial is related only to A serial and is not affected by any other object. wl_resource would be a wrong place in many ways. Not all objects need a serial tag (variable), and certainly not a generator. The serial cannot work if it is per-object, as objects are per-client or even multiple-per-client. One of the goals is to prevent one client from hijacking things when another client is already being targeted, e.g. cursor change after the pointer already moved onto another client's surface. It would be more overhead to maintain serial tags of each "thing" per-object, rather than having them per-"thing". We only need one serial generator that is monotonic (rare wraparound allowed). Then we need serial tags for each "thing", like a weston_seat or xdg_surface. The tags are not per-client and not per-wl_resource. The important part is when to assign a new serial into a tag (i.e. bump), and how to compare the serials from clients to the correct tag. This came up on IRC recently, so writing down some points. Currently there are various assumptions on what serials are that are used here and there, most notably in xdg_shell. This assumption is that for a wl_seat, all the serials for all the input devices classes are not disjoint, i.e. they use the same serial counter thus there will never be a "touch" serial that is the same as either a "keyboard" or a "pointer" serial. This is used to determine whether a grab request (e.g. popup grab, show window menu, move, resize) should be ignored or not, or there was some other interaction event that should cancel it. For example, it should be possible for a compositor to not show a user touched somewhere after a popup triggering click happened but before the compositor saw the popup request. This is possible if the serials are not disjoint. This could be seen as relying on an implementation detail, and it kind of is, but I believe we should codify this in the protocol, which right now does not specify this at all. -- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/wayland/wayland/issues/15. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.
There is confusion on what the basic principles on using serials, at least in Weston, should be. Sure, Weston needs lots of fixes, but we should also document how a compositor is expected to use serials, and especially the wl_display_next_serial() API. < pq> does anyone really understand how the serial (wl_display_{get,next}_serial()) system is *supposed* to be used? < pq> when to use next_serial() vs. get_serial() < pq> how to compare serials < pq> when to record a valid serial for later comparison, etc. < pq> and where to get the serial value to deliver to clients with input and other events: current value from wl_display or some custom saved value? < pq> how do multiple wl_seats affect these? < pq> does a pointer button-down on one wl_seat "invalidate" the button-down on another wl_seat? < daniels> no < daniels> ... is the short answer :P < pq> ok, so I think all the serial tracking is at least broken to multiple wl_seats, which does not surprise me, but I think it might be broken even more < daniels> i mean, if you have two seats (A & B) acting independently delivering to independent windows, window Y gets a button down from seat A and window Z gets a button down from seat B later, wl_shell_surface::resize with serial A on window Y should still work < daniels> i'm absolutely astounded that multiple seats in weston work as well as they do < daniels> it was only ever half-finished at best < daniels> significant input events (button, key, touch) should always use next_serial; iirc, the only chaining (same-serial) event is key followed by modifier < daniels> the intention being that it's sometimes nice to be able to relate modifier changes back to the provoking key event < pq> and motion < pq> okay < daniels> right, motion isn't a significant event < daniels> so it always sticks with current serial < pq> but if I understood the code at all, it is sending the wl_display's current seial value to clients in events < pq> the current value can be bumped by pretty much anything < daniels> modifier will use the serial of a key event if it was directly provoked by that, or the next serial if it happened independently of a key event < daniels> hmm, what bumps it other than button/key/touch? < pq> if the serial checks compare for equal, a client might be using a serial that is too *new* and get rejected by that < daniels> in which scenario? < pq> I didn't identify an exact scenario yet, I was just looking at the grep results for wl_display_next_serial() and grab_serial < daniels> do we ever bump the serial outside of button/key/touch? (don't have the source to hand, sorry) < daniels> but yes, storing one _global_ value is going to break multi-seat completely < daniels> or at least, multiple seats independently interacting with different surfaces < pq> yes, a lot of places < pq> http://pastie.org/pastes/9526487/text < pq> one of the presumably latest additions is xdg_send_configure(), which wants to use a totally unrelated serial to match configure to ack_configure < daniels> ugh, yes < daniels> that's the main one i can see which is objectively broken < pq> we only have one serial source counter in wl_display, so I think we should be wl_display_get_serial()'ing a lot less than we do now < daniels> that really needs to be in a separate namespace < pq> well, wouldn't we need one serial generator per wl_seat? < pq> I mean namespace < daniels> bumping serial on focus change is definitely broken too, though i guess probably inadvertently gets us the behaviour we want :P < daniels> i.e. if you click on the titlebar of an unfocused window, you don't want to start a move < daniels> so it might be that clients are trying to start moves with the button-down serial, which then gets rejected because focus is newer :P < daniels> namespace/generator no, comparator yes < daniels> i.e. store last_serial in wl_seat rather than wl_display, and use that to compare against < pq> http://pastie.org/pastes/9526493/text for get_serial usage < daniels> keymap bumping serial is a little questionable too, but trying to relate events across keymap changes is ... ugh < pq> daniels, serials are already stored separately to be compared against, but the serials sent to clients are retrieved from wl_display, which may have advanced for unrelated reasons < daniels> hmmm, yes < daniels> right, so get_serial should also be retrieving from wl_seat where it relates to that < pq> one question I had was, should the comparison be for equal, or greater or equal? < daniels> == < daniels> if you're sending a serial greater than the last significant one, you've screwed something up < pq> okay, then definitely one should be sending saved serials, not from wl_display < daniels> hmm, i think we can actually keep using next_serial - we just need to deprecate get_serial on wl_display, and instead require that everyone who needs a serial store it somewhere appropriate, e.g. in the shell info structure