Hide Launcher app search box when empty

Hide Launcher app search box when empty

BUG=661380
TEST=Open launcher, type a search that returns no app results.  The
horizontal box that usually contains app results should be hidden.

Committed: https://crrev.com/32247f5271fd583a683bc8dc2e4c07ce3cf72b85
Cr-Commit-Position: refs/heads/master@{#433686}

[Greg Levin, 2016-11-03 16:32:09]

glevin@chromium.org changed reviewers:
+ calamity@chromium.org, mgiuca@chromium.org, mtomasz@chromium.org

[Greg Levin, 2016-11-03 16:32:09]

Hey guys.  I'm proposing a partial revert of
https://codereview.chromium.org/2165483002 to fix this bug.  There's a
conversation in that CL about why the switch from BoxLayout to GridLayout was
needed (specifically around Comment #32
(https://codereview.chromium.org/2165483002#msg32), but I can't say that I
understand it.  I've added some notes and screenshots to Issue 661380; as I say
there, I can't see any negative effect from switching back to BoxLayout, but I
may not be trying the right test cases.  Could you let me know if I'm missing
something, or if this fix looks okay?  Thanks!

[calamity, 2016-11-04 00:46:55]

On 2016/11/03 16:32:09, Greg Levin wrote:
> Hey guys.  I'm proposing a partial revert of
> https://codereview.chromium.org/2165483002 to fix this bug.  There's a
> conversation in that CL about why the switch from BoxLayout to GridLayout was
> needed (specifically around Comment #32
> (https://codereview.chromium.org/2165483002#msg32), but I can't say that I
> understand it.  I've added some notes and screenshots to Issue 661380; as I
say
> there, I can't see any negative effect from switching back to BoxLayout, but I
> may not be trying the right test cases.  Could you let me know if I'm missing
> something, or if this fix looks okay?  Thanks!

https://codereview.chromium.org/2165483002#msg23 explains the desired behavior.
BoxLayout would be fine, but is probably less robust to slight changes in the
width of the search box container. I would say I actually prefer the BoxLayout
since it has padding and makes the tile text look less squished.

lgtm.

[mtomasz, 2016-11-04 00:55:39]

On 2016/11/04 00:46:55, calamity wrote:
> On 2016/11/03 16:32:09, Greg Levin wrote:
> > Hey guys.  I'm proposing a partial revert of
> > https://codereview.chromium.org/2165483002 to fix this bug.  There's a
> > conversation in that CL about why the switch from BoxLayout to GridLayout
was
> > needed (specifically around Comment #32
> > (https://codereview.chromium.org/2165483002#msg32), but I can't say that I
> > understand it.  I've added some notes and screenshots to Issue 661380; as I
> say
> > there, I can't see any negative effect from switching back to BoxLayout, but
I
> > may not be trying the right test cases.  Could you let me know if I'm
missing
> > something, or if this fix looks okay?  Thanks!
> 
> https://codereview.chromium.org/2165483002#msg23 explains the desired
behavior.
> BoxLayout would be fine, but is probably less robust to slight changes in the
> width of the search box container. I would say I actually prefer the BoxLayout
> since it has padding and makes the tile text look less squished.
> 
> lgtm.

Did the UI change? Does it work well when filled with 8 apps, and few apps? If
so, then could you share screenshots? When working on the original CL I tried
hard to make it work with box layout, but it didn't look good, so went with
grid.

[Greg Levin, 2016-11-16 21:46:41]

> BoxLayout would be fine, but is probably less robust to slight changes in the
> width of the search box container. I would say I actually prefer the BoxLayout
> since it has padding and makes the tile text look less squished.

Yes, without more experience with these, I would guess that BoxLayout would not
adjust as well to a resize of the launcher.  But at the moment, the icon spacing
is identical between Box and Grid layouts with any number of icons (up to 8).

> Did the UI change? Does it work well when filled with 8 apps, and few apps? If
> so, then could you share screenshots? When working on the original CL I tried
> hard to make it work with box layout, but it didn't look good, so went with
> grid.

I included screenshots in
https://bugs.chromium.org/p/chromium/issues/detail?id=661380#c5.  Except for the
barely perceptible whitespaces between selected and hovered items (noted in the
comment), the UI is identical, as far as I can tell.  I've looked at a varying
number of results (from 1 to 8), and seen no difference.  Though again, there
may be something I'm not thinking of to check.

[mtomasz, 2016-11-21 02:46:48]

Thanks for the screenshots. LGTM.

[Greg Levin, 2016-11-21 18:14:01]

The CQ bit was checked by glevin@chromium.org

[commit-bot: I haz the power, 2016-11-21 18:14:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-21 20:40:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-21 20:40:39]

Failed to apply patch for
ui/app_list/views/search_result_tile_item_list_view.cc:
While running git apply --index -p1;
  error: patch failed: ui/app_list/views/search_result_tile_item_list_view.cc:46
  error: ui/app_list/views/search_result_tile_item_list_view.cc: patch does not
apply

Patch:       ui/app_list/views/search_result_tile_item_list_view.cc
Index: ui/app_list/views/search_result_tile_item_list_view.cc
diff --git a/ui/app_list/views/search_result_tile_item_list_view.cc
b/ui/app_list/views/search_result_tile_item_list_view.cc
index
2d44e4bdff2d0c1bfc467d9bcb803955581cff37..792749ce8b63f573c515c50a65d3d3e822484128
100644
--- a/ui/app_list/views/search_result_tile_item_list_view.cc
+++ b/ui/app_list/views/search_result_tile_item_list_view.cc
@@ -14,12 +14,14 @@
 #include "ui/views/background.h"
 #include "ui/views/border.h"
 #include "ui/views/controls/textfield/textfield.h"
-#include "ui/views/layout/grid_layout.h"
+#include "ui/views/layout/box_layout.h"
 
 namespace {
 
 // Layout constants.
 const size_t kNumSearchResultTiles = 8;
+const int kHorizontalBorderSpacing = 1;
+const int kBetweenTileSpacing = 2;
 const int kTopBottomPadding = 8;
 
 }  // namespace
@@ -30,14 +32,9 @@ SearchResultTileItemListView::SearchResultTileItemListView(
     views::Textfield* search_box,
     AppListViewDelegate* view_delegate)
     : search_box_(search_box) {
-  views::GridLayout* layout = new views::GridLayout(this);
-  SetLayoutManager(layout);
-  views::ColumnSet* column_set = layout->AddColumnSet(0);
-  for (size_t i = 0; i < kNumSearchResultTiles; ++i) {
-    column_set->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 1,
-                          views::GridLayout::USE_PREF, 0, 0);
-  }
-  layout->StartRow(0, 0);
+  SetLayoutManager(new views::BoxLayout(views::BoxLayout::kHorizontal,
+                                        kHorizontalBorderSpacing, 0,
+                                        kBetweenTileSpacing));
 
   for (size_t i = 0; i < kNumSearchResultTiles; ++i) {
     SearchResultTileItemView* tile_item =
@@ -46,7 +43,7 @@ SearchResultTileItemListView::SearchResultTileItemListView(
     tile_item->SetBorder(views::Border::CreateEmptyBorder(
         kTopBottomPadding, 0, kTopBottomPadding, 0));
     tile_views_.push_back(tile_item);
-    layout->AddView(tile_item);
+    AddChildView(tile_item);
   }
 }

[Greg Levin, 2016-11-21 22:26:39]

The CQ bit was checked by glevin@chromium.org

[Greg Levin, 2016-11-21 22:26:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
mtomasz@chromium.org, calamity@chromium.org
Link to the patchset: https://codereview.chromium.org/2473033002/#ps20001
(title: "Merge")

[commit-bot: I haz the power, 2016-11-21 22:27:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-21 23:19:10]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1479767199045620,
"parent_rev": "de26a9c2a103c4f7908bd886932eda73c893ff37", "commit_rev":
"6cff97252ce060f3ce899aba0f87643582235f5a"}

[commit-bot: I haz the power, 2016-11-21 23:19:37]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-21 23:26:54]

Description was changed from

==========
Hide Launcher app search box when empty

BUG=661380
TEST=Open launcher, type a search that returns no app results.  The
horizontal box that usually contains app results should be hidden.
==========

to

==========
Hide Launcher app search box when empty

BUG=661380
TEST=Open launcher, type a search that returns no app results.  The
horizontal box that usually contains app results should be hidden.

Committed: https://crrev.com/32247f5271fd583a683bc8dc2e4c07ce3cf72b85
Cr-Commit-Position: refs/heads/master@{#433686}
==========

[commit-bot: I haz the power, 2016-11-21 23:26:55]

Patchset 2 (id:??) landed as
https://crrev.com/32247f5271fd583a683bc8dc2e4c07ce3cf72b85
Cr-Commit-Position: refs/heads/master@{#433686}