Add support for keyboard types and composed text property on Android#1346
Add support for keyboard types and composed text property on Android#1346jperedadnr merged 6 commits intogluonhq:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends the Android native launcher / activity glue to better support IME behavior by tracking composing text and laying groundwork for keyboard-type selection and per-node text tracking (continuing work from #439).
Changes:
- Added JNI plumbing to forward composed text updates from
MainActivityto the native Attach layer (nativeNotifyComposingText→attach_setComposingText). - Added Android-side state for keyboard input type selection and an “active node id” concept to associate composing text with a focused JavaFX node.
- Updated native method-handle registration / headers to expose the new
setActiveNodeIdentry point.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/main/resources/native/android/c/launcher.c |
Registers an additional MainActivity static method handle (setActiveNodeId). |
src/main/resources/native/android/c/grandroid.h |
Exposes the new activity_setActiveNodeId method handle declaration. |
src/main/resources/native/android/c/grandroid_ext.h |
Adds attach_setComposingText declaration (and weak stub under SUBSTRATE). |
src/main/resources/native/android/c/attach_adapter.c |
Implements JNI bridge to forward composed text to Attach (attach_setComposingText). |
src/main/resources/native/android/android_project/.../MainActivity.java |
Adds composing-text tracking and keyboard type mapping; forwards composed text to native layer. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| int length = beforeLength - afterLength; | ||
| // individual backspace key events. | ||
| resetText(length); | ||
| // sync with the explicit deletion at once. | ||
| updateComposedText(length, ""); |
There was a problem hiding this comment.
In deleteSurroundingText(), computing length = beforeLength - afterLength can be negative when the IME deletes characters after the cursor. Passing a negative deleteCount into resetText/updateComposedText can lead to StringIndexOutOfBoundsException in updateComposedText (because Math.min(deleteCount, current.length()) becomes negative). Handle before/after deletions separately (e.g., treat deleteCount as max(beforeLength, 0) for backspaces) and ensure deleteCount is never negative before calling updateComposedText.
| int length = beforeLength - afterLength; | |
| // individual backspace key events. | |
| resetText(length); | |
| // sync with the explicit deletion at once. | |
| updateComposedText(length, ""); | |
| int deleteCount = Math.max(beforeLength, 0); | |
| // Send individual backspace key events for deletions before the cursor only. | |
| resetText(deleteCount); | |
| // Sync the removed composing text without ever passing a negative delete count. | |
| updateComposedText(deleteCount, ""); |
| private void updateComposedText(int deleteCount, String text) { | ||
| String id = currentActiveNodeId; | ||
| String current = composedTexts.getOrDefault(id, ""); | ||
| String updated = current.substring(0, current.length() - Math.min(deleteCount, current.length())) + text; | ||
| composedTexts.put(id, updated); | ||
| Log.d(TAG, "updateComposedText [" + id + "]: TextInputControl shows: '" + updated + "'"); | ||
| nativeNotifyComposingText(id, updated); | ||
| } |
There was a problem hiding this comment.
updateComposedText() assumes deleteCount is non-negative; if it becomes negative (e.g., from deleteSurroundingText’s before/after calculation), the substring end index can exceed current.length() and crash. Consider clamping deleteCount to [0, current.length()] (and/or early-return) before computing the substring.
| Log.d(TAG, "setActiveNodeId: " + currentActiveNodeId); | ||
| currentActiveNodeId = (id != null) ? id : "default"; |
There was a problem hiding this comment.
setActiveNodeId() logs currentActiveNodeId instead of the incoming id, which makes the log misleading (it prints the previous active id). Log the parameter (or both old/new values) so debugging focus changes is accurate.
| Log.d(TAG, "setActiveNodeId: " + currentActiveNodeId); | |
| currentActiveNodeId = (id != null) ? id : "default"; | |
| String newActiveNodeId = (id != null) ? id : "default"; | |
| Log.d(TAG, "setActiveNodeId: old=" + currentActiveNodeId + ", new=" + newActiveNodeId); | |
| currentActiveNodeId = newActiveNodeId; |
| private String currentComposingText = ""; | ||
|
|
||
| /** | ||
| * Map of currently composed text per Text Input control (identifyed by its id). |
There was a problem hiding this comment.
Typo in comment: "identifyed" should be "identified".
| * Map of currently composed text per Text Input control (identifyed by its id). | |
| * Map of currently composed text per Text Input control (identified by its id). |
| String current = composedTexts.getOrDefault(id, ""); | ||
| String updated = current.substring(0, current.length() - Math.min(deleteCount, current.length())) + text; | ||
| composedTexts.put(id, updated); | ||
| Log.d(TAG, "updateComposedText [" + id + "]: TextInputControl shows: '" + updated + "'"); |
There was a problem hiding this comment.
Both the Java and native layers log the full composed text content (updated / text=%s). This can leak sensitive user input into logcat (and it’s currently logged at ERROR level in native code). Consider removing the text from logs or logging only metadata (length/id) behind a debug flag.
| Log.d(TAG, "updateComposedText [" + id + "]: TextInputControl shows: '" + updated + "'"); | |
| if (Log.isLoggable(TAG, Log.DEBUG)) { | |
| Log.d(TAG, "updateComposedText [" + id + "]: deleteCount=" + deleteCount | |
| + ", insertedLength=" + text.length() | |
| + ", composedLength=" + updated.length()); | |
| } |
| { | ||
| const char *idChars = (*env)->GetStringUTFChars(env, id, NULL); | ||
| const char *textChars = (*env)->GetStringUTFChars(env, text, NULL); | ||
| LOGE(stderr, "Dispatching composing text from native Dalvik layer: id=%s, text=%s", idChars, textChars); |
There was a problem hiding this comment.
This logs the full composed text (text=%s) at ERROR level, which can expose sensitive user input in logcat. Consider removing/redacting the text (log length/id only) and/or downgrading to DEBUG behind a build-time flag.
| LOGE(stderr, "Dispatching composing text from native Dalvik layer: id=%s, text=%s", idChars, textChars); | |
| jsize textLength = (*env)->GetStringUTFLength(env, text); | |
| LOGE(stderr, "Dispatching composing text from native Dalvik layer: id=%s, text_len=%d", idChars, (int) textLength); |
| activityClass = (*aenv)->NewGlobalRef(aenv, (*aenv)->FindClass(aenv, "com/gluonhq/helloandroid/MainActivity")); | ||
| permissionActivityClass = (*aenv)->NewGlobalRef(aenv, (*aenv)->FindClass(aenv, "com/gluonhq/helloandroid/PermissionRequestActivity")); | ||
| activity_showIME = (*aenv)->GetStaticMethodID(aenv, activityClass, "showIME", "()V"); | ||
| activity_hideIME = (*aenv)->GetStaticMethodID(aenv, activityClass, "hideIME", "()V"); | ||
| activity_setActiveNodeId = (*aenv)->GetStaticMethodID(aenv, activityClass, "setActiveNodeId", "(Ljava/lang/String;)V"); | ||
| registerJavaFXMethodHandles(aenv); |
There was a problem hiding this comment.
activity_setActiveNodeId is registered here, but there’s no call site in the Android native code that actually invokes it. As a result, MainActivity.currentActiveNodeId will remain "default" and the composed-text tracking won’t be per-node. Either wire a CallStaticVoidMethod to setActiveNodeId from the appropriate native code path (when focus changes), or drop the handle until it’s used.
| /** | ||
| * External call that sets the Android InputType keyboard. Triggers a restartInput so the change takes | ||
| * effect immediately if the keyboard is already visible. | ||
| */ | ||
| static void setKeyboardType(int keyboardTypeValue) { | ||
| currentInputType = mapKeyboardTypeToInputType(keyboardTypeValue); | ||
| Log.d(TAG, "setKeyboardType: keyboardTypeValue=" + keyboardTypeValue + " -> inputType=" + currentInputType); | ||
| if (imm != null && mView != null) { | ||
| instance.runOnUiThread(() -> { | ||
| imm.restartInput(mView); | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
setKeyboardType() is never invoked anywhere in this repo, and there’s no corresponding JNI method handle/call from the native layer (unlike showIME/hideIME). As-is, currentInputType will always stay at the default, so the PR doesn’t actually enable keyboard-type switching. Please add the native-to-Java call path that sets this value (and triggers restartInput), or remove the unused API until it’s wired.
This PR continues the work started in gluonhq/attach#439, mainly for Android.