Bug 67569 - Rename confirmCompositionWithoutDisturbingSelection to cancelComposition
: Rename confirmCompositionWithoutDisturbingSelection to cancelComposition
Status: RESOLVED FIXED
: WebKit
HTML Editing
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 67377
:
  Show dependency treegraph
 
Reported: 2011-09-03 16:37 PST by
Modified: 2011-09-06 16:05 PST (History)


Attachments
cleanup (16.61 KB, patch)
2011-09-06 12:49 PST, Ryosuke Niwa
tonikitoo: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-09-03 16:37:09 PST
Per discussion on the bug 67377, we found that confirmCompositionWithoutDisturbingSelection is only used to cancel composition and the corresponding changeset http://trac.webkit.org/changeset/94497 made a change that enforces this notion.  We should rename this function to match the actual behavior now.
------- Comment #1 From 2011-09-06 12:49:15 PST -------
Created an attachment (id=106469) [details]
cleanup
------- Comment #2 From 2011-09-06 13:17:37 PST -------
So what if I really want to confirm? We are depending a lot of this method in our slightly outdated branch as we commit the composition instead of cancelling it in many cases (ie. moving from one field to another etc)
------- Comment #3 From 2011-09-06 13:20:21 PST -------
(In reply to comment #2)
> So what if I really want to confirm? We are depending a lot of this method in our slightly outdated branch as we commit the composition instead of cancelling it in many cases (ie. moving from one field to another etc)

r+ upon kenneth's concern is not an issue.

I was about to ask the same: if one want to cancel selection without actually disturbing selection.
------- Comment #4 From 2011-09-06 13:23:23 PST -------
(In reply to comment #3)
> (In reply to comment #2)
> > So what if I really want to confirm? We are depending a lot of this method in our slightly outdated branch as we commit the composition instead of cancelling it in many cases (ie. moving from one field to another etc)
> 
> r+ upon kenneth's concern is not an issue.
> 
> I was about to ask the same: if one want to cancel selection without actually disturbing selection.

We kind of need that. That is how our mobile platform works. We commit when loosing focus (could be just moving the cursor position with a touch in the same input field) and before injecting compositions into our virtual keyboard (used when the user touches a word and it becomes a composition)
------- Comment #5 From 2011-09-06 13:25:43 PST -------
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > So what if I really want to confirm? We are depending a lot of this method in our slightly outdated branch as we commit the composition instead of cancelling it in many cases (ie. moving from one field to another etc)
> > 
> > r+ upon kenneth's concern is not an issue.
> > 
> > I was about to ask the same: if one want to cancel selection without actually disturbing selection.
> 
> We kind of need that. That is how our mobile platform works. We commit when loosing focus (could be just moving the cursor position with a touch in the same input field) and before injecting compositions into our virtual keyboard (used when the user touches a word and it becomes a composition)

Although the patch is not chaning any actual behavior, but cleaning it up instead. So if it is/was broken for us with the patch, it will keep beeing.
------- Comment #6 From 2011-09-06 13:28:28 PST -------
(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (In reply to comment #2)
> > > > So what if I really want to confirm? We are depending a lot of this method in our slightly outdated branch as we commit the composition instead of cancelling it in many cases (ie. moving from one field to another etc)
> > > 
> > > r+ upon kenneth's concern is not an issue.
> > > 
> > > I was about to ask the same: if one want to cancel selection without actually disturbing selection.
> > 
> > We kind of need that. That is how our mobile platform works. We commit when loosing focus (could be just moving the cursor position with a touch in the same input field) and before injecting compositions into our virtual keyboard (used when the user touches a word and it becomes a composition)
> 
> Although the patch is not chaning any actual behavior, but cleaning it up instead. So if it is/was broken for us with the patch, it will keep beeing.

Sure, but the behavior was changed without me being aware of it. Examples of use:

http://gitorious.org/+qtwebkit-webkit2-dev/webkit/qtwebkit-webkit2-dev/blobs/master/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp

I think we are using it one other place, but I don't have the source handy right now.
------- Comment #7 From 2011-09-06 14:31:33 PST -------
(In reply to comment #6)
> Sure, but the behavior was changed without me being aware of it. Examples of use:
> 
> http://gitorious.org/+qtwebkit-webkit2-dev/webkit/qtwebkit-webkit2-dev/blobs/master/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp
> 
> I think we are using it one other place, but I don't have the source handy right now.

Kenneth and I chatted over this and he's going to investigate further if he can work-around by using other API.

I'm going to land this patch for now because I don't want to leave the function name mismatching to its behavior. We can always add new API for Qt if it's necessary.
------- Comment #8 From 2011-09-06 15:45:40 PST -------
(From update of attachment 106469 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=106469&action=review

> Source/WebCore/editing/Editor.cpp:1494
> -    if (preserveSelection)
> +    if (mode == ConfirmComposition)

Oops, this obviously needs to be "mode == CancelComposition". Will fix before landing the patch.
------- Comment #9 From 2011-09-06 16:05:30 PST -------
Committed r94613: <http://trac.webkit.org/changeset/94613>