Closed Bug 520732 Opened 15 years ago Closed 14 years ago

Separate IME related code to another file from gtk2/nsWindow.cpp

Categories

(Core :: Widget: Gtk, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.3a4

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

(Keywords: inputmethod, intl)

Attachments

(2 files, 15 obsolete files)

Patch v5.0.1
102.05 KB, patch
karlt
: review+
Details | Diff | Splinter Review
follow up patch for bug 496360
609 bytes, patch
karlt
: review+
Details | Diff | Splinter Review 
I separated the IME related code from cocoa/nsChildView.mm and windows/nsWindow.cpp. It makes simple nsIWidget implementation and protects the IME related code from non-IME using developers. So, we should do it on GTK2 too. Then, we can remove #ifdef USE_XIM from nsWindow.cpp.

And also this might help bug 460056.

I have a question. Is #ifdef USE_XIM really needed now? If it's only needed for to clarify whether it's IME related code or not, we can remove them from new code.
(In reply to comment #0)
> Is #ifdef USE_XIM really needed now? If it's only needed for
> to clarify whether it's IME related code or not, we can remove them from new
> code.

It's defined unconditionally in the Makefiles, so it doesn't serve any configuration purpose.

I don't even think it clarifies the code.  The code uses GTK input modules, which may or may not use XIM.

So please feel free to remove USE_XIM.
thank you, karlt. I'll try it.

> 6789 /* static */
> 6790 void
> 6791 IM_preedit_changed_cb(GtkIMContext *aContext,
> 6792                       nsWindow     *aWindow)
> 6793 {
> 6794     gchar *preedit_string;
> 6795     gint cursor_pos;
> 6796     PangoAttrList *feedback_list;
> 6797 
> 6798     // if gFocusWindow is null, use the last focused gIMEFocusWindow
> 6799     nsRefPtr<nsWindow> window = gFocusWindow ? gFocusWindow : gIMEFocusWindow;
> 6800     if (!window || IM_get_input_context(window) != aContext)
> 6801         return;

> 6849 IM_commit_cb(GtkIMContext *aContext,
> 6850              const gchar  *aUtf8_str,
> 6851              nsWindow     *aWindow)
> 6852 {
> 6853     if (gIMESuppressCommit)
> 6854         return;
> 6855 
> 6856     LOGIM(("IM_commit_cb\n"));
> 6857 
> 6858     gKeyEventCommitted = PR_TRUE;
> 6859 
> 6860     // if gFocusWindow is null, use the last focused gIMEFocusWindow
> 6861     nsRefPtr<nsWindow> window = gFocusWindow ? gFocusWindow : gIMEFocusWindow;
> 6862 
> 6863     if (!window || IM_get_input_context(window) != aContext)
> 6864         return;
> 6865 

They're strange. We ignore the given nsWindow pointer. I think that we shouldn't do so ideally. Probably, the given pointer refers the focused context (but we should be careful bug 472635). I think that the current code might have some hidden bugs (might be related to bug 519913). I should check this issue...
Status: NEW → ASSIGNED
Attached patch Patch v0.4 (obsolete) — — Splinter Review 
This works fine on Ubuntu 9.04 (SCIM +Anthy) and Ubuntu 9.10 (uim + Anthy).

Can somebody test on some other environment?
Attachment #417047 - Attachment is obsolete: true
Looks like Korean IME and Chinese IMEs also work fine.

The dead keys works fine on password field.
Attached patch Patch v0.5 (obsolete) — — Splinter Review 
I changed ResetInputState(). This patch doesn't commit the composition string ourselves. This patch uses native commit event. This behavior should be better. And also this suppress the double committing.
Attachment #420292 - Attachment is obsolete: true
Ugh... The fired key events at composing is wrong.

* No keydown event is fired at starting composition.
* All keyup events are fired during composition.

I'll fix them in next patch.
Attached patch Patch v0.6 (obsolete) — — Splinter Review 
ok, this dispatches the events as following order:

* keydown
* compositionstart
* input
* compositionend
* keyup

However, SCIM, uim and iBus use snooper and they eat all (native) keypress events during composition. It includes the first keydown event trigger. So, the first keydown event isn't fired with those IMEs.
Attachment #420497 - Attachment is obsolete: true
Attached patch Patch v0.6.1 (obsolete) — — Splinter Review 
fix a mistake.
Attachment #420518 - Attachment is obsolete: true
Attached patch Patch v0.7 (obsolete) — — Splinter Review 
(In reply to comment #8)
> Created an attachment (id=420497) [details]
> Patch v0.5
> 
> I changed ResetInputState(). This patch doesn't commit the composition string
> ourselves. This patch uses native commit event. This behavior should be better.
> And also this suppress the double committing.

Sigh...

This approach is wrong because the composition events *after* focus moves to another content are dispatched on the new focused content.

This patch commits the composition string manually. And ignores all native composition events until we receive the native composition end event. By this change, the users can be confused by that any key typing has no effect until the composition is finished.

I'm still looking for the good landing point for the platform side problem.
Attachment #420521 - Attachment is obsolete: true
My current plan:

On blur (nsIContent level): Commits the composition string.
ResetInputState: Just calls ResetIME().
On focus (nsIContent level): Starts composition if IME is in composition.

For the first plan, I need to fix a bug of nsIMEStateManager.
Um, it seems that we cannot support uim and iBus which don't provide a way to commit forcedly until we change our IME event model and nsEditor. So, it should be separated from this bug.

I'll post a final patch which doesn't fix the uim/iBus issue.
Attached patch Patch v1.0 (obsolete) — — Splinter Review 
Katakai-san, would you review this?

When there is a composition and you click somewhere, the committing doesn't work fine with uim and iBus, but it should be fixed later because it needs pretty large changes in gecko. And it's not regression of this patch. Please ignore it.
Attachment #420674 - Attachment is obsolete: true
Attachment #423766 - Flags: review?(masaki.katakai)
Comment on attachment 423766 [details] [diff] [review]
Patch v1.0

And Karl, would you also review this? IME code should be reviewed by Katakai-san, so, you don't need to worry about the IME behavior.
Attachment #423766 - Flags: review?(karlt)
Hi Nakano-san, I'll try. Btw, shouldn't we change file name below for newly created files?

 * Original nsWindow.cpp Contributor(s):
(In reply to comment #18)
> Hi Nakano-san, I'll try. Btw, shouldn't we change file name below for newly
> created files?
> 
>  * Original nsWindow.cpp Contributor(s):

I meant that they have contributed on nsWindow.cpp which includes origin code of the new class.
Comment on attachment 423766 [details] [diff] [review]
Patch v1.0

Sorry for late. Looks OK for me. Actually key input will not work after implicit commit, but it should be OK.
Attachment #423766 - Flags: review?(masaki.katakai) → review+
Thank you, Katakai-san.

(In reply to comment #20)
> Actually key input will not work after implicit commit, but it should be OK.

What did you mean? I cannot understand this.
Hi Nakano-san,

Here is the behavior on my environment, Ubuntu 9.10.

1. turn conversion on
2. type something
   composed string appears
3. click somewhere
   composed string is committed
4. type something again
   nothing happens - composed string should appear

As you mentioned in comment #16, committing is not working properly when click somewhere while un-committed string exists. Is this a known behavior too? If so, I mean I could ignore.
Yes, it's a known issue. I'll try to fix it in another bug because it cannot fix only under widget/src/gtk2. We need to change nsEditor implementation and nsIMEStateManager.

Thank you again.
karlt:

Would you review the nsWindow part?
Masayuki, the original description sounded like this just involved moving code, but the comments here suggest that there are IME behavior changes too.
There are a few iterations here, so I'm unclear on which changes stayed in and which were reverted.

Can you summarize, please, the behavior changes between current and most recent patch, and the reasons for the changes?

One particular thing I'm interested in is uim.  We get quite a few crash reports in uim (e.g. bp-10b3ca92-9f30-4777-a8b7-cb3ff2100208) suggesting that uim is used a fair bit.  Comments above point to a problem with uim.  Does this patch do anything that might make that problem happen more often?
(In reply to comment #25)
> Can you summarize, please, the behavior changes between current and most recent
> patch, and the reasons for the changes?

The changes from originally nsWindow code:
1. Testers can log the IME related code behavior even if it's released build (this helps debugging some IMEs whom we cannot get).
2. The new code removes mSimpleContext on Maemo (for footprint).
3. Handling preedit_start and preedit_end signals newly, this needs to fix bug 283136. (Chinese IME doesn't send preedit_changed signal to applications)
4. Adding mIsIMFocused which is set to TRUE when gtk_im_context_focus_in is called. We're checking whether we were set focus to the IM context before we process . (I'm not sure there are actual bugs by this code.)
5. The patch uses nsQueryContentEvent events instead of the reply of nsCompositionEvent because it has been obsolete, see bug 543398. And also this fixes bug 283136 with Japanese IME.
6. OnKeyEvent() now filters all key events during composition. This is DOM3 composition events related behavior. (Currently, DOM keyup events are only dispatched only on Linux during composition.)
7. The new patch honors the given IM context at signal callback function. However, this may not work fine on deactivated window, see bug 519913. But this is correct change in widget level, XP level event handling is wrong.

So, 5, 6 and 7 changes actual behavior, but the others (maybe) are not so. And this patch improves focus management of IM context. This might fix some unknown bugs.

> One particular thing I'm interested in is uim.  We get quite a few crash
> reports in uim (e.g. bp-10b3ca92-9f30-4777-a8b7-cb3ff2100208) suggesting that
> uim is used a fair bit.  Comments above point to a problem with uim.  Does this
> patch do anything that might make that problem happen more often?

No, the patch doesn't change the destroying process. But I'll check it in another bug.
> 4. Adding mIsIMFocused which is set to TRUE when gtk_im_context_focus_in is
> called. We're checking whether we were set focus to the IM context before we
> process . (I'm not sure there are actual bugs by this code.)

before we process blur event.
(In reply to comment #25)
> Comments above point to a problem with uim.  Does this
> patch do anything that might make that problem happen more often?

Here I was thinking of the issues mentioned in comments 11, 15 and 16.
Do the symptoms of those issues all exist without the patch.
(In reply to comment #28)
> (In reply to comment #25)
> > Comments above point to a problem with uim.  Does this
> > patch do anything that might make that problem happen more often?
> 
> Here I was thinking of the issues mentioned in comments 11, 15 and 16.
> Do the symptoms of those issues all exist without the patch.

I don't think so because the problem which we cannot commit the composition of uim and iBus forcedly is the spec's problem of gtk immodule. 
http://library.gnome.org/devel/gtk/unstable/GtkIMContext.html#gtk-im-context-reset
> Notify the input method that a change such as a change in cursor position has been made. This will typically cause the input method to clear the preedit state.

The API document doesn't say the API *should* commit the composition. I contacted to uim community, then, they said that they think the API calling is just a notification. So, they chose a different interpretation from older IMEs.

So, the behavior was designed behavior for uim (and also maybe iBus). I think that the crash is just an accident of another reason.
(In reply to comment #29)
> I don't think so because...

Oops, please ignore here.

The crash isn't related to the problem (and my patch), I think.
I was interested in the answer re uim forced commit, thanks (unless there's some reason why that statement is not correct).

Yes, I assume the crash is unrelated.  For me, it was a data point indicating that uim has a significant user community.
Comment on attachment 423766 [details] [diff] [review]
Patch v1.0

The documentation here is very helpful, thanks, Masayuki.

>+ * Original nsWindow.cpp Contributor(s):

The wording of the licence block should not be changed unnecessarily.  The
terms have meanings in the MPL, so I think the best thing is to simply add all
names to "Contributor(s):".

>+    g_signal_connect(G_OBJECT(mContext), "preedit_changed",
>+                     G_CALLBACK(nsGtkIMModule::OnChangeCompositionCallback),
>+                     this);

The G_OBJECT() cast is not actually necessary here as the g_signal_connect
parameter is void*.

>+    mContext = gtk_im_multicontext_new();
>+    NS_ENSURE_TRUE(mContext, PR_FALSE);

Checking mContext etc is not necessary because these *_new() functions abort
on memory allocation failure.

>+nsresult
>+nsGtkIMModule::SetIMEEnabled(nsWindow* aCaller, PRUint32 aState)
>+{
>+    if (aState == mEnabled || NS_UNLIKELY(IsDestroyed())) {
>+        return NS_OK;
>+    }
>+
>+    PR_LOG(gGtkIMLog, PR_LOG_ALWAYS,
>+        ("GtkIMModule(%p): SetIMEEnabled, aCaller=%p, aState=%s",
>+         this, aCaller, GetEnabledStateName(aState)));
>+
>+    if (aCaller != mLastFocusedWindow) {
>+        PR_LOG(gGtkIMLog, PR_LOG_ALWAYS,
>+            ("    FAILED, the caller isn't focused window, mLastFocusedWindow=%p",
>+             mLastFocusedWindow));
>+        return NS_OK;

There seems to be a behavior change here but I'm not quite grasping how this
is meant to be used.

Is SetIMEEnabled called for each nsWindow or for one nsWindow controlling
the IME module for an entire hierarchy of windows?
IIUC the old behavior was for one nsWindow for an IMEData for the entire
hierarchy but with the new behavior, the specific window is relevant.

Is SetIMEEnabled called only when an nsWindow has focus?
Is SetIMEEnabled called each time focus is lost or gained?
The old behavior was that the enabled state was set even when the nsWindow
did not have focus.
The new behavior is that the enabled state is only set when the nsWindow has
focus, but the state is not cleared when the nsWindow loses focus, IIUC.

It's the (aCaller != mLastFocusedWindow) case that doesn't make sense to me.

>+nsresult
>+nsGtkIMModule::GetIMEEnabled(nsWindow* aCaller, PRUint32* aState)
>+{
>+    NS_ENSURE_ARG_POINTER(aState);
>+    *aState = mEnabled;
>+    return NS_OK;
>+}

Similarly, the aCaller parameter is not needed for this method.

>+nsGtkIMModule::OnStartCompositionCallback(GtkIMContext *aContext,
>+                                          nsGtkIMModule* aModule)
>+{
>+    PR_LOG(gGtkIMLog, PR_LOG_ALWAYS,
>+        ("GtkIMModule(%p): OnStartCompositionCallback, aContext=%p",
>+         aModule, aContext));
>+
>+    if (NS_UNLIKELY(!aContext) || NS_UNLIKELY(!aModule)) {
>+        PR_LOG(gGtkIMLog, PR_LOG_ALWAYS,
>+            ("    FAILED"));
>+        return;

No need for the NULL checks as a signal cannot be sent on a NULL object, and
aModule is the data passed to g_signal_connect.

That also means that the logging in the static callbacks is essentially
duplicating the logging in the OnEndCompositionNative method.

Similarly in OnEndCompositionCallback, OnChangeCompositionCallback, and
OnCommitCompositionCallback.

>+    mCompositionStartContent = selection.mReply.mContentsRoot;

Given that this is not cleared until preedit-end, it looks like it would be a
good idea to hold a reference until it is cleared.  (Otherwise 
ShouldIgnoreNativeCompositionEvent may end up matching the wrong content
sometimes.)  A weak reference would be good, if that is a possibility.

>+    NS_ConvertUTF8toUTF16 str(static_cast<const char*>(aUTF8Char));

The cast is not necessary.

>+    gtk_im_context_get_preedit_string(GetContext(), &preedit_string,

preedit_string needs to be passed to g_free.  (No need to NULL check.)
(In GetCompositionString and SetTextRangeList.)

BTW, docs say that pango_attr_list_unref is NULL-safe.

>+        aCompositionString =
>+            NS_ConvertUTF8toUTF16(static_cast<const char*>(preedit_string));

No cast needed.
Using CopyUTF8toUTF16() instead saves going through the temporary variable.

>+    nsWindow* rootWindow =
>+        static_cast<nsWindow*>(mLastFocusedWindow->GetTopLevelWidget());

Is NS_QUERY_TEXT_RECT relative to the toplevel GTK widget or the topmost Gecko
widget?

>+nsGtkIMModule::InitEvent(nsGUIEvent &aEvent)
>+{
>+    aEvent.refPoint.x = 0;
>+    aEvent.refPoint.y = 0;

nsEvent already initializes these in the constructor.

>+    nsrefcnt Release()
>+    {
>+        NS_PRECONDITION(mRefCnt != 0, "mRefCnt is alrady zero");
>+        --mRefCnt;
>+        NS_LOG_RELEASE(this, mRefCnt, "nsGtkIMModule");
>+        if (mRefCnt == 0) {
>+            delete this;
>+        }
>+        return mRefCnt;

Don't touch this->mRefCnt after deleting this.  Instead "return 0" after
delete.  Think about possibly using NS_IMPL_ADDREF etc.

>+    PRInt32 GetRefCount() { return mRefCnt; }

This doesn't appear to be used.

>+    virtual ~nsGtkIMModule();

Does this need to be virtual?

>+    // The owner window must release the contexts when it's destroyed becasue
>+    // the IME context needs the native window.

"because"

Probably worth mentioning OnDestroyWindow here because the owner window
doesn't directly destroy the contexts.

>+    // OnKeyEvent() sets the given native event to mProcessingKeyEvent.  And it

How about: "OnKeyEvent() temporarily sets mProcessingKeyEvent to the given
native event."

>+    // mIsComposing is set to TRUE when we dispatches the composition start
>+    // event.  And it's set to FALSE when we dispatches the composition end

"we dispatch the"

>+    PRPackedBool mDontFilterKeyEvent;

It might make things easier to understand if this were called mFilterKeyEvent
and the meaning was negated, unless there was a good reason for the "Dont".

>+    // When mIgnoreNativeCompositionEvent is TRUE, the all native composition
>+    // should be ignored excepting that the compositon should be restarted in
>+    // another contents.  Don't refer this value directly, use

Do you mean ", the native composition" or ", all native composition"?

"except that"

Do you mean "another content" or "another context"?

>+    // I.e., the forcus is in the normal editors or the password editors or

"focus"

>+    // Does something for platform bugs before destroying the context.
>+    void PrepareToDestroyContext(GtkIMContext *aContext);

"Called before destroying the context to work around some platform bugs."

>+static PRBool
>+IsAltTabKeyEvent(GdkEventKey *aEvent)
>+{
>+    return aEvent->keyval == GDK_Tab &&
>+        aEvent->state & GDK_CONTROL_MASK && aEvent->state & GDK_MOD1_MASK;
>+}

This is checking for Ctrl+Alt+TAB.  "IsCtrlAltTab" would be fine.

>+    nsCOMPtr<nsIWidget> kungFuDeathGrip = this;

kungFuDeathGrip looks unnecessary in DispatchKeyDownEvent because the callers
already hold a reference and DispatchKeyDownEvent doesn't touch *this after
dispatching the event.

>+    PRBool             DispatchKeyDownEvent(GdkEventKey *aEvent,
>+                                            PRBool *aIsCancelled);

Can you document the return value, please?

>+     * This is owned by top level widget or child window which is embedded in
>+     * non-our widget.

How about "This is owned by the top-level nsWindow or the topmost child
nsWindow embedded in a non-Gecko widget".

>+     * ancestor widget's instance.  So, one set of IM context is created for
>+     * each window.  If the children are released after the top level window
>+     * is released, the children refer vaild pointer, however, IME doesn't work

"one set of IM contexts is created for all windows in a hierarchy."
vaild -> valid
"the children still have a valid pointer,"
(In reply to comment #32)
>>+    mContext = gtk_im_multicontext_new();
>>+    NS_ENSURE_TRUE(mContext, PR_FALSE);
> 
> Checking mContext etc is not necessary because these *_new() functions abort
> on memory allocation failure.

Then, shouldn't we clean up something we did? E.g., mSimpleContext is null, shouldn't we disconnect mContext related stuff?

> >+nsresult
> >+nsGtkIMModule::SetIMEEnabled(nsWindow* aCaller, PRUint32 aState)
> >+{
> >+    if (aState == mEnabled || NS_UNLIKELY(IsDestroyed())) {
> >+        return NS_OK;
> >+    }
> >+
> >+    PR_LOG(gGtkIMLog, PR_LOG_ALWAYS,
> >+        ("GtkIMModule(%p): SetIMEEnabled, aCaller=%p, aState=%s",
> >+         this, aCaller, GetEnabledStateName(aState)));
> >+
> >+    if (aCaller != mLastFocusedWindow) {
> >+        PR_LOG(gGtkIMLog, PR_LOG_ALWAYS,
> >+            ("    FAILED, the caller isn't focused window, mLastFocusedWindow=%p",
> >+             mLastFocusedWindow));
> >+        return NS_OK;
> 
> There seems to be a behavior change here but I'm not quite grasping how this
> is meant to be used.
> 
> Is SetIMEEnabled called for each nsWindow or for one nsWindow controlling
> the IME module for an entire hierarchy of windows?

The former is.

> IIUC the old behavior was for one nsWindow for an IMEData for the entire
> hierarchy but with the new behavior, the specific window is relevant.
> 
> Is SetIMEEnabled called only when an nsWindow has focus?
> Is SetIMEEnabled called each time focus is lost or gained?

It's called when focus changed in gecko. I.e., at activating and deactivating, shouldn't be called.

> The old behavior was that the enabled state was set even when the nsWindow
> did not have focus.
> The new behavior is that the enabled state is only set when the nsWindow has
> focus, but the state is not cleared when the nsWindow loses focus, IIUC.
> 
> It's the (aCaller != mLastFocusedWindow) case that doesn't make sense to me.

It was a bug. I forgot this change, sorry. mLastFocusedWindow means last focused nsWindow in the top level window which owns the nsGtkIMModule. So, even if the method called on nsWindow when it doesn't have focus, it shouldn't modify the state of its *shared* IME context. When the widget will get focus, SetIMEEnabled() will be called.

Note that even if the window is deactive, mLastFocusedWindow refers the last focused window in the window because at activating the window, SetIMEEnabled() will not be called.

> >+nsGtkIMModule::OnStartCompositionCallback(GtkIMContext *aContext,
> >+                                          nsGtkIMModule* aModule)
> >+{
> >+    PR_LOG(gGtkIMLog, PR_LOG_ALWAYS,
> >+        ("GtkIMModule(%p): OnStartCompositionCallback, aContext=%p",
> >+         aModule, aContext));
> >+
> >+    if (NS_UNLIKELY(!aContext) || NS_UNLIKELY(!aModule)) {
> >+        PR_LOG(gGtkIMLog, PR_LOG_ALWAYS,
> >+            ("    FAILED"));
> >+        return;
> 
> No need for the NULL checks as a signal cannot be sent on a NULL object, and
> aModule is the data passed to g_signal_connect.

O.K.

> That also means that the logging in the static callbacks is essentially
> duplicating the logging in the OnEndCompositionNative method.
> 
> Similarly in OnEndCompositionCallback, OnChangeCompositionCallback, and
> OnCommitCompositionCallback.

I think that the logging should be there because it helps us for debugging on some environment which we don't have.

> >+    mCompositionStartContent = selection.mReply.mContentsRoot;
> 
> Given that this is not cleared until preedit-end, it looks like it would be a
> good idea to hold a reference until it is cleared.  (Otherwise 
> ShouldIgnoreNativeCompositionEvent may end up matching the wrong content
> sometimes.)  A weak reference would be good, if that is a possibility.

I designed so.

> >+    nsWindow* rootWindow =
> >+        static_cast<nsWindow*>(mLastFocusedWindow->GetTopLevelWidget());
> 
> Is NS_QUERY_TEXT_RECT relative to the toplevel GTK widget or the topmost Gecko
> widget?

Yes, it is.
(In reply to comment #33)
> > >+nsGtkIMModule::OnStartCompositionCallback(GtkIMContext *aContext,
> > >+                                          nsGtkIMModule* aModule)
> > >+{
> > >+    PR_LOG(gGtkIMLog, PR_LOG_ALWAYS,
> > >+        ("GtkIMModule(%p): OnStartCompositionCallback, aContext=%p",
> > >+         aModule, aContext));
> > >+
> > >+    if (NS_UNLIKELY(!aContext) || NS_UNLIKELY(!aModule)) {
> > >+        PR_LOG(gGtkIMLog, PR_LOG_ALWAYS,
> > >+            ("    FAILED"));
> > >+        return;
> > 
> > No need for the NULL checks as a signal cannot be sent on a NULL object, and
> > aModule is the data passed to g_signal_connect.
> 
> O.K.
> 
> > That also means that the logging in the static callbacks is essentially
> > duplicating the logging in the OnEndCompositionNative method.
> > 
> > Similarly in OnEndCompositionCallback, OnChangeCompositionCallback, and
> > OnCommitCompositionCallback.
> 
> I think that the logging should be there because it helps us for debugging on
> some environment which we don't have.

Ah, I see, ignore this reply.
> Think about possibly using NS_IMPL_ADDREF etc.

I cannot use this because it's not a subclass of nsISupports. Looks like there are no macros.
(In reply to comment #35)
> > Think about possibly using NS_IMPL_ADDREF etc.
> 
> I cannot use this because it's not a subclass of nsISupports. Looks like there
> are no macros.

Ah, there is THEBES_INLINE_DECL_REFCOUNTING(). However, probably, we shouldn't use this macro from widget. I think that such macro should be in nsAutoPtr.h or somewhere.
Attached patch Patch v2.0 (obsolete) — — Splinter Review 
fixed most issues except the pending issues in comment 32.

And I removed LOGIM related stuff, I forgot to do that.
Attachment #423766 - Attachment is obsolete: true
Attachment #428635 - Flags: review?(karlt)
Attachment #423766 - Flags: review?(karlt)
(In reply to comment #36)
> (In reply to comment #35)
> > > Think about possibly using NS_IMPL_ADDREF etc.
> > 
> > I cannot use this because it's not a subclass of nsISupports. Looks like there
> > are no macros.

> I think that such macro should be in nsAutoPtr.h or somewhere.

Yes, that may be a sensible solution for the future.

I think it would be possible to use NS_IMPL_ADDREF with NS_DECL_OWNINGTHREAD
even without inheriting from nsISupports.  nsCSSRule and nsCSSFrameConstructor
do this.  But, I'm not too concerned about this.

(In reply to comment #33)
> (In reply to comment #32)
> >>+    mContext = gtk_im_multicontext_new();
> >>+    NS_ENSURE_TRUE(mContext, PR_FALSE);
> > 
> > Checking mContext etc is not necessary because these *_new() functions abort
> > on memory allocation failure.
> 
> Then, shouldn't we clean up something we did? E.g., mSimpleContext is null,
> shouldn't we disconnect mContext related stuff?

mSimpleContext won't be null.  Either gtk_im_multicontext_new() will succeed
or it will abort (kill) the whole process.

> It was a bug. I forgot this change, sorry. mLastFocusedWindow means last
> focused nsWindow in the top level window which owns the nsGtkIMModule. So, even
> if the method called on nsWindow when it doesn't have focus, it shouldn't
> modify the state of its *shared* IME context. When the widget will get focus,
> SetIMEEnabled() will be called.
> 
> Note that even if the window is deactive, mLastFocusedWindow refers the last
> focused window in the window because at activating the window, SetIMEEnabled()
> will not be called.

Thanks for the explanation.  I don't have a good understanding here, but will
trust your assertion that this is the right thing to do.

> > >+    mCompositionStartContent = selection.mReply.mContentsRoot;
> > 
> > Given that this is not cleared until preedit-end, it looks like it would be a
> > good idea to hold a reference until it is cleared.  (Otherwise 
> > ShouldIgnoreNativeCompositionEvent may end up matching the wrong content
> > sometimes.)  A weak reference would be good, if that is a possibility.
> 
> I designed so.

I don't understand.  ShouldIgnoreNativeCompositionEvent() uses the value of
this pointer, but I don't see anything that will clear this pointer if the
content is destroyed between DispatchCompositionStart() and
OnEndCompositionNative().

> 
> > >+    nsWindow* rootWindow =
> > >+        static_cast<nsWindow*>(mLastFocusedWindow->GetTopLevelWidget());
> > 
> > Is NS_QUERY_TEXT_RECT relative to the toplevel GTK widget or the topmost Gecko
> > widget?
> 
> Yes, it is.

The toplevel GTK widget and the topmost Gecko widget will be different in an
embedded gecko, so it is important to choose whether either GetTopLevelWidget
or GetMozContainerWidget is appropriate here.

Given that nsContentEventHandler::OnQueryTextRect() uses
ConvertToRootViewRelativeOffset(), I'm guessing that GetMozContainerWidget() is
more appropriate.
> > It was a bug. I forgot this change, sorry. mLastFocusedWindow means last
> > focused nsWindow in the top level window which owns the nsGtkIMModule. So, even
> > if the method called on nsWindow when it doesn't have focus, it shouldn't
> > modify the state of its *shared* IME context. When the widget will get focus,
> > SetIMEEnabled() will be called.
> > 
> > Note that even if the window is deactive, mLastFocusedWindow refers the last
> > focused window in the window because at activating the window, SetIMEEnabled()
> > will not be called.
> 
> Thanks for the explanation.  I don't have a good understanding here, but will
> trust your assertion that this is the right thing to do.

An unfocused window in a toplevel window shouldn't change the shared IME state (in the toplevel window).

IME enabled state is being managed per every widget. However, GTK2 widget shared one state is shared per every top level widget. Even if the unfocused state is unexpected state, it's not matter because when it'll be focused, SetIMEEnabled() must be called.

> > > >+    mCompositionStartContent = selection.mReply.mContentsRoot;
> > > 
> > > Given that this is not cleared until preedit-end, it looks like it would be a
> > > good idea to hold a reference until it is cleared.  (Otherwise 
> > > ShouldIgnoreNativeCompositionEvent may end up matching the wrong content
> > > sometimes.)  A weak reference would be good, if that is a possibility.
> > 
> > I designed so.
> 
> I don't understand.  ShouldIgnoreNativeCompositionEvent() uses the value of
> this pointer, but I don't see anything that will clear this pointer if the
> content is destroyed between DispatchCompositionStart() and
> OnEndCompositionNative().

But only refers the pointer address, is not using the content actually. I want to check whether the current target for current composition events are changed or not. If the content was dead or unfocused already, the new content pointer must be different. However, I forgot to add the code which restarts the new composition. I'll add that in next patch.

> > > >+    nsWindow* rootWindow =
> > > >+        static_cast<nsWindow*>(mLastFocusedWindow->GetTopLevelWidget());
> > > 
> > > Is NS_QUERY_TEXT_RECT relative to the toplevel GTK widget or the topmost Gecko
> > > widget?
> > 
> > Yes, it is.
> 
> The toplevel GTK widget and the topmost Gecko widget will be different in an
> embedded gecko, so it is important to choose whether either GetTopLevelWidget
> or GetMozContainerWidget is appropriate here.
> 
> Given that nsContentEventHandler::OnQueryTextRect() uses
> ConvertToRootViewRelativeOffset(), I'm guessing that GetMozContainerWidget() is
> more appropriate.

Oh... Right.
(In reply to comment #39)
> But only refers the pointer address, is not using the content actually. I want
> to check whether the current target for current composition events are changed
> or not. If the content was dead or unfocused already, the new content pointer
> must be different.

If the old target is destroyed the new target may be allocated at the same address.  This is even quite likely if the structures for the old and new target are the same size.

To ensure that the targets don't appear to be the same when the old target has been destroyed, it is necessary to either keep the old target alive (strong reference) or clear the pointer when the old target is destroyed (weak reference).
Attached patch Patch v3.0 (obsolete) — — Splinter Review 
O.K. This handles the blur event of editors. So, we don't need mCompositionStartContent now.

The changes in nsIMEStateManager and nsIWidget are necessary because we have bug 496360. (And also even if there is not the bug, the other notification listening isn't necessary for Mac and Linux.)

> The toplevel GTK widget and the topmost Gecko widget will be different in an
> embedded gecko, so it is important to choose whether either GetTopLevelWidget
> or GetMozContainerWidget is appropriate here.
> 
> Given that nsContentEventHandler::OnQueryTextRect() uses
> ConvertToRootViewRelativeOffset(), I'm guessing that GetMozContainerWidget() is
> more appropriate.

I tested on embedded build (TestGtkEmbed), but the candidate window position is correct by the current patch. So, we don't need to change it.
Attachment #428635 - Attachment is obsolete: true
Attachment #430024 - Flags: review?(karlt)
Attachment #428635 - Flags: review?(karlt)
Attached patch Patch v3.0 (obsolete) — — Splinter Review 
failed to merge... :-(
Attachment #430024 - Attachment is obsolete: true
Attachment #430026 - Flags: review?(karlt)
Attachment #430024 - Flags: review?(karlt)
(In reply to comment #41)
> I tested on embedded build (TestGtkEmbed), but the candidate window position is
> correct by the current patch. So, we don't need to change it.

Thanks for testing.  Sorry, I was thinking of nsWindow::GetToplevelWidget(GtkWidget*), but you are using nsBaseWidget::GetTopLevelWidget(), which should do the right thing I expect.
Comment on attachment 430026 [details] [diff] [review]
Patch v3.0

nsGtkIMModule::Init() always returns true now so the return value and the
check in the nsGtkIMModule are now unnecessary.

>+    gtk_im_context_get_preedit_string(GetContext(), &preedit_string,

GetCompositionString looks good now, but SetTextRangeList is still leaking
preedit_string.

>+    // just clean up composition if there is it and it's related to the
>+    // destroying child window.

"just clean up any existing composition if it's related to the"?

>+    mFilterKeyEvent(PR_FALSE), mIgnoreNativeCompositionEvent(PR_FALSE)

I would usually not initialize mFilterKeyEvent here.  The value here doesn't
really mean anything.  By leaving it uninitialized here, valgrind will catch
if it is ever not initialized in the correct place.

    // Reset the current composition of IME.  If aDiscardCommitEvent is TRUE,
    // all composition events during this processing are ignored.
    void ResetIME(PRBool aDiscardCommitEvent);

This is only used with aDiscardCommitEvent == PR_TRUE, so the parameter seems
unnecessary.

>+     * aNeededNotification is not NULL when aFocus is true.  Then, the
>+     * implementation can set the follosing flags.  The caller must initialize
>+     * aNeededNotifications to NOTIFY_NONE.
>      */
>-    NS_IMETHOD OnIMEFocusChange(PRBool aFocus) = 0;
>+    enum {
>+      NOTIFY_NONE                                   = 0x00,
>+      NOTIFY_BLUR                                   = 0x01,
>+      NOTIFY_TEXT_CHANGE                            = 0x02,
>+      NOTIFY_SELECTION_CHANGE                       = 0x04,
>+      NOTIFY_ALL                                    = 0xFF
>+    };

I didn't understand immediately that the focus notification is always sent.

I first wondered about changing NOTIFY_NONE to NOTIFY_FOCUS_ONLY, but I'm not sure that would be a good change.  It seems a little strange to me to send focus notifications but not blur notifications.

What do you think about this?:

* Blur notifications are always sent if the OnIMEFocusChange on focus-in
  call succeeded.

* The second parameter to OnIMEFocusChange could be named
  aOtherNotifications and the enum would not need NOTIFY_BLUR.
I'm having trouble understanding mIgnoreNativeCompositionEvent and
OnIMEFocusChange/OnFocusChangeInGecko.

mIgnoreNativeCompositionEvent is only set to true in ResetIME.
Is this to ignore signals dispatched during gtk_im_context_reset()?

mIgnoreNativeCompositionEvent is reset to false on the preedit-end signal.
Is the expectation that gtk_im_context_reset() will result in a preedit-end
signal if there is a preedit in progress?

I'm confused by OnFocusChangeInGecko resetting mIgnoreNativeCompositionEvent
to false but not calling ResetIME.
Is OnFocusChangeInGecko going to be called during gtk_im_context_reset?  Is
this meant to indicate that preedit changes and commits that began while focus
was elsewhere and were being ignored should now be accepted in the new
content?

nsIWidget::OnIMEFocusChange() is called from
nsIMEStateManager::OnTextStateBlur(),
and nsIWidget::ResetInputState() is called from
nsIMEStateManager::OnChangeFocus().
Can you explain for me please, the desired difference in behavior between these two situations please?

Is a OnTextStateBlur() call going to be soon followed by a OnChangeFocus()?
What is the desired effect of that on mIgnoreNativeCompositionEvent?
(In reply to comment #46)
> mIgnoreNativeCompositionEvent is only set to true in ResetIME.
> Is this to ignore signals dispatched during gtk_im_context_reset()?

Yes. The API's behavior depends on the IME. Some IME might commit the last composition string, others might cancel it. For our behavior consistency between IMEs, we're manually commit/cancel the composition string. Therefore, we need to ignore the native signals.

> mIgnoreNativeCompositionEvent is reset to false on the preedit-end signal.
> Is the expectation that gtk_im_context_reset() will result in a preedit-end
> signal if there is a preedit in progress?

Yes. Our current expected behavior of gtk_im_context_reset() is the active IME commit or cancel the current composition.

However, we have found some IMEs not so, but it will be worked on another bug. So, we should keep the current expectation in the code.

> I'm confused by OnFocusChangeInGecko resetting mIgnoreNativeCompositionEvent
> to false but not calling ResetIME.
> Is OnFocusChangeInGecko going to be called during gtk_im_context_reset?  Is
> this meant to indicate that preedit changes and commits that began while focus
> was elsewhere and were being ignored should now be accepted in the new
> content?

Yes. If we keep to ignore the native signals, any key events return no reaction visually if the user uses uim or iBus which don't commit/cancel the composition by gtk_im_context_reset(). It makes the users more confusing. (*1)

> nsIWidget::OnIMEFocusChange() is called from
> nsIMEStateManager::OnTextStateBlur(),
> and nsIWidget::ResetInputState() is called from
> nsIMEStateManager::OnChangeFocus().
> Can you explain for me please, the desired difference in behavior between these
> two situations please?

ResetInputState() isn't good name. That should be CommitComposition() right now. So, ResetInputState() is called from other points. OnIMEFocusChange() is called only when the focus is moved to/from a editor.

> Is a OnTextStateBlur() call going to be soon followed by a OnChangeFocus()?

Yes.

> What is the desired effect of that on mIgnoreNativeCompositionEvent?

ResetInputState() which is called by OnChangeFocus() may do nothing. If mIsComposing is false, ResetInputState() just returns. Note that mIsComposing means our composition state, not the actual IME's state. If gtk_im_context_reset() doesn't commit immediately with some IMEs, mIsComposing could be false but there is active composition in IME. So, OnIMEFocusChange() should cancel the hacky behavior which can confuse the users.

The related code which you were confused by is not stable. They will be fixed when we will solve the uim/iBus problem...
Attached patch Patch v4.0 (obsolete) — — Splinter Review 

        Attachment #430026 -
        Attachment is obsolete: true

        Attachment #433026 -
        Flags: review?(karlt)

        Attachment #430026 -
        Flags: review?(karlt)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v4.1 (obsolete)
        — 
          — Splinter Review
      

    


  
fix a mistake.

        Attachment #433026 -
        Attachment is obsolete: true

        Attachment #433053 -
        Flags: review?(karlt)

        Attachment #433026 -
        Flags: review?(karlt)

    
    

    
  
Thanks for the answers to all my questions.  That helps me quite a bit.
So IIUC, the point of OnFocusChangeInGecko resetting
mIgnoreNativeCompositionEvent is to accept input from uim or iBus after a
ResetInputState() or CancelIMEComposition() does not produce a preedit-end and
only when the focused content changes.  I assume the reason for not accepting
the input when the focused content is still the same is that an older version
of the composition has already been committed (with ResetInputState() at
least) to that content, and so accepting further (likely duplicate) input
would not be helpful.

However, if OnTextStateBlur() is always followed by a OnChangeFocus(), then
ResetInputState will set mIgnoreNativeCompositionEvent to true again unless
mIsComposing is false.

mIsComposing is reset to false only in DispatchCompositionEnd, which is
called only on preedit-end signal and from CommitCompositionBy.
If preedit-end is received, then mIgnoreNativeCompositionEvent will be reset
to false anyway.

I can't think how CommitCompositionBy might be called between
OnTextStateBlur() and OnChangeFocus()?

    
    

    
  
Ah, you're right. The quirks for uim and iBus are broken right now.

The condition is wrong. That should be |if (aFocus)|, so, we don't need the interface change for this bug.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v5.0 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #433053 -
        Attachment is obsolete: true

        Attachment #433244 -
        Flags: review?(karlt)

        Attachment #433053 -
        Flags: review?(karlt)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v5.0.1
        — 
          — Splinter Review
      

    


  
oops, sorry for the spam. the previous patch doesn't have a comment.

        Attachment #433244 -
        Attachment is obsolete: true

        Attachment #433245 -
        Flags: review?(karlt)

        Attachment #433244 -
        Flags: review?(karlt)

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

    
    

    
  
Thank you, Karl!

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/c73392866d96
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a4

    
    

    
  
I compile trunk builds with --disable-logging and I get this error. I think the #include "nsGtkIMModule.h" line needs to be moved outside of #ifdef MOZ_LOGGING.

/home/bsjacks/mozilla/widget/src/gtk2/nsWindow.h:514: error: ‘nsGtkIMModule’ was not declared in this scope

    
    

    
  
I filed bug 553640, thank you.

    
    

    
  


  
I forgot a thing. We should return NS_ERROR_NOT_IMPLEMENTED from nsWindow::OnIMEFocusChange(). Current code returns NS_OK, that causes bug 496360.

        Attachment #433691 -
        Flags: review?(karlt)

        Attachment #433691 -
        Flags: review?(karlt) → review+
Keywords: checkin-needed
Whiteboard: [needs landing "follow up patch for bug 496360"]

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/d03261d7898a
Keywords: checkin-needed
Whiteboard: [needs landing "follow up patch for bug 496360"]
Depends on: 603728
No longer depends on: 603728

    
  
Blocks: 458703

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: