Created attachment 164355 [details] [review]
[searchResults] Use WellGrid for application results

Extend WellGrid with an optional rowLimit parameter and an accessor
function for the grid's items, then use it to replace the custom
container used to display the application search results.
Also adjust the spacing of the search results slightly to make it
consistent with the app well.

Is this worth reviewing now, or is it obsoleted by the new overview layout work?

(In reply to comment #2)
> Is this worth reviewing now, or is it obsoleted by the new overview layout
> work?

I think it is still worth reviewing - the search results are still used with the new layout, so even without the benefit of more consistent padding/spacing in the dash there will be less code duplication.

Review of attachment 164355 [details] [review]:

Generally looks good and nice to eliminate a bunch of code and go to a consistent appearance. Few small issues.

::: data/theme/gnome-shell.css
@@ +454,2 @@
 .dash-search-section-header {
+    padding: 4px 0px;

It looks like this will generally change the padding on all the search sections. I'll trust you on the change :-) ... no easy way to check if it makes sense or not.

::: js/ui/appDisplay.js
@@ +27,3 @@
 const APPICON_SIZE = 48;
 const WELL_MAX_COLUMNS = 8;
+const WELL_MAX_ROWS = 1;

Should this really be called WELL_MAX_ROWS if it's a maximum only for the search WellGrid and not the other two WellGrid instances in this file?

@@ +230,3 @@
         this.actor = new St.Bin({ name: 'dashAppSearchResults',
                                   x_align: St.Align.START });
+        this.actor.child = this._grid.actor;

Hmm, rather if this used set_child().. I somehow don't really like modifying the actor hierarchy by changing a property - that seems inconsistent with what we do for St.BoxLayout, or whatever, and specific to bins.

@@ +852,3 @@
+
+            if (childBox.y2 > availHeight) {
+                this._grid.set_skip_paint(children[i], true);

By going off of y here, you might end up with more than rowLimit rows if the actor is allocated a larger area than it requests. This seems potentially broken to me - e.g., with the search it adds always 8 items because it has a max of 8 columns and 1 row. But what if it got 5 columns and 2 rows? Then we'd have 5 on the first row and 3 on the second.

It seems likely better to just keep track of what row we are allocating and skip_paint everything after rowLimit rows.

Created attachment 170296 [details] [review]
Use WellGrid for application results

(In reply to comment #4)
> ::: data/theme/gnome-shell.css
> @@ +454,2 @@
>  .dash-search-section-header {
> +    padding: 4px 0px;
> 
> It looks like this will generally change the padding on all the search
> sections. I'll trust you on the change :-) ... no easy way to check if it makes
> sense or not.

It's obviously not required for the patch, but it makes the search display more consistent with the "normal" dash. The change is (in my opinion) most noticable with the "Application" header - there was a jump when switching between search and app well header (see also bug 612348).

Review of attachment 170296 [details] [review]:

Some confusing counting, looks fine to commit with that fixed.

::: js/ui/appDisplay.js
@@ +828,3 @@
         for (let i = 0; i < children.length; i++) {
+            if (columnIndex == 0)
+                rowIndex++;

this makes rowIndex start at 1 while columnIndex starts at 0 - I'd expect the increment to be in the:

 if (columnIndex == nColumns) {

block.

@@ +850,3 @@
             childBox.y2 = childBox.y1 + height;
+
+            if (this._rowLimit && this._rowLimit < rowIndex) {

So this would be <=, or clearer

 rowIndex >= this._rowLimit

Attachment 170296 [details] pushed as 1bd0ad1 - Use WellGrid for application results