Last Comment Bug 576065 - Fixing Android IME implementation
: Fixing Android IME implementation
Status: RESOLVED FIXED
: inputmethod
Product: Core
Classification: Components
Component: Widget: Android (show other bugs)
: unspecified
: ARM Android
: -- normal (vote)
: ---
Assigned To: Jim Chen [:jchen] [:darchons]
:
Mentors:
Depends on:
Blocks: 570603 573736 575007 576728 580219 580378 580388
  Show dependency treegraph
 
Reported: 2010-06-30 12:54 PDT by Jim Chen [:jchen] [:darchons]
Modified: 2010-08-04 12:49 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
2.0a1+


Attachments
Android IME fix v1 (26.17 KB, patch)
2010-06-30 12:54 PDT, Jim Chen [:jchen] [:darchons]
no flags Details | Diff | Review
Android IME fix v2 (46.35 KB, patch)
2010-07-12 18:06 PDT, Jim Chen [:jchen] [:darchons]
no flags Details | Diff | Review
Android IME fix v3 (63.24 KB, patch)
2010-07-20 13:44 PDT, Jim Chen [:jchen] [:darchons]
no flags Details | Diff | Review
(1/4) Widget events v1 (16.44 KB, patch)
2010-07-29 16:50 PDT, Jim Chen [:jchen] [:darchons]
mwu.code: review+
Details | Diff | Review
(2/4) Widget notifications v1 (16.38 KB, patch)
2010-07-29 16:50 PDT, Jim Chen [:jchen] [:darchons]
no flags Details | Diff | Review
(3/4) InputConnection v1 (25.80 KB, patch)
2010-07-29 16:51 PDT, Jim Chen [:jchen] [:darchons]
no flags Details | Diff | Review
(4/4) State updates/status notifications (Java side) v1 (11.85 KB, patch)
2010-07-29 16:53 PDT, Jim Chen [:jchen] [:darchons]
no flags Details | Diff | Review
(1/4) Widget events v1.1 (16.43 KB, patch)
2010-08-02 13:06 PDT, Jim Chen [:jchen] [:darchons]
no flags Details | Diff | Review
(1/4) Widget events v1.2 (16.66 KB, patch)
2010-08-02 14:54 PDT, Jim Chen [:jchen] [:darchons]
mwu.code: review+
Details | Diff | Review
(2/4) Widget notifications v2 (13.49 KB, patch)
2010-08-02 14:57 PDT, Jim Chen [:jchen] [:darchons]
mwu.code: review+
Details | Diff | Review
(3/4) InputConnection v2 (24.42 KB, patch)
2010-08-02 15:19 PDT, Jim Chen [:jchen] [:darchons]
mwu.code: review+
Details | Diff | Review
(4/4) State updates/status notifications (Java side) v2 (10.13 KB, patch)
2010-08-02 15:27 PDT, Jim Chen [:jchen] [:darchons]
no flags Details | Diff | Review
Combined patch (60.45 KB, patch)
2010-08-03 22:37 PDT, Jim Chen [:jchen] [:darchons]
mwu.code: review+
Details | Diff | Review

Description Jim Chen [:jchen] [:darchons] 2010-06-30 12:54:29 PDT
Created attachment 455226 [details] [diff] [review]
Android IME fix v1

There are some problems with the original IME implementation on Android. So I rewrote some of the code.

I'm moving most logic to the Java side and keeping basic operations on the C++ side.

Still working on:
* Composition text styling - query IME for how we should display composition text (underline, highlight, etc)
* The last-typed characters have underlines (will be fixed by the styling code)
* Text extraction/updates - tell IME when text content changes
* Context sensitivity - tell IME what type of input (phone/address/URL) we have
* IME display control - show/hide softkb in more user-friendly ways
** Tap on textbox to redisplay softkb
** Hide softkb on page load
Comment 1 Jim Chen [:jchen] [:darchons] 2010-07-12 18:06:33 PDT
Created attachment 456982 [details] [diff] [review]
Android IME fix v2

Added text styling code. (Still has some issues with Simeji IME)

Doesn't work with e10s, but I think we should land this before trying to make it e10s-compatible?
Comment 2 Michael Wu [:mwu] 2010-07-12 18:20:40 PDT
(In reply to comment #1)
> Doesn't work with e10s, but I think we should land this before trying to make
> it e10s-compatible?
Yeah, e10s compatibility is a separate issue that also affects other widget backends. Lets tackle that in a different bug.
Comment 3 Jim Chen [:jchen] [:darchons] 2010-07-20 13:44:30 PDT
Created attachment 458769 [details] [diff] [review]
Android IME fix v3

Fixed styling code and added text extraction/updates

Known issue with awesomebar, to be tracked in separate bug.
Comment 4 Michael Wu [:mwu] 2010-07-20 15:03:59 PDT
Comment on attachment 458769 [details] [diff] [review]
Android IME fix v3

>@@ -170,35 +178,94 @@ class GeckoAppShell
>+    public static void notifyIME(int type, int arg1, int arg2, int arg3) {
>+    
>+        if (GeckoApp.surfaceView == null ||
>+            GeckoApp.surfaceView.inputConnection == null) return;
Return should be on the next line.

>+        switch (type) {
Hrm.. this API is unpleasant but we can fix it at a later point. It's best to split this up into a few functions. The gecko side doesn't do that but it also needs to be fixed.

>+        case NOTIFY_IME_RESETINPUTSTATE:
>+            {
>+                GeckoApp.surfaceView.inputConnection.finishComposingText();
>+                imm.restartInput(GeckoApp.surfaceView);
>+            }
>+            break;
>+        case NOTIFY_IME_SETOPENSTATE:
>+            {
>+                /* When IME is 'closed', IME prcoessing is disabled and keys
>+                    are directly converted to their corresponding characters.
>+                    In this case the IME UI is still shown. */
>+                    
>+                // This mode doesn't seem to exist in the Android IME API
No need to have this code then. A comment on the widget side is sufficient.

>+        case NOTIFY_IME_FOCUSCHANGE:
>+            {
>+                GeckoApp.surfaceView.mIMEFocus = arg1 != 0;
>+                imm.restartInput(GeckoApp.surfaceView);
>+            }
>+            break;
Hmm, so mIMEFocus is used to control what we return onCheckIsTextEditor returns. What effect does this have? There used to be code which controlled the return value of onCheckIsTextEditor but it was removed because it did nothing useful. As far as I know, it controls whether the view pops the keyboard by default on focus, but we do all that manually anyway so false is fine.

>diff --git a/embedding/android/GeckoInputConnection.java b/embedding/android/GeckoInputConnection.java
>--- a/embedding/android/GeckoInputConnection.java
>+++ b/embedding/android/GeckoInputConnection.java
>@@ -40,135 +40,462 @@ package org.mozilla.gecko;
> import java.io.*;
> import java.util.*;
> import java.util.concurrent.*;
> import java.util.concurrent.atomic.*;
> 
> import android.os.*;
> import android.app.*;
> import android.text.*;
>+import android.text.style.*;
> import android.view.*;
> import android.view.inputmethod.*;
> import android.content.*;
> 
> import android.util.*;
> 
> public class GeckoInputConnection
>     extends BaseInputConnection
> {
>     public GeckoInputConnection (View targetView) {
>         super(targetView, true);
>         mQueryResult = new SynchronousQueue<String>();
>-        mExtractedText.partialStartOffset = -1;
>-        mExtractedText.partialEndOffset = -1;
>-    }
>-
>-    @Override
>-    public Editable getEditable() {
>-        Log.i("GeckoAppJava", "getEditable");
>-        return null;
>     }
> 
>     @Override
>     public boolean beginBatchEdit() {
>-        GeckoAppShell.sendEventToGecko(new GeckoEvent(true, null));
>+        Log.v("GeckoAppJava", "IME: beginBatchEdit");
>+
>+        /* The only function of a batch edit seems to be
>+            preventing updates from being sent to the IME */
>+            
>         return true;
>     }
>+    
>     @Override
>     public boolean commitCompletion(CompletionInfo text) {
>-        Log.i("GeckoAppJava", "Stub: commitCompletion");
>+        Log.v("GeckoAppJava", "IME: commitCompletion");
>+        
>+        return commitText(text.getText(), 1);
>+    }
>+    
>+    @Override
>+    public boolean commitText(CharSequence text, int newCursorPosition) {
>+    
To keep things consistent, avoid empty lines after the method definition.

>+        Log.v("GeckoAppJava", "IME: commitText: t=" +
>+            (text != null ? text.length() : 0) + ", nCP=" + newCursorPosition);
>+        
>+        setComposingText(text, newCursorPosition);
>+        finishComposingText();
>+        
>         return true;
>     }
>+    
>     @Override
>-    public boolean commitText(CharSequence text, int newCursorPosition) {
>-        GeckoAppShell.sendEventToGecko(new GeckoEvent(true, text.toString()));
>-        endBatchEdit();
>+    public boolean deleteSurroundingText(int leftLength, int rightLength) {
>+    
>+        Log.v("GeckoAppJava", "IME: deleteSurroundingText: lL=" + leftLength + ", rL=" + rightLength);
>+
>+        /* deleteSurroundingText is supposed to ignore the composing text,
>+            so we cancel any pending composition, delete the text, and then
>+            restart the composition */
>+            
I suspect you actually want to do all this on the Gecko side. The synchronous IME events that require the use of mQueryResult should be avoided since they freeze up the Java thread while waiting for results to be returned.

>+        if (mComposing) {
>+            // Cancel current composition
>+            GeckoAppShell.sendEventToGecko(new GeckoEvent(GeckoEvent.IME_EVENT, 
>+                GeckoEvent.IME_SET_TEXT, null));
>+            GeckoAppShell.sendEventToGecko(new GeckoEvent(GeckoEvent.IME_EVENT,
>+                GeckoEvent.IME_COMPOSITION_END));
>+        }
>+        
>+        // Select text to be deleted
>+        int delStart, delLen;
>+        GeckoAppShell.sendEventToGecko(new GeckoEvent(GeckoEvent.IME_EVENT,
>+            GeckoEvent.IME_GET_SELECTION));
>+        try {
>+            mQueryResult.take();
>+        } catch (InterruptedException e) {
>+            Log.e("GeckoAppJava", "IME: deleteSurroundingText interrupted");
>+            return false;
>+        }
>+        delStart = mSelectionStart > leftLength ?
>+                    mSelectionStart - leftLength : 0;
>+        delLen = mSelectionStart + rightLength - delStart;
>+        GeckoAppShell.sendEventToGecko(new GeckoEvent(GeckoEvent.IME_EVENT,
>+            GeckoEvent.IME_SET_SELECTION, delStart, delLen));
>+        
>+        // Restore composition / delete text
>+        if (mComposing) {
>+            GeckoAppShell.sendEventToGecko(new GeckoEvent(GeckoEvent.IME_EVENT, 
>+                GeckoEvent.IME_COMPOSITION_BEGIN));
>+            if (mComposingText.length() > 0) {
>+                /* IME_SET_TEXT doesn't work well with empty strings */
>+                GeckoAppShell.sendEventToGecko(new GeckoEvent(GeckoEvent.IME_EVENT,
>+                    GeckoEvent.IME_SET_TEXT, mComposingText.toString()));
>+            }
>+        } else {
>+            GeckoAppShell.sendEventToGecko(new GeckoEvent(GeckoEvent.IME_EVENT,
>+                GeckoEvent.IME_DELETE_TEXT));
>+        }
>         return true;
>     }
>+    
>     @Override
>-    public boolean deleteSurroundingText(int leftLength, int rightLength) {
>-        GeckoAppShell.sendEventToGecko(new GeckoEvent(leftLength, rightLength));
>-        updateExtractedText();
>+    public boolean endBatchEdit() {
>+        Log.v("GeckoAppJava", "IME: endBatchEdit");
>+        
>+        // See comment for beginBatchEdit
>+        
I know I'm getting really nit picky now, but comments about the method should go before the method declaration. :)

>         return true;
>     }
>+    
>     @Override
>-    public boolean endBatchEdit() {
>-        updateExtractedText();
>-        GeckoAppShell.sendEventToGecko(new GeckoEvent(false, null));
>+    public boolean finishComposingText() {
>+    
>+        Log.v("GeckoAppJava", "IME: finishComposingText");
>+        
>+        if (mComposing) {
>+            // Set style to none
>+            GeckoAppShell.sendEventToGecko(new GeckoEvent(
>+                GeckoEvent.IME_EVENT, GeckoEvent.IME_ADD_RANGE,
>+                0, mComposingText.length(),
>+                GeckoEvent.IME_RANGE_RAWINPUT, 0, 0, 0));
>+            GeckoAppShell.sendEventToGecko(new GeckoEvent(GeckoEvent.IME_EVENT,
>+                GeckoEvent.IME_SET_TEXT, mComposingText));
>+            GeckoAppShell.sendEventToGecko(new GeckoEvent(GeckoEvent.IME_EVENT,
>+                GeckoEvent.IME_COMPOSITION_END));
>+            mComposing = false;
>+            mComposingText = null;
>+            
>+            // Make sure cursor stays at the same position
>+            GeckoAppShell.sendEventToGecko(new GeckoEvent(GeckoEvent.IME_EVENT,
>+                GeckoEvent.IME_SET_SELECTION,
>+                mCompositionStart + mCompositionSelStart, 0));
Are you sure this cursor repositioning is necessary?

>+    @Override
>+    public ExtractedText getExtractedText(ExtractedTextRequest req, int flags) {
>+    
>+        if (req == null) return null;
>+        
Can req ever be null?

>+        Log.v("GeckoAppJava", "IME: getExtractedText: f=" + flags +
>+            ", rf=" + req.flags + ", mc=" + req.hintMaxChars +
>+            ", ml=" + req.hintMaxLines + ", tk=" + req.token);
>+        
>+        ExtractedText extract = new ExtractedText();
>+        extract.flags = 0;
>+        extract.partialStartOffset = -1;
>+        extract.partialEndOffset = -1;
>+        
? Why this? The old ExtractedText handling avoided allocations.

>+    @Override
>+    public boolean setComposingText(CharSequence text, int newCursorPosition) {
>+    
>+        Log.v("GeckoAppJava", "IME: setComposingText: t=" +
>+            (text != null ? text.length() : 0) + ", nCP=" + newCursorPosition);
>+    
>+        // Set new composing text
>+        mComposingText = text != null ? text.toString() : "";
>+        
>+        if (!mComposing) {
>+            // Get current selection
>+            GeckoAppShell.sendEventToGecko(new GeckoEvent(GeckoEvent.IME_EVENT,
>+                GeckoEvent.IME_GET_SELECTION));
>+            try {
>+                mQueryResult.take();
>+            } catch (InterruptedException e) {
>+                Log.e("GeckoAppJava", "IME: setComposingText interrupted");
>+                return false;
>+            }
>+            // Make sure we are in a composition
>+            GeckoAppShell.sendEventToGecko(new GeckoEvent(
>+                GeckoEvent.IME_EVENT, GeckoEvent.IME_COMPOSITION_BEGIN));
>+            mComposing = true;
>+            mCompositionStart = mSelectionLength >= 0 ?
>+                mSelectionStart : mSelectionStart + mSelectionLength;
>+        }
>+        
I'm not sure why you're accounting for the selection length. The selection will be destroyed by the composition.

>+        // Set new selection
>+        // New selection should be within the composition
>+        mCompositionSelStart = newCursorPosition > 0 ? mComposingText.length() : 0;
>+        mCompositionSelLen = 0;
>+        
>+        // Handling composition text styles
>+        if (text != null && text instanceof Spanned) {
>+            Spanned span = (Spanned) text;
>+            int spanStart = 0, spanEnd = 0;
>+            boolean pastSelStart = false, pastSelEnd = false;
>+            
>+            for (Object obj : span.getSpans(0, text.length(), Object.class)) {
>+                Log.v("GeckoAppJava", "IME:     span: " + obj.getClass().toString());
>+            }
>+            
>+            do {
>+                int rangeType = GeckoEvent.IME_RANGE_CONVERTEDTEXT;
>+                int rangeStyles = 0, rangeForeColor = 0, rangeBackColor = 0;
>+                
>+                // Find next offset where there is a style transition
>+                spanEnd = span.nextSpanTransition(spanStart + 1, text.length(),
>+                    CharacterStyle.class);
>+                
>+                // We need to count the selection as a transition
>+                if (mCompositionSelLen >= 0) {
>+                    if (!pastSelStart && spanEnd >= mCompositionSelStart) {
>+                        spanEnd = mCompositionSelStart;
>+                        pastSelStart = true;
>+                    } else if (!pastSelEnd && spanEnd >=
>+                            mCompositionSelStart + mCompositionSelLen) {
>+                        spanEnd = mCompositionSelStart + mCompositionSelLen;
>+                        pastSelEnd = true;
>+                        rangeType = GeckoEvent.IME_RANGE_SELECTEDRAWTEXT;
>+                    }
>+                } else {
>+                    if (!pastSelEnd && spanEnd >=
>+                            mCompositionSelStart + mCompositionSelLen) {
>+                        spanEnd = mCompositionSelStart + mCompositionSelLen;
>+                        pastSelEnd = true;
>+                    } else if (!pastSelStart &&
>+                            spanEnd >= mCompositionSelStart) {
>+                        spanEnd = mCompositionSelStart;
>+                        pastSelStart = true;
>+                        rangeType = GeckoEvent.IME_RANGE_SELECTEDRAWTEXT;
>+                    }
>+                }
>+                // Empty range, continue
>+                if (spanEnd <= spanStart) continue;
>+                
The continue should also be on the next line.

This is a lot of code. I'll be back.
Comment 5 Jim Chen [:jchen] [:darchons] 2010-07-20 16:07:55 PDT
(In reply to comment #4)
> Comment on attachment 458769 [details] [diff] [review]
> Android IME fix v3
> 
> Hrm.. this API is unpleasant but we can fix it at a later point. It's best to
> split this up into a few functions. The gecko side doesn't do that but it also
> needs to be fixed.
> 
Alright. I guess the trade off would be on the Gecko side we have to cache the IDs of each function

> Hmm, so mIMEFocus is used to control what we return onCheckIsTextEditor
> returns. What effect does this have? There used to be code which controlled the
> return value of onCheckIsTextEditor but it was removed because it did nothing
> useful. As far as I know, it controls whether the view pops the keyboard by
> default on focus, but we do all that manually anyway so false is fine.
> 
According to the docs it tells the caller whether onCreateInputConnection will likely return something. In onCreateInputConnection, we return either GeckoInputConnection or null based on mIMEFocus

> >+        /* deleteSurroundingText is supposed to ignore the composing text,
> >+            so we cancel any pending composition, delete the text, and then
> >+            restart the composition */
> >+            
> I suspect you actually want to do all this on the Gecko side. The synchronous
> IME events that require the use of mQueryResult should be avoided since they
> freeze up the Java thread while waiting for results to be returned.
> 
Yeah that's quite messy; I wrote it a while ago. I see several places I can optimize. In real testing though, I've never seen deleteSurroundingText getting called.

> >+            // Make sure cursor stays at the same position
> >+            GeckoAppShell.sendEventToGecko(new GeckoEvent(GeckoEvent.IME_EVENT,
> >+                GeckoEvent.IME_SET_SELECTION,
> >+                mCompositionStart + mCompositionSelStart, 0));
> Are you sure this cursor `repositioning is necessary?
> 
I think the caller expects it to be at the same place, but in practice it might not be necessary.

> >+    @Override
> >+    public ExtractedText getExtractedText(ExtractedTextRequest req, int flags) {
> >+    
> >+        if (req == null) return null;
> >+        
> Can req ever be null?
> 
Hm not sure. Probably not.

> >+        ExtractedText extract = new ExtractedText();
> >+        extract.flags = 0;
> >+        extract.partialStartOffset = -1;
> >+        extract.partialEndOffset = -1;
> >+        
> ? Why this? The old ExtractedText handling avoided allocations.
> 
That's what the native Android implementation does. Although for updates, the native implementation reuses the same object.

> >+            // Make sure we are in a composition
> >+            GeckoAppShell.sendEventToGecko(new GeckoEvent(
> >+                GeckoEvent.IME_EVENT, GeckoEvent.IME_COMPOSITION_BEGIN));
> >+            mComposing = true;
> >+            mCompositionStart = mSelectionLength >= 0 ?
> >+                mSelectionStart : mSelectionStart + mSelectionLength;
> >+        }
> >+        
> I'm not sure why you're accounting for the selection length. The selection will
> be destroyed by the composition.
> 
IME_GET_SELECTION can return negative lengths for reverse selections (end < start), although we can change this behavior.
Comment 6 Jim Chen [:jchen] [:darchons] 2010-07-28 13:47:32 PDT
mwu: I'm splitting the patch; freshly baked patches coming right up.
Comment 7 Jim Chen [:jchen] [:darchons] 2010-07-29 16:50:02 PDT
Created attachment 461399 [details] [diff] [review]
(1/4) Widget events v1
Comment 8 Jim Chen [:jchen] [:darchons] 2010-07-29 16:50:44 PDT
Created attachment 461400 [details] [diff] [review]
(2/4) Widget notifications v1
Comment 9 Jim Chen [:jchen] [:darchons] 2010-07-29 16:51:29 PDT
Created attachment 461401 [details] [diff] [review]
(3/4) InputConnection v1
Comment 10 Jim Chen [:jchen] [:darchons] 2010-07-29 16:53:15 PDT
Created attachment 461402 [details] [diff] [review]
(4/4) State updates/status notifications (Java side) v1
Comment 11 Michael Wu [:mwu] 2010-07-30 16:42:11 PDT
(In reply to comment #7)
> Created attachment 461399 [details] [diff] [review]
> (1/4) Widget events v1
I like this. There's just a couple of race issues I'm a little concerned with but we can address them in a followup.

The problem is if an operation requires more than one event to complete , the gecko side can send in some sort of event and mess everything up. One example would be deleting - the java side sends a select event, the gecko side sends its own select event, and then the java side sends a delete event.

One other thing - the new ALOGs worry me a bit. We should make a different sort of ALOG that can be turned on or off so the console doesn't need to spew so much for people not debugging IME. Up to you if you want to do it here or in a follow up. We already have too much event spew so it's not a big deal.
Comment 12 Michael Wu [:mwu] 2010-07-30 17:17:14 PDT
Comment on attachment 461400 [details] [diff] [review]
(2/4) Widget notifications v1

> void
>-AndroidBridge::ReturnIMEQueryResult(const PRUnichar *result, PRUint32 len, int selectionStart, int selectionEnd)
>+AndroidBridge::ReturnIMEQueryResult(PRBool aUseQueue, const PRUnichar *aResult, PRUint32 aLen, int aSelStart, int aSelLen)
> {
>-    jvalue args[3];
>-    args[0].l = mJNIEnv->NewString(result, len);
>-    args[1].i = selectionStart;
>-    args[2].i = selectionEnd;
>+    jvalue args[4];
>+    AutoLocalJNIFrame jniFrame(1);
>+    args[0].z = aUseQueue == PR_TRUE ? JNI_TRUE : JNI_FALSE;
aUseQueue ? JNI_TRUE : JNI_FALSE

>diff --git a/widget/src/android/AndroidBridge.h b/widget/src/android/AndroidBridge.h
>--- a/widget/src/android/AndroidBridge.h
>+++ b/widget/src/android/AndroidBridge.h
>@@ -54,16 +54,26 @@
> 
> class nsWindow;
> 
> namespace mozilla {
> 
> class AndroidBridge
> {
> public:
>+    enum {
>+        NOTIFY_IME_RESETINPUTSTATE = 0,
>+        NOTIFY_IME_SETOPENSTATE = 1,
>+        NOTIFY_IME_SETENABLED = 2,
>+        NOTIFY_IME_CANCELCOMPOSITION = 3,
>+        NOTIFY_IME_FOCUSCHANGE = 4,
>+        NOTIFY_IME_TEXTCHANGE = 5,
>+        NOTIFY_IME_SELECTIONCHANGE = 6
>+    };
>+
Hmmm.. I really don't like making multiplexing functions.. Well, easy enough to address in a followup.

>@@ -84,23 +94,23 @@ public:
>     // SetMainThread should be called which will create the JNIEnv for
>     // us to use.  toolkit/xre/nsAndroidStartup.cpp calls
>     // SetMainThread.
>     PRBool SetMainThread(void *thr);
> 
>     JNIEnv* AttachThread(PRBool asDaemon = PR_TRUE);
> 
>     /* These are all implemented in Java */
>-    void ShowIME(int aState);
>+    void NotifyIME(int aType, int aArg1, int aArg2, int aArg3);
> 
And this is exactly why I don't like multiplexing functions. The meaning of the args becomes difficult to discover.

Seems like a lot of the callers require casts to int, too.

>+    case AndroidGeckoEvent::IME_TEXT_CHANGE:
>+        {
>+            ALOG("IME: IME_TEXT_CHANGE");
>+            
>+            if (ae->NewCount() > 0) {
>+                nsQueryContentEvent event(PR_TRUE,
>+                    NS_QUERY_TEXT_CONTENT, this);
>+                InitEvent(event, nsnull);
>+                event.InitForQueryTextContent(ae->Offset(), ae->NewCount());
>+                DispatchEvent(&event);
>+                
>+                if (!event.mSucceeded) return;
>+                
>+                AndroidBridge::Bridge()->ReturnIMEQueryResult(PR_FALSE,
>+                    event.mReply.mString.get(), 
>+                    event.mReply.mString.Length(), 0, 0);
>+            } else {
>+                AndroidBridge::Bridge()->ReturnIMEQueryResult(PR_FALSE,
>+                    nsnull, 0, 0, 0);
>+            }
>+            
>+            if (AndroidBridge::Bridge())
>+                AndroidBridge::Bridge()->NotifyIME(
>+                    AndroidBridge::NOTIFY_IME_TEXTCHANGE,
>+                    int(ae->Offset()), int(ae->Count()), int(ae->NewCount()));
>+        }
Oh. Actually, we definitely want a dedicated function for sending text changes. This will let you avoid adding that argument to ReturnIMEQueryResult and avoid adding the NOTIFY_IME_TEXTCHANGE enum.

>+nsWindow::ResetInputState()
>+{
>+    // Cancel composition on Gecko side
>+    nsCompositionEvent event(PR_TRUE, NS_COMPOSITION_END, this);
Hm this feels like the sort of thing that the gecko side should be doing for us..
Oh well.
Comment 13 Michael Wu [:mwu] 2010-07-30 18:22:32 PDT
Comment on attachment 461401 [details] [diff] [review]
(3/4) InputConnection v1

A lot of logging in this code. The java code logs all the time regardless of whether we're in an opt or debug build, so they should be removed unless they're particularly useful for debugging. (comment out in that case)

There is a lot of redundant IME_EVENT setting. If you can create a unique GeckoEvent constructor that won't be used by anything but IME code, have it set the IME_EVENT type by itself. Yes, this is evil too but it involves less code. One day GeckoEvent will be torn up and the bridge will let us do things right.

>+    private void InitIMERange(int type, int action, int offset, int count,
>+            int rangeType, int rangeStyles,
>+            int rangeForeColor, int rangeBackColor) {
I recommend aligning with the starting parenthesis here and elsewhere in this file.

>+    public GeckoEvent(int type, int action, int offset, int count,
>+            int rangeType, int rangeStyles, int rangeForeColor,
>+            int rangeBackColor) {
>+        InitIMERange(type, action, offset, count, rangeType, rangeStyles,
>+                    rangeForeColor, rangeBackColor);
:/

>+    @Override
>+    public boolean deleteSurroundingText(int leftLength, int rightLength) {
>+        Log.v("GeckoAppJava", "IME: deleteSurroundingText: lL=" + leftLength + ", rL=" + rightLength);
>+
>+        /* deleteSurroundingText is supposed to ignore the composing text,
>+            so we cancel any pending composition, delete the text, and then
>+            restart the composition */
>+            
Hm, I guess if this is the only way..

>+    @Override
>+    public int getCursorCapsMode(int reqModes) {
>+        Log.v("GeckoAppJava", "IME: getCursorCapsMode");
>+
>+        return 0;
>     }
We'll probably have to implement this properly one day.

>+    public ExtractedText getExtractedText(ExtractedTextRequest req, int flags) {
>+        if (req == null) return null;
return on the next line.

>+
>+        Log.v("GeckoAppJava", "IME: getExtractedText: f=" + flags +
>+            ", rf=" + req.flags + ", mc=" + req.hintMaxChars +
>+            ", ml=" + req.hintMaxLines + ", tk=" + req.token);
>+
>+        ExtractedText extract = new ExtractedText();
>+        extract.flags = 0;
>+        extract.partialStartOffset = -1;
>+        extract.partialEndOffset = -1;
>+
>+        GeckoAppShell.sendEventToGecko(new GeckoEvent(GeckoEvent.IME_EVENT,
>+            GeckoEvent.IME_GET_SELECTION));
>+        try {
>+            mQueryResult.take();
>+        } catch (InterruptedException e) {
>+            Log.e("GeckoAppJava", "IME: getExtractedText interrupted");
>+            return null;
>+        }
>+        extract.selectionStart = mSelectionStart;
>+        extract.selectionEnd = mSelectionStart + mSelectionLength;
>+
>+        GeckoAppShell.sendEventToGecko(new GeckoEvent(GeckoEvent.IME_EVENT,
>+            GeckoEvent.IME_GET_TEXT, 0, Integer.MAX_VALUE));
>+        try {
>+            extract.startOffset = 0;
>+            extract.text = mQueryResult.take();
>+
>+            return extract;
>+        } catch (InterruptedException e) {
>+            Log.e("GeckoAppJava", "IME: getExtractedText interrupted");
>+            return null;
>+        }
>     }
I wonder if we can do this entirely asynchronously. This function would just return an empty string and gecko would come back at some point later and put in the right text.

>+        GeckoAppShell.sendEventToGecko(new GeckoEvent(GeckoEvent.IME_EVENT,
>+            GeckoEvent.IME_GET_TEXT, textStart, textLength));
How about something like:
        GeckoAppShell.sendEventToGecko(
            new GeckoEvent(GeckoEvent.IME_EVENT, GeckoEvent.IME_GET_TEXT,
                           textStart, textLength));
for this and other lines where we're posting events to gecko?

>+        // Handle composition text styles
>+        if (text != null && text instanceof Spanned) {
>+            Spanned span = (Spanned) text;
>+            int spanStart = 0, spanEnd = 0;
>+            boolean pastSelStart = false, pastSelEnd = false;
>+
>+            for (Object obj : span.getSpans(0, text.length(), Object.class)) {
>+                Log.v("GeckoAppJava", "IME:     span: " + obj.getClass().toString());
>+            }
Braces should be avoided for single statements.

>+            do {
>+                int rangeType = GeckoEvent.IME_RANGE_CONVERTEDTEXT;
>+                int rangeStyles = 0, rangeForeColor = 0, rangeBackColor = 0;
>+
>+                // Find next offset where there is a style transition
>+                spanEnd = span.nextSpanTransition(spanStart + 1, text.length(),
>+                    CharacterStyle.class);
>+
>+                // We need to count the selection as a transition
>+                if (mCompositionSelLen >= 0) {
Do negative selection lengths happen?

>+                    if (!pastSelStart && spanEnd >= mCompositionSelStart) {
>+                        spanEnd = mCompositionSelStart;
>+                        pastSelStart = true;
>+                    } else if (!pastSelEnd && spanEnd >=
>+                            mCompositionSelStart + mCompositionSelLen) {
>+                        spanEnd = mCompositionSelStart + mCompositionSelLen;
>+                        pastSelEnd = true;
>+                        rangeType = GeckoEvent.IME_RANGE_SELECTEDRAWTEXT;
>+                    }
>+                } else {
>+                    if (!pastSelEnd && spanEnd >=
>+                            mCompositionSelStart + mCompositionSelLen) {
>+                        spanEnd = mCompositionSelStart + mCompositionSelLen;
>+                        pastSelEnd = true;
>+                    } else if (!pastSelStart &&
>+                            spanEnd >= mCompositionSelStart) {
>+                        spanEnd = mCompositionSelStart;
>+                        pastSelStart = true;
>+                        rangeType = GeckoEvent.IME_RANGE_SELECTEDRAWTEXT;
>+                    }
>+                }
>+                // Empty range, continue
>+                if (spanEnd <= spanStart) {
>+                    continue;
>+                }
>+
>+                // Get and iterate through list of span objects within range
>+                CharacterStyle styles[] = span.getSpans(
>+                    spanStart, spanEnd, CharacterStyle.class);
>+
>+                for (CharacterStyle style : styles) {
>+                    if (style instanceof UnderlineSpan) {
>+                        // Text should be underlined
>+                        rangeStyles |= GeckoEvent.IME_RANGE_UNDERLINE;
>+
>+                    } else if (style instanceof ForegroundColorSpan) {
>+                        // Text should be of a different foreground color
>+                        rangeStyles |= GeckoEvent.IME_RANGE_FORECOLOR;
>+                        rangeForeColor =
>+                            ((ForegroundColorSpan)style).getForegroundColor();
>+
>+                    } else if (style instanceof BackgroundColorSpan) {
>+                        // Text should be of a different background color
>+                        rangeStyles |= GeckoEvent.IME_RANGE_BACKCOLOR;
>+                        rangeBackColor =
>+                            ((BackgroundColorSpan)style).getBackgroundColor();
>+                    }
>+                }
>+
>+                Log.v("GeckoAppJava", "IME:     addRange: s=" + spanStart +
>+                    ", e=" + spanEnd + ", t=" + rangeType +
>+                    ", st=" + rangeStyles + ", f=" + rangeForeColor + 
>+                    ", b=" + rangeBackColor);
>+
>+                // Add range to array, the actual styles are
>+                //  applied when IME_SET_TEXT is sent
>+                GeckoAppShell.sendEventToGecko(new GeckoEvent(
>+                    GeckoEvent.IME_EVENT, GeckoEvent.IME_ADD_RANGE,
>+                    spanStart, spanEnd - spanStart, rangeType, rangeStyles,
>+                    rangeForeColor, rangeBackColor));
>+
>+                spanStart = spanEnd;
>+            } while (spanStart < text.length());
>+        } else {
>+            GeckoAppShell.sendEventToGecko(new GeckoEvent(
>+                GeckoEvent.IME_EVENT, GeckoEvent.IME_ADD_RANGE,
>+                0, text == null ? 0 : text.length(),
>+                GeckoEvent.IME_RANGE_RAWINPUT,
>+                GeckoEvent.IME_RANGE_UNDERLINE, 0, 0));
Hm there's nothing on the platform side that defines the default style for the composition? I'd like to use that if possible.

>+        }
>+
>+        // Change composition (treating selection end as where the caret is)
>+        GeckoAppShell.sendEventToGecko(new GeckoEvent(GeckoEvent.IME_EVENT,
>+            GeckoEvent.IME_SET_TEXT, mCompositionSelStart + mCompositionSelLen,
>+            0, GeckoEvent.IME_RANGE_CARETPOSITION, 0, 0, 0, mComposingText));
>+
I don't see anything check for IME_RANGE_CARETPOSITION. Is the offset specified here possible to use? Why is this GeckoEvent constructor necessary?

>+    @Override
>+    public boolean setSelection(int start, int end) {
>+        Log.v("GeckoAppJava", "IME: setSelection: s=" + start + ", e=" + end);
>+
>+        if (mComposing) {
>+            /* Translate to fake selection positions */
>+            start -= mCompositionStart;
>+            end -= mCompositionStart;
>+
>+            if (start < 0) start = 0;
start = 0; on the next line.

>+            else if (start > mComposingText.length())
>+                start = mComposingText.length();
>+            if (end < 0) end = 0;
end = 0; on the next line.

>+    public boolean onKeyDel() {
>+        // Some IMEs don't update us on deletions
>+        // In that case we are not updated when a composition
>+        // is destroyed, and Bad Things happen
>+
>+        if (!mComposing) return false;
return on the next line.

>+        
>+        if (mComposingText.length() > 0) {
>+            mComposingText = mComposingText.substring(0,
>+                mComposingText.length() - 1);
>+            if (mComposingText.length() > 0) return false;
>         }
> 
>-        InputMethodManager imm = (InputMethodManager)
>-            GeckoApp.surfaceView.getContext().getSystemService(Context.INPUT_METHOD_SERVICE);
>-        imm.updateExtractedText(GeckoApp.surfaceView, mExtractToken, mExtractedText);
>+        commitText(null, 1);
So... hitting delete automatically ends the composition? That doesn't sound correct..

>+        return true;
>     }
> 
>-    int mExtractToken;
>-    final ExtractedText mExtractedText = new ExtractedText();
>+    // Is a composition active?
>+    boolean mComposing;
>+    // Composition text when a composition is active
>+    String mComposingText;
>+    // Start index of the composition within the text body
>+    int mCompositionStart;
>+    /* When a composition is active, Gecko displays a fake selection.
>+        This fake selection can change while the actual DOM selection stays fixed.
>+        We need to keep track of this fake selection to inform Gecko. */
This comment doesn't sound right. The actual DOM selection should've been lost the moment we started composing.
Comment 14 Michael Wu [:mwu] 2010-07-30 18:36:11 PDT
Comment on attachment 461402 [details] [diff] [review]
(4/4) State updates/status notifications (Java side) v1

>@@ -191,35 +199,94 @@ class GeckoAppShell
>             e = new GeckoEvent(GeckoEvent.DRAW, new Rect(x, y, w, h));
>         }
> 
>         e.mNativeWindow = nativeWindow;
> 
>         sendEventToGecko(e);
>     }
> 
>-    public static void showIME(int state) {
>-        GeckoApp.surfaceView.mIMEState = state;
>+    protected static void enableIME(boolean enable) {
Hmmm let's not change mIMEState to mIMEEnabled. The reason we use an int for the state is to distinguish between password and non-password fields. state is set to nsIWidget::IME_STATUS_PASSWORD for password fields. We'll want to pass this info when showing the IME.

>+        GeckoApp.surfaceView.mIMEEnabled = enable;
> 
>+        /* Bug 573800 */
>         if (mSoftKBTimer == null) {
>             mSoftKBTimer = new Timer();
>             mSoftKBTimer.schedule(new TimerTask() {
>                 public void run() {
>                     InputMethodManager imm = (InputMethodManager) 
>                         GeckoApp.surfaceView.getContext().getSystemService(Context.INPUT_METHOD_SERVICE);
>+                    if (imm == null) return;
return on the next line.

> 
>-                    if (GeckoApp.surfaceView.mIMEState != 0)
>+                    if (GeckoApp.surfaceView.mIMEEnabled)
>                         imm.showSoftInput(GeckoApp.surfaceView, 0);
>                     else
>                         imm.hideSoftInputFromWindow(GeckoApp.surfaceView.getWindowToken(), 0);
>                     mSoftKBTimer = null;
>-                    
>+
>                 }
>             }, 200);
>         }
>+        return;
Remove this return.

>+        case NOTIFY_IME_SETOPENSTATE:
>+            {
>+                // This mode doesn't seem to exist in the Android IME API
>+            }
>+            break;
This can be removed then, along with the set/getIMEOpenState stuff which doesn't do anything for us.
Comment 15 Brad Lassey [:blassey] (use needinfo?) 2010-07-31 16:17:54 PDT
(In reply to comment #11)
> One other thing - the new ALOGs worry me a bit. We should make a different sort
> of ALOG that can be turned on or off so the console doesn't need to spew so
> much for people not debugging IME. Up to you if you want to do it here or in a
> follow up. We already have too much event spew so it's not a big deal.

IMO, we should be using prlog. As is it can be directed to a file and on the next nspr update we'll pick up bug 578496 which allows us to direct prlog to the android log system.
Comment 16 Jim Chen [:jchen] [:darchons] 2010-08-02 13:06:45 PDT
Created attachment 462143 [details] [diff] [review]
(1/4) Widget events v1.1

Did some refactoring; no functionality change

(In reply to comment #11)
> (In reply to comment #7)
> > Created attachment 461399 [details] [diff] [review] [details]
> > (1/4) Widget events v1
> I like this. There's just a couple of race issues I'm a little concerned with
> but we can address them in a followup.
> 
> The problem is if an operation requires more than one event to complete , the
> gecko side can send in some sort of event and mess everything up. One example
> would be deleting - the java side sends a select event, the gecko side sends
> its own select event, and then the java side sends a delete event.

Right, the Gecko side shouldn't send select events, etc., without the Java side telling it to.

> One other thing - the new ALOGs worry me a bit. We should make a different sort
> of ALOG that can be turned on or off so the console doesn't need to spew so
> much for people not debugging IME. Up to you if you want to do it here or in a
> follow up. We already have too much event spew so it's not a big deal.

Since everything else is ALOG right now, I think we should clean up everything in a follow up bug.
Comment 17 Jim Chen [:jchen] [:darchons] 2010-08-02 14:54:18 PDT
Created attachment 462195 [details] [diff] [review]
(1/4) Widget events v1.2

Added ALOGIME
Comment 18 Jim Chen [:jchen] [:darchons] 2010-08-02 14:57:33 PDT
Created attachment 462198 [details] [diff] [review]
(2/4) Widget notifications v2

(In reply to comment #12)
> Comment on attachment 461400 [details] [diff] [review]
> (2/4) Widget notifications v1
> 
> >+            if (AndroidBridge::Bridge())
> >+                AndroidBridge::Bridge()->NotifyIME(
> >+                    AndroidBridge::NOTIFY_IME_TEXTCHANGE,
> >+                    int(ae->Offset()), int(ae->Count()), int(ae->NewCount()));
> >+        }
> Oh. Actually, we definitely want a dedicated function for sending text changes.
> This will let you avoid adding that argument to ReturnIMEQueryResult and avoid
> adding the NOTIFY_IME_TEXTCHANGE enum.
> 
Simplified AndroidBridge::NotifyIME and added AndroidBridge::NotifyIMEChange. The remote bridge in bug 581535 should be updated to reflect this.
Comment 19 Jim Chen [:jchen] [:darchons] 2010-08-02 15:19:53 PDT
Created attachment 462213 [details] [diff] [review]
(3/4) InputConnection v2

Refactoring and other changes

(In reply to comment #13)
> Comment on attachment 461401 [details] [diff] [review]
> (3/4) InputConnection v1
> 
> A lot of logging in this code. The java code logs all the time regardless of
> whether we're in an opt or debug build, so they should be removed unless
> they're particularly useful for debugging. (comment out in that case)
> 
Commented out a lot of the logging code. And Log.d doesn't do anything when not in debugging mode.

> There is a lot of redundant IME_EVENT setting. If you can create a unique
> GeckoEvent constructor that won't be used by anything but IME code, have it set
> the IME_EVENT type by itself. Yes, this is evil too but it involves less code.
> One day GeckoEvent will be torn up and the bridge will let us do things right.
> 
OK.

> >+            Log.e("GeckoAppJava", "IME: getExtractedText interrupted");
> >+            return null;
> >+        }
> >     }
> I wonder if we can do this entirely asynchronously. This function would just
> return an empty string and gecko would come back at some point later and put in
> the right text.
> 
Hm, we don't know / can't assume the system behavior though.

> >+                    CharacterStyle.class);
> >+
> >+                // We need to count the selection as a transition
> >+                if (mCompositionSelLen >= 0) {
> Do negative selection lengths happen?
> 
It is possible but don't know if it actually happens.

> >+                GeckoEvent.IME_EVENT, GeckoEvent.IME_ADD_RANGE,
> >+                0, text == null ? 0 : text.length(),
> >+                GeckoEvent.IME_RANGE_RAWINPUT,
> >+                GeckoEvent.IME_RANGE_UNDERLINE, 0, 0));
> Hm there's nothing on the platform side that defines the default style for the
> composition? I'd like to use that if possible.
> 
NAFAIK, but the default style on Android (and everywhere else pretty much) is underline.

> >+        // Change composition (treating selection end as where the caret is)
> >+        GeckoAppShell.sendEventToGecko(new GeckoEvent(GeckoEvent.IME_EVENT,
> >+            GeckoEvent.IME_SET_TEXT, mCompositionSelStart + mCompositionSelLen,
> >+            0, GeckoEvent.IME_RANGE_CARETPOSITION, 0, 0, 0, mComposingText));
> >+
> I don't see anything check for IME_RANGE_CARETPOSITION. Is the offset specified
> here possible to use? Why is this GeckoEvent constructor necessary?
> 
We make Gecko happy when we pass the caret position as a range when sending the NS_TEXT_TEXT event.

Usually the event sequence goes something like this: ADD_RANGE, ADD_RANGE, ADD_RANGE, SET_TEXT. Right now we integrate the last ADD_RANGE into the SET_TEXT event as an optimization.

> >+    public boolean onKeyDel() {
> >+        // Some IMEs don't update us on deletions
> >+        // In that case we are not updated when a composition
> >+        // is destroyed, and Bad Things happen
> >+
> >+        if (!mComposing) return false;
> return on the next line.
> 
> >+        
> >+        if (mComposingText.length() > 0) {
> >+            mComposingText = mComposingText.substring(0,
> >+                mComposingText.length() - 1);
> >+            if (mComposingText.length() > 0) return false;
> >         }
> > 
> >-        InputMethodManager imm = (InputMethodManager)
> >-            GeckoApp.surfaceView.getContext().getSystemService(Context.INPUT_METHOD_SERVICE);
> >-        imm.updateExtractedText(GeckoApp.surfaceView, mExtractToken, mExtractedText);
> >+        commitText(null, 1);
> So... hitting delete automatically ends the composition? That doesn't sound
> correct..
> 
Only when the composition string is empty. In a perfect world where humans and Androids live peacefully together, the IME should tell us to end the composition when that happens. But some IME's don't do that, so this fixes it.

> >+    /* When a composition is active, Gecko displays a fake selection.
> >+        This fake selection can change while the actual DOM selection stays fixed.
> >+        We need to keep track of this fake selection to inform Gecko. */
> This comment doesn't sound right. The actual DOM selection should've been lost
> the moment we started composing.
Yeah I rephrased that. Basically we should not touch the actual DOM selection during a composition.
Comment 20 Jim Chen [:jchen] [:darchons] 2010-08-02 15:27:44 PDT
Created attachment 462219 [details] [diff] [review]
(4/4) State updates/status notifications (Java side) v2

One major change is the IME delayed enable code. I made the timer task into a separate child class and added the option of resetting the IME. For ResetInputState and CancelComposition, we want to do a delayed reset.

(In reply to comment #14)
> Comment on attachment 461402 [details] [diff] [review]
> (4/4) State updates/status notifications (Java side) v1
>
> >-    public static void showIME(int state) {
> >-        GeckoApp.surfaceView.mIMEState = state;
> >+    protected static void enableIME(boolean enable) {
> Hmmm let's not change mIMEState to mIMEEnabled. The reason we use an int for
> the state is to distinguish between password and non-password fields. state is
> set to nsIWidget::IME_STATUS_PASSWORD for password fields. We'll want to pass
> this info when showing the IME.
> 
You're right. I have another patch for the password bug BTW.

> >+        case NOTIFY_IME_SETOPENSTATE:
> >+            {
> >+                // This mode doesn't seem to exist in the Android IME API
> >+            }
> >+            break;
> This can be removed then, along with the set/getIMEOpenState stuff which
> doesn't do anything for us.
> 
OK
Comment 21 Michael Wu [:mwu] 2010-08-03 18:48:09 PDT
Comment on attachment 462195 [details] [diff] [review]
(1/4) Widget events v1.2

Do you actually want the commented out ALOGIMEs? Looks fine otherwise.
Comment 22 Michael Wu [:mwu] 2010-08-03 18:53:18 PDT
Comment on attachment 462198 [details] [diff] [review]
(2/4) Widget notifications v2

Looks pretty good.

Just one thing - do we actually need the stub Set/GetIMEOpenState functions? IIRC the callers handle the absence of those functions fine.
Comment 23 Michael Wu [:mwu] 2010-08-03 18:59:27 PDT
Comment on attachment 462213 [details] [diff] [review]
(3/4) InputConnection v2


>+    public GeckoEvent(int offset, int count,
>+                      int rangeType, int rangeStyles,
>+                      int rangeForeColor, int rangeBackColor, String text) {
>+        InitIMERange(IME_SET_TEXT, offset, count, rangeType, rangeStyles,
>+                    rangeForeColor, rangeBackColor);
Misalignment here.

>+    public GeckoEvent(int offset, int count,
>+                      int rangeType, int rangeStyles,
>+                      int rangeForeColor, int rangeBackColor) {
>+        InitIMERange(IME_ADD_RANGE, offset, count, rangeType, rangeStyles,
>+                    rangeForeColor, rangeBackColor);
And here.

Looks fine otherwise. I can fix any nits like these on checkin.
Comment 24 Michael Wu [:mwu] 2010-08-03 20:57:47 PDT
Comment on attachment 462219 [details] [diff] [review]
(4/4) State updates/status notifications (Java side) v2


>+        public void run() {
>+            synchronized(IMEStateUpdater.class) {
>+                instance = null;
>+            }
>+
>+            InputMethodManager imm = (InputMethodManager) 
>+                GeckoApp.surfaceView.getContext().getSystemService(
>+                    Context.INPUT_METHOD_SERVICE);
>+            if (imm == null)
>+                return;
>+
>+            if (mReset)
>+                imm.restartInput(GeckoApp.surfaceView);
>+
>+            if (mEnable) {
if (!mEnable)
    return;

>+                if (GeckoApp.surfaceView.mIMEState != 0)
>+                    imm.showSoftInput(GeckoApp.surfaceView, 0);
>+                else
>+                    imm.hideSoftInputFromWindow(
>+                        GeckoApp.surfaceView.getWindowToken(), 0);
>+            }
>+        }
>+    }
>+
>+    public static void notifyIME(int type, int state) {
>+        if (GeckoApp.surfaceView == null ||
>+            GeckoApp.surfaceView.inputConnection == null)
>+            return;
>+
>+        InputMethodManager imm = (InputMethodManager) 
>+            GeckoApp.surfaceView.getContext().getSystemService(
>+                Context.INPUT_METHOD_SERVICE);
>+        if (imm == null)
>+            return;
Who uses imm?

>+
>+        switch (type) {
>+        case NOTIFY_IME_RESETINPUTSTATE:
>+            {
>+                IMEStateUpdater.resetIME();
>+                // keep current enabled state
>+                IMEStateUpdater.enableIME();
>+            }
>+            break;
>+        case NOTIFY_IME_SETENABLED:
>+            {
>+                /* When IME is 'disabled', IME processing is disabled.
>+                    In addition, the IME UI is hidden */
>+                GeckoApp.surfaceView.mIMEState = state;
>+                IMEStateUpdater.enableIME();
>+            }
>+            break;
>+        case NOTIFY_IME_CANCELCOMPOSITION:
>+            {
>+                IMEStateUpdater.resetIME();
>+            }
>+            break;
>+        case NOTIFY_IME_FOCUSCHANGE:
>+            {
>+                GeckoApp.surfaceView.mIMEFocus = state != 0;
>+            }
>+            break;
Curly braces in these cases not necessary.

>+        }
>+        return;
No need to return at the end of a void method.

>+    }
>+
>+    public static void notifyIMEChange(String text, int start, int end, int newEnd) {
>+        if (GeckoApp.surfaceView == null ||
>+            GeckoApp.surfaceView.inputConnection == null)
>+            return;
>+
>+        InputMethodManager imm = (InputMethodManager) 
>+            GeckoApp.surfaceView.getContext().getSystemService(
>+                Context.INPUT_METHOD_SERVICE);
>+        if (imm == null)
>+            return;
>+
>+        if (newEnd < 0) {
>+            GeckoApp.surfaceView.inputConnection.notifySelectionChange(
>+                imm, start, end);
>+        } else {
>+            GeckoApp.surfaceView.inputConnection.notifyTextChange(
>+                imm, text, start, end, newEnd);
>+        }
Avoid curly braces here.

>+        return;
And this return.

>     }
> 
>     public static void enableAccelerometer(boolean enable) {
>         SensorManager sm = (SensorManager) 
>             GeckoApp.surfaceView.getContext().getSystemService(Context.SENSOR_SERVICE);
> 
>         if (enable) {
>             Sensor accelSensor = sm.getDefaultSensor(Sensor.TYPE_ACCELEROMETER);
>diff --git a/embedding/android/GeckoInputConnection.java b/embedding/android/GeckoInputConnection.java
>--- a/embedding/android/GeckoInputConnection.java
>+++ b/embedding/android/GeckoInputConnection.java
>@@ -204,16 +204,19 @@ public class GeckoInputConnection
>         extract.selectionEnd = mSelectionStart + mSelectionLength;
> 
>         GeckoAppShell.sendEventToGecko(
>             new GeckoEvent(GeckoEvent.IME_GET_TEXT, 0, Integer.MAX_VALUE));
>         try {
>             extract.startOffset = 0;
>             extract.text = mQueryResult.take();
> 
>+            if ((flags & GET_EXTRACTED_TEXT_MONITOR) != 0) {
>+                mUpdateRequest = req;
>+            }
And here.

>             return extract;
> 
>         } catch (InterruptedException e) {
>             Log.e("GeckoAppJava", "IME: getExtractedText interrupted");
>             return null;
>         }
>     }
> 
>@@ -415,25 +418,70 @@ public class GeckoInputConnection
>             if (mComposingText.length() > 0)
>                 return false;
>         }
> 
>         commitText(null, 1);
>         return true;
>     }
> 
>+    public void notifyTextChange(InputMethodManager imm, String text,
>+                                 int start, int oldEnd, int newEnd) {
>+        //Log.d("GeckoAppJava", "IME: notifyTextChange");
>+
>+        if (mUpdateRequest == null) {
>+            return;
>+        }
And here.

>+
>+        mUpdateExtract.flags = 0;
>+        mUpdateExtract.partialStartOffset = 0;
>+        mUpdateExtract.partialEndOffset = oldEnd;
>+
>+        // Faster to not query for selection
>+        mUpdateExtract.selectionStart = newEnd;
>+        mUpdateExtract.selectionEnd = newEnd;
>+
>+        mUpdateExtract.text = text;
>+        mUpdateExtract.startOffset = 0;
>+
>+        imm.updateExtractedText(GeckoApp.surfaceView,
>+            mUpdateRequest.token, mUpdateExtract);
>+
>+        return;
And here.

>+    }
>+    
>+    public void notifySelectionChange(InputMethodManager imm,
>+                                      int start, int end) {
>+        //Log.d("GeckoAppJava", "IME: notifySelectionChange");
>+
>+        if (mComposing) {
>+            imm.updateSelection(GeckoApp.surfaceView,
>+                mCompositionStart + mCompositionSelStart,
>+                mCompositionStart + mCompositionSelStart + mCompositionSelLen,
>+                mCompositionStart,
>+                mCompositionStart + mComposingText.length());
>+
>+        } else {
>+            imm.updateSelection(GeckoApp.surfaceView, start, end, -1, -1);
>+        }
And here.

>diff --git a/embedding/android/GeckoSurfaceView.java b/embedding/android/GeckoSurfaceView.java
>--- a/embedding/android/GeckoSurfaceView.java
>+++ b/embedding/android/GeckoSurfaceView.java
>@@ -223,21 +223,23 @@ class GeckoSurfaceView
>         }
>         mSoftwareBitmap.copyPixelsFromBuffer(mSoftwareBuffer);
>         c.drawBitmap(mSoftwareBitmap, 0, 0, null);
>         getHolder().unlockCanvasAndPost(c);
>     }
> 
>     @Override
>     public boolean onCheckIsTextEditor () {
>-        return false;
>+        return mIMEFocus;
Can you see if it makes any difference if we just return false all the time? If it doesn't, I would prefer to always return false.

>     }
> 
>     @Override
>     public InputConnection onCreateInputConnection(EditorInfo outAttrs) {
>+        if (!mIMEFocus) return null;
Return on next line.
Comment 25 Jim Chen [:jchen] [:darchons] 2010-08-03 22:37:15 PDT
Created attachment 462684 [details] [diff] [review]
Combined patch

(In reply to comment #21)
> Comment on attachment 462195 [details] [diff] [review]
> (1/4) Widget events v1.2
> 
> Do you actually want the commented out ALOGIMEs? Looks fine otherwise.

Yeah useful to keep them.

(In reply to comment #22)
> Comment on attachment 462198 [details] [diff] [review]
> (2/4) Widget notifications v2
> 
> Just one thing - do we actually need the stub Set/GetIMEOpenState functions?
> IIRC the callers handle the absence of those functions fine.

I think we should keep the stubs there. Costs nothing.

(In reply to comment #24)
> Comment on attachment 462219 [details] [diff] [review]
> (4/4) State updates/status notifications (Java side) v2
> 
> >+            GeckoApp.surfaceView.getContext().getSystemService(
> >+                Context.INPUT_METHOD_SERVICE);
> >+        if (imm == null)
> >+            return;
> Who uses imm?
> 
Hm didn't see that. Removed.

> >+        } else {
> >+            GeckoApp.surfaceView.inputConnection.notifyTextChange(
> >+                imm, text, start, end, newEnd);
> >+        }
> Avoid curly braces here.
> 
Old habit :D

> >     @Override
> >     public boolean onCheckIsTextEditor () {
> >-        return false;
> >+        return mIMEFocus;
> Can you see if it makes any difference if we just return false all the time? If
> it doesn't, I would prefer to always return false.
> 
According to Android source code, it only gets called when view is focusing, so yeah not a big deal to return false.
Comment 26 Michael Wu [:mwu] 2010-08-04 11:40:01 PDT
Comment on attachment 462684 [details] [diff] [review]
Combined patch

I'll check this in.
Comment 27 Michael Wu [:mwu] 2010-08-04 12:49:21 PDT
http://hg.mozilla.org/mozilla-central/rev/6cc5db7e2fad

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