From 90fb110069374bc92184a0e16590dca279c697b8 Mon Sep 17 00:00:00 2001 From: will wade Date: Thu, 29 Oct 2026 16:39:00 +0000 Subject: [PATCH 1/3] =?UTF-8?q?fix(ime):=20=E2=8C=AB=20deletes=20the=20sel?= =?UTF-8?q?ection=20=E2=80=94=20empty=20commitText=20instead=20of=20caret-?= =?UTF-8?q?only=20deleteSurroundingText?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Heide (2026-10-29): "if you select something, whether it's everything or partial, and you use the delete button on the toolbar, it doesn't do anything. It only works when you don't have anything selected." backspace() always called deleteSurroundingText(n, 0), which deletes around the CARET — with an active selection (including one made by our own Select All button) editors ignore it or no-op: the reported dead ⌫. performImeBackspace (extracted, unit-testable per the repo's pure-logic pattern): - selection present → commitText("", 1) — the canonical IME way to replace a selection with nothing; DEL key-event fallback for editors that refuse the empty commit - caret → unchanged codepoint-correct backspace (surrogate-aware) Six unit tests with a fake InputConnection pin the contract: selection deleted via empty commit; refusing editor gets the DEL fallback; empty selection falls through; caret/surrogate/empty-field cases preserved. Signed-off-by: will wade --- .../java/at/dasher/android/EditingToolbar.kt | 53 ++++++--- .../at/dasher/android/ImeBackspaceTest.kt | 109 ++++++++++++++++++ 2 files changed, 145 insertions(+), 17 deletions(-) create mode 100644 app/src/test/java/at/dasher/android/ImeBackspaceTest.kt diff --git a/app/src/main/java/at/dasher/android/EditingToolbar.kt b/app/src/main/java/at/dasher/android/EditingToolbar.kt index 8d8a623..285e4bc 100644 --- a/app/src/main/java/at/dasher/android/EditingToolbar.kt +++ b/app/src/main/java/at/dasher/android/EditingToolbar.kt @@ -54,7 +54,9 @@ class EditingToolbar( addTool(R.drawable.ic_lucide_arrow_left, "Move cursor left", mutates = true) { moveCursor(-1) } addTool(R.drawable.ic_lucide_arrow_right, "Move cursor right", mutates = true) { moveCursor(1) } addSpacer(dp(8)) - addTool(R.drawable.ic_lucide_delete, "Delete", large = true, mutates = true) { backspace() } + addTool(R.drawable.ic_lucide_delete, "Delete", large = true, mutates = true) { + inputConnection()?.let { performImeBackspace(it) } + } addSpacer(dp(8)) addTool(R.drawable.ic_lucide_text_select, "Select all", mutates = false) { performAction(android.R.id.selectAll) } addTool(R.drawable.ic_lucide_copy, "Copy", mutates = false) { performAction(android.R.id.copy) } @@ -118,22 +120,6 @@ class EditingToolbar( // ── Actions ──────────────────────────────────────────────────────────── - /** - * Delete one codepoint before the cursor (#50: Heide's #1 request). - * [InputConnection.deleteSurroundingText] expects UTF-16 code-unit - * counts; a surrogate pair (emoji, CJK beyond BMP) is 2 units. - */ - private fun backspace() { - val ic = inputConnection() ?: return - val before = ic.getTextBeforeCursor(2, 0) - val units = when { - before == null || before.isEmpty() -> return - before.length >= 2 && Character.isHighSurrogate(before[before.length - 2]) -> 2 - else -> 1 - } - ic.deleteSurroundingText(units, 0) - } - /** Move the cursor one position left/right via the target's key handler (#50). */ private fun moveCursor(direction: Int) { val ic = inputConnection() ?: return @@ -147,3 +133,36 @@ class EditingToolbar( inputConnection()?.performContextMenuAction(actionId) } } + +/** + * The toolbar ⌫, extracted so its contract is unit-testable with a fake + * [InputConnection] (the EditorSync.kt / ImeOutputCoalescer.kt pattern). + * + * Two cases (Heide, 2026-10-29: "if you select something, the delete button + * doesn't do anything — it only works when you don't have anything + * selected"): + * - SELECTION: [InputConnection.deleteSurroundingText] deletes around the + * caret and ignores (or no-ops on) an active selection — including one our + * own Select All made. Committing empty text replaces the selection with + * nothing (the canonical IME move); a DEL key event is the fallback for + * editors that refuse the empty commit. + * - CARET: one codepoint back — deleteSurroundingText with UTF-16 unit + * counts (a surrogate pair, e.g. any emoji, is 2). + */ +fun performImeBackspace(ic: InputConnection) { + val selected = ic.getSelectedText(0) + if (selected != null && selected.isNotEmpty()) { + if (!ic.commitText("", 1)) { + ic.sendKeyEvent(KeyEvent(KeyEvent.ACTION_DOWN, KeyEvent.KEYCODE_DEL)) + ic.sendKeyEvent(KeyEvent(KeyEvent.ACTION_UP, KeyEvent.KEYCODE_DEL)) + } + return + } + val before = ic.getTextBeforeCursor(2, 0) + val units = when { + before == null || before.isEmpty() -> return + before.length >= 2 && Character.isHighSurrogate(before[before.length - 2]) -> 2 + else -> 1 + } + ic.deleteSurroundingText(units, 0) +} diff --git a/app/src/test/java/at/dasher/android/ImeBackspaceTest.kt b/app/src/test/java/at/dasher/android/ImeBackspaceTest.kt new file mode 100644 index 0000000..ac604a5 --- /dev/null +++ b/app/src/test/java/at/dasher/android/ImeBackspaceTest.kt @@ -0,0 +1,109 @@ +package at.dasher.android + +import android.view.KeyEvent +import android.view.inputmethod.InputConnection +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * Heide's 2026-10-29 report: "if you select something, whether it's + * everything or partial, and you use the delete button on the toolbar, it + * doesn't do anything. It only works when you don't have anything selected." + * + * performImeBackspace must delete the SELECTION as a unit (empty commitText — + * deleteSurroundingText ignores selections) and keep the caret backspace + * behaviour (codepoint-correct, surrogate-aware) when nothing is selected. + */ +class ImeBackspaceTest { + + /** Records calls; controlled getSelectedText/getTextBeforeCursor. Pure JVM. */ + private class FakeIc( + private val selected: CharSequence? = null, + private val beforeCursor: CharSequence? = null, + private val commitAccepts: Boolean = true, + ) : InputConnection { + val calls = mutableListOf() + override fun getSelectedText(flags: Int): CharSequence? = selected + override fun getTextBeforeCursor(n: Int, flags: Int): CharSequence? = beforeCursor + override fun commitText(text: CharSequence, newCursorPosition: Int): Boolean { + calls += "commitText(${text.length})" + return commitAccepts + } + override fun deleteSurroundingText(beforeLength: Int, afterLength: Int): Boolean { + calls += "deleteSurroundingText($beforeLength)" + return true + } + override fun sendKeyEvent(event: KeyEvent): Boolean { + // KeyEvent.getAction is not mocked on the JVM — record without + // inspecting (performImeBackspace sends DOWN+UP; assert on count). + calls += "sendKeyEvent" + return true + } + // Unused in these paths + override fun getTextAfterCursor(n: Int, flags: Int): CharSequence? = null + override fun getCursorCapsMode(reqModes: Int): Int = 0 + override fun setComposingText(text: CharSequence, newCursorPosition: Int): Boolean = false + override fun setComposingRegion(start: Int, end: Int): Boolean = false + override fun finishComposingText(): Boolean = true + override fun setSelection(start: Int, end: Int): Boolean = false + override fun performEditorAction(actionCode: Int): Boolean = false + override fun performContextMenuAction(id: Int): Boolean = false + override fun beginBatchEdit(): Boolean = true + override fun endBatchEdit(): Boolean = true + override fun clearMetaKeyStates(states: Int): Boolean = true + override fun reportFullscreenMode(enabled: Boolean): Boolean = true + override fun performPrivateCommand(action: String?, data: android.os.Bundle?): Boolean = false + override fun getExtractedText(request: android.view.inputmethod.ExtractedTextRequest?, flags: Int): android.view.inputmethod.ExtractedText? = null + override fun deleteSurroundingTextInCodePoints(beforeLength: Int, afterLength: Int): Boolean = false + override fun getHandler(): android.os.Handler? = null + override fun closeConnection() {} + override fun commitContent(inputContent: android.view.inputmethod.InputContentInfo, flags: Int, opts: android.os.Bundle?): Boolean = false + override fun commitCorrection(correctionInfo: android.view.inputmethod.CorrectionInfo?): Boolean = false + override fun commitCompletion(text: android.view.inputmethod.CompletionInfo?): Boolean = false + override fun requestCursorUpdates(cursorUpdateMode: Int): Boolean = false + } + + @Test + fun selection_is_deleted_with_an_empty_commit() { + val ic = FakeIc(selected = "everything or partial") + performImeBackspace(ic) + assertEquals(listOf("commitText(0)"), ic.calls) + } + + @Test + fun selection_refusing_editor_gets_del_keyevent_fallback() { + val ic = FakeIc(selected = "sel", commitAccepts = false) + performImeBackspace(ic) + assertEquals(listOf("commitText(0)", "sendKeyEvent", "sendKeyEvent"), ic.calls) + } + + @Test + fun empty_selection_falls_through_to_caret_backspace() { + // Some editors return "" rather than null with no selection + val ic = FakeIc(selected = "", beforeCursor = "ab") + performImeBackspace(ic) + assertEquals(listOf("deleteSurroundingText(1)"), ic.calls) + } + + @Test + fun no_selection_deletes_one_unit() { + val ic = FakeIc(selected = null, beforeCursor = "ab") + performImeBackspace(ic) + assertEquals(listOf("deleteSurroundingText(1)"), ic.calls) + } + + @Test + fun surrogate_pair_before_caret_deletes_two_units() { + val ic = FakeIc(selected = null, beforeCursor = "a😀") + performImeBackspace(ic) + assertEquals(listOf("deleteSurroundingText(2)"), ic.calls) + } + + @Test + fun empty_field_is_a_noop() { + val ic = FakeIc(selected = null, beforeCursor = "") + performImeBackspace(ic) + assertTrue(ic.calls.isEmpty()) + } +} From 8fbd677da819506081fd00a6ecd1b0ae44d7bb69 Mon Sep 17 00:00:00 2001 From: will wade Date: Thu, 29 Oct 2026 16:55:26 +0000 Subject: [PATCH 2/3] =?UTF-8?q?fix(ime):=20review=20loop=201=20=E2=80=94?= =?UTF-8?q?=20verify=20the=20selection=20actually=20deleted;=20outcome=20t?= =?UTF-8?q?elemetry?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - I1 (the bug-#5 lesson): hostile editors (OneNote/Word class) return TRUE from commitText("") while doing nothing — the DEL fallback previously fired only on a false return, i.e. the failure mode real editors rarely produce. The selection path now re-verifies via getSelectedText: if the selection survived a true commit, the DEL fallback fires anyway (a lying editor that DID delete loses one char — the lesser evil vs. a dead ⌫). Test: the lying-editor case. - M4: performImeBackspace returns a sealed ImeBackspaceOutcome (SelectionDeleted/CaretDeleted/Noop); the toolbar reports state=selection|char|none in the ui_action backspace event — the distinction that motivated this fix. Schema state enum extended. - M2: the fake's getTextBeforeCursor now windows to the last n UTF-16 units (real semantics); fake selection state is now mutable so getSelectedText reflects the commit. Signed-off-by: will wade --- analytics-events.json | 229 ++++++++++++++++-- .../java/at/dasher/android/EditingToolbar.kt | 59 +++-- .../at/dasher/android/ImeBackspaceTest.kt | 26 +- 3 files changed, 269 insertions(+), 45 deletions(-) diff --git a/analytics-events.json b/analytics-events.json index 4b71d53..c0a30df 100644 --- a/analytics-events.json +++ b/analytics-events.json @@ -5,39 +5,97 @@ "default_properties": { "description": "Automatically appended to every event from every frontend", "properties": { - "platform": { "type": "string", "examples": ["windows", "macos", "ios", "visionos", "linux", "android"] }, - "app_variant": { "type": "string", "examples": ["dasher-windows", "dasher-mac", "dasher-ios", "dasher-gtk", "dasher-android"] }, - "app_version": { "type": "string", "example": "0.1.0-preview9" }, - "os_version": { "type": "string", "example": "Microsoft Windows 10.0.19045" }, - "$geoip_disable": { "type": "boolean", "const": true, "description": "Sent on every event (including crashes): PostHog Cloud must not derive location from the client IP (RFC 0001: no location data)." } + "platform": { + "type": "string", + "examples": [ + "windows", + "macos", + "ios", + "visionos", + "linux", + "android" + ] + }, + "app_variant": { + "type": "string", + "examples": [ + "dasher-windows", + "dasher-mac", + "dasher-ios", + "dasher-gtk", + "dasher-android" + ] + }, + "app_version": { + "type": "string", + "example": "0.1.0-preview9" + }, + "os_version": { + "type": "string", + "example": "Microsoft Windows 10.0.19045" + }, + "$geoip_disable": { + "type": "boolean", + "const": true, + "description": "Sent on every event (including crashes): PostHog Cloud must not derive location from the client IP (RFC 0001: no location data)." + } } }, "events": { "app_launched": { "description": "App starts (after opt-in)", "properties": { - "locale": { "type": "string", "example": "en-GB" } + "locale": { + "type": "string", + "example": "en-GB" + } }, "pii": false }, "input_method_changed": { "description": "User selects input method", "properties": { - "method": { "type": "string", "enum": ["pointer", "touch", "switch", "joystick", "eye_gaze", "tilt"] } + "method": { + "type": "string", + "enum": [ + "pointer", + "touch", + "switch", + "joystick", + "eye_gaze", + "tilt" + ] + } }, "pii": false }, "alphabet_selected": { "description": "User picks alphabet", "properties": { - "alphabet_id": { "type": "string", "example": "English with limited punctuation" } + "alphabet_id": { + "type": "string", + "example": "English with limited punctuation" + } }, "pii": false }, "settings_viewed": { "description": "User opens a settings tab", "properties": { - "tab_name": { "type": "string", "enum": ["Input", "Language", "Customization", "Speed", "Output", "Speech", "Privacy", "Appearance", "Advanced"] } + "tab_name": { + "type": "string", + "enum": [ + "Input", + "Language", + "Customization", + "Speed", + "Output", + "Speech", + "Privacy", + "Appearance", + "Advanced" + ] + } }, "pii": false }, @@ -54,59 +112,178 @@ "crash": { "description": "Uncaught exception", "properties": { - "stack_trace": { "type": "string", "note": "May contain file paths but not user text" } + "stack_trace": { + "type": "string", + "note": "May contain file paths but not user text" + } }, "pii": false }, "ime_mode_restore": { "description": "The IME decided which mode (docked/floating) to open in — every input-view creation. Android only. Decision inputs for the dock/float lifecycle debugging (Dasher-Android #73/#75).", "properties": { - "saved_mode": { "type": "string", "enum": ["docked", "floating", ""] }, - "always_docked": { "type": "boolean", "description": "The Settings → Input escape hatch is enabled" }, - "floating_before": { "type": "boolean", "description": "Stale floating state present BEFORE self-healing (true = the #73 path)" }, - "restored_floating": { "type": "boolean", "description": "The decision: the keyboard opened floating" }, - "overlay_perm": { "type": "boolean", "description": "SYSTEM_ALERT_WINDOW permission granted" } + "saved_mode": { + "type": "string", + "enum": [ + "docked", + "floating", + "" + ] + }, + "always_docked": { + "type": "boolean", + "description": "The Settings → Input escape hatch is enabled" + }, + "floating_before": { + "type": "boolean", + "description": "Stale floating state present BEFORE self-healing (true = the #73 path)" + }, + "restored_floating": { + "type": "boolean", + "description": "The decision: the keyboard opened floating" + }, + "overlay_perm": { + "type": "boolean", + "description": "SYSTEM_ALERT_WINDOW permission granted" + } }, "pii": false }, "ime_float_enter": { "description": "The IME entered floating mode. Android only.", "properties": { - "reason": { "type": "string", "enum": ["button", "restore"], "description": "button = explicit user tap; restore = saved-preference restore on keyboard show" } + "reason": { + "type": "string", + "enum": [ + "button", + "restore" + ], + "description": "button = explicit user tap; restore = saved-preference restore on keyboard show" + } }, "pii": false }, "ime_float_exit": { "description": "The IME left floating mode. Android only.", "properties": { - "persist": { "type": "boolean", "description": "false = system-driven cleanup, the saved preference is unchanged" }, - "reason": { "type": "string", "enum": ["button", "selfheal", "unbind", "destroy"], "description": "button = user tapped Dock; selfheal = stale-state cleanup at show; unbind = input binding went away (IME switch); destroy = the IME service was destroyed while floating" } + "persist": { + "type": "boolean", + "description": "false = system-driven cleanup, the saved preference is unchanged" + }, + "reason": { + "type": "string", + "enum": [ + "button", + "selfheal", + "unbind", + "destroy" + ], + "description": "button = user tapped Dock; selfheal = stale-state cleanup at show; unbind = input binding went away (IME switch); destroy = the IME service was destroyed while floating" + } }, "pii": false }, "ime_always_docked_changed": { "description": "User toggled Settings → Input → 'Keyboard always starts docked'. Android only. Adoption of this escape hatch is the signal for whether last-mode restore is the right default.", "properties": { - "enabled": { "type": "boolean" } + "enabled": { + "type": "boolean" + } }, "pii": false }, "ui_action": { "description": "User activated a control in the main UI (outside settings windows) — RFC 0001 2026-10-28 amendment. Answers 'where should main-UI focus go'.", "properties": { - "surface": { "type": "string", "enum": ["main_toolbar", "status_bar", "ime_toolbar", "ime_topbar"] }, - "action": { "type": "string", "description": "Fixed vocabulary; new identifiers require a schema update", "examples": ["new", "open", "save", "pause_play", "game_mode", "settings", "copy", "share", "emoji_toggle", "speed_up", "speed_down", "layout", "float", "dock", "hide", "cursor_left", "cursor_right", "backspace", "select_all", "paste"] }, - "state": { "type": "string", "enum": ["on", "off", "paused", "playing", "text", "emoji"], "description": "Optional — for toggles" } + "surface": { + "type": "string", + "enum": [ + "main_toolbar", + "status_bar", + "ime_toolbar", + "ime_topbar" + ] + }, + "action": { + "type": "string", + "description": "Fixed vocabulary; new identifiers require a schema update", + "examples": [ + "new", + "open", + "save", + "pause_play", + "game_mode", + "settings", + "copy", + "share", + "emoji_toggle", + "speed_up", + "speed_down", + "layout", + "float", + "dock", + "hide", + "cursor_left", + "cursor_right", + "backspace", + "select_all", + "paste" + ] + }, + "state": { + "type": "string", + "enum": [ + "on", + "off", + "paused", + "playing", + "text", + "emoji", + "selection", + "char", + "none" + ], + "description": "Optional — for toggles" + } }, "pii": false }, "setting_changed": { "description": "User committed a settings change — RFC 0001 2026-10-28 amendment. Answers 'which settings are the priority'. Fires at the widget commit site only (never for programmatic engine changes).", "properties": { - "setting_id": { "type": "string", "description": "Canonical identifier: DasherCore parameter name or frontend setting key", "examples": ["Max Bitrate", "output_font_size"] }, - "value_type": { "type": "string", "enum": ["bool", "number", "enum", "string"] }, - "value": { "type": ["boolean", "number", "string"], "description": "Present ONLY for bool/number/enum (fixed vocabularies). NEVER for string settings — free text is not collected" }, - "surface": { "type": "string", "enum": ["settings", "main_ui", "ime"] } + "setting_id": { + "type": "string", + "description": "Canonical identifier: DasherCore parameter name or frontend setting key", + "examples": [ + "Max Bitrate", + "output_font_size" + ] + }, + "value_type": { + "type": "string", + "enum": [ + "bool", + "number", + "enum", + "string" + ] + }, + "value": { + "type": [ + "boolean", + "number", + "string" + ], + "description": "Present ONLY for bool/number/enum (fixed vocabularies). NEVER for string settings — free text is not collected" + }, + "surface": { + "type": "string", + "enum": [ + "settings", + "main_ui", + "ime" + ] + } }, "pii": false } diff --git a/app/src/main/java/at/dasher/android/EditingToolbar.kt b/app/src/main/java/at/dasher/android/EditingToolbar.kt index 285e4bc..0efa406 100644 --- a/app/src/main/java/at/dasher/android/EditingToolbar.kt +++ b/app/src/main/java/at/dasher/android/EditingToolbar.kt @@ -54,8 +54,16 @@ class EditingToolbar( addTool(R.drawable.ic_lucide_arrow_left, "Move cursor left", mutates = true) { moveCursor(-1) } addTool(R.drawable.ic_lucide_arrow_right, "Move cursor right", mutates = true) { moveCursor(1) } addSpacer(dp(8)) - addTool(R.drawable.ic_lucide_delete, "Delete", large = true, mutates = true) { - inputConnection()?.let { performImeBackspace(it) } + addTool(R.drawable.ic_lucide_delete, "Delete", large = true, mutates = true, logTelemetry = false) { + val ic = inputConnection() ?: return@addTool + // Telemetry distinguishes selection-delete from caret backspace + // (review M4 — the distinction that motivated this fix). + val state = when (performImeBackspace(ic)) { + ImeBackspaceOutcome.SelectionDeleted -> "selection" + ImeBackspaceOutcome.CaretDeleted -> "char" + ImeBackspaceOutcome.Noop -> "none" + } + UiTelemetry.action("ime_toolbar", "backspace", state) } addSpacer(dp(8)) addTool(R.drawable.ic_lucide_text_select, "Select all", mutates = false) { performAction(android.R.id.selectAll) } @@ -90,12 +98,15 @@ class EditingToolbar( } private fun addTool(iconRes: Int, tooltip: String, - large: Boolean = false, mutates: Boolean = true, action: () -> Unit) { + large: Boolean = false, mutates: Boolean = true, + logTelemetry: Boolean = true, action: () -> Unit) { addView(toolButton(iconRes, tooltip, large).apply { setOnClickListener { - // RFC 0001 amendment: which IME editing tools are actually - // used (Heide's SwiftKey round-trips suggested these matter). - UiTelemetry.action("ime_toolbar", actionIdFor(tooltip)) + if (logTelemetry) { + // RFC 0001 amendment: which IME editing tools are actually + // used (Heide's SwiftKey round-trips suggested these matter). + UiTelemetry.action("ime_toolbar", actionIdFor(tooltip)) + } action() if (mutates) onBufferChanged() } @@ -141,28 +152,46 @@ class EditingToolbar( * Two cases (Heide, 2026-10-29: "if you select something, the delete button * doesn't do anything — it only works when you don't have anything * selected"): - * - SELECTION: [InputConnection.deleteSurroundingText] deletes around the - * caret and ignores (or no-ops on) an active selection — including one our - * own Select All made. Committing empty text replaces the selection with - * nothing (the canonical IME move); a DEL key event is the fallback for - * editors that refuse the empty commit. + * - SELECTION: [InputConnection.deleteSurroundingText] cannot affect a + * selection (it deletes BEFORE the selection start — a precise no-op when + * the selection starts at the field beginning, i.e. Select All). + * commitText("") replaces the selection with nothing. Review loop 1 (I1): + * the editors documented hostile in bug #5 return TRUE while doing + * nothing, so a true commit is re-verified — if the selection survived, + * the DEL key-event fallback fires anyway (at worst a lying editor that + * DID delete gets one extra char — the lesser evil vs. a dead ⌫). * - CARET: one codepoint back — deleteSurroundingText with UTF-16 unit * counts (a surrogate pair, e.g. any emoji, is 2). + * + * @return the outcome, for telemetry (selection vs caret distinction). */ -fun performImeBackspace(ic: InputConnection) { +sealed interface ImeBackspaceOutcome { + /** The selection (if any) was deleted. */ + object SelectionDeleted : ImeBackspaceOutcome + /** One codepoint before the caret was deleted. */ + object CaretDeleted : ImeBackspaceOutcome + /** Nothing to delete (empty selection + empty before-cursor). */ + object Noop : ImeBackspaceOutcome +} + +fun performImeBackspace(ic: InputConnection): ImeBackspaceOutcome { val selected = ic.getSelectedText(0) if (selected != null && selected.isNotEmpty()) { - if (!ic.commitText("", 1)) { + val committed = ic.commitText("", 1) + // Bug #5's lesson: a `true` return does not mean it happened. + val survived = ic.getSelectedText(0) + if (!committed || (survived != null && survived.isNotEmpty())) { ic.sendKeyEvent(KeyEvent(KeyEvent.ACTION_DOWN, KeyEvent.KEYCODE_DEL)) ic.sendKeyEvent(KeyEvent(KeyEvent.ACTION_UP, KeyEvent.KEYCODE_DEL)) } - return + return ImeBackspaceOutcome.SelectionDeleted } val before = ic.getTextBeforeCursor(2, 0) val units = when { - before == null || before.isEmpty() -> return + before == null || before.isEmpty() -> return ImeBackspaceOutcome.Noop before.length >= 2 && Character.isHighSurrogate(before[before.length - 2]) -> 2 else -> 1 } ic.deleteSurroundingText(units, 0) + return ImeBackspaceOutcome.CaretDeleted } diff --git a/app/src/test/java/at/dasher/android/ImeBackspaceTest.kt b/app/src/test/java/at/dasher/android/ImeBackspaceTest.kt index ac604a5..b6d1404 100644 --- a/app/src/test/java/at/dasher/android/ImeBackspaceTest.kt +++ b/app/src/test/java/at/dasher/android/ImeBackspaceTest.kt @@ -22,14 +22,22 @@ class ImeBackspaceTest { private val selected: CharSequence? = null, private val beforeCursor: CharSequence? = null, private val commitAccepts: Boolean = true, + /** Bug-#5 editor class: commitText returns true AND the selection + * survives (the async-editor lie the review loop flagged). */ + private val selectionSurvivesCommit: Boolean = false, ) : InputConnection { val calls = mutableListOf() - override fun getSelectedText(flags: Int): CharSequence? = selected - override fun getTextBeforeCursor(n: Int, flags: Int): CharSequence? = beforeCursor + private var committed = false + override fun getSelectedText(flags: Int): CharSequence? = + if (committed) { if (selectionSurvivesCommit) selected else null } else selected + // Real windowing semantics (review M2): the last n UTF-16 units + override fun getTextBeforeCursor(n: Int, flags: Int): CharSequence? = beforeCursor?.takeLast(n) override fun commitText(text: CharSequence, newCursorPosition: Int): Boolean { calls += "commitText(${text.length})" + committed = true return commitAccepts } + override fun deleteSurroundingText(beforeLength: Int, afterLength: Int): Boolean { calls += "deleteSurroundingText($beforeLength)" return true @@ -67,14 +75,24 @@ class ImeBackspaceTest { @Test fun selection_is_deleted_with_an_empty_commit() { val ic = FakeIc(selected = "everything or partial") - performImeBackspace(ic) + val outcome = performImeBackspace(ic) + assertEquals(ImeBackspaceOutcome.SelectionDeleted, outcome) assertEquals(listOf("commitText(0)"), ic.calls) } @Test fun selection_refusing_editor_gets_del_keyevent_fallback() { val ic = FakeIc(selected = "sel", commitAccepts = false) - performImeBackspace(ic) + assertEquals(ImeBackspaceOutcome.SelectionDeleted, performImeBackspace(ic)) + assertEquals(listOf("commitText(0)", "sendKeyEvent", "sendKeyEvent"), ic.calls) + } + + @Test + fun lying_editor_true_commit_but_selection_survives_gets_del_fallback() { + // Review loop 1 (I1): the bug-#5 editor class returns TRUE while + // doing nothing. The re-verify must catch it and fire the fallback. + val ic = FakeIc(selected = "sel", commitAccepts = true, selectionSurvivesCommit = true) + assertEquals(ImeBackspaceOutcome.SelectionDeleted, performImeBackspace(ic)) assertEquals(listOf("commitText(0)", "sendKeyEvent", "sendKeyEvent"), ic.calls) } From d05f08ef2ea0b4ffa4d812ae818aa1f3cc197d8f Mon Sep 17 00:00:00 2001 From: will wade Date: Thu, 29 Oct 2026 17:00:18 +0000 Subject: [PATCH 3/3] =?UTF-8?q?chore(ime):=20review=20loop=202=20polish=20?= =?UTF-8?q?=E2=80=94=20schema=20diff=20restored=20to=20one=20line,=20telem?= =?UTF-8?q?etry=20symmetry,=20mapping=20pinned?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - NEW-1 (merge-blocker): the indent-2 rewrite of analytics-events.json reverted; the state enum extension is now the ONLY line changed, preserving the shared schema's formatting (reviewer: churn taxes every cross-repo sync of this hot file) - NEW-2: the ⌫ emits ui_action even with no connection (state no_connection) — a dead delete from a missing InputConnection is exactly the symptom class the telemetry should see - NEW-3: the outcome→state mapping moved onto ImeBackspaceOutcome (telemetryState) — testable, no duplicate string at the call site - Schema state description corrected (toggle states OR the delete kind) - Tests: empty surviving selection = success (no fallback fired); telemetry mapping pinned. 92 tests green. Signed-off-by: will wade --- analytics-events.json | 229 ++---------------- .../java/at/dasher/android/EditingToolbar.kt | 35 +-- .../at/dasher/android/ImeBackspaceTest.kt | 16 ++ 3 files changed, 61 insertions(+), 219 deletions(-) diff --git a/analytics-events.json b/analytics-events.json index c0a30df..aa25042 100644 --- a/analytics-events.json +++ b/analytics-events.json @@ -5,97 +5,39 @@ "default_properties": { "description": "Automatically appended to every event from every frontend", "properties": { - "platform": { - "type": "string", - "examples": [ - "windows", - "macos", - "ios", - "visionos", - "linux", - "android" - ] - }, - "app_variant": { - "type": "string", - "examples": [ - "dasher-windows", - "dasher-mac", - "dasher-ios", - "dasher-gtk", - "dasher-android" - ] - }, - "app_version": { - "type": "string", - "example": "0.1.0-preview9" - }, - "os_version": { - "type": "string", - "example": "Microsoft Windows 10.0.19045" - }, - "$geoip_disable": { - "type": "boolean", - "const": true, - "description": "Sent on every event (including crashes): PostHog Cloud must not derive location from the client IP (RFC 0001: no location data)." - } + "platform": { "type": "string", "examples": ["windows", "macos", "ios", "visionos", "linux", "android"] }, + "app_variant": { "type": "string", "examples": ["dasher-windows", "dasher-mac", "dasher-ios", "dasher-gtk", "dasher-android"] }, + "app_version": { "type": "string", "example": "0.1.0-preview9" }, + "os_version": { "type": "string", "example": "Microsoft Windows 10.0.19045" }, + "$geoip_disable": { "type": "boolean", "const": true, "description": "Sent on every event (including crashes): PostHog Cloud must not derive location from the client IP (RFC 0001: no location data)." } } }, "events": { "app_launched": { "description": "App starts (after opt-in)", "properties": { - "locale": { - "type": "string", - "example": "en-GB" - } + "locale": { "type": "string", "example": "en-GB" } }, "pii": false }, "input_method_changed": { "description": "User selects input method", "properties": { - "method": { - "type": "string", - "enum": [ - "pointer", - "touch", - "switch", - "joystick", - "eye_gaze", - "tilt" - ] - } + "method": { "type": "string", "enum": ["pointer", "touch", "switch", "joystick", "eye_gaze", "tilt"] } }, "pii": false }, "alphabet_selected": { "description": "User picks alphabet", "properties": { - "alphabet_id": { - "type": "string", - "example": "English with limited punctuation" - } + "alphabet_id": { "type": "string", "example": "English with limited punctuation" } }, "pii": false }, "settings_viewed": { "description": "User opens a settings tab", "properties": { - "tab_name": { - "type": "string", - "enum": [ - "Input", - "Language", - "Customization", - "Speed", - "Output", - "Speech", - "Privacy", - "Appearance", - "Advanced" - ] - } + "tab_name": { "type": "string", "enum": ["Input", "Language", "Customization", "Speed", "Output", "Speech", "Privacy", "Appearance", "Advanced"] } }, "pii": false }, @@ -112,178 +54,59 @@ "crash": { "description": "Uncaught exception", "properties": { - "stack_trace": { - "type": "string", - "note": "May contain file paths but not user text" - } + "stack_trace": { "type": "string", "note": "May contain file paths but not user text" } }, "pii": false }, "ime_mode_restore": { "description": "The IME decided which mode (docked/floating) to open in — every input-view creation. Android only. Decision inputs for the dock/float lifecycle debugging (Dasher-Android #73/#75).", "properties": { - "saved_mode": { - "type": "string", - "enum": [ - "docked", - "floating", - "" - ] - }, - "always_docked": { - "type": "boolean", - "description": "The Settings → Input escape hatch is enabled" - }, - "floating_before": { - "type": "boolean", - "description": "Stale floating state present BEFORE self-healing (true = the #73 path)" - }, - "restored_floating": { - "type": "boolean", - "description": "The decision: the keyboard opened floating" - }, - "overlay_perm": { - "type": "boolean", - "description": "SYSTEM_ALERT_WINDOW permission granted" - } + "saved_mode": { "type": "string", "enum": ["docked", "floating", ""] }, + "always_docked": { "type": "boolean", "description": "The Settings → Input escape hatch is enabled" }, + "floating_before": { "type": "boolean", "description": "Stale floating state present BEFORE self-healing (true = the #73 path)" }, + "restored_floating": { "type": "boolean", "description": "The decision: the keyboard opened floating" }, + "overlay_perm": { "type": "boolean", "description": "SYSTEM_ALERT_WINDOW permission granted" } }, "pii": false }, "ime_float_enter": { "description": "The IME entered floating mode. Android only.", "properties": { - "reason": { - "type": "string", - "enum": [ - "button", - "restore" - ], - "description": "button = explicit user tap; restore = saved-preference restore on keyboard show" - } + "reason": { "type": "string", "enum": ["button", "restore"], "description": "button = explicit user tap; restore = saved-preference restore on keyboard show" } }, "pii": false }, "ime_float_exit": { "description": "The IME left floating mode. Android only.", "properties": { - "persist": { - "type": "boolean", - "description": "false = system-driven cleanup, the saved preference is unchanged" - }, - "reason": { - "type": "string", - "enum": [ - "button", - "selfheal", - "unbind", - "destroy" - ], - "description": "button = user tapped Dock; selfheal = stale-state cleanup at show; unbind = input binding went away (IME switch); destroy = the IME service was destroyed while floating" - } + "persist": { "type": "boolean", "description": "false = system-driven cleanup, the saved preference is unchanged" }, + "reason": { "type": "string", "enum": ["button", "selfheal", "unbind", "destroy"], "description": "button = user tapped Dock; selfheal = stale-state cleanup at show; unbind = input binding went away (IME switch); destroy = the IME service was destroyed while floating" } }, "pii": false }, "ime_always_docked_changed": { "description": "User toggled Settings → Input → 'Keyboard always starts docked'. Android only. Adoption of this escape hatch is the signal for whether last-mode restore is the right default.", "properties": { - "enabled": { - "type": "boolean" - } + "enabled": { "type": "boolean" } }, "pii": false }, "ui_action": { "description": "User activated a control in the main UI (outside settings windows) — RFC 0001 2026-10-28 amendment. Answers 'where should main-UI focus go'.", "properties": { - "surface": { - "type": "string", - "enum": [ - "main_toolbar", - "status_bar", - "ime_toolbar", - "ime_topbar" - ] - }, - "action": { - "type": "string", - "description": "Fixed vocabulary; new identifiers require a schema update", - "examples": [ - "new", - "open", - "save", - "pause_play", - "game_mode", - "settings", - "copy", - "share", - "emoji_toggle", - "speed_up", - "speed_down", - "layout", - "float", - "dock", - "hide", - "cursor_left", - "cursor_right", - "backspace", - "select_all", - "paste" - ] - }, - "state": { - "type": "string", - "enum": [ - "on", - "off", - "paused", - "playing", - "text", - "emoji", - "selection", - "char", - "none" - ], - "description": "Optional — for toggles" - } + "surface": { "type": "string", "enum": ["main_toolbar", "status_bar", "ime_toolbar", "ime_topbar"] }, + "action": { "type": "string", "description": "Fixed vocabulary; new identifiers require a schema update", "examples": ["new", "open", "save", "pause_play", "game_mode", "settings", "copy", "share", "emoji_toggle", "speed_up", "speed_down", "layout", "float", "dock", "hide", "cursor_left", "cursor_right", "backspace", "select_all", "paste"] }, + "state": { "type": "string", "enum": ["on", "off", "paused", "playing", "text", "emoji", "selection", "char", "none", "no_connection"], "description": "Optional — toggle states, or the delete kind for the backspace action" } }, "pii": false }, "setting_changed": { "description": "User committed a settings change — RFC 0001 2026-10-28 amendment. Answers 'which settings are the priority'. Fires at the widget commit site only (never for programmatic engine changes).", "properties": { - "setting_id": { - "type": "string", - "description": "Canonical identifier: DasherCore parameter name or frontend setting key", - "examples": [ - "Max Bitrate", - "output_font_size" - ] - }, - "value_type": { - "type": "string", - "enum": [ - "bool", - "number", - "enum", - "string" - ] - }, - "value": { - "type": [ - "boolean", - "number", - "string" - ], - "description": "Present ONLY for bool/number/enum (fixed vocabularies). NEVER for string settings — free text is not collected" - }, - "surface": { - "type": "string", - "enum": [ - "settings", - "main_ui", - "ime" - ] - } + "setting_id": { "type": "string", "description": "Canonical identifier: DasherCore parameter name or frontend setting key", "examples": ["Max Bitrate", "output_font_size"] }, + "value_type": { "type": "string", "enum": ["bool", "number", "enum", "string"] }, + "value": { "type": ["boolean", "number", "string"], "description": "Present ONLY for bool/number/enum (fixed vocabularies). NEVER for string settings — free text is not collected" }, + "surface": { "type": "string", "enum": ["settings", "main_ui", "ime"] } }, "pii": false } diff --git a/app/src/main/java/at/dasher/android/EditingToolbar.kt b/app/src/main/java/at/dasher/android/EditingToolbar.kt index 0efa406..c8cb9a6 100644 --- a/app/src/main/java/at/dasher/android/EditingToolbar.kt +++ b/app/src/main/java/at/dasher/android/EditingToolbar.kt @@ -55,15 +55,13 @@ class EditingToolbar( addTool(R.drawable.ic_lucide_arrow_right, "Move cursor right", mutates = true) { moveCursor(1) } addSpacer(dp(8)) addTool(R.drawable.ic_lucide_delete, "Delete", large = true, mutates = true, logTelemetry = false) { - val ic = inputConnection() ?: return@addTool - // Telemetry distinguishes selection-delete from caret backspace - // (review M4 — the distinction that motivated this fix). - val state = when (performImeBackspace(ic)) { - ImeBackspaceOutcome.SelectionDeleted -> "selection" - ImeBackspaceOutcome.CaretDeleted -> "char" - ImeBackspaceOutcome.Noop -> "none" - } - UiTelemetry.action("ime_toolbar", "backspace", state) + val ic = inputConnection() + // Telemetry symmetry (review NEW-2): every other tool emits + // before acting — a dead ⌫ from a missing connection is exactly + // the symptom class worth seeing in the data. + val state = if (ic == null) "no_connection" + else performImeBackspace(ic).telemetryState + UiTelemetry.action("ime_toolbar", actionIdFor(tooltip = "Delete"), state) } addSpacer(dp(8)) addTool(R.drawable.ic_lucide_text_select, "Select all", mutates = false) { performAction(android.R.id.selectAll) } @@ -159,26 +157,31 @@ class EditingToolbar( * the editors documented hostile in bug #5 return TRUE while doing * nothing, so a true commit is re-verified — if the selection survived, * the DEL key-event fallback fires anyway (at worst a lying editor that - * DID delete gets one extra char — the lesser evil vs. a dead ⌫). + * DID delete loses extra text — the lesser evil vs. a dead ⌫). * - CARET: one codepoint back — deleteSurroundingText with UTF-16 unit * counts (a surrogate pair, e.g. any emoji, is 2). - * - * @return the outcome, for telemetry (selection vs caret distinction). */ sealed interface ImeBackspaceOutcome { + /** + * The ui_action state string for this outcome (review NEW-3: the + * mapping lives with the type, testable, not duplicated at call sites). + */ + abstract val telemetryState: String + /** The selection (if any) was deleted. */ - object SelectionDeleted : ImeBackspaceOutcome + object SelectionDeleted : ImeBackspaceOutcome { override val telemetryState = "selection" } /** One codepoint before the caret was deleted. */ - object CaretDeleted : ImeBackspaceOutcome + object CaretDeleted : ImeBackspaceOutcome { override val telemetryState = "char" } /** Nothing to delete (empty selection + empty before-cursor). */ - object Noop : ImeBackspaceOutcome + object Noop : ImeBackspaceOutcome { override val telemetryState = "none" } } fun performImeBackspace(ic: InputConnection): ImeBackspaceOutcome { val selected = ic.getSelectedText(0) if (selected != null && selected.isNotEmpty()) { val committed = ic.commitText("", 1) - // Bug #5's lesson: a `true` return does not mean it happened. + // Bug #5's lesson: a `true` return does not mean it happened. An + // empty (non-null) surviving selection is success — no fallback. val survived = ic.getSelectedText(0) if (!committed || (survived != null && survived.isNotEmpty())) { ic.sendKeyEvent(KeyEvent(KeyEvent.ACTION_DOWN, KeyEvent.KEYCODE_DEL)) diff --git a/app/src/test/java/at/dasher/android/ImeBackspaceTest.kt b/app/src/test/java/at/dasher/android/ImeBackspaceTest.kt index b6d1404..02666b3 100644 --- a/app/src/test/java/at/dasher/android/ImeBackspaceTest.kt +++ b/app/src/test/java/at/dasher/android/ImeBackspaceTest.kt @@ -96,6 +96,15 @@ class ImeBackspaceTest { assertEquals(listOf("commitText(0)", "sendKeyEvent", "sendKeyEvent"), ic.calls) } + @Test + fun empty_surviving_selection_is_success_no_fallback() { + // Post-commit re-verify: a non-null but EMPTY selection means the + // delete landed — no DEL fallback may fire. + val ic = FakeIc(selected = "sel", selectionSurvivesCommit = false) + assertEquals(ImeBackspaceOutcome.SelectionDeleted, performImeBackspace(ic)) + assertEquals(listOf("commitText(0)"), ic.calls) + } + @Test fun empty_selection_falls_through_to_caret_backspace() { // Some editors return "" rather than null with no selection @@ -118,6 +127,13 @@ class ImeBackspaceTest { assertEquals(listOf("deleteSurroundingText(2)"), ic.calls) } + @Test + fun telemetry_state_mapping_is_pinned() { + assertEquals("selection", ImeBackspaceOutcome.SelectionDeleted.telemetryState) + assertEquals("char", ImeBackspaceOutcome.CaretDeleted.telemetryState) + assertEquals("none", ImeBackspaceOutcome.Noop.telemetryState) + } + @Test fun empty_field_is_a_noop() { val ic = FakeIc(selected = null, beforeCursor = "")