GNOME Bugzilla – Bug 768138
Update the GDK-Mir backend to fix a few problems
Last modified: 2016-08-12 10:15:12 UTC
Created attachment 330483 [details] [review] The refreshed version of the refreshed mir backend Please find a patch against the mir-gdk backend attached. It is based on the patch ubuntu xenial currently adds to gtk-3.18.9. It changes the way the MirSurface used to represent the GdkWindow is created. It uses the MirSurfaceSpec API to only update the surfaces when necessary. Still the creation is delayed until the necessary properties are available. Additionally further GdkWindow properties are wired to MirSurface configuration options - like output scaling, or geometry hints.
Created attachment 330543 [details] [review] Make use of MirSurfaceOutputEvent
Created attachment 330544 [details] [review] GCC warning fix
Created attachment 330545 [details] [review] Bumps required minimum mir version and resolves another warning
Created attachment 330546 [details] [review] Forward repeats as additional down events
Created attachment 330547 [details] [review] Avoids recreating the surface on simple changes like resize...
Created attachment 330548 [details] [review] Do not apply surface state changes to mir when another state change is already in flight
Created attachment 330549 [details] [review] Window title property
Created attachment 330550 [details] [review] Support for geometry hints
Created attachment 330551 [details] [review] Apply a type hint change only if it would change the mir surface type
Created attachment 330552 [details] [review] Use GdkFrameClock for redraw This change was taken from Marco Trevisans github branches
Thats all I have so far. Will add more when mir 0.24 is released.
I'm not sure who is going to review mir patches - William ?
Review of attachment 330545 [details] [review]: Looks good
Review of attachment 330544 [details] [review]: I didn't reproduce the warning, but it makes sense. Or alternatively, use a "default:" instead.
Review of attachment 330546 [details] [review]: Looks good
Review of attachment 330547 [details] [review]: ::: gdk/mir/gdkmirwindowimpl.c @@ +106,3 @@ + impl->type_hint = attributes->type_hint; + + return &impl->parent_instance; Can we cast this instead? I think returning the address of the parent_instance is not a common practice. @@ +179,3 @@ } +MirSurfaceSpec *create_window_type_spec (GdkDisplay *display, Should be static. @@ +293,3 @@ + MirSurfaceSpec *spec = create_spec (window, impl); + + mir_surface_apply_spec (impl->surface, spec); There needs to be a check to see if impl->surface actually exists, or a call to ensure_surface () before calling this. @@ +388,2 @@ static void +ensure_surface (GdkWindow *window, GdkMirWindowImpl *impl) Is there a reason why you added impl to the parameter list here? It seems like it was ok to just get it directly from the window itself. @@ +655,2 @@ /* If resize requested then rebuild surface */ + if (width >= 0 && (window->width != width || window->height != height)) I guess update_surface_spec () needs to be called in this case too.
Review of attachment 330548 [details] [review]: What's the meaning of "pending_spec_update" here? In some cases, you update the surface spec if it's TRUE, in other cases you only do it when it's FALSE. And update_surface_spec () changes it to FALSE, so I would expect it to do nothing if it's already FALSE.
Review of attachment 330549 [details] [review]: Looks good, just a couple of whitespace nits. ::: gdk/mir/gdkmirwindowimpl.c @@ +126,3 @@ + impl->title = g_strdup(attributes->title); + else + impl->title = g_strdup(get_default_title ()); Minor whitespace nit. @@ +981,3 @@ + { + MirSurfaceSpec* spec = mir_connection_create_spec_for_changes (connection); + mir_surface_spec_set_name(spec, impl->title); Minor whitespace nit.
Review of attachment 330549 [details] [review]: ::: gdk/mir/gdkmirwindowimpl.c @@ +74,3 @@ + + GdkGeometry geometry_hints; + GdkWindowHints geometry_mask; I guess these two should be in the next commit.
Review of attachment 330550 [details] [review]: ::: gdk/mir/gdkmirwindowimpl.c @@ +325,3 @@ + { + mir_surface_spec_set_min_aspect_ratio (spec, 1000, (unsigned)(1000.0/impl->geometry_hints.min_aspect)); + mir_surface_spec_set_min_aspect_ratio (spec, 1000, (unsigned)(1000.0/impl->geometry_hints.max_aspect)); I guess this should be mir_surface_spec_set_max_aspect_ratio ().
Review of attachment 330551 [details] [review]: Looks good. ::: gdk/mir/gdkmirwindowimpl.c @@ +981,3 @@ GdkMirWindowImpl *impl = GDK_MIR_WINDOW_IMPL (window->impl); + if (type_hint_differs(hint, impl->type_hint)) Minor whitespace nit.
Review of attachment 330550 [details] [review]: Looks good, it just needs the one min -> max change.
Review of attachment 330552 [details] [review]: Looks reasonable, just a couple of nits. ::: gdk/mir/gdkmirwindowimpl.c @@ +89,3 @@ gboolean cursor_inside; + gboolean pending_redraw; What's this field intended for? I don't see anything in the diff that's using it. @@ +593,3 @@ + return; + + impl->pending_commit = TRUE; This could just be inlined with send_buffer ().
Review of attachment 330543 [details] [review]: It looks good to me in principle with some minor corrections, but I'm not sure how to test this without a way of generating surface output events that dynamically change the output scale factor. ::: gdk/mir/gdkmirwindowimpl.c @@ +340,3 @@ impl->type_hint, buffer_usage); + impl->buffer_stream = mir_surface_get_buffer_stream (impl->surface); This call seems to be deprecated in Mir. @@ +1256,3 @@ + // //g_printerr ("gdk_mir_window_impl_get_scale_factor window=%p\n", window); + GdkMirWindowImpl *impl = GDK_MIR_WINDOW_IMPL (window->impl); + return round (impl->output_scale); I'm wondering why this doesn't generate a warning when round returns double and a gint is expected. @@ +1520,3 @@ + GdkMirWindowImpl *impl = GDK_MIR_WINDOW_IMPL (window->impl); + + if (impl->output_scale != scale) Just a nitpick, but maybe we should check to see if round (impl->output_scale) != round (scale), and possibly only store the rounded scale factor instead.
Review of attachment 330548 [details] [review]: Ok, I think I understand what's happening here now. You're trying to postpone the spec update until the next ensure_surface_full () and send_buffer (), and updating it immediately in all the other cases. Seems reasonable to me. ::: gdk/mir/gdkmirwindowimpl.c @@ +364,3 @@ + { + if (impl->pending_spec_update) + update_surface_spec(window); Whitespace nit.
Created attachment 331329 [details] [review] Make use of MirSurfaceOutputEvent As you suggested - now storing an integer scaling factor. Also now triggers a redraw to actually apply the new scaling. And only if the rounded scaling factor differs from the old factor. I tested that with a not yet proposed mir_demo_server that comes up with scale factors based on the physical size and single viewer distance.
Created attachment 331344 [details] [review] Avoids recreating the surface on simple changes like resize... Reverted the ensure_surface change - which was a left over from an intermediate attempt to make the surface creation happen only once and early in the process.
Created attachment 331345 [details] [review] Do not apply surface state changes to mir when another state change is already in flight Patch update because context changed from previous patch.
Created attachment 331346 [details] [review] Window title property Split out changes that belong into geometry hints patch.
Created attachment 331347 [details] [review] Support for geometry hints s/min/max in second aspect ratio line, and some additional hunks that accidentally turned up in the window title patch.
Created attachment 331348 [details] [review] Apply a type hint change only if it would change the mir surface type white space fixed..
Created attachment 331349 [details] [review] Make use of MirSurfaceOutputEvent
Created attachment 331350 [details] [review] Switch to mir_surface_spec_set_streams to avoid the deprecated mir_surface_buffer_stream This addresses one of the comments above about mir_surface_get_buffer_stream being deprecated. With more work around this function we could also support the subsurface window type in mir.
Created attachment 331351 [details] [review] Make use of MirSurfaceOutputEvent Outch, accidently replaced the wrong patch file.. As you suggested - now storing an integer scaling factor. Also now triggers a redraw to actually apply the new scaling. And only if the rounded scaling factor differs from the old factor. I tested that with a not yet proposed mir_demo_server that comes up with scale factors based on the physical size and single viewer distance.
Created attachment 331352 [details] [review] Use GdkFrameClock for redraw
Created attachment 331716 [details] [review] [PATCH 01/11] Use the surface output event to keep track of the suggested scale value
Created attachment 331717 [details] [review] [PATCH 02/11] Fix gcc warning on potentially uninitialized gdk_event.
Created attachment 331718 [details] [review] [PATCH 03/11] Fix warning on newer version so mir 0.22 and newer
Created attachment 331719 [details] [review] [PATCH 04/11] Forward repeated key presses as further down keys
Created attachment 331720 [details] [review] [PATCH 05/11] Rework window construction - only recreate surface when necessary
Created attachment 331721 [details] [review] [PATCH 06/11] Only update surface spec when there is no spec change pending
Created attachment 331722 [details] [review] [PATCH 07/11] Apply and forward title changes of gdk windows
Created attachment 331723 [details] [review] [PATCH 08/10] Apply geometry hints to mir surface
Created attachment 331724 [details] [review] [PATCH 08/11] Apply geometry hints to mir surface
Created attachment 331725 [details] [review] [PATCH 09/11] Only apply type hint if it would map to a different mir surface type
Created attachment 331726 [details] [review] [PATCH 10/11] Make use GdkFrameClock
Created attachment 331727 [details] [review] [PATCH 11/11] Switch to manually specifying MirBufferStreams for surface composition
Review of attachment 331716 [details] [review]: It looks good mostly, but I'd like to test it with scale changing if you know a way to do that. Can you also address the gl_paint_context case and the uninitialized area, and possibly the deprecated mir_surface_get_buffer_stream ()? ::: gdk/mir/gdkmireventsource.c @@ +551,3 @@ + _gdk_mir_window_set_surface_output (window, + mir_surface_output_event_get_scale (event) + ); Nit: this might be better off as a single line. ::: gdk/mir/gdkmirwindowimpl.c @@ +63,3 @@ MirSurface *surface; + MirBufferStream *buffer_stream; + Nit: whitespace @@ +348,3 @@ impl->type_hint, buffer_usage); + impl->buffer_stream = mir_surface_get_buffer_stream (impl->surface); This function seems to be deprecated. @@ +489,3 @@ region.height, region.stride); + cairo_surface_set_device_scale (cairo_surface, (double) impl->output_scale, (double) impl->output_scale); I think gint is implicitly cast to double here, so the explicit cast isn't necessary. Anyways, does this also have to be done in the other case when the window has a gl_paint_context too? @@ +1263,3 @@ gdk_mir_window_impl_get_scale_factor (GdkWindow *window) { + // //g_printerr ("gdk_mir_window_impl_get_scale_factor window=%p\n", window); Nit: This seems to be commented twice. @@ +1526,3 @@ +_gdk_mir_window_set_surface_output (GdkWindow *window, gdouble scale) +{ + // g_printerr ("_gdk_mir_window_impl_set_surface_output impl=%p %s\n", impl); It's commented out, but there are two format specifiers here and only one argument. @@ +1541,3 @@ + mir_buffer_stream_set_scale (impl->buffer_stream, (float) new_scale); + + region = cairo_region_create_rectangle (&area); area hasn't been initialized. Should it be set to the full area of the window here?
Review of attachment 331717 [details] [review]: I didn't reproduce the warning, but it makes sense. Or alternatively, use a "default:" instead.
Review of attachment 331718 [details] [review]: Looks good.
Review of attachment 331719 [details] [review]: Looks good. ::: gdk/mir/gdkmireventsource.c @@ +291,3 @@ return; + // FIXME: Convert keycode - which is already converted to xkb keysym - so what needs converting? I guess this comment can be removed entirely.
(In reply to William Hua from comment #48) > Review of attachment 331716 [details] [review] [review]: > > It looks good mostly, but I'd like to test it with scale changing if you > know a way to do that. > > Can you also address the gl_paint_context case and the uninitialized area, > and possibly the deprecated mir_surface_get_buffer_stream ()? > > ::: gdk/mir/gdkmireventsource.c > @@ +551,3 @@ > + _gdk_mir_window_set_surface_output (window, > + mir_surface_output_event_get_scale > (event) > + ); > > Nit: this might be better off as a single line. > > ::: gdk/mir/gdkmirwindowimpl.c > @@ +63,3 @@ > MirSurface *surface; > + MirBufferStream *buffer_stream; > + > > Nit: whitespace > > @@ +348,3 @@ > impl->type_hint, > buffer_usage); > + impl->buffer_stream = mir_surface_get_buffer_stream (impl->surface); > > This function seems to be deprecated. I resolved that in the eleventh patch > @@ +489,3 @@ > region.height, > region.stride); > + cairo_surface_set_device_scale (cairo_surface, (double) > impl->output_scale, (double) impl->output_scale); > > I think gint is implicitly cast to double here, so the explicit cast isn't > necessary. > > Anyways, does this also have to be done in the other case when the window > has a gl_paint_context too? Yes.. but I am unsure about the gl paint context. Is there still cairo involved?
Review of attachment 331720 [details] [review]: Seems ok mostly, but needs some clarification. ::: gdk/mir/gdkmirwindowimpl.c @@ +112,3 @@ + + if (attributes_mask & GDK_WA_TYPE_HINT) + impl->type_hint = attributes->type_hint; I'd probably add a check to make sure attributes isn't NULL here, just in case. Luckily it doesn't crash because the mask is zero in the case where it is NULL. @@ -277,1 @@ - mir_surface_spec_set_name (spec, g_get_prgname ()); Was this intentionally removed? Do we want to keep this in the new patch? @@ +359,3 @@ static void ensure_surface_full (GdkWindow *window, + GdkMirWindowImpl *impl, Same comment about the impl here. Is it necessary? It seems like it's always the same one obtainable from the window. @@ +404,3 @@ { + ensure_surface_full (window, + GDK_MIR_WINDOW_IMPL (window), This should say "window->impl". @@ +412,2 @@ static void +ensure_no_surface (GdkWindow *window, GdkMirWindowImpl *impl) Same thing with the impl above. @@ +601,3 @@ + ensure_surface (window); + + set_surface_state (impl, mir_surface_state_hidden); Just an opinion. Do you think it might be better to check if the surface exists and set its state to hidden instead of ensuring it here? @@ +614,3 @@ + ensure_surface (window); + + set_surface_state (impl, mir_surface_state_hidden); Same question as above. @@ +673,3 @@ /* We accept any resize */ window->width = width; window->height = height; Should the surface spec be updated here?
Created attachment 331772 [details] [review] [PATCH 01/11] Use the surface output event to keep track of the suggested scale value
Created attachment 331773 [details] [review] [PATCH 02/11] Fix gcc warning on potentially uninitialized gdk_event.
Created attachment 331774 [details] [review] [PATCH 03/11] Fix warning on newer version so mir 0.22 and newer
Created attachment 331775 [details] [review] [PATCH 04/11] Forward repeated key presses as further down keys
Created attachment 331776 [details] [review] [PATCH 05/11] Rework window construction - only recreate surface when necessary
Created attachment 331777 [details] [review] [PATCH 06/11] Only update surface spec when there is no spec change pending
Created attachment 331778 [details] [review] [PATCH 07/11] Apply and forward title changes of gdk windows
Created attachment 331779 [details] [review] [PATCH 08/11] Apply geometry hints to mir surface
Created attachment 331780 [details] [review] [PATCH 09/11] Only apply type hint if it would map to a different mir surface type
Created attachment 331781 [details] [review] [PATCH 10/11] Make use GdkFrameClock
Created attachment 331782 [details] [review] [PATCH 11/11] Switch to manually specifying MirBufferStreams for surface composition
(In reply to William Hua from comment #53) > Review of attachment 331720 [details] [review] [review]: > > Seems ok mostly, but needs some clarification. > > ::: gdk/mir/gdkmirwindowimpl.c > @@ +112,3 @@ > + > + if (attributes_mask & GDK_WA_TYPE_HINT) > + impl->type_hint = attributes->type_hint; > > I'd probably add a check to make sure attributes isn't NULL here, just in > case. Luckily it doesn't crash because the mask is zero in the case where it > is NULL. done > @@ -277,1 @@ > - mir_surface_spec_set_name (spec, g_get_prgname ()); > > Was this intentionally removed? Do we want to keep this in the new patch? yes and a version of that is added in the patch about setting the title > @@ +359,3 @@ > static void > ensure_surface_full (GdkWindow *window, > + GdkMirWindowImpl *impl, > > Same comment about the impl here. Is it necessary? It seems like it's always > the same one obtainable from the window. yes cleaned that up.. > > @@ +404,3 @@ > { > + ensure_surface_full (window, > + GDK_MIR_WINDOW_IMPL (window), > > This should say "window->impl". ouch .. thats a left over caused by me rebasing stuff back and forth between the ubuntu verson and the upstream version. Should be fixed now > > @@ +412,2 @@ > static void > +ensure_no_surface (GdkWindow *window, GdkMirWindowImpl *impl) > > Same thing with the impl above. done removes that > @@ +601,3 @@ > + ensure_surface (window); > + > + set_surface_state (impl, mir_surface_state_hidden); > > Just an opinion. Do you think it might be better to check if the surface > exists and set its state to hidden instead of ensuring it here? I do that in set_surface_state now.. and also no longer have the ensure_surface there. It is really not necessary. > > @@ +614,3 @@ > + ensure_surface (window); > + > + set_surface_state (impl, mir_surface_state_hidden); > > Same question as above. > > @@ +673,3 @@ > /* We accept any resize */ > window->width = width; > window->height = height; > > Should the surface spec be updated here? At the moment it is not necessary. The size of the buffer stream already got changed by the server .. or via an output scale event. There is no need yet to communicate that back at the moment. It will be necessary for output scale events in releases after mir-0.24.
Review of attachment 331773 [details] [review]: OK
Review of attachment 331774 [details] [review]: Looks good.
Review of attachment 331775 [details] [review]: Looks good.
Review of attachment 331776 [details] [review]: Thanks, this looks good. It needs to be rebased on top of master though because of the recent GtkMenu changes. ::: gdk/mir/gdkmirwindowimpl.c @@ +110,3 @@ + impl->display = display; + + if (attributes && attributes_mask & GDK_WA_TYPE_HINT) Hmm, just an opinion, but I'd recommend parentheses here for readability, even though the default precedence should do the right thing. Feel free to ignore if you want though.
Review of attachment 331777 [details] [review]: ::: gdk/mir/gdkmirwindowimpl.c @@ +184,3 @@ + mir_surface_apply_spec (impl->surface, spec); + mir_surface_spec_release (spec); + } I don't understand how this works. If impl->pending_spec_update == TRUE, and set_surface_state () is called, then the surface state isn't changed in Mir. But impl->surface_state is never used anywhere except for the comparison on line 176. So in that case, the surface state can be changed in Mir only if there is no pending spec update. If there is one pending, then the request seems to be completely ignored...
Review of attachment 331778 [details] [review]: Looks good. ::: gdk/mir/gdkmirwindowimpl.c @@ +127,3 @@ impl->display = display; + if (attributes && attributes_mask & GDK_WA_TITLE) Same recommendation about parentheses here. @@ +130,3 @@ + impl->title = g_strdup (attributes->title); + else + impl->title = g_strdup (get_default_title ()); Question: is it a problem at all for Mir if two windows have the same title?
Review of attachment 331779 [details] [review]: Looks good.
Review of attachment 331780 [details] [review]: ::: gdk/mir/gdkmirwindowimpl.c @@ +141,3 @@ + } +} + If I might make a suggestion, this is duplicating the switch statement in create_window_type_spec (). Instead, we could have a function "get_mir_surface_type_for_hint ()" so that if Mir adds more surface types, we only have to change that one function. And type_hint_differs () would just check if get_mir_surface_type_for_hint () returns differently.
Review of attachment 331781 [details] [review]: ::: gdk/mir/gdkmirwindowimpl.c @@ +972,3 @@ //g_printerr ("gdk_mir_window_impl_begin_paint window=%p\n", window); /* Indicate we are ready to be drawn onto directly? */ + return impl->pending_swap; Is this the right thing to return here? Shouldn't it return FALSE because the return value is used to determine if a copy of the paint surface should be created or not?
Created attachment 332770 [details] [review] [PATCH 01/13] Use the surface output event to keep track of the suggested scale value
Created attachment 332771 [details] [review] [PATCH 02/13] Fix gcc warning on potentially uninitialized gdk_event.
Created attachment 332772 [details] [review] [PATCH 03/13] Fix warning on newer version so mir 0.22 and newer
Created attachment 332773 [details] [review] [PATCH 04/13] Forward repeated key presses as further down keys
Created attachment 332774 [details] [review] [PATCH 05/13] Rework window construction - only recreate surface when necessary
Created attachment 332775 [details] [review] [PATCH 06/13] Only update surface spec when there is no spec change pending
Created attachment 332776 [details] [review] [PATCH 07/13] Apply and forward title changes of gdk windows
Created attachment 332777 [details] [review] [PATCH 08/13] Apply geometry hints to mir surface
Created attachment 332778 [details] [review] [PATCH 09/13] Only apply type hint if it would map to a different mir surface type
Created attachment 332779 [details] [review] [PATCH 10/13] Make use GdkFrameClock
Created attachment 332780 [details] [review] [PATCH 11/13] Fix execution of dialog
Created attachment 332781 [details] [review] [PATCH 12/13] Use Menus to implement tooltips
Created attachment 332782 [details] [review] [PATCH 13/13] Remove outdated comments
@william(In reply to William Hua from comment #73) > Review of attachment 331780 [details] [review] [review]: > > ::: gdk/mir/gdkmirwindowimpl.c > @@ +141,3 @@ > + } > +} > + > > If I might make a suggestion, this is duplicating the switch statement in > create_window_type_spec (). Instead, we could have a function > "get_mir_surface_type_for_hint ()" so that if Mir adds more surface types, > we only have to change that one function. > > And type_hint_differs () would just check if get_mir_surface_type_for_hint > () returns differently. Yeah I tried that but mirs surface type enum does not reflect modal dialogs. I will bring that up.
(In reply to William Hua from comment #74) > Review of attachment 331781 [details] [review] [review]: > > ::: gdk/mir/gdkmirwindowimpl.c > @@ +972,3 @@ > //g_printerr ("gdk_mir_window_impl_begin_paint window=%p\n", window); > /* Indicate we are ready to be drawn onto directly? */ > + return impl->pending_swap; > > Is this the right thing to return here? Shouldn't it return FALSE because > the return value is used to determine if a copy of the paint surface should > be created or not? You are right.. The updated patch series has that fix included. Additionally I left the buffer stream change out, since this needs a newer mir release.
Review of attachment 332771 [details] [review]: OK
Review of attachment 332772 [details] [review]: OK
Review of attachment 332773 [details] [review]: OK
Review of attachment 332774 [details] [review]: Looks good.
Review of attachment 332775 [details] [review]: OK
Review of attachment 332776 [details] [review]: OK
Review of attachment 332777 [details] [review]: Looks OK. Just not sure if it's possible for min_aspect/max_aspect to be zero here. ::: gdk/mir/gdkmirwindowimpl.c @@ +351,3 @@ + { + mir_surface_spec_set_min_aspect_ratio (spec, 1000, (unsigned)(1000.0/impl->geometry_hints.min_aspect)); + mir_surface_spec_set_min_width (spec, impl->geometry_hints.min_width); Is division by zero possible here? Would it be better to do: mir_surface_spec_set_min_aspect_ratio (spec, (unsigned int) (1000.0 * impl->geometry_hints.min_aspect), 1000.0);
(In reply to Andreas Pokorny from comment #88) > @william(In reply to William Hua from comment #73) > > Review of attachment 331780 [details] [review] [review] [review]: > > > > ::: gdk/mir/gdkmirwindowimpl.c > > @@ +141,3 @@ > > + } > > +} > > + > > > > If I might make a suggestion, this is duplicating the switch statement in > > create_window_type_spec (). Instead, we could have a function > > "get_mir_surface_type_for_hint ()" so that if Mir adds more surface types, > > we only have to change that one function. > > > > And type_hint_differs () would just check if get_mir_surface_type_for_hint > > () returns differently. > > Yeah I tried that but mirs surface type enum does not reflect modal dialogs. > I will bring that up. What exactly do you mean by this? There's a MirSurfaceType that doesn't map to a GdkWindowTypeHint?
(In reply to William Hua from comment #97) > (In reply to Andreas Pokorny from comment #88) > > @william(In reply to William Hua from comment #73) > > > Review of attachment 331780 [details] [review] [review] [review] [review]: > > > > > > ::: gdk/mir/gdkmirwindowimpl.c > > > @@ +141,3 @@ > > > + } > > > +} > > > + > > > > > > If I might make a suggestion, this is duplicating the switch statement in > > > create_window_type_spec (). Instead, we could have a function > > > "get_mir_surface_type_for_hint ()" so that if Mir adds more surface types, > > > we only have to change that one function. > > > > > > And type_hint_differs () would just check if get_mir_surface_type_for_hint > > > () returns differently. > > > > Yeah I tried that but mirs surface type enum does not reflect modal dialogs. > > I will bring that up. > > What exactly do you mean by this? There's a MirSurfaceType that doesn't map > to a GdkWindowTypeHint? The other way round .. MirSurfaceType does not differ between modal and "normal" dialog.
Review of attachment 332770 [details] [review]: OK
Review of attachment 332778 [details] [review]: OK
Review of attachment 332779 [details] [review]: I'm getting some pretty bad flickering because of this commit when popping up a menu under the mir demo server. Any idea why?
Review of attachment 332780 [details] [review]: OK
Review of attachment 332782 [details] [review]: OK
Review of attachment 332779 [details] [review]: Let's leave this out. We can fix it if we need it in the future.
Review of attachment 332781 [details] [review]: Looks good to me, the Mir demo server doesn't seem to be respecting changes to the menu adjacency rectangle though.
(In reply to William Hua from comment #105) > Review of attachment 332781 [details] [review] [review]: > > Looks good to me, the Mir demo server doesn't seem to be respecting changes > to the menu adjacency rectangle though. Yes thats a known problem addressed via miral - which will be reintegrated soon