Closed Bug 1416918 Opened 5 years ago Closed 5 years ago

Provide InputConnection/key-event API for GeckoSession

Categories

(GeckoView :: IME, enhancement)

Product:
GeckoView
Component:
IME
Platform:
All
Android
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(6 files)

Bug 1416918 - 1. Add TextInputController;
59 bytes, text/x-review-board-request
esawin
: review+
Details
Bug 1416918 - 2. Make GeckoEditable/GeckoInputConnection work with TextInputController;
59 bytes, text/x-review-board-request
esawin
: review+
Details
Bug 1416918 - 3. Use TextInputController in GeckoView;
59 bytes, text/x-review-board-request
esawin
: review+
Details
Bug 1416918 - 4. Remove unused code;
59 bytes, text/x-review-board-request
esawin
: review+
Details
Bug 1416918 - 5. Update generated bindings;
59 bytes, text/x-review-board-request
jchen
: review+
Details
Bug 1416918 - 4b. Move GeckoEditable{Client,Listener} to TextInputController;
59 bytes, text/x-review-board-request
esawin
: review+
Details 
GeckoSession should provide an API for getting an InputConnection instance and for handling key events. That will involve changing GeckoEditable because it relies on GeckoView right now. Furthermore, it would be even better if we can get rid of the WrapForJNI annotations from GeckoEditable itself, to prepare for the eventual process split. Finally, GeckoView will use the new API to handle methods like onCreateInputConnection.
Assignee: nobody → nchen
Status: NEW → ASSIGNED
Comment on attachment 8935428 [details]
Bug 1416918 - 1. Add TextInputController;

https://reviewboard.mozilla.org/r/206324/#review212086

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoSession.java:559
(Diff revision 1)
> +        ThreadUtils.assertOnUiThread();
> +
> +        if (mTextInput == null) {
> +            mTextInput = new TextInputController(this);
> +            if (mWindow != null) {
> +                mTextInput.onWindowReady(mNativeQueue, mWindow);

Can we guarantee that mWindow is set?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/TextInputController.java:18
(Diff revision 1)
> +import android.view.KeyEvent;
> +import android.view.View;
> +import android.view.inputmethod.EditorInfo;
> +import android.view.inputmethod.InputConnection;
> +
> +public final class TextInputController {

Please add a class description.

::: widget/android/nsWindow.cpp
(Diff revision 1)
>  
>  void
>  nsWindow::GeckoViewSupport::Attach(const GeckoSession::Window::LocalRef& inst,
>                                     jni::Object::Param aView)
>  {
> -    // Associate our previous GeckoEditable with the new GeckoView.

Can we remove this now?
Attachment #8935428 - Flags: review?(esawin) → review+
Comment on attachment 8935429 [details]
Bug 1416918 - 2. Make GeckoEditable/GeckoInputConnection work with TextInputController;

https://reviewboard.mozilla.org/r/206326/#review212100

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoEditable.java:46
(Diff revision 1)
>     GeckoEditable implements only some functions of Editable
>     The field mText contains the actual underlying
>     SpannableStringBuilder/Editable that contains our text.
>  */
> -final class GeckoEditable extends IGeckoEditableParent.Stub
> -        implements InvocationHandler, Editable, GeckoEditableClient {
> +/* package */ final class GeckoEditable extends IGeckoEditableParent.Stub
> +                                        implements InvocationHandler,

To improve readability, I would prefer moving both, extends and implements, into a new line but with the base 4 spaces indentation. This will allow more consistent indentation across classes, including those with absurdly long names.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoInputConnection.java:48
(Diff revision 1)
>  import android.view.inputmethod.ExtractedTextRequest;
>  import android.view.inputmethod.InputConnection;
>  import android.view.inputmethod.InputMethodManager;
>  
> -class GeckoInputConnection
> -    extends BaseInputConnection
> +/* package */ class GeckoInputConnection extends BaseInputConnection
> +                                         implements TextInputController.Delegate,

See above comment.
Attachment #8935429 - Flags: review?(esawin) → review+
Comment on attachment 8935430 [details]
Bug 1416918 - 3. Use TextInputController in GeckoView;

https://reviewboard.mozilla.org/r/206328/#review212104
Attachment #8935430 - Flags: review?(esawin) → review+
Comment on attachment 8935431 [details]
Bug 1416918 - 4. Remove unused code;

https://reviewboard.mozilla.org/r/206330/#review212106
Attachment #8935431 - Flags: review?(esawin) → review+
Comment on attachment 8935869 [details]
Bug 1416918 - 4b. Move GeckoEditable{Client,Listener} to TextInputController;

https://reviewboard.mozilla.org/r/206726/#review212990
Attachment #8935869 - Flags: review?(esawin) → review+
Comment on attachment 8935432 [details]
Bug 1416918 - 5. Update generated bindings;

https://reviewboard.mozilla.org/r/206332/#review213372
Attachment #8935432 - Flags: review+
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3e18c58d0320
1. Add TextInputController; r=esawin
https://hg.mozilla.org/integration/autoland/rev/29a3c1e94980
2. Make GeckoEditable/GeckoInputConnection work with TextInputController; r=esawin
https://hg.mozilla.org/integration/autoland/rev/39f1117294d1
3. Use TextInputController in GeckoView; r=esawin
https://hg.mozilla.org/integration/autoland/rev/40e3669c4a30
4. Remove unused code; r=esawin
https://hg.mozilla.org/integration/autoland/rev/a0b80fff3ab5
4b. Move GeckoEditable{Client,Listener} to TextInputController; r=esawin
https://hg.mozilla.org/integration/autoland/rev/cc7d974a7a95
5. Update generated bindings; r=jchen
Depends on: 1449569
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 59 → mozilla59

Moving some cursor and key event bugs to the new GeckoView::IME component.

Component: General → IME
You need to log in before you can comment on or make changes to this bug.