Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
98463012:
Infinite Suggest for mac (Closed)
|
Created:
7 years ago by Anuj Modified:
6 years, 9 months ago CC:
chromium-reviews, James Su, David Black Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionInfinite Suggest for mac
The infinite suggestions are shown with a leading ellipsis suggesting the completion of the trailing part of the query. Type "try google instant s" on google.com on web to see it in action.
If the window width is shorter, the suggestions are moved back such that
- the ellipsis are vertically aligned for all suggestions.
- the longest suggestion is visible
If the window width is shorter than the longest suggestion, all suggestions are left aligned, while right end is clipped as appropriate.
LTR in RTL and RTL in LTR and Mixed LTR-RTL does not look good, but is acceptable.
BUG=327833
Patch Set 1 #Patch Set 2 : Add Infinite Suggest for Mac #
Total comments: 10
Patch Set 3 : Addressed comments, updated bug #
Total comments: 6
Patch Set 4 : Addressed reviewer comments #
Total comments: 21
Patch Set 5 : Addressed comments, bit rewrite #Patch Set 6 : Addressed comments, bit rewrite #
Total comments: 12
Messages
Total messages: 37 (0 generated)
Mark, please take a look at search_provider. Scott, please let me know if you need further clarifications. I will upload screenshots in the bug.
On 2013/12/12 02:27:59, Anuj wrote: > Mark, please take a look at search_provider. > > Scott, please let me know if you need further clarifications. I will upload > screenshots in the bug. Replied on bug. Will not review the bug is settled. --mark
These comments are high-level, I'm still digesting the various offset calculations, but figured since Mark had concerns maybe I shouldn't invest a ton of time on that, yet. Suffice to say, it looks like things could be streamlined in some ways. https://codereview.chromium.org/98463012/diff/20001/chrome/browser/ui/cocoa/o... File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h (right): https://codereview.chromium.org/98463012/diff/20001/chrome/browser/ui/cocoa/o... chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h:10: #include "base/mac/scoped_nsobject.h" This doesn't appear to be needed? https://codereview.chromium.org/98463012/diff/20001/chrome/browser/ui/cocoa/o... File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right): https://codereview.chromium.org/98463012/diff/20001/chrome/browser/ui/cocoa/o... chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:89: CGFloat textXOffset = kTextXOffset + [self additionalOffset]; I think this might make more sense as a method -textOffset which returns kTextXOffset adjusted as necessary to account for -setAdditionalOffset:. https://codereview.chromium.org/98463012/diff/20001/chrome/browser/ui/cocoa/o... File chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm (right): https://codereview.chromium.org/98463012/diff/20001/chrome/browser/ui/cocoa/o... chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm:50: const char16 kEllipsis[] = { 0x2026, 0x20, 0x0 }; Don't define your own constant. ElideString() uses gfx::ElideText(), which uses gfx::kEllipsisUTF16. Doesn't have the space, but that's easy enough to construct. https://codereview.chromium.org/98463012/diff/20001/chrome/browser/ui/cocoa/o... chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm:138: MatchText(match, result_font, matrix_width); This is going to construct a result which you are then going to throw away in the SEARCH_SUGGEST_INFINITE case and construct all over again using very similar code. So it seems like it might make sense to abstract it out into a similar helper, rather than having it inline, obscuring the overall matrix setup. I'm still trying to detangle how one might handle ignored_prefix_width and ellipsis_prefix_width, which seem constant for all SEARCH_SUGGEST_INFINITE, versus the max_ values which aren't. Your screen shots show the offsets as being the same for all values, so the per-iteration calculations in the second loop seem suspect, to me. I'm wondering if it would be more appropriate to tell the cell itself whether it's one of these cells, and then calculate a constant offset and set it for _all_ of the cells, with only the cells set to that type actually adjusting by it. https://codereview.chromium.org/98463012/diff/20001/chrome/browser/ui/cocoa/o... chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm:141: No empty line.
- Will address Mark's concerns shortly. - Replied to your "big" comment to get some more input. https://codereview.chromium.org/98463012/diff/20001/chrome/browser/ui/cocoa/o... File chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm (right): https://codereview.chromium.org/98463012/diff/20001/chrome/browser/ui/cocoa/o... chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm:138: MatchText(match, result_font, matrix_width); 1. I can change the code as follows if (ignored_prefix_width == 0) ignored_prefix_width = [ignored_prefix_as size].width; Same for ellipsis_prefix_width. 2. max_match_width and max_required_width still need to be computed. 3. You are right - The additional_offset calculation can be moved out of the second loop. 4. I should probably use scoped_ptr<NSMutableAttributedString> inside the loop - please advice.
I am working on a screenshot where a URL suggestion mixes up with infinite suggestion. As explained on bug, it is difficult to simulate. Please take a look if possible. https://codereview.chromium.org/98463012/diff/20001/chrome/browser/ui/cocoa/o... File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h (right): https://codereview.chromium.org/98463012/diff/20001/chrome/browser/ui/cocoa/o... chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h:10: #include "base/mac/scoped_nsobject.h" On 2013/12/12 19:11:15, shess wrote: > This doesn't appear to be needed? Done. https://codereview.chromium.org/98463012/diff/20001/chrome/browser/ui/cocoa/o... File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right): https://codereview.chromium.org/98463012/diff/20001/chrome/browser/ui/cocoa/o... chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:89: CGFloat textXOffset = kTextXOffset + [self additionalOffset]; On 2013/12/12 19:11:15, shess wrote: > I think this might make more sense as a method -textOffset which returns > kTextXOffset adjusted as necessary to account for -setAdditionalOffset:. Done. https://codereview.chromium.org/98463012/diff/20001/chrome/browser/ui/cocoa/o... File chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm (right): https://codereview.chromium.org/98463012/diff/20001/chrome/browser/ui/cocoa/o... chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm:50: const char16 kEllipsis[] = { 0x2026, 0x20, 0x0 }; On 2013/12/12 19:11:15, shess wrote: > Don't define your own constant. ElideString() uses gfx::ElideText(), which uses > gfx::kEllipsisUTF16. Doesn't have the space, but that's easy enough to > construct. Done. https://codereview.chromium.org/98463012/diff/20001/chrome/browser/ui/cocoa/o... chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm:141: On 2013/12/12 19:11:15, shess wrote: > No empty line. Done.
I have added as many screenshots as feasible. Please elaborate on the need for URL suggestion, and I will try to look into further. Note that the infinite suggestions are guarded by a server side experiment.
Comments below on the autocomplete/... As you can see from the bug comments, I'm also concerned judging by your screenshots about whether the mac code (which I haven't reviewed) its rendering the right thing. --mark https://codereview.chromium.org/98463012/diff/60001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/98463012/diff/60001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:115: query_string.substr(0, query_string.rfind(match_contents)); What if query_string does not contain match_contents? I think this will behave badly. (I know you might claim that'll never happen, but here in SearchProvider we try to guarantee everything works in a sane way regardless of what we get back from the server.) https://codereview.chromium.org/98463012/diff/60001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:117: input_text.substr(prefix.length()) : input_text; Should the latter case ever happen on matches marked as infinite suggest?
I have updated the screenshots on the bug. I have shown the behavior to David Black (TL), and we are fine with the experience. As mentioned earlier, this will still be restricted by server side experiment. https://codereview.chromium.org/98463012/diff/60001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/98463012/diff/60001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:115: query_string.substr(0, query_string.rfind(match_contents)); On 2013/12/13 16:58:01, Mark P wrote: > What if query_string does not contain match_contents? I think this will behave > badly. > > (I know you might claim that'll never happen, but here in SearchProvider we try > to guarantee everything works in a sane way regardless of what we get back from > the server.) Done. https://codereview.chromium.org/98463012/diff/60001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:117: input_text.substr(prefix.length()) : input_text; It should never happen, but like you said in previous comment - sanity!
Ping! I need this for M33.
Apologies - I had done a bunch of drafts, then someone distributed me in series, so it sat there pretty much all day. https://codereview.chromium.org/98463012/diff/80001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/98463012/diff/80001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:114: size_t content_index = query_string.rfind(match_contents); I'm like 27% confident in this code (probably less). Can match_contents appear multiple times in query_string? https://codereview.chromium.org/98463012/diff/80001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:118: input_text.substr(prefix.length()) : input_text; std::string::find() will keep looking for |prefix| in |input_text| until it finds it or there isn't room for it to be found. input_text.compare(0, prefix.length(), prefix) == 0 will only check the beginning. In fact, input_text.compare(0, content_index, query_string) == 0 should also work. https://codereview.chromium.org/98463012/diff/80001/chrome/browser/ui/cocoa/o... File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right): https://codereview.chromium.org/98463012/diff/80001/chrome/browser/ui/cocoa/o... chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:33: } Is this method needed? -setAdditionalOffset: is needed, but nobody external ever calls this. You can just expose -setAdditionalOffset: as a method not a @property. https://codereview.chromium.org/98463012/diff/80001/chrome/browser/ui/cocoa/o... File chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.h (right): https://codereview.chromium.org/98463012/diff/80001/chrome/browser/ui/cocoa/o... chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.h:112: CGFloat content_offsetx_; Is this ever used? Doesn't seem necessary. https://codereview.chromium.org/98463012/diff/80001/chrome/browser/ui/cocoa/o... File chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm (right): https://codereview.chromium.org/98463012/diff/80001/chrome/browser/ui/cocoa/o... chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm:148: DimContentTextColor(), result_font); This is confusing. Since you're passing empty classification, it only exists to snag the attributes? Abstract out the big of DecorateMatchedString() which converts a base::string16 to an attributed string and share the attributes that way. https://codereview.chromium.org/98463012/diff/80001/chrome/browser/ui/cocoa/o... chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm:150: ignored_prefix_width = [ignored_prefix_as size].width; My concern about this (and others) isn't actually whether you set them over and over, it's whether they're always the same. Just looking at the code doesn't tell me whether they're always the same, unless I dig down into how the matches themselves work. You could also just add a comment saying "These are always the same", or a DCHECK. It is slightly reasonable to check-and-skip in a case like this one where you otherwise make a method call. But this function runs in response to user actions, so it's not essential to make that kinds of savings. https://codereview.chromium.org/98463012/diff/80001/chrome/browser/ui/cocoa/o... chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm:160: result_font); Use the abstracted helper here, too. https://codereview.chromium.org/98463012/diff/80001/chrome/browser/ui/cocoa/o... chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm:184: } This is all very complicated. I honestly have no idea what it's doing, because you're calculating three things at once. I suspect it could be simpler, but I'm not sure it's easy to simplify on a stepwise basis. My suggestion is to first loop through filling the cells in with "... <part we keep>", setting the offset to the ignored_prefix_width. Don't track required width at all. Put the cells which were from SEARCH_SUGGEST_INFINITE in an NSMutableArray, like: NSMutableArray* infinite_cells = [NSMutableArray arrayWithCapacity:rows]; ... [infinite_cells addObject:cell]; At this point, some cells maybe extend beyond the RHS. So add a loop over infinite_cells to shift things left as needed: CGFloat final_offset = ignored_prefix_width; for (OmniboxPopupCell* cell in infinite_cells) { CGFloat x = NSMaxX(matrix_width) - [cell attributedTitle size].width - other_stuff; final_offset = std::min(final_offset, x); } final_offset = std::max(final_offset, 0.0); for (OmniboxPopupCell* cell in infinite_cells) { [cell setAdditionalOffset:final_offset]; } When I first started writing that, I wanted to check whether final_offset differed from ignored_prefix_width, I'm not sure it matters. Just run through finding the min() x-coord which works (possibly ignored_prefix_width), then reset them all.
https://codereview.chromium.org/98463012/diff/60001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/98463012/diff/60001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:117: input_text.substr(prefix.length()) : input_text; On 2013/12/13 18:57:49, Anuj wrote: > It should never happen, but like you said in previous comment - sanity! Okay, But why don't you structure this the same way as the rest of the block? (only override lookup_text if the condition is true, otherwise leave it unchanged as input_text) https://codereview.chromium.org/98463012/diff/80001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/98463012/diff/80001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:114: size_t content_index = query_string.rfind(match_contents); On 2013/12/14 00:40:57, shess wrote: > I'm like 27% confident in this code (probably less). Can match_contents appear > multiple times in query_string? FYI, I'm confident it can appear multiple times. Actually, come to think of it, I think the string is supposed to end in match_contents, right Anuj?
Reading the changelist description again, how do you prevent RTL suggestions
from doing something that looks stupid? ("RTL support is not provided. ") Are
you hoping the server won't return any? Are you hoping that the code in
search_provider.cc will leave a sane state?
--mark
Mixed LTR-RTL gets ugly, and the effort needed to handle that is not justified at the stage of experimentation. google.com doesn't handle it well either. AFAICT Mac code doesn't handle RTL correctly (the offset is only incremented when rendering text after icons).
On Fri, Dec 13, 2013 at 6:32 PM, <skanuj@chromium.org> wrote: > Mixed LTR-RTL gets ugly, and the effort needed to handle that is not > justified > at the stage of experimentation. google.com doesn't handle it well either. > Yes, I understand all that. I'm wondering where the logic is that prevents RTL infinite suggestions from being displayed. What happens if the server returns them? Try the input RTL input עבר to get RTL suggestions rendered on the left. What happens with infinite suggest in these types of situations? I'm not sure how to get suggestions displayed RTL like this https://chromium.googlecode.com/issues/attachment?aid=-3017271539696330777&na... but I'd be surprised if we don't do it in Mac because I know we do it on Linux, Windows, ChromeOS, and Android. --mark > > AFAICT Mac code doesn't handle RTL correctly (the offset is only > incremented > when rendering text after icons). > > > https://codereview.chromium.org/98463012/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I have changed the implementation. The offset calculation is now done by the OmniboxPopupCell based on required and contents width of the corresponding match and the max required and contents width. I have also fixed the implementation for RTL UI, and have uploaded the screenshots. It is difficult to define the layout for the LTR infinite suggest in RTL and RTL infinite suggest in LTR. If there is any proposal, I think implementation will be trivial. That said I think it is okay to ignore the above scenarios for experiments. https://codereview.chromium.org/98463012/diff/60001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/98463012/diff/60001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:117: input_text.substr(prefix.length()) : input_text; On 2013/12/14 00:49:53, Mark P wrote: > On 2013/12/13 18:57:49, Anuj wrote: > > It should never happen, but like you said in previous comment - sanity! > > Okay, But why don't you structure this the same way as the rest of the block? > (only override lookup_text if the condition is true, otherwise leave it > unchanged as input_text) Done. https://codereview.chromium.org/98463012/diff/80001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/98463012/diff/80001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:114: size_t content_index = query_string.rfind(match_contents); Yes, match_contents can appear multiple times, but the query_string is supposed to end in match_contents. Given both the above conditions, what problem do you see with this code? https://codereview.chromium.org/98463012/diff/80001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:114: size_t content_index = query_string.rfind(match_contents); On 2013/12/14 00:49:53, Mark P wrote: > On 2013/12/14 00:40:57, shess wrote: > > I'm like 27% confident in this code (probably less). Can match_contents > appear > > multiple times in query_string? > > FYI, I'm confident it can appear multiple times. > > Actually, come to think of it, I think the string is supposed to end in > match_contents, right Anuj? Done. https://codereview.chromium.org/98463012/diff/80001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:118: input_text.substr(prefix.length()) : input_text; On 2013/12/14 00:40:57, shess wrote: > std::string::find() will keep looking for |prefix| in |input_text| until it > finds it or there isn't room for it to be found. input_text.compare(0, > prefix.length(), prefix) == 0 will only check the beginning. In fact, > input_text.compare(0, content_index, query_string) == 0 should also work. Done. https://codereview.chromium.org/98463012/diff/80001/chrome/browser/ui/cocoa/o... File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right): https://codereview.chromium.org/98463012/diff/80001/chrome/browser/ui/cocoa/o... chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:33: } On 2013/12/14 00:40:57, shess wrote: > Is this method needed? -setAdditionalOffset: is needed, but nobody external > ever calls this. You can just expose -setAdditionalOffset: as a method not a > @property. Done. https://codereview.chromium.org/98463012/diff/80001/chrome/browser/ui/cocoa/o... File chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.h (right): https://codereview.chromium.org/98463012/diff/80001/chrome/browser/ui/cocoa/o... chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.h:112: CGFloat content_offsetx_; On 2013/12/14 00:40:57, shess wrote: > Is this ever used? Doesn't seem necessary. Done. https://codereview.chromium.org/98463012/diff/80001/chrome/browser/ui/cocoa/o... File chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm (right): https://codereview.chromium.org/98463012/diff/80001/chrome/browser/ui/cocoa/o... chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm:148: DimContentTextColor(), result_font); On 2013/12/14 00:40:57, shess wrote: > This is confusing. Since you're passing empty classification, it only exists to > snag the attributes? Abstract out the big of DecorateMatchedString() which > converts a base::string16 to an attributed string and share the attributes that > way. Done. https://codereview.chromium.org/98463012/diff/80001/chrome/browser/ui/cocoa/o... chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm:150: ignored_prefix_width = [ignored_prefix_as size].width; On 2013/12/14 00:40:57, shess wrote: > My concern about this (and others) isn't actually whether you set them over and > over, it's whether they're always the same. Just looking at the code doesn't > tell me whether they're always the same, unless I dig down into how the matches > themselves work. You could also just add a comment saying "These are always the > same", or a DCHECK. > > It is slightly reasonable to check-and-skip in a case like this one where you > otherwise make a method call. But this function runs in response to user > actions, so it's not essential to make that kinds of savings. Done. https://codereview.chromium.org/98463012/diff/80001/chrome/browser/ui/cocoa/o... chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm:160: result_font); On 2013/12/14 00:40:57, shess wrote: > Use the abstracted helper here, too. Done. https://codereview.chromium.org/98463012/diff/80001/chrome/browser/ui/cocoa/o... chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm:184: } I have modified the implementation. Note that the width values passed to the cell should be reset in case the older values were from an infinite suggestion, but not the new one.
I forgot to mention - I can not access the link you provided https://chromium.googlecode.com/issues/attachment?aid=-3017271539696330777&na...
Did you upload the latest change? I see cases where you say "Done" but I'm not seeing an additional change. Sorry that I didn't notice this earlier, my weekend was full of holiday stuff. https://codereview.chromium.org/98463012/diff/80001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/98463012/diff/80001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:114: size_t content_index = query_string.rfind(match_contents); On 2013/12/16 02:53:19, Anuj wrote: > Yes, match_contents can appear multiple times, but the query_string is supposed > to end in match_contents. > > Given both the above conditions, what problem do you see with this code? I'll defer to Mark. I was mostly concerned that you had explicitly considered that case.
Sorry. Looks like I missed uploading the patchset. Uploaded now.
> error: old chunk mismatch Sigh. You're going to have to give it another go, looks like. On Mon, Dec 16, 2013 at 12:37 PM, <skanuj@chromium.org> wrote: > Sorry. Looks like I missed uploading the patchset. > > Uploaded now. > > https://codereview.chromium.org/98463012/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Uploaded patch again. The diffs look fine now. One note: The existing implementation left the search icon on LHS even in RTL UI. My CL changes that (See the RTL screenshots in the bug). This behavior matches the RTL UI on Windows. Note that the location bar continues to show the search icon on LHS in RTL UI, and is a separate bug for RTL UI of Chrome on Mac. If needed, I can easily remove the flipping of search icon to RHS. But I think this is one step in the right direction.
https://codereview.chromium.org/98463012/diff/120001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/98463012/diff/120001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:118: contents_index, query_length, match_contents) == 0)) {} isn't optional if the if() condition causes line breaks. Also, I generally prefer the style being used |(x.compare(...) == 0)| because it reads like it means, but if |!x.compare(...)| fits without breaking the lines, I'm comfortable with that, too. Also, I think using contents_index as the anchor and query_length as the length isn't right. I think it should either be match_contents.length() or base::string16::npos. I think the latter is probably better, actually, albeit a little uglier. https://codereview.chromium.org/98463012/diff/120001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right): https://codereview.chromium.org/98463012/diff/120001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:37: ((frame.origin.x + frame.size.width) - (rect.origin.x + rect.size.width)); Try: result.origin.x = NSMinX(frame) + (NSMaxX(frame) - NSMaxX(rect)); same results, but a bit more self-documenting. https://codereview.chromium.org/98463012/diff/120001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm (right): https://codereview.chromium.org/98463012/diff/120001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm:181: } I feel like we're missing each other, here. I'm concerned about the extra 30 lines of opaque per-cell setup landed in the loop above, but you've added additional state to the cell where all of the cells are going to replicate some calculations. I'm fine with the alignment code being here - this is the level which needs to calculate the alignment, after all, the cell just needs to know where the offset is, it doesn't have to know everything that goes into calculating the offset. Could you perhaps review my previous comment in this area and let me know which parts you disagreed with? I thought I had figured things out well enough to suggest ways to avoid having to have a {required, content} * {desired, max} for each. Maybe I missed something subtle? https://codereview.chromium.org/98463012/diff/120001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm:299: NSMutableAttributedString* OmniboxPopupViewMac::CreateAttributedString( I think DecorateMatchedString() was a static member so that it could be tested. So either test this, or move it to the anonymous namespace. Looks to me like the test for DecorateMatchedString() is pretty basic, so I'm not gung-ho on writing a test for this, but OTOH you could basically copy/paste DecorateMatchedStringNoMatch and be done, so your call as to which path to take. https://codereview.chromium.org/98463012/diff/120001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm:336: NSFont* bold_font = nil; The next comment should come after a whitespace line.
Also, I am not really confident on the RTL stuff. It's not so much the "How to accomplish something engineering-wise" as "I don't know if it's an explicit non-goal, given RTL support on OSX in general." Looping in Jeremy for that. He's in Israel, so it's possible/likely that his responses will be unsynced with us. He might be able to suggest someone closer timezone-wise if he thinks it's worthwhile to go forward but will need changes. [Personally, I'd lean towards just fitting with whatever the previous code did in a way which isn't insane (over-drawing yourself would be insane), and move adding more explicit RTL support to a separate CL.]
Thanks for looping me in Scott! It's important to make the distinction between an RTL-language UI and support for RTL language input/display, while Chrome on OS X doesn't do the former very well we do have good support for the latter and I think it's important not to regress that. In particular I don't think that leaving RTL support to some undefined later stage is a good idea, experience has shown that the later you wait with RTL support in a product the harder it gets, not to mention the fact that it's often not added at all. Please be very careful not to break the experience for RTL language input. Happy to discuss over VC and help move this forward quickly.
Also fine adding RTL support in a separate CL as long as this CL doesn't break the Omnibox for RTL language input and we have a clear path towards adding RTL support.
I have responded to your earlier comment to explain why I prefer my approach. https://codereview.chromium.org/98463012/diff/80001/chrome/browser/ui/cocoa/o... File chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm (right): https://codereview.chromium.org/98463012/diff/80001/chrome/browser/ui/cocoa/o... chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm:184: } On 2013/12/14 00:40:57, shess wrote: > This is all very complicated. I honestly have no idea what it's doing, because > you're calculating three things at once. I suspect it could be simpler, but I'm > not sure it's easy to simplify on a stepwise basis. > > My suggestion is to first loop through filling the cells in with "... <part we > keep>", setting the offset to the ignored_prefix_width. Don't track required > width at all. Put the cells which were from SEARCH_SUGGEST_INFINITE in an > NSMutableArray, like: > NSMutableArray* infinite_cells = [NSMutableArray arrayWithCapacity:rows]; > ... > [infinite_cells addObject:cell]; Since cells get reused, the offset needs to be reset on all cells. So there is no specific advantage of tracking the infinite_cells. > > At this point, some cells maybe extend beyond the RHS. So add a loop over > infinite_cells to shift things left as needed: > CGFloat final_offset = ignored_prefix_width; > for (OmniboxPopupCell* cell in infinite_cells) { > CGFloat x = NSMaxX(matrix_width) - [cell attributedTitle size].width - > other_stuff; > final_offset = std::min(final_offset, x); > } > final_offset = std::max(final_offset, 0.0); > for (OmniboxPopupCell* cell in infinite_cells) { > [cell setAdditionalOffset:final_offset]; > } This is not sufficient. We need the max required width, because we want the suggestions to be vertically aligned on the ellipsis. > > When I first started writing that, I wanted to check whether final_offset > differed from ignored_prefix_width, I'm not sure it matters. Just run through > finding the min() x-coord which works (possibly ignored_prefix_width), then > reset them all. https://codereview.chromium.org/98463012/diff/120001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/98463012/diff/120001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:118: contents_index, query_length, match_contents) == 0)) On 2013/12/16 22:22:05, shess wrote: > {} isn't optional if the if() condition causes line breaks. > > Also, I generally prefer the style being used |(x.compare(...) == 0)| because it > reads like it means, but if |!x.compare(...)| fits without breaking the lines, > I'm comfortable with that, too. > > Also, I think using contents_index as the anchor and query_length as the length > isn't right. I think it should either be match_contents.length() or > base::string16::npos. I think the latter is probably better, actually, albeit a > little uglier. Done. https://codereview.chromium.org/98463012/diff/120001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right): https://codereview.chromium.org/98463012/diff/120001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:37: ((frame.origin.x + frame.size.width) - (rect.origin.x + rect.size.width)); On 2013/12/16 22:22:05, shess wrote: > Try: > result.origin.x = NSMinX(frame) + (NSMaxX(frame) - NSMaxX(rect)); > same results, but a bit more self-documenting. Done. https://codereview.chromium.org/98463012/diff/120001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm (right): https://codereview.chromium.org/98463012/diff/120001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm:181: } My problem is that the implementations of omnibox dropdown in views vs mac vs android differ a lot. This model where there is a cell/match-level class which captures the width information seems to work well across the three implementations. For better maintainability, I will prefer a solution which looks similar on all platforms. As such, how a cell is rendered should be the responsibility of the cell-level class, and I can move a lot of MatchText/NSAttributedString related code to OmniboxPopupCell too. But that will be too much of a change for this CL. Given the current solution looks uniform on all platforms, and satisfies the use-cases correctly, I will prefer not to modify this further at this point of time. I will revisit the three implementations again and see if I can move the offset calculation to the popup level class. https://codereview.chromium.org/98463012/diff/120001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm:299: NSMutableAttributedString* OmniboxPopupViewMac::CreateAttributedString( Moved to the anonymous namespace. https://codereview.chromium.org/98463012/diff/120001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm:336: NSFont* bold_font = nil; On 2013/12/16 22:22:05, shess wrote: > The next comment should come after a whitespace line. Done.
On 2013/12/16 22:25:43, shess wrote: > Also, I am not really confident on the RTL stuff. It's not so much the "How to > accomplish something engineering-wise" as "I don't know if it's an explicit > non-goal, given RTL support on OSX in general." Looping in Jeremy for that. > He's in Israel, so it's possible/likely that his responses will be unsynced with > us. He might be able to suggest someone closer timezone-wise if he thinks it's > worthwhile to go forward but will need changes. I don't think we want to prevent RTL support. If something works better than before for RTL, what is the harm/concern? It is unlikely that it will become a full-fledged effort to improve RTL support anytime soon. So I think incremental changes are good. > [Personally, I'd lean towards just fitting with whatever the previous code did > in a way which isn't insane (over-drawing yourself would be insane), and move > adding more explicit RTL support to a separate CL.] What do you mean by "over-drawing"? The RTL support is rather trivial - just that FlipVertical method. And since we are past branch cut, I can incrementally fix other RTL behavior too.
On 2013/12/17 12:37:53, jeremy wrote: > Thanks for looping me in Scott! > > It's important to make the distinction between an RTL-language UI and support > for RTL language input/display, while Chrome on OS X doesn't do the former very > well we do have good support for the latter and I think it's important not to > regress that. There is no regression being introduced by my CL. I am adding an experimental feature, which has good well-defined behavior for RTL in RTL and LTR in LTR. The RTL in LTR UI or LTR in RTL UI or Bidi is not a well-defined use case, and incidence rate does not warrant the investment when we are still experimenting. My CL improves things by putting the search icon in dropdown on RHS in RTL UI, which is where it should have been. > In particular I don't think that leaving RTL support to some undefined later > stage is a good idea, experience has shown that the later you wait with RTL > support in a product the harder it gets, not to mention the fact that it's often > not added at all. Can you please take a look at screenshots attached to the bug? I think the changes are satisfactory. > Please be very careful not to break the experience for RTL language input. > > Happy to discuss over VC and help move this forward quickly. Please suggest what times work for you? I am generally comfortable staying up till midnight PST.
On 2013/12/19 05:39:46, Anuj wrote: > On 2013/12/16 22:25:43, shess wrote: > > Also, I am not really confident on the RTL stuff. It's not so much the "How > to > > accomplish something engineering-wise" as "I don't know if it's an explicit > > non-goal, given RTL support on OSX in general." Looping in Jeremy for that. > > He's in Israel, so it's possible/likely that his responses will be unsynced > with > > us. He might be able to suggest someone closer timezone-wise if he thinks > it's > > worthwhile to go forward but will need changes. > > I don't think we want to prevent RTL support. If something works better than > before for RTL, what is the harm/concern? It is unlikely that it will become a > full-fledged effort to improve RTL support anytime soon. So I think incremental > changes are good. The omnibox and the popup are designed to go together. Adding RTL support in one and not the other could be weird. For instance, if the omnibox text is left-aligned but the popup text is right-aligned, you cannot scan down from omnibox to popup (think in terms of windows on a widescreen monitor to see why that might matter). I'm not saying "No", I'm just saying that it's not clear that half-done RTL support with no specific plans to finish it is better than no RTL support. > > [Personally, I'd lean towards just fitting with whatever the previous code did > > in a way which isn't insane (over-drawing yourself would be insane), and move > > adding more explicit RTL support to a separate CL.] > > What do you mean by "over-drawing"? The RTL support is rather trivial - just > that FlipVertical method. > And since we are past branch cut, I can incrementally fix other RTL behavior > too. I meant that a common result of quick-and-dirty hacks is things like drawing pieces of RTL text overlapping pieces of LTR text, which is clearly bad. I don't think your code does that, it was just an example of the kind of thing which should be avoided. I probably hadn't reviewed the code at the point I wrote that, since I was just wanting to get Jeremy notified earlier, due to his timezone difference.
These are comments-on-comments, I figured having them mixed with actual code-review comments would probably confuse things. https://codereview.chromium.org/98463012/diff/80001/chrome/browser/ui/cocoa/o... File chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm (right): https://codereview.chromium.org/98463012/diff/80001/chrome/browser/ui/cocoa/o... chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm:184: } On 2013/12/19 05:36:13, Anuj wrote: > On 2013/12/14 00:40:57, shess wrote: > > This is all very complicated. I honestly have no idea what it's doing, > because > > you're calculating three things at once. I suspect it could be simpler, but > I'm > > not sure it's easy to simplify on a stepwise basis. > > > > My suggestion is to first loop through filling the cells in with "... <part we > > keep>", setting the offset to the ignored_prefix_width. Don't track required > > width at all. Put the cells which were from SEARCH_SUGGEST_INFINITE in an > > NSMutableArray, like: > > NSMutableArray* infinite_cells = [NSMutableArray arrayWithCapacity:rows]; > > ... > > [infinite_cells addObject:cell]; > > Since cells get reused, the offset needs to be reset on all cells. So there is > no specific advantage of tracking the infinite_cells. The point to tracking the infinite cells is because they are the cells which need to be considered to calculate the required offset to be shared between those cells. _All_ of the cells would get their default offset (based on the stripped prefix or not-applicable) on the first loop > > At this point, some cells maybe extend beyond the RHS. So add a loop over > > infinite_cells to shift things left as needed: > > CGFloat final_offset = ignored_prefix_width; > > for (OmniboxPopupCell* cell in infinite_cells) { > > CGFloat x = NSMaxX(matrix_width) - [cell attributedTitle size].width - > > other_stuff; > > final_offset = std::min(final_offset, x); > > } > > final_offset = std::max(final_offset, 0.0); > > for (OmniboxPopupCell* cell in infinite_cells) { > > [cell setAdditionalOffset:final_offset]; > > } > > This is not sufficient. We need the max required width, because we want the > suggestions to be vertically aligned on the ellipsis. I'm sure I'm completely misunderstanding something, then, because the point of the above code is to find the cell with the max required width, and adjust all of them to be aligned with that one. The difference is that it doesn't find the max required width, if finds the min required x-pos, which is (AFAICT) the point of finding the max required width. https://codereview.chromium.org/98463012/diff/120001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm (right): https://codereview.chromium.org/98463012/diff/120001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm:181: } On 2013/12/19 05:36:13, Anuj wrote: > My problem is that the implementations of omnibox dropdown in views vs mac vs > android differ a lot. This model where there is a cell/match-level class which > captures the width information seems to work well across the three > implementations. > For better maintainability, I will prefer a solution which looks similar on all > platforms. That helps maintainability of your feature, but doesn't necessarily help maintainability of the per-platform classes. Views controls are not Mac controls are not Android controls, it's just the way it is. If you want to share calculators, abstract it out into a helper so that it _looks_ similar (I'm pretty sure [cell setMaxRequiredWidth:0] doesn't look like anything on Views ...). My issue about having the cell understand the min/max/etc constraints is that the cell cannot calculate the results in isolation. The matrix must be involved to propagate things between neighboring cells. But once the matrix is involved, then you might as well have the matrix do the calculation directly, rather than distribute it, since they all have to make it exactly the same. I'm not worried about the duplication of CPU effort, I'm worried about whether thee moving parts can get out of sync with each other, having a single point calculating it helps prevent that.
Hmm, looks like there's nothing new to review? Let me know. I'll be around over the holidays, but obviously it would be nicer to come to some sort of closure like today or something (given how "fun" the commit queue has been lately). https://codereview.chromium.org/98463012/diff/120001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/98463012/diff/120001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:118: contents_index, query_length, match_contents) == 0)) On 2013/12/19 05:36:13, Anuj wrote: > On 2013/12/16 22:22:05, shess wrote: > > {} isn't optional if the if() condition causes line breaks. > > > > Also, I generally prefer the style being used |(x.compare(...) == 0)| because > it > > reads like it means, but if |!x.compare(...)| fits without breaking the lines, > > I'm comfortable with that, too. > > > > Also, I think using contents_index as the anchor and query_length as the > length > > isn't right. I think it should either be match_contents.length() or > > base::string16::npos. I think the latter is probably better, actually, albeit > a > > little uglier. > > Done. Still don't see the {}, here, did you need to re-upload?
On 2013/12/19 19:21:31, shess wrote: > On 2013/12/19 05:39:46, Anuj wrote: > > On 2013/12/16 22:25:43, shess wrote: > > > Also, I am not really confident on the RTL stuff. It's not so much the "How > > to > > > accomplish something engineering-wise" as "I don't know if it's an explicit > > > non-goal, given RTL support on OSX in general." Looping in Jeremy for that. > > > > He's in Israel, so it's possible/likely that his responses will be unsynced > > with > > > us. He might be able to suggest someone closer timezone-wise if he thinks > > it's > > > worthwhile to go forward but will need changes. > > > > I don't think we want to prevent RTL support. If something works better than > > before for RTL, what is the harm/concern? It is unlikely that it will become a > > full-fledged effort to improve RTL support anytime soon. So I think > incremental > > changes are good. > > The omnibox and the popup are designed to go together. Adding RTL support in > one and not the other could be weird. For instance, if the omnibox text is > left-aligned but the popup text is right-aligned, you cannot scan down from > omnibox to popup (think in terms of windows on a widescreen monitor to see why > that might matter). Noted. Please take a look at screenshot. The issues you describe are not observed. > I'm not saying "No", I'm just saying that it's not clear that half-done RTL > support with no specific plans to finish it is better than no RTL support. If you are not saying "Yes" or "No", I am not sure what to do to make this change better. I have uploaded a screenshot of RTL in RTL for Safari. I think my change (flipping search icon to RHS) definitely makes the resultant UI look better. Another change will be needed in LocationBar (switch left and right) https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... I don't think any other change is needed, given Safari doesn't flip navigation buttons in RTL UI. > > > [Personally, I'd lean towards just fitting with whatever the previous code > did > > > in a way which isn't insane (over-drawing yourself would be insane), and > move > > > adding more explicit RTL support to a separate CL.] > > > > What do you mean by "over-drawing"? The RTL support is rather trivial - just > > that FlipVertical method. > > And since we are past branch cut, I can incrementally fix other RTL behavior > > too. > > I meant that a common result of quick-and-dirty hacks is things like drawing > pieces of RTL text overlapping pieces of LTR text, which is clearly bad. I > don't think your code does that, it was just an example of the kind of thing > which should be avoided. I probably hadn't reviewed the code at the point I > wrote that, since I was just wanting to get Jeremy notified earlier, due to his > timezone difference. Noted.
Looks like the email sending part of the code review server went haywire.
On 2013/12/19 21:37:53, Anuj wrote: > On 2013/12/19 19:21:31, shess wrote: > > On 2013/12/19 05:39:46, Anuj wrote: > > > On 2013/12/16 22:25:43, shess wrote: > > > > Also, I am not really confident on the RTL stuff. It's not so much the > "How > > > to > > > > accomplish something engineering-wise" as "I don't know if it's an > explicit > > > > non-goal, given RTL support on OSX in general." Looping in Jeremy for > that. > > > > > > He's in Israel, so it's possible/likely that his responses will be > unsynced > > > with > > > > us. He might be able to suggest someone closer timezone-wise if he thinks > > > it's > > > > worthwhile to go forward but will need changes. > > > > > > I don't think we want to prevent RTL support. If something works better than > > > before for RTL, what is the harm/concern? It is unlikely that it will become > a > > > full-fledged effort to improve RTL support anytime soon. So I think > > incremental > > > changes are good. > > > > The omnibox and the popup are designed to go together. Adding RTL support in > > one and not the other could be weird. For instance, if the omnibox text is > > left-aligned but the popup text is right-aligned, you cannot scan down from > > omnibox to popup (think in terms of windows on a widescreen monitor to see why > > that might matter). > > Noted. Please take a look at screenshot. The issues you describe are not > observed. Which screenshot? The bug has like a dozen from different platforms, and AFAICT the only Chrome/OSX one which might be relevant (comment #17) has the RTL text left-justified. The FlipVertical() change should right-justify them, unless I misunderstand what's going on, so it will be more like the Safari screenshot (comment #32). > > I'm not saying "No", I'm just saying that it's not clear that half-done RTL > > support with no specific plans to finish it is better than no RTL support. > > If you are not saying "Yes" or "No", I am not sure what to do to make this > change better. > I have uploaded a screenshot of RTL in RTL for Safari. > I think my change (flipping search icon to RHS) definitely makes the resultant > UI look better. > > Another change will be needed in LocationBar (switch left and right) > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > I don't think any other change is needed, given Safari doesn't flip navigation > buttons in RTL UI. We aren't a Safari clone. I would prefer the final thing to look more like the Windows screenshot in comment #19 rather than attempting to emulate Safari, unless someone wants to argue on the merits of that layout. I'm not saying yes-or-no because I'm not a RTL user. Jeremy seemed happy with partial progress, so I'm fine with partial progress if he's fine with it, so we probably don't need to further debate it.
On 2013/12/19 21:47:13, shess wrote: > On 2013/12/19 21:37:53, Anuj wrote: > > On 2013/12/19 19:21:31, shess wrote: > > > On 2013/12/19 05:39:46, Anuj wrote: > > > > On 2013/12/16 22:25:43, shess wrote: > > > > > Also, I am not really confident on the RTL stuff. It's not so much the > > "How > > > > to > > > > > accomplish something engineering-wise" as "I don't know if it's an > > explicit > > > > > non-goal, given RTL support on OSX in general." Looping in Jeremy for > > that. > > > > > > > > He's in Israel, so it's possible/likely that his responses will be > > unsynced > > > > with > > > > > us. He might be able to suggest someone closer timezone-wise if he > thinks > > > > it's > > > > > worthwhile to go forward but will need changes. > > > > > > > > I don't think we want to prevent RTL support. If something works better > than > > > > before for RTL, what is the harm/concern? It is unlikely that it will > become > > a > > > > full-fledged effort to improve RTL support anytime soon. So I think > > > incremental > > > > changes are good. > > > > > > The omnibox and the popup are designed to go together. Adding RTL support > in > > > one and not the other could be weird. For instance, if the omnibox text is > > > left-aligned but the popup text is right-aligned, you cannot scan down from > > > omnibox to popup (think in terms of windows on a widescreen monitor to see > why > > > that might matter). > > > > Noted. Please take a look at screenshot. The issues you describe are not > > observed. > > Which screenshot? The bug has like a dozen from different platforms, and AFAICT > the only Chrome/OSX one which might be relevant (comment #17) has the RTL text > left-justified. The FlipVertical() change should right-justify them, unless I > misunderstand what's going on, so it will be more like the Safari screenshot > (comment #32). See screenshot #16 for RTL in RTL UI. Screenshot #17 corresponds to RTL in LTR UI, a use-case for which we do not have a well-defined behavior. > > > I'm not saying "No", I'm just saying that it's not clear that half-done RTL > > > support with no specific plans to finish it is better than no RTL support. > > > > If you are not saying "Yes" or "No", I am not sure what to do to make this > > change better. > > I have uploaded a screenshot of RTL in RTL for Safari. > > I think my change (flipping search icon to RHS) definitely makes the resultant > > UI look better. > > > > Another change will be needed in LocationBar (switch left and right) > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > I don't think any other change is needed, given Safari doesn't flip navigation > > buttons in RTL UI. > > We aren't a Safari clone. I would prefer the final thing to look more like the > Windows screenshot in comment #19 rather than attempting to emulate Safari, > unless someone wants to argue on the merits of that layout. I was offering a plan to bring location-bar and omnibox-dropdown to a state of consistent RTL support. I cited Safari because I think platform conventions matter. None of Safari, TextEdit or Preview switch controls from LHS to RHS in RTL UI. So I think the navigation controls don't need to be rearranged, but that goes beyond the scope for me as of now. > I'm not saying yes-or-no because I'm not a RTL user. Jeremy seemed happy with > partial progress, so I'm fine with partial progress if he's fine with it, so we > probably don't need to further debate it. Cool. So we agree that the current behavior of this CL is satisfactory.
Hi Anuj, Thanks for posting screenshots on the bug, but as I'm sure you know this doesn't really provide a good picture of how the feature is supposed to work. I've set up a VC next Monday, lets discuss further there. Sounds like we're on the same page, but at the risk of stating the obvious - here again are the points I wanted to make: * Chrome has tons of RTL-language users - we can not regress Omnibox behavior for them. * Entering RTL text in an LTR UI and vice versa is very common, we cannot regress this either. You are right that the behavior in these cases can be really tricky, but this is exactly what we need to nail down :) * Not getting things perfect in the first CL is absolutely fine just as long as we have a concrete plan going forward. * Experience has shown that the earlier Bidi considerations are taken care of, the easier it is to implement them. Delaying decisions about Bidi support or stating that "someone should take care of this later" isn't a good idea. Absolutely don't want to slow you down, as long as we have a plan for BiDi support going forward and don't provide a bad experience for BiDi-language users I'm fine with this CL. On Fri, Dec 20, 2013 at 12:11 AM, <skanuj@chromium.org> wrote: > On 2013/12/19 21:47:13, shess wrote: > >> On 2013/12/19 21:37:53, Anuj wrote: >> > On 2013/12/19 19:21:31, shess wrote: >> > > On 2013/12/19 05:39:46, Anuj wrote: >> > > > On 2013/12/16 22:25:43, shess wrote: >> > > > > Also, I am not really confident on the RTL stuff. It's not so >> much >> > the > >> > "How >> > > > to >> > > > > accomplish something engineering-wise" as "I don't know if it's an >> > explicit >> > > > > non-goal, given RTL support on OSX in general." Looping in >> Jeremy for >> > that. >> > > >> > > > > He's in Israel, so it's possible/likely that his responses will be >> > unsynced >> > > > with >> > > > > us. He might be able to suggest someone closer timezone-wise if >> he >> thinks >> > > > it's >> > > > > worthwhile to go forward but will need changes. >> > > > >> > > > I don't think we want to prevent RTL support. If something works >> better >> than >> > > > before for RTL, what is the harm/concern? It is unlikely that it >> will >> become >> > a >> > > > full-fledged effort to improve RTL support anytime soon. So I think >> > > incremental >> > > > changes are good. >> > > >> > > The omnibox and the popup are designed to go together. Adding RTL >> support >> in >> > > one and not the other could be weird. For instance, if the omnibox >> text >> > is > >> > > left-aligned but the popup text is right-aligned, you cannot scan down >> > from > >> > > omnibox to popup (think in terms of windows on a widescreen monitor >> to see >> why >> > > that might matter). >> > >> > Noted. Please take a look at screenshot. The issues you describe are not >> > observed. >> > > Which screenshot? The bug has like a dozen from different platforms, and >> > AFAICT > >> the only Chrome/OSX one which might be relevant (comment #17) has the RTL >> text >> left-justified. The FlipVertical() change should right-justify them, >> unless I >> misunderstand what's going on, so it will be more like the Safari >> screenshot >> (comment #32). >> > > See screenshot #16 for RTL in RTL UI. > Screenshot #17 corresponds to RTL in LTR UI, a use-case for which we do > not have > a well-defined behavior. > > > > > I'm not saying "No", I'm just saying that it's not clear that >> half-done >> > RTL > >> > > support with no specific plans to finish it is better than no RTL >> support. >> > >> > If you are not saying "Yes" or "No", I am not sure what to do to make >> this >> > change better. >> > I have uploaded a screenshot of RTL in RTL for Safari. >> > I think my change (flipping search icon to RHS) definitely makes the >> > resultant > >> > UI look better. >> > >> > Another change will be needed in LocationBar (switch left and right) >> > >> > > https://code.google.com/p/chromium/codesearch#chromium/ > src/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm&l=388 > >> > I don't think any other change is needed, given Safari doesn't flip >> > navigation > >> > buttons in RTL UI. >> > > We aren't a Safari clone. I would prefer the final thing to look more >> like >> > the > >> Windows screenshot in comment #19 rather than attempting to emulate >> Safari, >> unless someone wants to argue on the merits of that layout. >> > > I was offering a plan to bring location-bar and omnibox-dropdown to a > state of > consistent RTL support. > I cited Safari because I think platform conventions matter. None of Safari, > TextEdit or Preview switch controls from LHS to RHS in RTL UI. > So I think the navigation controls don't need to be rearranged, but that > goes > beyond the scope for me as of now. > > > I'm not saying yes-or-no because I'm not a RTL user. Jeremy seemed happy >> with >> partial progress, so I'm fine with partial progress if he's fine with it, >> so >> > we > >> probably don't need to further debate it. >> > > Cool. So we agree that the current behavior of this CL is satisfactory. > > > https://codereview.chromium.org/98463012/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Hi Anuj, I know by now you've done a lot of BiDi stuff, which were the main concerns about this change. What's the current status of infinite suggest for Mac? If this code still needed / relevant or has it been made obsolete by more recent changes? --mark On 2013/12/29 10:08:40, jeremy wrote: > Hi Anuj, > > Thanks for posting screenshots on the bug, but as I'm sure you know this > doesn't really provide a good picture of how the feature is supposed to > work. I've set up a VC next Monday, lets discuss further there. > > Sounds like we're on the same page, but at the risk of stating the obvious > - here again are the points I wanted to make: > * Chrome has tons of RTL-language users - we can not regress Omnibox > behavior for them. > * Entering RTL text in an LTR UI and vice versa is very common, we cannot > regress this either. You are right that the behavior in these cases can be > really tricky, but this is exactly what we need to nail down :) > * Not getting things perfect in the first CL is absolutely fine just as > long as we have a concrete plan going forward. > * Experience has shown that the earlier Bidi considerations are taken care > of, the easier it is to implement them. Delaying decisions about Bidi > support or stating that "someone should take care of this later" isn't a > good idea. > > Absolutely don't want to slow you down, as long as we have a plan for BiDi > support going forward and don't provide a bad experience for BiDi-language > users I'm fine with this CL. > > > On Fri, Dec 20, 2013 at 12:11 AM, <mailto:skanuj@chromium.org> wrote: > > > On 2013/12/19 21:47:13, shess wrote: > > > >> On 2013/12/19 21:37:53, Anuj wrote: > >> > On 2013/12/19 19:21:31, shess wrote: > >> > > On 2013/12/19 05:39:46, Anuj wrote: > >> > > > On 2013/12/16 22:25:43, shess wrote: > >> > > > > Also, I am not really confident on the RTL stuff. It's not so > >> much > >> > > the > > > >> > "How > >> > > > to > >> > > > > accomplish something engineering-wise" as "I don't know if it's an > >> > explicit > >> > > > > non-goal, given RTL support on OSX in general." Looping in > >> Jeremy for > >> > that. > >> > > > >> > > > > He's in Israel, so it's possible/likely that his responses will be > >> > unsynced > >> > > > with > >> > > > > us. He might be able to suggest someone closer timezone-wise if > >> he > >> thinks > >> > > > it's > >> > > > > worthwhile to go forward but will need changes. > >> > > > > >> > > > I don't think we want to prevent RTL support. If something works > >> better > >> than > >> > > > before for RTL, what is the harm/concern? It is unlikely that it > >> will > >> become > >> > a > >> > > > full-fledged effort to improve RTL support anytime soon. So I think > >> > > incremental > >> > > > changes are good. > >> > > > >> > > The omnibox and the popup are designed to go together. Adding RTL > >> support > >> in > >> > > one and not the other could be weird. For instance, if the omnibox > >> text > >> > > is > > > >> > > left-aligned but the popup text is right-aligned, you cannot scan down > >> > > from > > > >> > > omnibox to popup (think in terms of windows on a widescreen monitor > >> to see > >> why > >> > > that might matter). > >> > > >> > Noted. Please take a look at screenshot. The issues you describe are not > >> > observed. > >> > > > > Which screenshot? The bug has like a dozen from different platforms, and > >> > > AFAICT > > > >> the only Chrome/OSX one which might be relevant (comment #17) has the RTL > >> text > >> left-justified. The FlipVertical() change should right-justify them, > >> unless I > >> misunderstand what's going on, so it will be more like the Safari > >> screenshot > >> (comment #32). > >> > > > > See screenshot #16 for RTL in RTL UI. > > Screenshot #17 corresponds to RTL in LTR UI, a use-case for which we do > > not have > > a well-defined behavior. > > > > > > > > I'm not saying "No", I'm just saying that it's not clear that > >> half-done > >> > > RTL > > > >> > > support with no specific plans to finish it is better than no RTL > >> support. > >> > > >> > If you are not saying "Yes" or "No", I am not sure what to do to make > >> this > >> > change better. > >> > I have uploaded a screenshot of RTL in RTL for Safari. > >> > I think my change (flipping search icon to RHS) definitely makes the > >> > > resultant > > > >> > UI look better. > >> > > >> > Another change will be needed in LocationBar (switch left and right) > >> > > >> > > > > https://code.google.com/p/chromium/codesearch#chromium/ > > src/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm&l=388 > > > >> > I don't think any other change is needed, given Safari doesn't flip > >> > > navigation > > > >> > buttons in RTL UI. > >> > > > > We aren't a Safari clone. I would prefer the final thing to look more > >> like > >> > > the > > > >> Windows screenshot in comment #19 rather than attempting to emulate > >> Safari, > >> unless someone wants to argue on the merits of that layout. > >> > > > > I was offering a plan to bring location-bar and omnibox-dropdown to a > > state of > > consistent RTL support. > > I cited Safari because I think platform conventions matter. None of Safari, > > TextEdit or Preview switch controls from LHS to RHS in RTL UI. > > So I think the navigation controls don't need to be rearranged, but that > > goes > > beyond the scope for me as of now. > > > > > > I'm not saying yes-or-no because I'm not a RTL user. Jeremy seemed happy > >> with > >> partial progress, so I'm fine with partial progress if he's fine with it, > >> so > >> > > we > > > >> probably don't need to further debate it. > >> > > > > Cool. So we agree that the current behavior of this CL is satisfactory. > > > > > > https://codereview.chromium.org/98463012/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
This code review is now obsolete. I will send a replacement CL soon. On Mar 13, 2014 10:29 AM, <mpearson@chromium.org> wrote: > Hi Anuj, > > I know by now you've done a lot of BiDi stuff, which were the main > concerns about this change. > > What's the current status of infinite suggest for Mac? If this code > still needed / relevant or has it been made obsolete by more recent > changes? > > --mark > > On 2013/12/29 10:08:40, jeremy wrote: > >> Hi Anuj, >> > > Thanks for posting screenshots on the bug, but as I'm sure you know this >> doesn't really provide a good picture of how the feature is supposed to >> work. I've set up a VC next Monday, lets discuss further there. >> > > Sounds like we're on the same page, but at the risk of stating the obvious >> - here again are the points I wanted to make: >> * Chrome has tons of RTL-language users - we can not regress Omnibox >> behavior for them. >> * Entering RTL text in an LTR UI and vice versa is very common, we cannot >> regress this either. You are right that the behavior in these cases can >> be >> really tricky, but this is exactly what we need to nail down :) >> * Not getting things perfect in the first CL is absolutely fine just as >> long as we have a concrete plan going forward. >> * Experience has shown that the earlier Bidi considerations are taken care >> of, the easier it is to implement them. Delaying decisions about Bidi >> support or stating that "someone should take care of this later" isn't a >> good idea. >> > > Absolutely don't want to slow you down, as long as we have a plan for BiDi >> support going forward and don't provide a bad experience for BiDi-language >> users I'm fine with this CL. >> > > > On Fri, Dec 20, 2013 at 12:11 AM, <mailto:skanuj@chromium.org> wrote: >> > > > On 2013/12/19 21:47:13, shess wrote: >> > >> >> On 2013/12/19 21:37:53, Anuj wrote: >> >> > On 2013/12/19 19:21:31, shess wrote: >> >> > > On 2013/12/19 05:39:46, Anuj wrote: >> >> > > > On 2013/12/16 22:25:43, shess wrote: >> >> > > > > Also, I am not really confident on the RTL stuff. It's not so >> >> much >> >> >> > the >> > >> >> > "How >> >> > > > to >> >> > > > > accomplish something engineering-wise" as "I don't know if >> it's an >> >> > explicit >> >> > > > > non-goal, given RTL support on OSX in general." Looping in >> >> Jeremy for >> >> > that. >> >> > > >> >> > > > > He's in Israel, so it's possible/likely that his responses >> will be >> >> > unsynced >> >> > > > with >> >> > > > > us. He might be able to suggest someone closer timezone-wise >> if >> >> he >> >> thinks >> >> > > > it's >> >> > > > > worthwhile to go forward but will need changes. >> >> > > > >> >> > > > I don't think we want to prevent RTL support. If something works >> >> better >> >> than >> >> > > > before for RTL, what is the harm/concern? It is unlikely that it >> >> will >> >> become >> >> > a >> >> > > > full-fledged effort to improve RTL support anytime soon. So I >> think >> >> > > incremental >> >> > > > changes are good. >> >> > > >> >> > > The omnibox and the popup are designed to go together. Adding RTL >> >> support >> >> in >> >> > > one and not the other could be weird. For instance, if the omnibox >> >> text >> >> >> > is >> > >> >> > > left-aligned but the popup text is right-aligned, you cannot scan >> down >> >> >> > from >> > >> >> > > omnibox to popup (think in terms of windows on a widescreen monitor >> >> to see >> >> why >> >> > > that might matter). >> >> > >> >> > Noted. Please take a look at screenshot. The issues you describe are >> not >> >> > observed. >> >> >> > >> > Which screenshot? The bug has like a dozen from different platforms, >> and >> >> >> > AFAICT >> > >> >> the only Chrome/OSX one which might be relevant (comment #17) has the >> RTL >> >> text >> >> left-justified. The FlipVertical() change should right-justify them, >> >> unless I >> >> misunderstand what's going on, so it will be more like the Safari >> >> screenshot >> >> (comment #32). >> >> >> > >> > See screenshot #16 for RTL in RTL UI. >> > Screenshot #17 corresponds to RTL in LTR UI, a use-case for which we do >> > not have >> > a well-defined behavior. >> > >> > >> > > > I'm not saying "No", I'm just saying that it's not clear that >> >> half-done >> >> >> > RTL >> > >> >> > > support with no specific plans to finish it is better than no RTL >> >> support. >> >> > >> >> > If you are not saying "Yes" or "No", I am not sure what to do to make >> >> this >> >> > change better. >> >> > I have uploaded a screenshot of RTL in RTL for Safari. >> >> > I think my change (flipping search icon to RHS) definitely makes the >> >> >> > resultant >> > >> >> > UI look better. >> >> > >> >> > Another change will be needed in LocationBar (switch left and right) >> >> > >> >> >> > >> > https://code.google.com/p/chromium/codesearch#chromium/ >> > src/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm&l=388 >> > >> >> > I don't think any other change is needed, given Safari doesn't flip >> >> >> > navigation >> > >> >> > buttons in RTL UI. >> >> >> > >> > We aren't a Safari clone. I would prefer the final thing to look more >> >> like >> >> >> > the >> > >> >> Windows screenshot in comment #19 rather than attempting to emulate >> >> Safari, >> >> unless someone wants to argue on the merits of that layout. >> >> >> > >> > I was offering a plan to bring location-bar and omnibox-dropdown to a >> > state of >> > consistent RTL support. >> > I cited Safari because I think platform conventions matter. None of >> Safari, >> > TextEdit or Preview switch controls from LHS to RHS in RTL UI. >> > So I think the navigation controls don't need to be rearranged, but that >> > goes >> > beyond the scope for me as of now. >> > >> > >> > I'm not saying yes-or-no because I'm not a RTL user. Jeremy seemed >> happy >> >> with >> >> partial progress, so I'm fine with partial progress if he's fine with >> it, >> >> so >> >> >> > we >> > >> >> probably don't need to further debate it. >> >> >> > >> > Cool. So we agree that the current behavior of this CL is satisfactory. >> > >> > >> > https://codereview.chromium.org/98463012/ >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > > https://codereview.chromium.org/98463012/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
