[Webkit-unassigned] [Bug 40518] [GTK] Last Hangul letter is typed again when a composition is finished with mouse press

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=40518





--- Comment #9 from Martin Robinson <mrobinson at webkit.org>  2010-08-05 16:58:59 PST ---
(From update of attachment 63137)
Nice patch! I think this is very close, but I have a couple suggestions.

> +    // If this signal fires during a mousepress event when we are in the middle
> +    // of a composition, skip this 'commit' because the composition is already confirmed. 
> +    if (client->preventContextCommitWhenComposition()) {
> +        client->clearPreventContextCommit();
> +        return;
> +    }

As discussed in IRC, maybe it would be safer to avoid resetting
the value here and doing it in handleInputMethodKeydown. That
way if, for some reason, the IM context doesn't do anything
during the mouse event things will still work out okay.

> +    // When a mouse press fires, the commit signal happens during a composition.
> +    // In this case, if the focusd node is changed, the commit signal happens in a diffrent node.
> +    // We need to confirm the current compositon before handling the commit signal. 

Spelling nit: focusd -> focused

The comment should probably be more explicit about the fate
of the next commit signal too: "We need to confirm the current
compositon before handling the commit signal." should be "We
need to confirm the current compositon and ignore the next
commit signal."

> +    GOwnPtr<gchar> newPreedit(0);
> +    gtk_im_context_get_preedit_string(priv->imContext, &newPreedit.outPtr(), 0, 0);
> +    
> +    if (g_utf8_strlen(newPreedit.get(), -1)) {
> +        m_preventContextCommit = true;
> +        targetFrame->editor()->confirmComposition();
> +        gtk_im_context_reset(priv->imContext);
> +    } 
> +}

Doesn't it make more sense to use targetFrame->editor()->hasComposition()
here?I may be wrong about that, but if not, please use that instead.
WebKit style isto use early returns as well. So perhaps something like:

    if (!targetFrame->editor()->hasComposition())
        return;

    targetFrame->editor()->confirmComposition();
    m_preventNextCompositionCommit = true;
    gtk_im_context_reset(priv->imContext);


>          bool treatContextCommitAsKeyEvent() { return m_treatContextCommitAsKeyEvent; }
> +        bool preventContextCommitWhenComposition() { return m_preventContextCommit; }
> +        void clearPreventContextCommit() { m_preventContextCommit = false; }
> +        bool m_preventContextCommit;

The naming here is a little bit off. Perhaps m_preventNextCompositionCommit,
preventNextCompositionCommit(), and acceptNextCompositionCommit()?

>      gboolean result = frame->eventHandler()->handleMousePressEvent(platformEvent);
> +    // Handle the IM context when a mouse press fires
> +    WebKit::EditorClient* client = static_cast<WebKit::EditorClient*>(core(webView)->editorClient());
> +    client->handleInputMethodMousePress();

There's no need to store this value, so just do:
    static_cast<WebKit::EditorClient*>(core(webView)->editorClient())->handleInputMethodMousePress();

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.