Closed Bug 729774 Opened 12 years ago Closed 12 years ago

Don't include composition string in the result of retrieve_surrounding signal

Categories

(Core :: Widget: Gtk, defect)

Product:
Core
Component:
Widget: Gtk
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla13

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(3 files, 6 obsolete files)

part.1 nsGtkIMModule should manage composition state more closely
16.67 KB, patch
Details | Diff | Splinter Review
part.2 nsGtkIMModule should store selected text which will be removed by first text event
4.48 KB, patch
Details | Diff | Splinter Review
part.3 nsGtkIMModule::GetCurrentParagraph() and DeleteText() should work with the text content which was immediately before the latest compositionstart
10.53 KB, patch
karlt
: review+
Details | Diff | Splinter Review 
Now, we don't remove composition string from result of retrieve_surrounding signal at nsGtkIMModule::GetCurrentParagraph().
http://mxr.mozilla.org/mozilla-central/source/widget/gtk2/nsGtkIMModule.cpp#1441

The result will be returned by gtk_im_context_set_surrounding(). Its document said:
http://developer.gnome.org/gtk/unstable/GtkIMContext.html#gtk-im-context-set-surrounding
text :
	text surrounding the insertion point, as UTF-8. the preedit string should not be included within text.

I don't know actual problem by this bug, but we should fix the behavior.
Attached patch Patch (obsolete) — — Splinter Review 

        Attachment #599875 -
        Flags: review?(karlt)

    
    

    
  
Comment on attachment 599875 [details] [diff] [review]
Patch

My understanding here is limited.  mCompositionStart is set to the selection
offset in DispatchCompositionStart.  Can you explain for me, please, in what
situations mCompositionStart would no longer match the selection offset?

Should OnDeleteSurroundingNative also ignore the composition string when
handling offsets?

>+        PRUint32 selStartInComposition =
>+          NS_MIN(NS_MAX(selOffset, mCompositionStart), compEnd);
>+        PRUint32 selEndInComposition =
>+          NS_MAX(NS_MIN(selEnd, compEnd), mCompositionStart);

clamped is available from nsAlgorithm.h.

A couple of other things, not directly related to this patch, that I noticed:

nsGtkIMModule::GetSelectionOffset is declared but not defined.

OnRetrieveSurroundingNative treats cursorPos as a number of Unicode characters,
but I think it is actually a number of UTF16 codes in the sequence.
The two differ when there are surrogates.

    
    

    
  
(In reply to Karl Tomlinson (:karlt) from comment #2)
> My understanding here is limited.  mCompositionStart is set to the selection
> offset in DispatchCompositionStart.  Can you explain for me, please, in what
> situations mCompositionStart would no longer match the selection offset?

I did some experimenting and noticed that the selection is deleted when the pre-edit string is inserted, so the selection offset will move (I assume).
But then the selection is of zero length.  Is there a situation where the selection can be of non-zero length while the pre-edit string is shown?

    
    

    
  
(In reply to Karl Tomlinson (:karlt) from comment #2)
> My understanding here is limited.  mCompositionStart is set to the selection
> offset in DispatchCompositionStart.  Can you explain for me, please, in what
> situations mCompositionStart would no longer match the selection offset?

Oh, there is no such case, maybe. When I worked on TSF (Windows' new IME framework), I realized a fact. That is, nsEditor accepts trusted key events during composition. Therefore, I saw the caret is moving out of composition string if IME doesn't consume the native arrow key messages. However, nsGtkIMModule refuses all key events during composition.
http://mxr.mozilla.org/mozilla-central/source/widget/gtk2/nsGtkIMModule.cpp#415

However, I should confirm whether web applications can change normal selection by script during composition.

    
    

    
  
When events are dispatched to content, content could do all sorts of things, including running nested event loops.  But, if the composition is cancelled when the selection is changed by content, then perhaps there is not much to be concerned about.

If the selection can change during the composition (except for the changes from inserting the pre-edit string), then I wonder whether OnDeleteSurroundingNative and GetCurrentParagraph should be using mCompositionStart or the selection offset?

When the selection is replaced by the pre-edit string, the text returned by "retrieve-surrounding" will still change due to the selection being deleted.
Is it assumed that the gtk_im_context_set_cursor_location call is enough to tell the IME that the surrounding text may have changed?
I'm wondering whether perhaps the selection should not be included in the surrounding text.

    
    

    
  
Yeah, you're right.

    
    

    
  


  
How about this?

I have no idea to test this patch with Thai IME. I don't know Thai language...

        Attachment #599875 -
        Attachment is obsolete: true

        Attachment #601544 -
        Flags: review?(karlt)

        Attachment #599875 -
        Flags: review?(karlt)

    
    

    
  
It might not be necessary to change "delete-surrounding" signal handler. I guess that if it's composing by IME, it doesn't work fine.

    
    

    
  
I think that if it's composing, DeleteText() should finish current composition temporarily by dispatching empty text event and compositionend event. After that, change the selection and delete the text. Finally, it should restart composition by dispatching compositionstart and compositionupdate and text event.

    
    

    
  
I haven't thought carefully about the patch or comment 9 yet, but have started thinking about the issues, so I'll write down the results of my investigations to date.

    
    

    
  
gtk_im_context_reset() should be called when "a change such as a change in
cursor position has been made".  I think we should consider it a bug if this
is not called when the selection or cursor changes (expect in the situation
described below).

After gtk_im_context_reset(), we always DispatchCompositionEnd(),
which sets mIsComposing = false, and when !mIsComposing, DispatchTextEvent() calls
DispatchCompositionStart().  That means that mCompositionStart should correctly be updated to the new selection offset before any text event is dispatched after a selection change.

The one time mIsComposing is not set to false when the selection changes is
after a text event is dispatched for a preedit string.  In this situation only,
mCompositionStart does not match the selection offset, but the selection should be zero length and the cursor/offset is within the range of the preedit string.

    
    

    
  
In trying to understand GTK APIs such as these, it is often helpful to look at
how GTK uses them.  IMEs will be written to work with standard GTK widgets and
so, if Gecko behaves similarly to the GTK widgets, then behaviour should be as
expected.

I looked at GtkEntry
http://git.gnome.org/browse/gtk+/tree/gtk/gtkentry.c?h=gtk-2-24
(GtkTextView is the other client in GTK.  It is more complicated to follow,
but looks similar on the surface.)

GtkEntry keeps the preedit string separate from the editable text buffer, so the text buffer does not change until the "commit" signal.  This leads to the following effects:

1. Both retrieve-surrounding and delete-surrounding do not consider the
   preedit string.

2. Selected text remains part of the text buffer until the commit.

   The preedit string is shown beside the selection.  It is shown on the
   cursor side of the selection.  The cursor side of the selection depends on
   how the selection was created, but is not visually distinct from the other
   side.

3. Resetting the IM context during preedit leaves the selection in place.
   (Also, the preedit string is removed.)

4. Both retrieve-surrounding and delete-surrounding do consider the selection.

Effect 4 seems a bit strange, given the selection is deleted on commit.
That may mean that delete-surrounding and so also retrieve-surrounding are not
so useful when there is a selection.

Effects 2 and 3 differ from Gecko's behavior in which the selection is deleted
during preedit.  (And Gecko also leaves the preedit in place after reset.)
That is not directly part of this bug and may be intended behavior, but that
difference may mean that we should also handle retrieve-surrounding and
delete-surrounding differently from GtkEntry's behaviour.

retrieve-surrounding and delete-surrounding need to behave consistently to be
useful.  Otherwise delete-surrounding will delete different text from what is
intended.

Also, I think they should behave consistently between gtk_im_context_reset()
calls, because the IME doesn't get any other sort of notification that the
surrounding text may have changed.  i.e. I think that retrieve-surrounding
should return the same string whether sent before and after the selection is deleted, and delete-surrounding should delete essentially the same text whether sent before and after the selection is deleted.

    
    

    
  
(In reply to Karl Tomlinson (:karlt) from comment #12)
> Also, I think they should behave consistently between gtk_im_context_reset()
> calls, because the IME doesn't get any other sort of notification that the
> surrounding text may have changed.  i.e. I think that retrieve-surrounding
> should return the same string whether sent before and after the selection is
> deleted, and delete-surrounding should delete essentially the same text
> whether sent before and after the selection is deleted.

When I wrote this, I hadn't read the documentation for gtk_im_context_delete_surrounding():  "you should first call gtk_im_context_get_surrounding() to get the current context, and call this function immediately afterwards to make sure that you know what you are deleting."

That means that there is no need for these functions to continue to return or delete the same string through all stages of composing or not composing.

All that is important, it seems, is that delete-surrounding behaves in a manner that is consistent with what retrieve-surrounding returned or would return immediately prior to the delete-surrounding signal.

    
    

    
  
Comment on attachment 601544 [details] [diff] [review]
Remove composition string from the result of retrieve_surround signal and use composition string's offset instead of selection during composition

If what is returned from retrieve-surrounding is changed to exclude the
preedit string, then I think something needs to be done to delete the correct
text during delete-surrounding.

I imagined simply correcting the offsets from delete-surrounding for the
position of the preedit string.  This might delete the preedit string if it
falls within the range.  However, I'm not familiar with what the editor is
expecting while composing.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #9)
> I think that if it's composing, DeleteText() should finish current
> composition temporarily by dispatching empty text event and compositionend
> event. After that, change the selection and delete the text. Finally, it
> should restart composition by dispatching compositionstart and
> compositionupdate and text event.

It sounds a bit like this is trying to work around the editor not expecting
delete events while composing.  Is that right?
If so, then perhaps this is a reasonable approach.

Dispatching internal events sounds fine so long as this doesn't cause any
calls back into the IME.  If any events are sent to content, then content
event handlers may initiate nested event loops, which could confuse the IME,
but that may be difficult to deal with and may not be a new issue anyway.

I'm not clear whether sending an empty text event is the right thing if
composing has begun but there has been no preedit string dispatched.
i.e. when preedit-start has been received but no preedit-changed.  But I guess
that's not a situation we expect to be common.

OnDeleteSurroundingNative currently makes no effort to correct
mCompositionStart when deleting text before the cursor.  If the
NS_SELECTION_SET and NS_CONTENT_COMMAND_DELETE events don't cause the
composition to be cancelled, then this inconsistency and this bug (with the preedit string) suggest that the changes for bug 353776 only made delete-surrounding work appropriately when not composing and/or when deleting text after the composition.

A couple of comments on minor things:

>+    // When it's not composing by IME, we should use current selection.
>+    // Otherwise, we should not use current selection.

The second sentence here is too strong, I think, if comment 11 is accurate.

Using the selection offset+length instead of mCompositionStart would be fine
even when composing, I expect, but using mCompositionStart simplifies the
offset/length calculations.

>+    PRUint32 selOffset = mCompositionStart;
>+    PRUint32 selLength = 0;

There is a change in behavior here in an edge case:
If composing has begun but there has been no preedit string dispatched, then
the selection will still have non-zero length.  If the selection crosses
paragraph boundaries then the text after the insertion point won't be included.

Perhaps this is not a situation that is expected, or it is at least rare, but
if changing the mIsComposing test to use
mDispatchedCompositionString.IsEmpty() would resolve the discrepancy then that
seems simple enough.

>+        // XXX nsString::Find and nsString::RFind take PRInt32 for offset, so,
>+        //     we cannot support this request when the current offset is larger
>+        //     than PR_INT32_MAX.
>+        if (selOffset > PR_INT32_MAX || selLength > PR_INT32_MAX ||
>+            selOffset + selLength > PR_INT32_MAX) {
>+            PR_LOG(gGtkIMLog, PR_LOG_ALWAYS,
>+                ("    FAILED, The selection is out of range"));
>+            PR_LOG(gGtkIMLog, PR_LOG_ALWAYS,
>+                ("        selOffset=%u, selLength=%u",
>+                 selOffset, selLength));
>+            return NS_ERROR_FAILURE;
>+        }
>     }

mCompositionStart is PRUint32, so should this test be performed even when
using mCompositionStart?

        Attachment #601544 -
        Flags: review?(karlt) → review-

    
    

    
  
(In reply to Karl Tomlinson (:karlt) from comment #11)
> I think we should consider it a bug if this
> is not called when the selection or cursor changes (expect in the situation
> described below).

I agree with it. When I work on redesigning composition in XP level code, I should be careful for this.

(In reply to Karl Tomlinson (:karlt) from comment #12)
> GtkEntry keeps the preedit string separate from the editable text buffer, so
> the text buffer does not change until the "commit" signal.  This leads to
> the following effects:
> 
> 1. Both retrieve-surrounding and delete-surrounding do not consider the
>    preedit string.
> 
> 2. Selected text remains part of the text buffer until the commit.
> 
>    The preedit string is shown beside the selection.  It is shown on the
>    cursor side of the selection.  The cursor side of the selection depends on
>    how the selection was created, but is not visually distinct from the other
>    side.

Oh, I forgot this fact. Yes, we are removing selected string by first text event. The removing string is set to NS_COMPOSITION_START event's data member (by ESM, http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsEventStateManager.cpp#1374). I think that we should include this string in the result for consistency with GtkEntry. E.g., some IMEs might abort some jobs if we return unexpected result to IME which expects GtkEntry's behavior.

> 3. Resetting the IM context during preedit leaves the selection in place.
>    (Also, the preedit string is removed.)

I think we should restore the original selected text when it's canceled, at least on Gtk, in another bug.

> 4. Both retrieve-surrounding and delete-surrounding do consider the
> selection.
> 
> Effect 4 seems a bit strange, given the selection is deleted on commit.
> That may mean that delete-surrounding and so also retrieve-surrounding are
> not
> so useful when there is a selection.

I'm not sure what you said about this.

> Effects 2 and 3 differ from Gecko's behavior in which the selection is
> deleted
> during preedit.  (And Gecko also leaves the preedit in place after reset.)
> That is not directly part of this bug and may be intended behavior, but that
> difference may mean that we should also handle retrieve-surrounding and
> delete-surrounding differently from GtkEntry's behaviour.

Yeah, I should work more for emulating native behavior on each platform.

(In reply to Karl Tomlinson (:karlt) from comment #13)
> All that is important, it seems, is that delete-surrounding behaves in a
> manner that is consistent with what retrieve-surrounding returned or would
> return immediately prior to the delete-surrounding signal.

I agree.

(In reply to Karl Tomlinson (:karlt) from comment #14)
> I imagined simply correcting the offsets from delete-surrounding for the
> position of the preedit string.  This might delete the preedit string if it
> falls within the range.

Right. Probably, there is no IME which using retrieve-surrounding ant delete-surrounding during composition. However, our wrong implementation may block IME innovations. So, I think we should fix such bugs as far as possible.

> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #9)
> > I think that if it's composing, DeleteText() should finish current
> > composition temporarily by dispatching empty text event and compositionend
> > event. After that, change the selection and delete the text. Finally, it
> > should restart composition by dispatching compositionstart and
> > compositionupdate and text event.
> 
> It sounds a bit like this is trying to work around the editor not expecting
> delete events while composing.  Is that right?
> If so, then perhaps this is a reasonable approach.

Yes.

> I'm not clear whether sending an empty text event is the right thing if
> composing has begun but there has been no preedit string dispatched.
> i.e. when preedit-start has been received but no preedit-changed.  But I
> guess
> that's not a situation we expect to be common.

Oh, it's good point. I think that we should check mDispatchedCompositionString.

> OnDeleteSurroundingNative currently makes no effort to correct
> mCompositionStart when deleting text before the cursor.  If the
> NS_SELECTION_SET and NS_CONTENT_COMMAND_DELETE events don't cause the
> composition to be cancelled, then this inconsistency and this bug (with the
> preedit string) suggest that the changes for bug 353776 only made
> delete-surrounding work appropriately when not composing and/or when
> deleting text after the composition.

I think so. But for IME developers, we should fix this bug. A Google Japanese Input (mozc is open source version of it) developer said, they were waiting most major applications (including Firefox) implement these signals. Our buggy implementation may block some IME developers' work.

Thanks for your such great review!

    
    

    
  
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #15)

I've reordered my replies a bit.

> > 4. Both retrieve-surrounding and delete-surrounding do consider the
> > selection.
> > 
> > Effect 4 seems a bit strange, given the selection is deleted on commit.
> > That may mean that delete-surrounding and so also retrieve-surrounding are
> > not
> > so useful when there is a selection.
> 
> I'm not sure what you said about this.

I find it hard to imagine how the IME can make intelligent use of the
surrounding text context if it includes the selection and the selection is
going to be deleted from the surrounding text.  I could imagine the context is
useful for constraining or hinting at which subset of characters will be
input, but I would expect that it is the characters that will be adjacent to
the committed string that will be most helpful.
gtk_im_context_set_surrounding() does not tell the IME which part of the
surrounding context is the selection, so the IME doesn't know which part will be deleted nor which part will be adjacent after the commit.

Similarly with delete-surrounding, I found it hard to imagine how the IME can
choose which part of the surrounding text to delete if it doesn't know which
other part will also be deleted.

When retrieve-surrounding and delete-surrounding are used outside of a
composition, however, the selection wont be deleted, so perhaps it would make
sense to include the selection in this situation at least.

> > 2. Selected text remains part of the text buffer until the commit.
> > 
> >    The preedit string is shown beside the selection.  It is shown on the
> >    cursor side of the selection.  The cursor side of the selection depends on
> >    how the selection was created, but is not visually distinct from the other
> >    side.
> 
> Oh, I forgot this fact. Yes, we are removing selected string by first text
> event. The removing string is set to NS_COMPOSITION_START event's data
> member (by ESM,
> http://mxr.mozilla.org/mozilla-central/source/content/events/src/
> nsEventStateManager.cpp#1374). I think that we should include this string in
> the result for consistency with GtkEntry. E.g., some IMEs might abort some
> jobs if we return unexpected result to IME which expects GtkEntry's behavior.

I don't feel too worried about IMEs aborting if we behave a little differently
from GtkEntry here.  I guess you are right that there's a chance an IME might be surprised by the surrounding context changing when composition starts (because the selection gets deleted), but the documentation implies that the IME should be prepared for changes in the surrounding text returned.  I guess we just don't know whether this is important or not.  Perhaps the mozc developers have some opinions.

> > Effects 2 and 3 differ from Gecko's behavior in which the selection is
> > deleted
> > during preedit.  (And Gecko also leaves the preedit in place after reset.)
> > That is not directly part of this bug and may be intended behavior, but that
> > difference may mean that we should also handle retrieve-surrounding and
> > delete-surrounding differently from GtkEntry's behaviour.
> 
> Yeah, I should work more for emulating native behavior on each platform.

I quite like Gecko's behavior of displaying the preedit string where it will
be committed (in place of the selection).  It looks odd to me to have the
preedit string beside the selection, but I should let people who use IMEs make
the choices here.

> > 3. Resetting the IM context during preedit leaves the selection in place.
> >    (Also, the preedit string is removed.)
> 
> I think we should restore the original selected text when it's canceled, at
> least on Gtk, in another bug.

That sounds like it could be sensible.

    
    

    
  
(In reply to Karl Tomlinson (:karlt) from comment #16)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #15)
> 
> I've reordered my replies a bit.
> 
> > > 4. Both retrieve-surrounding and delete-surrounding do consider the
> > > selection.
> > > 
> > > Effect 4 seems a bit strange, given the selection is deleted on commit.
> > > That may mean that delete-surrounding and so also retrieve-surrounding are
> > > not
> > > so useful when there is a selection.
> > 
> > I'm not sure what you said about this.
> 
> I find it hard to imagine how the IME can make intelligent use of the
> surrounding text context if it includes the selection and the selection is
> going to be deleted from the surrounding text.  I could imagine the context
> is
> useful for constraining or hinting at which subset of characters will be
> input, but I would expect that it is the characters that will be adjacent to
> the committed string that will be most helpful.

There is a major function of Japanese IME which needs to know the selection range. That is reconversion. When user selects some Kanji characters which were committed as wrong (unexpected) result, user restarts the composition of the selected characters by pressing a shortcut key for the reconversion.

I looked mozc source code. Then, I find iBus has this inernal API:

ibus_engine_get_surrounding_text(engine, &text, &cursor_pos, &anchor_pos);

Probably the anchor_pos is computed by here:

https://github.com/phuang/ibus/blob/master/client/gtk2/ibusimcontext.c#L960

So, it works on GtkTextView() only. This seems there is no APIs like get_surrounding() for retrieving selection range. If we could, we should suggest to add such API on GTK or GDK, though.

> gtk_im_context_set_surrounding() does not tell the IME which part of the
> surrounding context is the selection, so the IME doesn't know which part
> will be deleted nor which part will be adjacent after the commit.
> 
> Similarly with delete-surrounding, I found it hard to imagine how the IME can
> choose which part of the surrounding text to delete if it doesn't know which
> other part will also be deleted.
> 
> When retrieve-surrounding and delete-surrounding are used outside of a
> composition, however, the selection wont be deleted, so perhaps it would make
> sense to include the selection in this situation at least.

Hmm... I see.

> > > 2. Selected text remains part of the text buffer until the commit.
> > > 
> > >    The preedit string is shown beside the selection.  It is shown on the
> > >    cursor side of the selection.  The cursor side of the selection depends on
> > >    how the selection was created, but is not visually distinct from the other
> > >    side.
> > 
> > Oh, I forgot this fact. Yes, we are removing selected string by first text
> > event. The removing string is set to NS_COMPOSITION_START event's data
> > member (by ESM,
> > http://mxr.mozilla.org/mozilla-central/source/content/events/src/
> > nsEventStateManager.cpp#1374). I think that we should include this string in
> > the result for consistency with GtkEntry. E.g., some IMEs might abort some
> > jobs if we return unexpected result to IME which expects GtkEntry's behavior.
> 
> I don't feel too worried about IMEs aborting if we behave a little
> differently
> from GtkEntry here.  I guess you are right that there's a chance an IME
> might be surprised by the surrounding context changing when composition
> starts (because the selection gets deleted), but the documentation implies
> that the IME should be prepared for changes in the surrounding text
> returned.  I guess we just don't know whether this is important or not. 
> Perhaps the mozc developers have some opinions.
> 
> > > Effects 2 and 3 differ from Gecko's behavior in which the selection is
> > > deleted
> > > during preedit.  (And Gecko also leaves the preedit in place after reset.)
> > > That is not directly part of this bug and may be intended behavior, but that
> > > difference may mean that we should also handle retrieve-surrounding and
> > > delete-surrounding differently from GtkEntry's behaviour.
> > 
> > Yeah, I should work more for emulating native behavior on each platform.
> 
> I quite like Gecko's behavior of displaying the preedit string where it will
> be committed (in place of the selection).  It looks odd to me to have the
> preedit string beside the selection, but I should let people who use IMEs
> make
> the choices here.

I think that preedit sting replaces the selection string is OK. It looks better for me. But I think if IME commits empty string, i.e., cancels the composition, we should restore the deleted selected string for native Look&Feel.

> > > 3. Resetting the IM context during preedit leaves the selection in place.
> > >    (Also, the preedit string is removed.)
> > 
> > I think we should restore the original selected text when it's canceled, at
> > least on Gtk, in another bug.
> 
> That sounds like it could be sensible.

Somebody might hate the behavior because they need to undo for restoring the deleted string.

    
    

    
  
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #17)
> There is a major function of Japanese IME which needs to know the selection
> range. That is reconversion. When user selects some Kanji characters which
> were committed as wrong (unexpected) result, user restarts the composition
> of the selected characters by pressing a shortcut key for the reconversion.
> 
> I looked mozc source code. Then, I find iBus has this inernal API:
> 
> ibus_engine_get_surrounding_text(engine, &text, &cursor_pos, &anchor_pos);
> 
> Probably the anchor_pos is computed by here:
> 
> https://github.com/phuang/ibus/blob/master/client/gtk2/ibusimcontext.c#L960
> 
> So, it works on GtkTextView() only.

Interesting.

> This seems there is no APIs like
> get_surrounding() for retrieving selection range. If we could, we should
> suggest to add such API on GTK or GDK, though.

Yes, I don't see any suitable APIs, and adding something to GTK sounds better than trying to invent some other conventions.

> But I think if IME commits empty string, i.e., cancels the
> composition, we should restore the deleted selected string for native
> Look&Feel.

Sounds good to me.

    
    

    
  
(In reply to Karl Tomlinson (:karlt) from comment #18)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #17)
> > But I think if IME commits empty string, i.e., cancels the
> > composition, we should restore the deleted selected string for native
> > Look&Feel.
> 
> Sounds good to me.

Hmm, this should be done by nsEditor side, I think. I filed bug 733646.

    
    

    
  


  
The DeleteText() implementation is a little bit hacky. I think that we should add new content command event for this later.

        Attachment #603616 -
        Flags: review?(karlt)

    
    

    
  
Comment on attachment 603613 [details] [diff] [review]
part.1 nsGtkIMModule should manage composition state more closely

>+    mCompositionState = (textEvent.rangeCount > 0) ?
>+        eCompositionState_TextEventDispatched :
>+        eCompositionState_CommitTextEventDispatched;

What is the reason for using the text ranges for the distinction here instead
of the "commit" signal?

    
    

    
  
Comment on attachment 603614 [details] [diff] [review]
part.2 nsGtkIMModule should store selected text which will be removed by first text event

Would storing the selection string when returned from the NS_COMPOSITION_START event be simpler than dispatching another NS_QUERY_SELECTED_TEXT event?

    
    

    
  
(In reply to Karl Tomlinson (:karlt) from comment #24)
> Comment on attachment 603613 [details] [diff] [review]
> part.1 nsGtkIMModule should manage composition state more closely
> 
> >+    mCompositionState = (textEvent.rangeCount > 0) ?
> >+        eCompositionState_TextEventDispatched :
> >+        eCompositionState_CommitTextEventDispatched;
> 
> What is the reason for using the text ranges for the distinction here instead
> of the "commit" signal?

Did you mean aCheckAttr? If so, we can use it.

(In reply to Karl Tomlinson (:karlt) from comment #25)
> Comment on attachment 603614 [details] [diff] [review]
> part.2 nsGtkIMModule should store selected text which will be removed by
> first text event
> 
> Would storing the selection string when returned from the
> NS_COMPOSITION_START event be simpler than dispatching another
> NS_QUERY_SELECTED_TEXT event?

I realized that compositionstart event handler can change selection before actually composition started. (Maybe, compositionupdate does it, it does not work or causes ResetInputState() called)

Therefore, we need to reget it after compositionupdate.

FYI: if compositionupdate changes selection, we might not dispatch text event, but it's a bug in all platforms.

    
    

    
  
Comment on attachment 603613 [details] [diff] [review]
part.1 nsGtkIMModule should manage composition state more closely

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #26)
> (In reply to Karl Tomlinson (:karlt) from comment #24)
> > What is the reason for using the text ranges for the distinction here instead
> > of the "commit" signal?
> 
> Did you mean aCheckAttr? If so, we can use it.

Yes.  A "commit" signal seems the designed indication of a committed string (rather than a lack of text attributes).  However, I don't know if it really matters, so this is fine as is or change it if you like.

If using the parameter, it would be nice to change the name from aCheckAttr to aIsCommit (and reversing the logic).  The documentation in the header is actually reversed.  I wonder about even passing eCompositionState_TextEventDispatched/CommitTextEventDispatched as the parameter so that it doesn't need to be boolean.

        Attachment #603613 -
        Flags: review?(karlt) → review+

    
    

    
  
Comment on attachment 603614 [details] [diff] [review]
part.2 nsGtkIMModule should store selected text which will be removed by first text event

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #26)
> (In reply to Karl Tomlinson (:karlt) from comment #25)
> > Would storing the selection string when returned from the
> > NS_COMPOSITION_START event be simpler than dispatching another
> > NS_QUERY_SELECTED_TEXT event?
> 
> I realized that compositionstart event handler can change selection before
> actually composition started. (Maybe, compositionupdate does it, it does not
> work or causes ResetInputState() called)

Note that mCompositionStart is set based on the selection before NS_COMPOSITION_START is sent, and mCompositionStart is the position where the selection will be restored, so I think that would be a better time to record the selection.  But either way, we are not wanting the selection to change so it shouldn't make much difference.  If you prefer this approach, that's OK.

        Attachment #603614 -
        Flags: review?(karlt) → review+

    
    

    
  
Comment on attachment 603629 [details] [diff] [review]
part.3 nsGtkIMModule::GetCurrentParagraph() and DeleteText() should work with the text content which was immediately before the latest compositionstart

>+        selLength = querySelectedTextEvent.mReply.mString.Length();
>+
>+    }

I guess the blank line here is not intentional.

>+        if (editorHadCompositionString &&
>+            !DispatchTextEvent(mSelectedString, false)) {
>+            PR_LOG(gGtkIMLog, PR_LOG_ALWAYS,
>+                ("    FAILED, quitting from DeletText"));
>+            ResetIME();
>+            return NS_ERROR_FAILURE;
>+        }
>+        if (!DispatchCompositionEnd()) {
>+            PR_LOG(gGtkIMLog, PR_LOG_ALWAYS,
>+                ("    FAILED, quitting from DeletText"));
>+            ResetIME();
>+            return NS_ERROR_FAILURE;

I worry that calling gtk_im_context_reset() here could confuse the IME.
Can we skip that here (and below)?

    
    

    
  
(In reply to Karl Tomlinson (:karlt) from comment #28)
> Comment on attachment 603614 [details] [diff] [review]
> part.2 nsGtkIMModule should store selected text which will be removed by
> first text event
> 
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #26)
> > (In reply to Karl Tomlinson (:karlt) from comment #25)
> > > Would storing the selection string when returned from the
> > > NS_COMPOSITION_START event be simpler than dispatching another
> > > NS_QUERY_SELECTED_TEXT event?
> > 
> > I realized that compositionstart event handler can change selection before
> > actually composition started. (Maybe, compositionupdate does it, it does not
> > work or causes ResetInputState() called)
> 
> Note that mCompositionStart is set based on the selection before
> NS_COMPOSITION_START is sent, and mCompositionStart is the position where
> the selection will be restored, so I think that would be a better time to
> record the selection.  But either way, we are not wanting the selection to
> change so it shouldn't make much difference.  If you prefer this approach,
> that's OK.

Ah, that's good point. I think that we should update mCompositionStart at there too.

(In reply to Karl Tomlinson (:karlt) from comment #29)
> Comment on attachment 603629 [details] [diff] [review]
> part.3 nsGtkIMModule::GetCurrentParagraph() and DeleteText() should work
> with the text content which was immediately before the latest
> compositionstart
> 
> >+        selLength = querySelectedTextEvent.mReply.mString.Length();
> >+
> >+    }
> 
> I guess the blank line here is not intentional.
> 
> >+        if (editorHadCompositionString &&
> >+            !DispatchTextEvent(mSelectedString, false)) {
> >+            PR_LOG(gGtkIMLog, PR_LOG_ALWAYS,
> >+                ("    FAILED, quitting from DeletText"));
> >+            ResetIME();
> >+            return NS_ERROR_FAILURE;
> >+        }
> >+        if (!DispatchCompositionEnd()) {
> >+            PR_LOG(gGtkIMLog, PR_LOG_ALWAYS,
> >+                ("    FAILED, quitting from DeletText"));
> >+            ResetIME();
> >+            return NS_ERROR_FAILURE;
> 
> I worry that calling gtk_im_context_reset() here could confuse the IME.
> Can we skip that here (and below)?

Hmm, yeah. But it's actually bad. I think that we should notify selection change after we finish handling an IME's signal. TSF (Windows) handling code has similar issue. So, I should find easy mechanism in XP level for fixing it. I'm adding it in my todo list...

        Attachment #604009 -
        Flags: review?(karlt) → review+

    
    

    
  
Hi,

Bug 754701 is a duplication of this bug?

    
    

    
  
(In reply to Karl Tomlinson (:karlt) from comment #2)
> OnRetrieveSurroundingNative treats cursorPos as a number of Unicode
> characters,
> but I think it is actually a number of UTF16 codes in the sequence.
> The two differ when there are surrogates.

Bug 757049 covers this.

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: