Created attachment 160823 [details] [review]
[plugin] Create ShellGlobal later to avoid connecting to X during build

Comment on attachment 160823 [details] [review]
[plugin] Create ShellGlobal later to avoid connecting to X during build

Hm, this causes a panel regression...

Nice, that would also fix the GSettings issue where the program aborts if settings aren't installed yet.

Created attachment 160824 [details] [review]
[plugin] Create ShellGlobal later to avoid connecting to X during build

The ShellGlobal initialization performs several actions like connecting
to the X server, ensuring directories exist, etc., that are problematic
because we were creating the object even when running the binary for
introspection scanning.  During compilation we may not even have X11
available in e.g. autobuilder type environments, and it's just a
bad idea to connect even if we do.

Avoid this by deferring creation of the ShellGlobal object
until the plugin is actually started.  Now that we're initializing
things later, remove the connection to screen changes, and initialize
cached ShellGlobal state at the point when the plugin is set.

Review of attachment 160824 [details] [review]:

::: src/shell-global.c
@@ +508,3 @@
+  meta_later_add (META_LATER_BEFORE_REDRAW,
+                  on_screen_size_changed_cb,
+                  global,

I was nodding along to this patch, until I got to this, which raised all sorts of questions:

 * Is this mentioned in your commit message?

   Maybe? is that what you meant by "initialize cached ShellGlobal 
   state at the point when the plugin is set."

 * How was the screen size changed cb being called before on startup?

   Turns out that Clutter will emit notify::height and notify::width
   for the stage on the first time the stage is size allocated.

 * Why would your patch change whether that happens or not?

   I can't see why it would. Was this added because it seemed necessary
   rather than you hit problems?

 * Is it bad to call on_screen_size_changed_cb as late as "BEFORE_REDRAW".

   Well, it's probably better than what happens now, which I think is that
   the frame is allocated, then the screen-size-changed signal is emitted
   before the *next* frame, triggering an immediate relayout/repaint. 
   Because of the signal connection to Main.js:_relayout() I think you might
   even see the panel hop from from one monitor to another.

 * How *should* this be done?

   I think we should:
 
    A) Set last_change_screen_width, last_change_screen_height here
    B) Do the initial sizing on the root pixmap actor when we create it
    C) Not emit screen-size-changed at startup
    D) Make Main.js:_start() call _relayout() explicitly.

Created attachment 161231 [details] [review]
Create ShellGlobal later to avoid connecting to X during build

The ShellGlobal initialization performs several actions like connecting
to the X server, ensuring directories exist, etc., that are problematic
because we were creating the object even when running the binary for
introspection scanning.  During compilation we may not even have X11
available in e.g. autobuilder type environments, and it's just a
bad idea to connect even if we do.

Avoid this by deferring creation of the ShellGlobal object
until the plugin is actually started.

Now that we're initializing things later, remove the connection to
screen changes, and initialize cached ShellGlobal state at the point
when the plugin is set.  The root pixmap actor is now sized initially
on creation too.  Instead of relying on screen-size-changed being
emitted on startup, explicitly invoke _relayout().

(updated for all comments)

Review of attachment 161231 [details] [review]:

One problem, OK to commit with that fixed.

::: src/shell-global.c
@@ +461,3 @@
+
+  update_screen_size (global);
+  g_signal_emit (G_OBJECT (global), shell_global_signals[SCREEN_SIZE_CHANGED], 0);

Signal shouldn't be emitted unconditionally - this will cause _relayout() to happen *twice* on startup. make update_screen_size() return a 'changed' boolean.

Attachment 161231 [details] pushed as 84716bc - Create ShellGlobal later to avoid connecting to X during build