From a28f118786e13a3bc8e5b455d3b8c2d0dfa5a126 Mon Sep 17 00:00:00 2001 From: will wade Date: Thu, 15 Oct 2026 15:08:10 +0100 Subject: [PATCH 1/3] fix(ime): atomic per-frame output + force-show on hw-keyboard apps (bug #5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit User report #5: "When using Dasher in docked mode, the typed letters get completely jumbled up in applications like OneNote and Word, though it works perfectly fine inside AquaMail" and "Dasher wouldn't pop up on Chrome… on the MuMu emulator, I had to switch to Firefox." Part 1 — jumbled text in OneNote/Word. The IME applied every engine delta as its own binder call (one commitText per node entry, one deleteSurroundingText per deletion). Editors with synchronous InputConnections (AquaMail's EditText) apply those in order; OneNote and Word route InputConnection edits through their own async editing pipeline (undo coalescing, autosave) where a burst of unbatched deltas can land out of order or partially — garbled text. Same class of problem Dasher-Windows #17 fixed by forwarding deletions through the target's own input path. - ImeOutputCoalescer (pure, unit-tested): accumulates the frame's deltas into one Flush(deleteUnits, insert); a delete that spans the flushed/pending boundary is split exactly (UTF-16 units, surrogate pairs included). - flushOutputToTarget(): applied once per frame after the canvas submit, inside beginBatchEdit/endBatchEdit — the editor sees one atomic edit, not N unrelated calls. - Deletion fallback: if deleteSurroundingText returns false (WebView/custom editors), send KEYCODE_DEL key events through the target's own key pipeline instead of dropping the deletion (which silently desynced the engine buffer from the field — every later commit then landed wrong). - Buffer-clear (type 2) stays a no-op: committed text belongs to the field; the engine re-anchors separately. Part 2 — no popup on Chrome under a hardware keyboard. Emulators (MuMu) ship with a hardware keyboard attached; in that state Chrome never calls showSoftInput when a field gains focus (Firefox does). onEvaluateInputViewShown()=true only helps when someone asks. The IME now requestShowSelf()s on every fresh focus of a real text field (TYPE_NULL excluded) — consistent with the existing accessibility decision that our users always want the soft keyboard. Signed-off-by: will wade --- .../at/dasher/android/DasherImeService.kt | 73 +++++++++++++- .../at/dasher/android/ImeOutputCoalescer.kt | 81 +++++++++++++++ .../dasher/android/ImeOutputCoalescerTest.kt | 99 +++++++++++++++++++ 3 files changed, 248 insertions(+), 5 deletions(-) create mode 100644 app/src/main/java/at/dasher/android/ImeOutputCoalescer.kt create mode 100644 app/src/test/java/at/dasher/android/ImeOutputCoalescerTest.kt diff --git a/app/src/main/java/at/dasher/android/DasherImeService.kt b/app/src/main/java/at/dasher/android/DasherImeService.kt index 8966eb2..5f9f768 100644 --- a/app/src/main/java/at/dasher/android/DasherImeService.kt +++ b/app/src/main/java/at/dasher/android/DasherImeService.kt @@ -14,6 +14,7 @@ import android.os.Looper import android.util.Log import android.util.TypedValue import android.view.Gravity +import android.view.KeyEvent import android.view.MotionEvent import android.view.View import android.view.ViewGroup @@ -34,6 +35,12 @@ import at.dasher.android.ui.DasherCanvasView class DasherImeService : InputMethodService() { private var engine: DasherEngine? = null + + // Bug #5 ("letters get completely jumbled up in OneNote and Word"): + // engine deltas coalesce here and flush ONCE per frame as one atomic + // batch edit — see ImeOutputCoalescer. + private val outputCoalescer = ImeOutputCoalescer() + private var canvasView: DasherCanvasView? = null private var canvasHost: android.widget.FrameLayout? = null private var dockedRoot: LinearLayout? = null @@ -424,6 +431,10 @@ class DasherImeService : InputMethodService() { val userDir = DataInstaller.userDir(this).apply { mkdirs() } val eng = DasherEngine.create(dataDir, userDir.absolutePath) { commands, strings -> canvasView?.submitFrame(commands, strings) + // All of this frame's engine deltas have now fired — apply them + // to the target field as ONE atomic edit (bug #5: unbatched + // deltas garble text in OneNote/Word's async editors). + flushOutputToTarget() } if (eng == null) { Log.e(TAG, "IME engine creation failed (dataDir=$dataDir)") @@ -544,12 +555,10 @@ class DasherImeService : InputMethodService() { // Per-engine-instance listeners (#56): the JNI dispatch passes the // handle; only THIS engine's listener is called. No cross-talk with // the main app's engine. + // Output deltas are COALESCED, not applied — see flushOutputToTarget + // (bug #5: OneNote/Word jumbled unbatched per-delta edits). eng.setOutputListener { type, text -> - val ic = currentInputConnection - if (ic != null) { - if (type == 0) ic.commitText(text, 1) - else if (text.isNotEmpty()) ic.deleteSurroundingText(text.length, 0) - } + outputCoalescer.onEngineOutput(type, text) } eng.setMessageListener { _, msg -> Toast.makeText(this, msg, Toast.LENGTH_SHORT).show() @@ -561,6 +570,40 @@ class DasherImeService : InputMethodService() { eng.clearSpeakListener() // IME doesn't speak (the app does) } + /** + * Applies the coalesced frame output to the target field as one atomic + * batch edit (bug #5): first the deletion, then the insertion, inside a + * beginBatchEdit/endBatchEdit pair so the editor cannot reformat between + * the two halves. + * + * Deletion strategy: deleteSurroundingText first; if the editor reports + * it did not handle it (WebView/custom editors — precisely the + * OneNote/Word class), fall back to KEYCODE_DEL key events, which go + * through the target's own key pipeline where a delta would be dropped + * (the same principle as Dasher-Windows #17's deletion forwarding). + * + * No connection yet (field switch in flight): keep the deltas pending — + * the next frame flushes them once a field is served. + */ + private fun flushOutputToTarget() { + val ic = currentInputConnection ?: return + val flush = outputCoalescer.takeFlush() ?: return + ic.beginBatchEdit() + try { + if (flush.deleteUnits > 0) { + if (!ic.deleteSurroundingText(flush.deleteUnits, 0)) { + repeat(flush.deleteUnits) { + ic.sendKeyEvent(KeyEvent(KeyEvent.ACTION_DOWN, KeyEvent.KEYCODE_DEL)) + ic.sendKeyEvent(KeyEvent(KeyEvent.ACTION_UP, KeyEvent.KEYCODE_DEL)) + } + } + } + if (flush.insert.isNotEmpty()) ic.commitText(flush.insert, 1) + } finally { + ic.endBatchEdit() + } + } + // API 36+ (Android 16): measure the actual docked-root position in the // window so the insets animation has a concrete target (review C1/C3: // imeHeightPx() is a HEIGHT, not an offset — a constant created a dead @@ -581,6 +624,26 @@ class DasherImeService : InputMethodService() { outInsets.touchableInsets = Insets.TOUCHABLE_INSETS_CONTENT } + /** + * Bug #5b ("Dasher wouldn't pop up on Chrome… on the MuMu emulator, I had + * to switch to Firefox"): while a hardware keyboard is attached (emulators + * default to one), Chrome never asks the system to show the IME when a + * field gains focus — Firefox asks regardless, which is why it worked + * there. onEvaluateInputViewShown()=true only helps when something asks; + * nobody asked. We serve motor-impaired AAC users who always want the + * soft keyboard, so ask ourselves on every fresh focus of a real text + * field (TYPE_NULL fields are non-editing views and stay untouched). + */ + override fun onStartInput(info: EditorInfo?, restarting: Boolean) { + super.onStartInput(info, restarting) + if (!restarting && info != null && + info.inputType != android.text.InputType.TYPE_NULL && + !isInputViewShown + ) { + requestShowSelf(0) + } + } + override fun onStartInputView(info: EditorInfo?, restarting: Boolean) { super.onStartInputView(info, restarting) // Re-assert ALL IME-owned static listeners — the main app may have diff --git a/app/src/main/java/at/dasher/android/ImeOutputCoalescer.kt b/app/src/main/java/at/dasher/android/ImeOutputCoalescer.kt new file mode 100644 index 0000000..59daef0 --- /dev/null +++ b/app/src/main/java/at/dasher/android/ImeOutputCoalescer.kt @@ -0,0 +1,81 @@ +package at.dasher.android + +/** + * Coalesces engine output deltas into ONE atomic flush per frame (user bug + * report #5: "typed letters get completely jumbled up in applications like + * OneNote and Word, though it works perfectly fine inside AquaMail"). + * + * Dasher emits text as a stream of small deltas — one commit per node entry, + * one delete per backspace-class control node. Applied delta-by-delta, each + * op is a separate binder call that the TARGET app applies on its own + * pipeline. Editors with synchronous InputConnections (AquaMail's EditText) + * apply them in order and everything works. OneNote and Word route edits + * through their own async editing engines (undo coalescing, autosave), + * where rapid unbatched deltas can land out of order or partially — the + * field ends up jumbled. (Android's version of the problem Dasher-Windows + * #17 fixed by forwarding deletions through the target's own input path.) + * + * The IME feeds every engine output event here as it fires during + * dasher_frame(), then flushes ONCE after the frame inside a + * beginBatchEdit/endBatchEdit pair: the editor sees a single atomic + * "delete N, insert S" step instead of a burst of unrelated calls. + * + * Pure logic — no InputConnection, so the delta arithmetic (including the + * surrogate-pair and boundary-spanning cases) is unit-testable, following + * the EditorSync.kt pattern. + */ +class ImeOutputCoalescer { + + /** + * One atomic edit: delete [deleteUnits] UTF-16 code units before the + * caret, then insert [insert]. Either half may be empty. + */ + data class Flush(val deleteUnits: Int, val insert: String) + + private var pendingDeleteUnits = 0 + private val pendingInsert = StringBuilder() + + /** + * Engine output event (dasher_output_callback semantics from dasher.h): + * 0 = commit `text`; 1 = delete `text` (a backspace — always removes the + * buffer TAIL); 2 = buffer clear (dasher_reset / alphabet change — no + * delta can express it; the IME re-anchors the engine separately, and + * committed text already belongs to the target field). + */ + fun onEngineOutput(type: Int, text: String) { + when (type) { + 0 -> pendingInsert.append(text) + 1 -> { + if (text.isEmpty()) return + // The delete removes the engine buffer's tail. That tail is + // [text already flushed to the field][pending insert]. Split + // the deleted length across that boundary: + if (text.length <= pendingInsert.length) { + // Entirely inside this frame's pending insert — those + // chars never reached the field; just cancel them. + pendingInsert.setLength(pendingInsert.length - text.length) + } else { + // Spans past the pending insert into text the field + // already holds — cancel all of pending, and delete only + // the remainder from the field. + pendingDeleteUnits += text.length - pendingInsert.length + pendingInsert.setLength(0) + } + } + else -> {} // type 2 (buffer clear) — nothing to apply + } + } + + /** + * The pending edit, or null when nothing accumulated. Resets the state — + * callers own the returned edit. (UTF-16 units throughout: + * deleteSurroundingText and String.length agree; a surrogate pair is 2.) + */ + fun takeFlush(): Flush? { + if (pendingDeleteUnits == 0 && pendingInsert.isEmpty()) return null + val flush = Flush(pendingDeleteUnits, pendingInsert.toString()) + pendingDeleteUnits = 0 + pendingInsert.setLength(0) + return flush + } +} diff --git a/app/src/test/java/at/dasher/android/ImeOutputCoalescerTest.kt b/app/src/test/java/at/dasher/android/ImeOutputCoalescerTest.kt new file mode 100644 index 0000000..d6f18a7 --- /dev/null +++ b/app/src/test/java/at/dasher/android/ImeOutputCoalescerTest.kt @@ -0,0 +1,99 @@ +package at.dasher.android + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Test + +/** + * Bug #5 delta arithmetic (pure — no engine/InputConnection): + * engine deltas coalesce per frame, deletes split correctly across the + * flushed-text/pending-insert boundary, UTF-16 units throughout. + */ +class ImeOutputCoalescerTest { + + @Test + fun nothing_pending_flushes_null() { + assertNull(ImeOutputCoalescer().takeFlush()) + } + + @Test + fun commits_within_a_frame_merge_into_one_insert() { + val c = ImeOutputCoalescer() + c.onEngineOutput(0, "th") + c.onEngineOutput(0, "ere") + assertEquals(ImeOutputCoalescer.Flush(0, "there"), c.takeFlush()) + } + + @Test + fun delete_of_flushed_text_becomes_delete_units() { + val c = ImeOutputCoalescer() + // Previous frame committed "there" and flushed; this frame the user + // deletes "ere" — nothing pending, so the whole delete hits the field. + c.onEngineOutput(1, "ere") + assertEquals(ImeOutputCoalescer.Flush(3, ""), c.takeFlush()) + } + + @Test + fun delete_inside_the_pending_insert_cancels_it() { + val c = ImeOutputCoalescer() + c.onEngineOutput(0, "ther") + c.onEngineOutput(1, "r") // engine reversed before anything was sent + assertEquals(ImeOutputCoalescer.Flush(0, "the"), c.takeFlush()) + } + + @Test + fun delete_spanning_boundary_cancels_pending_and_deletes_remainder() { + val c = ImeOutputCoalescer() + // Field already holds "hello " (flushed last frame); this frame the + // engine committed "wor" then the user deleted "o wor" — 5 units + // removed, of which 3 were still pending. + c.onEngineOutput(0, "wor") + c.onEngineOutput(1, "o wor") + assertEquals(ImeOutputCoalescer.Flush(2, ""), c.takeFlush()) + } + + @Test + fun delete_then_commit_keeps_both_halves_in_order() { + val c = ImeOutputCoalescer() + c.onEngineOutput(1, "e ") // fix "the " → "th" + c.onEngineOutput(0, "at") // now "that" + assertEquals(ImeOutputCoalescer.Flush(2, "at"), c.takeFlush()) + } + + @Test + fun take_flush_resets_state() { + val c = ImeOutputCoalescer() + c.onEngineOutput(0, "a") + assertEquals(ImeOutputCoalescer.Flush(0, "a"), c.takeFlush()) + assertNull(c.takeFlush()) + // And accumulates afresh after a flush. + c.onEngineOutput(0, "b") + assertEquals(ImeOutputCoalescer.Flush(0, "b"), c.takeFlush()) + } + + @Test + fun buffer_clear_emits_nothing() { + // dasher.h type 2: buffer clear carries empty text — the deltas + // cannot express it and the IME re-anchors instead. It must not + // poison pending state either. + val c = ImeOutputCoalescer() + c.onEngineOutput(2, "") + assertNull(c.takeFlush()) + } + + @Test + fun surrogate_pairs_count_two_units() { + // deleteSurroundingText counts UTF-16 code units: an emoji is 2 — + // String.length already agrees, pinning the contract. + val c = ImeOutputCoalescer() + c.onEngineOutput(1, "😀") + assertEquals(ImeOutputCoalescer.Flush(2, ""), c.takeFlush()) + } + + @Test + fun empty_delete_text_is_ignored() { + val c = ImeOutputCoalescer() + c.onEngineOutput(1, "") + assertNull(c.takeFlush()) + } +} From ad8ebc170e8ce66b8276951aff4f375f09a534a9 Mon Sep 17 00:00:00 2001 From: will wade Date: Thu, 15 Oct 2026 15:40:11 +0100 Subject: [PATCH 2/3] =?UTF-8?q?fix(ime):=20review=20loop=201=20=E2=80=94?= =?UTF-8?q?=20DEL=20fallback=20counts=20chars=20(not=20UTF-16=20units),=20?= =?UTF-8?q?flood=20cap,=20field-switch=20reset,=20contract=20docs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the #71 review (score 7/10): - C1: the KEYCODE_DEL fallback sent one DEL per UTF-16 unit, but one DEL deletes one CHARACTER — a surrogate pair (any emoji; we ship an Emoji alphabet) would over-delete into the preceding character. New pure delKeyEventCount(units, beforeCursor) derives the count from the actual text before the caret (codepoints in the last N units); unit-tested. - C2: bulk control-mode deletes could flood the target with 2×N key events in one frame (jank/ANR). DEL fallback is now capped at MAX_DEL_KEY_EVENTS=64; beyond it the deletion is skipped with a log (desync is recoverable, an ANR is not). - I2: pending deltas from the previous field could land in the next focused field after a null-IC flush. onStartInput(restarting=false) now clears the coalescer — text must never appear in the wrong app. - I1: the class doc claimed deletes always remove the buffer tail — false for control-mode deletes (CAPI.cpp ctrlDelete removes AT the cursor, possibly mid-buffer/forwards; the event carries no position or direction). Documented as an approximation (same as pre-batching behavior); upstream filed as DasherCore#100. editDelete also fires the callback when its guard rejects the erase — DasherCore#101. - I3: tests for the risky paths — delKeyEventCount (surrogates, null, clamped), accumulating deletes, interleaved delete/commit/spanning- delete, clear() semantics. M3 (NewStringUTF mangling astral chars) split out as #72. - M1/M2/M4/M5: Hide-vs-force-show documented as deliberate; InputType import; unknown event types logged; type-2 safety invariant noted. Signed-off-by: will wade --- .../at/dasher/android/DasherImeService.kt | 61 +++++++++++--- .../at/dasher/android/ImeOutputCoalescer.kt | 80 +++++++++++++++---- .../dasher/android/ImeOutputCoalescerTest.kt | 67 ++++++++++++++++ 3 files changed, 180 insertions(+), 28 deletions(-) diff --git a/app/src/main/java/at/dasher/android/DasherImeService.kt b/app/src/main/java/at/dasher/android/DasherImeService.kt index 5f9f768..e91103b 100644 --- a/app/src/main/java/at/dasher/android/DasherImeService.kt +++ b/app/src/main/java/at/dasher/android/DasherImeService.kt @@ -11,6 +11,7 @@ import android.inputmethodservice.InputMethodService import android.net.Uri import android.os.Handler import android.os.Looper +import android.text.InputType import android.util.Log import android.util.TypedValue import android.view.Gravity @@ -558,6 +559,9 @@ class DasherImeService : InputMethodService() { // Output deltas are COALESCED, not applied — see flushOutputToTarget // (bug #5: OneNote/Word jumbled unbatched per-delta edits). eng.setOutputListener { type, text -> + if (type != 0 && type != 1 && type != 2) { + Log.w(TAG, "Unknown engine output event type $type (text='${text.take(32)}')") + } outputCoalescer.onEngineOutput(type, text) } eng.setMessageListener { _, msg -> @@ -578,12 +582,16 @@ class DasherImeService : InputMethodService() { * * Deletion strategy: deleteSurroundingText first; if the editor reports * it did not handle it (WebView/custom editors — precisely the - * OneNote/Word class), fall back to KEYCODE_DEL key events, which go - * through the target's own key pipeline where a delta would be dropped - * (the same principle as Dasher-Windows #17's deletion forwarding). + * OneNote/Word class), fall back to KEYCODE_DEL key events through the + * target's own key pipeline (the same principle as Dasher-Windows #17's + * deletion forwarding). DELs count CHARACTERS, not UTF-16 units + * (delKeyEventCount), and are capped: a bulk control-mode delete must + * not flood the target with hundreds of key events (jank/ANR) — beyond + * the cap the deletion is skipped with a log rather than corrupt text. * * No connection yet (field switch in flight): keep the deltas pending — - * the next frame flushes them once a field is served. + * the next frame flushes them once a field is served. (Stranded deltas + * are dropped on a fresh field start — onStartInput.) */ private fun flushOutputToTarget() { val ic = currentInputConnection ?: return @@ -592,9 +600,18 @@ class DasherImeService : InputMethodService() { try { if (flush.deleteUnits > 0) { if (!ic.deleteSurroundingText(flush.deleteUnits, 0)) { - repeat(flush.deleteUnits) { - ic.sendKeyEvent(KeyEvent(KeyEvent.ACTION_DOWN, KeyEvent.KEYCODE_DEL)) - ic.sendKeyEvent(KeyEvent(KeyEvent.ACTION_UP, KeyEvent.KEYCODE_DEL)) + val dels = delKeyEventCount( + flush.deleteUnits, + ic.getTextBeforeCursor(flush.deleteUnits, 0)?.toString() + ) + if (dels == null || dels > MAX_DEL_KEY_EVENTS) { + Log.w(TAG, "DEL fallback skipped (dels=$dels, units=${flush.deleteUnits}) — " + + "no safe count or bulk delete over the ${MAX_DEL_KEY_EVENTS}-event cap") + } else { + repeat(dels) { + ic.sendKeyEvent(KeyEvent(KeyEvent.ACTION_DOWN, KeyEvent.KEYCODE_DEL)) + ic.sendKeyEvent(KeyEvent(KeyEvent.ACTION_UP, KeyEvent.KEYCODE_DEL)) + } } } } @@ -633,14 +650,26 @@ class DasherImeService : InputMethodService() { * nobody asked. We serve motor-impaired AAC users who always want the * soft keyboard, so ask ourselves on every fresh focus of a real text * field (TYPE_NULL fields are non-editing views and stay untouched). + * + * Deliberate consequence (review M1): the Hide button dismisses the + * keyboard only until the next field focus — on hardware-keyboard devices + * nothing else would ever bring it back, and "keyboard follows focus" is + * the accessibility stance this IME already takes. Hide still works + * within a field (re-focuses of the same field arrive as restarting=true). */ override fun onStartInput(info: EditorInfo?, restarting: Boolean) { super.onStartInput(info, restarting) - if (!restarting && info != null && - info.inputType != android.text.InputType.TYPE_NULL && - !isInputViewShown - ) { - requestShowSelf(0) + if (!restarting) { + // Fresh target field: any deltas still pending from the previous + // field must NEVER land here (review I2 — for an AAC user, text + // appearing in the wrong app's field is a trust failure). + outputCoalescer.clear() + if (info != null && + info.inputType != InputType.TYPE_NULL && + !isInputViewShown + ) { + requestShowSelf(0) + } } } @@ -692,5 +721,13 @@ class DasherImeService : InputMethodService() { const val TAG = "DasherImeService" private val MATCH_PARENT = ViewGroup.LayoutParams.MATCH_PARENT private val WRAP_CONTENT = ViewGroup.LayoutParams.WRAP_CONTENT + + /** + * Upper bound on KEYCODE_DEL fallback events per flush (review C2): + * each is a key event the target must process — a bulk control-mode + * delete beyond this cap is skipped with a log (engine/field desync + * is recoverable; an ANR or an editor dropping half the flood is not). + */ + const val MAX_DEL_KEY_EVENTS = 64 } } diff --git a/app/src/main/java/at/dasher/android/ImeOutputCoalescer.kt b/app/src/main/java/at/dasher/android/ImeOutputCoalescer.kt index 59daef0..88cb868 100644 --- a/app/src/main/java/at/dasher/android/ImeOutputCoalescer.kt +++ b/app/src/main/java/at/dasher/android/ImeOutputCoalescer.kt @@ -6,20 +6,36 @@ package at.dasher.android * OneNote and Word, though it works perfectly fine inside AquaMail"). * * Dasher emits text as a stream of small deltas — one commit per node entry, - * one delete per backspace-class control node. Applied delta-by-delta, each - * op is a separate binder call that the TARGET app applies on its own - * pipeline. Editors with synchronous InputConnections (AquaMail's EditText) - * apply them in order and everything works. OneNote and Word route edits - * through their own async editing engines (undo coalescing, autosave), - * where rapid unbatched deltas can land out of order or partially — the - * field ends up jumbled. (Android's version of the problem Dasher-Windows - * #17 fixed by forwarding deletions through the target's own input path.) + * one delete per deletion. Applied delta-by-delta, each op is a separate + * binder call that the TARGET app applies on its own pipeline. Editors with + * synchronous InputConnections (AquaMail's EditText) apply them in order and + * everything works. OneNote and Word route edits through their own async + * editing engines (undo coalescing, autosave), where rapid unbatched deltas + * can land out of order or partially — the field ends up jumbled. (Android's + * version of the problem Dasher-Windows #17 fixed by forwarding deletions + * through the target's own input path.) * * The IME feeds every engine output event here as it fires during * dasher_frame(), then flushes ONCE after the frame inside a * beginBatchEdit/endBatchEdit pair: the editor sees a single atomic * "delete N, insert S" step instead of a burst of unrelated calls. * + * Delete-event contract (see DasherCore src/CAPI.cpp) and its limits: + * type-1 events come from two engine paths. `editDelete` is a true + * backspace — it always removes the buffer TAIL, which is what the split + * arithmetic below assumes. `ctrlDelete` (control-mode delete nodes, if the + * user enables them) removes a range AT the cursor — possibly mid-buffer, + * possibly forwards — but the event carries only the removed string, with + * no position or direction, so no frontend can express it faithfully + * (deleteSurroundingText only deletes backwards from the caret). Those are + * approximated as tail deletes — exactly what the pre-batching code did + * per-event, so no regression; proper fix needs the event contract to grow + * position/direction (DasherCore issue filed). + * + * Type-2 (buffer clear) is ignored, and that is only safe because the flush + * runs at the end of every dasher_frame() while resets can only run between + * frames on the same thread — pending state is always empty at a reset. + * * Pure logic — no InputConnection, so the delta arithmetic (including the * surrogate-pair and boundary-spanning cases) is unit-testable, following * the EditorSync.kt pattern. @@ -37,19 +53,20 @@ class ImeOutputCoalescer { /** * Engine output event (dasher_output_callback semantics from dasher.h): - * 0 = commit `text`; 1 = delete `text` (a backspace — always removes the - * buffer TAIL); 2 = buffer clear (dasher_reset / alphabet change — no - * delta can express it; the IME re-anchors the engine separately, and - * committed text already belongs to the target field). + * 0 = commit `text`; 1 = delete `text` (see the class doc for the + * tail-delete assumption and its control-mode limits); 2 = buffer clear + * (dasher_reset / alphabet change — no delta can express it; the IME + * re-anchors the engine separately, and committed text already belongs + * to the target field). */ fun onEngineOutput(type: Int, text: String) { when (type) { 0 -> pendingInsert.append(text) 1 -> { if (text.isEmpty()) return - // The delete removes the engine buffer's tail. That tail is - // [text already flushed to the field][pending insert]. Split - // the deleted length across that boundary: + // A tail delete removes the engine buffer's tail. That tail + // is [text already flushed to the field][pending insert]. + // Split the deleted length across that boundary: if (text.length <= pendingInsert.length) { // Entirely inside this frame's pending insert — those // chars never reached the field; just cancel them. @@ -62,7 +79,7 @@ class ImeOutputCoalescer { pendingInsert.setLength(0) } } - else -> {} // type 2 (buffer clear) — nothing to apply + else -> {} // type 2 (buffer clear) — nothing to apply; see class doc } } @@ -78,4 +95,35 @@ class ImeOutputCoalescer { pendingInsert.setLength(0) return flush } + + /** Drop any pending deltas (e.g. on a field switch — see onStartInput). */ + fun clear() { + pendingDeleteUnits = 0 + pendingInsert.setLength(0) + } +} + +/** + * How many KEYCODE_DEL events delete the same text as + * `deleteSurroundingText(units, 0)` — the fallback path for editors that + * refuse delta deletions (bug #5 review: OneNote/Word class). + * + * One DEL deletes one CHARACTER (a codepoint), not one UTF-16 unit: a + * surrogate pair (e.g. any emoji — this IME ships an Emoji alphabet) is two + * units but ONE DEL. Sending `units` DELs would over-delete into the + * preceding character, so the count is derived from the actual text before + * the caret. + * + * @param units UTF-16 units the engine wants deleted + * @param beforeCursor the field text actually before the caret, at least + * `units` long from a cooperative editor — shorter text is clamped (the + * editor truncated the read; deleting what it reported is the best move) + * @return the DEL count, or null when no safe count exists (editor returned + * no text — connection effectively dead) + */ +fun delKeyEventCount(units: Int, beforeCursor: String?): Int? { + if (units <= 0) return 0 + if (beforeCursor == null) return null + val from = (beforeCursor.length - units).coerceAtLeast(0) + return Character.codePointCount(beforeCursor, from, beforeCursor.length) } diff --git a/app/src/test/java/at/dasher/android/ImeOutputCoalescerTest.kt b/app/src/test/java/at/dasher/android/ImeOutputCoalescerTest.kt index d6f18a7..a79d173 100644 --- a/app/src/test/java/at/dasher/android/ImeOutputCoalescerTest.kt +++ b/app/src/test/java/at/dasher/android/ImeOutputCoalescerTest.kt @@ -96,4 +96,71 @@ class ImeOutputCoalescerTest { c.onEngineOutput(1, "") assertNull(c.takeFlush()) } + + @Test + fun separate_deletes_accumulate() { + val c = ImeOutputCoalescer() + c.onEngineOutput(1, "a") + c.onEngineOutput(1, "b") + assertEquals(ImeOutputCoalescer.Flush(2, ""), c.takeFlush()) + } + + @Test + fun interleaved_delete_commit_delete_spanning() { + // Field flushed "hello" last frame. This frame: engine deletes "lo", + // commits "p", then deletes "op" — the last delete spans the pending + // "p" (1 unit) and 1 flushed unit. Net: delete 2, insert nothing. + val c = ImeOutputCoalescer() + c.onEngineOutput(1, "lo") // D=2 + c.onEngineOutput(0, "p") // P="p" + c.onEngineOutput(1, "op") // spans: D += 2-1, P="" + assertEquals(ImeOutputCoalescer.Flush(3, ""), c.takeFlush()) + } + + @Test + fun clear_drops_pending_deltas() { + // Review I2: a field switch must never deliver the previous field's + // deltas to the new one. + val c = ImeOutputCoalescer() + c.onEngineOutput(0, "orphan") + c.onEngineOutput(1, "x") + c.clear() + assertNull(c.takeFlush()) + } + + // ── delKeyEventCount: DEL events count CHARACTERS, not UTF-16 units ── + + @Test + fun del_count_for_plain_text_matches_units() { + assertEquals(3, delKeyEventCount(3, "abcde")) + } + + @Test + fun del_count_collapses_surrogate_pairs() { + // Deleting "a😀" = 3 UTF-16 units but 2 CHARACTERS — sending 3 DELs + // would eat the character before the emoji (review C1). + assertEquals(2, delKeyEventCount(3, "xya😀")) + } + + @Test + fun del_count_all_emoji_is_half_the_units() { + assertEquals(2, delKeyEventCount(4, "ab😀😀")) + } + + @Test + fun del_count_null_before_cursor_is_unknown() { + // Editor returned no text — no safe count exists. + assertNull(delKeyEventCount(2, null)) + } + + @Test + fun del_count_clamps_when_editor_returned_less_text() { + // Editor truncated the read — delete what it reported, char-wise. + assertEquals(2, delKeyEventCount(5, "ab")) + } + + @Test + fun del_count_zero_units_is_zero() { + assertEquals(0, delKeyEventCount(0, "anything")) + } } From 03b6acfcd4739f4ae9e6fee8115c68528bdf8d2a Mon Sep 17 00:00:00 2001 From: will wade Date: Thu, 15 Oct 2026 15:49:37 +0100 Subject: [PATCH 3/3] =?UTF-8?q?fix(ime):=20review=20loop=202=20polish=20?= =?UTF-8?q?=E2=80=94=20buffer-clear=20drops=20pending,=20skip=20recovery,?= =?UTF-8?q?=20caret-relative=20docs,=20cap=20decision=20extracted?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Final polish from the #71 second review (8/10): - I-1: type-2 (buffer clear) now clears pending deltas — dasher.h says subscribers must 'clear your copy', and the pending state IS a shadow copy. The old comment claimed an 'always empty at reset' invariant the code itself violated (stranded null-connection flushes + seedBuffer's type-2 on every toolbar reanchor). Test added. - M-1: a skipped DEL fallback (no-text or over-cap) now re-anchors the engine from the field so the desync cannot compound on later deletes. - M-2: editDelete doc corrected — the real invariant is caret-relative ('N units before the caret' = deleteSurroundingText semantics); mid-buffer-cursor after ctrlMove joins ctrlDelete in the documented approximation bucket. - M-3: delKeyEventCount mid-surrogate-pair boundary pinned as decided desync behavior (orphaned low surrogate counts as one codepoint). - M-4: cap decision extracted to pure delFallbackPlan (Send/SkipNoText/ SkipOverCap) with boundary tests at the cap. - M-5: docs cite DasherCore#100/#101. Signed-off-by: will wade --- .../at/dasher/android/DasherImeService.kt | 33 +++++++---- .../at/dasher/android/ImeOutputCoalescer.kt | 59 ++++++++++++++----- .../dasher/android/ImeOutputCoalescerTest.kt | 46 ++++++++++++++- 3 files changed, 110 insertions(+), 28 deletions(-) diff --git a/app/src/main/java/at/dasher/android/DasherImeService.kt b/app/src/main/java/at/dasher/android/DasherImeService.kt index e91103b..6cadedb 100644 --- a/app/src/main/java/at/dasher/android/DasherImeService.kt +++ b/app/src/main/java/at/dasher/android/DasherImeService.kt @@ -585,9 +585,10 @@ class DasherImeService : InputMethodService() { * OneNote/Word class), fall back to KEYCODE_DEL key events through the * target's own key pipeline (the same principle as Dasher-Windows #17's * deletion forwarding). DELs count CHARACTERS, not UTF-16 units - * (delKeyEventCount), and are capped: a bulk control-mode delete must - * not flood the target with hundreds of key events (jank/ANR) — beyond - * the cap the deletion is skipped with a log rather than corrupt text. + * (delKeyEventCount), and are capped (delFallbackPlan): a bulk + * control-mode delete must not flood the target with hundreds of key + * events (jank/ANR). A skipped deletion re-anchors the engine from the + * field (reanchorEngineToTarget) so the mismatch doesn't compound. * * No connection yet (field switch in flight): keep the deltas pending — * the next frame flushes them once a field is served. (Stranded deltas @@ -596,6 +597,7 @@ class DasherImeService : InputMethodService() { private fun flushOutputToTarget() { val ic = currentInputConnection ?: return val flush = outputCoalescer.takeFlush() ?: return + var skippedDeletion = false ic.beginBatchEdit() try { if (flush.deleteUnits > 0) { @@ -604,21 +606,30 @@ class DasherImeService : InputMethodService() { flush.deleteUnits, ic.getTextBeforeCursor(flush.deleteUnits, 0)?.toString() ) - if (dels == null || dels > MAX_DEL_KEY_EVENTS) { - Log.w(TAG, "DEL fallback skipped (dels=$dels, units=${flush.deleteUnits}) — " + - "no safe count or bulk delete over the ${MAX_DEL_KEY_EVENTS}-event cap") - } else { - repeat(dels) { - ic.sendKeyEvent(KeyEvent(KeyEvent.ACTION_DOWN, KeyEvent.KEYCODE_DEL)) - ic.sendKeyEvent(KeyEvent(KeyEvent.ACTION_UP, KeyEvent.KEYCODE_DEL)) - } + when (val plan = delFallbackPlan(dels, MAX_DEL_KEY_EVENTS)) { + is DelFallback.Send -> + repeat(plan.keyEvents) { + ic.sendKeyEvent(KeyEvent(KeyEvent.ACTION_DOWN, KeyEvent.KEYCODE_DEL)) + ic.sendKeyEvent(KeyEvent(KeyEvent.ACTION_UP, KeyEvent.KEYCODE_DEL)) + } + is DelFallback.SkipOverCap -> + Log.w(TAG, "DEL fallback skipped: $plan — bulk delete over the " + + "${MAX_DEL_KEY_EVENTS}-event flood cap") + DelFallback.SkipNoText -> + Log.w(TAG, "DEL fallback skipped: editor returned no text before caret") } + if (dels == null || dels > MAX_DEL_KEY_EVENTS) skippedDeletion = true } } if (flush.insert.isNotEmpty()) ic.commitText(flush.insert, 1) } finally { ic.endBatchEdit() } + // Review M-1: a skipped deletion leaves engine and field desynced — + // re-anchor from the field so the mismatch doesn't compound on every + // later delete (seedBuffer re-reads the field and fires the type-2 + // clear, dropping any stale pending state). + if (skippedDeletion) reanchorEngineToTarget() } // API 36+ (Android 16): measure the actual docked-root position in the diff --git a/app/src/main/java/at/dasher/android/ImeOutputCoalescer.kt b/app/src/main/java/at/dasher/android/ImeOutputCoalescer.kt index 88cb868..922b13c 100644 --- a/app/src/main/java/at/dasher/android/ImeOutputCoalescer.kt +++ b/app/src/main/java/at/dasher/android/ImeOutputCoalescer.kt @@ -21,20 +21,26 @@ package at.dasher.android * "delete N, insert S" step instead of a burst of unrelated calls. * * Delete-event contract (see DasherCore src/CAPI.cpp) and its limits: - * type-1 events come from two engine paths. `editDelete` is a true - * backspace — it always removes the buffer TAIL, which is what the split - * arithmetic below assumes. `ctrlDelete` (control-mode delete nodes, if the - * user enables them) removes a range AT the cursor — possibly mid-buffer, - * possibly forwards — but the event carries only the removed string, with - * no position or direction, so no frontend can express it faithfully - * (deleteSurroundingText only deletes backwards from the caret). Those are - * approximated as tail deletes — exactly what the pre-batching code did - * per-event, so no regression; proper fix needs the event contract to grow - * position/direction (DasherCore issue filed). + * type-1 events come from two engine paths. `editDelete` is a + * caret-relative backspace — it erases N units immediately BEFORE + * cursorPos, which is exactly `deleteSurroundingText(N, 0)` semantics + * (mid-buffer only after a control-mode ctrlMove moved the cursor — see + * below). `ctrlDelete` (control-mode delete nodes, if the user enables + * them) removes a range AT the cursor — possibly spanning it, possibly + * forwards — but the event carries only the removed string, with no + * position or direction, so no frontend can express it faithfully. Both + * non-caret-relative cases are approximated as caret deletes — exactly + * what the pre-batching code did per-event, so no regression; the proper + * fix needs the event contract to grow position/direction (DasherCore + * #100; #101 covers editDelete firing the event even when its guard + * rejected the erase). * - * Type-2 (buffer clear) is ignored, and that is only safe because the flush - * runs at the end of every dasher_frame() while resets can only run between - * frames on the same thread — pending state is always empty at a reset. + * Type-2 (buffer clear) DROPS pending deltas — dasher.h says subscribers + * "must treat this as 'clear your copy'", and the pending state is exactly a + * shadow copy of the engine buffer. This matters because the flush can strand + * deltas when the connection is momentarily null (WebView focus churn); a + * later reset (dasher_seed_buffer fires type-2 on every toolbar reanchor — + * CAPI_edit.cpp) must not let those stale deltas apply to post-reset context. * * Pure logic — no InputConnection, so the delta arithmetic (including the * surrogate-pair and boundary-spanning cases) is unit-testable, following @@ -79,7 +85,7 @@ class ImeOutputCoalescer { pendingInsert.setLength(0) } } - else -> {} // type 2 (buffer clear) — nothing to apply; see class doc + else -> clear() // type 2 (buffer clear) — drop the shadow copy; see class doc } } @@ -114,6 +120,11 @@ class ImeOutputCoalescer { * preceding character, so the count is derived from the actual text before * the caret. * + * Boundary note (decided behavior, review M-3): if the range start lands + * INSIDE a surrogate pair (only possible when engine and field are already + * desynced — this is a fallback of a fallback), the orphaned low surrogate + * counts as one codepoint. Pinned by test. + * * @param units UTF-16 units the engine wants deleted * @param beforeCursor the field text actually before the caret, at least * `units` long from a cooperative editor — shorter text is clamped (the @@ -127,3 +138,23 @@ fun delKeyEventCount(units: Int, beforeCursor: String?): Int? { val from = (beforeCursor.length - units).coerceAtLeast(0) return Character.codePointCount(beforeCursor, from, beforeCursor.length) } + +/** + * The DEL-fallback decision for one flush (review M-4 — extracted so the + * cap boundary is unit-testable, not buried in the service). + * [DelFallback.Send] means `keyEvents` DOWN/UP pairs (i.e. 2×keyEvents + * `sendKeyEvent` calls) inside the batch edit. + */ +sealed interface DelFallback { + data class Send(val keyEvents: Int) : DelFallback + /** Editor returned no text before the caret — no safe count exists. */ + object SkipNoText : DelFallback + /** Bulk delete over the flood cap — skipped to avoid jank/ANR. */ + data class SkipOverCap(val requested: Int, val cap: Int) : DelFallback +} + +fun delFallbackPlan(dels: Int?, cap: Int): DelFallback = when { + dels == null -> DelFallback.SkipNoText + dels > cap -> DelFallback.SkipOverCap(dels, cap) + else -> DelFallback.Send(dels) +} diff --git a/app/src/test/java/at/dasher/android/ImeOutputCoalescerTest.kt b/app/src/test/java/at/dasher/android/ImeOutputCoalescerTest.kt index a79d173..801e3c4 100644 --- a/app/src/test/java/at/dasher/android/ImeOutputCoalescerTest.kt +++ b/app/src/test/java/at/dasher/android/ImeOutputCoalescerTest.kt @@ -73,14 +73,25 @@ class ImeOutputCoalescerTest { @Test fun buffer_clear_emits_nothing() { - // dasher.h type 2: buffer clear carries empty text — the deltas - // cannot express it and the IME re-anchors instead. It must not - // poison pending state either. + // dasher.h type 2: buffer clear carries empty text — no delta applies. val c = ImeOutputCoalescer() c.onEngineOutput(2, "") assertNull(c.takeFlush()) } + @Test + fun buffer_clear_drops_pending_deltas() { + // Review I-1: pending state is a shadow copy of the engine buffer; + // dasher.h says subscribers must "clear your copy" on event 2. A + // reset after a stranded (null-connection) flush must never let + // stale deltas apply to post-reset context. + val c = ImeOutputCoalescer() + c.onEngineOutput(0, "stale") + c.onEngineOutput(1, "x") + c.onEngineOutput(2, "") + assertNull(c.takeFlush()) + } + @Test fun surrogate_pairs_count_two_units() { // deleteSurroundingText counts UTF-16 code units: an emoji is 2 — @@ -163,4 +174,33 @@ class ImeOutputCoalescerTest { fun del_count_zero_units_is_zero() { assertEquals(0, delKeyEventCount(0, "anything")) } + + @Test + fun del_count_mid_surrogate_pair_is_pinned_desync_behavior() { + // Review M-3: "a😀b" (4 units). units=2 → range starts at the LOW + // surrogate of the pair — only possible when engine and field are + // already desynced. The orphaned low surrogate counts as one + // codepoint + 'b' = 2. Decided behavior, not an accident. + assertEquals(2, delKeyEventCount(2, "a😀b")) + } + + // ── delFallbackPlan: the cap decision is pure and boundary-tested ── + + @Test + fun fallback_plan_at_cap_sends() { + assertEquals(DelFallback.Send(64), delFallbackPlan(64, 64)) + } + + @Test + fun fallback_plan_over_cap_skips() { + assertEquals( + DelFallback.SkipOverCap(65, 64), + delFallbackPlan(65, 64) + ) + } + + @Test + fun fallback_plan_null_is_skip_no_text() { + assertEquals(DelFallback.SkipNoText, delFallbackPlan(null, 64)) + } }