Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2856253002:
[Android Omnibox] Remove doubling of height for answers (Closed)
|
Created:
3 years, 7 months ago by Kevin Bailey Modified:
3 years, 7 months ago CC:
chromium-reviews, jdonnelly+watch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Android Omnibox] Remove doubling of height for answers
We were adding twice the height necessary when an answer in suggest
was present, which created a large empty space at the end of the
suggestions. This changes it to adding custom "definition" line
height.
BUG=714042
Review-Url: https://codereview.chromium.org/2856253002
Cr-Commit-Position: refs/heads/master@{#469427}
Committed: https://chromium.googlesource.com/chromium/src/+/2c16490e12bda68c5b784908105de4ec8edb3777
Patch Set 1 #
Total comments: 3
Patch Set 2 : Increase padding with new constant #
Messages
Total messages: 22 (11 generated)
Description was changed from ========== [Android Omnibox] Remove doubling of height for answers We were adding twice the height necessary when an answer in suggest was present, which created a large empty space at the end of the suggestions. This changes it to adding a single answer line height. BUG=714042 ========== to ========== [Android Omnibox] Remove doubling of height for answers We were adding twice the height necessary when an answer in suggest was present, which created a large empty space at the end of the suggestions. This changes it to adding a single answer line height. BUG=714042 ==========
krb@chromium.org changed reviewers: + tedchoc@chromium.org
Hi Ted, If you think the image in the bug looks better, here's the change.
I don't know if it looks better to me. Now we are potentially cutting off (and maybe fully hiding in some cases) a suggestion instead of having too much space. If folk feel strongly about this, then I'll go with it, but I don't see this as an obvious win. https://codereview.chromium.org/2856253002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java (left): https://codereview.chromium.org/2856253002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:609: idealListSize += mSuggestionAnswerHeight * 2; instead of a 2x multiplier, could we add some small padding (5-10dp) for each additional line over one line to make this look better? It wouldn't account for small phones or landscape where a single line could be much shorter or longer than expected, but in both those situations the visible real estate is much shorter and likely wouldn't show all suggestions anywhere. Tablets could likely be negatively effective in the same way as before where there is too much space, but I wonder if there is a better short term solution until we fully account for their actual heights.
jdonnelly@chromium.org changed reviewers: + jdonnelly@chromium.org
https://codereview.chromium.org/2856253002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java (left): https://codereview.chromium.org/2856253002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:609: idealListSize += mSuggestionAnswerHeight * 2; On 2017/05/03 23:50:16, Ted C wrote: > instead of a 2x multiplier, could we add some small padding (5-10dp) for each > additional line over one line to make this look better? It wouldn't account for > small phones or landscape where a single line could be much shorter or longer > than expected, but in both those situations the visible real estate is much > shorter and likely wouldn't show all suggestions anywhere. Tablets could likely > be negatively effective in the same way as before where there is too much space, > but I wonder if there is a better short term solution until we fully account for > their actual heights. This proposal sgtm.
https://codereview.chromium.org/2856253002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java (left): https://codereview.chromium.org/2856253002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:609: idealListSize += mSuggestionAnswerHeight * 2; On 2017/05/04 13:55:25, Justin Donnelly wrote: > On 2017/05/03 23:50:16, Ted C wrote: > > instead of a 2x multiplier, could we add some small padding (5-10dp) for each > > additional line over one line to make this look better? It wouldn't account > for > > small phones or landscape where a single line could be much shorter or longer > > than expected, but in both those situations the visible real estate is much > > shorter and likely wouldn't show all suggestions anywhere. Tablets could > likely > > be negatively effective in the same way as before where there is too much > space, > > but I wonder if there is a better short term solution until we fully account > for > > their actual heights. > > This proposal sgtm. Just so everyone is on the same page, what has to happen here is, first we need to calculate how many lines the definition occupies given the width. (Over in Views-land, we allocate a separate RenderText to calculate this. I haven't yet spotted how to do this in Android; something along the lines of creating a non-visible window and rendering there, I imagine.) Once we get the number of lines (max 3) we can calculate the additional space needed.
On 2017/05/04 15:49:50, Kevin Bailey wrote: > https://codereview.chromium.org/2856253002/diff/1/chrome/android/java/src/org... > File > chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java > (left): > > https://codereview.chromium.org/2856253002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:609: > idealListSize += mSuggestionAnswerHeight * 2; > On 2017/05/04 13:55:25, Justin Donnelly wrote: > > On 2017/05/03 23:50:16, Ted C wrote: > > > instead of a 2x multiplier, could we add some small padding (5-10dp) for > each > > > additional line over one line to make this look better? It wouldn't account > > for > > > small phones or landscape where a single line could be much shorter or > longer > > > than expected, but in both those situations the visible real estate is much > > > shorter and likely wouldn't show all suggestions anywhere. Tablets could > > likely > > > be negatively effective in the same way as before where there is too much > > space, > > > but I wonder if there is a better short term solution until we fully account > > for > > > their actual heights. > > > > This proposal sgtm. > > Just so everyone is on the same page, what has to happen here is, first we need > to calculate how many lines the definition occupies given the width. (Over in > Views-land, we allocate a separate RenderText to calculate this. I haven't yet > spotted how to do this in Android; something along the lines of creating a > non-visible window and rendering there, I imagine.) Once we get the number of > lines (max 3) we can calculate the additional space needed. I was thinking something much more simple to start. Something like: idealListSize += mSuggestionAnswerHeight + (5dp * num - 1); If we wanted to be smarter around actual width, I think we could look at https://developer.android.com/reference/android/text/StaticLayout.html to measure the number of lines and then the width of them. But yeah, I think that might be overkill to start as we can try to come up with a solution that is a bit better than what we currently have.
On 2017/05/04 16:34:07, Ted C wrote: > On 2017/05/04 15:49:50, Kevin Bailey wrote: > > > https://codereview.chromium.org/2856253002/diff/1/chrome/android/java/src/org... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java > > (left): > > > > > https://codereview.chromium.org/2856253002/diff/1/chrome/android/java/src/org... > > > chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:609: > > idealListSize += mSuggestionAnswerHeight * 2; > > On 2017/05/04 13:55:25, Justin Donnelly wrote: > > > On 2017/05/03 23:50:16, Ted C wrote: > > > > instead of a 2x multiplier, could we add some small padding (5-10dp) for > > each > > > > additional line over one line to make this look better? It wouldn't > account > > > for > > > > small phones or landscape where a single line could be much shorter or > > longer > > > > than expected, but in both those situations the visible real estate is > much > > > > shorter and likely wouldn't show all suggestions anywhere. Tablets could > > > likely > > > > be negatively effective in the same way as before where there is too much > > > space, > > > > but I wonder if there is a better short term solution until we fully > account > > > for > > > > their actual heights. > > > > > > This proposal sgtm. > > > > Just so everyone is on the same page, what has to happen here is, first we > need > > to calculate how many lines the definition occupies given the width. (Over in > > Views-land, we allocate a separate RenderText to calculate this. I haven't yet > > spotted how to do this in Android; something along the lines of creating a > > non-visible window and rendering there, I imagine.) Once we get the number of > > lines (max 3) we can calculate the additional space needed. > > I was thinking something much more simple to start. > > Something like: > > idealListSize += mSuggestionAnswerHeight + (5dp * num - 1); > > If we wanted to be smarter around actual width, I think we could look at > https://developer.android.com/reference/android/text/StaticLayout.html to > measure > the number of lines and then the width of them. But yeah, I think that > might be overkill to start as we can try to come up with a solution that is > a bit better than what we currently have. 'num' is always 3. (GWS doesn't know how wide your screen is or the fonts used.) If we don't mind a constant here, I can certainly put one, but if we want the actual height, we'll have to pull some rendering trick.
On 2017/05/04 16:46:38, Kevin Bailey wrote: > On 2017/05/04 16:34:07, Ted C wrote: > > On 2017/05/04 15:49:50, Kevin Bailey wrote: > > > > > > https://codereview.chromium.org/2856253002/diff/1/chrome/android/java/src/org... > > > File > > > > > > chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java > > > (left): > > > > > > > > > https://codereview.chromium.org/2856253002/diff/1/chrome/android/java/src/org... > > > > > > chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:609: > > > idealListSize += mSuggestionAnswerHeight * 2; > > > On 2017/05/04 13:55:25, Justin Donnelly wrote: > > > > On 2017/05/03 23:50:16, Ted C wrote: > > > > > instead of a 2x multiplier, could we add some small padding (5-10dp) for > > > each > > > > > additional line over one line to make this look better? It wouldn't > > account > > > > for > > > > > small phones or landscape where a single line could be much shorter or > > > longer > > > > > than expected, but in both those situations the visible real estate is > > much > > > > > shorter and likely wouldn't show all suggestions anywhere. Tablets > could > > > > likely > > > > > be negatively effective in the same way as before where there is too > much > > > > space, > > > > > but I wonder if there is a better short term solution until we fully > > account > > > > for > > > > > their actual heights. > > > > > > > > This proposal sgtm. > > > > > > Just so everyone is on the same page, what has to happen here is, first we > > need > > > to calculate how many lines the definition occupies given the width. (Over > in > > > Views-land, we allocate a separate RenderText to calculate this. I haven't > yet > > > spotted how to do this in Android; something along the lines of creating a > > > non-visible window and rendering there, I imagine.) Once we get the number > of > > > lines (max 3) we can calculate the additional space needed. > > > > I was thinking something much more simple to start. > > > > Something like: > > > > idealListSize += mSuggestionAnswerHeight + (5dp * num - 1); > > > > If we wanted to be smarter around actual width, I think we could look at > > https://developer.android.com/reference/android/text/StaticLayout.html to > > measure > > the number of lines and then the width of them. But yeah, I think that > > might be overkill to start as we can try to come up with a solution that is > > a bit better than what we currently have. > > 'num' is always 3. (GWS doesn't know how wide your screen is or the fonts > used.) If we don't mind a constant here, I can certainly put one, but if > we want the actual height, we'll have to pull some rendering trick. Ha :-P...ok then. Maybe we just add a constant for now to make a bit better. I do think we should figure out a path to calculate the actual height, but that will be finicky and take some time to make sure we share as much as possible from SuggestionView. One thing we could look at is picking a conservative initial value but then recalculate the height once the list displays itself (where we can walk through views and see their actual measured heights).
On 2017/05/04 16:52:58, Ted C wrote: > On 2017/05/04 16:46:38, Kevin Bailey wrote: > > On 2017/05/04 16:34:07, Ted C wrote: > > > On 2017/05/04 15:49:50, Kevin Bailey wrote: > > > > > > > > > > https://codereview.chromium.org/2856253002/diff/1/chrome/android/java/src/org... > > > > File > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java > > > > (left): > > > > > > > > > > > > > > https://codereview.chromium.org/2856253002/diff/1/chrome/android/java/src/org... > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:609: > > > > idealListSize += mSuggestionAnswerHeight * 2; > > > > On 2017/05/04 13:55:25, Justin Donnelly wrote: > > > > > On 2017/05/03 23:50:16, Ted C wrote: > > > > > > instead of a 2x multiplier, could we add some small padding (5-10dp) > for > > > > each > > > > > > additional line over one line to make this look better? It wouldn't > > > account > > > > > for > > > > > > small phones or landscape where a single line could be much shorter or > > > > longer > > > > > > than expected, but in both those situations the visible real estate is > > > much > > > > > > shorter and likely wouldn't show all suggestions anywhere. Tablets > > could > > > > > likely > > > > > > be negatively effective in the same way as before where there is too > > much > > > > > space, > > > > > > but I wonder if there is a better short term solution until we fully > > > account > > > > > for > > > > > > their actual heights. > > > > > > > > > > This proposal sgtm. > > > > > > > > Just so everyone is on the same page, what has to happen here is, first we > > > need > > > > to calculate how many lines the definition occupies given the width. (Over > > in > > > > Views-land, we allocate a separate RenderText to calculate this. I haven't > > yet > > > > spotted how to do this in Android; something along the lines of creating a > > > > non-visible window and rendering there, I imagine.) Once we get the number > > of > > > > lines (max 3) we can calculate the additional space needed. > > > > > > I was thinking something much more simple to start. > > > > > > Something like: > > > > > > idealListSize += mSuggestionAnswerHeight + (5dp * num - 1); > > > > > > If we wanted to be smarter around actual width, I think we could look at > > > https://developer.android.com/reference/android/text/StaticLayout.html to > > > measure > > > the number of lines and then the width of them. But yeah, I think that > > > might be overkill to start as we can try to come up with a solution that is > > > a bit better than what we currently have. > > > > 'num' is always 3. (GWS doesn't know how wide your screen is or the fonts > > used.) If we don't mind a constant here, I can certainly put one, but if > > we want the actual height, we'll have to pull some rendering trick. > > Ha :-P...ok then. > > Maybe we just add a constant for now to make a bit better. I do think we > should figure out a path to calculate the actual height, but that will be > finicky and take some time to make sure we share as much as possible from > SuggestionView. One thing we could look at is picking a conservative > initial value but then recalculate the height once the list displays itself > (where we can walk through views and see their actual measured heights). Ok, tried to hit compromise between phone and tablet.
lgtm
Description was changed from ========== [Android Omnibox] Remove doubling of height for answers We were adding twice the height necessary when an answer in suggest was present, which created a large empty space at the end of the suggestions. This changes it to adding a single answer line height. BUG=714042 ========== to ========== [Android Omnibox] Remove doubling of height for answers We were adding twice the height necessary when an answer in suggest was present, which created a large empty space at the end of the suggestions. This changes it to adding custom "definition" line height. BUG=714042 ==========
The CQ bit was checked by krb@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.
The CQ bit was checked by krb@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493926372724420,
"parent_rev": "2277d587c39624d35fb97ea65cc610dc29a9306e", "commit_rev":
"2c16490e12bda68c5b784908105de4ec8edb3777"}
Message was sent while issue was closed.
Description was changed from ========== [Android Omnibox] Remove doubling of height for answers We were adding twice the height necessary when an answer in suggest was present, which created a large empty space at the end of the suggestions. This changes it to adding custom "definition" line height. BUG=714042 ========== to ========== [Android Omnibox] Remove doubling of height for answers We were adding twice the height necessary when an answer in suggest was present, which created a large empty space at the end of the suggestions. This changes it to adding custom "definition" line height. BUG=714042 Review-Url: https://codereview.chromium.org/2856253002 Cr-Commit-Position: refs/heads/master@{#469427} Committed: https://chromium.googlesource.com/chromium/src/+/2c16490e12bda68c5b784908105d... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/2c16490e12bda68c5b784908105d... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
