Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7461102:
modification to RenderText for inheritance/SelectionModel (Closed)
|
Created:
9 years, 5 months ago by xji Modified:
9 years, 4 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
Descriptionextend RenderText for inheritance. It
1. Moves temporary color definition to gfx, declares some function as virtual for override,
removes const from some functions so that the override function is able to modify local data.
2. Cache cursor bounds and compute it (along with display_offset_) when necessary.
3. Introduce SelectionModel (not derivable) for visual cursor positioning.
BUG=90426
TEST=--use-pure-views text editing
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95508
Patch Set 1 #Patch Set 2 : '' #
Total comments: 20
Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #
Total comments: 61
Patch Set 7 : '' #Patch Set 8 : sync and change non-const reference to pointer #
Total comments: 43
Patch Set 9 : replace isalnum with u_isalnum which takes care of non-ascii #Patch Set 10 : update per comments, sync and add SetMergedSelectionModel/SelectSelectionModel for touch #Patch Set 11 : add native_textfield_views.h into CL #Patch Set 12 : fix a lint error #
Total comments: 16
Patch Set 13 : '' #
Total comments: 22
Patch Set 14 : update per comments #
Messages
Total messages: 16 (0 generated)
http://codereview.chromium.org/7461102/diff/4001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/7461102/diff/4001/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:317: StyleRanges style_ranges; Use the copy constructor with |style_ranges_|, as was done previously. http://codereview.chromium.org/7461102/diff/4001/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:402: gfx::Rect RenderText::GetCursorBounds(size_t cursor_pos, Single line. http://codereview.chromium.org/7461102/diff/4001/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:415: bool move_by_word) { Single line. http://codereview.chromium.org/7461102/diff/4001/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:450: bool move_by_word) { Single line. http://codereview.chromium.org/7461102/diff/4001/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:488: void RenderText::ApplyCompositionSelectionOverride( This definition is out of order from the declarations. Sadly, I made the same mistake with the ctor and dtor, would you mind reordering these to match their declarations? http://codereview.chromium.org/7461102/diff/4001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/7461102/diff/4001/ui/gfx/render_text.h#newcode29 ui/gfx/render_text.h:29: // Color settings for text, backgrounds and cursor. Why do we need to include these colors in the gfx namespace? Any file that includes render_text.h can access these from their anonymous namespace. http://codereview.chromium.org/7461102/diff/4001/ui/gfx/render_text.h#newcode168 ui/gfx/render_text.h:168: bool move_by_word); These two declarations each fit on a single line now. http://codereview.chromium.org/7461102/diff/4001/ui/gfx/render_text.h#newcode172 ui/gfx/render_text.h:172: void ApplyCompositionSelectionOverride(StyleRanges* style_ranges) const; Add a comment here. Perhaps rename ApplyCompositionAndSelectionStyles? Pass by reference (for parity with ApplyStyleRangeImpl) rather than pointer.
http://codereview.chromium.org/7461102/diff/4001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/7461102/diff/4001/ui/gfx/render_text.h#newcode29 ui/gfx/render_text.h:29: // Color settings for text, backgrounds and cursor. On 2011/07/27 19:34:47, msw wrote: > Why do we need to include these colors in the gfx namespace? Any file that > includes render_text.h can access these from their anonymous namespace. Seems like I missed this in the first review, but anonymous namespace in header file doesn't make sense. It happens to work because they're const but i believe this is something we recommend in chrome. We should put them all in gfx if this is used in other places, even better, put it in internal namespace so that application doesn't use it. (or putting them in separate header is another solution) http://codereview.chromium.org/7461102/diff/4001/ui/gfx/render_text.h#newcode138 ui/gfx/render_text.h:138: virtual int GetStringWidth(); If this is the reason why we had to remove const from other method, please explain it in the CL description so that we know why we're doing. It'd also allow us to give early feedback.
http://codereview.chromium.org/7461102/diff/4001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/7461102/diff/4001/ui/gfx/render_text.h#newcode29 ui/gfx/render_text.h:29: // Color settings for text, backgrounds and cursor. On 2011/07/28 08:53:04, oshima wrote: > On 2011/07/27 19:34:47, msw wrote: > > Why do we need to include these colors in the gfx namespace? Any file that > > includes render_text.h can access these from their anonymous namespace. > > Seems like I missed this in the first review, but anonymous namespace in header > file doesn't make sense. It happens to work because they're const but i believe > this is something we recommend in chrome. We should put them all in gfx if this should read this is NOT something we recommend. sorry for typo. > is used in other places, even better, put it in internal namespace so that > application doesn't use it. (or putting them in separate header is another > solution) > >
http://codereview.chromium.org/7461102/diff/4001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/7461102/diff/4001/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:317: StyleRanges style_ranges; On 2011/07/27 19:34:47, msw wrote: > Use the copy constructor with |style_ranges_|, as was done previously. Done. http://codereview.chromium.org/7461102/diff/4001/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:402: gfx::Rect RenderText::GetCursorBounds(size_t cursor_pos, On 2011/07/27 19:34:47, msw wrote: > Single line. Done. http://codereview.chromium.org/7461102/diff/4001/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:415: bool move_by_word) { On 2011/07/27 19:34:47, msw wrote: > Single line. The signature changed. not applicable anymore. http://codereview.chromium.org/7461102/diff/4001/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:450: bool move_by_word) { On 2011/07/27 19:34:47, msw wrote: > Single line. ditto. http://codereview.chromium.org/7461102/diff/4001/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:488: void RenderText::ApplyCompositionSelectionOverride( On 2011/07/27 19:34:47, msw wrote: > This definition is out of order from the declarations. Sadly, I made the same > mistake with the ctor and dtor, would you mind reordering these to match their > declarations? Done. http://codereview.chromium.org/7461102/diff/4001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/7461102/diff/4001/ui/gfx/render_text.h#newcode29 ui/gfx/render_text.h:29: // Color settings for text, backgrounds and cursor. On 2011/07/28 08:53:04, oshima wrote: > On 2011/07/27 19:34:47, msw wrote: > > Why do we need to include these colors in the gfx namespace? Any file that > > includes render_text.h can access these from their anonymous namespace. > > Seems like I missed this in the first review, but anonymous namespace in header > file doesn't make sense. It happens to work because they're const but i believe > this is something we recommend in chrome. We should put them all in gfx if this > is used in other places, even better, put it in internal namespace so that > application doesn't use it. (or putting them in separate header is another > solution) > > Can I leave them as is (in gfx namespace for share in subclass) since they are tentative settings and will be removed? http://codereview.chromium.org/7461102/diff/4001/ui/gfx/render_text.h#newcode138 ui/gfx/render_text.h:138: virtual int GetStringWidth(); On 2011/07/28 08:53:04, oshima wrote: > If this is the reason why we had to remove const from other method, please > explain it in the CL description so that we know why we're doing. It'd also > allow us to give early feedback. Done. http://codereview.chromium.org/7461102/diff/4001/ui/gfx/render_text.h#newcode168 ui/gfx/render_text.h:168: bool move_by_word); On 2011/07/27 19:34:47, msw wrote: > These two declarations each fit on a single line now. signature changed. not applicable anymore. http://codereview.chromium.org/7461102/diff/4001/ui/gfx/render_text.h#newcode172 ui/gfx/render_text.h:172: void ApplyCompositionSelectionOverride(StyleRanges* style_ranges) const; On 2011/07/27 19:34:47, msw wrote: > Add a comment here. Perhaps rename ApplyCompositionAndSelectionStyles? done. > Pass by reference (for parity with ApplyStyleRangeImpl) rather than pointer. I used (StyleRange& style_ranges), but it did not pass olint. It reports a style error as: "Is this a non-const reference? If so, make const or use a pointer.". I will just ignore the lint error.
I'm sorry for the deluge of feedback, but it is what it is. We can discuss this tomorrow if you'd like. http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:88: SelectionModel::SelectionModel() { These constructors should have initialization lists. http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:148: SetSelection(position, position); outdent two spaces. http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:193: bool RenderText::MoveCursorTo(const SelectionModel& selection, bool select) { This should essentially become SetSelectionModel or SetCursorAndSelection; and we can just copy values by assigning |selection| to |cursor_and_selection_|. http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:209: void RenderText::SetSelection(size_t start, size_t end) { With a revised MoveCursorTo, we may want to rename or change this function as well. http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:269: return composition_range_; outdent 2 spaces http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:450: return position == 0? SelectionModel(position) : This code ought to be deprecated soon, but please change this to: return SelectionModel(std::max(position - 1, 0)); http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcode62 ui/gfx/render_text.h:62: // For bidirectional text, the mappging between visual position and logical "bi-directional" and "mapping" http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcode64 ui/gfx/render_text.h:64: // letter stands for Hebrew, the visual display is "abcFED". According to the "capital *letters stand*" or "*a* capital letter stands". I would move this sentence with the example string below with the rest of the example. http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcode65 ui/gfx/render_text.h:65: // bidi editing guide: Perhaps we should formally post the bidi editing guide somewhere, otherwise this mention is meaningless. http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcode73 ui/gfx/render_text.h:73: // pointing to the right half of 'c' and pointing to the right half of 'D' both "*P*ointing" http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcode74 ui/gfx/render_text.h:74: // set cursor logical position as 3. But the cursor displayed visually at "set *the* logical cursor position *to*"... "the cursor *is* displayed" http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcode76 ui/gfx/render_text.h:76: // pointing right half of 'c' displays cursor right of 'c' as "abc|FED". "*P*ointing *to the* right"... "displays *the* cursor *to the* right". Same for the next line. If you want to keep these as single lines, maybe write: "Pointing to the right side of 'c' yields the visual cursor position "abc|FED."" http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcode78 ui/gfx/render_text.h:78: // So, besides the logical selection start point and end point, we need extra "point*s*" http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcode80 ui/gfx/render_text.h:80: // the visual cursor is bound to. For example, the visual cursor is bounds to "bound" http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcode81 ui/gfx/render_text.h:81: // the trailing side of the 2nd character 'c' when point to right half of 'c'. "point*ing*" http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcode82 ui/gfx/render_text.h:82: // And it bounds to the leading edge of the 3rd character 'D' when point to "binds" or "is bound to"... "point*ing*" http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcode89 ui/gfx/render_text.h:89: PREVIOUS_GRAPHEME_TRAILING, We'll have to discuss how this behaves in Pango... I don't have anything similar in Uniscribe, and I'll be implementing this manually. http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:114: virtual bool CursorChanged(const SelectionModel& sel) const { Make this an operator== http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:121: void init(size_t pos, size_t index, LeadingTrailingStatus status); Capitalize "Init". Please separate these members with blank lines. http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:122: // Logical selection start. If there is non-empty selection, The cursor's un-capitalize "the cursor's" http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:123: // visual start point is the leading edge of the selection_start_. What do you mean by the "cursor's visual start point"? http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:128: size_t cursor_pos_; The dichotomy between cursor_pos_ and char_index_ is unclear from their names. I don't have a great suggestion, but maybe rename "char_index_" as "caret_pos_". Then we could use the terminology of cursor versus caret to distinguish between logical and visual... I don't know... Perhaps we could still represent the selection range as a ui::Range (using the selection_range_.end() as the logical editing position), and use "cursor_pos_" to mean the associated character where the cursor is drawn. http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:129: // The following 2 fields are used to guide cursor visual position. replace "2" with "two", if you must include it. http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:132: // Cursor is attached to the leading or trailing edge of current character. "The visual placement of the cursor, relative to its associated character." http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:133: LeadingTrailingStatus bounding_edge_; Please rename "bounding_edge_", perhaps use "cursor_placement_" "cursor_edge_" or "edge_binding_". http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:152: void set_cursor_bounds_valid(bool valid) { cursor_bounds_valid_ = valid; } This should be protected. http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:192: // Moves the cursor to the specified |selection|. What does this mean now? Will this function only use certain values from the SelectionModel object (just the cursor), or take all the values of the argument SelectionModel? How does |select| interact with the notion of the selection being provided in the struct? I think we need to change this function name and clarify its comment. http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:204: size_t MinOfSelection() const { This seems unnecessary, move it to RenderTextLinux if you need it, or at least make it private/protected with MaxOfSelection. http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:210: void SetSelection(size_t start, size_t end); Comment on this function, especially how it effects (or doesn't effect) the cursor. Should this be a private/protected member? http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:246: // cursor_and_selection_. These bounds This should not use cursor_and_selection_. http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:249: // TODO(xji): do I stil need |position|? can I use cursor_and_selection_ only? This ought to take a SelectionModel, and yes, it needs to use the argument information, not the current cursor_and_selection_. The drag and drop code, and likely other instances will make use of this information. http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:285: size_t MaxOfSelection() const { This seems unnecessary, move it to RenderTextLinux if you need it. http://codereview.chromium.org/7461102/diff/22001/views/controls/textfield/na... File views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/7461102/diff/22001/views/controls/textfield/na... views/controls/textfield/native_textfield_views.cc:653: gfx::RenderText* render_text = GetRenderText(); May as well condense these lines into: return GetRenderText()->CursorBounds(); http://codereview.chromium.org/7461102/diff/22001/views/controls/textfield/te... File views/controls/textfield/textfield_views_model.cc (right): http://codereview.chromium.org/7461102/diff/22001/views/controls/textfield/te... views/controls/textfield/textfield_views_model.cc:383: bool TextfieldViewsModel::MoveCursorTo(const gfx::SelectionModel& selection, We'll need to reconsider this function similarly to the RenderText function, since |selection| may conflict with |select|, and its behavior is unclear. http://codereview.chromium.org/7461102/diff/22001/views/controls/textfield/te... views/controls/textfield/textfield_views_model.cc:403: std::abs(static_cast<long>(render_text_->GetCursorPosition() - why the static cast to long? Shouldn't this be a size_t cast outside the std::abs if needed? http://codereview.chromium.org/7461102/diff/22001/views/controls/textfield/te... File views/controls/textfield/textfield_views_model.h (right): http://codereview.chromium.org/7461102/diff/22001/views/controls/textfield/te... views/controls/textfield/textfield_views_model.h:136: // Moves the cursor to the specified |position|. Comment needs updating at the very least.
http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:88: SelectionModel::SelectionModel() { Thanks for the detailed review! For the comments that I did not reply specifically, it means "done". On 2011/08/01 05:02:23, msw wrote: > These constructors should have initialization lists. The first 2 constructors could be just a call to the 3rd constructor. init() is introduced to remove redundant code. If you think initialization list is better, I could change them to use initialization list. http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcode65 ui/gfx/render_text.h:65: // bidi editing guide: On 2011/08/01 05:02:23, msw wrote: > Perhaps we should formally post the bidi editing guide somewhere, otherwise this > mention is meaningless. You are right. Since the meeting note is not a completed document (and has restricted accessibility), and the original guideline is from non-Googler and may not be ready to public yet (besides we are modifying some of them), how about I just have a place holder for the doc here? http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcode78 ui/gfx/render_text.h:78: // So, besides the logical selection start point and end point, we need extra On 2011/08/01 05:02:23, msw wrote: > "point*s*" change to "start and end points"? http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcode89 ui/gfx/render_text.h:89: PREVIOUS_GRAPHEME_TRAILING, On 2011/08/01 05:02:23, msw wrote: > We'll have to discuss how this behaves in Pango... I don't have anything similar > in Uniscribe, and I'll be implementing this manually. It is the default mode, used in insertText/deleteText. I need to think whether this could be combined with LEADING/TRAILING, which means when insertText, we need to set the cursor visual position in SelectionModel as (cursor_logical_potion - 1, TRAILING). Let's discuss it more in person. http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:114: virtual bool CursorChanged(const SelectionModel& sel) const { On 2011/08/01 05:02:23, msw wrote: > Make this an operator== But this does not check whether selection_start_ changes or not. http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:123: // visual start point is the leading edge of the selection_start_. On 2011/08/01 05:02:23, msw wrote: > What do you mean by the "cursor's visual start point"? I am trying to say: we only need to save cursor (selection end)'s visual position (char_index_, bounding_edge) because the logical to visual mapping could be ambiguous. When there is selection, the visual selection (begin to end) is not ambiguous. The selection always starts visually at the leading edge of the selection_start_. Maybe I changed it to: The selection always starts visually at the leading edge of the selection_start_? Any suggestions? http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:128: size_t cursor_pos_; On 2011/08/01 05:02:23, msw wrote: > The dichotomy between cursor_pos_ and char_index_ is unclear from > their names. I don't have a great suggestion, but maybe rename "char_index_" as > "caret_pos_". Then we could use the terminology of cursor versus caret to > distinguish between logical and visual... I don't know... > > Perhaps we could still represent the selection range as a ui::Range (using the > selection_range_.end() as the logical editing position), and use "cursor_pos_" > to mean the associated character where the cursor is drawn. Maybe change cursor_pos_ to selection_end_? and change char_index_ to cursor_pos_? but without using ui::Range. ( I feel a range inside a 4 data member class is a bit overdo). change char_index_ to caret_pos_ is fine too. http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:192: // Moves the cursor to the specified |selection|. On 2011/08/01 05:02:23, msw wrote: > What does this mean now? Will this function only use certain values from the > SelectionModel object (just the cursor), or take all the values of the argument > SelectionModel? How does |select| interact with the notion of the selection > being provided in the struct? I think we need to change this function name and > clarify its comment. Talked to Michael offline, will change the signature to: bool MoveCursorTo(const SelectionModel& selection) and introduce SetSelectionModel(). http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:204: size_t MinOfSelection() const { On 2011/08/01 05:02:23, msw wrote: > This seems unnecessary, move it to RenderTextLinux if you need it, or at least > make it private/protected with MaxOfSelection. It is used in TextfieldViewModel. It is just a helper function. I could remove it and do the computation inside caller. But providing it is more convenient. http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:210: void SetSelection(size_t start, size_t end); On 2011/08/01 05:02:23, msw wrote: > Comment on this function, especially how it effects (or doesn't effect) the > cursor. Should this be a private/protected member? Talked to Michael offline, will change this to private and add setter/getter of SelectionModel for external callsites. http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:249: // TODO(xji): do I stil need |position|? can I use cursor_and_selection_ only? On 2011/08/01 05:02:23, msw wrote: > This ought to take a SelectionModel, and yes, it needs to use the argument > information, not the current cursor_and_selection_. The drag and drop code, and > likely other instances will make use of this information. Make sense. I planned to introduce a private one in RenderTextLinux accept SelectionModel. But you are right that RenderText::GetCursorBounds() should accept SelectionModel instead. http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:285: size_t MaxOfSelection() const { On 2011/08/01 05:02:23, msw wrote: > This seems unnecessary, move it to RenderTextLinux if you need it. It is used in RenderText::ApplyCompositionAndSelectionStyles() and IsPointInSelection(). http://codereview.chromium.org/7461102/diff/22001/views/controls/textfield/te... File views/controls/textfield/textfield_views_model.cc (right): http://codereview.chromium.org/7461102/diff/22001/views/controls/textfield/te... views/controls/textfield/textfield_views_model.cc:403: std::abs(static_cast<long>(render_text_->GetCursorPosition() - On 2011/08/01 05:02:23, msw wrote: > why the static cast to long? Shouldn't this be a size_t cast outside the > std::abs if needed? is (size_t - size_t) a size_t? then, you need to cast it to signed before std::abs.
Feedback in comments. http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:88: SelectionModel::SelectionModel() { On 2011/08/01 23:38:42, xji wrote: > Thanks for the detailed review! > > For the comments that I did not reply specifically, it means "done". > > On 2011/08/01 05:02:23, msw wrote: > > These constructors should have initialization lists. > > The first 2 constructors could be just a call to the 3rd constructor. init() is > introduced to remove redundant code. If you think initialization list is better, > I could change them to use initialization list. seems like a matter of taste, so you can leave it alone, but make sure to make the ctor with one arg explicit. http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcode65 ui/gfx/render_text.h:65: // bidi editing guide: On 2011/08/01 23:38:42, xji wrote: > On 2011/08/01 05:02:23, msw wrote: > > Perhaps we should formally post the bidi editing guide somewhere, otherwise > this > > mention is meaningless. > > You are right. Since the meeting note is not a completed document (and has > restricted accessibility), and the original guideline is from non-Googler and > may not be ready to public yet (besides we are modifying some of them), how > about I just have a place holder for the doc here? ok http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcode78 ui/gfx/render_text.h:78: // So, besides the logical selection start point and end point, we need extra On 2011/08/01 23:38:42, xji wrote: > On 2011/08/01 05:02:23, msw wrote: > > "point*s*" > > change to "start and end points"? oh, i misread this. your suggestion or the current text is fine. http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcode89 ui/gfx/render_text.h:89: PREVIOUS_GRAPHEME_TRAILING, On 2011/08/01 23:38:42, xji wrote: > On 2011/08/01 05:02:23, msw wrote: > > We'll have to discuss how this behaves in Pango... I don't have anything > similar > > in Uniscribe, and I'll be implementing this manually. > > It is the default mode, used in insertText/deleteText. I need to think whether > this could be combined with LEADING/TRAILING, which means when insertText, we > need to set the cursor visual position in SelectionModel as > (cursor_logical_potion - 1, TRAILING). Let's discuss it more in person. ok, this is fine for now, we can work out kinks later. http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcode95 ui/gfx/render_text.h:95: SelectionModel(size_t pos); Make this ctor explicit. http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:114: virtual bool CursorChanged(const SelectionModel& sel) const { On 2011/08/01 23:38:42, xji wrote: > On 2011/08/01 05:02:23, msw wrote: > > Make this an operator== > > But this does not check whether selection_start_ changes or not. Ah, okay. This should be renamed CusorEquals or similar, and make an explicit note that it ignores the selection start. http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:123: // visual start point is the leading edge of the selection_start_. On 2011/08/01 23:38:42, xji wrote: > On 2011/08/01 05:02:23, msw wrote: > > What do you mean by the "cursor's visual start point"? > > I am trying to say: > we only need to save cursor (selection end)'s visual position (char_index_, > bounding_edge) because the logical to visual mapping could be ambiguous. > > When there is selection, the visual selection (begin to end) is not ambiguous. > The selection always starts visually at the leading edge of the > selection_start_. > > Maybe I changed it to: > The selection always starts visually at the leading edge of the > selection_start_? > > Any suggestions? I think your suggested text is better, let's go with that. At some point we may want a more complete description of the visual representation of a logical selection range, up to you if you want to try to add that now. http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:128: size_t cursor_pos_; On 2011/08/01 23:38:42, xji wrote: > On 2011/08/01 05:02:23, msw wrote: > > The dichotomy between cursor_pos_ and char_index_ is unclear from > > their names. I don't have a great suggestion, but maybe rename "char_index_" > as > > "caret_pos_". Then we could use the terminology of cursor versus caret to > > distinguish between logical and visual... I don't know... > > > > Perhaps we could still represent the selection range as a ui::Range (using the > > selection_range_.end() as the logical editing position), and use "cursor_pos_" > > to mean the associated character where the cursor is drawn. > > Maybe change cursor_pos_ to selection_end_? > and change char_index_ to cursor_pos_? > but without using ui::Range. ( I feel a range inside a 4 data member class is a > bit overdo). > > change char_index_ to caret_pos_ is fine too. Sure, let's do selection_end_ and cursor_pos_. http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:192: // Moves the cursor to the specified |selection|. On 2011/08/01 23:38:42, xji wrote: > On 2011/08/01 05:02:23, msw wrote: > > What does this mean now? Will this function only use certain values from the > > SelectionModel object (just the cursor), or take all the values of the > argument > > SelectionModel? How does |select| interact with the notion of the selection > > being provided in the struct? I think we need to change this function name and > > clarify its comment. > > Talked to Michael offline, will change the signature to: > bool MoveCursorTo(const SelectionModel& selection) > and introduce SetSelectionModel(). Sorry, what will the revised MoveCursorTo do differently from SetSelectionModel? Also, I suppose it ought to be called set_cursor_and_selection(), or you could rename the data member to be selection_model_ (I think I like that more). http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:204: size_t MinOfSelection() const { On 2011/08/01 23:38:42, xji wrote: > On 2011/08/01 05:02:23, msw wrote: > > This seems unnecessary, move it to RenderTextLinux if you need it, or at least > > make it private/protected with MaxOfSelection. > > It is used in TextfieldViewModel. > It is just a helper function. I could remove it and do the computation inside > caller. But providing it is more convenient. It was easy to do this with a ui::Range before. But fine, if we're going to provide this convenience function, we may as well make the matching MaxOfSelection public too... http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:210: void SetSelection(size_t start, size_t end); On 2011/08/01 23:38:42, xji wrote: > On 2011/08/01 05:02:23, msw wrote: > > Comment on this function, especially how it effects (or doesn't effect) the > > cursor. Should this be a private/protected member? > > Talked to Michael offline, will change this to private and add setter/getter of > SelectionModel for external callsites. good. http://codereview.chromium.org/7461102/diff/22001/views/controls/textfield/te... File views/controls/textfield/textfield_views_model.cc (right): http://codereview.chromium.org/7461102/diff/22001/views/controls/textfield/te... views/controls/textfield/textfield_views_model.cc:403: std::abs(static_cast<long>(render_text_->GetCursorPosition() - On 2011/08/01 23:38:42, xji wrote: > On 2011/08/01 05:02:23, msw wrote: > > why the static cast to long? Shouldn't this be a size_t cast outside the > > std::abs if needed? > > is (size_t - size_t) a size_t? then, you need to cast it to signed before > std::abs. The call to std::abs implicitly casts the subtraction result to a signed type, and call to substr implicitly casts the std::abs result to an unsigned type; I don't think any explicit casting is necessary. However, my mentor recommends explicitly casting each of the render_text_ values to longs (or ints) and then subtracting them (he seems to think this is more readable). This was a little easier with a ui::Range, but do as you please, I don't really think it matters all that much.
(always forgot to set the comment when upload patch. Patch 7 mainly has 3 changes: Signature change of RenderText::MoveCursorTo(const SelectionModel&). Signature change of RenderText::GetCursorBounds(const SelectionModel&, bool). RenderText::SetSelection() changed to be private.) Thanks again! replies inlined. On 2011/08/02 01:29:05, msw wrote: > Feedback in comments. > > http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.cc > File ui/gfx/render_text.cc (right): > > http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.cc#newco... > ui/gfx/render_text.cc:88: SelectionModel::SelectionModel() { > On 2011/08/01 23:38:42, xji wrote: > > Thanks for the detailed review! > > > > For the comments that I did not reply specifically, it means "done". > > > > On 2011/08/01 05:02:23, msw wrote: > > > These constructors should have initialization lists. > > > > The first 2 constructors could be just a call to the 3rd constructor. init() > is > > introduced to remove redundant code. If you think initialization list is > better, > > I could change them to use initialization list. > > seems like a matter of taste, so you can leave it alone, but make sure to make > the ctor with one arg explicit. done making the ctor with on arg explicit. > > http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h > File ui/gfx/render_text.h (right): > > http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcode65 > ui/gfx/render_text.h:65: // bidi editing guide: > On 2011/08/01 23:38:42, xji wrote: > > On 2011/08/01 05:02:23, msw wrote: > > > Perhaps we should formally post the bidi editing guide somewhere, otherwise > > this > > > mention is meaningless. > > > > You are right. Since the meeting note is not a completed document (and has > > restricted accessibility), and the original guideline is from non-Googler and > > may not be ready to public yet (besides we are modifying some of them), how > > about I just have a place holder for the doc here? > > ok > > http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcode78 > ui/gfx/render_text.h:78: // So, besides the logical selection start point and > end point, we need extra > On 2011/08/01 23:38:42, xji wrote: > > On 2011/08/01 05:02:23, msw wrote: > > > "point*s*" > > > > change to "start and end points"? > > oh, i misread this. your suggestion or the current text is fine. > > http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcode89 > ui/gfx/render_text.h:89: PREVIOUS_GRAPHEME_TRAILING, > On 2011/08/01 23:38:42, xji wrote: > > On 2011/08/01 05:02:23, msw wrote: > > > We'll have to discuss how this behaves in Pango... I don't have anything > > similar > > > in Uniscribe, and I'll be implementing this manually. > > > > It is the default mode, used in insertText/deleteText. I need to think whether > > this could be combined with LEADING/TRAILING, which means when insertText, we > > need to set the cursor visual position in SelectionModel as > > (cursor_logical_potion - 1, TRAILING). Let's discuss it more in person. > > ok, this is fine for now, we can work out kinks later. > > http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcode95 > ui/gfx/render_text.h:95: SelectionModel(size_t pos); > Make this ctor explicit. done. > > http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... > ui/gfx/render_text.h:114: virtual bool CursorChanged(const SelectionModel& sel) > const { > On 2011/08/01 23:38:42, xji wrote: > > On 2011/08/01 05:02:23, msw wrote: > > > Make this an operator== > > > > But this does not check whether selection_start_ changes or not. > > Ah, okay. This should be renamed CusorEquals or similar, and make an explicit > note that it ignores the selection start. > I changed this to operator!=() and changed the 'changed' checking in MoveCursorTo() accordingly. > http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... > ui/gfx/render_text.h:123: // visual start point is the leading edge of the > selection_start_. > On 2011/08/01 23:38:42, xji wrote: > > On 2011/08/01 05:02:23, msw wrote: > > > What do you mean by the "cursor's visual start point"? > > > > I am trying to say: > > we only need to save cursor (selection end)'s visual position (char_index_, > > bounding_edge) because the logical to visual mapping could be ambiguous. > > > > When there is selection, the visual selection (begin to end) is not ambiguous. > > The selection always starts visually at the leading edge of the > > selection_start_. > > > > Maybe I changed it to: > > The selection always starts visually at the leading edge of the > > selection_start_? > > > > Any suggestions? > > I think your suggested text is better, let's go with that. At some point we may > want a more complete description of the visual representation of a logical > selection range, up to you if you want to try to add that now. > > http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... > ui/gfx/render_text.h:128: size_t cursor_pos_; > On 2011/08/01 23:38:42, xji wrote: > > On 2011/08/01 05:02:23, msw wrote: > > > The dichotomy between cursor_pos_ and char_index_ is unclear from > > > their names. I don't have a great suggestion, but maybe rename "char_index_" > > as > > > "caret_pos_". Then we could use the terminology of cursor versus caret to > > > distinguish between logical and visual... I don't know... > > > > > > Perhaps we could still represent the selection range as a ui::Range (using > the > > > selection_range_.end() as the logical editing position), and use > "cursor_pos_" > > > to mean the associated character where the cursor is drawn. > > > > Maybe change cursor_pos_ to selection_end_? > > and change char_index_ to cursor_pos_? > > but without using ui::Range. ( I feel a range inside a 4 data member class is > a > > bit overdo). > > > > change char_index_ to caret_pos_ is fine too. > > Sure, let's do selection_end_ and cursor_pos_. done. > > http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... > ui/gfx/render_text.h:192: // Moves the cursor to the specified |selection|. > On 2011/08/01 23:38:42, xji wrote: > > On 2011/08/01 05:02:23, msw wrote: > > > What does this mean now? Will this function only use certain values from the > > > SelectionModel object (just the cursor), or take all the values of the > > argument > > > SelectionModel? How does |select| interact with the notion of the selection > > > being provided in the struct? I think we need to change this function name > and > > > clarify its comment. > > > > Talked to Michael offline, will change the signature to: > > bool MoveCursorTo(const SelectionModel& selection) > > and introduce SetSelectionModel(). > > Sorry, what will the revised MoveCursorTo do differently from SetSelectionModel? > Also, I suppose it ought to be called set_cursor_and_selection(), or you could > rename the data member to be selection_model_ (I think I like that more). > MoveCursorTo() sets |changed| value and invalidate cursor_bounds and calls SetSelectionModel(). changed cursor_and_selection_ to selection_model_. > http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... > ui/gfx/render_text.h:204: size_t MinOfSelection() const { > On 2011/08/01 23:38:42, xji wrote: > > On 2011/08/01 05:02:23, msw wrote: > > > This seems unnecessary, move it to RenderTextLinux if you need it, or at > least > > > make it private/protected with MaxOfSelection. > > > > It is used in TextfieldViewModel. > > It is just a helper function. I could remove it and do the computation inside > > caller. But providing it is more convenient. > > It was easy to do this with a ui::Range before. But fine, if we're going to > provide this convenience function, we may as well make the matching > MaxOfSelection public too... > made MaxOfSelection public too. > http://codereview.chromium.org/7461102/diff/22001/ui/gfx/render_text.h#newcod... > ui/gfx/render_text.h:210: void SetSelection(size_t start, size_t end); > On 2011/08/01 23:38:42, xji wrote: > > On 2011/08/01 05:02:23, msw wrote: > > > Comment on this function, especially how it effects (or doesn't effect) the > > > cursor. Should this be a private/protected member? > > > > Talked to Michael offline, will change this to private and add setter/getter > of > > SelectionModel for external callsites. > > good. > > http://codereview.chromium.org/7461102/diff/22001/views/controls/textfield/te... > File views/controls/textfield/textfield_views_model.cc (right): > > http://codereview.chromium.org/7461102/diff/22001/views/controls/textfield/te... > views/controls/textfield/textfield_views_model.cc:403: > std::abs(static_cast<long>(render_text_->GetCursorPosition() - > On 2011/08/01 23:38:42, xji wrote: > > On 2011/08/01 05:02:23, msw wrote: > > > why the static cast to long? Shouldn't this be a size_t cast outside the > > > std::abs if needed? > > > > is (size_t - size_t) a size_t? then, you need to cast it to signed before > > std::abs. > > The call to std::abs implicitly casts the subtraction result to a signed type, > and call to substr implicitly casts the std::abs result to an unsigned type; I > don't think any explicit casting is necessary. > > However, my mentor recommends explicitly casting each of the render_text_ values > to longs (or ints) and then subtracting them (he seems to think this is more > readable). > > This was a little easier with a ui::Range, but do as you please, I don't really > think it matters all that much. hm... I prefer leave it as is.
http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:41: void ApplyStyleRangeImpl(gfx::StyleRanges* style_ranges, Thank you! http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:114: SelectionModel::~SelectionModel() { Move this above Init to match the header. http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:226: cursor_bounds_valid_ = false; SetSelectionModel invalidates the cursor bounds (via SetSelection), remove this. http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:412: while (std::abs(static_cast<long>(right_pos - left_pos) > 1)) { You didn't do this, but the closing parens are wrong, please fix as: ... left_pos)) > 1) { http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:481: static_cast<long>(0))); You can at least drop the static cast on the 0 literal. http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.h#newcode89 ui/gfx/render_text.h:89: enum LeadingTrailingStatus { Optional: rename CaretPlacement / CursorPlacement. http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:106: void set_selection_start(size_t start) { selection_start_ = start; } Nit: use consistent argument names for these three: all use |pos| or something, or each uses |selection_start|, |selection_end|, and |cursor_pos|, respectively. http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:121: void SetSelectionStart(size_t selection_start, bool select) { Remove this; it's unused and isn't needed. http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:128: void SetSelectionEmpty() { selection_start_ = selection_end_; } I'd prefer that we remove this; fewer mutators is better and this doesn't add much convenience. But I won't push hard, add a comment if we keep it. http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:130: bool operator!=(const SelectionModel& sel) const { Sorry if I told you to do operator overloading, make this an Equals function instead, and move the definition into the cc. http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:143: // do not need extra information for visual cursor bounding. Optional: "visual selection bounding". http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:152: size_t cursor_pos_; I'm really sorry, but upon further reflection, I think this ought to be called caret_pos_ to distinguish it from the logical cursor position used for inserting text. http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:154: LeadingTrailingStatus cursor_placement_; If we rename cursor_pos_ to caret_pos_ this ought to be renamed caret_placement_. http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:173: SelectionModel selection_model() const { return selection_model_; } Return a const reference. http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:194: SelectionModel::LeadingTrailingStatus GetCursorPlacement() const { Remove this, see my comment for its only use in TextfieldViewsModel::ReplaceTextInternal. http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:197: void SetCursorPlacement(SelectionModel::LeadingTrailingStatus placement) { Remove this, it's unused. http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:203: size_t GetCursorBoundingCharIndex() const { Remove this, see my comment for its only use in TextfieldViewsModel::ReplaceTextInternal. http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:317: void SetSelection(size_t start, size_t end); We should eliminate this function and use SetSelectionModel as the underlying mutator for all of its members. You don't have to do this now, but at least add a TODO. http://codereview.chromium.org/7461102/diff/29003/views/controls/textfield/te... File views/controls/textfield/textfield_views_model.cc (right): http://codereview.chromium.org/7461102/diff/29003/views/controls/textfield/te... views/controls/textfield/textfield_views_model.cc:386: // ConfirmCompositionText() updates cursor position. Need to update This is horrible, callers won't expect their argument to be modified. We should disregard the cursor set by confirming the composition text and pass |selection| to RenderText::MoveCursorTo unmodified. Otherwise, pass by const ref, create a copy to perform this unexpected (and I think undesirable) behavior, and comment on this behavior in the header. http://codereview.chromium.org/7461102/diff/29003/views/controls/textfield/te... views/controls/textfield/textfield_views_model.cc:413: *range = ui::Range(render_text_->GetSelectionStart(), Optional: Change this to range->set_start() and range->set_end(). http://codereview.chromium.org/7461102/diff/29003/views/controls/textfield/te... views/controls/textfield/textfield_views_model.cc:662: gfx::SelectionModel sel(cursor + text.length(), cursor, Try this instead: gfx::SelectionModel sel(render_text_->selection_model()); sel.set_selection_start(cursor + text.length()); sel.set_selection_end(cursor); ...
http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:114: SelectionModel::~SelectionModel() { On 2011/08/03 02:13:06, msw wrote: > Move this above Init to match the header. Done. http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:226: cursor_bounds_valid_ = false; On 2011/08/03 02:13:06, msw wrote: > SetSelectionModel invalidates the cursor bounds (via SetSelection), remove this. Done. http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:412: while (std::abs(static_cast<long>(right_pos - left_pos) > 1)) { On 2011/08/03 02:13:06, msw wrote: > You didn't do this, but the closing parens are wrong, please fix as: > ... left_pos)) > 1) { Done. http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:481: static_cast<long>(0))); On 2011/08/03 02:13:06, msw wrote: > You can at least drop the static cast on the 0 literal. std::max(x,y) requires 2 arguments to be the same type. removal will cause compilation error. http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.h#newcode89 ui/gfx/render_text.h:89: enum LeadingTrailingStatus { On 2011/08/03 02:13:06, msw wrote: > Optional: rename CaretPlacement / CursorPlacement. changed to CaretPlacement http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:106: void set_selection_start(size_t start) { selection_start_ = start; } On 2011/08/03 02:13:06, msw wrote: > Nit: use consistent argument names for these three: all use |pos| or something, > or each uses |selection_start|, |selection_end|, and |cursor_pos|, respectively. used |pos| for all. http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:121: void SetSelectionStart(size_t selection_start, bool select) { On 2011/08/03 02:13:06, msw wrote: > Remove this; it's unused and isn't needed. Done. http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:128: void SetSelectionEmpty() { selection_start_ = selection_end_; } On 2011/08/03 02:13:06, msw wrote: > I'd prefer that we remove this; fewer mutators is better and this doesn't add > much convenience. But I won't push hard, add a comment if we keep it. removed http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:130: bool operator!=(const SelectionModel& sel) const { On 2011/08/03 02:13:06, msw wrote: > Sorry if I told you to do operator overloading, make this an Equals function > instead, and move the definition into the cc. Done. http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:143: // do not need extra information for visual cursor bounding. On 2011/08/03 02:13:06, msw wrote: > Optional: "visual selection bounding". Done. http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:152: size_t cursor_pos_; On 2011/08/03 02:13:06, msw wrote: > I'm really sorry, but upon further reflection, I think this ought to be called > caret_pos_ to distinguish it from the logical cursor position used for inserting > text. Done and it looks much better. http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:154: LeadingTrailingStatus cursor_placement_; On 2011/08/03 02:13:06, msw wrote: > If we rename cursor_pos_ to caret_pos_ this ought to be renamed > caret_placement_. Done. http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:173: SelectionModel selection_model() const { return selection_model_; } On 2011/08/03 02:13:06, msw wrote: > Return a const reference. Done. http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:194: SelectionModel::LeadingTrailingStatus GetCursorPlacement() const { On 2011/08/03 02:13:06, msw wrote: > Remove this, see my comment for its only use in > TextfieldViewsModel::ReplaceTextInternal. Done. http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:197: void SetCursorPlacement(SelectionModel::LeadingTrailingStatus placement) { On 2011/08/03 02:13:06, msw wrote: > Remove this, it's unused. This is used in RenderTextLinux. caret_placement is set to PREVIOUS_GRAPHEME_TRAILING in SetText(). http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:203: size_t GetCursorBoundingCharIndex() const { On 2011/08/03 02:13:06, msw wrote: > Remove this, see my comment for its only use in > TextfieldViewsModel::ReplaceTextInternal. Done. http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:317: void SetSelection(size_t start, size_t end); On 2011/08/03 02:13:06, msw wrote: > We should eliminate this function and use SetSelectionModel as the underlying > mutator for all of its members. You don't have to do this now, but at least add > a TODO. removed. http://codereview.chromium.org/7461102/diff/29003/views/controls/textfield/te... File views/controls/textfield/textfield_views_model.cc (right): http://codereview.chromium.org/7461102/diff/29003/views/controls/textfield/te... views/controls/textfield/textfield_views_model.cc:386: // ConfirmCompositionText() updates cursor position. Need to update On 2011/08/03 02:13:06, msw wrote: > This is horrible, callers won't expect their argument to be modified. We should > disregard the cursor set by confirming the composition text and pass |selection| > to RenderText::MoveCursorTo unmodified. Otherwise, pass by const ref, create a > copy to perform this unexpected (and I think undesirable) behavior, and comment > on this behavior in the header. Agree that this is terrible. Previously the MoveCursorTo() signature is MoveCursorTo(const SelectionModel&, bool select), inside the function, if |select| is true, selection's start remains, and cursor moves to selection's end. Now since we pass in SelectionModel alone, if any function (such as ConfirmCompositionText()) changes selection start, we have to reflect it in the SelectionModel . There is a test case in TextfieldViewsModelTest.CompositionTextTest (Line 611-623 in patch, search for the 3rd sel.set_selection_start) to cover this: the original text is "678", the (will-be-appended) composition text is "678" where "8" is selected. During SetCompositionText(), the selection range becomes (5, 6) (I do not know why it changes render_text's selection range in SetCompositionText, should it be done in ConfirmCompositionText? But it is orthogonal to the issue we discussed here.). Then, we call MoveCursorTo() and pass 5 as selection_start. Inside MoveCursorTo(), ConfirmCompositionText() is called and the cursor position is set to 6. We need to move the selection start to 6 as well so the subsequent operation works correctly. The next operation is SetCompositionText(), which will DeleteSelection if there is any. changing selection range from (5, 0) to (6, 0) makes difference for test "EXPECT_STR_EQ("678", model.GetText());". I am pass by const ref here and added comments in header. Any suggestion is welcomed. http://codereview.chromium.org/7461102/diff/29003/views/controls/textfield/te... views/controls/textfield/textfield_views_model.cc:413: *range = ui::Range(render_text_->GetSelectionStart(), On 2011/08/03 02:13:06, msw wrote: > Optional: Change this to range->set_start() and range->set_end(). Done. http://codereview.chromium.org/7461102/diff/29003/views/controls/textfield/te... views/controls/textfield/textfield_views_model.cc:662: gfx::SelectionModel sel(cursor + text.length(), cursor, On 2011/08/03 02:13:06, msw wrote: > Try this instead: > gfx::SelectionModel sel(render_text_->selection_model()); > sel.set_selection_start(cursor + text.length()); > sel.set_selection_end(cursor); > ... Done and seems "sel.set_selection_end(cursor);" is not needed.
This is looking great now! I have just a few suggestions, and I'm excited to use the new cursor/selection model! http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:481: static_cast<long>(0))); On 2011/08/03 18:47:40, xji wrote: > On 2011/08/03 02:13:06, msw wrote: > > You can at least drop the static cast on the 0 literal. > > std::max(x,y) requires 2 arguments to be the same type. removal will cause > compilation error. OK http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/7461102/diff/29003/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:197: void SetCursorPlacement(SelectionModel::LeadingTrailingStatus placement) { On 2011/08/03 18:47:40, xji wrote: > On 2011/08/03 02:13:06, msw wrote: > > Remove this, it's unused. > > This is used in RenderTextLinux. caret_placement is set to > PREVIOUS_GRAPHEME_TRAILING in SetText(). OK http://codereview.chromium.org/7461102/diff/1004/ui/gfx/render_text.h#newcode175 ui/gfx/render_text.h:175: Can you add a comment here like: This cursor position corresponds to SelectionModel::selection_end. In addition to representing the selection end, it's also where logical text edits take place, and doesn't necessarily correspond to SelectionModel::caret_pos. http://codereview.chromium.org/7461102/diff/1004/views/controls/textfield/nat... File views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/7461102/diff/1004/views/controls/textfield/nat... views/controls/textfield/native_textfield_views.cc:259: SetMergedSelectionModel(start_pos, end_pos); Inline SetMergedSelectionModel here. http://codereview.chromium.org/7461102/diff/1004/views/controls/textfield/nat... views/controls/textfield/native_textfield_views.cc:768: if (GetTextInputType() != ui::TEXT_INPUT_TYPE_TEXT) This will block password fields as well, this should be: if (GetTextInputType() != ui::TEXT_INPUT_TYPE_NONE) http://codereview.chromium.org/7461102/diff/1004/views/controls/textfield/nat... views/controls/textfield/native_textfield_views.cc:771: && start.selection_end() == std::numeric_limits<size_t>::max()) What inspired this check? Perhaps we should be doing this at a lower level. http://codereview.chromium.org/7461102/diff/1004/views/controls/textfield/nat... views/controls/textfield/native_textfield_views.cc:777: SelectSelectionModel(sel); Inline SelectSelectionModel here. http://codereview.chromium.org/7461102/diff/1004/views/controls/textfield/nat... File views/controls/textfield/native_textfield_views.h (right): http://codereview.chromium.org/7461102/diff/1004/views/controls/textfield/nat... views/controls/textfield/native_textfield_views.h:125: // TODO(xji): might need to declare it in NativeTextfieldWrapper. I think we ought to inline this function at its one use for now, and we can expose it later if needed. http://codereview.chromium.org/7461102/diff/1004/views/controls/textfield/nat... views/controls/textfield/native_textfield_views.h:175: bool SetMergedSelectionModel(const gfx::SelectionModel& start, I think we ought to inline this function at its one use for now, and we can expose it later if needed. http://codereview.chromium.org/7461102/diff/1004/views/controls/textfield/tex... File views/controls/textfield/textfield_views_model.cc (right): http://codereview.chromium.org/7461102/diff/1004/views/controls/textfield/tex... views/controls/textfield/textfield_views_model.cc:408: std::abs(static_cast<long>(render_text_->GetCursorPosition() - It just occurred to me, can't we safely use MaxOfSelection-MinOfSelection here without the cast or std::abs? http://codereview.chromium.org/7461102/diff/1004/views/controls/textfield/tex... views/controls/textfield/textfield_views_model.cc:422: render_text_->SetSelectionModel(selection); call TextfieldViewsModel::SelectSelectionModel instead and drop the explicit ConfirmCompositionText from this function.
http://codereview.chromium.org/7461102/diff/1004/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/7461102/diff/1004/ui/gfx/render_text.h#newcode175 ui/gfx/render_text.h:175: size_t GetCursorPosition() const; On 2011/08/03 19:46:39, msw wrote: > Can you add a comment here like: > This cursor position corresponds to SelectionModel::selection_end. In addition > to representing the selection end, it's also where logical text edits take > place, and doesn't necessarily correspond to SelectionModel::caret_pos. Done. http://codereview.chromium.org/7461102/diff/1004/views/controls/textfield/nat... File views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/7461102/diff/1004/views/controls/textfield/nat... views/controls/textfield/native_textfield_views.cc:259: SetMergedSelectionModel(start_pos, end_pos); On 2011/08/03 19:46:39, msw wrote: > Inline SetMergedSelectionModel here. Done. http://codereview.chromium.org/7461102/diff/1004/views/controls/textfield/nat... views/controls/textfield/native_textfield_views.cc:768: if (GetTextInputType() != ui::TEXT_INPUT_TYPE_TEXT) On 2011/08/03 19:46:39, msw wrote: > This will block password fields as well, this should be: > if (GetTextInputType() != ui::TEXT_INPUT_TYPE_NONE) Done. http://codereview.chromium.org/7461102/diff/1004/views/controls/textfield/nat... views/controls/textfield/native_textfield_views.cc:771: && start.selection_end() == std::numeric_limits<size_t>::max()) On 2011/08/03 19:46:39, msw wrote: > What inspired this check? Perhaps we should be doing this at a lower level. Honestly, I do not know. In SetSelectionRange(), it checks that "!range.IsValid()". I am doing the same thing here. http://codereview.chromium.org/7461102/diff/1004/views/controls/textfield/nat... views/controls/textfield/native_textfield_views.cc:777: SelectSelectionModel(sel); On 2011/08/03 19:46:39, msw wrote: > Inline SelectSelectionModel here. Done. http://codereview.chromium.org/7461102/diff/1004/views/controls/textfield/tex... File views/controls/textfield/textfield_views_model.cc (right): http://codereview.chromium.org/7461102/diff/1004/views/controls/textfield/tex... views/controls/textfield/textfield_views_model.cc:408: std::abs(static_cast<long>(render_text_->GetCursorPosition() - On 2011/08/03 19:46:39, msw wrote: > It just occurred to me, can't we safely use MaxOfSelection-MinOfSelection here > without the cast or std::abs? Done. http://codereview.chromium.org/7461102/diff/1004/views/controls/textfield/tex... views/controls/textfield/textfield_views_model.cc:422: render_text_->SetSelectionModel(selection); On 2011/08/03 19:46:39, msw wrote: > call TextfieldViewsModel::SelectSelectionModel instead and drop the explicit > ConfirmCompositionText from this function. Done.
Just a few more nits, thanks for bearing with me. http://codereview.chromium.org/7461102/diff/47003/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/7461102/diff/47003/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:98: CaretPlacement placement) { If the arguments don't all fit on one line, they'll have to be each on their own line, like ApplyStyleRangeImpl. http://codereview.chromium.org/7461102/diff/47003/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:102: SelectionModel::SelectionModel(size_t start, size_t end, size_t pos, ditto (args on new lines) http://codereview.chromium.org/7461102/diff/47003/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:111: return selection_start_ == sel.selection_start() && goes on the end of the line. http://codereview.chromium.org/7461102/diff/47003/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:117: void SelectionModel::Init(size_t start, size_t end, size_t pos, ditto (args on new lines) http://codereview.chromium.org/7461102/diff/47003/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:171: } Add a blank line between these function definitions. http://codereview.chromium.org/7461102/diff/47003/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/7461102/diff/47003/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:257: Rect CursorBounds(); This should return a const ref. http://codereview.chromium.org/7461102/diff/47003/views/controls/textfield/na... File views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/7461102/diff/47003/views/controls/textfield/na... views/controls/textfield/native_textfield_views.cc:260: if (GetTextInputType() != ui::TEXT_INPUT_TYPE_NONE) Move this check above the start_pos and end_pos calculations. Blarg, the (GetTextInputType() != ui::TEXT_INPUT_TYPE_TEXT) checks are everywhere, I bet password fields are pretty broken, we can look into that issue later. You can revert this to match the other checks if you want... http://codereview.chromium.org/7461102/diff/47003/views/controls/textfield/na... views/controls/textfield/native_textfield_views.cc:262: if (start_pos.selection_end() == end_pos.selection_end() OK, I think we can trust the results of FindCursorPosition well enough to discard this check. The 'corresponding' check in SetSelectionRange is done on 'raw' input to the class, which is less trustworthy. http://codereview.chromium.org/7461102/diff/47003/views/controls/textfield/te... File views/controls/textfield/textfield_views_model.cc (right): http://codereview.chromium.org/7461102/diff/47003/views/controls/textfield/te... views/controls/textfield/textfield_views_model.cc:589: range.end()); Move everything after the assignment operator onto the next line: size_t start = std::min(range.start() + composition.selection.start(), range.end()); http://codereview.chromium.org/7461102/diff/47003/views/controls/textfield/te... views/controls/textfield/textfield_views_model.cc:591: range.end()); ditto on the line split here. http://codereview.chromium.org/7461102/diff/47003/views/controls/textfield/te... views/controls/textfield/textfield_views_model.cc:725: render_text_->GetCursorPosition(); ditto (put the whole comparison on the next line)
http://codereview.chromium.org/7461102/diff/47003/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/7461102/diff/47003/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:98: CaretPlacement placement) { On 2011/08/03 23:44:38, msw wrote: > If the arguments don't all fit on one line, they'll have to be each on their own > line, like ApplyStyleRangeImpl. Done. http://codereview.chromium.org/7461102/diff/47003/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:102: SelectionModel::SelectionModel(size_t start, size_t end, size_t pos, On 2011/08/03 23:44:38, msw wrote: > ditto (args on new lines) Done. http://codereview.chromium.org/7461102/diff/47003/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:111: return selection_start_ == sel.selection_start() On 2011/08/03 23:44:38, msw wrote: > && goes on the end of the line. Done. http://codereview.chromium.org/7461102/diff/47003/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:117: void SelectionModel::Init(size_t start, size_t end, size_t pos, On 2011/08/03 23:44:38, msw wrote: > ditto (args on new lines) Done. http://codereview.chromium.org/7461102/diff/47003/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:171: } On 2011/08/03 23:44:38, msw wrote: > Add a blank line between these function definitions. Done. http://codereview.chromium.org/7461102/diff/47003/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/7461102/diff/47003/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:257: Rect CursorBounds(); On 2011/08/03 23:44:38, msw wrote: > This should return a const ref. Done. http://codereview.chromium.org/7461102/diff/47003/views/controls/textfield/na... File views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/7461102/diff/47003/views/controls/textfield/na... views/controls/textfield/native_textfield_views.cc:260: if (GetTextInputType() != ui::TEXT_INPUT_TYPE_NONE) On 2011/08/03 23:44:38, msw wrote: > Move this check above the start_pos and end_pos calculations. > > Blarg, the (GetTextInputType() != ui::TEXT_INPUT_TYPE_TEXT) checks are > everywhere, I bet password fields are pretty broken, we can look into that issue > later. You can revert this to match the other checks if you want... Done. http://codereview.chromium.org/7461102/diff/47003/views/controls/textfield/na... views/controls/textfield/native_textfield_views.cc:262: if (start_pos.selection_end() == end_pos.selection_end() On 2011/08/03 23:44:38, msw wrote: > OK, I think we can trust the results of FindCursorPosition well enough to > discard this check. The 'corresponding' check in SetSelectionRange is done on > 'raw' input to the class, which is less trustworthy. Ah, you are right here. I suspect that we will need Get/SelectSelectionRange() later (to save/restore) selection model during switch tab content. But I agree with you that we will worry about that later. Done. http://codereview.chromium.org/7461102/diff/47003/views/controls/textfield/te... File views/controls/textfield/textfield_views_model.cc (right): http://codereview.chromium.org/7461102/diff/47003/views/controls/textfield/te... views/controls/textfield/textfield_views_model.cc:589: range.end()); On 2011/08/03 23:44:38, msw wrote: > Move everything after the assignment operator onto the next line: > size_t start = > std::min(range.start() + composition.selection.start(), range.end()); Done. http://codereview.chromium.org/7461102/diff/47003/views/controls/textfield/te... views/controls/textfield/textfield_views_model.cc:591: range.end()); On 2011/08/03 23:44:38, msw wrote: > ditto on the line split here. Done. http://codereview.chromium.org/7461102/diff/47003/views/controls/textfield/te... views/controls/textfield/textfield_views_model.cc:725: render_text_->GetCursorPosition(); On 2011/08/03 23:44:38, msw wrote: > ditto (put the whole comparison on the next line) Done.
I suspect that there might be lingering details to resolve, but it would help to actually work with these changes. LGTM. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
