Created attachment 280438 [details] [review]
Add a framework for restarting the compositor with nice visuals

Created attachment 280880 [details] [review]
Add a framework for restarting the compositor with nice visuals

The current GNOME Shell Alt-F2 restart looks very messy and also
provides no indication to the user what is going on. We need to
restart the compositor to switch in and out of stereo mode, so
add a framework for doing this more cleanly:

Additions:

 meta_restart(): restarts the compositor with a message
 MetaDisplay::show-restart-message: signal the embedding
    shell to show a message
 MetaDisplay::restart: signal the embedding shell to restart
    itself.
 meta_is_restart(): indicates whether the current instance is a
                    restart so we can suppress login animations.

A helper program meta-restart-helper holds the composite overlay
window up during the restart to avoid visual artifacts.

Created attachment 280881 [details] [review]
Add support for meta_restart() and MetaDisplay::restart

Support was added to Mutter to allow it to trigger a restart
to allow for restarts when switching in or out of stereo mode.

Hook up to the new signals on MetaDisplay to show the restart
message and reexec. Meta.is_restart() is used to suppress
the startup animation.

This also allows us to do 'Alt-F2 r' restarts more cleanly
without a visual flash and animation.

Review of attachment 280880 [details] [review]:

::: src/core/display.c
@@ +292,3 @@
+   *   indicates that the compositor did not show the message.
+   */
+  display_signals[SHOW_RESTART_MESSAGE] =

We usually do these as compositor vfuncs. Any reason you opted for signals on MetaDisplay instead?

::: src/core/restart-helper.c
@@ +12,3 @@
+
+/*
+ * Copyright (C) 2014 Red Hat, Inc.

I think we usually put the copyright header above the SECTION doc.

::: src/core/restart.c
@@ +82,3 @@
+      meta_warning ("Failed to read output from restart helper%s%s\n",
+                    error ? ": " : NULL,
+                    error ? error->message : NULL);

Is there a case where the error would be NULL when it failed?

@@ +115,3 @@
+ */
+void
+meta_restart (const char *message)

Can this be meta_display_restart (MetaDisplay *, const char *); ?  I don't like using the global display if we can pass it around.

@@ +157,3 @@
+      goto error;
+    }
+  else

I tend not to do:

     if (!foo)
       ...
     else
       ...

I just find the inversion is hard to follow. Since there's a goto in the block above, just remove the else?

@@ +177,3 @@
+
+ error:
+  restart_helper_started = TRUE;

This needs a comment, because it's an obvious lie.

@@ +184,3 @@
+
+void
+meta_restart_finish (void)

Change to meta_display_finish_restart (MetaDisplay *); ?

Review of attachment 280881 [details] [review]:

Looks OK.

(In reply to comment #4)
> Review of attachment 280880 [details] [review]:
> 
> ::: src/core/display.c
> @@ +292,3 @@
> +   *   indicates that the compositor did not show the message.
> +   */
> +  display_signals[SHOW_RESTART_MESSAGE] =
> 
> We usually do these as compositor vfuncs. Any reason you opted for signals on
> MetaDisplay instead?

You mean as plugin vfuncs? I guess my opinion was that the plugin system is dead and just there because we haven't prioritized removing it. We just convert the plugin vfuncs into signals anyways inside GNOME Shell.. (though conceivably we could not do that now that we have GObject inheritance in JS)

It also avoids an ABI break, which is slightly useful to me when patching RHEL 7 in that if somebody gets a mismatch on their system, their system doesn't fall over. (Package requirements should deal with that, of course.)
 
> ::: src/core/restart-helper.c
> @@ +12,3 @@
> +
> +/*
> + * Copyright (C) 2014 Red Hat, Inc.
> 
> I think we usually put the copyright header above the SECTION doc.

It varies ... in terms of files with useful section docs (which is pretty much only stack-tracker.c :-) the section docs are first. I can certainly put it them after.

> ::: src/core/restart.c
> @@ +82,3 @@
> +      meta_warning ("Failed to read output from restart helper%s%s\n",
> +                    error ? ": " : NULL,
> +                    error ? error->message : NULL);
> 
> Is there a case where the error would be NULL when it failed?

Yes - when the restart helper exits without printing anything.

> @@ +115,3 @@
> + */
> +void
> +meta_restart (const char *message)
> 
> Can this be meta_display_restart (MetaDisplay *, const char *); ?  I don't like
> using the global display if we can pass it around.

If you like.

> @@ +157,3 @@
> +      goto error;
> +    }
> +  else
> 
> I tend not to do:
> 
>      if (!foo)
>        ...
>      else
>        ...
> 
> I just find the inversion is hard to follow. Since there's a goto in the block
> above, just remove the else?

Sure

> @@ +177,3 @@
> +
> + error:
> +  restart_helper_started = TRUE;
> 
> This needs a comment, because it's an obvious lie.

OK.

> @@ +184,3 @@
> +
> +void
> +meta_restart_finish (void)
> 
> Change to meta_display_finish_restart (MetaDisplay *); ?

Let me know what you prefer with the signals vs. plugins and I'll make the other changes. I'd certainly prefer to keep the signal, but if you have a conception of the plugin system as something we're going to keep extending (or you meant something else by compositor vfuncs), I can be flexible.

I'm fine with either, I was just curious. I'm not the biggest fan of doing vfuncs-through-signals, but it's true that the plugin interface just requires a lot of unnecessary plumbing.

(In reply to comment #4)
> @@ +115,3 @@
> + */
> +void
> +meta_restart (const char *message)
> 
> Can this be meta_display_restart (MetaDisplay *, const char *); ?  I don't like
> using the global display if we can pass it around.

Actually, thinking about this, I don't agree with this change. Restarting is clearly of the Mutter process. Conceptually if we modified Mutter to allow multiple displays, you can't restart one and not the other. If we had multiple displays, this function would need to iterate over all displays and show a message and start a separate restart helper for each display. It's a global function like meta_run().

 > +void
> +meta_restart_finish (void)
> 
> Change to meta_display_finish_restart (MetaDisplay *); ?

Definitely not for this one (which is internal API) - it's closely paired with meta_restart_init() which is called before the MetaDisplay is open.

(In reply to comment #8)
> Actually, thinking about this, I don't agree with this change. Restarting is
> clearly of the Mutter process. Conceptually if we modified Mutter to allow
> multiple displays, you can't restart one and not the other.

Then it doesn't make sense for "restart" to be a signal on MetaDisplay, either. I understand the technicalities of why you did that (and it would make even *less* sense for "restart" to be a compositor vfunc, all things considered).

Can you, at the very least, not use meta_ui_get_display(); in meta_restart_finish, and instead move to meta_display_get_xdisplay (meta_get_display ()); ? I'm trying to reduce the dependency of the UI layer at various parts.

(In reply to comment #9)
> (In reply to comment #8)
> > Actually, thinking about this, I don't agree with this change. Restarting is
> > clearly of the Mutter process. Conceptually if we modified Mutter to allow
> > multiple displays, you can't restart one and not the other.
> 
> Then it doesn't make sense for "restart" to be a signal on MetaDisplay, either.
> I understand the technicalities of why you did that (and it would make even
> *less* sense for "restart" to be a compositor vfunc, all things considered).

Well, one of the signals is "display a restart message to the user", so it would be fine as a compositor vfunc ... and also makes sense conceptually on MetaDisplay, since a message has to be shown on a particular display. The operation to actually restart, less so.... but I'm certainly not going to add another object for the whole process (MetaDisplayFactory?) :-)

> Can you, at the very least, not use meta_ui_get_display(); in
> meta_restart_finish, and instead move to meta_display_get_xdisplay
> (meta_get_display ()); ? I'm trying to reduce the dependency of the UI layer at
> various parts.

OK. (Can't fix the paired meta_restart_init(), since that runs at meta_init() time before there is a MetaDisplay.)

Comment on attachment 280880 [details] [review]
Add a framework for restarting the compositor with nice visuals

Attachment 280880 [details] pushed as 3a57f84 - Add a framework for restarting the compositor with nice visuals

Attachment 280881 [details] pushed as b6f3e15 - Add support for meta_restart() and MetaDisplay::restart