Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9390022:
Simplify handling of BiDi cursor movement (Closed)
|
Created:
8 years, 10 months ago by benrg Modified:
8 years, 9 months ago CC:
chromium-reviews, sadrul, yusukes+watch_chromium.org, ben+watch_chromium.org, dhollowa+watch_chromium.org, penghuang+watch_chromium.org, tfarina, James Su, msw, Alexei Svitkine (slow) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSimplify handling of BiDi cursor movement
The current caret position and the end of the current selection in a text field are logically the same thing, but the current code represents them separately as SelectionModel::caret_pos_ and SelectionModel::selection_end_, which sometimes have different values, for two reasons:
1. selection_end_ indexes a point between two graphemes (range 0 to N, where N is the text length), but caret_pos_ indexes a grapheme (range 0 to N-1). SelectionModel::caret_placement_ specifies whether the caret is to be displayed at the leading or trailing edge of that grapheme. When the placement is LEADING, selection_end_ and caret_pos_ have the same value, but when it's TRAILING, caret_pos_ points to the start of the grapheme while selection_end_ points to the end.
2. The first and last logical graphemes of the text are not always at the visual boundary (for example, when the text is visually abcFED, the last logical character, F, is in the middle). In order to make the cursor appear at the visual boundary in such cases, the current code attaches it to the leading edge of the D, whereas selection_end_ points after the F.
The first case can be fixed by making caret_pos_ point between graphemes like selection_end_. This is enough to determine the caret position except when it is between bidi runs that aren't visually contiguous: in that case, an extra field ("affinity", replacing placement) indicates whether it appears attached to the previous or the next run.
The second case can be fixed by pretending that the text is surrounded on both sides by text in the default bidi direction, and allowing the caret to be attached to those runs. This makes EdgeSelectionModel() much simpler: it always returns either (0, previous) or (text_length, next).
After these changes, caret_pos_ and selection_end_ always have the same value, so I merged them. For examples of how the old and new SelectionModels compare, look at the changes to ui/gfx/render_text_unittest.cc.
The visual behavior of the text field is mostly unaffected by this change. I'm not sure that the current behavior is actually what we want (it's different from WebKit, Windows, and GTK behavior, all of which are different from each other), but that's a separate issue.
Visible changes:
* RenderText::IsPointInSelection should now behave as expected when clicking on the trailing half of a glyph (it was a TODO).
* Clicking on the right half of a glyph used to place the caret to the left of the glyph on Windows. It should now place it on the right.
* There was a bug in the Windows cursoring code: with the text "CBAdef" in a LTR locale, pressing End, then Left repeatedly, causes the caret to cycle through the leftmost five visual positions instead of stopping at the left. This should now be fixed.
Drive-by changes:
* Replaced the selection with a ui::Range and use ui::Range methods like GetMin() and is_reversed() instead of SelectionModel-specific code.
* Merged RenderText{Linux,Win}::{EdgeSelectionModel,GetCursorBounds} into RenderText. I didn't merge other methods (such as AdjacentCharSelectionModel) that should eventually be merged but aren't as easy to merge.
* Some simplification of the unit tests.
BUG=90426
TEST=existing unit tests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=126287
Patch Set 1 : . #
Total comments: 4
Patch Set 2 : comments #
Total comments: 85
Patch Set 3 : comments #
Total comments: 26
Patch Set 4 : address comments, fix overstrike and operator<< bugs #Patch Set 5 : make set_selection_start private, fix accidental inclusion of local hacks #
Total comments: 39
Patch Set 6 : rebase (not for review) #Patch Set 7 : address comments #
Total comments: 6
Patch Set 8 : rewrite comments #Patch Set 9 : address comment #Patch Set 10 : rebase (no changes) #
Total comments: 6
Patch Set 11 : windows fixes #Patch Set 12 : rebase, more windows fixes #Patch Set 13 : fix merge breakage #
Messages
Total messages: 39 (0 generated)
xji, are you willing to take a look at this?
http://codereview.chromium.org/9390022/diff/10005/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/9390022/diff/10005/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:414: // NB: exploits unsigned wraparound (0 - 1) This is undefined behavior per the C++ standard, so an aggressive optimizer may cause this to do something you don't expect. Please re-write without relying on this. http://codereview.chromium.org/9390022/diff/10005/ui/gfx/selection_model.h File ui/gfx/selection_model.h (right): http://codereview.chromium.org/9390022/diff/10005/ui/gfx/selection_model.h#ne... ui/gfx/selection_model.h:80: UI_EXPORT std::ostream& operator<<(std::ostream& out, I think the convention is to have a ToString() function. e.g. see ui/gfx/range.h.
On 2012/02/14 19:25:32, benrg wrote: > xji, are you willing to take a look at this? Thanks for making those changes! It is inline with Behdad's comments at http://code.google.com/p/chromium/issues/detail?id=90426#c14. Adding Behdad and Mike as reviewers. caret_pos is redundant with selection_end in the sense that it is either the same or offset by 1 grapheme except in visual boundary case. As to the visual boundary case, the selection model (cursor_pos, caret_pos, caret_placement) triplet was implemented as (0, 0, LEADING) if text is RTL and (text_length, text_length, LEADING) if text is LTR in Linux originally. But I remember Michael had some concerns of caret_pos being text_length in Windows, so we later restricted caret_pos to be [0, text_length) and changed implementation accordingly. It probably should be fine in Windows if we completely remove caret_pos. But it would be better that Mike could double check. I have a couple questions here: >RenderText::IsPointInSelection should now behave correctly when clicking on the trailing half of a glyph (it was a TODO). IsPointInSelection() did not work correctly before? Using the example in the TODO: In case of "abcFED", if "ED" is selected, |point| points to the right half of 'c', was it considered as point in selection? What I meant in the TODO is whether IsPointInSelection() should check the cursor's logical position is contained in the logical selection range (as what it is and after your change), or it should check the cursor's visual position is contained in the seletion's visual bounds (for the above example, click right of 'c' should not be treated as IsPointInSelection()). > Also, clicking on the trailing half now places the caret appropriately on Windows. Could you give an example of what is wrong without the CL? >Cursoring to the logical beginning of the text was buggy on Windows and should now behave like the mirror image of cursoring to the end. Could you give an example too? > I think it was theoretically buggy on Linux too, but it wasn't observable because the dominant text direction is determined by the first run (so the first logical glyph is always visually first too). That is correct in Linux. I briefly browsed the selection_model and part of render_text/_linux. Will take a closer look.
>IsPointInSelection() did not work correctly before? Before, in the case of abcFED, with ED selected, the right half of c counted as in the selection (because the code tested 3 <= 3 < 5). Now it is not in the selection (because the code tests Range(3, 5).Contains(Range(3, 2))). I think the new behavior is correct from both a visual and a logical perspective. It tests whether the glyph containing the point is part of the selection. >>Also, clicking on the trailing half now places the caret >>appropriately on Windows. > >Could you give an example of what is wrong without the CL? Actually, clicking on the right half of a character always places the caret to the left. I updated the description for this and the problem with cursoring to the left. >>I think it was theoretically buggy on Linux too, but it >>wasn't observable because the dominant text direction is >>determined by the first run (so the first logical glyph >>is always visually first too). > >That is correct in Linux. I know -- what I meant is that I saw funny behavior when I hardcoded RenderTextLinux::GetTextDirection to return LEFT_TO_RIGHT. I think this case is never hit normally, so it's not really a bug. http://codereview.chromium.org/9390022/diff/10005/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/9390022/diff/10005/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:414: // NB: exploits unsigned wraparound (0 - 1) On 2012/02/14 20:16:31, Alexei Svitkine wrote: > This is undefined behavior per the C++ standard Only signed arithmetic overflow is undefined. Unsigned arithmetic works modulo 2^n (WG14/N1124 section 6.2.5 paragraph 9). http://codereview.chromium.org/9390022/diff/10005/ui/gfx/selection_model.h File ui/gfx/selection_model.h (right): http://codereview.chromium.org/9390022/diff/10005/ui/gfx/selection_model.h#ne... ui/gfx/selection_model.h:80: UI_EXPORT std::ostream& operator<<(std::ostream& out, On 2012/02/14 20:16:31, Alexei Svitkine wrote: > I think the convention is to have a ToString() function. > e.g. see ui/gfx/range.h. I looked at Range when writing this, and it defines operator<<. I see that Insets, Point, Rect, and Size all define ToString instead. But when I tried switching to ToString, render_text_unittest.cc complained about the missing operator<<. What should I do? This overload is only here to make test failures easier to read.
> > http://codereview.chromium.**org/9390022/diff/10005/ui/gfx/** > render_text_linux.cc#**newcode414<http://codereview.chromium.org/9390022/diff/10005/ui/gfx/render_text_linux.cc#newcode414> > ui/gfx/render_text_linux.cc:**414: // NB: exploits unsigned wraparound (0 > - 1) > On 2012/02/14 20:16:31, Alexei Svitkine wrote: > >> This is undefined behavior per the C++ standard >> > > Only signed arithmetic overflow is undefined. Unsigned arithmetic works > modulo 2^n (WG14/N1124 section 6.2.5 paragraph 9). Ah, I see. Disregard my comment in that case then. http://codereview.chromium.**org/9390022/diff/10005/ui/gfx/** > selection_model.h#newcode80<http://codereview.chromium.org/9390022/diff/10005/ui/gfx/selection_model.h#newcode80> > ui/gfx/selection_model.h:80: UI_EXPORT std::ostream& > operator<<(std::ostream& out, > On 2012/02/14 20:16:31, Alexei Svitkine wrote: > >> I think the convention is to have a ToString() function. >> e.g. see ui/gfx/range.h. >> > > I looked at Range when writing this, and it defines operator<<. I see > that Insets, Point, Rect, and Size all define ToString instead. But when > I tried switching to ToString, render_text_unittest.cc complained about > the missing operator<<. What should I do? This overload is only here to > make test failures easier to read. > (I meant ui/gfx/rect.h in that comment..) Digging at the history for rect/point/etc it seems there was an effort to remove the operators and instead use ToString() - e.g. http://codereview.chromium.org/8044015 Is there a reason you can't just call ToString() in the relevant parts of render_text_unittest.cc?
This is pretty awesome, I always wanted SelectionModel to contain a single range, and I think it used to be that way before we discovered some problem I don't remember at the moment. In the future, be aware that it's easier on your reviewers to submit smaller, incremental changes. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:412: ui::Range range(std::min(model.selection().start(), text_length), nit: move range down to where it's used. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:416: // Make the cursor appear at the edge of the text, not next to the final nit: Wrap multi-line if blocks in braces, even for comments. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:609: return SelectionModel( nit: Move this return out of the else block. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:83: int width, height; Please explicitly init these locals. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:115: DCHECK(trailing >= 0); Use DCHECK_GE. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:116: if (trailing) { Why remove the explicit check for "> 0"? http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:123: Utf8IndexToUtf16Index(caret_pos), nit: move to the line above and fix the next line's indent. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:134: // Cursor not in any run: we're at the visual & logical edge. grammar nit: "The cursor is not". Also, use "and" instead of "&". http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:137: edge == SelectionModel(caret, selection.caret_affinity())) Use |selection| instead of constructing an identical SelectionModel. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:149: if (caret < run_end) { nit: inline the |run_end| value calc here. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:155: if (caret > run_start) { nit: inline the |run_start| value calc here and use "else if" above. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:205: DCHECK(range.GetMax() <= text().length()); DCHECK_LE http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:215: return CalculateSubstringBounds(range); Move this return out of the else block. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:151: // TODO(msw): Use the last visual run's font instead of the default font? This TODO can be removed or rewritten as something like: "Use the largest font instead of the default font?" http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:160: DCHECK(run_index < runs_.size()); DCHECK_LT http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:190: DCHECK(trailing >= 0); DCHECK_GE http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:208: edge == SelectionModel(caret, selection.caret_affinity())) Use |selection| instead of constructing an identical SelectionModel. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:210: else if (direction == CURSOR_RIGHT) nit: the two else cases can just be one with: run = (direction == CURSOR_RIGHT)? runs_.front() : runs_.back(); http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:225: } else { nit: make this an "else if" with the contained if. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/selection_model.cc File ui/gfx/selection_model.cc (right): http://codereview.chromium.org/9390022/diff/12022/ui/gfx/selection_model.cc#n... ui/gfx/selection_model.cc:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. nit: Remember to update the (c) dates here and elsewhere. http://codereview.chromium.org/9390022/diff/12022/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/9390022/diff/12022/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:251: model_->SelectSelectionModel(end_model); nit: Optionally set the selection start on |end_model| and always call model_->SelectSelectionModel(end_model); http://codereview.chromium.org/9390022/diff/12022/ui/views/controls/textfield... File ui/views/controls/textfield/textfield_views_model.cc (right): http://codereview.chromium.org/9390022/diff/12022/ui/views/controls/textfield... ui/views/controls/textfield/textfield_views_model.cc:367: ExecuteAndRecordDelete(ui::Range(cursor_position, cursor_position - 1), Hmm, shouldn't the range end be IndexOfAdjacentGrapheme(cursor_position, gfx::CURSOR_BACKWARD) corresponding with the behavior in TextfieldViewsModel::Delete? http://codereview.chromium.org/9390022/diff/12022/ui/views/controls/textfield... ui/views/controls/textfield/textfield_views_model.cc:393: return render_text_->SelectRange(range); nit: Optionally set the range on a SelectionModel and always call MoveCursorTo. http://codereview.chromium.org/9390022/diff/12022/ui/views/controls/textfield... ui/views/controls/textfield/textfield_views_model.cc:411: void TextfieldViewsModel::GetSelectedRange(ui::Range* range) const { Should this convenience function be deprecated? NativeTextfieldViews::GetSelectedRange could just assign GetRenderText()->selection(); to its |range| arg. http://codereview.chromium.org/9390022/diff/12022/ui/views/controls/textfield... File ui/views/controls/textfield/textfield_views_model_unittest.cc (right): http://codereview.chromium.org/9390022/diff/12022/ui/views/controls/textfield... ui/views/controls/textfield/textfield_views_model_unittest.cc:39: void MoveCursorTo(views::TextfieldViewsModel& model, size_t pos) { Eh, I'd prefer just inlining this code, but it's not important. http://codereview.chromium.org/9390022/diff/12022/ui/views/controls/textfield... ui/views/controls/textfield/textfield_views_model_unittest.cc:326: EXPECT_EQ(3U, model.GetCursorPosition()); It's probably safe to drop the redundant model.GetCursorPosition() checks here and below.
Also, set BUG=90426.
http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:415: if (model.caret_pos() > text_length) are "{" and "}" needed here for style correct-ness? http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:444: (sel.is_reversed() || (sel.is_empty() && sel.start() != 0)) ? why sel.start() == 0 is handled differently from sel.start() != 0 when sel.is_empty()? http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:795: caret_pos - 1 : caret_pos + 1; wrong indentation. theoretically, it should not be -/+1, should be the index of previous/next grapheme boundary, but it probably works fine in practice. so, (0, backward) and (text().length(), forward) will never be considered within range. Are (0, backward) and (text().length(), forward) reserved for visual boundary only and will never be returned otherwise? Which does not seems true, moving-cursor-left-right reaches those 2 models. Or is it because the passing in parameters (caret_pos, caret_affinity) are returned from FindCursorPosition(), and FindCursorPosition will never return (0, backward) and (text().length(), forward)? http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:146: size_t cursor_position() const { return selection_model_.caret_pos(); } is this style only used for getter of data members? for others, should be CamelCase. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:175: const ui::Range& selection() const { return selection_model_.selection(); } ditto http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:124: trailing ? CURSOR_BACKWARD : CURSOR_FORWARD); "trailing > 0" here too. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:139: else if (direction == CURSOR_RIGHT) why need to check the condition at line 136/137? and in which cases it reaches 'else if' and 'else'? http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:418: if (RangeContainsCaret(ui::Range(item->offset, item->offset + item->length), hm... what happens if you continuously press left/right arrow after reach visual boundary? http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_unittest.cc File ui/gfx/render_text_unittest.cc (right): http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_unittest... ui/gfx/render_text_unittest.cc:308: expected.push_back(SelectionModel(3, CURSOR_FORWARD)); add one more line which is a duplicate of line 308 to test that (3, FORWARD) is the ending movement. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_unittest... ui/gfx/render_text_unittest.cc:316: expected.push_back(SelectionModel(0, CURSOR_BACKWARD)); ditto. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_unittest... ui/gfx/render_text_unittest.cc:345: expected.push_back(SelectionModel(0, CURSOR_BACKWARD)); ditto. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_unittest... ui/gfx/render_text_unittest.cc:358: expected.push_back(SelectionModel(3, CURSOR_FORWARD)); ditto. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_unittest... ui/gfx/render_text_unittest.cc:366: expected.push_back(SelectionModel(0, CURSOR_BACKWARD)); ditto. And other similar vector patterns below. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_unittest... ui/gfx/render_text_unittest.cc:386: expected.push_back(SelectionModel(3, CURSOR_FORWARD)); ditto. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_unittest... ui/gfx/render_text_unittest.cc:388: expected.push_back(SelectionModel(0, CURSOR_BACKWARD)); ditto. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_unittest... ui/gfx/render_text_unittest.cc:403: expected.push_back(SelectionModel(0, CURSOR_BACKWARD)); ditto. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_unittest... ui/gfx/render_text_unittest.cc:435: expected.push_back(SelectionModel(0, CURSOR_BACKWARD)); ditto. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_unittest... ui/gfx/render_text_unittest.cc:449: expected.push_back(SelectionModel(0, CURSOR_BACKWARD)); ditto. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_unittest... ui/gfx/render_text_unittest.cc:474: expected.push_back(SelectionModel(3, CURSOR_FORWARD)); ditto. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_unittest... ui/gfx/render_text_unittest.cc:480: expected.push_back(SelectionModel(0, CURSOR_BACKWARD)); ditto. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_unittest... ui/gfx/render_text_unittest.cc:490: expected.push_back(SelectionModel(0, CURSOR_BACKWARD)); ditto. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/selection_model.h File ui/gfx/selection_model.h (right): http://codereview.chromium.org/9390022/diff/12022/ui/gfx/selection_model.h#ne... ui/gfx/selection_model.h:24: enum VisualCursorDirection { Again, I do not like the 'Cursor' part in those 2 enum names. It is the direction of cursor movement, not the direction of the cursor itself (in some implementation, such as in windows, cursor itself can have a triangle flag on top of it to indicates its direction, similar to, though not the same, our caret affinity). I'd appreciate the opinions of other reviewers. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/selection_model.h#ne... ui/gfx/selection_model.h:33: class UI_EXPORT SelectionModel { I think the comments from old code Line 15-42 should be kept and modified accordingly. http://codereview.chromium.org/9390022/diff/12022/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/9390022/diff/12022/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:242: size_t start_pos = GetRenderText()->FindCursorPosition(start).caret_pos(); s/caret_pos()/selection_start()/ http://codereview.chromium.org/9390022/diff/12022/ui/views/touchui/touch_sele... File ui/views/touchui/touch_selection_controller_impl_unittest.cc (right): http://codereview.chromium.org/9390022/diff/12022/ui/views/touchui/touch_sele... ui/views/touchui/touch_selection_controller_impl_unittest.cc:152: gfx::SelectionModel(3, gfx::CURSOR_PREVIOUS)); s/CURSOR_PREVIOUS/CURSOR_BACKWARD/
http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:418: if (RangeContainsCaret(ui::Range(item->offset, item->offset + item->length), On 2012/02/16 02:38:29, xji wrote: > hm... what happens if you continuously press left/right arrow after reach visual > boundary? forgot to update the comment. It is handled in moving left right function.
msw (and everybody), sorry about the complexity of the CL. Alexei: > Is there a reason you can't just call ToString() in the > relevant parts of render_text_unittest.cc? The problem is EXPECT_EQ does its own stringification. According to the documentation it requires either operator<<(ostream&, Type const&) or PrintTo(Type const&, ostream*). I just tried EXPECT_EQ(Rect(), Rect(1, 2, 3, 4)) and indeed the ToString method isn't called: the Rects are printed as raw bytes. I could stringify the arguments to EXPECT_EQ, but that seems really hacky. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:444: (sel.is_reversed() || (sel.is_empty() && sel.start() != 0)) ? On 2012/02/16 02:38:29, xji wrote: > why sel.start() == 0 is handled differently from > sel.start() != 0 when sel.is_empty()? The original code attached the cursor to the last character in the selection if the selection was nonempty, to the visual end if the selection was Range(text_length, text_length), and to the logically following character otherwise. The new code replicates this behavior except that it attaches the cursor to the visual beginning when the selection is Range(0, 0), because I wanted to treat the two ends of the text symmetrically. Fundamentally the problem is that the argument to this method doesn't provide enough information, forcing us to guess the rest. The solution is probably to replace it everywhere with MoveCursorTo or another method that takes a SelectionModel. I started to do this, but it touched a lot of code and this CL was complicated enough already. For now I changed the test to sel.is_reversed() || sel.is_empty(), which should match the old behavior. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:795: caret_pos - 1 : caret_pos + 1; On 2012/02/16 02:38:29, xji wrote: > theoretically, it should not be -/+1, should be the index > of previous/next grapheme boundary, but it probably works > fine in practice. It works in practice. It is kind of hacky. I'm thinking about maintaining a list of run boundaries in logical order and using std::{lower,upper}_bound to find the proper run in O(log n) time. Then this code could go away. > so, (0, backward) and (text().length(), forward) will > never be considered within range. Yes: in those cases the cursor is attached to the imaginary runs on either side of the text, so it's not in any text run. > Are (0, backward) and (text().length(), forward) reserved > for visual boundary only and will never be returned > otherwise? Which does not seems true, > moving-cursor-left-right reaches those 2 models. They refer to the visual-and-logical boundary. When you initially cursor to the visual edge you're attached to the character you just cursored over, which might not be the logically first/last character. If you press the arrow key once more, the cursor doesn't move visually, but it moves logically to the edge. That's represented by (0, backward) or (text().length(), forward). That's how the code worked before this CL, except for the issue with the start of the text on Windows. > Or is it because the passing in > parameters (caret_pos, caret_affinity) are returned from > FindCursorPosition(), and FindCursorPosition will never > return (0, backward) and (text().length(), forward)? FindCursorPosition returns those results when the given point is out of bounds to the left or right. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:146: size_t cursor_position() const { return selection_model_.caret_pos(); } On 2012/02/16 02:38:29, xji wrote: > is this style only used for getter of data members? for > others, should be CamelCase. The style guide says this_style is for members that "do so little work they're effectively free". The cost of these methods is the same as the cost of a simple getter (since selection_model_ is bodily contained in RenderText). http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:83: int width, height; On 2012/02/16 00:09:21, msw wrote: > Please explicitly init these locals. I'm not sure what to initialize them to. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:116: if (trailing) { On 2012/02/16 00:09:21, msw wrote: > Why remove the explicit check for "> 0"? No reason. Reverted. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:137: edge == SelectionModel(caret, selection.caret_affinity())) On 2012/02/16 00:09:21, msw wrote: > Use |selection| instead of constructing an identical SelectionModel. It's not identical since |selection| might have a nonempty selection range. But actually a much simpler test works here. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:139: else if (direction == CURSOR_RIGHT) On 2012/02/16 02:38:29, xji wrote: > why need to check the condition at line 136/137? > and in which cases it reaches 'else if' and 'else'? if (!run) then the caret is either at (0, backward) or (text_length, forward). If it's the correct side of the text then we're done (first case), but if it's the opposite side then we need to cursor into the text. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:155: if (caret > run_start) { On 2012/02/16 00:09:21, msw wrote: > nit: inline the |run_start| value calc here and use "else if" above. The then and else clauses here are mirror images. I think it's clearer if they have the same indentation. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_unittest.cc File ui/gfx/render_text_unittest.cc (right): http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_unittest... ui/gfx/render_text_unittest.cc:308: expected.push_back(SelectionModel(3, CURSOR_FORWARD)); On 2012/02/16 02:38:29, xji wrote: > add one more line which is a duplicate of line 308 to test > that (3, FORWARD) is the ending movement. I moved this test into RunMoveCursorLeftRightTest instead. I hope that's okay. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:151: // TODO(msw): Use the last visual run's font instead of the default font? On 2012/02/16 00:09:21, msw wrote: > This TODO can be removed or rewritten as something like: > "Use the largest font instead of the default font?" Changed. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:208: edge == SelectionModel(caret, selection.caret_affinity())) On 2012/02/16 00:09:21, msw wrote: > Use |selection| instead of constructing an identical > SelectionModel. (see render_text_linux.cc) http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:210: else if (direction == CURSOR_RIGHT) On 2012/02/16 00:09:21, msw wrote: > nit: the two else cases can just be one with: > run = (direction == CURSOR_RIGHT)? runs_.front() : runs_.back(); Done. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:225: } else { On 2012/02/16 00:09:21, msw wrote: > nit: make this an "else if" with the contained if. (see render_text_linux.cc) http://codereview.chromium.org/9390022/diff/12022/ui/gfx/selection_model.cc File ui/gfx/selection_model.cc (right): http://codereview.chromium.org/9390022/diff/12022/ui/gfx/selection_model.cc#n... ui/gfx/selection_model.cc:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2012/02/16 00:09:21, msw wrote: > nit: Remember to update the (c) dates here and elsewhere. I'm going to do it at the last moment because I was bitten by a spurious merge conflict in the copyright line once. I think PRESUBMIT enforces it so it's impossible to forget. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/selection_model.h File ui/gfx/selection_model.h (right): http://codereview.chromium.org/9390022/diff/12022/ui/gfx/selection_model.h#ne... ui/gfx/selection_model.h:24: enum VisualCursorDirection { On 2012/02/16 02:38:29, xji wrote: > Again, I do not like the 'Cursor' part in those 2 enum > names. It is the direction of cursor movement, not the > direction of the cursor itself [...] > I'd appreciate the opinions of other reviewers. I'm unhappy with these names, but also with all of the alternatives that I can think of. The affinity is not really about motion, just direction, so anything suggesting motion seems wrong for that. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/selection_model.h#ne... ui/gfx/selection_model.h:33: class UI_EXPORT SelectionModel { On 2012/02/16 02:38:29, xji wrote: > I think the comments from old code Line 15-42 should be > kept and modified accordingly. I haven't done this yet. I'd like to look more carefully at the bidi documents that are mentioned in bug 90426. http://codereview.chromium.org/9390022/diff/12022/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/9390022/diff/12022/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:242: size_t start_pos = GetRenderText()->FindCursorPosition(start).caret_pos(); On 2012/02/16 02:38:29, xji wrote: > s/caret_pos()/selection_start()/ FindCursorPosition returns a caret position with an empty selection. I think caret_pos() makes more sense. http://codereview.chromium.org/9390022/diff/12022/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:251: model_->SelectSelectionModel(end_model); On 2012/02/16 00:09:21, msw wrote: > nit: Optionally set the selection start on |end_model| and > always call model_->SelectSelectionModel(end_model); This is another place where I was just trying to preserve existing behavior, but thinking about it now, I think your change will preserve the observable behavior even though it sometimes leads to a different SelectionModel. I used set_selection_start(), which necessitated making it public. I don't know if that's a problem. The default assignment operator was public already. Actually, I think this method may be dead. It's only called from touch_selection_controller_impl.cc, which isn't listed in any GYP file. http://codereview.chromium.org/9390022/diff/12022/ui/views/controls/textfield... File ui/views/controls/textfield/textfield_views_model.cc (right): http://codereview.chromium.org/9390022/diff/12022/ui/views/controls/textfield... ui/views/controls/textfield/textfield_views_model.cc:367: ExecuteAndRecordDelete(ui::Range(cursor_position, cursor_position - 1), On 2012/02/16 00:09:21, msw wrote: > Hmm, shouldn't the range end be IndexOfAdjacentGrapheme(cursor_position, > gfx::CURSOR_BACKWARD) corresponding with the behavior in > TextfieldViewsModel::Delete? I think the idea is that Backspace should remove combining diacritics one by one, while Delete should remove the whole grapheme with all diacritics. This code should check for surrogate pairs -- I have a pending change that fixes that. It's possible that it should do other special things that I don't know about. http://codereview.chromium.org/9390022/diff/12022/ui/views/controls/textfield... ui/views/controls/textfield/textfield_views_model.cc:393: return render_text_->SelectRange(range); On 2012/02/16 00:09:21, msw wrote: > nit: Optionally set the range on a SelectionModel and always call MoveCursorTo. That would probably make sense. As I said elsewhere, I started to overhaul this code and eventually decided to leave it for another CL. It seems weird (probably a bug?) that this method behaves as though Shift is held down when there's composition text. http://codereview.chromium.org/9390022/diff/12022/ui/views/controls/textfield... ui/views/controls/textfield/textfield_views_model.cc:411: void TextfieldViewsModel::GetSelectedRange(ui::Range* range) const { On 2012/02/16 00:09:21, msw wrote: > Should this convenience function be deprecated? > NativeTextfieldViews::GetSelectedRange could just assign > GetRenderText()->selection(); to its |range| arg. Seems simple. Done. http://codereview.chromium.org/9390022/diff/12022/ui/views/controls/textfield... File ui/views/controls/textfield/textfield_views_model_unittest.cc (right): http://codereview.chromium.org/9390022/diff/12022/ui/views/controls/textfield... ui/views/controls/textfield/textfield_views_model_unittest.cc:39: void MoveCursorTo(views::TextfieldViewsModel& model, size_t pos) { On 2012/02/16 00:09:21, msw wrote: > Eh, I'd prefer just inlining this code, but it's not important. It seemed kind of verbose when the only interesting bit was the one-or-two-digit position. But just say the word and I'll inline it. http://codereview.chromium.org/9390022/diff/12022/ui/views/controls/textfield... ui/views/controls/textfield/textfield_views_model_unittest.cc:326: EXPECT_EQ(3U, model.GetCursorPosition()); On 2012/02/16 00:09:21, msw wrote: > It's probably safe to drop the redundant model.GetCursorPosition() checks here > and below. Done. http://codereview.chromium.org/9390022/diff/12022/ui/views/touchui/touch_sele... File ui/views/touchui/touch_selection_controller_impl_unittest.cc (right): http://codereview.chromium.org/9390022/diff/12022/ui/views/touchui/touch_sele... ui/views/touchui/touch_selection_controller_impl_unittest.cc:152: gfx::SelectionModel(3, gfx::CURSOR_PREVIOUS)); On 2012/02/16 02:38:29, xji wrote: > s/CURSOR_PREVIOUS/CURSOR_BACKWARD/ Done. I think this entire file may be dead. It doesn't seem to be mentioned in any .gyp file.
http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:444: (sel.is_reversed() || (sel.is_empty() && sel.start() != 0)) ? On 2012/02/16 09:20:55, benrg wrote: > On 2012/02/16 02:38:29, xji wrote: > > why sel.start() == 0 is handled differently from > > sel.start() != 0 when sel.is_empty()? > > The original code attached the cursor to the last character in the selection if > the selection was nonempty, to the visual end if the selection was > Range(text_length, text_length), and to the logically following character > otherwise. The new code replicates this behavior except that it attaches the > cursor to the visual beginning when the selection is Range(0, 0), because I > wanted to treat the two ends of the text symmetrically. I see. > > Fundamentally the problem is that the argument to this method doesn't provide > enough information, forcing us to guess the rest. The solution is probably to > replace it everywhere with MoveCursorTo or another method that takes a > SelectionModel. Yes, that is the problem. It should be replaced by a method that takes a pair of SelectionModel as argument, instead of Range (a pair of only caret_pos). And it needs to change upper layer to get/pass the correct caret_affinity. > I started to do this, but it touched a lot of code and this CL > was complicated enough already. For now I changed the test to sel.is_reversed() > || sel.is_empty(), which should match the old behavior. I am fine with either way. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:795: caret_pos - 1 : caret_pos + 1; On 2012/02/16 09:20:55, benrg wrote: > On 2012/02/16 02:38:29, xji wrote: > > theoretically, it should not be -/+1, should be the index > > of previous/next grapheme boundary, but it probably works > > fine in practice. > > It works in practice. It is kind of hacky. I'm thinking about maintaining a list > of run boundaries in logical order and using std::{lower,upper}_bound to find > the proper run in O(log n) time. Then this code could go away. I guess you are talking about replacing GetRunContainingCaret(). But RangeContainsCaret() is used in IsPointInSelection() as well. Thanks for the explanation. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:139: else if (direction == CURSOR_RIGHT) On 2012/02/16 09:20:55, benrg wrote: > On 2012/02/16 02:38:29, xji wrote: > > why need to check the condition at line 136/137? > > and in which cases it reaches 'else if' and 'else'? > > if (!run) then the caret is either at (0, backward) or (text_length, forward). > If it's the correct side of the text then we're done (first case), but if it's > the opposite side then we need to cursor into the text. Ha, I realized that. Thanks for the explanation. http://codereview.chromium.org/9390022/diff/12022/ui/gfx/selection_model.h File ui/gfx/selection_model.h (right): http://codereview.chromium.org/9390022/diff/12022/ui/gfx/selection_model.h#ne... ui/gfx/selection_model.h:24: enum VisualCursorDirection { On 2012/02/16 09:20:55, benrg wrote: > On 2012/02/16 02:38:29, xji wrote: > > Again, I do not like the 'Cursor' part in those 2 enum > > names. It is the direction of cursor movement, not the > > direction of the cursor itself [...] > > I'd appreciate the opinions of other reviewers. > > I'm unhappy with these names, but also with all of the alternatives that I can > think of. The affinity is not really about motion, just direction, so anything > suggesting motion seems wrong for that. ya, those names are originally for motion in cursor movement functions etc. and reused for caret affinity which is not motion, but more about direction. http://codereview.chromium.org/9390022/diff/12022/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/9390022/diff/12022/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:251: model_->SelectSelectionModel(end_model); On 2012/02/16 09:20:55, benrg wrote: > On 2012/02/16 00:09:21, msw wrote: > > nit: Optionally set the selection start on |end_model| and > > always call model_->SelectSelectionModel(end_model); > > This is another place where I was just trying to preserve existing behavior, but > thinking about it now, I think your change will preserve the observable behavior > even though it sometimes leads to a different SelectionModel. > > I used set_selection_start(), which necessitated making it public. I don't know > if that's a problem. The default assignment operator was public already. I remember the reason to make SelectionModel::set_selection_start() is that we do not want upper layers (except RenderText) to change that value arbitrarily without considering its impact to caret_affinity. for example, text "abcDEF", visually as "abcFED", if original selection is (1, 3), the caret affinity is BACKWARD, if set_selection_start as 5, the selection range changes to (5, 3), the affinity should be changed to FORWARD. > > Actually, I think this method may be dead. It's only called from > touch_selection_controller_impl.cc, which isn't listed in any GYP file. touch_selection_controller_impl.cc is disabled for now. I think varanjain will enable the functionality, but I do not know whether the code will remain the same. http://codereview.chromium.org/9390022/diff/12022/ui/views/touchui/touch_sele... File ui/views/touchui/touch_selection_controller_impl_unittest.cc (right): http://codereview.chromium.org/9390022/diff/12022/ui/views/touchui/touch_sele... ui/views/touchui/touch_selection_controller_impl_unittest.cc:152: gfx::SelectionModel(3, gfx::CURSOR_PREVIOUS)); On 2012/02/16 09:20:55, benrg wrote: > On 2012/02/16 02:38:29, xji wrote: > > s/CURSOR_PREVIOUS/CURSOR_BACKWARD/ > > Done. I think this entire file may be dead. It doesn't seem to be mentioned in > any .gyp file. it is disabled now.
http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/9390022/diff/12022/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:83: int width, height; On 2012/02/16 09:20:55, benrg wrote: > On 2012/02/16 00:09:21, msw wrote: > > Please explicitly init these locals. > > I'm not sure what to initialize them to. I see that you updated them to 0; that's better than garbage, thanks. (Just saying "Done" would have saved us both a little time) http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:444: sel.is_reversed() || sel.is_empty() ? CURSOR_FORWARD : CURSOR_BACKWARD; nit: Put parens around the condition here. http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:648: return SelectionModel(0, CURSOR_BACKWARD); Move this return out of the else block. http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:791: size_t adjacent = caret_affinity == CURSOR_BACKWARD ? nit: Put parens around the condition here. http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:123: trailing > 0 ? CURSOR_BACKWARD : CURSOR_FORWARD); nit: Put parens around the condition here. http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:136: run = direction == CURSOR_RIGHT ? nit: Put parens around the condition here. http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:411: position, affinity)) |affinity| should be on its own line. http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text_unittest.cc File ui/gfx/render_text_unittest.cc (right): http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text_unittest... ui/gfx/render_text_unittest.cc:335: expected.push_back(SelectionModel(6, CURSOR_FORWARD)); Replace the "expected.push_back(SelectionModel(6, CURSOR_FORWARD));" line here. It's duplicated to check cursor placement at the end of the text; same below. http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text_unittest... ui/gfx/render_text_unittest.cc:406: expected.push_back(SelectionModel(3, CURSOR_FORWARD)); Same here, replace the line to check EOL cursor movement. http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text_unittest... ui/gfx/render_text_unittest.cc:425: expected.push_back(SelectionModel(6, CURSOR_FORWARD)); Same here, replace the line to check EOL cursor movement. http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text_unittest... ui/gfx/render_text_unittest.cc:456: expected.push_back(SelectionModel(6, CURSOR_BACKWARD)); Same here, replace the line to check EOL cursor movement. http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text_unittest... ui/gfx/render_text_unittest.cc:494: expected.push_back(SelectionModel(3, CURSOR_FORWARD)); Same here, replace the line to check EOL cursor movement. http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:109: int x; Init this local to 0. http://codereview.chromium.org/9390022/diff/28022/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/9390022/diff/28022/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:243: selection.set_selection_start( I agree with xji's reasoning for keeping SelectionModel::set_selection_start() private. Change this back to your Patch Set 2 version; sorry for the bad tip.
+oshima for review
http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:208: virtual void GetGlyphBounds(size_t index, ui::Range* xspan, int* height) = 0; Does this need to be public? Also, please clarify in the comment what coordinate system these bounds are in. Specifically, that these do not take into account horizontal alignment / display offset (I believe). (See ToTextPoint() vs ToViewPoint()). http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:108: int GetGlyphXBoundary(internal::TextRun* run, size_t index, bool trailing) { Please add a comment for this function. http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:111: index - run->range.start(), Can you add DCHECKs that index >= run->range.start() and index - run->range.start() < run->range.length()?
In addition to the comments I fixed a regression in the overstrike behavior. Even with this fix the overstrike behavior is kind of broken (in the same way it was before). Cursoring left through aDCBe looks like this: aDCBe| aDCB* aD*Be a*CBe aDCB* *DCBe Do we need to support overstrike mode? I've tested a lot of other non-rich text edit controls and have yet to find one that supports it. It's not a feature of Chrome or any other major browser right now. Another thing that's not a regression but may be problematic is that there are situations where we may want the cursor to appear in a place that's not visually adjacent to either of its logical neighbors (even counting the imaginary edge runs). For example, if the text is abc[G[123]FED] and the user selects and deletes the G, and we want the cursor to remain visually and logically where the G used to be, we're in trouble. This can be solved by maintaining a cursor bidi level instead of an affinity. The FORWARD and BACKWARD affinities are equivalent to the special cases where the cursor level matches that of the glyph in that logical direction. I can't decide whether it's worth trying to replace the affinity with a level in this CL. It can probably wait. http://codereview.chromium.org/9390022/diff/12022/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/9390022/diff/12022/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:251: model_->SelectSelectionModel(end_model); On 2012/02/16 19:52:55, xji wrote: > I remember the reason to make SelectionModel::set_selection_start() > is that we do not want upper layers (except RenderText) to > change that value arbitrarily without considering its > impact to caret_affinity. > > for example, text "abcDEF", visually as "abcFED", if > original selection is (1, 3), the caret affinity is > BACKWARD, if set_selection_start as 5, the selection range > changes to (5, 3), the affinity should be changed to > FORWARD. I changed it back, but I'm not sure I believe that the affinity should depend on the selection in the way you describe. Consider abcF|ED -> Shift+Left -> abc|*ED -> Shift+Left -> ab|*F** -> Shift+Right -> ???. Currently it goes to abc|F** because we don't set the affinity according to the selection. If we did, the cursor would jump from ab|*F** to abcF**|. In practice it doesn't matter because we don't draw the cursor when there's a selection. Also, if my selection proposal (currently private) is adopted, this will basically be irrelevant. http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:444: sel.is_reversed() || sel.is_empty() ? CURSOR_FORWARD : CURSOR_BACKWARD; On 2012/02/21 21:13:39, msw wrote: > nit: Put parens around the condition here. Done (here and elsewhere), but note that Oshima often asks me to remove parentheses in these cases. :-) http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:648: return SelectionModel(0, CURSOR_BACKWARD); On 2012/02/21 21:13:39, msw wrote: > Move this return out of the else block. Done. http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:208: virtual void GetGlyphBounds(size_t index, ui::Range* xspan, int* height) = 0; On 2012/02/21 21:40:28, Alexei Svitkine wrote: > Does this need to be public? > > Also, please clarify in the comment what coordinate system these bounds are in. > Specifically, that these do not take into account horizontal alignment / display > offset (I believe). (See ToTextPoint() vs ToViewPoint()). I made it protected and fixed the comment. http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:411: position, affinity)) On 2012/02/21 21:13:39, msw wrote: > |affinity| should be on its own line. The style guide allows this ("When calling a function, feel free to collapse things to take less space"). I rearranged to avoid the problem. http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text_unittest.cc File ui/gfx/render_text_unittest.cc (right): http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text_unittest... ui/gfx/render_text_unittest.cc:335: expected.push_back(SelectionModel(6, CURSOR_FORWARD)); On 2012/02/21 21:13:39, msw wrote: > Replace the "expected.push_back(SelectionModel(6, CURSOR_FORWARD));" line > here. It's duplicated to check cursor placement at the end of the text; same > below. I moved this check into RunMoveCursorLeftRightTest; it now checks the last item twice. http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:108: int GetGlyphXBoundary(internal::TextRun* run, size_t index, bool trailing) { On 2012/02/21 21:40:28, Alexei Svitkine wrote: > Please add a comment for this function. Done. http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:109: int x; On 2012/02/21 21:13:39, msw wrote: > Init this local to 0. Done. http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:111: index - run->range.start(), On 2012/02/21 21:40:28, Alexei Svitkine wrote: > Can you add DCHECKs that index >= run->range.start() and index - > run->range.start() < run->range.length()? Done. http://codereview.chromium.org/9390022/diff/28022/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/9390022/diff/28022/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:243: selection.set_selection_start( On 2012/02/21 21:13:39, msw wrote: > I agree with xji's reasoning for keeping > SelectionModel::set_selection_start() private. Change this > back to your Patch Set 2 version; sorry for the bad tip. I rewrote it to no longer use set_selection_start, but I didn't restore the more complicated old logic (which I think was unnecessary -- see earlier comment). See also my response to xji.
On 2012/02/21 23:21:45, benrg wrote: > In addition to the comments I fixed a regression in the overstrike behavior. > Even with this fix the overstrike behavior is kind of broken (in the same way it > was before). Cursoring left through aDCBe looks like this: > > aDCBe| > aDCB* > aD*Be > a*CBe > aDCB* > *DCBe We are completely based on highlighting logically next character (next to caret_pos). I am not sure how helpful (intuitive) that is. Let's check with our bidi experts on the expected behavior. > > Do we need to support overstrike mode? I've tested a lot of other non-rich text > edit controls and have yet to find one that supports it. It's not a feature of > Chrome or any other major browser right now. Our linux supports it. It highlights 'B' when move cursor from "aD|CBe" to "a|DCBe". > > Another thing that's not a regression but may be problematic is that there are > situations where we may want the cursor to appear in a place that's not visually > adjacent to either of its logical neighbors (even counting the imaginary edge > runs). For example, if the text is abc[G[123]FED] and the user selects and > deletes the G, and we want the cursor to remain visually and logically where the > G used to be, we're in trouble. I am not sure whether we can completely avoid cursor jump around. For example, delete could delete character not adjacent to cursor. Mati changed his proposal to treat delete as a visual operation, which our native speaker expert do not like. And there is discussion on what to do on 'delete' when the character deleted is not adjacent to cursor. Whether make the first 'delete' as no-op by just moving the cursor and do actual deletion on second try. Seems that our native speaker experts do not like this approach either. >This can be solved by maintaining a cursor bidi > level instead of an affinity. The FORWARD and BACKWARD affinities are equivalent > to the special cases where the cursor level matches that of the glyph in that > logical direction. I can't decide whether it's worth trying to replace the > affinity with a level in this CL. It can probably wait. You've read a lot of Bidi related stuff these days! (I should read Mati's proposal again to see how to avoid cursor jump around). We should check with our native speaker experts to reach a consensus before implementing. http://codereview.chromium.org/9390022/diff/12022/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/9390022/diff/12022/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:251: model_->SelectSelectionModel(end_model); On 2012/02/21 23:21:45, benrg wrote: > On 2012/02/16 19:52:55, xji wrote: > > I remember the reason to make SelectionModel::set_selection_start() > > is that we do not want upper layers (except RenderText) to > > change that value arbitrarily without considering its > > impact to caret_affinity. > > > > for example, text "abcDEF", visually as "abcFED", if > > original selection is (1, 3), the caret affinity is > > BACKWARD, if set_selection_start as 5, the selection range > > changes to (5, 3), the affinity should be changed to > > FORWARD. > > I changed it back, but I'm not sure I believe that the affinity should depend on > the selection in the way you describe. Consider abcF|ED -> Shift+Left -> abc|*ED > -> Shift+Left -> ab|*F** -> Shift+Right -> ???. Currently it goes to abc|F** > because we don't set the affinity according to the selection. If we did, the > cursor would jump from ab|*F** to abcF**|. You are right in this case, in which we have explicitly set the (caret_pos, caret_affinity) of the selection's start and end point. when cursor is at "abcF|ED", the selection start's selection model is (4, BACKWARD) or (5, FORWARD). when shift+right from ab|*F**, the selection end is attached to the character it just passed over, so the selection model is (3, BACKWARD). But currently, we have SelectRange() which upper layer calls to just set the selection range without any affinity. That is what I meant the default affinity would take place. As you mentioned, SelectRange() is problematic. Ideally, we should never just set selection's start/end position without affinity. In UI, mouse or keyboard/cursor operation always indicate affinity. But I am not sure whether all of our upper layer code could be changed to calculate and pass in affinity for both selection start and selection end. > > In practice it doesn't matter because we don't draw the cursor when there's a > selection. It matters when we collapse selection, for example, press left (or right) arrow when there is selection. > Also, if my selection proposal (currently private) is adopted, this > will basically be irrelevant. the proposal you emailed Aharon?
LGTM with following comments addressed. http://codereview.chromium.org/9390022/diff/34024/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/9390022/diff/34024/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:568: // In overstrike mode, ignore the affinity and always indicate that we will nit: I looked up in wiki, http://en.wikipedia.org/wiki/Insert_key. Probably should replace 'overstrike' with 'overtype' here and in other places. http://codereview.chromium.org/9390022/diff/34024/ui/gfx/selection_model.cc File ui/gfx/selection_model.cc (right): http://codereview.chromium.org/9390022/diff/34024/ui/gfx/selection_model.cc#n... ui/gfx/selection_model.cc:32: bool backward = sel.caret_affinity() == CURSOR_FORWARD; s/CURSOR_FORWARD/CURSOR_BACKWARD/ http://codereview.chromium.org/9390022/diff/34024/ui/gfx/selection_model.h File ui/gfx/selection_model.h (right): http://codereview.chromium.org/9390022/diff/34024/ui/gfx/selection_model.h#ne... ui/gfx/selection_model.h:33: class UI_EXPORT SelectionModel { Please add comments.
http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text_unittest.cc File ui/gfx/render_text_unittest.cc (right): http://codereview.chromium.org/9390022/diff/28022/ui/gfx/render_text_unittest... ui/gfx/render_text_unittest.cc:335: expected.push_back(SelectionModel(6, CURSOR_FORWARD)); On 2012/02/21 23:21:45, benrg wrote: > On 2012/02/21 21:13:39, msw wrote: > > Replace the "expected.push_back(SelectionModel(6, CURSOR_FORWARD));" line > > here. It's duplicated to check cursor placement at the end of the text; same > > below. > > I moved this check into RunMoveCursorLeftRightTest; it now checks the last item > twice. Awesome! http://codereview.chromium.org/9390022/diff/34024/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/9390022/diff/34024/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:458: SetSelectionModel(SelectionModel(selection_model_.caret_pos(), Use cursor_position() instead of selection_model_.caret_pos(). http://codereview.chromium.org/9390022/diff/34024/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:472: size_t cursor_position = selection_model_.caret_pos(); Use cursor_position() instead of selection_model_.caret_pos(). http://codereview.chromium.org/9390022/diff/34024/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:572: int x, width, height; Init these locals to 0. http://codereview.chromium.org/9390022/diff/34024/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:578: x = 0; With x init to 0, rewrite this to just handle the else case. http://codereview.chromium.org/9390022/diff/34024/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:581: width = 0; Unnecessary with width init to 0. http://codereview.chromium.org/9390022/diff/34024/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:590: width = 0; Unnecessary with width init to 0. http://codereview.chromium.org/9390022/diff/34024/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:607: const ui::Range& selection = selection_model_.selection(); Use selection() instead of selection_model_.selection(). http://codereview.chromium.org/9390022/diff/34024/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:801: ui::Range pos(cursor); nit: optionally: ui::Range pos(select ? selection().start() : cursor, cursor); http://codereview.chromium.org/9390022/diff/34024/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:804: LogicalCursorDirection affinity = cursor ? CURSOR_BACKWARD : CURSOR_FORWARD; nit: I'd prefer explicit "(cursor != 0) ?", but also like single-line ops... http://codereview.chromium.org/9390022/diff/34024/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/9390022/diff/34024/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:175: const ui::Range& selection() const { return selection_model_.selection(); } nit: move this up with cursor_position() http://codereview.chromium.org/9390022/diff/34024/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/9390022/diff/34024/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:102: int caret_pos, trailing; Init these locals to 0. http://codereview.chromium.org/9390022/diff/34024/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): http://codereview.chromium.org/9390022/diff/34024/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:222: // NB: exploits unsigned wraparound (WG14/N1124 section 6.2.5 paragraph 9). nit: Just make visual_index an int and check for < 0 at line 225. http://codereview.chromium.org/9390022/diff/34024/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:223: visual_index = nit: Make this: visual_index += (direction == CURSOR_LEFT) ? -1 : 1; http://codereview.chromium.org/9390022/diff/34024/ui/gfx/selection_model.h File ui/gfx/selection_model.h (right): http://codereview.chromium.org/9390022/diff/34024/ui/gfx/selection_model.h#ne... ui/gfx/selection_model.h:56: void set_selection_start(size_t pos) { selection_.set_start(pos); } Add a comment about why this is private, in case we forget again. http://codereview.chromium.org/9390022/diff/34024/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/9390022/diff/34024/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:727: *range = sel.selection(); nit: Just call GetSelectedRange(range); nix |sel|/GetSelectionModel().
msw, PTAL. https://chromiumcodereview.appspot.com/9390022/diff/34024/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://chromiumcodereview.appspot.com/9390022/diff/34024/ui/gfx/render_text.... ui/gfx/render_text.cc:458: SetSelectionModel(SelectionModel(selection_model_.caret_pos(), On 2012/02/28 22:51:32, msw wrote: > Use cursor_position() instead of selection_model_.caret_pos(). Done. https://chromiumcodereview.appspot.com/9390022/diff/34024/ui/gfx/render_text.... ui/gfx/render_text.cc:472: size_t cursor_position = selection_model_.caret_pos(); On 2012/02/28 22:51:32, msw wrote: > Use cursor_position() instead of selection_model_.caret_pos(). Done. https://chromiumcodereview.appspot.com/9390022/diff/34024/ui/gfx/render_text.... ui/gfx/render_text.cc:568: // In overstrike mode, ignore the affinity and always indicate that we will On 2012/02/24 20:16:52, xji wrote: > nit: I looked up in wiki, http://en.wikipedia.org/wiki/Insert_key. Probably > should replace 'overstrike' with 'overtype' here and in other places. Done. https://chromiumcodereview.appspot.com/9390022/diff/34024/ui/gfx/render_text.... ui/gfx/render_text.cc:572: int x, width, height; On 2012/02/28 22:51:32, msw wrote: > Init these locals to 0. For the record, I don't like doing this because it can hide bugs that the compiler would otherwise catch through dataflow analysis. But done. It does shorten the code in this case. https://chromiumcodereview.appspot.com/9390022/diff/34024/ui/gfx/render_text.... ui/gfx/render_text.cc:578: x = 0; On 2012/02/28 22:51:32, msw wrote: > With x init to 0, rewrite this to just handle the else case. Done. https://chromiumcodereview.appspot.com/9390022/diff/34024/ui/gfx/render_text.... ui/gfx/render_text.cc:581: width = 0; On 2012/02/28 22:51:32, msw wrote: > Unnecessary with width init to 0. Done. https://chromiumcodereview.appspot.com/9390022/diff/34024/ui/gfx/render_text.... ui/gfx/render_text.cc:590: width = 0; On 2012/02/28 22:51:32, msw wrote: > Unnecessary with width init to 0. Done. https://chromiumcodereview.appspot.com/9390022/diff/34024/ui/gfx/render_text.... ui/gfx/render_text.cc:607: const ui::Range& selection = selection_model_.selection(); On 2012/02/28 22:51:32, msw wrote: > Use selection() instead of selection_model_.selection(). Done. https://chromiumcodereview.appspot.com/9390022/diff/34024/ui/gfx/render_text.... ui/gfx/render_text.cc:801: ui::Range pos(cursor); On 2012/02/28 22:51:32, msw wrote: > nit: optionally: ui::Range pos(select ? selection().start() : cursor, cursor); Done. https://chromiumcodereview.appspot.com/9390022/diff/34024/ui/gfx/render_text.... ui/gfx/render_text.cc:804: LogicalCursorDirection affinity = cursor ? CURSOR_BACKWARD : CURSOR_FORWARD; On 2012/02/28 22:51:32, msw wrote: > nit: I'd prefer explicit "(cursor != 0) ?", but also like single-line ops... Do you like my rewritten version? https://chromiumcodereview.appspot.com/9390022/diff/34024/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://chromiumcodereview.appspot.com/9390022/diff/34024/ui/gfx/render_text.... ui/gfx/render_text.h:175: const ui::Range& selection() const { return selection_model_.selection(); } On 2012/02/28 22:51:32, msw wrote: > nit: move this up with cursor_position() Done. I also moved selection_model(). https://chromiumcodereview.appspot.com/9390022/diff/34024/ui/gfx/render_text_... File ui/gfx/render_text_linux.cc (right): https://chromiumcodereview.appspot.com/9390022/diff/34024/ui/gfx/render_text_... ui/gfx/render_text_linux.cc:102: int caret_pos, trailing; On 2012/02/28 22:51:32, msw wrote: > Init these locals to 0. Done (under protest -- though I don't feel all that strongly about this). https://chromiumcodereview.appspot.com/9390022/diff/34024/ui/gfx/render_text_... File ui/gfx/render_text_win.cc (right): https://chromiumcodereview.appspot.com/9390022/diff/34024/ui/gfx/render_text_... ui/gfx/render_text_win.cc:222: // NB: exploits unsigned wraparound (WG14/N1124 section 6.2.5 paragraph 9). On 2012/02/28 22:51:32, msw wrote: > nit: Just make visual_index an int and check for < 0 at line 225. Yeah, okay. https://chromiumcodereview.appspot.com/9390022/diff/34024/ui/gfx/render_text_... ui/gfx/render_text_win.cc:223: visual_index = On 2012/02/28 22:51:32, msw wrote: > nit: Make this: visual_index += (direction == CURSOR_LEFT) ? -1 : 1; Done. https://chromiumcodereview.appspot.com/9390022/diff/34024/ui/gfx/selection_mo... File ui/gfx/selection_model.cc (right): https://chromiumcodereview.appspot.com/9390022/diff/34024/ui/gfx/selection_mo... ui/gfx/selection_model.cc:32: bool backward = sel.caret_affinity() == CURSOR_FORWARD; On 2012/02/24 20:16:52, xji wrote: > s/CURSOR_FORWARD/CURSOR_BACKWARD/ Oops. https://chromiumcodereview.appspot.com/9390022/diff/34024/ui/gfx/selection_mo... File ui/gfx/selection_model.h (right): https://chromiumcodereview.appspot.com/9390022/diff/34024/ui/gfx/selection_mo... ui/gfx/selection_model.h:33: class UI_EXPORT SelectionModel { On 2012/02/24 20:16:52, xji wrote: > Please add comments. I restored your original comment because actually it seems fine as a description of the new SelectionModel. https://chromiumcodereview.appspot.com/9390022/diff/34024/ui/gfx/selection_mo... ui/gfx/selection_model.h:56: void set_selection_start(size_t pos) { selection_.set_start(pos); } On 2012/02/28 22:51:32, msw wrote: > Add a comment about why this is private, in case we forget again. I added a comment, but to be honest I'm not sure I get it. It was supposed to be because you shouldn't set the selection start without also updating the caret affinity, but the two uses of this method in RenderText *do* set the selection start without updating the affinity. If that's wrong, as the comment implies, then we ought to fix it, after which this method could be removed. For that matter, should SelectionModel(range, affinity) be public? Right now anybody can simulate sel.set_selection_start(foo) by doing sel = SelectionModel(Range(foo, sel.caret_pos()), sel.caret_affinity()). Even RenderText could do that... https://chromiumcodereview.appspot.com/9390022/diff/34024/ui/views/controls/t... File ui/views/controls/textfield/native_textfield_views.cc (right): https://chromiumcodereview.appspot.com/9390022/diff/34024/ui/views/controls/t... ui/views/controls/textfield/native_textfield_views.cc:727: *range = sel.selection(); On 2012/02/28 22:51:32, msw wrote: > nit: Just call GetSelectedRange(range); nix |sel|/GetSelectionModel(). Done.
https://chromiumcodereview.appspot.com/9390022/diff/34024/ui/gfx/selection_mo... File ui/gfx/selection_model.h (right): https://chromiumcodereview.appspot.com/9390022/diff/34024/ui/gfx/selection_mo... ui/gfx/selection_model.h:56: void set_selection_start(size_t pos) { selection_.set_start(pos); } That's correct; users should flip the affinity when the selection directionality changes *and* the selection start is at a BiDi level change. Add this requirement as a comment with Xiaomei's following example: Given "abcDEF" (visually "abcFED"), with original selection (1, 3, BACKWARD): set_selection_start(5) should also change the affinity to yield (5, 3, FORWARD). RenderText::MoveCursor[To] (4 total functions, we should disambiguate the overrides...) should be audited, it appears they fail to do this. Your point about the ctor is legitimate too, and I don't see any uses in NativeTextfieldViews nor TextfieldViewsModel, so it should be easy to make that ctor private, and give it the same note. We should also write tests to ensure this is happening correctly. https://chromiumcodereview.appspot.com/9390022/diff/62021/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://chromiumcodereview.appspot.com/9390022/diff/62021/ui/gfx/render_text.... ui/gfx/render_text.cc:427: if (model.caret_pos() > text_length) { When is this true? What tries to move the cursor to a position beyond the text length, and why do we support it?
https://chromiumcodereview.appspot.com/9390022/diff/34024/ui/gfx/selection_mo... File ui/gfx/selection_model.h (right): https://chromiumcodereview.appspot.com/9390022/diff/34024/ui/gfx/selection_mo... ui/gfx/selection_model.h:56: void set_selection_start(size_t pos) { selection_.set_start(pos); } Gah, correction: when the "selection *end* is at a BiDi level change".
https://chromiumcodereview.appspot.com/9390022/diff/34024/ui/gfx/selection_mo... File ui/gfx/selection_model.h (right): https://chromiumcodereview.appspot.com/9390022/diff/34024/ui/gfx/selection_mo... ui/gfx/selection_model.h:56: void set_selection_start(size_t pos) { selection_.set_start(pos); } On 2012/03/07 01:04:44, benrg wrote: > On 2012/02/28 22:51:32, msw wrote: > > Add a comment about why this is private, in case we forget again. > > I added a comment, but to be honest I'm not sure I get it. It was supposed to be > because you shouldn't set the selection start without also updating the caret > affinity, but the two uses of this method in RenderText *do* set the selection > start without updating the affinity. If that's wrong, as the comment implies, > then we ought to fix it, after which this method could be removed. What I mean is that if we explicitly know where the caret_affinity should be (such as when mouse click), we do not change it in set_selectiion_start(). Those should be the cases in RenderText. If we do not, such as in SelectRange() (which we already agreed that it has design flaw, and it should never happen), caret_affinity is determined by selection_start/end range. We do not want to expose set_selection_start to be public because we do not want users to call it randomly without knowing what he is doing. (And I think we already agreed that we need (caret_pos, caret_affinity) for both selection_end and selection_start, so there wont be this problem, and the issue of SelectionRange()). Looking at all the usage of set_selection_start() in RenderText: 1. In MoveCursor(BreakType, VisualCursorDirection, bool), the carate_pos and caret_affinity should be determined by either GetAdjacentSelectionModel() or , GetSelectionModelForSelectionStart(). So, set_selection_start() at the end should not affect them. 2. in MoveCursorTo(const SelectionModel&), set_selection_start is an error handling step. 3. in MoveCursorTo(const Point&, bool) caret_pos and caret_affinity is determined by FindCursorPosition(). And final set_selection_start should not affect them. 4. ClearSelection() should be fine. 5. SelectAll() should be fine. 6. SetSelectionModel() should be fine. https://chromiumcodereview.appspot.com/9390022/diff/62021/ui/gfx/selection_mo... File ui/gfx/selection_model.h (right): https://chromiumcodereview.appspot.com/9390022/diff/62021/ui/gfx/selection_mo... ui/gfx/selection_model.h:60: // pointing to right of 'D'. This paragraph probably need re-write since you are no longer use 'information to specify to which character and on which edge' the visual cursor is bound to.
Saintlou understandably wants me to work on higher-priority bugs, so I think I should either check this in more or less as-is and fix remaining problems later, or else put it on ice for a couple of weeks. http://codereview.chromium.org/9390022/diff/62021/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/9390022/diff/62021/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:427: if (model.caret_pos() > text_length) { On 2012/03/08 18:07:19, msw wrote: > When is this true? What tries to move the cursor to a position beyond the text > length, and why do we support it? I think that nobody should, but I'm nervous about changing this because I don't understand every call site. For example OmniboxViewViews calls it indirectly and I don't know what's going on with autocompletion, etc. Even if all SelectionModel constructors were private, a client could accidentally or deliberately apply a now-invalid saved selection after changing the text. It might be worth making SelectionModel completely private and force clients to do everything, including changing the text, through operations like ReplaceSelection(). http://codereview.chromium.org/9390022/diff/62021/ui/gfx/selection_model.h File ui/gfx/selection_model.h (right): http://codereview.chromium.org/9390022/diff/62021/ui/gfx/selection_model.h#ne... ui/gfx/selection_model.h:60: // pointing to right of 'D'. On 2012/03/08 20:30:02, xji wrote: > This paragraph probably need re-write since you are no longer use 'information > to specify to which character and on which edge' the visual cursor is bound > to. Well, the cursor is bound to the trailing side of 'c' whether that's represented by (2, TRAILING) or (3, BACKWARD), but maybe you're right. I rewrote it.
I think you addressed all my major concerns (except below). LGTM, but be prepared to fix any potential regressions. http://codereview.chromium.org/9390022/diff/62021/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/9390022/diff/62021/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:427: if (model.caret_pos() > text_length) { Let's just remove this; in the worst case, a caller with garbage data will wind up with the wrong affinity.
Ben, do you have time to look at this? http://codereview.chromium.org/9390022/diff/62021/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/9390022/diff/62021/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:427: if (model.caret_pos() > text_length) { On 2012/03/08 21:33:57, msw wrote: > Let's just remove this; in the worst case, a caller with garbage data will > wind up with the wrong affinity. Done.
http://codereview.chromium.org/9390022/diff/70022/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): http://codereview.chromium.org/9390022/diff/70022/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:144: DCHECK_GE(index, run->range.start()) Add ; http://codereview.chromium.org/9390022/diff/70022/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:145: DCHECK_LT(index, run->range.end()) Add ; http://codereview.chromium.org/9390022/diff/70022/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:239: else Remove the else if you're returning in the if.
http://codereview.chromium.org/9390022/diff/70022/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): http://codereview.chromium.org/9390022/diff/70022/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:144: DCHECK_GE(index, run->range.start()) These have been broken since Patch Set 4? :( Please build and test your code on affected platforms...
I don't really have any idea how any of this text rendering works, so I'll just support xji & msw here and give an owners' rubber stamp LGTM.
http://codereview.chromium.org/9390022/diff/70022/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): http://codereview.chromium.org/9390022/diff/70022/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:144: DCHECK_GE(index, run->range.start()) On 2012/03/08 22:37:48, msw wrote: > These have been broken since Patch Set 4? :( > Please build and test your code on affected platforms... In my defense, the only changes after patch set 3 are these two DCHECKS, initializing x=0, and changing the visibility of a method. I should probably run the trybots more often. I'll manually test again before committing. http://codereview.chromium.org/9390022/diff/70022/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:239: else On 2012/03/08 22:32:15, Alexei Svitkine wrote: > Remove the else if you're returning in the if. Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benrg@chromium.org/9390022/82002
Try job failure for 9390022-82002 (retry) on linux_chromeos for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benrg@chromium.org/9390022/86003
Try job failure for 9390022-86003 (retry) on linux_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benrg@chromium.org/9390022/86003
Try job failure for 9390022-86003 (retry) on win_rel for step "check_deps". It's a second try, previously, step "check_deps" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benrg@chromium.org/9390022/86003
Change committed as 126287 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
