Created attachment 157346 [details] [review]
sections in all apps view

Section is based on categories, but It is user reorderable.
Section can contain 0 or more categories.
Section with 0 categories contain apps, that doesn't included to other
sections.
Section can contain only apps, that move by user to it.

Seems very close to what I had in mind.  Good job.  Though I think maybe the following change makes for a better default:

-       <default>[,Office,Utility;Development,Game]</default>
+       <default>[,Game,Utility;Development;System]</default>

Created attachment 157501 [details]
separator-white.svg

On second thought that separator looks a little too chunky.  Let's try this one instead.  Note it only works on a dark background.

Created attachment 157568 [details] [review]
sections in all apps view

(In reply to comment #3)
> Created an attachment (id=157501) [details]
> separator-white.svg
> 
> On second thought that separator looks a little too chunky.  Let's try this one
> instead.  Note it only works on a dark background.

librsvg2 doesn't correct render image. I convert it to png.

Looks good.  Nice work.

I noticed something yesterday while using the last version of the patch.  At some point I started seeing duplicate icons for all my apps.  It might have been after I did a new app install or something.  I am not exactly sure.  And may not be related to this patch at all.

Created attachment 157767 [details] [review]
sections in all apps view

Still looks good to me.

Review of attachment 157767 [details] [review]:

In general, this looks and feels very good. I'm not totally sold that letting the user drag stuff around is a good idea:

 - It's easy to do by accident, and a user won't be expecting to have made a utility a game just by dragging it "Let's play gnome-terminal!"
 - It seems you'd want to be able to add sections in some way
 - Things will get weird, say, if the set of sections is changed after the user drags things around

But it's neat and let's see how it works out.

::: data/gnome-shell.schemas
@@ +39,3 @@
+	<default>[,Game,Utility;Development;System]</default>
+	<locale name="C">
+	  <short>Categories in AllAppView</short>

This needs a human readable short description ('Categories for each section in the application browser') and then a long description that fully describes the contents of the key.

@@ +51,3 @@
+	<default>[]</default>
+	<locale name="C">
+	  <short>Apps moved by user</short>

Same here - may for the short 'Applications moved between application browser sections'

::: data/theme/gnome-shell.css
@@ +459,3 @@
 }
 
+.all-app-divider-container {

This class should be something like .app-section-divider-container. It's not a divider in AllAppView, but a divider between sections.

[ AllAppView is a pretty confusing name now (since it doesn't hold all apps) and maybe we should rename it ]

::: js/ui/appDisplay.js
@@ +27,3 @@
 
+const ENABLED_CATEGORIES_KEY = 'enabled_categories';
+const CHAGES_IN_CATEGORIES_KEY = 'changes_in_categories';

Type - CHAGES instead of CHANGES

@@ +141,3 @@
+            }
+            this._categories[i] = { view: new AllAppView(),
+                                    categories: categories[i].split(';'),

After reading through this patch a bunch, I realized that you are actually using 'category' for two different things:

 - The sections of the applications view
 - The categories from the desktop file

This is why we have things like category.categories[]. I think the whole thing will read better if you use the word 'section' for the sections and reserve 'category' for the desktop file category keywords.

@@ +143,3 @@
+                                    categories: categories[i].split(';'),
+                                    apps: [],
+                                    moved_apps: []};

This is an array, but you use it like a "dictionary" - you assign:

 this._categories[t[1]].moved_apps[t[2]]

So you probably meant - moved_apps: {}

@@ +161,3 @@
+
+    _removeAll: function() {
+        this.actor.remove_all();

Generally we want to explicitly destroy actors instead of removing them and counting on them to be freed when garbage collected. (This prevents problems with reference cycles involving C and JS)

 this.actor.get_children().forEach(function (actor) { actor.destroy(); });

Would be better.

@@ +176,3 @@
+        }
+        if (!this._categories[c].categories[0].length)
+            return false;

The special value for a list containing a single value [''] is weird and confusing. Better to use some explicit keyword like 'OTHER'

@@ +178,3 @@
+            return false;
+
+        let categories = app.get_categories();

Suggest appCategories, since we are comparing two different lists that are 'categories', using that as a variable name is confusing.

@@ +180,3 @@
+        let categories = app.get_categories();
+        for (let i = 0; i < categories.length; i++) {
+            for (let k = 0; k < this._categories[c].categories.length; k++)

This is going to read better with something like

 let category = this._categories[c]

(not for efficiency, but for compactness and readability)

@@ +181,3 @@
+        for (let i = 0; i < categories.length; i++) {
+            for (let k = 0; k < this._categories[c].categories.length; k++)
+                res = res || (categories[i] == this._categories[c].categories[k]);

simpler as just 'if (...) return true', and a 'return false' below.

@@ +194,3 @@
+
+        for (let i = 0; i < apps.length; i++) {
+            let b = true;

'let found = false'

@@ +218,3 @@
+};
+
+Signals.addSignalMethods(ViewByCategories.prototype);

You don't have any signals that I can see

::: src/shell-app-system.c
@@ +1114,1 @@
 shell_app_info_get_categories (ShellAppInfo *info)

Your implementation involves reparsing all the desktop files, which is pretty bad from a performance point of view.

I talked to walters about this, and there wasn't an obvious better solution, but I think it would be good to submit a patch against gmenu to add gmenu_tree_entry_get_categories(), and then make this conditional onL:

 HAVE_GMENU_TREE_ENTRY_GET_CATEGORIES

(With an AC_CHECK_FUNCS in configure.ac - you'll have to save the libs/cflags, add the ones from gmenu, then restore. You can find examples of this in various configure.ac files if you look around.)

Also, with the current implementation, you really, really, need to cache this. Right now, it looks like you are parsing the desktop file every time _categoryContainApplication() is called.

@@ +1127,3 @@
+      g_key_file_free (keyfile);
+
+      return NULL;

I don't think returning NULL like this is OK.

Either 

 * we consider this an error that the caller might want to handle. In this case you should take a GError and pass it along to the GKeyFile functions. (Caller needs a catch block.)

 * we want to simply say "no categories" - you should return an empty array not NULL. Hack to easily return an empty array: return g_strsplit("")

Created attachment 160300 [details] [review]
sections in all apps view

Review of attachment 160300 [details] [review]:

Definitely looking better. Still a few more comments.

In the commit message

 Section can contain only apps, that move by user to it.

Do you mean:

 If you have a section with no categories, then it will only have the applications that the user moves to it explicitly

?

::: configure.ac
@@ +79,3 @@
 LIBS=$MUTTER_PLUGIN_LIBS
 AC_CHECK_FUNCS(sn_startup_sequence_get_application_id)
+AC_CHECK_FUNCS(gmenu_tree_entry_get_categories)

I'd just put all the functions we want to check into one AC_CHECK_FUNCS

AC_CHECK_FUNCS(gmenu_tree_entry_get_categories
               sn_startup_sequence_get_application_id

Or something like that.

::: data/gnome-shell.schemas
@@ +33,2 @@
       <schema>
+        <key>/schemas/desktop/gnome/shell/enabled_categories</key>

Suggest enabled_categories => app_section_categories

@@ +39,3 @@
+	<default>[OTHER,Game,Utility;Development;System]</default>
+	<locale name="C">
+	  <short>Categories for each section in the application browser</short>

I'd really like to see a long description that mentions a) that the elements in the list are the sections b) that the categories in each section are separated by ; c) the meaning of OTHER.

@@ +45,3 @@
+      <schema>
+        <key>/schemas/desktop/gnome/shell/changes_in_categories</key>
+	<applyto>/desktop/gnome/shell/changes_in_categories</applyto>

Suggest:

 changes_in_categories => app_section_changes

@@ +51,3 @@
+	<default>[]</default>
+	<locale name="C">
+	  <short>Applications moved between application browser sections</short>

Similarly here, you really should have a long description that says what is in here.

::: js/ui/appDisplay.js
@@ +113,3 @@
+    },
+
+    _moveToCategory: function (app, c) {

This is moveToSection (and so 'c' should be section_index or something)

@@ +149,3 @@
+        for (let i = 0; i < list.length; i++) {
+            let t = list[i].split(':');
+            if (t.length != 3 && t[0] != 'add')

|| not &&

@@ +252,3 @@
+// This need only for AlphabeticalView.
+//        this._appView.connect('launching', Lang.bind(this, this.close));
+//        this._appView.connect('drag-begin', Lang.bind(this, this.close));

Is the commented out part stray from some other patch? There's no AlphabeticalView in the sources.

::: src/shell-app-system.c
@@ +1122,3 @@
+  g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED, "Not supported");
+  return NULL;
+#else

I think we really need caching here inside the AppInfo struct - parsing the KeyFile every time this function is called is really expensive.

@@ +1128,3 @@
+
+  if (!(path = shell_app_info_get_desktop_file_path (info)))
+    return NULL;

You return NULL here without setting the error

Created attachment 160403 [details] [review]
sections in all apps view

> Is the commented out part stray from some other patch? There's no
> AlphabeticalView in the sources.
I rename AllAppVIew to AlphabeticalView

> I think we really need caching here inside the AppInfo struct - parsing the
> KeyFile every time this function is called is really expensive.
It will be cached in gmenu.

Review of attachment 160403 [details] [review]:

In terms of the caching. There things that are slightly inefficient, and having a plan to do better in the future is good enough. Then there are things that are so inefficient that I think its worth a few lines of code to avoid that inefficiency right now. This is the latter category :-). I don't think it's much code at all to cache a char ** categories in the AppInfo structure.

(Note that if you cache, you probably should switch to silently returning an empty list rather than returning a GError, since you don't want to have to cache a copy of the GError in the AppInfo structure.)

::: data/gnome-shell.schemas
@@ +41,3 @@
+	  <short>Categories for each section in the application browser</short>
+          <long>
+            Each element of the list describe a section. The categories in each section are separated by ";". "OTHER" keyword mean all application, that does not include in other sections.

Can you line-wrap these descriptions to a reasonable width?

Content looks good. Slight edit:

    Each element of the list describe a section, and lists the
    desktop files categories for that section, separated by ";".
    The "OTHER" keyword mean all applications that are not included 
    other sections.

@@ +56,3 @@
+	  <short>Applications moved between application browser sections</short>
+          <long>
+            Each element represent an action. Supported follow actions: add(move application to this section). String has format action:section_num:app_id.

And here:

  Each element represent an action. Only one action is currently supported:
  "add", which moves an application to this section. The string for add 
  has the format "add:section_num:app_id".

(I'm assuming that if we have other actions, they won't have the exact same format since they will need different parameters.)

::: js/ui/appDisplay.js
@@ +252,3 @@
+// This need only for AlphabeticalView.
+//        this._appView.connect('launching', Lang.bind(this, this.close));
+//        this._appView.connect('drag-begin', Lang.bind(this, this.close));

OK, so what does the comment mean and why did you comment out the code rather than removing it? Is something unfinished here?

No native English speaker here, so take those comments with a grain of salt:

(In reply to comment #13)
> Content looks good. Slight edit:
>
>     The "OTHER" keyword mean all applications that are not included 
>     other sections.

Shouldn't it be "means" (or maybe "refers to") and "included in other sections"?


> And here:
> 
>   Each element represent an action.

represents

(In reply to comment #14)
> No native English speaker here, so take those comments with a grain of salt:
>
> >     The "OTHER" keyword mean all applications that are not included
> >     other sections.
> 
> Shouldn't it be "means" (or maybe "refers to") and "included in other
> sections"?
> 
> >   Each element represent an action.
> 
> represents

Yes, both should have the 's'

(Sometimes, when reading, I see what I expect to see not what is actually there :-)

Created attachment 162156 [details] [review]
sections in all apps view

> (Note that if you cache, you probably should switch to silently returning an
> empty list rather than returning a GError, since you don't want to have to
> cache a copy of the GError in the AppInfo structure.)

App without categories OR got an Error here is very rary. So if info->categories == NULL, I just do work again.

We can use custom [prefix-]applications.menu (It will distribute with GnomeShell) and forget about ENABLED_CATEGORIES_KEY/CHAGES_IN_CATEGORIES_KEY. Is such implementation better?

Created attachment 162423 [details] [review]
sections in all apps view

It works in same way. There is only one difference - user can't drag app between sections.
(It conflict with feature from bug 618055)

It set XDG_MENU_PREFIX="gs-" on start. It use
${XDG_MENU_PREFIX}applications.menu as application menu layout. Menu can
be changed via SystemPreferences->MainMenu.
Section can't contain subsection.

Review of attachment 162423 [details] [review]:

This certainly looks simpler and cleaner than the last version. Only tiny comments below.

I'm going to ask Colin about his opinion on using the menu system this way.

My main reservation is that alacarte (Preferences/Main Menu) is *not* a good experience for the user at all. But no worse than editing GConf keys manually :-)

::: data/gs-applications.menu
@@ +34,3 @@
+		</Include>
+	</Menu>
+</Menu>

Should this have a <DefaultMergeDirs/> element?

::: js/ui/appDisplay.js
@@ +118,3 @@
+
+    _removeAll: function() {
+        this.actor.get_children().forEach(function (actor) { actor.destroy(); });

Can use this.actor.destroy_children() from StContainer

::: src/shell-app-system.c
@@ +1153,3 @@
+ * shell_app_info_get_section:
+ *
+ * return name of section, that contain this application.

This isn't a properly formatted doc comment - should have 'Returns: ...'

@@ +1155,3 @@
+ * return name of section, that contain this application.
+ */
+char*

char *

@@ +1172,3 @@
+    return NULL;
+
+  while ( (pparent = gmenu_tree_item_get_parent ((GMenuTreeItem*)parent)) )

we generally avoid inline assignments in almost all cases, so I'd do

 while (TRUE)
   {
      GmenuTreeDirectory *pparent = gmenu_tree_item_get_parent ((GMenuTreeItem*)parent));
      if (!pparent)
         break;

Created attachment 162721 [details] [review]
sections in all apps view

> Should this have a <DefaultMergeDirs/> element?
No. It ignore ALL other *.menu files.
such files only add additional menu entry (for example: wine add 'wine' SubMenu).

> My main reservation is that alacarte (Preferences/Main Menu) is *not* a good
> experience for the user at all.
I agree.

> But no worse than editing GConf keys manually
alacarte can't change categories in SubMenus. (So need edit .menu file)
In my opinion, Things like 'drag to other section', 'hide item' should be part of UI.
But drag conflict with feature from bug 618055.
'Hide item' can be include in right click menu.

(In reply to comment #20)
> Created an attachment (id=162721) [details] [review]
> sections in all apps view
> 
> > Should this have a <DefaultMergeDirs/> element?
> No. It ignore ALL other *.menu files.
> such files only add additional menu entry (for example: wine add 'wine'
> SubMenu).

OK. We can always revisit this later if Wine has a good case for wanting to add its own section...

> > My main reservation is that alacarte (Preferences/Main Menu) is *not* a good
> > experience for the user at all.
> I agree.
> 
> > But no worse than editing GConf keys manually
> alacarte can't change categories in SubMenus. (So need edit .menu file)
> In my opinion, Things like 'drag to other section', 'hide item' should be part
> of UI.

Hmm, I'm not really sure that in place is the best way to edit - how would you add a new category, for example?

But in any case, editing is minor compared to getting the sections in at all. Let's go with this patch since it's straightforward and simple.

Review of attachment 162721 [details] [review]:

Suggested commit message (same information as yours, but less telegraphic:

===
Add sections to the all apps view

Separate out the main app view into different sections based on the categories
in the desktop file. The configuration is done via gmenu and the desktop menu
specification, we set XDG_MENU_PREFIX="gs-" on startup, so that gmenu reads
gs-applications.menu, which we install.

There is no support for "submenus" - only the menus directly under Applications
will be displayed as categories.
===

alacarte not mentioned, since I couldn't figure out how you would use it to
edit gs-applications.menu rather than applications.menu.

gs-applications.menu is missing in the patch which breaks the build.

make[2]: *** No rule to make target `gs-applications.menu', needed by `all-am'

Created attachment 162779 [details] [review]
sections in all apps view

Add missing file.

If shell was compiled via jhbuild, It choose incorrect .menu file.
So I add export 'XDG_CONFIG_DIRS' to src/gnome-shell.in

Review of attachment 162779 [details] [review]:

This one works fine.

(In reply to comment #22)
> I couldn't figure out how you would use it to
> edit gs-applications.menu rather than applications.menu.

set XDG_MENU_PREFIX="gs-" OR run it under gnome-shell