Created attachment 166021 [details] [review]
correct handle state for applications with several .destop files

Created attachment 166022 [details] [review]
show startup notification only for current workspace

I don't really understand these patches at all honestly - shouldn't we just special case Nautilus as "running but hidden"?

(In reply to comment #3)
> I don't really understand these patches at all honestly - shouldn't we just
> special case Nautilus as "running but hidden"?

1. we start nautilus-folder-handler.desktop (and got this desktop file path  from startup-notification).
2. When nautilus window is open, we got nautilus.desktop from WMCLASS. 
So nautilus-folder-handler.desktop will always has STARTING state.

Same will happen for all applications with 2 (or more) desktop files.


Second patch is about disrespect workspace property from startup notification (We show notification on all workspaces).

(In reply to comment #4)
> (In reply to comment #3)
> > I don't really understand these patches at all honestly - shouldn't we just
> > special case Nautilus as "running but hidden"?
> 
> 1. we start nautilus-folder-handler.desktop (and got this desktop file path 
> from startup-notification).

Can we just ignore the start for nautilus-folder-handler.desktop because it's NoDisplay=true?  (Check shell_app_info_get_is_nodisplay)

This would mean we wouldn't show startup notification for Evince, but I think we just need to revert that Evince issue.

(In reply to comment #5)
> Can we just ignore the start for nautilus-folder-handler.desktop because it's
> NoDisplay=true?  (Check shell_app_info_get_is_nodisplay)
> 
> This would mean we wouldn't show startup notification for Evince, but I think
> we just need to revert that Evince issue.

We can't.
If we do that 
1. startup notification will not work for some applications. ( I think more then 1)
2. In my opinion, Not all application (with 2 or more .desktop files) has only 1 .desktop file _without_ NoDisplay=true.
(For example: I create .desktop file with custom command line. And I want it and original .desktop file in menu)

(In reply to comment #6)
> (In reply to comment #5)
> > Can we just ignore the start for nautilus-folder-handler.desktop because it's
> > NoDisplay=true?  (Check shell_app_info_get_is_nodisplay)
> > 
> > This would mean we wouldn't show startup notification for Evince, but I think
> > we just need to revert that Evince issue.
> 
> We can't.
> If we do that 
> 1. startup notification will not work for some applications. ( I think more
> then 1)

What other ones?  The other fixes are:

* Don't have the shell launch nautilus through the .desktop file for these
* Remove StartupNotify=true from nautilus-folder-handler.desktop
* Fix nautilus/gtk+ to correctly complete startup notification when showing a window from nautilus-folder-handler.desktop

> 2. In my opinion, Not all application (with 2 or more .desktop files) has only
> 1 .desktop file _without_ NoDisplay=true.
> (For example: I create .desktop file with custom command line. And I want it
> and original .desktop file in menu)

If you have a new .desktop file you should have a new app, basically.  Is this use case something like a custom .desktop file that launches gnome-terminal --blah ?  We could have a special .desktop key Shortcut=true or something for that I guess.

(In reply to comment #7)
> What other ones?  The other fixes are:
> * Don't have the shell launch nautilus through the .desktop file for these
Will not work.
Any application that use g_app_info_launch_default_for_uri ('/a/b/') will bring same problem.

> * Remove StartupNotify=true from nautilus-folder-handler.desktop
> * Fix nautilus/gtk+ to correctly complete startup notification when showing a
> window from nautilus-folder-handler.desktop

Is it normal, that "broken" application can bring such problem?

> If you have a new .desktop file you should have a new app, basically.  Is this
> use case something like a custom .desktop file that launches gnome-terminal
> --blah ?
yes

What about second patch?

Created attachment 171277 [details] [review]
[ShellApp] refactor handling startup sequence 

1. move logic to shell-app.c
2. change state to RUNNING only after startup sequence complete
3. correct handle state for applications with several .destop files

Created attachment 171278 [details] [review]
show startup notification only for current workspace

merge with HEAD

Created attachment 171335 [details] [review]
[ShellApp] refactor handling startup sequence

1. move logic to shell-app.c
2. change state to RUNNING only after startup sequence complete
3. correct handle state for applications with several .destop files

(In reply to comment #0)
> 1. Start shell
> 2. Launch "Desktop" place via dash
> 3. Return to overview and create a new workspace (defaults to grid mode!?)
> 4. On new workspace "Open Folder" is the app and it is starting

What's the desired result actually?  Should the "File Manager" app be starting?  (I assume so)

If the file manager loads after you switch workspaces, should it appear on the workspace you launched it from (I assume so)?

The expected result is that there is no app running (or starting) on the new workspace.

Hmm...so taking out the workspace switching part:

Should we be displaying any notification at all of File Manager starting, ever?

Right now it looks like nautilus blocks display of the whole window until it's mostly loaded the contents.  We could change it to try to get at least an empty window up faster?

(In reply to comment #14)
> Should we be displaying any notification at all of File Manager starting, ever?

yes. But on right workspace.

Review of attachment 171278 [details] [review]:

::: js/ui/panel.js
@@ +191,1 @@
+        this._targedApp = null;

Looks like a typo, should be this._targetApp or this._targetedApp ?

@@ +448,1 @@
+        if (!focusedApp && 0) {

Why is this here?

Created attachment 171851 [details] [review]
show startup notification only for current workspace

> Looks like a typo, should be this._targetApp or this._targetedApp ?
this._targetApp

> Why is this here?
Sorry. I forget remove this after testing.

Review of attachment 171851 [details] [review]:

OK.  For commit message, I suggest:

panel: Only show starting applications for current workspace

Add the workspace we started on to ShellApp.  Use it inside panel.js
to filter the list.

Review of attachment 171335 [details] [review]:

Looks good to me.  (One typo in your commit message, s/destop/desktop).