Created attachment 156086 [details] [review]
Move focus change handling out of a meta_later

Ok, there admittedly wasn't a strong rationale for having it in a
later, and by performing this immediately we reduce race conditions
for our focus_app versus startup_notification handling.

Created attachment 156087 [details] [review]
Consume startup-notification APPLICATION_ID

This patch ensures we're showing the correct data when doing
startup-notification.

Review of attachment 156087 [details] [review]:

Doing a quick self-review...

::: js/ui/panel.js
@@ +276,3 @@
+        if (focusedApp == null && lastSequence != null) {
+        log("current focus=" + focusedApp + " lastSequence=" + lastSequence);
+

Do we actually need this anymore?

::: src/shell-app-system.c
@@ +1194,1 @@
 

SHould break this out as a separate patch.

::: src/shell-window-tracker.c
@@ +636,3 @@
+  const char *application_id;
+  const char *application_path = sn_startup_sequence_get_application_id (sequence);
+  const char *id = sn_startup_sequence_get_id (sequence);

We need to remove this at some later point; I think it should be safe to remove when we get the REMOVE for the sequence, but I need to convince myself that's race-free.

Created attachment 157082 [details] [review]
Consume startup-notification APPLICATION_ID

* remove debug logging

Created attachment 157116 [details] [review]
[ShellAppSystem] Add shell_app_system_get_app_for_path

Allows looking up an app given an absolute path to its
desktop file; will be used for startup-notification.

Created attachment 157117 [details] [review]
Consume startup-notification APPLICATION_ID

* We don't need to hold a hash table of notifications since we
  should always get the SN before the window map event

Comment on attachment 156086 [details] [review]
Move focus change handling out of a meta_later

>Ok, there admittedly wasn't a strong rationale for having it in a
>later

or if there was, we'll find out soon :)

Comment on attachment 157116 [details] [review]
[ShellAppSystem] Add shell_app_system_get_app_for_path

>+ * @desktop_path: (utf8): UTF-8 encoded absolute file name

I'm not sure to what extent this is already a problem with the code, but filesystem paths are in the locale encoding, and so you need to use g_filename_{to,from}_utf8 if you're in a non-UTF-8 locale.

>+ * For files outside the dtaadirs,

typo and the rest of the sentence is missing

>+  return shell_app_system_get_app (system, basename);

Don't you need to check that the path matches here too?

It also seems like if I double-click ~/Desktop/firefox.desktop, and then try to look up /usr/share/applications/firefox.desktop, that this will fail (because I'll have a cached ShellApp with the wrong path), but if I do the lookup without having opened the Desktop .desktop file first, then it would work. Which is weird.

Comment on attachment 157117 [details] [review]
Consume startup-notification APPLICATION_ID

>+      if (meta_window_get_window_type (group_window) != META_WINDOW_NORMAL)
>+        continue;
>+
>+       result = g_hash_table_lookup (monitor->window_to_app, group_window);
>+       if (result)
>+         break;

the second bit is indented one space too far

>+  /* First, we check whether we already know about this window,
>+   * if so, just return that.  Next, try looking up the app
>+   * directly from this window.  We then later see if that
>+   * matches up with what we have using other heuristics.
>+   */

not true, there's no "see if that matches"

>   if (result != NULL)
>+    return result;
> 
>+  return direct_result;

leaks direct_result if both are non-NULL. (But then, currently there's no need for direct_result; you could just move the get_app_for_window() direct call to the end of the function.)

(In reply to comment #8)
> (From update of attachment 157116 [details] [review])
> >+ * @desktop_path: (utf8): UTF-8 encoded absolute file name
> 
> I'm not sure to what extent this is already a problem with the code, but
> filesystem paths are in the locale encoding, and so you need to use
> g_filename_{to,from}_utf8 if you're in a non-UTF-8 locale.

The data we get from startup notification is UTF-8.

> >+ * For files outside the dtaadirs,
> 
> typo and the rest of the sentence is missing

Fixed.
> 
> >+  return shell_app_system_get_app (system, basename);
> 
> Don't you need to check that the path matches here too?

I don't think so because we can't have a ShellApp which doesn't share the ShellAppInfo's id too, and we checked that.

> It also seems like if I double-click ~/Desktop/firefox.desktop, and then try to
> look up /usr/share/applications/firefox.desktop, that this will fail (because
> I'll have a cached ShellApp with the wrong path), but if I do the lookup
> without having opened the Desktop .desktop file first, then it would work.
> Which is weird.

~/Desktop isn't in XDG_DATA_DIRS, so there will be no ShellAppInfo which matches, so we'll always return %NULL.

(In reply to comment #10)
> > >+ * @desktop_path: (utf8): UTF-8 encoded absolute file name
> > 
> > I'm not sure to what extent this is already a problem with the code, but
> > filesystem paths are in the locale encoding, and so you need to use
> > g_filename_{to,from}_utf8 if you're in a non-UTF-8 locale.
> 
> The data we get from startup notification is UTF-8.

Right, but what about the data you got back from libgnome-menu? Hm... it looks like the answer is that libgnome-menu doesn't think about filename encoding at all, and the only reason it works is that the paths in question are always ASCII-only... OK, nothing to see here I guess. :-)

> > Don't you need to check that the path matches here too?
> 
> I don't think so because we can't have a ShellApp which doesn't share the
> ShellAppInfo's id too, and we checked that.

ah, yeah, misread the code

Created attachment 157203 [details] [review]
Add shell_app_system_get_app_for_path

Created attachment 157204 [details] [review]
Consume startup-notification APPLICATION_ID

the "direct result" that was in the previous patch.

Attachment 156086 [details] pushed as 1a25cd9 - Move focus change handling out of a meta_later
Attachment 157203 [details] pushed as fe52a9e - Add shell_app_system_get_app_for_path
Attachment 157204 [details] pushed as c92ce59 - Consume startup-notification APPLICATION_ID