GNOME Bugzilla – Bug 770991
MetaWindowActor: Sync thawed state when surface actor is set
Last modified: 2016-09-09 03:10:34 UTC
We don't sync window actor state when surface is not set, as that makes the window actor considered "frozen". Setting the surface didn't do the same thing as thawing the window actor would. The attached patch fixes this. This makes for example the menus in Steam work properly.
Created attachment 334963 [details] [review] MetaWindowActor: Sync thawed state when surface actor is set Not having a surface actor would cause the window actor state to be considerde frozen, thus causing various state (such as geometry, shape etc) synchronization to be delayed until thawed. If the window actor was "thawed" due to having a surface set, not all state would be properly synchronized, causing the thawed window actor to be dispalyed incorrectly. This patch fixes this by putting state synhronization after thawing in a common function, calling it both from frozen count decreasing and surface setting. This fixes for example misplaced menus in Steam.
Review of attachment 334963 [details] [review]: Typos in commit message: "considerde frozen" "to be dispalyed" ::: src/compositor/meta-window-actor.c @@ +354,2 @@ { MetaWindowActorPrivate *priv = self->priv; Maybe add an is_frozen() early return for safety? @@ -404,3 @@ - /* If the previous surface actor was frozen, start out - * frozen as well... */ - meta_surface_actor_set_frozen (priv->surface, priv->freeze_count > 0); Is this intentional? If yes, then I think it is better off in a separate patch.
(In reply to Florian Müllner from comment #2) > Review of attachment 334963 [details] [review] [review]: > > Typos in commit message: > "considerde frozen" > "to be dispalyed" > > ::: src/compositor/meta-window-actor.c > @@ +354,2 @@ > { > MetaWindowActorPrivate *priv = self->priv; > > Maybe add an is_frozen() early return for safety? > > @@ -404,3 @@ > - /* If the previous surface actor was frozen, start out > - * frozen as well... */ > - meta_surface_actor_set_frozen (priv->surface, priv->freeze_count > 0); > > Is this intentional? If yes, then I think it is better off in a separate > patch. Yes. The idea is to put the "actually thaw" code in a common path. This was part of it.
(In reply to Jonas Ådahl from comment #3) > Yes. The idea is to put the "actually thaw" code in a common path. This was > part of it. I still don't understand that bit - there's no thawing in that case (because setting the surface doesn't decrease the freeze count), but we now have an unfrozen surface actor attached to a frozen window actor ...
(In reply to Florian Müllner from comment #4) > (In reply to Jonas Ådahl from comment #3) > > Yes. The idea is to put the "actually thaw" code in a common path. This was > > part of it. > > I still don't understand that bit - there's no thawing in that case (because > setting the surface doesn't decrease the freeze count), but we now have an > unfrozen surface actor attached to a frozen window actor ... I see now, should probably be if (is_frozen (self)) meta_window_actor_sync_actor_geometry (self, TRUE); else meta_window_actor_sync_thawed_state (self); in set_surface() to be correct.(In reply to Florian Müllner from comment #2) > > ::: src/compositor/meta-window-actor.c > @@ +354,2 @@ > { > MetaWindowActorPrivate *priv = self->priv; > > Maybe add an is_frozen() early return for safety? > Don't think its needed. All calls to this function have exactly that condition for making the call.
Created attachment 334973 [details] [review] MetaWindowActor: Sync thawed state when surface actor is set Not having a surface actor would cause the window actor state to be considered frozen, thus causing various state (such as geometry, shape etc) synchronization to be delayed until thawed. If the window actor was "thawed" due to having a surface set, not all state would be properly synchronized, causing the thawed window actor to be displayed incorrectly. This patch fixes this by putting state synchronization after thawing in a common function, calling it both from frozen count decreasing and surface setting. This fixes for example misplaced menus in Steam.
(In reply to Jonas Ådahl from comment #5) > (In reply to Florian Müllner from comment #4) > > (In reply to Jonas Ådahl from comment #3) > > > Yes. The idea is to put the "actually thaw" code in a common path. This was > > > part of it. > > > > I still don't understand that bit - there's no thawing in that case (because > > setting the surface doesn't decrease the freeze count), but we now have an > > unfrozen surface actor attached to a frozen window actor ... > > I see now, should probably be > > if (is_frozen (self)) > meta_window_actor_sync_actor_geometry (self, TRUE); > else > meta_window_actor_sync_thawed_state (self); > > in set_surface() to be correct. It's still not clear to me why meta_window_actor_update_surface (actor); meta_window_actor_freeze (actor); and meta_window_actor_freeze (actor); meta_window_actor_update_surface (actor); should behave differently and process damage immediately in one case and queue it for later processing later in the other. > (In reply to Florian Müllner from comment #2) > > Don't think its needed. All calls to this function have exactly that > condition for making the call. Sorry, I didn't want to imply that it was needed. The question was whether it makes sense to guard against additional calls being added in the future without that check (probably not ...)
(In reply to Florian Müllner from comment #7) > (In reply to Jonas Ådahl from comment #5) > > (In reply to Florian Müllner from comment #4) > > > (In reply to Jonas Ådahl from comment #3) > > > > Yes. The idea is to put the "actually thaw" code in a common path. This was > > > > part of it. > > > > > > I still don't understand that bit - there's no thawing in that case (because > > > setting the surface doesn't decrease the freeze count), but we now have an > > > unfrozen surface actor attached to a frozen window actor ... > > > > I see now, should probably be > > > > if (is_frozen (self)) > > meta_window_actor_sync_actor_geometry (self, TRUE); > > else > > meta_window_actor_sync_thawed_state (self); > > > > in set_surface() to be correct. > > It's still not clear to me why > > meta_window_actor_update_surface (actor); > meta_window_actor_freeze (actor); > > and > > meta_window_actor_freeze (actor); > meta_window_actor_update_surface (actor); > > should behave differently and process damage immediately in one case and > queue it for later processing later in the other. > I don't think it'll be different. meta_window_actor_update_surface (actor) would either way mark it as needing a reshape, since creating the shape depends on having a surface actor. The actual shape calculation is not done until pre paint if nothing is frozen though. The only difference I see is that if we freeze first, a repaint on the surface actor won't be queued (only the window actor, maybe that causes the surface actor (being a child of the window) to be queued implicitly?). If we want to make it more identical, I suppose update_surface() should set some kind of "queue redraw on thaw" boolean that we look at in sync_thawed_state().
(In reply to Jonas Ådahl from comment #8) > (In reply to Florian Müllner from comment #7) > > (In reply to Jonas Ådahl from comment #5) > > > (In reply to Florian Müllner from comment #4) > > > > (In reply to Jonas Ådahl from comment #3) > > > > > Yes. The idea is to put the "actually thaw" code in a common path. This was > > > > > part of it. > > > > > > > > I still don't understand that bit - there's no thawing in that case (because > > > > setting the surface doesn't decrease the freeze count), but we now have an > > > > unfrozen surface actor attached to a frozen window actor ... > > > > > > I see now, should probably be > > > > > > if (is_frozen (self)) > > > meta_window_actor_sync_actor_geometry (self, TRUE); > > > else > > > meta_window_actor_sync_thawed_state (self); > > > > > > in set_surface() to be correct. > > > > It's still not clear to me why > > > > meta_window_actor_update_surface (actor); > > meta_window_actor_freeze (actor); > > > > and > > > > meta_window_actor_freeze (actor); > > meta_window_actor_update_surface (actor); > > > > should behave differently and process damage immediately in one case and > > queue it for later processing later in the other. > > > > I don't think it'll be different. meta_window_actor_update_surface (actor) > would either way mark it as needing a reshape, since creating the shape > depends on having a surface actor. The actual shape calculation is not done > until pre paint if nothing is frozen though. Correction, it'll be done immediately if update_surface() is done before freezing. On the other hand, isn't that the point of freeze? Postpone any updates until thawed?
Created attachment 335116 [details] [review] MetaWindowActor: Sync thawed state when surface actor is set Not having a surface actor would cause the window actor state to be considered frozen, thus causing various state (such as geometry, shape etc) synchronization to be delayed until thawed. If the window actor was "thawed" due to having a surface set, not all state would be properly synchronized, causing the thawed window actor to be displayed incorrectly. This patch fixes this by putting state synchronization after thawing in a common function, calling it both from frozen count decreasing and surface setting. This fixes for example misplaced menus in Steam.
Review of attachment 335116 [details] [review]: LGTM now ::: src/compositor/meta-window-actor.c @@ +417,3 @@ + + if (is_frozen (self)) + meta_surface_actor_set_frozen (priv->surface, TRUE); This was the case I was worried about, thanks!
Attachment 335116 [details] pushed as 2e7f113 - MetaWindowActor: Sync thawed state when surface actor is set