Created attachment 221077 [details] [review]
iconGrid: Add updateStyle()

This function allows us to make sure the values pulled from the CSS are
correct before we do anything with them.

Created attachment 221078 [details] [review]
searchDisplay, and others: Switch from provider title to provider icon

Display a '+' icon on the provider icon if there are more results that are
hidden. If the provider icon is clicked, ask the provider to launch itself and
perform a search with the current terms.

--

On the very first search with a (re)started Shell, it seems
IconGrid.childrenInRow() is not picking up the icon width from the CSS before
we draw the results, so it considers 48px as icon width to calculate
from, when in fact it is more like 118px. That means _notDisplayedResult is
wrong in GridSearcResults, which throws off the 'more' icon initially. Things
sort themselves on subsequent searches (or sub-searches), but if we explicitly
ask IconGrid to pull values from the CSS before doing calculations, the very
first search works properly as well.

This patch adds a new function remote providers need to support,
'launchSearch' which should open the provider application and initiate
a search within the application with the current search terms.

Created attachment 221079 [details] [review]
iconGrid: Add ResultsList class

A thin wrapper around a BoxLayout.

Created attachment 221080 [details] [review]
searchDisplay: Add ListSearchResult and ListSearchResults

These are for all search results except apps.

--

This patch pulls in PopupMenu to use PopupSeparatorMenuItem for the search
results separator. I doubt this is what we want ultimately, but should
searchDisplay.js have it's own small separator class or otherwise
centralize/share the drawing code between the two uses? The search separator
will need to be more white than the popup separator, so we need to be able to
style them separately in the end.

Created attachment 221081 [details] [review]
searchDisplay: Set can_focus on provider icon only if its results have focus

This way, the search results take priority, but while the user is in the
search section, they can navigate to that provider icon to launch a search.

Created attachment 221083 [details] [review]
viewSelector: Adapt to SearchDisplay name change

Created attachment 221084 [details] [review]
theme: Update for search redesign

Created attachment 221085 [details] [review]
{app,place}Display: Add provider icons for the search system

Only temporary, to test with, unless remote providers for Settings and Places
& Devices don't materialize. this.icon should be a GIcon since that is what
remote providers will return.

I noticed you didn't remove the OpenSearch system. Any reason for that? It's removed in the designs, and I already have a patch to remove it entirely.

https://bugzilla.gnome.org/show_bug.cgi?id=670168

Review of attachment 221077 [details] [review]:

This is a hack workaround. I'd like it not to go in eventually.

Review of attachment 221078 [details] [review]:

::: js/ui/search.js
@@ +175,3 @@
+     */
+    launchSearch: function(terms) {
+     * @terms: Current search terms

Do we want to check for the existence of this in normal search providers? What do we do in the case of more than 5 or so apps?

Needs design.

::: js/ui/searchDisplay.js
@@ +133,2 @@
     getResultsForDisplay: function() {
+        this._grid.updateStyle();

Hack.

@@ +146,3 @@
     },
 
+    isMoreResults: function() {

hasMoreResults

@@ +147,3 @@
 
+    isMoreResults: function() {
+        return this._notDisplayedResult.length > 0 ? true : false;

Comparisons already produce booleans, no need to make one yourself.

@@ +410,3 @@
+                meta.icon.updateTerms(terms);
+
+                let isMoreResults = meta.resultDisplay.isMoreResults();

hasMoreResults

@@ +489,3 @@
+        this.actor.connect('clicked', Lang.bind(this, this._onIconClicked));
+
+        this.provider = provider;

It seems you could do this same exact layout with a ClutterBinLayout, adjusting alignment properties.

@@ +497,3 @@
+                                       icon_size: this.MORE_ICON_SIZE,
+                                       icon_name: 'list-add' });
+                                     reactive: true,

What's the bin for? Why not add the style class to the icon?

@@ +523,3 @@
+
+        if (provider.icon)
+                                       icon_name: 'list-add' });

I don't think we ever have a "provider.icon". I see it as a thing passed to the constructor in RemoteSearchProvider, but we never assign it.

@@ +525,3 @@
+            icon.gicon = provider.icon;
+        else
+        this._moreBin = new St.Bin({ style_class: 'search-section-icon-more',

Ouch. I'd prefer that we just crash on a NULL icon, as that means that a missing icon will get fixed immediately instead of just being ugly.

But Florian is free to override :)

@@ +530,3 @@
+    },
+
+                                     width: this.MORE_ICON_SIZE,

I'd just expose moreBin directly, so we can do:

    providerIcon.moreBin.visible = isMoreResults;

@@ +543,3 @@
+
+    activate: function() {
+

Not sure I like this. I'd have it emit a signal that the search system connects to.

Review of attachment 221079 [details] [review]:

I guess I don't see the point.

Review of attachment 221080 [details] [review]:

::: js/ui/searchDisplay.js
@@ +44,3 @@
+
+        let icon;
+        this.actor._delegate = this;

No...


No No No No No.

(And this isn't right either. It will just match DOCUMENTS, and not CONTACTS.)

@@ +50,3 @@
+            // By definition, createIcon should return a Clutter.Texture, but
+            // external apps may not want to include Clutter, so allow them to
+                                         vertical: false });

No. And to be honest, they really should be returning GIcons.

@@ +67,3 @@
+            content.add(icon);
+        }
+            // By definition, createIcon should return a Clutter.Texture, but

Seems like we're missing something that sets the icon for a non-documents (Wanda, Settings, Remote) provider?

@@ +446,3 @@
+                                             vertical: true });
+        let providerBoxContent = new St.BoxLayout({ style_class: 'search-section-content' });
+        let isAppsProvider = provider.title == _("APPLICATIONS");

Still nope. We should rearchitecture it so that you pass the kind of renderer you want when you add the search provider.

Review of attachment 221081 [details] [review]:

I don't think we should ever dynamically change focus. You should be able to bounce up and down providers while you have one provided, to me.

Needs design, of course. Marking as reviewed, tentatively. You still have some FIXME comments that need to be addressed, too.

Review of attachment 221083 [details] [review]:

This should be squashed when the change was made.

Review of attachment 221084 [details] [review]:

Just CSS.

Review of attachment 221085 [details] [review]:

::: js/ui/appDisplay.js
@@ +312,3 @@
         this._appSys = Shell.AppSystem.get_default();
+        // Could just set icon to 'noDisplay' or something and check for that
+        // instead of for title

I don't understand?

(In reply to comment #9)
> I noticed you didn't remove the OpenSearch system. Any reason for that? It's
> removed in the designs, and I already have a patch to remove it entirely.
> 
> https://bugzilla.gnome.org/show_bug.cgi?id=670168

The following work is now rebased on that patch. 

Additional dependency on Bug 670168.

Created attachment 221278 [details] [review]
searchDisplay, and others: Switch from provider title to provider icon

Display a '+' icon on the provider icon if there are more results that are
hidden. If the provider icon is clicked, ask the provider to launch itself and
perform a search with the current terms.

--

Ultimately, we don't need launchSearch for internal providers since the apps
provider should be the only internal provider and it doesn't have a provider
icon to even activate. But, while Places and Settings providers are still
internal we may.

Changes:
 * isMoreResults --> hasMoreResults + cleanup
 * Switch to a St.Widget and Clutter.BinLayout for ProviderIcon
 * Use ensure_style instead of adding updateStyle(). Still hacky, but remove
     some useless code.
 * Make moreIcon a public property to make setting visibility simpler.
 * Add launchSearch() to SearchSystem and have ProviderIcon emit
    'launch-search' signal which is connected to SearchSystem in
    _createProviderMeta. Maybe not exactly what Jasper had in mind.
 * Set this.icon for remote providers.

Created attachment 221281 [details] [review]
searchDisplay: Add ListSearchResult and ListSearchResults

These are for all search results except apps.

--

The last instructions I received were that only the apps provider gets a grid
layout and no provider icon. In addition, only Documents and Contacts get an
icon/thumbnail with their result. Checking against titles seemed the easiest
way to treat these unique situations. Although, I suppose providers could just
spoof their titles to get what layout they wanted (though we wouldn't be
responsible for breakage). So without checking for title uniqueness (which is
probably more trouble than it's worth), letting providers configure by params
may be the best option.

Changes:
 * Fix check for Documents and Contacts providers. (still using titles)
 * Documents and Contacts should just return GIcons for their content
     icon/thumbnail, so simplify code.
 * Remove ResultsList, just let ListSearchResults do its own layout (never
     liked it being separate anyway)

Created attachment 221285 [details] [review]
searchDisplay: Set can_focus on provider icon only if its results have focus

This way, the search results take priority, but while the user is in the
search section, they can navigate to that provider icon to launch a search.

--

Changes:
 * _content is now public in GridSearchResults

Created attachment 221286 [details] [review]
theme: Update for search redesign

--

Rearranged some things, but no real changes.

Created attachment 221287 [details] [review]
{app,place}Display: Add provider icons for the search system

Only temporary, to test with, unless remote providers for Settings and Places
& Devices don't materialize. this.icon should be a GIcon since that is what
remote providers will return.

--

Removed an old comment.

Created attachment 221497 [details] [review]
searchDisplay, and others: Switch from provider title to provider icon

Display a '+' icon on the provider icon if there are more results that are
hidden. If the provider icon is clicked, ask the provider to launch itself and
perform a search with the current terms.

--

Changes:
 * Handle RTL locales properly for moreIcon
 * Use instanceof to check for apps provider instead of title

Created attachment 221498 [details] [review]
searchDisplay: Add ListSearchResult and ListSearchResults

These are for all search results except apps.

--

Changes:
 * Remove checks for Documents/Contacts. If a provider wishes to use
     a thumbnail in their result, they just need to set it in the meta. Most
     providers probably don't need to set a thumbnail, text should be good
     enough and they should prefer it, but in cases where a thumbnail is
     simply better for the content, have the option there.
 * Update docs for getResultMetas to include that an icon is now optional and
     that an optional description can also be set.

Created attachment 221499 [details] [review]
searchDisplay: Set can_focus on provider icon only if its results have focus

This way, the search results take priority, but while the user is in the
search section, they can navigate to that provider icon to launch a search.

--

No changes.

Created attachment 221500 [details] [review]
theme: Update for search redesign

--

No changes.

Created attachment 221501 [details] [review]
{app,place}Display: Add provider icons for the search system

Only temporary, to test with, unless remote providers for Settings and Places
& Devices don't materialize. this.icon should be a GIcon since that is what
remote providers will return.

--

No changes.

Created attachment 221502 [details] [review]
popupMenu: Break separator drawing code out of PopupSeparatorMenuItem

--

So the search separator and popup menu separator can share code, but set
different css classes (since control of the separator is done in css). This
can be squashed with the relevant commits if approved.

Review of attachment 221497 [details] [review]:

::: js/ui/searchDisplay.js
@@ +233,3 @@
+            providerIcon = new ProviderIcon(provider);
+            providerIcon.connect('launch-search', Lang.bind(this, function(providerIcon) {
+                this._searchSystem.launchSearch(providerIcon.provider);

This is *exactly* what I wanted. Thanks.

@@ +434,3 @@
+                                     accessible_role: Atk.Role.PUSH_BUTTON });
+        this.actor.connect('button-release-event', Lang.bind(this, this._onIconClicked));
+    _init: function(provider) {

Use an StButton; it handles this for you.

... hm, but that makes it reject multiple things inside of it. Put the StWidget inside an StButton, I guess.

@@ +446,3 @@
+                                      icon_name: 'list-add',
+                                      visible: false,
+                                     track_hover: true,

According to

http://git.gnome.org/browse/clutter/tree/clutter/clutter-actor.c#n15149

this should happen automatically if the actor is marked as RTL. This requires some investigation, then.

@@ +466,3 @@
+        let provider = this.provider;
+
+

Extra whitespace.

Review of attachment 221498 [details] [review]:

Mostly fine.

::: js/ui/searchDisplay.js
@@ +43,3 @@
+        this.actor.set_child(content);
+
+        this.actor._delegate = this;

???

@@ +44,3 @@
+
+        // An icon for or thumbnail of content
+        this.actor._delegate = this;

The existing code in GridSearchResult can handle it fine. We should probably refactor it out into a createIconFromMetaInfo method that takes the metaInfo and a size.

Review of attachment 221499 [details] [review]:

I expressed concerns about this before. Is there a design for this?

Review of attachment 221500 [details] [review]:

"Just CSS"

Review of attachment 221501 [details] [review]:

Nice.

Review of attachment 221502 [details] [review]:

::: js/ui/popupMenu.js
@@ +408,3 @@
+    _init: function (params) {
+        this.params = Params.parse(params, { style_class: 'default-horz-separator' });
+

Just use new St.DrawingArea(params);

Created attachment 221713 [details] [review]
searchDisplay, and others: Switch from provider title to provider icon

Display a '+' icon on the provider icon if there are more results that are
hidden. If the provider icon is clicked, ask the provider to launch itself and
perform a search with the current terms.

--

Changes:
 * Put ProviderIcon St.Widget inside a St.Button
 * Cleanup

Created attachment 221714 [details] [review]
searchDisplay: Add ListSearchResult and ListSearchResults

These are for all search results except apps.

--

Changes:
 * Try to clarify comment, though it could still be kinda hard to parse. Could
     probably just remove it.
 * I didn't look close enough in remoteSearch.js apparently, the createIcon
     method handles images passed in with results for us. No need for new code.

Created attachment 221715 [details] [review]
searchDisplay: Set can_focus on provider icon only if its results have focus

This way, the search results take priority, but while the user is in the
search section, they can navigate to that provider icon to launch a search.

--

The design page states, "Tab order starts with application handler being first,
down arrow goes directly through specific results." This code achieves the
latter, though ignores the former. With this patch, the down arrow goes
straight to the results and then you can move over to the icon if you want.
Without it, the provider icon is selected first if you press down, presumably
because it is the first child of the search sections St.BoxLayout. There's
probably a better way to handle this.

Created attachment 221716 [details] [review]
theme: Update for search redesign

--

No changes.

Created attachment 221717 [details] [review]
{app,place}Display: Add provider icons for the search system

Only temporary, to test with, unless remote providers for Settings and Places
& Devices don't materialize. this.icon should be a GIcon since that is what
remote providers will return.

--

No changes.

Created attachment 221718 [details] [review]
popupMenu: Break separator drawing code out of PopupSeparatorMenuItem

--

Changes:
 * Pass params straight to DrawingArea.

Created attachment 221719 [details] [review]
remoteSearch: We do not need a fallback for createIcon

Remote providers no longer have access to a grid layout, where an icon is
a requirement. If they don't specify an icon, don't create one.

Created attachment 221720 [details] [review]
remoteSearch: Only add createIcon to meta if an icon is specified

Instead of doing this, we could just check if the return from createIcon is
undefined or null and just not add it to the ListSearchResult on creation.

--

I don't know which approach I prefer, the one here or always calling
createIcon and just not adding the return to the result if it isn't suitable.

Created attachment 221721 [details] [review]
appDisplay: Remove createIcon from meta of SettingsSearchProvider

The settings search results are not necessarily improved by including icons,
so don't.

--

I suppose this is debatable, but the mockups don't show icons for settings.

Created attachment 221722 [details] [review]
placeDisplay: Remove createIcon from meta of PlaceSearchProvider

Review of attachment 221713 [details] [review]:

OK, this looks good.

Review of attachment 221714 [details] [review]:

Nice!

Review of attachment 221715 [details] [review]:

Still disturbed by this. I don't think it's the end of the world if this doesn't make it in, and I think it causes extreme experience issues.

We installed something called the FocusTrap when rtcm did keyboard navigation I think (did that patch make it in) -- an invisible actor in between the search display and search box that would move the focus to the second actor if you hit "right". Maybe something similar can be done here.

Review of attachment 221716 [details] [review]:

.

Review of attachment 221717 [details] [review]:

Mhm.

Review of attachment 221718 [details] [review]:

Sure.

Review of attachment 221719 [details] [review]:

I wonder if we could remove createIcon altogether? It's not used, right? But sure.

Review of attachment 221720 [details] [review]:

Yeah, not a fan. I'd rather modify createIcon.

... Wait, you removed the code that had the fallback in the last path. I don't understand this?

Review of attachment 221721 [details] [review]:

Sure.

Review of attachment 221722 [details] [review]:

Squash with the previous patch, please.

Review of attachment 221720 [details] [review]:

OK, yeah, I see what's happening here. I'd just make the code that calls createIcon handle nulls properly.

Created attachment 222019 [details] [review]
searchDisplay, and others: Switch from provider title to provider icon

Display a '+' icon on the provider icon if there are more results that are
hidden. If the provider icon is clicked, ask the provider to launch itself and
perform a search with the current terms.

__

No changes.

Created attachment 222020 [details] [review]
searchDisplay: Add ListSearchResult and ListSearchResults

These are for all search results except apps.

--

Changes:
 * Handle nulls from createIcon.

Created attachment 222021 [details] [review]
searchDisplay: Set can_focus on provider icon only if its results have focus

This way, the search results take priority, but while the user is in the
search section, they can navigate to that provider icon to launch a search.

--

For me, keyboard nav will be broken, or at least more annoying, if I'm always
going to hit the provider icon first when I press down to get to the search
results. So if we don't like this patch, but still want the new layout,
I would just remove can_focus from the provider icon entirely and only be able
to launch it with a pointer, until we can get proper keyboard nav setup. My
feelings on the matter.

Created attachment 222022 [details] [review]
theme: Update for search redesign

--

No changes.

Created attachment 222023 [details] [review]
{app,place}Display: Add provider icons for the search system

Only temporary, to test with, unless remote providers for Settings and Places
& Devices don't materialize. this.icon should be a GIcon since that is what
remote providers will return.

--

The commit message could use some work. No changes.

Created attachment 222024 [details] [review]
popupMenu: Break separator drawing code out of PopupSeparatorMenuItem

--

No changes.

Created attachment 222025 [details] [review]
remoteSearch: We do not need a fallback for createIcon

Remote providers no longer have access to a grid layout, where an icon is
a requirement. If they don't specify an icon, don't create one.

--

Changes:
 * Return null if no icon is specified.

Created attachment 222026 [details] [review]
{app,place}Display: Don't use icons in results

The search results are not necessarily improved by including icons, so don't.

The previous patch set has been rebased on current master.

Created attachment 230944 [details] [review]
searchDisplay, and others: Switch from provider title to provider icon

Display a '+' icon on the provider icon if there are more results that are
hidden. If the provider icon is clicked, ask the provider to launch itself and
perform a search with the current terms.

Created attachment 230945 [details] [review]
popupMenu: Break separator drawing code out of PopupSeparatorMenuItem

And into a separate HorizontalSeparatorClass.

Created attachment 230946 [details] [review]
searchDisplay: Add ListSearchResult and ListSearchResults

These are for all search results except apps (and Wanda).
We also simplify a bit the packing of search results, which removes some
ugly code in navigateFocus() where we needed to call
st_widget_navigate_focus() twice, since the grid icon was composed by
two nested boxes, both focusable.

Created attachment 230947 [details] [review]
remoteSearch: We do not need a fallback for createIcon

Remote providers no longer have access to a grid layout, where an icon is
a requirement. If they don't specify an icon, don't create one.

Created attachment 230948 [details] [review]
appDisplay: Don't use icons in results for settings

The search results are not necessarily improved by including icons, so don't.

Created attachment 230949 [details] [review]
search: remove SearchResultDisplay base class

It's unused, and the clear() method is just wrong. Remove it.

Created attachment 230950 [details] [review]
search: remove SearchProvider base class

This is causing more confusion than anything else these days; the DBus
API is properly documented now and that's what people are expected to
use, the rest are implementation details we're not interested in
exposing.

Created attachment 230951 [details] [review]
search: remove more dead code

This is Tanner's patchset reworked and rebased to master.
It changes the search display from grid view to list view (except for applications), but it doesn't touch other parts of the overview layout.

Review of attachment 230944 [details] [review]:

::: data/theme/more-results.svg
@@ +1,2 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>
+<!-- Created with Inkscape (http://www.inkscape.org/) -->

Since I can't quite read this with Inkscape, this icon includes the rounded box, right?

::: js/ui/appDisplay.js
@@ +377,1 @@
         this._appSys = Shell.AppSystem.get_default();

You're missing an ID?

::: js/ui/searchDisplay.js
@@ +230,3 @@
+        let providerIcon = null;
+
+        if (provider.appInfo) {

We might want to make Wanda be able to have an icon, but not sure it's worth it. Just don't make sure it doesn't outright break.

::: js/ui/wanda.js
@@ +138,2 @@
     _init: function() {
+        this.parent();

You're missing an ID?

Review of attachment 230945 [details] [review]:

This is fine for me, but I'd like to move a lot of these into a "widgets" file or directory.

Review of attachment 230946 [details] [review]:

::: js/ui/searchDisplay.js
@@ +15,3 @@
 const Search = imports.ui.search;
 
+const MAX_SEARCH_RESULTS_ROWS = 3;

MAX_LIST_SEARCH_RESULTS_ROWS

@@ +106,3 @@
         this._dragActorSource = null;
 
         let content = provider.createResultActor(metaInfo, terms);

.oO( I wonder if we should tear down createResultActor down eventually, since it's tied to the grid system, which should only be used for apps. )

@@ +113,2 @@
             let icon = new IconGrid.BaseIcon(this.metaInfo['name'],
                                              { createIcon: this.metaInfo['createIcon'] });

.oO( Same with this IconGrid nonsense. )

@@ +118,3 @@
         } else {
             if (content._delegate && content._delegate.getDragActorSource)
+                dragSource = content._delegate.getDragActorSource();

What implements this, if you don't mind me asking?

@@ +139,3 @@
+        if (!dragSource)
+            // not exactly right, but alignment problems are hard to notice
+            dragSource = content;

What's all this shuffling around? This should be a separate patch.

@@ +182,3 @@
+        this._content = new St.BoxLayout({ style_class: 'list-search-results',
+                                           vertical: true });
+        this._content.connect('notify::width', Lang.bind(this, function() {

Why should the width changing allow us to have more results? It's a *vertical list*?

Actually, why should the width change in general?

Do we actually need all this crazy state?

We might be able to tear it down in the grid results as well. Might be a relic of the past.

@@ +202,3 @@
+        let canDisplay = MAX_SEARCH_RESULTS_ROWS - alreadyVisible;
+
+        let numResults = Math.min(this._notDisplayedResult.length, canDisplay);

Shouldn't be necessary -- the array should already do this implicitly for you.

@@ +204,3 @@
+        let numResults = Math.min(this._notDisplayedResult.length, canDisplay);
+
+        return this._notDisplayedResult.splice(0, numResults);

Too clever.

@@ -410,3 @@
         let from = this._defaultResult ? this._defaultResult.actor : null;
         this.actor.navigate_focus(from, direction, false);
-        if (this._defaultResult) {

Why was this removed? Should we be able to remove it now, independently of the grid system?

Review of attachment 230947 [details] [review]:

Nope, we don't.

Review of attachment 230948 [details] [review]:

Run it by a designer first.

::: js/ui/appDisplay.js
@@ +384,3 @@
             metas.push({ 'id': pref,
                          'name': pref.get_name(),
+                         'createIcon': function() { return null; }

We can just omit the function altogether, right?

Review of attachment 230949 [details] [review]:

Goodbye.

Review of attachment 230950 [details] [review]:

::: js/ui/wanda.js
@@ +135,2 @@
     _init: function() {
+        this.id = 'wanda';

Aha! That's where that went.

Review of attachment 230951 [details] [review]:

Of course.

Created attachment 231187 [details] [review]
searchDisplay, and others: Switch from provider title to provider icon

--

Fixes review comments. Yes, the icon includes the white rounded box.

Created attachment 231189 [details] [review]
searchDisplay: simplify drag actor source code

--

Split out from the following patch as requested.

Created attachment 231191 [details] [review]
searchDisplay: Add ListSearchResult and ListSearchResults

--

Fixes review comments:
- removed connections to notify::width
- simplified the getResultsForDisplay/splice code

Created attachment 231193 [details] [review]
search: remove SearchProvider base class

--

Trivial change, moved the wanda id to the other patch.

(In reply to comment #75)
> Review of attachment 230944 [details] [review]:
> ::: js/ui/appDisplay.js
> @@ +377,1 @@
>          this._appSys = Shell.AppSystem.get_default();
> 
> You're missing an ID?

No, it's set automatically to the appInfo's id for the settings provider.

(In reply to comment #77)

> .oO( I wonder if we should tear down createResultActor down eventually, since
> it's tied to the grid system, which should only be used for apps. )

After this patchset, createResultActor is used for application icons and for the wanda provider. The only reason I kept it for now is that the wanda actor is the animated fish, and I didn't want to break it. We can improve it/clean it up further in later patches though.

> @@ +113,2 @@
>              let icon = new IconGrid.BaseIcon(this.metaInfo['name'],
>                                               { createIcon:
> this.metaInfo['createIcon'] });
> 
> .oO( Same with this IconGrid nonsense. )

Yeah, I just don't think it should be part of this patchset.

> @@ +118,3 @@
>          } else {
>              if (content._delegate && content._delegate.getDragActorSource)
> +                dragSource = content._delegate.getDragActorSource();
> 
> What implements this, if you don't mind me asking?

This is implemented by AppDisplay. createResultActor() in appDisplay.js will create an AppWellIcon, which has a getDragActorSource() method and does this.actor._delegate = this; in _init().

> @@ -410,3 @@
>          let from = this._defaultResult ? this._defaultResult.actor : null;
>          this.actor.navigate_focus(from, direction, false);
> -        if (this._defaultResult) {
> 
> Why was this removed? Should we be able to remove it now, independently of the
> grid system?

The reason why this is not needed anymore is in the commit message; with it the focus was getting moved to the second next actor.

(In reply to comment #79)

> Run it by a designer first.

I think this was originally written by Tanner due to designer's feedback.
All the mockups indeed do not show icons for settings panels.

> ::: js/ui/appDisplay.js
> @@ +384,3 @@
>              metas.push({ 'id': pref,
>                           'name': pref.get_name(),
> +                         'createIcon': function() { return null; }
> 
> We can just omit the function altogether, right?

Unfortunately we can't - we'd need to check for meta['createIcon'] != null before calling the function, which we currently don't. I don't think it's worth the hassle to refactor that code now to be honest, I'd rather clean up all these icon-related functions altogether.

Review of attachment 230948 [details] [review]:

OK.

Review of attachment 231187 [details] [review]:

Looks fine to me.

Review of attachment 231189 [details] [review]:

Sure.

Review of attachment 231193 [details] [review]:

Sure.

Review of attachment 231191 [details] [review]:

Sure.

And this first part is now merged to master, thanks everyone!

Attachment 230945 [details] pushed as b48a7d5 - popupMenu: Break separator drawing code out of PopupSeparatorMenuItem
Attachment 230947 [details] pushed as 6075332 - remoteSearch: We do not need a fallback for createIcon
Attachment 230948 [details] pushed as 835d4e4 - appDisplay: Don't use icons in results for settings
Attachment 230949 [details] pushed as 3aa0d45 - search: remove SearchResultDisplay base class
Attachment 230951 [details] pushed as 0155739 - search: remove more dead code
Attachment 231187 [details] pushed as 9af107f - searchDisplay, and others: Switch from provider title to provider icon
Attachment 231189 [details] pushed as 575ab0d - searchDisplay: simplify drag actor source code
Attachment 231191 [details] pushed as c5d8484 - searchDisplay: Add ListSearchResult and ListSearchResults
Attachment 231193 [details] pushed as d0902fa - search: remove SearchProvider base class