keyboard: Preserve ascii_mode across keyboard switches#1979
keyboard: Preserve ascii_mode across keyboard switches#1979Bambooin merged 1 commit intoosfans:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to stop ascii_mode from being unconditionally reset when switching keyboard layouts by preserving the last used ASCII mode per keyboard and making per-keyboard reset behavior fall back to the theme’s global default.
Changes:
- Persist the current
ascii_modeinto a per-keyboardlastAsciiModewhen switching layouts. - Make
resetAsciiModein keyboard presets optional (Boolean?) so it can fall back to the theme default. - Update keyboard reset behavior to default to
theme.generalStyle.resetASCIIModeinstead of alwaystrue.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| app/src/main/java/com/osfans/trime/ime/keyboard/KeyboardWindow.kt | Updates keyboard switching flow to preserve ascii_mode across layout switches. |
| app/src/main/java/com/osfans/trime/ime/keyboard/Keyboard.kt | Changes resetAsciiMode default to follow the theme-level setting. |
| app/src/main/java/com/osfans/trime/data/theme/model/TextKeyboard.kt | Makes resetAsciiMode nullable and adjusts YAML decoding to allow fallback behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
When switching between keyboard layouts, ascii_mode is unconditionally reset to the keyboard's default, discarding the user's current input mode. This happends due to TextKeyboard always defaults reset_ascii_mode to true, overriding the global theme setting, and lastAsciiMode is never saved when leaving a keyboard. Fix this by making TextKeyboard.resetAsciiMode nullable so that unset values fall through to the global theme settings in Keyboard, saving the current ascii_mode before switching away. Each keyboard's resetAsciiMode now falls back to the global setting and determines whether to reset or restore the previous mode. v1->v2: - Fix regression in KeyboardWindow::attachKeyboard() Signed-off-by: Xavier Hsinyuan <me@lstlx.com>
ENOA-REIAH-UION
left a comment
There was a problem hiding this comment.
Kind of awkward—this review was supposed to be posted yesterday, but something came up and it got stuck in my drafts. Even more surprising, @Bambooin went ahead and merged this pretty questionable PR.
| /** Keyboard default ascii mode */ | ||
| val asciiMode = selfConfig?.asciiMode ?: false | ||
| val resetAsciiMode = selfConfig?.resetAsciiMode ?: true | ||
| val resetAsciiMode = selfConfig?.resetAsciiMode ?: theme.generalStyle.resetASCIIMode |
There was a problem hiding this comment.
This change is fundamentally incorrect.
You should first read the wiki carefully. Although the parameter names are identical, reset_ascii_mode under the style node and under the preset_keyboards node differ in both semantics and scope. They cannot form a fallback relationship.
In other words, identical names do not imply the same configuration level, nor do they justify inheriting default values from each other.
If this is what you consider a bug, I would suggest reviewing the wiki in full before determining whether the current behavior is actually incorrect.
There was a problem hiding this comment.
Thanks for reviewing this RFC PR.
This change is fundamentally incorrect.
You should first read the wiki carefully. Although the parameter names are identical, reset_ascii_mode under the style node and under the preset_keyboards node differ in both semantics and scope. They cannot form a fallback relationship.
In other words, identical names do not imply the same configuration level, nor do they justify inheriting default values from each other.
If this is what you consider a bug, I would suggest reviewing the wiki in full before determining whether the current behavior is actually incorrect.
Currently, reset_ascii_mode is not well documented in wiki 1 2. Check for
trime-schema.json and trime.yaml, the fallback is the natural reading.
Would you like share the full documentation, or design decisions for both style/reset_ascii_mode and preset_keyboards/<keyboard>/reset_ascii_mode?
| currentKeyboard?.let { | ||
| val mode = rime.run { statusCached }.isAsciiMode | ||
|
|
||
| Timber.d("Leaving $currentKeyboardId with ascii_mode $mode, switching to $target") | ||
| it.lastAsciiMode = mode | ||
| } | ||
|
|
There was a problem hiding this comment.
| currentKeyboard?.let { | |
| val mode = rime.run { statusCached }.isAsciiMode | |
| Timber.d("Leaving $currentKeyboardId with ascii_mode $mode, switching to $target") | |
| it.lastAsciiMode = mode | |
| } |
This change seems unnecessary.
During the initial keyboard initialization, currentKeyboard should not be null. When switching keyboards, lastAsciiMode also appears to be updated in detachCurrentView.
Given this, this assignment does not seem to have any practical effect. Have you tested a case where this makes a difference, or is there something I might have missed?
…sfans#1979)" The commit misunderstands the relationship between `style/reset_ascii_mode` (GeneralStyle.resetASCIIMode) and `preset_keyboard/<keyboard>/reset_ascii_mode` (TextKeyboard.resetAsciiMode). These parameters are fundamentally different: - `style/reset_ascii_mode` controls whether to reset ascii_mode when the IME keyboard is shown - `preset_keyboard/<keyboard>/reset_ascii_mode` controls whether to reset ascii_mode when switching preset keyboards They should NOT be treated as a fallback relationship. Many other theme parameters do follow a similar default inheritance pattern (e.g., key_height, key_width, round_corner, etc.), but this pattern should NOT be applied to reset_ascii_mode without understanding its semantic meaning. Additionally, the code added to set lastAsciiMode in attachKeyboard() will NEVER trigger as expected.
…switches (osfans#1979)" The commit misunderstands the relationship between `style/reset_ascii_mode` (GeneralStyle.resetASCIIMode) and `preset_keyboard/<keyboard>/reset_ascii_mode` (TextKeyboard.resetAsciiMode). These parameters are fundamentally different: - `style/reset_ascii_mode` controls whether to reset ascii_mode when the IME keyboard is shown - `preset_keyboard/<keyboard>/reset_ascii_mode` controls whether to reset ascii_mode when switching preset keyboards They should NOT be treated as a fallback relationship. Many other theme parameters do follow a similar default inheritance pattern (e.g., key_height, key_width, round_corner, etc.), but this pattern should NOT be applied to reset_ascii_mode without understanding its semantic meaning. Additionally, the code added to set lastAsciiMode in attachKeyboard() will NEVER trigger as expected.
Pull request
Issue tracker
Fixes will automatically close the related issues
Fixes # N/A
Feature
Fix keyboard ascii_mode being unconditionally reset on layout, by making the per-keyboard setting fall back to the global theme default and preserving the current mode in lastAsciiMode before switching away.
Code of conduct
Code style
make sytle-lintBuild pass
make debugManually test
Code Review
Daily build
Login and download artifact at https://github.com/osfans/trime/actions
Additional Info
Hi all,
This RFC pull request fix keyboard ascii_mode being unconditionally reset on layout, by making the per-keyboard setting fall back to the global theme default and preserving the current mode in lastAsciiMode before switching away.
Thanks for your review.