GNOME Bugzilla – Bug 787363
Add xdg-output support
Last modified: 2021-07-05 13:50:56 UTC
xdg-output is a (new) protocol aims at describing outputs in way which is more in line with the concept of an output on desktop oriented systems. https://cgit.freedesktop.org/wayland/wayland-protocols/commit/?id=794a96a This is part of wayland-protocol-1.10. or now it just features the position and logical size which describe the output position and size in the global compositor space. This is however much useful for Xwayland to advertise the output size and position to X11 clients which need this to configure their surfaces in the global compositor space as the compositor may apply a different scale from what is advertised by the output scaling property (to achieve fractional scaling, for example).
Created attachment 359273 [details] [review] [PATCH 1/2] wayland: Add xdg-output support The xdg-output protocol aims at describing outputs in way which is more in line with the concept of an output on desktop oriented systems. For now it just features the position and logical size which describe the output position and size in the global compositor space. This is however much useful for Xwayland to advertise the output size and position to X11 clients which need this to configure their surfaces in the global compositor space as the compositor may apply a different scale from what is advertised by the output scaling property (to achieve fractional scaling, for example).
Created attachment 359274 [details] [review] [PATCH 2/2] wayland: Advertise MetaMonitor as wl_output With the logical size of outputs being handled by xdg-output, we don't need to "lie" to put Wayland clients anymor eabout the output size, so advertise the real mode size as wl_output regardless of the scale, as it should be (and like other Wayland compositors do).
Review of attachment 359273 [details] [review]: ::: src/wayland/meta-wayland-outputs.c @@ +308,3 @@ + struct wl_resource *resource = iter->data; + send_xdg_output_events (resource, wayland_output, logical_monitor); + } Hmm. This has nothing to do with this patch, but how would the caller know whether to expect xdg_output events when seeing wl_output events? For example, what if the client abstracts "monitors" and has a "monitors changed" event. It wouldn't know when to emit that event on wl_output.done or not. @@ +470,3 @@ + zxdg_output_v1_send_logical_position(resource, layout.x, layout.y); + zxdg_output_v1_send_logical_size(resource, layout.width, layout.height); + zxdg_output_v1_send_done(resource); nit: space between function name and ( @@ +477,3 @@ + struct wl_resource *resource, + uint32_t id, + struct wl_resource *output) nit: Argument alignment. @@ +483,3 @@ + + xdg_output_resource = + wl_resource_create (client, nit: indentation.
Review of attachment 359274 [details] [review]: ::: src/wayland/meta-wayland-outputs.c @@ +202,3 @@ if (current_mode == preferred_mode) mode_flags |= WL_OUTPUT_MODE_PREFERRED; + meta_monitor_mode_get_resolution (current_mode, &new_width, &new_height); Shouldn't we rotate the width/height according to the logical monitor rotation? Otherwise the aspect ratio of the mode and the logical output size might be different.
(In reply to Jonas Ådahl from comment #3) > [...] > Hmm. This has nothing to do with this patch, but how would the caller know > whether to expect xdg_output events when seeing wl_output events? What Xwayland does (in the patch I need to resend with fixes to xorg-devel) is to use xdg_output events and ignore wl_output events when xdg-output is available from the compositor. That's what I meant by: "Some of the information provided in this protocol might be identical to their counterparts already available from wl_output, in which case the information provided by this protocol should be preferred to their equivalent in wl_output. The goal is to move the desktop specific concepts (such as output location within the global compositor space, the connector name and types, etc.) out of the core wl_output protocol." in the xdg-output protocol definition.
(In reply to Olivier Fourdan from comment #5) > (In reply to Jonas Ådahl from comment #3) > > [...] > > Hmm. This has nothing to do with this patch, but how would the caller know > > whether to expect xdg_output events when seeing wl_output events? > > What Xwayland does (in the patch I need to resend with fixes to xorg-devel) > is to use xdg_output events and ignore wl_output events when xdg-output is > available from the compositor. > > That's what I meant by: > > "Some of the information provided in this protocol might be identical > to their counterparts already available from wl_output, in which case > the information provided by this protocol should be preferred to their > equivalent in wl_output. The goal is to move the desktop specific > concepts (such as output location within the global compositor space, > the connector name and types, etc.) out of the core wl_output protocol." > > in the xdg-output protocol definition. Right, but since we lie about the transform, we ought to lie about the mode too IMHO. The wierd issues will at least be slightly less wierd for legacy applications that isn't updated to xdg_output.
(In reply to Jonas Ådahl from comment #4) > Review of attachment 359274 [details] [review] [review]: > > ::: src/wayland/meta-wayland-outputs.c > @@ +202,3 @@ > if (current_mode == preferred_mode) > mode_flags |= WL_OUTPUT_MODE_PREFERRED; > + meta_monitor_mode_get_resolution (current_mode, &new_width, &new_height); > > Shouldn't we rotate the width/height according to the logical monitor > rotation? Otherwise the aspect ratio of the mode and the logical output size > might be different. Yeah, I've been thinking of this as well. I don't remember why mutter does not advertise rotations in wl_output (unlike weston). What weston does, is to advertise the monitor size regardless of rotations, and set the wl_output.transform to the actual outpout rotation so that Xwayland can translate that correctly in screen size and in Xrandr. Mutter doesn't do that, it swaps the width/height depending on the actual rotation band always use "no transform" for wl_output.transform, but I don't remember the reason for this...
(In reply to Jonas Ådahl from comment #6) > Right, but since we lie about the transform, we ought to lie about the mode > too IMHO. The wierd issues will at least be slightly less wierd for legacy > applications that isn't updated to xdg_output. You mean we should swap the width/height for xdg-output like we do for wl_output in mutter (i.e. we lie)?
(In reply to Olivier Fourdan from comment #7) > (In reply to Jonas Ådahl from comment #4) > > Review of attachment 359274 [details] [review] [review] [review]: > > > > ::: src/wayland/meta-wayland-outputs.c > > @@ +202,3 @@ > > if (current_mode == preferred_mode) > > mode_flags |= WL_OUTPUT_MODE_PREFERRED; > > + meta_monitor_mode_get_resolution (current_mode, &new_width, &new_height); > > > > Shouldn't we rotate the width/height according to the logical monitor > > rotation? Otherwise the aspect ratio of the mode and the logical output size > > might be different. > > Yeah, I've been thinking of this as well. I don't remember why mutter does > not advertise rotations in wl_output (unlike weston). > > What weston does, is to advertise the monitor size regardless of rotations, > and set the wl_output.transform to the actual outpout rotation so that > Xwayland can translate that correctly in screen size and in Xrandr. > > Mutter doesn't do that, it swaps the width/height depending on the actual > rotation band always use "no transform" for wl_output.transform, but I don't > remember the reason for this... Because we don't implement buffer transforms, meaning some clients being "smart" pre-rotates their buffers depending on the transform of the monitor. Thus, we should probably lie about the mode (i.e. swap the width/height if we are rotated). Which is what I meant in the comment above, but replied to a semi unrelated thing; sorry about that :P I'll try again: (In reply to Olivier Fourdan from comment #5) > (In reply to Jonas Ådahl from comment #3) > > [...] > > Hmm. This has nothing to do with this patch, but how would the caller know > > whether to expect xdg_output events when seeing wl_output events? > > What Xwayland does (in the patch I need to resend with fixes to xorg-devel) > is to use xdg_output events and ignore wl_output events when xdg-output is > available from the compositor. > > That's what I meant by: > > "Some of the information provided in this protocol might be identical > to their counterparts already available from wl_output, in which case > the information provided by this protocol should be preferred to their > equivalent in wl_output. The goal is to move the desktop specific > concepts (such as output location within the global compositor space, > the connector name and types, etc.) out of the core wl_output protocol." > > in the xdg-output protocol definition. Right, but the problem I see is, what if care about things that wl_output exposes that xdg_output won't? Like subpixel details, available *actual* modes (for preferred mode hints etc). Or do you expect to expose all of that in xdg_output as well?
(In reply to Jonas Ådahl from comment #9) > [...] > Because we don't implement buffer transforms, meaning some clients being > "smart" pre-rotates their buffers depending on the transform of the monitor. > Thus, we should probably lie about the mode (i.e. swap the width/height if > we are rotated). Right, this is what I thought as well, thanks for clarifying :) > Right, but the problem I see is, what if care about things that wl_output > exposes that xdg_output won't? Like subpixel details, available *actual* > modes (for preferred mode hints etc). Or do you expect to expose all of that > in xdg_output as well? Yeah, sorry, I wasn't clear, for the information available from both xdg-output and wl_output, a client should use those from xdg-output if the compositor supports it. xdg-output exposes the outputs in the global compositor space, the subpixel, transform, available modes remain part of wl_output and can be be obtained from wl_output events (unless we see the need to supersede those in xdg-output for desktop specific uses). For example, Xwayland still uses wl_output events for those, it's just the size and location that it gets from xdg-output (as this is what is available there).
> For example, what if the client abstracts "monitors" and has a "monitors > changed" event. It wouldn't know when to emit that event on wl_output.done or > not. If both the client and compositor support wl_output and xdg-output and if the client's monitor definition requires the logical size and position, then in this case the client should wait for both wl_output.done and xdg-output.done events as the complete definition comes from both.
(In reply to Olivier Fourdan from comment #11) > > For example, what if the client abstracts "monitors" and has a "monitors > > changed" event. It wouldn't know when to emit that event on wl_output.done or > > not. > > If both the client and compositor support wl_output and xdg-output and if > the client's monitor definition requires the logical size and position, then > in this case the client should wait for both wl_output.done and > xdg-output.done events as the complete definition comes from both. Is xdg_output.done always expected to be sent *after* a wl_output.done?
(In reply to Jonas Ådahl from comment #12) > Is xdg_output.done always expected to be sent *after* a wl_output.done? Nothing in the protocol definition specifies that, so a client would have to wait for both "done" events, whichever comes last, before emitting a "monitor-changed" event.
(In reply to Olivier Fourdan from comment #13) > (In reply to Jonas Ådahl from comment #12) > > Is xdg_output.done always expected to be sent *after* a wl_output.done? > > Nothing in the protocol definition specifies that, so a client would have to > wait for both "done" events, whichever comes last, before emitting a > "monitor-changed" event. If that's the case, you must change the code that emits wl_output stuff to always emit wl_output_done. Alternatively add a "needs_done" to both the code that sends wl_output and xdg_output stuff, then send both wl_output_done and xdg_output_done if needed in the end. Because as far as I can see, the current patch doesn't guarantee that one always receives none or both done events.
Yeap, that's a gross oversight in my patch :)
Created attachment 359359 [details] [review] [PATCH 1/2 v2] wayland: Add xdg-output support v2: Adjust indent as per comment 3 Make sure we send an wl_output.done event whenever we send a xdg-output.done event, so that clients can rely on both events to be received.
Created attachment 359360 [details] [review] [PATCH 2/2 v2] wayland: Advertise MetaMonitor as wl_output v2: swap width/height for wl_output as necessary depending on actual transform (rotation)
Review of attachment 359359 [details] [review]: ::: src/Makefile.am @@ +84,3 @@ keyboard-shortcuts-inhibit-unstable-v1-server-protocol.h \ + xdg-output-unstable-v1-protocol.c \ + xdg-output-unstable-v1-server-protocol.h \ These needs to be in .gitignore too I think. ::: src/wayland/meta-wayland-outputs.c @@ +47,3 @@ + struct wl_resource *wl_output; + struct wl_resource *xdg_output; +} XdgOutputResources; What is this needed for? I don't see how we need to know the actual wl_output an xdg_output is created for. It only matters what output the wl_output represents. @@ +132,3 @@ } +static gboolean I suggest the following way instead, which I think would be clearer, and still make it possible to avoid sending updates when no updates were done (like it is now). Change send_output_events() to add a gboolean *needs_done, instead of defining it itself. Define "needs_done" in the function calling send_(xdg_)output_events(). Don't send _done() in send_(xdg_)output_events(). Change send_xdg_output_events() to take the "gboolean *needs_done". Don't send the xdg_output events if nothing changed. Send both done events after having called both send_(xdg_)output_events() and any of them set the "needs_done" to TRUE. @@ +482,3 @@ + + g_free (xdg_output_resources); + } The actual cleanup should be in a 'zxdg_output_destructor'. @@ +521,3 @@ + wl_resource_create (client, + &zxdg_output_v1_interface, + META_ZXDG_OUTPUT_V1_VERSION, This should be "wl_resource_get_version (resource)," @@ +535,3 @@ + g_list_prepend (wayland_output->xdg_output_resources, xdg_output_resources); + + nit: Stray newline. @@ +564,3 @@ + resource = wl_resource_create (client, + &zxdg_output_manager_v1_interface, + META_ZXDG_OUTPUT_V1_VERSION, Technically this should be "MIN (META_ZXDG_OUTPUT_V1_VERSION, version)", but since we always get 1 anyway makes it not that important.
Review of attachment 359360 [details] [review]: ::: src/wayland/meta-wayland-outputs.c @@ +135,3 @@ +adjust_wayland_output_size (MetaLogicalMonitor *logical_monitor, + gint *width, + gint *height) Can use regular 'int' here. @@ +151,3 @@ + default: + break; + } I'd rather have this being a function called something like "get_fake_mode_resolution" that is implemented like this: if (meta_monitor_mode_is_rotated (current_mode) meta_monitor_mode_get_resolution (current_mode, &new_mode_width, &new_mode_height); else meta_monitor_mode_get_resolution (current_mode, &new_mode_height, &new_mode_width); @@ +334,3 @@ + adjust_wayland_output_size (logical_monitor, + &wayland_output->mode_width, + &wayland_output->mode_height); I'd prefer the same as above (no anonymous "adjust"). Alternatively use a helper called something like "get_fake_output_mode_resolution" that makes it clearer that its magic.
(In reply to Jonas Ådahl from comment #18) > Review of attachment 359359 [details] [review] [review]: > [...] > ::: src/wayland/meta-wayland-outputs.c > @@ +47,3 @@ > + struct wl_resource *wl_output; > + struct wl_resource *xdg_output; > +} XdgOutputResources; > > What is this needed for? I don't see how we need to know the actual > wl_output an xdg_output is created for. It only matters what output the > wl_output represents. The idea is that a client could have more then one xdg-output for each wl_ouput (using the get_xdg_output() request more than once on a given wl_output)
(In reply to Olivier Fourdan from comment #20) > (In reply to Jonas Ådahl from comment #18) > > Review of attachment 359359 [details] [review] [review] [review]: > > [...] > > ::: src/wayland/meta-wayland-outputs.c > > @@ +47,3 @@ > > + struct wl_resource *wl_output; > > + struct wl_resource *xdg_output; > > +} XdgOutputResources; > > > > What is this needed for? I don't see how we need to know the actual > > wl_output an xdg_output is created for. It only matters what output the > > wl_output represents. > > The idea is that a client could have more then one xdg-output for each > wl_ouput (using the get_xdg_output() request more than once on a given > wl_output) Right, but the validness of xdg_output is not tied to a wl_output but the MetaWaylandOutput, so no reason to keep any association between what wl_output was used to create the xdg_output. It could for example be valid to do wl_output = bind(wl_output_global) xdg_output = get_xdg_output(wl_output) wl_output_destroy(wl_output) then continue to use xdg_output.
Sure, but how do you make sure that a "done" event has been sent for both xdg-output and wl_output when either change? I'm sorry, I don't understand your point in comment 18 about passing a gboolean *, these are two separate sets of resources (xdg-output list of resources, wl_output list of resources) so two independent loops in wayland_output_update_for_output()
(In reply to Olivier Fourdan from comment #22) > Sure, but how do you make sure that a "done" event has been sent for both > xdg-output and wl_output when either change? I think adding something like this: static void meta_wayland_output_send_events (MetaWaylandOutput *output) { GList *l; gboolean need_done = FALSE; for (l = wayland_output->resources; l; l = l->next) { struct wl_resource *resource = l->data; send_output_events (resource, wayland_output, logical_monitor, FALSE, &need_done); } for (l = wayland_output->xdg_output_resources; l; l = l->next) { struct wl_resource *resource = l->data; send_xdg_output_events (resource, wayland_output, logical_monitor, FALSE, &need_done); } if (need_done) { g_list_foreach (wayland_output->resources, (GFunc) wl_output_send_done, NULL); g_list_foreach (wayland_output->xdg_resources, (GFunc) wl_output_send_done, NULL); } and adapt send_*_events to set 'need_done' to TRUE if any event was sent anywhere, but dont send the done event themself. > > I'm sorry, I don't understand your point in comment 18 about passing a > gboolean *, these are two separate sets of resources (xdg-output list of > resources, wl_output list of resources) so two independent loops in > wayland_output_update_for_output() It's to avoid the sending wl_output.done event in two different places. To me it makes more sense first do the things that determine whether a 'done' is required, then send the 'done's, as two distinct steps.
Created attachment 360114 [details] [review] [PATCH 1/2 v3] wayland: Add xdg-output support
Created attachment 360115 [details] [review] [PATCH 2/2 v3] wayland: Advertise MetaMonitor as wl_output
The patch had been amended following the reviews, can we reconsider those? The idea would be to land the first patch (attachment 360114 [details] [review]) in mutter, then land the fix in Xserver (https://patchwork.freedesktop.org/patch/175575/ which needs review there as well), then we can land the second patch in mutter (attachment 360115 [details] [review]) and eventually the second patch in Xwayland (https://patchwork.freedesktop.org/patch/175578/) so that it works with other compositor such as Weston.
Humble ping, can we do a review of those patches please, at least attachment 360114 [details] [review] at first?
Review of attachment 360114 [details] [review]: ::: src/wayland/meta-wayland-outputs.c @@ +269,3 @@ wayland_output->refresh_rate); + send_output_events (resource, wayland_output, logical_monitor, TRUE, NULL); Still need to send wl_output_done() here right? @@ +591,3 @@ + resource = wl_resource_create (client, + &zxdg_output_manager_v1_interface, + MIN (META_ZXDG_OUTPUT_V1_VERSION, version), It's up to the client to decide the version. We must only handle it, and disconnect a miss behaving client (e.g. if it tries to bind a version newer than we support).
Review of attachment 360115 [details] [review]: s/anymor eabout/anymore about/. Would it make sense to conditionalize not lying on xdg_output being supported? Would avoid a stream of bug reports for the period when the X server does not yet support xdg_output.
(In reply to Jonas Ådahl from comment #28) > Review of attachment 360114 [details] [review] [review]: Hi Jonas, thanks for the review! > ::: src/wayland/meta-wayland-outputs.c > @@ +269,3 @@ > wayland_output->refresh_rate); > > + send_output_events (resource, wayland_output, logical_monitor, TRUE, > NULL); > > Still need to send wl_output_done() here right? Err, nope, wl_output_done() is sent from send_output_events() because we passed TRUE for “need_all_events”) > @@ +591,3 @@ > + resource = wl_resource_create (client, > + &zxdg_output_manager_v1_interface, > + MIN (META_ZXDG_OUTPUT_V1_VERSION, version), > > It's up to the client to decide the version. We must only handle it, and > disconnect a miss behaving client (e.g. if it tries to bind a version newer > than we support). I'm confused, I may have misunderstood your previous review in comment 18: > [...] this should be "MIN (META_ZXDG_OUTPUT_V1_VERSION, version)", but since > we always get 1 anyway makes it not that important. So I did change to MIN (META_ZXDG_OUTPUT_V1_VERSION, version)
(In reply to Olivier Fourdan from comment #30) > (In reply to Jonas Ådahl from comment #28) > > Review of attachment 360114 [details] [review] [review] [review]: > > Hi Jonas, thanks for the review! > > > ::: src/wayland/meta-wayland-outputs.c > > @@ +269,3 @@ > > wayland_output->refresh_rate); > > > > + send_output_events (resource, wayland_output, logical_monitor, TRUE, > > NULL); > > > > Still need to send wl_output_done() here right? > > Err, nope, wl_output_done() is sent from send_output_events() because we > passed TRUE for “need_all_events”) Ah, right, you're right, missed that part. > > > @@ +591,3 @@ > > + resource = wl_resource_create (client, > > + &zxdg_output_manager_v1_interface, > > + MIN (META_ZXDG_OUTPUT_V1_VERSION, version), > > > > It's up to the client to decide the version. We must only handle it, and > > disconnect a miss behaving client (e.g. if it tries to bind a version newer > > than we support). > > I'm confused, I may have misunderstood your previous review in comment 18: > > > [...] this should be "MIN (META_ZXDG_OUTPUT_V1_VERSION, version)", but since > > we always get 1 anyway makes it not that important. > > So I did change to MIN (META_ZXDG_OUTPUT_V1_VERSION, version) Um, I must have confused myself. That is what clients should do :P sorry about that. The correct thing is to detect bad clients that request the wrong version, but in general in mutter we just takes the version from the client. I suggest to just pass "version", and then one day we can add sanity checks to all the globals.
Created attachment 365575 [details] [review] [PATCH 2/2 v4] wayland: Advertise MetaMonitor as wl_output (In reply to Jonas Ådahl from comment #29) > Review of attachment 360115 [details] [review] [review]: > > s/anymor eabout/anymore about/. Ah sorry, fixed in the commit message! > Would it make sense to conditionalize not lying on xdg_output being > supported? Would avoid a stream of bug reports for the period when the X > server does not yet support xdg_output. I don't see how to tell whether or not mutter should be lying to Xwayland... But the plan is to merge the first patch here (“Add xdg-output support”, patch 1/2) which basically does exactly the same as wl_output right now (so no discrepancy, no bug, basically no chnage whatsoever from a user point of view) Then get the Xwayland patch to add xdg-output support reviewed and possibly merged: https://patchwork.freedesktop.org/patch/175575/ Once xdg-output supports has landed in Xwayland, we can merge this second patch (patch 2/2, “Advertise MetaMonitor as wl_output”) so that wl_output in mutter behaves as per the spec. And at last, merge the second Xwayland patch which scales the output in Xwayland so that Xrandr in weston give the appropriate size: https://patchwork.freedesktop.org/patch/175578/
(In reply to Olivier Fourdan from comment #32) > Created attachment 365575 [details] [review] [review] > [PATCH 2/2 v4] wayland: Advertise MetaMonitor as wl_output > > (In reply to Jonas Ådahl from comment #29) > > Review of attachment 360115 [details] [review] [review] [review]: > > > > s/anymor eabout/anymore about/. > > Ah sorry, fixed in the commit message! > > > Would it make sense to conditionalize not lying on xdg_output being > > supported? Would avoid a stream of bug reports for the period when the X > > server does not yet support xdg_output. > > I don't see how to tell whether or not mutter should be lying to Xwayland... > > But the plan is to merge the first patch here (“Add xdg-output support”, > patch 1/2) which basically does exactly the same as wl_output right now (so > no discrepancy, no bug, basically no chnage whatsoever from a user point of > view) > > Then get the Xwayland patch to add xdg-output support reviewed and possibly > merged: https://patchwork.freedesktop.org/patch/175575/ > > Once xdg-output supports has landed in Xwayland, we can merge this second > patch (patch 2/2, “Advertise MetaMonitor as wl_output”) so that wl_output in > mutter behaves as per the spec. > > And at last, merge the second Xwayland patch which scales the output in > Xwayland so that Xrandr in weston give the appropriate size: > https://patchwork.freedesktop.org/patch/175578/ Fair enough. I guess detecting whether a client supports xdg_output (i.e. by iterating through the xdg_output resources and checking the client) is a bit too much of a hack just to avoid some bug reports.
(In reply to Jonas Ådahl from comment #33) > Fair enough. I guess detecting whether a client supports xdg_output (i.e. by > iterating through the xdg_output resources and checking the client) is a bit > too much of a hack just to avoid some bug reports. Oh no, it's not that (I ain't afraid on no hacks! ;-) ), but the problem I see is that the wl_output modes list is sent at “bind” time [1] and we (I mean, the compositor) may not know at that time whether or not the client supports xdg-output as well. [1] https://wayland.freedesktop.org/docs/html/apa.html#protocol-spec-wl_output “The event is sent when binding to the output object and there will always be one mode, the current mode. The event is sent again if an output changes mode, for the mode that is now current. In other words, the current mode is always the last mode that was received with the current flag set.”
(granted, mutter doesn't send any list of modes but the current one, but we may need -or want- to send a list in a near future)
(In reply to Olivier Fourdan from comment #34) > (In reply to Jonas Ådahl from comment #33) > > Fair enough. I guess detecting whether a client supports xdg_output (i.e. by > > iterating through the xdg_output resources and checking the client) is a bit > > too much of a hack just to avoid some bug reports. > > Oh no, it's not that (I ain't afraid on no hacks! ;-) ), but the problem I > see is that the wl_output modes list is sent at “bind” time [1] and we (I > mean, the compositor) may not know at that time whether or not the client > supports xdg-output as well. > > [1] > https://wayland.freedesktop.org/docs/html/apa.html#protocol-spec-wl_output > > “The event is sent when binding to the output object and there will always > be one mode, the current mode. The event is sent again if an output changes > mode, for the mode that is now current. In other words, the current mode is > always the last mode that was received with the current flag set.” Good point! We'd have to always send the fake wl_output state, then only send the correct state after xdg_output was bound, which would result in clients having wierd intermediate states. Careful ordering of patch landing seems a lot more sane than that.
Created attachment 365582 [details] [review] [PATCH 1/2 v4] wayland: Add xdg-output support (In reply to Jonas Ådahl from comment #31) > [...[ > Um, I must have confused myself. That is what clients should do :P sorry > about that. The correct thing is to detect bad clients that request the > wrong version, but in general in mutter we just takes the version from the > client. I suggest to just pass "version", and then one day we can add sanity > checks to all the globals. Thanks, done!
Review of attachment 365582 [details] [review]: lgtm. just a naming nit. don't need to upload a new patch here for that. ::: src/wayland/meta-wayland-outputs.c @@ +582,3 @@ + +static void +bind_xdg_output (struct wl_client *client, name nit: bind_xdg_output_manager
Review of attachment 365575 [details] [review]: The commit message is missleading I think. We don't advertise MetaMonitor as wl_output, because we still only advertise one wl_output per MetaLogicalMonitor. The difference is that the mode we advertise is the actual mode, not the logical monitor dimension.
Comment on attachment 365582 [details] [review] [PATCH 1/2 v4] wayland: Add xdg-output support attachment 365582 [details] [review] with change from comment 38 (renamed bind_xdg_output() to bind_xdg_output_manager()) pushed to git master as commit db32047 - wayland: Add xdg-output support
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/mutter/-/issues/ Thank you for your understanding and your help.