diff --git a/app/src/main/java/at/dasher/android/DasherImeService.kt b/app/src/main/java/at/dasher/android/DasherImeService.kt index 8966eb2..6cadedb 100644 --- a/app/src/main/java/at/dasher/android/DasherImeService.kt +++ b/app/src/main/java/at/dasher/android/DasherImeService.kt @@ -11,9 +11,11 @@ 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 +import android.view.KeyEvent import android.view.MotionEvent import android.view.View import android.view.ViewGroup @@ -34,6 +36,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 +432,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 +556,13 @@ 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) + 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 -> Toast.makeText(this, msg, Toast.LENGTH_SHORT).show() @@ -561,6 +574,64 @@ 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 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 (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 + * are dropped on a fresh field start — onStartInput.) + */ + private fun flushOutputToTarget() { + val ic = currentInputConnection ?: return + val flush = outputCoalescer.takeFlush() ?: return + var skippedDeletion = false + ic.beginBatchEdit() + try { + if (flush.deleteUnits > 0) { + if (!ic.deleteSurroundingText(flush.deleteUnits, 0)) { + val dels = delKeyEventCount( + flush.deleteUnits, + ic.getTextBeforeCursor(flush.deleteUnits, 0)?.toString() + ) + 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 // 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 +652,38 @@ 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). + * + * 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) { + // 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) + } + } + } + override fun onStartInputView(info: EditorInfo?, restarting: Boolean) { super.onStartInputView(info, restarting) // Re-assert ALL IME-owned static listeners — the main app may have @@ -629,5 +732,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 new file mode 100644 index 0000000..922b13c --- /dev/null +++ b/app/src/main/java/at/dasher/android/ImeOutputCoalescer.kt @@ -0,0 +1,160 @@ +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 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 + * 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) 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 + * 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` (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 + // 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. + 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 -> clear() // type 2 (buffer clear) — drop the shadow copy; see class doc + } + } + + /** + * 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 + } + + /** 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. + * + * 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 + * 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) +} + +/** + * 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 new file mode 100644 index 0000000..801e3c4 --- /dev/null +++ b/app/src/test/java/at/dasher/android/ImeOutputCoalescerTest.kt @@ -0,0 +1,206 @@ +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 — 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 — + // 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()) + } + + @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")) + } + + @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)) + } +}