After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 755605 - Fix stack - scene graph stacking synchronization issue
Fix stack - scene graph stacking synchronization issue
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
Depends on:
Blocks:
 
 
Reported: 2015-09-25 00:36 UTC by Jonas Ådahl
Modified: 2016-02-26 09:56 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
tests: Add unit tests framework runner (6.08 KB, patch)
2015-09-25 00:36 UTC, Jonas Ådahl
committed Details | Review
MetaLater: Invoke later callbacks queued by earlier ones (4.66 KB, patch)
2015-09-25 00:36 UTC, Jonas Ådahl
none Details | Review
tests: Test scheduling a later MetaLater from a later callback works (3.76 KB, patch)
2015-09-25 00:36 UTC, Jonas Ådahl
committed Details | Review
MetaLater: Invoke later callbacks queued by earlier ones (5.45 KB, patch)
2015-12-23 06:42 UTC, Jonas Ådahl
committed Details | Review

Description Jonas Ådahl 2015-09-25 00:36:03 UTC
We may update the stack from a META_LATER_CALC_SHOWING callback and as a result queue a META_LATER_SYNC_STACKING callback. This was broken due to queuing a MetaLater with a later 'when' only invoked that queued callback the frame after. These patches fixes this, and adds unit tests for testing a few MetaLater semantics.
Comment 1 Jonas Ådahl 2015-09-25 00:36:08 UTC
Created attachment 312096 [details] [review]
tests: Add unit tests framework runner

Separate from meta-test-runner which runs metatests testing window
manager operations, a new test program (mutter-unit-tests) is
introduced. This is meant to run unit test like tests on various units
in mutter.

An initial test testing the order of MetaLater callback invokation was
added.
Comment 2 Jonas Ådahl 2015-09-25 00:36:13 UTC
Created attachment 312097 [details] [review]
MetaLater: Invoke later callbacks queued by earlier ones

If a MetaLater callback queued another MetaLater with a scheduling
later than the one currently being invoked, make it so that the newly
scheduled callback will actually be invoked.

The fact that it doesn't already do this is a regression from
cd7a9680932868d1d2ef5447c77712cef9a443dd.
Comment 3 Jonas Ådahl 2015-09-25 00:36:18 UTC
Created attachment 312098 [details] [review]
tests: Test scheduling a later MetaLater from a later callback works

The added test tests for the bug fixed by
487e727ce0c9aac53afc8e53d4bcad8a2d7af5cd (MetaLater: Invoke later
callbacks queued by earlier ones).
Comment 4 Jonas Ådahl 2015-12-03 08:19:43 UTC
Ping.
Comment 5 Rui Matos 2015-12-17 16:56:58 UTC
Review of attachment 312097 [details] [review]:

The code looks correct. Just a few style comments below

::: src/core/util.c
@@ +741,3 @@
 
+static GSList *laters[] = {
+  NULL, /* META_LATER_RESIZE */

I find it more elegant to do this by appending a new enum value called META_LATER_LAST and then do:

static GSList *laters[META_LATER_LAST] = { 0 };

@@ +804,3 @@
 
+      if (!later->func || !later->func (later->data))
+        meta_later_remove (later->id);

The remove function could be refactored so that here we could call its inner loop functionality directly instead of iterating through all the lists.

@@ +818,3 @@
+  gboolean keep_timeline_running = FALSE;
+
+  for (i = 0; i < sizeof (laters) / sizeof (*laters); i++)

With META_LATER_LAST you wouldn't need the sizeof()s. Alternatively, glib has the G_N_ELEMENTS() macro.

@@ +825,3 @@
+  for (i = 0; i < sizeof (laters) / sizeof (*laters); i++)
+    {
+      for (l = laters[i]; l; l = l->next)

Instead of iterating again to find if there's a later to keep the timeline running, you could make run_repaint_later() return a boolean that tells you that.
Comment 6 Rui Matos 2015-12-17 17:04:56 UTC
Review of attachment 312096 [details] [review]:

looks fine
Comment 7 Rui Matos 2015-12-17 17:09:35 UTC
Review of attachment 312098 [details] [review]:

Dont forget to update the commit hash in the body message or just say "fixed by the previous commit".

Code looks good
Comment 8 Jonas Ådahl 2015-12-23 06:38:40 UTC
(In reply to Rui Matos from comment #5)
> Review of attachment 312097 [details] [review] [review]:
> 
> The code looks correct. Just a few style comments below
> 
> ::: src/core/util.c
> @@ +741,3 @@
>  
> +static GSList *laters[] = {
> +  NULL, /* META_LATER_RESIZE */
> 
> I find it more elegant to do this by appending a new enum value called
> META_LATER_LAST and then do:
> 
> static GSList *laters[META_LATER_LAST] = { 0 };
> 
> @@ +804,3 @@
>  
> +      if (!later->func || !later->func (later->data))
> +        meta_later_remove (later->id);
> 
> The remove function could be refactored so that here we could call its inner
> loop functionality directly instead of iterating through all the lists.
> 
> @@ +818,3 @@
> +  gboolean keep_timeline_running = FALSE;
> +
> +  for (i = 0; i < sizeof (laters) / sizeof (*laters); i++)
> 
> With META_LATER_LAST you wouldn't need the sizeof()s. Alternatively, glib
> has the G_N_ELEMENTS() macro.

That would need a public API change, so I don't think it's worth it.

> 
> @@ +825,3 @@
> +  for (i = 0; i < sizeof (laters) / sizeof (*laters); i++)
> +    {
> +      for (l = laters[i]; l; l = l->next)
> 
> Instead of iterating again to find if there's a later to keep the timeline
> running, you could make run_repaint_later() return a boolean that tells you
> that.

We need to iterate again because the state of the current laters could have changed while iterating through the list copy invoking the laters in run_repaint_laters().
Comment 9 Jonas Ådahl 2015-12-23 06:42:02 UTC
Created attachment 317808 [details] [review]
MetaLater: Invoke later callbacks queued by earlier ones

If a MetaLater callback queued another MetaLater with a scheduling
later than the one currently being invoked, make it so that the newly
scheduled callback will actually be invoked.

The fact that it doesn't already do this is a regression from
cd7a9680932868d1d2ef5447c77712cef9a443dd.
Comment 10 Rui Matos 2016-01-04 18:27:00 UTC
(In reply to Jonas Ådahl from comment #8)
> > With META_LATER_LAST you wouldn't need the sizeof()s. Alternatively, glib
> > has the G_N_ELEMENTS() macro.
> 
> That would need a public API change, so I don't think it's worth it.

Mutter's "public" API changes every release. In practice, gnome-shell is the only consumer so we can change it when needed.
Comment 11 Rui Matos 2016-01-04 18:27:33 UTC
Review of attachment 317808 [details] [review]:

Regardless of style preferences, this looks good
Comment 12 Jasper St. Pierre (not reading bugmail) 2016-01-04 18:28:44 UTC
(In reply to Rui Matos from comment #10)
> Mutter's "public" API changes every release. In practice, gnome-shell is the
> only consumer so we can change it when needed.

budgie and pantheon use it as well. That said, I'm comfortable with an API change like this.
Comment 13 Jonas Ådahl 2016-02-26 09:56:13 UTC
Had forgot to push these, so here it goes.

Attachment 312096 [details] pushed as 0013975 - tests: Add unit tests framework runner
Attachment 312098 [details] pushed as 821d737 - tests: Test scheduling a later MetaLater from a later callback works
Attachment 317808 [details] pushed as 35da6a9 - MetaLater: Invoke later callbacks queued by earlier ones