GNOME Bugzilla – Bug 766528
wayland-outputs: Refactor event sending to ensure we're consistent
Last modified: 2016-05-20 13:48:26 UTC
This makes us behave the same both on bind and when an output changes. In particular, we were not sending scale and done events on output changes. We were also unconditionally sending mode events on output changes even though these should only be sent if there is an actual mode change.
Created attachment 328004 [details] [review] wayland-outputs: Refactor event sending to ensure we're consistent
Review of attachment 328004 [details] [review]: ::: src/wayland/meta-wayland-outputs.c @@ +64,3 @@ +send_output (struct wl_resource *resource, + MetaWaylandOutput *wayland_output, + MetaMonitorInfo *monitor_info) nit: "send_output" sounds like we are sending an output, but we are sending the output metadata/state/info. Maybe rename it to reflect that? nit: While at it, align the function argument names. @@ +79,3 @@ + gboolean need_done = FALSE; + + if (old_monitor_info == monitor_info || Shouldn't this be "!old_monitor_info || old_monitor_info != monitor_info"? Why would you always send the new state if the monitor info didn't change? Reading the code some more it seems that "old_monitor_info" sometimes isn't the old monitor info, but the current one, since in the binding case it wasn't changed. That is a bit confusing. Maybe just have a flag to force the function to send all state, for the binding case? @@ +99,3 @@ + mode_flags |= WL_OUTPUT_MODE_PREFERRED; + + if (old_monitor_info == monitor_info || Same as above. @@ +115,3 @@ + if (version >= WL_OUTPUT_SCALE_SINCE_VERSION) + { + if (old_monitor_info == monitor_info || Same as above. @@ +164,3 @@ +static void +wayland_output_set_monitor_info (MetaWaylandOutput *wayland_output, + MetaMonitorInfo *monitor_info) nit: Align argument names.
Created attachment 328084 [details] [review] wayland-outputs: Refactor event sending to ensure we're consistent -- (In reply to Jonas Ådahl from comment #2) > nit: "send_output" sounds like we are sending an output, but we are sending > the output metadata/state/info. Maybe rename it to reflect that? True, changed to send_output_events as that's literally what it does > Shouldn't this be "!old_monitor_info || old_monitor_info != monitor_info"? old_monitor_info won't ever be NULL here > Why would you always send the new state if the monitor info didn't change? > > Reading the code some more it seems that "old_monitor_info" sometimes isn't > the old monitor info, but the current one, since in the binding case it > wasn't changed. Right, that's what I intended > That is a bit confusing. Maybe just have a flag to force the > function to send all state, for the binding case? Agreed, I changed it to have an explicit argument to send all events for the bind case now.
Review of attachment 328084 [details] [review]: Looks good to me.
I tested this patch and can attest it fixes the missing "output done" event (see https://bugs.freedesktop.org/show_bug.cgi?id=95491)
Attachment 328084 [details] pushed as 15300ae - wayland-outputs: Refactor event sending to ensure we're consistent