Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1023993003:
[AiS] Added option to put a space between answer values (Closed)
|
Created:
5 years, 9 months ago by dschuyler Modified:
5 years, 9 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, tfarina, James Su, groby-ooo-7-16 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[AiS] Added option to put a space between answer values
The answers in suggest doesn't put a space between, for example
+ the degrees Fahrenheit and the day of the week
+ the stock price and the stock delta (+/- value)
This CL puts a space between those values, which is more readable for the user.
BUG=
Committed: https://crrev.com/ea24fe817ef9eedb7a810dc0b9eb46fa66b5e059
Cr-Commit-Position: refs/heads/master@{#322071}
Patch Set 1 #Patch Set 2 : changed change list parent #
Total comments: 2
Patch Set 3 : removed default param #
Total comments: 6
Patch Set 4 : Changed args for AppendAnswerText #
Total comments: 8
Patch Set 5 : Changes for review nits #Patch Set 6 : Better range checking on GetTextStyle #
Total comments: 4
Patch Set 7 : Changes for review nits #
Total comments: 1
Messages
Total messages: 22 (2 generated)
dschuyler@chromium.org changed reviewers: + pkasting@chromium.org
How did Android deal with this? Is this an indicator that the designers here expected these strings to be laid out as separate entities with padding pixels between them (much like was described earlier for RTL)? If so I think it might be better to go ahead and move to that now. This would address the concern here and possibly also address the concern with multiple font sizes in a RenderText (plus allow you to remove the AppendText() functionality you added). https://codereview.chromium.org/1023993003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.h (right): https://codereview.chromium.org/1023993003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:164: bool prefix_space = false); Default args are (generally) banned by the Google style guide.
I've just created a bug, 469362 to track what *should* be done. In the short term, I feel that the space will be better for users (those using RTL as well) to have a space between the pieces of text. I agree with your suggestions and I intend to work toward that goal. I'd also like AiS to look as good as it may in the near term (and improve from here). https://codereview.chromium.org/1023993003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.h (right): https://codereview.chromium.org/1023993003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:164: bool prefix_space = false); On 2015/03/20 22:21:36, Peter Kasting wrote: > Default args are (generally) banned by the Google style guide. Done.
You didn't answer how Android dealt with this. On 2015/03/20 23:56:43, dschuyler wrote: > I've just created a bug, 469362 to track what *should* be done. In the short > term, I feel that the space will be better for users (those using RTL as well) > to have a space between the pieces of text. > > I agree with your suggestions and I intend to work toward that goal. I'd also > like AiS to look as good as it may in the near term (and improve from here). Why is it so important to get this fix in rather than just do the other fix? It seems like using separate RenderTexts ought not to be more than a day of work. Except for extremely unusual cases, the rule on Chromium changes is that we take the time to do the right thing (or what we think is the right thing) up front even if it takes a little longer, especially when the feature isn't even being used by any users yet. The exception here would be if we have reason to believe that using multiple separate pieces is not going to be what we're going to want to do in the long run, in which case it's potentially a waste to work on it until we know that for sure.
On 2015/03/21 00:03:24, Peter Kasting wrote: > You didn't answer how Android dealt with this. > > On 2015/03/20 23:56:43, dschuyler wrote: > > I've just created a bug, 469362 to track what *should* be done. In the short > > term, I feel that the space will be better for users (those using RTL as well) > > to have a space between the pieces of text. > > > > I agree with your suggestions and I intend to work toward that goal. I'd also > > like AiS to look as good as it may in the near term (and improve from here). > > Why is it so important to get this fix in rather than just do the other fix? It > seems like using separate RenderTexts ought not to be more than a day of work. > > Except for extremely unusual cases, the rule on Chromium changes is that we take > the time to do the right thing (or what we think is the right thing) up front > even if it takes a little longer, especially when the feature isn't even being > used by any users yet. > > The exception here would be if we have reason to believe that using multiple > separate pieces is not going to be what we're going to want to do in the long > run, in which case it's potentially a waste to work on it until we know that for > sure. Android deals with it, as far as I know, by putting the pieces of text in different <div></div> entries. I don't feel that the UI is fully nailed down and that, at this point, this is still an experiment. Making it look a bit nicer for the test seemed worth a few minutes of effort, even if a real solution would take only a day (a few minutes is still smaller :) ). This CL would be nice to land, imo, but it's not a big deal. I think the ROI is solid, but if it's not the right investment for Chromium, then the return may not matter.
On 2015/03/21 00:26:24, dschuyler wrote: > I don't feel that the UI is fully nailed down and that, at this point, this is > still an experiment. Making it look a bit nicer for the test seemed worth a few > minutes of effort, even if a real solution would take only a day (a few minutes > is still smaller :) ). > > This CL would be nice to land, imo, but it's not a big deal. I think the ROI is > solid, but if it's not the right investment for Chromium, then the return may > not matter. If we're not certain enough of what the UI is to know if we want to split things into multiple pieces, let's wait on landing anything here at all. I imagine we can still make good judgments from the state of the UI without it, especially if we know this is a solvable problem.
On 2015/03/21 01:15:49, Peter Kasting wrote: > On 2015/03/21 00:26:24, dschuyler wrote: > > I don't feel that the UI is fully nailed down and that, at this point, this is > > still an experiment. Making it look a bit nicer for the test seemed worth a > few > > minutes of effort, even if a real solution would take only a day (a few > minutes > > is still smaller :) ). > > > > This CL would be nice to land, imo, but it's not a big deal. I think the ROI > is > > solid, but if it's not the right investment for Chromium, then the return may > > not matter. > > If we're not certain enough of what the UI is to know if we want to split things > into multiple pieces, let's wait on landing anything here at all. I imagine we > can still make good judgments from the state of the UI without it, especially if > we know this is a solvable problem. The implementation on Android and iOS is to just insert spaces between the three text elements. We had a discussion about this on one of Dave's earlier CLs in the context of localization. You expressed concern about the spacing varying with fonts, languages and so on. I agreed that this was a valid concern, but so far had worked well in practice. Dave, I think it's worth you taking the time to explore doing better than I did and actually positioning the labels at a fixed pixel width apart. That being said, if it's the difference between us being ready to experiment in M43 or not (I know there's other tasks still to do), then I think we should land this.
On 2015/03/23 14:40:59, Justin Donnelly wrote: > On 2015/03/21 01:15:49, Peter Kasting wrote: > > On 2015/03/21 00:26:24, dschuyler wrote: > > > I don't feel that the UI is fully nailed down and that, at this point, this > is > > > still an experiment. Making it look a bit nicer for the test seemed worth a > > few > > > minutes of effort, even if a real solution would take only a day (a few > > minutes > > > is still smaller :) ). > > > > > > This CL would be nice to land, imo, but it's not a big deal. I think the > ROI > > is > > > solid, but if it's not the right investment for Chromium, then the return > may > > > not matter. > > > > If we're not certain enough of what the UI is to know if we want to split > things > > into multiple pieces, let's wait on landing anything here at all. I imagine > we > > can still make good judgments from the state of the UI without it, especially > if > > we know this is a solvable problem. > > The implementation on Android and iOS is to just insert spaces between the three > text elements. We had a discussion about this on one of Dave's earlier CLs in > the context of localization. You expressed concern about the spacing varying > with fonts, languages and so on. I agreed that this was a valid concern, but so > far had worked well in practice. > > Dave, I think it's worth you taking the time to explore doing better than I did > and actually positioning the labels at a fixed pixel width apart. That being > said, if it's the difference between us being ready to experiment in M43 or not > (I know there's other tasks still to do), then I think we should land this. I agree it would be best to make further improvements. I'm concerned that M43 may go out with neither improvement (a space or nicer layout). That would leave the text crammed together. That's not excellent in a product excellence kind of way. I think it looks nice with a space even if it could look different with more layout options. Without at least a space between the words, it looks kind of sloppy - that's what I'm trying to avoid. We don't truly know at the moment which was is most correct for RTL languages. The words are things like temperature and a day of the week. Which comes before which is not normally standardized. It's not a verb to noun kind of discussion, which have clear right and wrong order for different languages. I think "40F Wed" is going to look better than "40FWed" in any language. Even if that language would prefer "Wed 40F", "40F Wed" is better than "'40FWed" or "Wed40F". For reference, The fancier layout options will allow choices like "40F Wed", and "Wed 40F". It's unclear whether the fancier options will make it into M43. The reason is that doing it properly involves more systems and a longer review cycle. The quick options are to hack a space between words or to hack a layout multiple RenderText objects (which would end up looking like a space between words with greater effort than a CL that is already ready). It would also be smart for me/us to contact those who commonly use RTL languages and find out what they feel is the best quality layout. It doesn't feel correct for me to presume that a RTL reader is going to prefer "Wed 40F", "40F Wed", "Wed 40F", or "40F Wed". (Though I feel fine saying that "40FWed" unlikely to be perfered). P.S. The other place I've seen this used is with stocks. Such as, "543.21+1.33". I think that would look nicer as "543.21 +1.33", even if a RTL reader may prefer "+1.33 543.21".
My suggestion to do nothing was based on the assumption that we're not yet ready to test this on users, we're still in only-the-dev-team-looks-at-it mode. If we actually want to start displaying this to external people, then I'm OK with the idea of landing something more temporary. I did recall the previous conversation about spaces but somehow thought this was something different (I dunno why). Make sure to resync your checkout so the trybots can apply the patch. https://codereview.chromium.org/1023993003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/1023993003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:707: true); I think it would be easier to just manually prepend the space in this call and the next one than adding functionality to the other function. https://codereview.chromium.org/1023993003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:764: text.insert(0, base::UTF8ToUTF16(" ")); Use ASCIIToUTF16(), it's cheaper. https://codereview.chromium.org/1023993003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.h (right): https://codereview.chromium.org/1023993003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:163: void AppendAnswerText(const SuggestionAnswer::TextField& text_field, While here: This function needs a comment about what it does.
Even if it's other Googlers (whether or not it's external) I think the space makes a difference :) https://codereview.chromium.org/1023993003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/1023993003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:707: true); On 2015/03/23 20:19:21, Peter Kasting wrote: > I think it would be easier to just manually prepend the space in this call and > the next one than adding functionality to the other function. Done. https://codereview.chromium.org/1023993003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:764: text.insert(0, base::UTF8ToUTF16(" ")); On 2015/03/23 20:19:21, Peter Kasting wrote: > Use ASCIIToUTF16(), it's cheaper. Done. https://codereview.chromium.org/1023993003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.h (right): https://codereview.chromium.org/1023993003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:163: void AppendAnswerText(const SuggestionAnswer::TextField& text_field, On 2015/03/23 20:19:21, Peter Kasting wrote: > While here: This function needs a comment about what it does. Done.
LGTM https://codereview.chromium.org/1023993003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/1023993003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:705: if (match_.answer->second_line().additional_text()) { Nit: Briefer, seems a bit more readable: base::char16 space(' '); const auto* text_field = match_.answer->second_line().additional_text(); if (text_field) AppendAnswerText(space + text_field->text(), text_field->type()); text_field = match_.answer->second_line().status_text(); if (text_field) AppendAnswerText(space + text_field->text(), text_field->type()); (I believe that char16 trick should work, but if not one could use a string16 there with ASCIIToUTF16() instead.) https://codereview.chromium.org/1023993003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.h (right): https://codereview.chromium.org/1023993003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:162: // Adds styled text to |description_rendertext_|. Nit: Can you flesh this out more? How is the text styled? For example: Adds |text| to |description_rendertext_|. |text_type| is an index into the kTextStyles constant defined in the .cc file and is used to style the text, including setting the font size, color, and baseline style. See the TextStyle struct in the .cc file for more. https://codereview.chromium.org/1023993003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:163: void AppendAnswerText(const base::string16& text, int text_type); Nit: |text_style| might be a better name for this arg. Now that we're passing this to a function defined in the .h I _almost_ want to make it an enum instead of an int, but I think an int is probably still OK for now.
https://codereview.chromium.org/1023993003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/1023993003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:705: if (match_.answer->second_line().additional_text()) { On 2015/03/24 01:05:35, Peter Kasting wrote: > Nit: Briefer, seems a bit more readable: > > base::char16 space(' '); > const auto* text_field = match_.answer->second_line().additional_text(); > if (text_field) > AppendAnswerText(space + text_field->text(), text_field->type()); > text_field = match_.answer->second_line().status_text(); > if (text_field) > AppendAnswerText(space + text_field->text(), text_field->type()); > > (I believe that char16 trick should work, but if not one could use a string16 > there with ASCIIToUTF16() instead.) Done. https://codereview.chromium.org/1023993003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.h (right): https://codereview.chromium.org/1023993003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:162: // Adds styled text to |description_rendertext_|. On 2015/03/24 01:05:35, Peter Kasting wrote: > Nit: Can you flesh this out more? How is the text styled? For example: > > Adds |text| to |description_rendertext_|. |text_type| is an index into the > kTextStyles constant defined in the .cc file and is used to style the text, > including setting the font size, color, and baseline style. See the TextStyle > struct in the .cc file for more. Thanks! Done. https://codereview.chromium.org/1023993003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:163: void AppendAnswerText(const base::string16& text, int text_type); On 2015/03/24 01:05:35, Peter Kasting wrote: > Nit: |text_style| might be a better name for this arg. > > Now that we're passing this to a function defined in the .h I _almost_ want to > make it an enum instead of an int, but I think an int is probably still OK for > now. I call the TextStyle instance text_style in the same function. Also the function being called to get the text_type is type(). Maybe not be best names ever, but there somewhat consistent in this space. (pun intended).
https://codereview.chromium.org/1023993003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.h (right): https://codereview.chromium.org/1023993003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:163: void AppendAnswerText(const base::string16& text, int text_type); On 2015/03/24 01:26:47, dschuyler wrote: > On 2015/03/24 01:05:35, Peter Kasting wrote: > > Nit: |text_style| might be a better name for this arg. > > > > Now that we're passing this to a function defined in the .h I _almost_ want to > > make it an enum instead of an int, but I think an int is probably still OK for > > now. > > I call the TextStyle instance text_style in the same function. Also the > function being called to get the text_type is type(). Maybe not be best names > ever, but there somewhat consistent in this space. (pun intended). Fair enough, but this highlights for me that there's a type mismatch problem here. You take this arg as an int in this function, but GetTextStyle() in the .cc file takes a size_t. The conversion is done implicitly. Probably GetTextStyle() should take an int and clamp values of 0 or less to 1. (Right now, calling that function with a type of 0 is actually going to crash Chrome, which seems bad.)
Thanks again! https://codereview.chromium.org/1023993003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.h (right): https://codereview.chromium.org/1023993003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:163: void AppendAnswerText(const base::string16& text, int text_type); On 2015/03/24 01:44:56, Peter Kasting wrote: > On 2015/03/24 01:26:47, dschuyler wrote: > > On 2015/03/24 01:05:35, Peter Kasting wrote: > > > Nit: |text_style| might be a better name for this arg. > > > > > > Now that we're passing this to a function defined in the .h I _almost_ want > to > > > make it an enum instead of an int, but I think an int is probably still OK > for > > > now. > > > > I call the TextStyle instance text_style in the same function. Also the > > function being called to get the text_type is type(). Maybe not be best names > > ever, but there somewhat consistent in this space. (pun intended). > > Fair enough, but this highlights for me that there's a type mismatch problem > here. You take this arg as an int in this function, but GetTextStyle() in the > .cc file takes a size_t. The conversion is done implicitly. > > Probably GetTextStyle() should take an int and clamp values of 0 or less to 1. > (Right now, calling that function with a type of 0 is actually going to crash > Chrome, which seems bad.) Done.
https://codereview.chromium.org/1023993003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/1023993003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:201: if (type < 1 || type > static_cast<int>(arraysize(kTextStyles))) Super nitpicky nit: Better to static_cast<size_t>(type) in the second comparison, since we've verified it's >= 1 and thus that cast must succeed, whereas in theory (not in practice), the downcast of arraysize() to int might fail. https://codereview.chromium.org/1023993003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:707: const auto* text_field = Why did you move these temps inside the conditionals? That forces you to repeat the long, readability-decreasing expression to compute |text_field| from the conditional each time. Eliminating that repetition was a major reason for the temp. The only reason I could see was to try to prevent a symbol definition conflict with |text_field| above, but that's scoped to the loop and thus won't be a problem.
https://codereview.chromium.org/1023993003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/1023993003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:201: if (type < 1 || type > static_cast<int>(arraysize(kTextStyles))) On 2015/03/24 02:28:11, Peter Kasting wrote: > Super nitpicky nit: Better to static_cast<size_t>(type) in the second > comparison, since we've verified it's >= 1 and thus that cast must succeed, > whereas in theory (not in practice), the downcast of arraysize() to int might > fail. Correct on all counts :) Done. https://codereview.chromium.org/1023993003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:707: const auto* text_field = On 2015/03/24 02:28:11, Peter Kasting wrote: > Why did you move these temps inside the conditionals? That forces you to repeat > the long, readability-decreasing expression to compute |text_field| from the > conditional each time. > > Eliminating that repetition was a major reason for the temp. > > The only reason I could see was to try to prevent a symbol definition conflict > with |text_field| above, but that's scoped to the loop and thus won't be a > problem. I didn't copy paste the example given. I.e. it wasn't a conscious choice to not change the conditional. I saw the biggest win in changing the block scope references. Changing the conditional is nice as well though. Done.
LGTM https://codereview.chromium.org/1023993003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/1023993003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:705: const base::char16 space(' '); Nit: I personally like const (non-pointer) locals and think they should be used more, but I think prevailing Chromium style is to not mark them const. You're OK either way, just be aware of the discrepancy.
The CQ bit was checked by dschuyler@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1023993003/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/ea24fe817ef9eedb7a810dc0b9eb46fa66b5e059 Cr-Commit-Position: refs/heads/master@{#322071} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
