Created attachment 360063 [details] [review]
shell-app: Don't sync state for not running apps

Patch attached

Review of attachment 360063 [details] [review]:

It is unclear what this patch is supposed to fix, neither from the code nor the commit message.

(In reply to Florian Müllner from comment #2)
> Review of attachment 360063 [details] [review] [review]:
> 
> It is unclear what this patch is supposed to fix, neither from the code nor
> the commit message.

It's a bit confusing, sorry about that. The present patch is a "merger" between changes originally done by Jasper a while ago [1] and other changes done on top by Cosimo [2], after having rebased our shell on top of 3.22.

I tried to explain the purpose of the patch, as I understand it now, in the commit message but it's true it's not great yet as it does not explain what's the problem it's supposed to fix, but that's hard for me since it seems from [1] and [2] that the situation has changed over time.

Now looking deeper into the internally reported bugs, it looks like the original intention from Jasper was to fix a race condition that could happen when you closed the splash screen that we show immediately to the user when launching an app (We call that "Speedwagon"), before the actual app started yet: in that case, without Jasper's patch, the app would still launch afterwards even if the user cancelled the splash screen. 

Unfortunately, that caused other problems related to the same "splash screen" that Cosimo fixed with the second patch, which is what we have applied now.

So, long way of saying that I'm not sure how to describe this better... maybe I can add a note on the endless-specific issues observed? Not very helpful from an upstream perspective, I know :(

[1] https://github.com/endlessm/eos-desktop/pull/67/commits/0a0ec41b1d0f901d73b3e8658b81bad8f0ea5280
[2] https://github.com/endlessm/eos-desktop/pull/123/commits/542aa041e0315dba22417bd4fac6a13a1e5fc59c

OK, that downstream code explains where this is coming from. It's just that when you combine the two commits, you end up without any actual change besides rearranging the code a bit. I'd say you can either drop the patch downstream, or provide a proper non-confusing commit message :-)

Interestingly enough I think my proposal is wrong right from the start :-(...

The original 2 patch sets where this "result patch" comes from attempted to fix different issues that are not impacted by having this patch applied or not. 

In more detail:
  * The first issue (closing the speedwagon window before the app launches) is still reproducible in Endless, regardless of this patch, suggesting that I missed something while doing the rebase that I need to investigate.

  * The second issue (a set of race conditions when opening http(s) URIs while the browser was already open) seems to be fixed downstream, regardless of this patch being applied or not.

So, long way of saying that this is not quite right yet and I need to investigate this further downstream before being able to provide a reasonable patch that is helpful upstream too in any way.

(In reply to Mario Sánchez Prada from comment #5)
> Interestingly enough I think my proposal is wrong right from the start :-(...

Not wrong, as it doesn't actually change anything. Of course that means it doesn't fix anything either :-)

In the end, it turns out we needed to bring back Jasper's original patch to fix the first issue I mentioned in my previous comment, without regressing (apparently, at least) other use cases: https://github.com/endlessm/gnome-shell/commit/3e28901bf293924e445b4ab06c0c19a45c5bd30c

After that, I've been looking a bit further into it in case there were some bits that could be upstreamed and this sentence from Jasper's commit message got my attention:

"If an app has an active startup-notification sequence, we shouldn't transition to STOPPED. **This matches what sync_running_state also does** ..."

Actually, he's right: how things stand right now the code in _add_window() behaves slightly different to _remove_window() in that the app->state is not being considered when transitioning the state, as there's some ad-hoc code in there to handle the transition.

Now, if I were going to attempt to upstream that change I could perhaps simply propose the same change that we now have in https://github.com/endlessm/gnome-shell/commit/3e28901bf293924e445b4ab06c0c19a45c5bd30c, but I wonder if something simpler might be accepted (which I checked and also fix our problem downstream.

What about something like this?

--- a/src/shell-app.c
+++ b/src/shell-app.c
@@ -894,7 +894,8 @@ shell_app_sync_running_state (ShellApp *app)
 
   if (app->state != SHELL_APP_STATE_STARTING)
     {
-      if (app->running_state->interesting_windows == 0)
+      if (app->running_state->windows == NULL ||
+          app->running_state->interesting_windows == 0)
         shell_app_state_transition (app, SHELL_APP_STATE_STOPPED);
       else
         shell_app_state_transition (app, SHELL_APP_STATE_RUNNING);
@@ -1068,15 +1069,10 @@ _shell_app_remove_window (ShellApp   *app,
   if (!meta_window_is_skip_taskbar (window))
     app->running_state->interesting_windows--;
 
+  shell_app_sync_running_state (app);
+
   if (app->running_state->windows == NULL)
-    {
       g_clear_pointer (&app->running_state, unref_running_state);
-      shell_app_state_transition (app, SHELL_APP_STATE_STOPPED);
-    }
-  else
-    {
-      shell_app_sync_running_state (app);
-    }
 
   g_signal_emit (app, shell_app_signals[WINDOWS_CHANGED], 0);
 }


Of course I'd need to rename this bug, but before doing so I'd be interested to know if whether a change might be correct and welcomed upstream.

(In reply to Mario Sánchez Prada from comment #7)
> 
> What about something like this?
> 
> --- a/src/shell-app.c
> +++ b/src/shell-app.c
> @@ -894,7 +894,8 @@ shell_app_sync_running_state (ShellApp *app)
>  
>    if (app->state != SHELL_APP_STATE_STARTING)
>      {
> -      if (app->running_state->interesting_windows == 0)
> +      if (app->running_state->windows == NULL ||
> +          app->running_state->interesting_windows == 0)

This doesn't make sense - if an app has no open windows at all, but we somehow think there are more than 0 interesting windows, then we have a bug in our window tracking. If there is such a bug, it should be fixed instead of papered over.


> @@ -1068,15 +1069,10 @@ _shell_app_remove_window (ShellApp   *app,
>    if (!meta_window_is_skip_taskbar (window))
>      app->running_state->interesting_windows--;
>  
> +  shell_app_sync_running_state (app);

Those changes on the other hand make sense to me. Splash screens have grown a bit out of fashion, but that's not a reason for not handling the case correctly.

(In reply to Florian Müllner from comment #8)
> (In reply to Mario Sánchez Prada from comment #7)
> > 
> > What about something like this?
> > 
> > --- a/src/shell-app.c
> > +++ b/src/shell-app.c
> > @@ -894,7 +894,8 @@ shell_app_sync_running_state (ShellApp *app)
> >  
> >    if (app->state != SHELL_APP_STATE_STARTING)
> >      {
> > -      if (app->running_state->interesting_windows == 0)
> > +      if (app->running_state->windows == NULL ||
> > +          app->running_state->interesting_windows == 0)
> 
> This doesn't make sense - if an app has no open windows at all, but we
> somehow think there are more than 0 interesting windows, then we have a bug
> in our window tracking. If there is such a bug, it should be fixed instead
> of papered over.

Tbh, I was unsure about this bit. The only reason I added it is because, with the change in the other side, I understand it could be possible to now call shell_app_sync_running_state() with app->running_state->windows == NULL.

But perhaps that shouldn't be an issue since, as you said, the check for interesting_windows would be enough (and otherwise it would be a bug).
 
> > @@ -1068,15 +1069,10 @@ _shell_app_remove_window (ShellApp   *app,
> >    if (!meta_window_is_skip_taskbar (window))
> >      app->running_state->interesting_windows--;
> >  
> > +  shell_app_sync_running_state (app);
> 
> Those changes on the other hand make sense to me. Splash screens have grown
> a bit out of fashion, but that's not a reason for not handling the case
> correctly.

Cool, I'll remove the other bit and rename this bug, then propose a new patch.

Thanks

Created attachment 360663 [details] [review]
shell-app: Rely on shell_app_sync_running_state() when removing windows

Here's the new patch

Review of attachment 360663 [details] [review]:

Code looks fine, the commit message could use some work (apart from "also that the state is now also") - the main point of the patch is not to make some function more consistent with some other function, it's to not transition an app to the STOPPED state while is is still STARTING according to its startup-notification. Or in less technical terms: To not stop an application that closes its spash screen before opening the main window :-)

Created attachment 360678 [details] [review]
shell-app: Don't transition to STOPPED while still in the STARTING state

Sorry about the commit message, what about this one?

Review of attachment 360678 [details] [review]:

That works better for me, thanks

Thanks. Pushed: https://git.gnome.org/browse/gnome-shell/commit/?id=b5f5a594ba7ad96bf9d9a2bbda214dd7e33f7d47