Created attachment 266180 [details] [review]
appDisplay: Move from instanceof-testing to polymorphic duck typing

Instead of having _compareItems, _getItemId, etc. on the view to
pull out info about items, use the AppIcon / FolderIcon items we
create as a place to track this additional info. We now require
that these items have a '.id' property for deduplication, and a
'.name' property to sort by.

Created attachment 266181 [details] [review]
appDisplay: Use the new org.gnome.desktop.app-folders schema for grouping

Rather than GMenu / app-folder-categories. This removes our last use of
gnome-menus in the stock gnome-shell, which is exciting, but also means
that app folders in Software start working.

Ideally, we'd have a button to launch our Software app as well from the
overview.

Review of attachment 266180 [details] [review]:

Looks good to me.

Review of attachment 266181 [details] [review]:

You're never filtering apps when they get moved in/out from the folders.

::: js/ui/appDisplay.js
@@ +56,3 @@
+    if (folder.get_boolean('translate')) {
+        let keyfile = new GLib.KeyFile();
+        let path = 'desktop-directories/' + name;

Maybe name + '.directory'?

@@ +64,3 @@
         }
+
+        name = keyfile.get_locale_string("Desktop Entry", "Name");

Single quotes (or use the constants)

Also, I'd put this statement inside the try, so we don't crash on malformed .directory files.

@@ +648,3 @@
+        this._folderSettings = new Gio.Settings({ schema: 'org.gnome.desktop.app-folders' });
+        this._folderSettings.connect('changed::folder-children', Lang.bind(this, function() {
+            Main.queueDeferredWork(this._frequentAppsWorkId);

allAppsWorkId

@@ +766,3 @@
+        folders.forEach(Lang.bind(this, function(id) {
+            let folder = new Gio.Settings({ schema_id: 'org.gnome.desktop.app-folders.folder',
+                                            path: this._folderSettings.path + 'folders/' + id + '/' });

1) A bad path aborts the shell. Should we protect us somehow?

@@ +994,3 @@
+        this._folder = folder;
+        this._folder.connect('changed::translated', Lang.bind(this, this._sync));
+        this._folder.connect('changed::apps', Lang.bind(this, this._sync));

changed::name too. Or just changed without detail.

@@ +1024,3 @@
+
+        this.view = new FolderView();
+        this._sync();

Should we use a deferred work to avoid wasting time when the overview is closed?

@@ +1042,3 @@
+            this.view.addItem(icon);
+        }));
+        this.view.loadGrid();

You must refresh the icon when the contents change.

You must also signal the view that the order may have changed.

Review of attachment 266181 [details] [review]:

::: js/ui/appDisplay.js
@@ +59,3 @@
+
+        try {
+            keyfile.load_from_data_dirs(path, null, GLib.KeyFileFlags.NONE);

Wrong invocation, should be
keyfile.load_from_data_dirs(path, GLib.KeyFileFlags.NONE)

@@ +64,3 @@
         }
+
+        name = keyfile.get_locale_string("Desktop Entry", "Name");

Also, wrong invocation, should be
keyfile.get_locale_string('Desktop Entry', 'Name', null)

Finally, how will we represent the default folders (Utilities and Sundry)?
Are we getting rid of them, defaulting to a flat view for core shell and traditional categories for classic?
Can we add a "category" setting to a folder (that would map to all apps with that category), for easier upgrading from 3.8/3.10 to 3.12?
In my case, switching from 3.10 folders to this bumped me from 4 to 8 pages of apps, so I'd like to get Games, Utilities, Sundry and Wine back into a folder without marking each app in Software.

Comment on attachment 266180 [details] [review]
appDisplay: Move from instanceof-testing to polymorphic duck typing

Attachment 266180 [details] pushed as 8fe7f92 - appDisplay: Move from instanceof-testing to polymorphic duck typing

Created attachment 266211 [details] [review]
appDisplay: Use the new org.gnome.desktop.app-folders schema for grouping

Rather than GMenu / app-folder-categories. This removes our last use of
gnome-menus in the stock gnome-shell, which is exciting, but also means
that app folders in Software start working.

Ideally, we'd have a button to launch our Software app as well from the
overview.

(In reply to comment #6)
> Finally, how will we represent the default folders (Utilities and Sundry)?
> Are we getting rid of them, defaulting to a flat view for core shell and
> traditional categories for classic?
> Can we add a "category" setting to a folder (that would map to all apps with
> that category), for easier upgrading from 3.8/3.10 to 3.12?
> In my case, switching from 3.10 folders to this bumped me from 4 to 8 pages of
> apps, so I'd like to get Games, Utilities, Sundry and Wine back into a folder
> without marking each app in Software.

Quick idea: do a one-time conversion of the old setting to the new schema. That gives us both the sundry + utility stuff, as well as any user configuration. That one-time conversion could live in gnome-software

Review of attachment 266211 [details] [review]:

Works fine, but there is space for some performance improvements.

::: js/ui/appDisplay.js
@@ +774,3 @@
+                                            path: this._folderSettings.path + 'folders/' + id + '/' });
+            folder.connect('changed', Lang.bind(this, function() {
+                Main.queueDeferredWork(this._allAppsWorkId);

It's sad to rebuild the entire view when the folder changes (especially because the view is quite heavy in resources).
Can we handle changed name and changed contents separately and
1) resort when the name changes
2) refilter when the content changes
Refilter should be easy if we keep all apps in the view and show/hide them as appropriate. Resorting should be possible if we remove all actors and readd them in the proper order.
We can also special case when the name changes but the relative order doesn't.

@@ +782,3 @@
+                let idx = apps.indexOf(appId);
+                if (idx >= 0)
+                    apps.splice(idx, 1);

Ugh, this is quadratic (and the number is high enough that it matters).
We need a better data structure, or just a tag on the ShellApp object marking it "foldered".

@@ +790,3 @@
+
+        apps.forEach(Lang.bind(this, function(appId) {
+            let app = appSys.lookup_app(appId);

I just noticed, we're building a GAppInfo for get_all(), and then throwing it away after the lookup_app().
In the indexed case, this probably doesn't matter, but for local apps or customized desktop files this means loading and parsing it twice.

I'd rather have a shell_app_system_get_all() method that calls g_app_info_get_all() internally but keeps the obtained GAppInfo in the ShellApp.

Attachment 266211 [details] pushed as 9df09db - appDisplay: Use the new org.gnome.desktop.app-folders schema for grouping


--

We agreed on IRC to do the performance improvements after landing.

*** Bug 719320 has been marked as a duplicate of this bug. ***