Bug 161894 – Undoing a candidate insertion results in the replaced text being selected

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tim Horton]

Undoing a candidate insertion results in the replaced text being selected

[Tim Horton]

Created attachment 288677 [details]
Patch

[Tim Horton]

rdar://problem/28225774

[Tim Horton]

Created attachment 288678 [details]
Patch

[Wenson Hsieh]

Comment on attachment 288678 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288678&action=review

> Source/WebCore/ChangeLog:24
> +        Make use of the new editor command to do candidate insertion as a single

Awesome! Will this also take care of the single EditorState update with selectionIsRange = true as well when inserting a candidate?

> Source/WebCore/editing/Editor.cpp:3641
> +            Ref<ReplaceRangeWithTextCommand> replaceCommand = ReplaceRangeWithTextCommand::create(range.get(), acceptedCandidate.replacement);

Can we use auto here?

[Simon Fraser (smfr)]

Comment on attachment 288678 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288678&action=review

> Source/WebCore/editing/Editor.cpp:3621
> +RefPtr<Range> Editor::rangeForTextCheckingResult(const TextCheckingResult& result) const

I can't parse this. Is it "range for text-checking-result", or "range for text, checking result"?

> Source/WebCore/editing/ReplaceRangeWithTextCommand.cpp:41
> +ReplaceRangeWithTextCommand::ReplaceRangeWithTextCommand(PassRefPtr<Range> rangeToBeReplaced, const String& text)

Do we have to still PassRefPtr<>?

> Source/WebCore/editing/ReplaceRangeWithTextCommand.cpp:52
> +    String previousText = plainText(m_rangeToBeReplaced.get());
> +    if (!previousText.length())
> +        return;

Why go to the cost of getting the previous text (which may be megabytes) rather than just asking the range if it's empty?

> Source/WebCore/editing/ReplaceRangeWithTextCommand.cpp:57
> +    if (!frame().selection().shouldChangeSelection(selection))
> +        return;

Would have been cheaper to do this first.

> Source/WebCore/editing/ReplaceRangeWithTextCommand.cpp:60
> +    if (!m_rangeToBeReplaced)
> +        return;

And this.

> Source/WebCore/editing/ReplaceRangeWithTextCommand.h:27
> +#ifndef ReplaceRangeWithTextCommand_h
> +#define ReplaceRangeWithTextCommand_h

The cool kids are using #pragma once now.

[Tim Horton]

(In reply to comment #5)
> Comment on attachment 288678 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=288678&action=review
> 
> > Source/WebCore/editing/Editor.cpp:3621
> > +RefPtr<Range> Editor::rangeForTextCheckingResult(const TextCheckingResult& result) const
> 
> I can't parse this. Is it "range for text-checking-result", or "range for
> text, checking result"?

The only argument is a TextCheckingResult :)

> > Source/WebCore/editing/ReplaceRangeWithTextCommand.cpp:41
> > +ReplaceRangeWithTextCommand::ReplaceRangeWithTextCommand(PassRefPtr<Range> rangeToBeReplaced, const String& text)
> 
> Do we have to still PassRefPtr<>?

No. I'll fix that.

> > Source/WebCore/editing/ReplaceRangeWithTextCommand.cpp:52
> > +    String previousText = plainText(m_rangeToBeReplaced.get());
> > +    if (!previousText.length())
> > +        return;
> 
> Why go to the cost of getting the previous text (which may be megabytes)
> rather than just asking the range if it's empty?

I don't know (I'm just mimicing spell checking code), but I do know that plainText().length() and the range length are two very different things.

> > Source/WebCore/editing/ReplaceRangeWithTextCommand.cpp:57
> > +    if (!frame().selection().shouldChangeSelection(selection))
> > +        return;
> 
> Would have been cheaper to do this first.
> 
> > Source/WebCore/editing/ReplaceRangeWithTextCommand.cpp:60
> > +    if (!m_rangeToBeReplaced)
> > +        return;
> 
> And this.
> 
> > Source/WebCore/editing/ReplaceRangeWithTextCommand.h:27
> > +#ifndef ReplaceRangeWithTextCommand_h
> > +#define ReplaceRangeWithTextCommand_h
> 
> The cool kids are using #pragma once now.

Sure.

[Tim Horton]

https://trac.webkit.org/changeset/205870

[Darin Adler]

Comment on attachment 288678 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288678&action=review

>>> Source/WebCore/editing/ReplaceRangeWithTextCommand.cpp:52
>>> +        return;
>> 
>> Why go to the cost of getting the previous text (which may be megabytes) rather than just asking the range if it's empty?
> 
> I don't know (I'm just mimicing spell checking code), but I do know that plainText().length() and the range length are two very different things.

There is no doubt that we can write a function that returns a boolean indicating whether plainText would be empty that is far more efficient than actually calling plainText and then length. But literally checking if the range is “empty” would not be the algorithm for that function.

[Darin Adler]

Comment on attachment 288678 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288678&action=review

> Source/WebCore/editing/Editor.cpp:3642
> +            Ref<ReplaceRangeWithTextCommand> replaceCommand = ReplaceRangeWithTextCommand::create(range.get(), acceptedCandidate.replacement);
> +            applyCommand(replaceCommand.ptr());

This kind of thing reads better without the local variable:

    applyCommand(ReplaceRangeWithTextCommand::create(range.get(), acceptedCandidate.replacement).ptr());