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.

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.

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).

Ping.

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.

Review of attachment 312096 [details] [review]:

looks fine

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

(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().

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.

(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.

Review of attachment 317808 [details] [review]:

Regardless of style preferences, this looks good

(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.

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