Created attachment 186231 [details] [review]
Initial port to gnome-menus 4

Created attachment 186397 [details] [review]
WIP: Port to gnome-menus 4

Since gnome-menus is now introspectable, eventually we can drop
ShellAppSystem entirely.  For now though, just do the basic port.

Created attachment 186463 [details] [review]
Kill off ShellAppInfo, move into ShellApp

This dramatically thins down and sanitizes the application code.

The ShellAppSystem changes in a number of ways:
* Preferences are moved up into JavaScript; they are a weird
  special case (they aren't apps, they're shortcuts for an app),
  and we don't have many of them, so don't need the optimizations
  in ShellAppSystem for searching.
* get_app() changes to lookup_app() and returns null if an app isn't
  found.  The semantics where it tried to find the .desktop file
  if we didn't know about it were just broken; I am pretty sure no
  caller needs this, and if they do we'll fix them.
* ShellAppSystem maintains two indexes on apps (by desktop file id
  and by GMenuTreeEntry), but is no longer in the business of
  dealing with GMenuTree as far as hierarchy and categories go.  That
  is moved up into js/ui/appDisplay.js.

Also, ShellWindowTracker is now the sole reference-owner for
window-backed apps.  We still do the weird "window:0x1234beef" id
for these apps, but a reference is not stored in ShellAppSystem.

The js/ui/appDisplay.js code is rewritten, and sucks a lot less.
Variable names are clearer:

_apps -> _appIcons
_filterApp -> _visibleApps
_filters -> _categoryBox

Similarly for function names.  We no longer call (for every app) a
recursive lookup in GMenuTree to see if it's in a particular section
on every category switch; it's all cached.

Created attachment 192633 [details] [review]
WIP: Port to new gnome-menus API

Rebase first patch against branch of gnome-menus that will get merged, as soon as we get approval for this patch.

Created attachment 193099 [details] [review]
Kill off ShellAppInfo, move into ShellApp

This dramatically thins down and sanitizes the application code.

The ShellAppSystem changes in a number of ways:
* Preferences are moved up into JavaScript; they are a weird
  special case (they aren't apps, they're shortcuts for an app),
  and we don't have many of them, so don't need the optimizations
  in ShellAppSystem for searching.
* get_app() changes to lookup_app() and returns null if an app isn't
  found.  The semantics where it tried to find the .desktop file
  if we didn't know about it were just broken; I am pretty sure no
  caller needs this, and if they do we'll fix them.
* ShellAppSystem maintains two indexes on apps (by desktop file id
  and by GMenuTreeEntry), but is no longer in the business of
  dealing with GMenuTree as far as hierarchy and categories go.  That
  is moved up into js/ui/appDisplay.js.

Also, ShellWindowTracker is now the sole reference-owner for
window-backed apps.  We still do the weird "window:0x1234beef" id
for these apps, but a reference is not stored in ShellAppSystem.

The js/ui/appDisplay.js code is rewritten, and sucks a lot less.
Variable names are clearer:

_apps -> _appIcons
_filterApp -> _visibleApps
_filters -> _categoryBox

Similarly for function names.  We no longer call (for every app) a
recursive lookup in GMenuTree to see if it's in a particular section
on every category switch; it's all cached.

NOTE - this intentionally reverts the incremental loading code from
commit 7813c5b93f6bcde8c4beae286e82bfc472b2b656.  It's fast enough
here without that.

Created attachment 193116 [details] [review]
Kill off ShellAppInfo, move into ShellApp

Various refcounting and other bug fixes

Created attachment 193124 [details] [review]
Kill off ShellAppInfo, move into ShellApp

Make preferences work again - push parts of it back down into
ShellAppSystem.

Push indexing down into gnome-menus (bug 655868).

Review of attachment 193124 [details] [review]:

be sure to check that the launching-one-control-panel-and-then-another-and-get-the-wrong-icon bug doesn't come back

::: js/ui/appDisplay.js
@@ +38,3 @@
         this._pendingAppLaterId = 0;
+        this._appIcons = {}; // desktop file id
+        this._visibleApps = null;

this._visibleApps is set in three places, but nothing ever reads it

@@ +78,2 @@
         if (this._filterApp && !this._filterApp(appInfo))
             appIcon.actor.hide();

cruft, _filterApp is gone

@@ +253,3 @@
+        var allApps = Shell.AppSystem.get_default().get_all();
+        allApps.sort(function(a, b) {
+            return a.get_name().localeCompare(b.get_name());

return a.compare_by_name(b) ?

@@ +338,3 @@
+        var apps = this._appSys.initial_search(terms);
+        for (var i = 0; i < apps.length; i++)
+            apps[i] = apps[i].get_id();

return apps.map(function (app) { return app.get_id; });
?

@@ +432,3 @@
+            for (var i = 0; i < terms.length; i++) {
+                var term = terms[i];
+                var idx = name.indexOf(term);

if we need to normalize and casefold for regular apps, we need to do it for settings too.

@@ +449,2 @@
+        var pref = this._appSys.lookup_setting(id);
+        pref.activate(params.workspace);

, params.timestamp

@@ +451,3 @@
     },
 
+    dragActivateResult: function(id, params) {

should just call this.activateResult(id, params);

@@ +461,3 @@
+    createResultActor: function (resultMeta, terms) {
+        // Override the "activate" so we spawn the right Exec= line
+        let icon = new AppWellIcon(this._gnomecc, Lang.bind(this, function () { this.activateResult(resultMeta['id']); }));

needs a {} or null or something as the second argument. onActivateOverride is third.

also, won't this mean that all settings search results will use the main control center icon, instead of their specific icons?

::: js/ui/dateMenu.js
@@ +200,3 @@
         this.menu.close();
         Main.overview.hide();
+        let app = Shell.AppSystem.get_default().lookup_app('gnome-datetime-panel.desktop');

should be lookup_setting

::: src/shell-app-private.h
@@ +31,3 @@
+ShellAppSearchMatch _shell_app_match_search_terms (ShellApp *app, GSList *terms);
+
+const char * _shell_app_get_name_collation_key (ShellApp *app);

does not exist

::: src/shell-app-system.c
@@ +47,2 @@
   gboolean loaded;
   gint app_monitor_id;

app_monitor_id is no longer used

@@ -220,2 @@
   g_object_unref (priv->apps_tree);
-  g_object_unref (priv->settings_tree);

this gets leaked now, along with priv->settings_entry_to_app

also, should g_source_remove(priv->app_change_timeout_id) if it's not 0.

@@ +598,3 @@
 
+static char *
+normalize_and_casefold (const char *str)

This is now defined in both shell-app-system.c and shell-app.c.

(Really we ought to have it in glib...)

@@ +632,2 @@
 static inline void
 shell_app_system_do_match (ShellAppSystem   *system,

this does not use @system at all, and if you made it into a ShellApp method then you wouldn't need to expose ShellAppSearchMatchType in shell-app-private.h

@@ +679,3 @@
+ */
+GSList *
+shell_app_system_get_all (ShellAppSystem  *self)

this is oddly placed, between shell_app_system_initial_search and its helper func.

@@ -904,2 +702,2 @@
  *
- * Returns: (transfer container) (element-type utf8): List of application identifiers
+ * Returns: (transfer container) (element-type ShellApp): List of applications

it is wacky that shell_app_system_initial_search() is changed to return ShellApps, but shell_app_system_subsearch() still takes ids

::: src/shell-app-system.h
@@ +62,3 @@
+GMenuTree      *shell_app_system_get_settings_tree           (ShellAppSystem *system);
+
+ShellApp       *shell_app_system_lookup_setting             (ShellAppSystem *system,

these should be aligned

::: src/shell-app.c
@@ +50,3 @@
   ShellAppState state;
 
+  GMenuTreeEntry *entry; /* If NULL, this app is backend by exactly one MetaWindow */

If an unidentifiable app creates two windows in the same window_group, I think they will end up with the same entry-less ShellApp?

@@ +126,2 @@
 shell_app_create_icon_texture (ShellApp   *app,
                                float size)

@size is cast to an int everywhere it's used, so you should probably just change the prototype

@@ +137,3 @@
+      return st_texture_cache_bind_pixbuf_property (st_texture_cache_get_default (),
+                                                    G_OBJECT (window),
+                                                    "icon");

The docs claim that we "create a #ClutterTexture for it at the given size", but this codepath ignores @size. Should either set .width and .height of the returned actor, or else clarify in the docs that @size is merely a hint.

(Yes, I realize this is not a change from the old behavior.)

@@ +277,3 @@
    */
+  if (!app->entry)
+    return NULL;

panel.js does not deal with get_faded_icon() returning NULL

@@ +548,3 @@
                            int            workspace)
 {
+  g_return_if_fail (app->entry != NULL);

appDisplay.js needs to disable the "New Window" menu item and Ctrl+click for window-backed apps then.

@@ +751,3 @@
+   * There are various other alternatives, but the address is unique
+   * and unchanging, which is pretty much the best we can do.
+   */

It's unique during the lifetime of the window, but reasonably likely to be reused by another window after this one is destroyed. Using meta_window_get_stable_sequence() might be better.

(This is particularly an issue if it is possible for a window-backed ShellApp to include multiple windows in the same window_group [as mentioned above], since then you could eventually end up with two window-backed ShellApps referring to different apps, but with the same ID.)

@@ +1036,3 @@
+
+  if (timestamp == 0)
+    timestamp = clutter_get_current_event_time ();

this was already wrong before, but it should be shell_global_get_current_time()

@@ +1075,3 @@
+ * @app: a #ShellApp
+ *
+ * Returns: (transfer none): The #GDesktopAppInfo for this app, or %NULL if backed by a window

s/GDesktopAppInfo/GMenuTreeEntry/

::: src/shell-window-tracker.c
@@ +49,2 @@
   /* <MetaWindow * window, ShellApp *app> */
+  GHashTable *window_backed_apps;  /* Owns a reference */

This is not used

@@ +344,3 @@
  */
 static ShellApp *
 get_app_for_window (ShellWindowTracker    *monitor,

It would be nice if (as a separate patch) you renamed all the "monitor" variables in shell-window-tracker.c to "tracker".

(In reply to comment #8)
>
> @@ +432,3 @@
> +            for (var i = 0; i < terms.length; i++) {
> +                var term = terms[i];
> +                var idx = name.indexOf(term);
> 
> if we need to normalize and casefold for regular apps, we need to do it for
> settings too.

Ok, I'm pushing search for settings back down into ShellAppSystem.

> @@ +449,2 @@
> +        var pref = this._appSys.lookup_setting(id);
> +        pref.activate(params.workspace);
> 
> , params.timestamp

Not a regression - think I can do this in another patch.

> also, won't this mean that all settings search results will use the main
> control center icon, instead of their specific icons?

Will look at this.
 
> +static char *
> +normalize_and_casefold (const char *str)
> 
> This is now defined in both shell-app-system.c and shell-app.c.
> 
> (Really we ought to have it in glib...)

Add this one as a TODO.

> ::: src/shell-app.c
> @@ +50,3 @@
>    ShellAppState state;
> 
> +  GMenuTreeEntry *entry; /* If NULL, this app is backend by exactly one
> MetaWindow */
> 
> If an unidentifiable app creates two windows in the same window_group, I think
> they will end up with the same entry-less ShellApp?

Added this as TODO.

> 
> @@ +126,2 @@
>  shell_app_create_icon_texture (ShellApp   *app,
>                                 float size)
> 
> @size is cast to an int everywhere it's used, so you should probably just
> change the prototype

TODO.

> 
> @@ +137,3 @@
> +      return st_texture_cache_bind_pixbuf_property
> (st_texture_cache_get_default (),
> +                                                    G_OBJECT (window),
> +                                                    "icon");
> 
> The docs claim that we "create a #ClutterTexture for it at the given size", but
> this codepath ignores @size. Should either set .width and .height of the
> returned actor, or else clarify in the docs that @size is merely a hint.
> 
> (Yes, I realize this is not a change from the old behavior.)

TODO.

> panel.js does not deal with get_faded_icon() returning NULL

Not a regression I think - TODO.

> @@ +548,3 @@
>                             int            workspace)
>  {
> +  g_return_if_fail (app->entry != NULL);
> 
> appDisplay.js needs to disable the "New Window" menu item and Ctrl+click for
> window-backed apps then.

Also not a regression - TODO.

> 
> @@ +751,3 @@
> +   * There are various other alternatives, but the address is unique
> +   * and unchanging, which is pretty much the best we can do.
> +   */
> 
> It's unique during the lifetime of the window, but reasonably likely to be
> reused by another window after this one is destroyed. Using
> meta_window_get_stable_sequence() might be better.

Not a regression - TODO.

> +
> +  if (timestamp == 0)
> +    timestamp = clutter_get_current_event_time ();
> 
> this was already wrong before, but it should be shell_global_get_current_time()

Not a regression - TODO.

> 
> @@ +1075,3 @@
> + * @app: a #ShellApp
> + *
> + * Returns: (transfer none): The #GDesktopAppInfo for this app, or %NULL if
> backed by a window
> 
> s/GDesktopAppInfo/GMenuTreeEntry/

No, it really does return a GDesktopAppInfo.

> @@ +344,3 @@
>   */
>  static ShellApp *
>  get_app_for_window (ShellWindowTracker    *monitor,
> 
> It would be nice if (as a separate patch) you renamed all the "monitor"
> variables in shell-window-tracker.c to "tracker".

Agree.

(In reply to comment #9)
> > panel.js does not deal with get_faded_icon() returning NULL
> 
> Not a regression I think - TODO.

With your patch, window-backed apps will now have no app menu icon, rather than just using the unfaded window icon like they did before.

> > +  g_return_if_fail (app->entry != NULL);
> > 
> > appDisplay.js needs to disable the "New Window" menu item and Ctrl+click for
> > window-backed apps then.
> 
> Also not a regression - TODO.

It's not a regression in the sense that those features didn't work before either, but it feels wrong to me to add a g_return_if_fail() without fixing the cases where we already know that it will trigger.

> > + * Returns: (transfer none): The #GDesktopAppInfo for this app, or %NULL if
> > backed by a window
> > 
> > s/GDesktopAppInfo/GMenuTreeEntry/
> 
> No, it really does return a GDesktopAppInfo.

(grumble grumble splinter not showing enough context). You copied the doc from shell_app_get_app_info() to shell_app_get_tree_entry(), but forgot to change the return type.

Comment on attachment 193124 [details] [review]
Kill off ShellAppInfo, move into ShellApp

Attachment 193124 [details] pushed as 10dcc10 - Kill off ShellAppInfo, move into ShellApp

Created attachment 193620 [details] [review]
ShellApp: Change activation API

Since almost all of the callers of shell_app_activate were using the
default workspace (by passing -1), remove that parameter.

Add a new shell_app_activate_full() API which takes a workspace as
well as a timestamp; previously we might have been ignoring event
timestamps from elsewhere.

Created attachment 193621 [details] [review]
ShellApp: Use integer for size, not float

We were basically casting it everywhere except for ClutterActor -
let's be consistent with StTextureCache and use integers.

Created attachment 193622 [details] [review]
ShellApp: Use global time, not clutter time

This is correct in more circumstances.

Created attachment 193623 [details] [review]
ShellApp: Use stable sequence for id, not pointer address

As danw points out,

  "It's unique during the lifetime of the window, but reasonably likely to be
  reused by another window after this one is destroyed. Using
  meta_window_get_stable_sequence() might be better."

Created attachment 193624 [details] [review]
ShellApp: Ensure we set the size of returned texture for window backed apps

Unify the two code paths too.

Created attachment 193625 [details] [review]
appDisplay: Don't expose "Add as favorite" for window-backed apps

We don't know how to do it.

Created attachment 193626 [details] [review]
ShellApp: Fix comment about window-backed apps

Created attachment 193627 [details] [review]
ShellWindowTracker: Rename self variable for consistency

Historically it was monitor, now tracker.

(I want to move more things to self, but that's a different bug).

Comment on attachment 193625 [details] [review]
appDisplay: Don't expose "Add as favorite" for window-backed apps

ok, but maybe mention "New Window" in the commit message too

Attachment 193620 [details] pushed as 4886275 - ShellApp: Change activation API
Attachment 193621 [details] pushed as 11f30e2 - ShellApp: Use integer for size, not float
Attachment 193622 [details] pushed as ff840db - ShellApp: Use global time, not clutter time
Attachment 193623 [details] pushed as b0cc778 - ShellApp: Use stable sequence for id, not pointer address
Attachment 193624 [details] pushed as b9edb1d - ShellApp: Ensure we set the size of returned texture for window backed apps
Attachment 193625 [details] pushed as 7f1d282 - appDisplay: Don't expose "Add as favorite" for window-backed apps
Attachment 193626 [details] pushed as 2efcbaf - ShellApp: Fix comment about window-backed apps
Attachment 193627 [details] pushed as d256109 - ShellWindowTracker: Rename self variable for consistency