Clean up of SelectionModel

1. change the setters of gfx::SelectionModel to be private. Set one alone might make SelectionModel into un-stable state, which should not be allowed.

2. Removing SelectionModel(size_t, size_t) constructor. Introduce RenderText::SelectRange(const ui::Range&) to handle range.

3. revert removal of SelectRange/GetSelectedRange in r103188.

BUG=90426
TEST=view_unittests.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=105138

[msw, 2011-09-29 02:20:21]

The changes here generally seem correct, yet they might be insufficient to
ensure correct usage. Did you do a pass over all the places we construct
SelectionModels to ensure they are correct? What about all the places that use
the caret positions, which now have a new potential value?

I'm concerned by the presence of this new caret position enum value that's
really only valid from SelectionModel construction through
RenderText::SetSelectionModel (which some SelectionModels may never actually
encounter). The new value isn't considered or guarded against elsewhere, which
is very concerning. For example, GetCursorBounds assumes the  placement has one
of two values, where in fact it could now be one of three. Do you have any ideas
on how to address all these new entry points for undefined behavior? I'd like it
if we could somehow avoid this new value altogether, but I don't have any solid
ideas on how to do that without further thought.

http://codereview.chromium.org/8044004/diff/6001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

http://codereview.chromium.org/8044004/diff/6001/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:421: return SelectionModel(selection_start,
You can use the single-argument ctor here for the same result:
return SelectionModel(selection_start);
But it's fine with me if you'd rather leave it explicit.

http://codereview.chromium.org/8044004/diff/6001/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:584: size_t selection_end =
selection_model.selection_end();
Can you add a RenderTextTest for this new behavior? It'd be good to test a
couple simple cases and one or two with complex script where the previous
grapheme isn't pos-1.

http://codereview.chromium.org/8044004/diff/6001/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:589:
selection_model_.set_caret_pos(GetIndexOfPreviousGrapheme(selection_end));
You need to check if GetIndexOfPreviousGrapheme returns the same position as its
argument (when there is no previous grapheme), and in that case we set the caret
position to leading... see how it's done in MoveCursorTo(size_t, bool) directly
below.

Maybe we need a warning on GetIndexOfPreviousGrapheme about that edge case,
since this isn't the first time it's come up.

http://codereview.chromium.org/8044004/diff/6001/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

http://codereview.chromium.org/8044004/diff/6001/ui/gfx/render_text.h#newcode178
ui/gfx/render_text.h:178: // Returns the selection model of selection_start_.
|selection_start_| isn't a member of this class, can you update the comment to
"Return a SelectionModel with the cursor at the current selection's start."

http://codereview.chromium.org/8044004/diff/6001/ui/gfx/selection_model.cc
File ui/gfx/selection_model.cc (right):

http://codereview.chromium.org/8044004/diff/6001/ui/gfx/selection_model.cc#ne...
ui/gfx/selection_model.cc:18: if (start < end)
Perhaps this condition deserves an explanation:
"The caret position should trail the selection end's previous grapheme in this
case [because/in order to/so that] ..."

http://codereview.chromium.org/8044004/diff/6001/ui/gfx/selection_model.h
File ui/gfx/selection_model.h (right):

http://codereview.chromium.org/8044004/diff/6001/ui/gfx/selection_model.h#new...
ui/gfx/selection_model.h:49: // edge of |selection_end_|'s previous grapheme. It
is used when create a
Grammar: "means *that the* cursor".
Grammar: "It is used when creating" or "It is used to create".

I don't think specifics about the current use(s) in the second sentence is
helpful. I think the first sentence of this comment sufficiently describes the
purpose of this value, and the third sentence attempts to note that the caret
position and placement are resolved on SetSelectionModel(), which is important,
but it does a poor job of describing that behavior and why it's necessary.
Instead try:

TRAILING_OF_PREVIOUS_GRAPHEME means that the cursor will visually trail
|selection_end_|'s previous grapheme or lead |selection_end_| if it has no
previous grapheme. The actual caret position and placement are resolved in
SetSelectionModel, where a previous grapheme can be calculated from the current
text.

http://codereview.chromium.org/8044004/diff/6001/ui/gfx/selection_model.h#new...
ui/gfx/selection_model.h:55: TRAILING_OF_PREVIOUS_GRAPHEME,
I like your old name (TRAILING_PREIVOUS_GRAPHEME) better.

http://codereview.chromium.org/8044004/diff/6001/ui/gfx/selection_model.h#new...
ui/gfx/selection_model.h:67: void set_selection_start(size_t pos) {
selection_start_ = pos; }
Should set_selection_start also be made private?

http://codereview.chromium.org/8044004/diff/6001/ui/gfx/selection_model.h#new...
ui/gfx/selection_model.h:76: friend class RenderText;
I don't know that making RenderText a friend and moving setters to private is a
good change. It seems okay, but I would appreciate another reviewer's
perspective.

http://codereview.chromium.org/8044004/diff/6001/ui/gfx/selection_model.h#new...
ui/gfx/selection_model.h:82: 
You can remove this blank line.

http://codereview.chromium.org/8044004/diff/6001/ui/gfx/selection_model.h#new...
ui/gfx/selection_model.h:85: }
You should add a blank line after this function, before the selection_start_
comment.

http://codereview.chromium.org/8044004/diff/6001/views/touchui/touch_selectio...
File views/touchui/touch_selection_controller_impl_unittest.cc (right):

http://codereview.chromium.org/8044004/diff/6001/views/touchui/touch_selectio...
views/touchui/touch_selection_controller_impl_unittest.cc:155: // Test selection
range (3, 2).
Please also test some other pertinent BiDi ranges.
Maybe: (2,3) (3,4) (4,3) (1,4) (4,1) (3,6) (6,3), or as you see fit.
You can split the test up if it gets too large.

[xji, 2011-09-29 21:11:10]

Ah, I got a "Invalid XSRF token." when I submit last time, and I have to re-type
everything!

On 2011/09/29 02:20:21, msw wrote:
> The changes here generally seem correct, yet they might be insufficient to
> ensure correct usage. Did you do a pass over all the places we construct
> SelectionModels to ensure they are correct?

(before introducing SelectSelectionModel,) the constructors are in
NativeTextfieldViews, TextfieldViewsModel, and RenderText itself.
I checked constructors inside them.
The 2 corrected constructors in this CL are mis-usage before.

> What about all the places that use
> the caret positions, which now have a new potential value?

caret_pos() and caret_placement() are used only inside RenderText.
The new potential value is only valid in SelectionModel constructors.
It wont affect the usage in RenderText (after fix of GetCursorBounds()). Please
see below for detail.

> 
> I'm concerned by the presence of this new caret position enum value that's
> really only valid from SelectionModel construction through
> RenderText::SetSelectionModel (which some SelectionModels may never actually
> encounter). The new value isn't considered or guarded against elsewhere, which
> is very concerning. For example, GetCursorBounds assumes the  placement has
one
> of two values, where in fact it could now be one of three. Do you have any
ideas
> on how to address all these new entry points for undefined behavior?

The new value is only valid in SelectionModel constructor.
There are 2 public api in RenderText that accept SelectionModel as IN
parameters: MoveCursorTo() and GetCursorBounds().
MoveCursorTo() reset (caret_pos, caret_placement). And you are right that
GetCursorBounds() should do the same (thanks for catching this). After which,
the usage in RenderText should not be affected.



> I'd like it
> if we could somehow avoid this new value altogether, but I don't have any
solid
> ideas on how to do that without further thought.

I have the same concern of introducing this new enum.
But I have not thought of a better way to handle it.
I think we should allow user to init SelectionModel by using range.
But we have to handle that the (caret_pos, caret_placement) is not
(selection_end, LEADING) when selection is not empty, selection_end >
selection_begin, and selection_end is at run boundary.
And we can not use (selection_end - 1, TRAILING) due to complex script.

We could leave the SelectionModel code untouched, which means (caret_pos,
caret_placement) might be initialized to a not-correct value. And reset those 2
values in MoveCursorTo() and GetCursorBounds(). I did not like this idea because
it leaves SelectModel in a not-stable stage and the reset operation is implicit.
(maybe we could make all the setters and getters as private except for
RenderText so it is less problematic).

I thought about split SelectionModel into logical range and visual
representation of (caret_pos, caret_placement). But I do not feel it solves the
problem. Whenever the range changes, its visual representation should be reset
implicitly. And since (caret_pos, caret_placement) is bounded to selection_end,
many of the places that SelectionModel is needed, we will have to pass 2
separate structures around.
 

>
http://codereview.chromium.org/8044004/diff/6001/ui/gfx/render_text.cc#newcod...
> ui/gfx/render_text.cc:589:
> selection_model_.set_caret_pos(GetIndexOfPreviousGrapheme(selection_end));
> You need to check if GetIndexOfPreviousGrapheme returns the same position as
its
> argument (when there is no previous grapheme), and in that case we set the
caret
> position to leading... see how it's done in MoveCursorTo(size_t, bool)
directly
> below.

In this case, GetIndexOfPreviousGrapheme != IndexOfCurrentGrapheme.
since selection_end > selection_start && indexOfCurrentGraphem == selection_end.
I can do a dcheck here.


> 
>
http://codereview.chromium.org/8044004/diff/6001/ui/gfx/selection_model.h#new...
> ui/gfx/selection_model.h:67: void set_selection_start(size_t pos) {
> selection_start_ = pos; }
> Should set_selection_start also be made private?

I left it there because it wont cause any un-stability.

> 
>
http://codereview.chromium.org/8044004/diff/6001/ui/gfx/selection_model.h#new...
> ui/gfx/selection_model.h:76: friend class RenderText;
> I don't know that making RenderText a friend and moving setters to private is
a
> good change. It seems okay, but I would appreciate another reviewer's
> perspective.
> 

The purpose is to avoid set one value without adjust the others, for example,
just set selection_end alone without adjust caret_pos and caret_placement.

I could make a public api which set the 3 values together. But I doubt the
callers (except RenderText) know what the 3 values should be so the function
would not be really useful.

[xji, 2011-09-29 23:59:47]

> > What about all the places that use
> > the caret positions, which now have a new potential value?
> 
> caret_pos() and caret_placement() are used only inside RenderText.
> The new potential value is only valid in SelectionModel constructors.
> It wont affect the usage in RenderText (after fix of GetCursorBounds()).
Please
> see below for detail.
> 
> > 
> > I'm concerned by the presence of this new caret position enum value that's
> > really only valid from SelectionModel construction through
> > RenderText::SetSelectionModel (which some SelectionModels may never actually
> > encounter). The new value isn't considered or guarded against elsewhere,
which
> > is very concerning. For example, GetCursorBounds assumes the  placement has
> one
> > of two values, where in fact it could now be one of three. Do you have any
> ideas
> > on how to address all these new entry points for undefined behavior?
> 
> The new value is only valid in SelectionModel constructor.
> There are 2 public api in RenderText that accept SelectionModel as IN
> parameters: MoveCursorTo() and GetCursorBounds().
> MoveCursorTo() reset (caret_pos, caret_placement). And you are right that
> GetCursorBounds() should do the same (thanks for catching this). After which,
> the usage in RenderText should not be affected.
> 

The error checking of caret_pos inside MoveCursorTo() should not be done in case
of TRAILING_OF_PREVIOUS_GRAPHEME.

[xji, 2011-09-30 00:53:43]

> I'm concerned by the presence of this new caret position enum value that's
> really only valid from SelectionModel construction through
> RenderText::SetSelectionModel (which some SelectionModels may never actually
> encounter). The new value isn't considered or guarded against elsewhere, which
> is very concerning. For example, GetCursorBounds assumes the  placement has
one
> of two values, where in fact it could now be one of three. Do you have any
ideas
> on how to address all these new entry points for undefined behavior? I'd like
it
> if we could somehow avoid this new value altogether, but I don't have any
solid
> ideas on how to do that without further thought.
> 

How about 
1. handling this new enum in public APIs and SetSelectionModel().
2. guard all other functions that having SelectionModel as IN parameter against
this new enum.
3. make getters of caret_pos() and caret_placement() as private (except for
RenderText*).

The problem is that it wont guard future usages except emphasize in comments.

[xji, 2011-09-30 01:39:35]

Looks like the 2 ways to completely get rid of TRAILING_OF_PREVIOUS_GRAPHEME
are:

remove SelectionModel(size_t start, size_t end) constructor; add
MoveCursorTo(const ui::Range) to handle selection_range and set
selection_start/end, caret_pos/placement correctly inside RenderText; re-add
SelectRange (and maybe GetSelectedRange) which calls MoveCursorTo(const
ui::Range).

Or split SelectionModel as logical range and visual representation.

Suggestions are very welcomed.

[msw, 2011-09-30 19:09:16]

On 2011/09/30 01:39:35, xji wrote:
> Looks like the 2 ways to completely get rid of TRAILING_OF_PREVIOUS_GRAPHEME
> are:
> 
> remove SelectionModel(size_t start, size_t end) constructor; add
> MoveCursorTo(const ui::Range) to handle selection_range and set
> selection_start/end, caret_pos/placement correctly inside RenderText; re-add
> SelectRange (and maybe GetSelectedRange) which calls MoveCursorTo(const
> ui::Range).
> 
> Or split SelectionModel as logical range and visual representation.
> 
> Suggestions are very welcomed.

Thanks for considering alternate (hopefully better) approaches. As we discussed
on chat, try replacing the (size_t, size_t) ctor with a SelectRange() function
on RenderText, also you should definitely move set_selection_start() to private
if possible (it's just another way users beyond RenderText could get into
invalid states).

In cases like this, it's often prudent to make the SelectionModel ctors private
and only manufacture them via RenderText functions, but I think our discussed
approach is okay without going to such an extreme measure.

[xji, 2011-10-03 23:18:57]

http://codereview.chromium.org/8044004/diff/6001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

http://codereview.chromium.org/8044004/diff/6001/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:421: return SelectionModel(selection_start,
On 2011/09/29 02:20:21, msw wrote:
> You can use the single-argument ctor here for the same result:
> return SelectionModel(selection_start);
> But it's fine with me if you'd rather leave it explicit.

left is as explicit.

http://codereview.chromium.org/8044004/diff/6001/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:584: size_t selection_end =
selection_model.selection_end();
On 2011/09/29 02:20:21, msw wrote:
> Can you add a RenderTextTest for this new behavior? It'd be good to test a
> couple simple cases and one or two with complex script where the previous
> grapheme isn't pos-1.

this function is reverted.

http://codereview.chromium.org/8044004/diff/6001/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

http://codereview.chromium.org/8044004/diff/6001/ui/gfx/render_text.h#newcode178
ui/gfx/render_text.h:178: // Returns the selection model of selection_start_.
On 2011/09/29 02:20:21, msw wrote:
> |selection_start_| isn't a member of this class, can you update the comment to
> "Return a SelectionModel with the cursor at the current selection's start."

Done.

http://codereview.chromium.org/8044004/diff/6001/ui/gfx/selection_model.cc
File ui/gfx/selection_model.cc (right):

http://codereview.chromium.org/8044004/diff/6001/ui/gfx/selection_model.cc#ne...
ui/gfx/selection_model.cc:18: if (start < end)
On 2011/09/29 02:20:21, msw wrote:
> Perhaps this condition deserves an explanation:
> "The caret position should trail the selection end's previous grapheme in this
> case [because/in order to/so that] ..."

this ctor is removed.

http://codereview.chromium.org/8044004/diff/6001/ui/gfx/selection_model.h
File ui/gfx/selection_model.h (right):

http://codereview.chromium.org/8044004/diff/6001/ui/gfx/selection_model.h#new...
ui/gfx/selection_model.h:49: // edge of |selection_end_|'s previous grapheme. It
is used when create a
On 2011/09/29 02:20:21, msw wrote:
> Grammar: "means *that the* cursor".
> Grammar: "It is used when creating" or "It is used to create".
> 
> I don't think specifics about the current use(s) in the second sentence is
> helpful. I think the first sentence of this comment sufficiently describes the
> purpose of this value, and the third sentence attempts to note that the caret
> position and placement are resolved on SetSelectionModel(), which is
important,
> but it does a poor job of describing that behavior and why it's necessary.
> Instead try:
> 
> TRAILING_OF_PREVIOUS_GRAPHEME means that the cursor will visually trail
> |selection_end_|'s previous grapheme or lead |selection_end_| if it has no
> previous grapheme. The actual caret position and placement are resolved in
> SetSelectionModel, where a previous grapheme can be calculated from the
current
> text.

it is removed.

http://codereview.chromium.org/8044004/diff/6001/ui/gfx/selection_model.h#new...
ui/gfx/selection_model.h:67: void set_selection_start(size_t pos) {
selection_start_ = pos; }
On 2011/09/29 02:20:21, msw wrote:
> Should set_selection_start also be made private?

Done.

http://codereview.chromium.org/8044004/diff/6001/ui/gfx/selection_model.h#new...
ui/gfx/selection_model.h:82: 
On 2011/09/29 02:20:21, msw wrote:
> You can remove this blank line.

Done.

http://codereview.chromium.org/8044004/diff/6001/ui/gfx/selection_model.h#new...
ui/gfx/selection_model.h:85: }
On 2011/09/29 02:20:21, msw wrote:
> You should add a blank line after this function, before the selection_start_
> comment.

Done.

http://codereview.chromium.org/8044004/diff/6001/views/touchui/touch_selectio...
File views/touchui/touch_selection_controller_impl_unittest.cc (right):

http://codereview.chromium.org/8044004/diff/6001/views/touchui/touch_selectio...
views/touchui/touch_selection_controller_impl_unittest.cc:155: // Test selection
range (3, 2).
On 2011/09/29 02:20:21, msw wrote:
> Please also test some other pertinent BiDi ranges.
> Maybe: (2,3) (3,4) (4,3) (1,4) (4,1) (3,6) (6,3), or as you see fit.
> You can split the test up if it gets too large.

Done.

[msw, 2011-10-04 07:25:41]

Mostly nits, I like this approach much better.

http://codereview.chromium.org/8044004/diff/15001/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

http://codereview.chromium.org/8044004/diff/15001/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:407: if (start ==
string16::npos || (current_text[start] != '?')) {
you can remove the braces for these conditional blocks.

http://codereview.chromium.org/8044004/diff/15001/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:694:
textfield_->SelectRange(range);
You could optionally inline this without additional lines at each callsite via:
textfield_->SelectRange(ui::Range([start], [end]));
I usually wouldn't add a convenience function for something that convenient.

http://codereview.chromium.org/8044004/diff/15001/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_view_views.h (right):

http://codereview.chromium.org/8044004/diff/15001/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_views.h:157: void
SelectRange(size_t caret, size_t end);
Please use |start| to match range and |end|, update comment and impl.
Alternately, take a range like the underlying Textfield::SelectRange.

http://codereview.chromium.org/8044004/diff/15001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

http://codereview.chromium.org/8044004/diff/15001/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:235: placement = SelectionModel::TRAILING;
Please DCHECK_LT(pos, end);

http://codereview.chromium.org/8044004/diff/15001/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:242: pos = end;
Initialize |pos| and |placement| with these 'default' values where they're
declared and eliminate this else block.

http://codereview.chromium.org/8044004/diff/15001/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:479: return SelectionModel(0, 0, SelectionModel::LEADING);
Can you please change this to "return LeftEndSelectionModel();"?

http://codereview.chromium.org/8044004/diff/15001/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:520: return SelectionModel(text().length(),
Can you please change this to "return RightEndSelectionModel();"?

http://codereview.chromium.org/8044004/diff/15001/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

http://codereview.chromium.org/8044004/diff/15001/ui/gfx/render_text.h#newcod...
ui/gfx/render_text.h:123: // The |range| is modified if invalid.
How is the range modified if invalid? It sounds like if the range is invalid,
it's a NO-OP and false is returned... The comment needs updating.

http://codereview.chromium.org/8044004/diff/15001/views/controls/textfield/na...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/8044004/diff/15001/views/controls/textfield/na...
views/controls/textfield/native_textfield_views.cc:236: drop_destination =
gfx::SelectionModel(drop_destination.selection_end(),
Why make this change? Constructing a new SelectionModel seems worse than setting
the selection start on the existing model; afaict the result is the same.

http://codereview.chromium.org/8044004/diff/15001/views/controls/textfield/te...
File views/controls/textfield/textfield_views_model.h (right):

http://codereview.chromium.org/8044004/diff/15001/views/controls/textfield/te...
views/controls/textfield/textfield_views_model.h:151: void
GetSelectedRange(ui::Range* range) const;
Please add a simple one line comment, like you have elsewhere:
// Gets the selected range.

http://codereview.chromium.org/8044004/diff/15001/views/controls/textfield/te...
views/controls/textfield/textfield_views_model.h:153: // The current composition
text will be confirmed. The
This is 3 lines if you wrap closer to 80 chars:
  // The current composition text will be confirmed. The selection starts with
  // the range's start position, and ends with the range's end position,
  // therefore the cursor position becomes the end position.

[xji, 2011-10-05 01:23:42]

I am not able to remove SelectionModel ctor in
TextfieldViewModel::MoveCursorTo(), I need either the ctor or the
set_selection_start() when 
render_text_->GetSelectionStart() != selection.select_start()
&&
render_text->GetSelectionStart() == render_text>GetSelectionStart()

I think in this case (when there is composition text),
render_text->GetSelectionStart() probably not equal to
render_text->GetSelectionStart(). But I am not sure and not comfortable to make
such change.

So, I am leaving the ctor there and leave them as public.

Also, I added test for NativeTextfieldViews::SelectRect() which reveals a bug in
either pango or the RenderTextLinux. I will further investigate and fix that
next.

http://codereview.chromium.org/8044004/diff/15001/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

http://codereview.chromium.org/8044004/diff/15001/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:407: if (start ==
string16::npos || (current_text[start] != '?')) {
On 2011/10/04 07:25:41, msw wrote:
> you can remove the braces for these conditional blocks.

Done.

http://codereview.chromium.org/8044004/diff/15001/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:694:
textfield_->SelectRange(range);
On 2011/10/04 07:25:41, msw wrote:
> You could optionally inline this without additional lines at each callsite
via:
> textfield_->SelectRange(ui::Range([start], [end]));
> I usually wouldn't add a convenience function for something that convenient.


Removed this convient function.
It was there originally, but I agree with you that it is not needed.

http://codereview.chromium.org/8044004/diff/15001/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_view_views.h (right):

http://codereview.chromium.org/8044004/diff/15001/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_views.h:157: void
SelectRange(size_t caret, size_t end);
On 2011/10/04 07:25:41, msw wrote:
> Please use |start| to match range and |end|, update comment and impl.
> Alternately, take a range like the underlying Textfield::SelectRange.

removed.

http://codereview.chromium.org/8044004/diff/15001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

http://codereview.chromium.org/8044004/diff/15001/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:235: placement = SelectionModel::TRAILING;
On 2011/10/04 07:25:41, msw wrote:
> Please DCHECK_LT(pos, end);

Done.

http://codereview.chromium.org/8044004/diff/15001/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:242: pos = end;
On 2011/10/04 07:25:41, msw wrote:
> Initialize |pos| and |placement| with these 'default' values where they're
> declared and eliminate this else block.

Done.

http://codereview.chromium.org/8044004/diff/15001/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:479: return SelectionModel(0, 0, SelectionModel::LEADING);
On 2011/10/04 07:25:41, msw wrote:
> Can you please change this to "return LeftEndSelectionModel();"?

Done.

http://codereview.chromium.org/8044004/diff/15001/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:520: return SelectionModel(text().length(),
On 2011/10/04 07:25:41, msw wrote:
> Can you please change this to "return RightEndSelectionModel();"?

Done.

http://codereview.chromium.org/8044004/diff/15001/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

http://codereview.chromium.org/8044004/diff/15001/ui/gfx/render_text.h#newcod...
ui/gfx/render_text.h:123: // The |range| is modified if invalid.
On 2011/10/04 07:25:41, msw wrote:
> How is the range modified if invalid? It sounds like if the range is invalid,
> it's a NO-OP and false is returned... The comment needs updating.

changed to

  // If the |range| start or end is greater than text length, it is modified to
be
  // the text length.

Other comments keep the same.

http://codereview.chromium.org/8044004/diff/15001/views/controls/textfield/na...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/8044004/diff/15001/views/controls/textfield/na...
views/controls/textfield/native_textfield_views.cc:236: drop_destination =
gfx::SelectionModel(drop_destination.selection_end(),
On 2011/10/04 07:25:41, msw wrote:
> Why make this change? Constructing a new SelectionModel seems worse than
setting
> the selection start on the existing model; afaict the result is the same.

Ah, this is not needed. drop_destination's selection_start is the same as
selection_end at this point.

http://codereview.chromium.org/8044004/diff/15001/views/controls/textfield/te...
File views/controls/textfield/textfield_views_model.h (right):

http://codereview.chromium.org/8044004/diff/15001/views/controls/textfield/te...
views/controls/textfield/textfield_views_model.h:151: void
GetSelectedRange(ui::Range* range) const;
On 2011/10/04 07:25:41, msw wrote:
> Please add a simple one line comment, like you have elsewhere:
> // Gets the selected range.

Done.

http://codereview.chromium.org/8044004/diff/15001/views/controls/textfield/te...
views/controls/textfield/textfield_views_model.h:153: // The current composition
text will be confirmed. The
On 2011/10/04 07:25:41, msw wrote:
> This is 3 lines if you wrap closer to 80 chars:
>   // The current composition text will be confirmed. The selection starts with
>   // the range's start position, and ends with the range's end position,
>   // therefore the cursor position becomes the end position.

Done.

[xji, 2011-10-05 01:38:05]

On 2011/10/05 01:23:42, xji wrote:
> I am not able to remove SelectionModel ctor in
> TextfieldViewModel::MoveCursorTo(), I need either the ctor or the
> set_selection_start() when 
> render_text_->GetSelectionStart() != selection.select_start()
> &&
> render_text->GetSelectionStart() == render_text>GetSelectionStart()

sorry for the typo, should be:
....
&&
render_text->GetSelectionStart() == selection.select_end()

> 
> I think in this case (when there is composition text),
> render_text->GetSelectionStart() probably not equal to
> render_text->GetSelectionStart(). But I am not sure and not 

render_text->GetSelectionStart() probably not equal to
selection.selection_end().

[xji, 2011-10-05 23:38:37]

On 2011/10/05 01:23:42, xji wrote:
> 
> Also, I added test for NativeTextfieldViews::SelectRect() which reveals a bug
in
> either pango or the RenderTextLinux. I will further investigate and fix that
> next.
> 

I talked with Behdad, what pango returns and the usage in RenderTextLinux are
correct.
We probably need to change the touch selection handle drag logic  to make the
test work. We will talk with varunjain about this tomorrow.

[msw, 2011-10-07 20:54:20]

http://codereview.chromium.org/8044004/diff/37003/views/controls/textfield/na...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/8044004/diff/37003/views/controls/textfield/na...
views/controls/textfield/native_textfield_views.cc:279: if
(start_pos.selection_start() == end_pos.selection_end())
Is it always safe to just use the SelectRange statement? Could the end_pos
caret/placement differ from those that would be set by SelectRange?

http://codereview.chromium.org/8044004/diff/37003/views/controls/textfield/te...
File views/controls/textfield/textfield_views_model.cc (right):

http://codereview.chromium.org/8044004/diff/37003/views/controls/textfield/te...
views/controls/textfield/textfield_views_model.cc:393: if
(render_text_->GetSelectionStart() != selection.selection_end())
Is it always safe to just use the SelectRange statement? Could the argument
caret/placement differ from those that would be set by SelectRange?

http://codereview.chromium.org/8044004/diff/37003/views/controls/textfield/te...
views/controls/textfield/textfield_views_model.cc:669: size_t next =
render_text_->GetIndexOfNextGrapheme(cursor);
Sadly, I forget why we do this; can you add a comment? Is it always safe to just
use the SelectRange statement? Could the |model| caret/placement differ from
those that would be set by SelectRange?

http://codereview.chromium.org/8044004/diff/37003/views/controls/textfield/te...
views/controls/textfield/textfield_views_model.cc:676: InsertTextInternal(text,
mergeable);
Are we doing the right thing here by setting the selection model before the text
is replaced? Could that set invalid non-grapheme-boundary positions? You can ask
Oshima if needed; he wrote the editing model.

[xji, 2011-10-07 21:42:04]

http://codereview.chromium.org/8044004/diff/37003/views/controls/textfield/na...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/8044004/diff/37003/views/controls/textfield/na...
views/controls/textfield/native_textfield_views.cc:279: if
(start_pos.selection_start() == end_pos.selection_end())
On 2011/10/07 20:54:20, msw wrote:
> Is it always safe to just use the SelectRange statement? Could the end_pos
> caret/placement differ from those that would be set by SelectRange?

I think you are asking about line 282.
When there is non-empty selection, using SelectRange (instead of setting only
the selection_start) is the right way.

For example, logical text "abcDEFhij", visually display as "abcFEDhij", if end's
selection model is (3, 3, 2, trailing), which means cursor is at "abc|FEDhij",
and if start's logical index is 4. the selection should be (4, 3) and [D] is
selected. The selection model should be (4, 3, 3, leading) which is what
SelectRange computed, not as (4, 3, 2, trailing) which is what we get by only
set_selection_start.

http://codereview.chromium.org/8044004/diff/37003/views/controls/textfield/te...
File views/controls/textfield/textfield_views_model.cc (right):

http://codereview.chromium.org/8044004/diff/37003/views/controls/textfield/te...
views/controls/textfield/textfield_views_model.cc:393: if
(render_text_->GetSelectionStart() != selection.selection_end())
On 2011/10/07 20:54:20, msw wrote:
> Is it always safe to just use the SelectRange statement? Could the argument
> caret/placement differ from those that would be set by SelectRange?

Using SelectRange() to recompute cursor's visual position is the right approach
than just resetting the selection_start. Please see detail explanation in
native_textfield_views

http://codereview.chromium.org/8044004/diff/37003/views/controls/textfield/te...
views/controls/textfield/textfield_views_model.cc:669: size_t next =
render_text_->GetIndexOfNextGrapheme(cursor);
On 2011/10/07 20:54:20, msw wrote:
> Sadly, I forget why we do this; can you add a comment? Is it always safe to
just
> use the SelectRange statement? Could the |model| caret/placement differ from
> those that would be set by SelectRange?

If there is non-empty selection range, we should use SelectRange() to recompute
the cursor's visual display (caret_pos/placement), and should not keep |model|'s
caret/placement. Please see the reply in native_textfield_views.cc for example.

I will add comments about 'next'

http://codereview.chromium.org/8044004/diff/37003/views/controls/textfield/te...
views/controls/textfield/textfield_views_model.cc:676: InsertTextInternal(text,
mergeable);
On 2011/10/07 20:54:20, msw wrote:
> Are we doing the right thing here by setting the selection model before the
text
> is replaced? Could that set invalid non-grapheme-boundary positions? You can
ask
> Oshima if needed; he wrote the editing model.

Before the text is replaced and when there is no selection, we need to set the
selection range as the range of next character (as in replace mode, which by
default is replacing next character with |text|).
Since we are using 'next=IndexOfNextGrapheme', we wont set it on
non-grapheme-boundary. There is test to test ReplaceChar() and ReplaceText() in
TextfieldViewModelTest.

[msw, 2011-10-07 23:59:11]

Each place I asked about the use of SelectRange, I was wondering if you change
the code to always call SelectRange, as opposed to switching the action based on
the equivalence of certain members.
=====
For example, instead of:
if (start_pos.selection_start() == end_pos.selection_end())
  model_->SelectSelectionModel(end_pos);
else
  model_->SelectRange(ui::Range(start_pos.selection_start(),
                                end_pos.selection_end()));
=====
Could you just do:
model_->SelectRange(ui::Range(start_pos.selection_start(),
                              end_pos.selection_end()));

[xji, 2011-10-08 00:15:34]

On 2011/10/07 23:59:11, msw wrote:
> Each place I asked about the use of SelectRange, I was wondering if you change
> the code to always call SelectRange, as opposed to switching the action based
on
> the equivalence of certain members.
> =====
> For example, instead of:
> if (start_pos.selection_start() == end_pos.selection_end())
>   model_->SelectSelectionModel(end_pos);
> else
>   model_->SelectRange(ui::Range(start_pos.selection_start(),
>                                 end_pos.selection_end()));
> =====
> Could you just do:
> model_->SelectRange(ui::Range(start_pos.selection_start(),
>                               end_pos.selection_end()));

Ah, misunderstood you.

Yes, the caret_pos/placement set in SelectRange() could be different.

In case of selection_start == selection_end,
SelectRange set the cursor position at (selection_end, leading), unless if
selection_end == text.length, it is (selection_end - 1, trailling).

I was thinking to make SelectRange() dcheck(start != end), but we indeed have
caller in upper layer (OmniboxViewView) that pass in
selection_start==selection_end. In the case that we do not have clear clue on
where the cursor should be put, put it to be (start_end, LEADING) is fine.

But in those places that you asked question, in which, we clearly knows the
cursor's visual position, it would be safe (and correct) to use that visual
position if condition satisfied.

[msw, 2011-10-08 01:15:06]

On 2011/10/08 00:15:34, xji wrote:
> On 2011/10/07 23:59:11, msw wrote:
> > Each place I asked about the use of SelectRange, I was wondering if you
change
> > the code to always call SelectRange, as opposed to switching the action
based
> on
> > the equivalence of certain members.
> > =====
> > For example, instead of:
> > if (start_pos.selection_start() == end_pos.selection_end())
> >   model_->SelectSelectionModel(end_pos);
> > else
> >   model_->SelectRange(ui::Range(start_pos.selection_start(),
> >                                 end_pos.selection_end()));
> > =====
> > Could you just do:
> > model_->SelectRange(ui::Range(start_pos.selection_start(),
> >                               end_pos.selection_end()));
> 
> Ah, misunderstood you.
> 
> Yes, the caret_pos/placement set in SelectRange() could be different.
> 
> In case of selection_start == selection_end,
> SelectRange set the cursor position at (selection_end, leading), unless if
> selection_end == text.length, it is (selection_end - 1, trailling).
> 
> I was thinking to make SelectRange() dcheck(start != end), but we indeed have
> caller in upper layer (OmniboxViewView) that pass in
> selection_start==selection_end. In the case that we do not have clear clue on
> where the cursor should be put, put it to be (start_end, LEADING) is fine.
> 
> But in those places that you asked question, in which, we clearly knows the
> cursor's visual position, it would be safe (and correct) to use that visual
> position if condition satisfied.

So, it would be wrong to just SelectRange regardless of the conditions in all
the cases I pointed out? Then this LGTM.

[oshima, 2011-10-08 01:40:47]

http://codereview.chromium.org/8044004/diff/37003/views/controls/textfield/te...
File views/controls/textfield/textfield_views_model.cc (right):

http://codereview.chromium.org/8044004/diff/37003/views/controls/textfield/te...
views/controls/textfield/textfield_views_model.cc:676: InsertTextInternal(text,
mergeable);
On 2011/10/07 21:42:04, xji wrote:
> On 2011/10/07 20:54:20, msw wrote:
> > Are we doing the right thing here by setting the selection model before the
> text
> > is replaced? Could that set invalid non-grapheme-boundary positions? You can
> ask
> > Oshima if needed; he wrote the editing model.
> 
> Before the text is replaced and when there is no selection, we need to set the
> selection range as the range of next character (as in replace mode, which by
> default is replacing next character with |text|).
> Since we are using 'next=IndexOfNextGrapheme', we wont set it on
> non-grapheme-boundary. There is test to test ReplaceChar() and ReplaceText()
in
> TextfieldViewModelTest.

Setting selection is necessary for "NO INSERT" mode editing.
When text is more than 1 char, we replace the same number (or max) of characters
that can be
replaced. This definition just happened to be this way and could be different
but i don't think
there is such real use case, so i just left that way.

So, I think just selecting one character (current to next) should be sufficient.

[xji, 2011-10-10 17:42:44]

On 2011/10/08 01:15:06, msw wrote:
> On 2011/10/08 00:15:34, xji wrote:
> > On 2011/10/07 23:59:11, msw wrote:
> > > Each place I asked about the use of SelectRange, I was wondering if you
> change
> > > the code to always call SelectRange, as opposed to switching the action
> based
> > on
> > > the equivalence of certain members.
> > > =====
> > > For example, instead of:
> > > if (start_pos.selection_start() == end_pos.selection_end())
> > >   model_->SelectSelectionModel(end_pos);
> > > else
> > >   model_->SelectRange(ui::Range(start_pos.selection_start(),
> > >                                 end_pos.selection_end()));
> > > =====
> > > Could you just do:
> > > model_->SelectRange(ui::Range(start_pos.selection_start(),
> > >                               end_pos.selection_end()));
> > 
> > Ah, misunderstood you.
> > 
> > Yes, the caret_pos/placement set in SelectRange() could be different.
> > 
> > In case of selection_start == selection_end,
> > SelectRange set the cursor position at (selection_end, leading), unless if
> > selection_end == text.length, it is (selection_end - 1, trailling).
> > 
> > I was thinking to make SelectRange() dcheck(start != end), but we indeed
have
> > caller in upper layer (OmniboxViewView) that pass in
> > selection_start==selection_end. In the case that we do not have clear clue
on
> > where the cursor should be put, put it to be (start_end, LEADING) is fine.
> > 
> > But in those places that you asked question, in which, we clearly knows the
> > cursor's visual position, it would be safe (and correct) to use that visual
> > position if condition satisfied.
> 
> So, it would be wrong to just SelectRange regardless of the conditions in all
> the cases I pointed out? 

Yes, that is right.
>Then this LGTM.

[xji, 2011-10-10 17:45:39]

On 2011/10/08 01:40:47, oshima wrote:
>
http://codereview.chromium.org/8044004/diff/37003/views/controls/textfield/te...
> File views/controls/textfield/textfield_views_model.cc (right):
> 
>
http://codereview.chromium.org/8044004/diff/37003/views/controls/textfield/te...
> views/controls/textfield/textfield_views_model.cc:676:
InsertTextInternal(text,
> mergeable);
> On 2011/10/07 21:42:04, xji wrote:
> > On 2011/10/07 20:54:20, msw wrote:
> > > Are we doing the right thing here by setting the selection model before
the
> > text
> > > is replaced? Could that set invalid non-grapheme-boundary positions? You
can
> > ask
> > > Oshima if needed; he wrote the editing model.
> > 
> > Before the text is replaced and when there is no selection, we need to set
the
> > selection range as the range of next character (as in replace mode, which by
> > default is replacing next character with |text|).
> > Since we are using 'next=IndexOfNextGrapheme', we wont set it on
> > non-grapheme-boundary. There is test to test ReplaceChar() and ReplaceText()
> in
> > TextfieldViewModelTest.
> 
> Setting selection is necessary for "NO INSERT" mode editing.
> When text is more than 1 char, we replace the same number (or max) of
characters
> that can be
> replaced. 

hmm..? I am confused.
If there is no existing selection, we always replace the next character (more
accurately, next grapheme), which is only 1 character, with text even there are
more than 1 characters in the text. Is it?


>This definition just happened to be this way and could be different
> but i don't think
> there is such real use case, so i just left that way.
> 
> So, I think just selecting one character (current to next) should be
sufficient.

[oshima, 2011-10-10 18:37:35]

On 2011/10/10 17:45:39, xji wrote:
> On 2011/10/08 01:40:47, oshima wrote:
> >
>
http://codereview.chromium.org/8044004/diff/37003/views/controls/textfield/te...
> > File views/controls/textfield/textfield_views_model.cc (right):
> > 
> >
>
http://codereview.chromium.org/8044004/diff/37003/views/controls/textfield/te...
> > views/controls/textfield/textfield_views_model.cc:676:
> InsertTextInternal(text,
> > mergeable);
> > On 2011/10/07 21:42:04, xji wrote:
> > > On 2011/10/07 20:54:20, msw wrote:
> > > > Are we doing the right thing here by setting the selection model before
> the
> > > text
> > > > is replaced? Could that set invalid non-grapheme-boundary positions? You
> can
> > > ask
> > > > Oshima if needed; he wrote the editing model.
> > > 
> > > Before the text is replaced and when there is no selection, we need to set
> the
> > > selection range as the range of next character (as in replace mode, which
by
> > > default is replacing next character with |text|).
> > > Since we are using 'next=IndexOfNextGrapheme', we wont set it on
> > > non-grapheme-boundary. There is test to test ReplaceChar() and
ReplaceText()
> > in
> > > TextfieldViewModelTest.
> > 
> > Setting selection is necessary for "NO INSERT" mode editing.
> > When text is more than 1 char, we replace the same number (or max) of
> characters
> > that can be
> > replaced. 
> 
> hmm..? I am confused.
> If there is no existing selection, we always replace the next character (more
> accurately, next grapheme), which is only 1 character, with text even there
are
> more than 1 characters in the text. Is it?

I think there is no such use case to replace with text longer than 1 character
wihtout selection.
We can either refractor method to deal with specific case, or add DCHECK to make
sure
we only replacing 1 character (when there is no selection) with proper
documentation.

> 
> 
> >This definition just happened to be this way and could be different
> > but i don't think
> > there is such real use case, so i just left that way.
> > 
> > So, I think just selecting one character (current to next) should be
> sufficient.