Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2165483002:
Fill the launcher with all found apps. (Closed)
DescriptionFill the launcher with all found apps.
Before, only 4 were shown, even though there was space for more.
It could confuse users, that there are no other apps matching the
criteria.
This CL increases this limit to 8, which fill the available space.
TEST=Tested manually on Samus.
BUG=629411
Committed: https://crrev.com/e9b526245329d9f5106ddcf9c7791a17b2d0650b
Cr-Commit-Position: refs/heads/master@{#407078}
Patch Set 1 #Patch Set 2 : Fixed. #
Total comments: 6
Patch Set 3 : Reverted history.cc #Patch Set 4 : Spacing -> padding, changed layout to GridLayout. #
Total comments: 2
Patch Set 5 : Removed consts. #
Messages
Total messages: 40 (23 generated)
The CQ bit was checked by mtomasz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by mtomasz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fill the launcher with all found apps. Before, only 4 were shown, even though there was space for more. It could confuse users, that there are no other apps matching the criteria. This CL increases this limit to 8, which fill the available space. TEST=Tested manually on Samus. BUG=629411 ========== to ========== Fill the launcher with all found apps. Before, only 4 were shown, even though there was space for more. It could confuse users, that there are no other apps matching the criteria. This CL increases this limit to 8, which fill the available space. TEST=Tested manually on Samus. BUG=629411 ==========
mtomasz@chromium.org changed reviewers: + mgiuca@chromium.org
@mgiuca: PTAL. Do we need to confirm it with UX? Thanks! https://codereview.chromium.org/2165483002/diff/20001/ui/app_list/search/hist... File ui/app_list/search/history.cc (right): https://codereview.chromium.org/2165483002/diff/20001/ui/app_list/search/hist... ui/app_list/search/history.cc:31: const size_t kMaxSecondaryQueries = 8; (Not sure if this change is necessary.)
Yes I think we should confirm with UX. Thanks for looking at this. https://codereview.chromium.org/2165483002/diff/20001/ui/app_list/search/hist... File ui/app_list/search/history.cc (right): https://codereview.chromium.org/2165483002/diff/20001/ui/app_list/search/hist... ui/app_list/search/history.cc:31: const size_t kMaxSecondaryQueries = 8; On 2016/07/19 07:07:56, mtomasz wrote: > (Not sure if this change is necessary.) I don't know what this is and I don't think it's relevant. If you change it, you should get xiyuan@ to look. https://codereview.chromium.org/2165483002/diff/20001/ui/app_list/views/searc... File ui/app_list/views/search_result_tile_item_list_view.cc (right): https://codereview.chromium.org/2165483002/diff/20001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view.cc:23: const int kTileSpacing = 2; What does the tile spacing change do? Is this changing the actual spacing of the tiles? Making these 2 changes should go through UI review as we had very precise specs when we implemented it (ask bettes@). I'm not sure why we had a max of 4 apps (that seems like an oversight), when we had a limit of 5 here. I'm OK with increasing the limit to 8.
mtomasz@chromium.org changed reviewers: + xiyuan@chromium.org
@xiyuan: PTAL at ui/app_list/search/history.cc. Thanks! https://codereview.chromium.org/2165483002/diff/20001/ui/app_list/views/searc... File ui/app_list/views/search_result_tile_item_list_view.cc (right): https://codereview.chromium.org/2165483002/diff/20001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view.cc:23: const int kTileSpacing = 2; On 2016/07/19 07:29:10, Matt Giuca wrote: > What does the tile spacing change do? Is this changing the actual spacing of the > tiles? Making these 2 changes should go through UI review as we had very precise > specs when we implemented it (ask bettes@). > > I'm not sure why we had a max of 4 apps (that seems like an oversight), when we > had a limit of 5 here. I'm OK with increasing the limit to 8. Yes, it changes the specs for tiles, so 8 tiles match the container nicely (without leaving much space). I looked at the Ares mocks, and I couldn't find detailed specs for the app search list. Let's confirm with @bettes.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2165483002/diff/20001/ui/app_list/search/hist... File ui/app_list/search/history.cc (right): https://codereview.chromium.org/2165483002/diff/20001/ui/app_list/search/hist... ui/app_list/search/history.cc:31: const size_t kMaxSecondaryQueries = 8; On 2016/07/19 07:29:10, Matt Giuca wrote: > On 2016/07/19 07:07:56, mtomasz wrote: > > (Not sure if this change is necessary.) > > I don't know what this is and I don't think it's relevant. If you change it, you > should get xiyuan@ to look. Matt is right that this is not relevant to what this CL tries to improve. Applist search history remembers query -> launched-result mapping. It is a one-to-many mapping. The primary one get boost score and we remember additional launched results using the same query as secondary to get a less boost score. And if a secondary result is launched using the query twice in a row, it is promoted to primary. See [1]. This number must be greater than 2. 5 is somewhat chosen arbitrarily. Changing to 8 probably will not get us more benefits but will increase the history data size. [1]: https://cs.chromium.org/chromium/src/ui/app_list/search/history_data.h?rcl=14...
https://codereview.chromium.org/2165483002/diff/20001/ui/app_list/search/hist... File ui/app_list/search/history.cc (right): https://codereview.chromium.org/2165483002/diff/20001/ui/app_list/search/hist... ui/app_list/search/history.cc:31: const size_t kMaxSecondaryQueries = 8; On 2016/07/19 15:22:55, xiyuan wrote: > On 2016/07/19 07:29:10, Matt Giuca wrote: > > On 2016/07/19 07:07:56, mtomasz wrote: > > > (Not sure if this change is necessary.) > > > > I don't know what this is and I don't think it's relevant. If you change it, > you > > should get xiyuan@ to look. > > Matt is right that this is not relevant to what this CL tries to improve. > Applist search history remembers query -> launched-result mapping. It is a > one-to-many mapping. The primary one get boost score and we remember additional > launched results using the same query as secondary to get a less boost score. > And if a secondary result is launched using the query twice in a row, it is > promoted to primary. See [1]. This number must be greater than 2. 5 is somewhat > chosen arbitrarily. Changing to 8 probably will not get us more benefits but > will increase the history data size. > > [1]: > https://cs.chromium.org/chromium/src/ui/app_list/search/history_data.h?rcl=14... > Removed. Done.
lgtm (after offline discussion with bettes@; he said to go 8 and reduce the margins).
The CQ bit was checked by mtomasz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
@mgiuca: PTAL one more time, as the code changed significantly. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mgiuca@chromium.org changed reviewers: + calamity@chromium.org
Hmm, hang on, I'm not quite clear why this is changing from BoxLayout to GridLayout. (I don't fully understand but in my mind I equate BoxLayout = good, GridLayout = bad.) Adding calamity to look over it. Why can't you just change the padding to 0 from within a BoxLayout?
On 2016/07/21 04:27:39, Matt Giuca wrote: > Hmm, hang on, I'm not quite clear why this is changing from BoxLayout to > GridLayout. (I don't fully understand but in my mind I equate BoxLayout = good, > GridLayout = bad.) > > Adding calamity to look over it. Why can't you just change the padding to 0 from > within a BoxLayout? If we make padding 0, then we'll have remaining unused space on the right side. The idea here was divide the available space into 8 equal tiles, so if we show 8, then they fit perfectly. GridLayout lets us do it easily (without manual calculations).
https://codereview.chromium.org/2165483002/diff/60001/ui/app_list/views/searc... File ui/app_list/views/search_result_tile_item_list_view.cc (right): https://codereview.chromium.org/2165483002/diff/60001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view.cc:33: views::GridLayout* const layout = new views::GridLayout(this); nit: I think these consts are unnecessary (although correct, it's generally overkill to add const to local variables that happen not to be modified).
https://codereview.chromium.org/2165483002/diff/60001/ui/app_list/views/searc... File ui/app_list/views/search_result_tile_item_list_view.cc (right): https://codereview.chromium.org/2165483002/diff/60001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view.cc:33: views::GridLayout* const layout = new views::GridLayout(this); On 2016/07/21 04:36:08, Matt Giuca wrote: > nit: I think these consts are unnecessary (although correct, it's generally > overkill to add const to local variables that happen not to be modified). I usually add them, as it's always some extra compile time error check against some accidental variable reassign. It also tells future engineers trying to understand this code that this variable is not reassigned later, so the value stays constant throughout the entire scope. Having said that, I don't have strong feelings about it. Removed. Done.
mtomasz@chromium.org changed reviewers: - xiyuan@chromium.org
@mgiuca: Is it ready to commit? Or, do you want calamity@ to take a look?
The CQ bit was checked by mtomasz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I see. If BoxLayout supported laying out children that weren't visible, you could just set the default flex to 1 to achieve the same effect. Unfortunately, I never got around to landing that feature so lgtm w/ GridLayout it is.
On 2016/07/22 03:42:31, calamity wrote: > I see. If BoxLayout supported laying out children that weren't visible, you > could just set the default flex to 1 to achieve the same effect. Unfortunately, > I never got around to landing that feature so lgtm w/ GridLayout it is. That's sad. Is there a CL we can land or does it need more work? In the interim, slgtm. If we fix BoxLayout let's go back to that.
The CQ bit was checked by mtomasz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mgiuca@chromium.org Link to the patchset: https://codereview.chromium.org/2165483002/#ps80001 (title: "Removed consts.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fill the launcher with all found apps. Before, only 4 were shown, even though there was space for more. It could confuse users, that there are no other apps matching the criteria. This CL increases this limit to 8, which fill the available space. TEST=Tested manually on Samus. BUG=629411 ========== to ========== Fill the launcher with all found apps. Before, only 4 were shown, even though there was space for more. It could confuse users, that there are no other apps matching the criteria. This CL increases this limit to 8, which fill the available space. TEST=Tested manually on Samus. BUG=629411 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fill the launcher with all found apps. Before, only 4 were shown, even though there was space for more. It could confuse users, that there are no other apps matching the criteria. This CL increases this limit to 8, which fill the available space. TEST=Tested manually on Samus. BUG=629411 ========== to ========== Fill the launcher with all found apps. Before, only 4 were shown, even though there was space for more. It could confuse users, that there are no other apps matching the criteria. This CL increases this limit to 8, which fill the available space. TEST=Tested manually on Samus. BUG=629411 Committed: https://crrev.com/e9b526245329d9f5106ddcf9c7791a17b2d0650b Cr-Commit-Position: refs/heads/master@{#407078} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e9b526245329d9f5106ddcf9c7791a17b2d0650b Cr-Commit-Position: refs/heads/master@{#407078} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
