Is there something new in this that I should address?  I just tried it and it is a little unclear what the changes are.  Should not that the first time I used it I ended up with a stuck grab after searching and clicking around the overview.  I had to alt-f2 / r.  But not sure if it was due to this patch or not.

> Is there something new in this that I should address?
new applications menu

> I just tried it and it is a little unclear what the changes are.
There is only one difference from mockup: 1 extra item in menu ("Other")

> But not sure if it was due to this patch or not.
This patch should not be a reason for such bug.

Assigning for Florian for review. It would make sense for me to rebase this patch on top of overview-relayout. I'd like that branch to be the next thing that lands for the overview.

Created attachment 175507 [details] [review]
adjust AllAppDisplay to match latest mockup

merge with HEAD

Review of attachment 175507 [details] [review]:

Looks pretty good - both the code and the actual behavior. I have a bunch of comments, but all of them should be easy to address. Looking forward to seeing this land!

::: data/theme/filter-selected.svg
@@ +10,3 @@
+   xmlns:sodipodi="http://sodipodi.sourceforge.net/DTD/sodipodi-0.dtd"
+   xmlns:inkscape="http://www.inkscape.org/namespaces/inkscape"
+   width="11"

Oh, is that where the 201 px in the CSS are coming from? Is it a big difference if you make the image 10x20? (Yay for textual image formats!)

::: data/theme/gnome-shell.css
@@ -63,3 @@
-    background-gradient-end: #111111;
-    height: 30px;
-}

Not sure I agree with that change - the shadows certainly look wrong now, but removing the CSS is just a quick way to hide the problem. It is certainly unrelated to this patch (e.g. the search results are affected as well)

@@ -487,3 @@
-.overview-pane {
-    width: 440px;
-}

Unrelated as well, but OK in my opinion - that class is just a left-over anyway.

@@ +479,3 @@
 
+.app-filters-container {
+    width: 201px;

Why 201 instead of 200? Certainly makes you wonder ...

::: js/ui/appDisplay.js
@@ +88,3 @@
     _init: function() {
         this._appSystem = Shell.AppSystem.get_default();
+        let container = new St.BoxLayout();

I'd add a style class here and assign some spacing in the CSS - the filters are too close to the content, and doing it this way seems cleaner than adding padding to the app-filters-container ...

(I'd suggest 'all-app', see comment below :-)

@@ +89,3 @@
         this._appSystem = Shell.AppSystem.get_default();
+        let container = new St.BoxLayout();
+        this.actor = new St.Bin({ child: container, x_fill: true, y_fill: true });

The St.Bin is not necessary, you can just use the container above for this.actor - the actor will be expanded appropriately when added to the view selector.

@@ +99,3 @@
+        this._view.connect('drag-begin', Lang.bind(this, function() {
+            this.emit('drag-begin');
+        }));

Those can actually be removed now - the 'launching' signal is completely unused, and 'drag-begin' is now handled via Main.overview.windowDragBegin().
I didn't remove those signals in the branch to reduce the number of conflicts with this patch, so now is a good time to remove them in both ViewByCategories and AlphabeticalView :-)

@@ +123,3 @@
+                this._sections[i].menuActor.add_style_pseudo_class('selected');
+            else
+                this._sections[i].menuActor.remove_style_pseudo_class('selected');

Nitpick: menuActor sounds like some kind of popup to me, can you rename this to something like filterTitle/filterActor?

@@ +136,3 @@
+        }));
+
+        return button;

It looks a bit strange to me that a function named addXyz() has a return value - how about passing the corresponding apps as well, special-casing -1 and moving the this._sections[num] = {...} code into the function?

@@ +149,3 @@
         let sections = this._appSystem.get_sections();
         if (!sections)
             return;

Hmm - don't we want at least the "All" filter?

@@ +187,3 @@
         this._appView = new ViewByCategories();
+        this.actor = this._appView.actor;
+        this.actor.style_class = 'all-app';

I don't like how the style class is assigned outside the ViewByCategories class - just add it to the constructor there.
More importantly, I don't see much sense any longer in the separation of AllAppDisplay and ViewByCategories - pretty much all the logic lives in ViewByCategories now, so the two could easily be merged. AppDisplay or AppView would both make sense as names for the joint prototype.

Created attachment 175884 [details] [review]
adjust AllAppDisplay to match latest mockup

> More importantly, I don't see much sense any longer in the separation of
> AllAppDisplay and ViewByCategories
main purpose of AllAppDisplay is easily switching between views.
I added setView to AllAppDisplay. So appView can be changing by extension.

Review of attachment 175884 [details] [review]:

I don't think the new method is a particularly good idea (reasoning follows). All other comments are style comments.

::: js/ui/appDisplay.js
@@ +81,3 @@
         this._appSystem = Shell.AppSystem.get_default();
+        let container = new St.BoxLayout();
+        this.actor = container;

Why not
this.actor = new St.BoxLayout({ style_class: 'all-app' });

I don't see how the separate container variable adds anything (readability, clarity, ...), and in my opinion the style class should always be assigned in the constructor unless the actor is created outside the scope (e.g. because it's inherited) or the style class is subject to change (e.g. to reflect state).

@@ +131,3 @@
+                                    name: name };
+        } else
+            this._allFilter = button;

The else part should have braces as well.

@@ +178,3 @@
 
+    setView: function(appView) {
+        this.actor.set_child(appView.child);

The code won't work (appView.child => appView.actor), but that's not the main issue I have with that method:
 1. In the new layout, "view" refers to one tab in the view selector
    (e.g. AllAppDisplay itself is the view, not ViewByCategories)
 2. The application view is part of the core design - at least in my understanding,
    extensions are not expected to remove/swap out core stuff at will.
    If there is interest in extending the view, we can add hooks later (e.g. when 
    we know what's actually needed).
 3. There's no way extensions can actually call this method, so unless an AllAppDisplay
    object is made accessible in some property, it's completely useless.

Of course, if an extension author absolutely insists on messing with the core, it's still possible:
    let myAppView = new MyAppView();
    Main.overview.viewSelector._tabs[1].page = myAppView.actor;
I don't think that making extension authors jump through hoops to do highly discouraged stuff is a bad thing.

I still don't see a point in having an extra object/container here, but OK when this method is removed.

Created attachment 176131 [details] [review]
adjust AllAppDisplay to match latest mockup

Previous patch has performance problems.
1. switching between categories can take 400ms
2. first show ~400ms (was before) (search also have this problem)

This problems have same root.
constructing 1 AppWellIcon/SearchResult ~3ms.

In this patch, AppWellIcons don't recreated on category switch.

Created attachment 176132 [details] [review]
async create icons in appMenu && search

Review of attachment 176131 [details] [review]:

::: js/ui/appDisplay.js
@@ +69,3 @@
+    _appFilter: function(app) {
+        return true;
+    },

The function name should be _filterApp (e.g. verb+object), but ... eeeeks, having the function in the prototype and overwriting it later is extremely ugly.

I'd rather have

this._filterFunc = null;

in the constructor, and use something like

if (this._filterFunc && !this._filterFunc(appInfo))
    appIcon.actor.hide();

in _addApp(). Not sure if filterApps(filterFunc) would be a better name for setFilter(filter) ...

::: js/ui/iconGrid.js
@@ +170,3 @@
+        children = children.filter(function(actor) {
+            return actor.visible;
+        });

It might be a bit cleaner to add a function _getVisibleChildren() and use that here and in _allocate().

Other than that, the change looks reasonable, but should be done in a separate patch, something like:

  iconGrid: Exclude hidden children from the layout

  As all children were considered for the grid's layout, hidden items showed up as
  empty space. Instead, exclude hidden children from the layout, so that the grid is
  only made up of visible items.

Created attachment 176657 [details] [review]
adjust AllAppDisplay to match latest mockup

Created attachment 176658 [details] [review]
iconGrid: Exclude hidden children from the layout

Review of attachment 176658 [details] [review]:

There should be a blank line between the body and the bug reference. OK to push with that change.

Review of attachment 176657 [details] [review]:

The code looks good to me, but the commit message needs improvement. I'd suggest something along the lines of

app-display: Implement filtering applications by category

Add a list of filters to the application view of the view selector, as in the latest mockups.

(In reply to comment #11)
> Created an attachment (id=176657) [details] [review]
> adjust AllAppDisplay to match latest mockup

It might be worth getting some designer input on this as well - I'm not sure it makes sense to preserve the filter in all (or maybe even most) cases, maybe it's reasonable to reset the filter when leaving the overview or even switching view tabs. Not blocking in any way though.