Created attachment 157831 [details] [review]
Major ShellApp API cleanup, startup notification, window focus handling

Review of attachment 157831 [details] [review]:

::: js/ui/appDisplay.js
@@ +440,1 @@
             style += " running";

could fix this to use st_widget_add/remove_style_class_name() while you're here

@@ +559,1 @@
             Main.overview.hide();

the hide is unnecessary now; you do it below

@@ +565,3 @@
+            else
+              this.app.activate();
+          }

might be simpler to redo this as "if (CONTROL_MASK and RUNNING) then open_new_window() else activate()" rather than having two activate()s

@@ -572,3 @@
-                Main.overview.hide();
-            } else {
-                this.activateMostRecentWindow();

i believe activateMostRecentWindow is unused now and can be removed

::: js/ui/panel.js
@@ +247,2 @@
         // If the currently focused app hasn't changed and the current
         // startup sequence hasn't changed, we have nothing to do

the code doesn't do anything with startup sequences any more. you can probably just remove the comment

::: src/shell-app.c
@@ +261,3 @@
+    {
+      case SHELL_APP_STATE_STOPPED:
+        /* TODO sensibly handle this error */

should shell_app_activate() return an error?

@@ +494,2 @@
 static void
+shell_app_state_transition (ShellApp      *app,

this could just be called shell_app_set_state(). (it has exactly the form of a typical foo_set_bar() method.)

@@ +604,3 @@
+    shell_app_state_transition (app, SHELL_APP_STATE_STARTING);
+  else if (!starting && app->state == SHELL_APP_STATE_STARTING)
+    shell_app_state_transition (app, SHELL_APP_STATE_STOPPED);

you never call this with starting=FALSE, so it's hard to say for sure, but wouldn't it make more sense to set state to RUNNING in the second case?

::: src/shell-window-tracker.c
@@ +648,3 @@
+       * if it's currently stopped, set it as our application focus,
+       * but focus the no_focus window.
+       */

This will happen regardless of how the app is launched (as long as it's launched with startup notification). Shouldn't it only apply to things explicitly launched by the user from the shell?

(In reply to comment #2)

[ snip lots of good comments which i've fixed ]
 
> ::: src/shell-app.c
> @@ +261,3 @@
> +    {
> +      case SHELL_APP_STATE_STOPPED:
> +        /* TODO sensibly handle this error */
> 
> should shell_app_activate() return an error?

I debated this a bit but decided that most of the interesting errors are asynchronous such as a crash during start, not a simple failure to exec().  So we need to handle these things async anyways.

Now we don't track app process state since the app could have been started by any random process, but eventually we should get everything sending a message to us to start an app.  One thought I had on this was to create a shadow .desktop file tree which changes the Exec= line to gnome-shell --launch=foo.desktop %args

> @@ +494,2 @@
>  static void
> +shell_app_state_transition (ShellApp      *app,
> 
> this could just be called shell_app_set_state(). (it has exactly the form of a
> typical foo_set_bar() method.)

I just added an assertion for the only invalid transition (running->starting).

> @@ +604,3 @@
> +    shell_app_state_transition (app, SHELL_APP_STATE_STARTING);
> +  else if (!starting && app->state == SHELL_APP_STATE_STARTING)
> +    shell_app_state_transition (app, SHELL_APP_STATE_STOPPED);
> 
> you never call this with starting=FALSE, so it's hard to say for sure, but
> wouldn't it make more sense to set state to RUNNING in the second case?

Oops yes.

> ::: src/shell-window-tracker.c
> @@ +648,3 @@
> +       * if it's currently stopped, set it as our application focus,
> +       * but focus the no_focus window.
> +       */
> 
> This will happen regardless of how the app is launched (as long as it's
> launched with startup notification). Shouldn't it only apply to things
> explicitly launched by the user from the shell?

Not sure.  Needs designer input.  If I choose an app from a file in Nautilus, what behavior do we want?

Created attachment 158330 [details] [review]
Major ShellApp API cleanup, startup notification, window focus handling

This patch combines several high level changes which are conceptually
independent but in practice rather intertwined.

* Add a "state" property to ShellApp which reflects whether it's
  stopped, starting, or started.  This will allow us to later clean
  up all the callers that are using ".get_windows().length > 0" as
  a proxy for this property
* Replace shell_app_launch with shell_app_activate and shell_app_open_new_window
  A lot of code was calling .launch, but it's signficantly clearer
  if we call this ".open_new_window()", and later if we gain the ability
  to call into an application's menu, we can implement this correctly rather
  than trying to update all .launch callers.
* Because ShellApp now has a "starting" state, rebase panel.js on top of
  this so that when we get a startup-notification sequence for an app
  and transition it to starting, it becomes the focus app, and panel.js
  cleanly just tracks the focus app, rather than bouncing between SN
  sequences.  This removes display of non-app startup sequences, which
  I consider an acceptable action in light of the committed changes
  to startup-notification and GTK+.

Note the patch is incomplete - I forgot to add some automake bits to it for the enums generation.  Will attach separate patch.

Created attachment 158366 [details] [review]
[build] Create GTypes for Shell namespace

Created attachment 158367 [details] [review]
Major ShellApp API cleanup, startup notification, window focus handling

This patch combines several high level changes which are conceptually
independent but in practice rather intertwined.

* Add a "state" property to ShellApp which reflects whether it's
  stopped, starting, or started.  This will allow us to later clean
  up all the callers that are using ".get_windows().length > 0" as
  a proxy for this property
* Replace shell_app_launch with shell_app_activate and shell_app_open_new_window
  A lot of code was calling .launch, but it's signficantly clearer
  if we call this ".open_new_window()", and later if we gain the ability
  to call into an application's menu, we can implement this correctly rather
  than trying to update all .launch callers.
* Because ShellApp now has a "starting" state, rebase panel.js on top of
  this so that when we get a startup-notification sequence for an app
  and transition it to starting, it becomes the focus app, and panel.js
  cleanly just tracks the focus app, rather than bouncing between SN
  sequences.  This removes display of non-app startup sequences, which
  I consider an acceptable action in light of the committed changes
  to startup-notification and GTK+.

Comment on attachment 158366 [details] [review]
[build] Create GTypes for Shell namespace

while I don't personally care, the switch to unaligned "\"s is inconsistent with all of the rest of the Makefiles.

also, "shell_source_h" doesn't seem like the best name since there are .h files that aren't in it... shell_public_headers maybe?

Comment on attachment 158367 [details] [review]
Major ShellApp API cleanup, startup notification, window focus handling

>     _updateStyleClass: function() {
>-        let windows = this.app.get_windows();
>-        let running = windows.length > 0;
>-        this._running = running;
>-        let style = "app-well-app";
>-        if (this._running)
>-            style += " running";
>+        this.actor.set_style_pseudo_class('app-well-app');
>+        if (this.app.state != Shell.AppState.STOPPED)
>+            this.actor.add_style_pseudo_class('running');
>         if (this._selected)
>-            style += " selected";
>-        this.actor.style_class = style;
>+            this.actor.add_style_pseudo_class('selected');
>     },

Sorry, I meant more like making it so that each time something changes, we only change the part of the style class that actually changed. So setSelected() would do:

    setSelected: function (isSelected) {
        this._selected = isSelected;
        if (this._selected)
            this.actor.add_style_pseudo_class('selected');
        else
            this.actor.remove_style_pseudo_class('selected');
    },

and "_updateStyleClass" would become "_updateRunning" or something and would just do:

    _updateRunning: function() {
        if (this.app.state == Shell.AppState.STOPPED)
            this.actor.remove_style_pseudo_class('running');
        else
            this.actor.add_style_pseudo_class('running');
    },

and then if we add, like a "favorite" style class later, we wouldn't have to change any of the selected/running code.

>+  g_assert (!(app->state == SHELL_APP_STATE_RUNNING &&
>+              state == SHELL_APP_STATE_STARTING));

g_return_if_fail, please. not worth crashing over.


good to commit with those changes

Attachment 158367 [details] pushed as 6aaf4b8 - Major ShellApp API cleanup, startup notification, window focus handling