feat(keyboard): add terminal keyboard layout - #724
Conversation
OGKevin
left a comment
There was a problem hiding this comment.
1 small revert request of the fr translation file, otherwise looks good 👍
|
@cursoragent can you open a new PR to not run crowdin sync on forks, as it can't access the secrets. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
WalkthroughThe PR adds workspace-managed Sequence Diagram(s)sequenceDiagram
participant Keyboard
participant KeyboardEvent
participant InputField
Keyboard->>Keyboard: process special key or control modifier
Keyboard->>KeyboardEvent: emit Arrow, Tab, Escape, or Control
KeyboardEvent->>InputField: handle focused-field event
InputField-->>InputField: preserve text and cursor for special events
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/core/src/view/input_field.rs`:
- Around line 298-301: Update the KeyboardEvent match in the input-field event
handler to handle KeyboardEvent::Control(char) with the intended Ctrl+letter
behavior, rather than ignoring it. If control events are not supported in this
handler, remove or prevent their emission in the keyboard view so they cannot be
silently dropped.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 8eb3fb56-8c9a-4d60-8725-0c21d1554b33
📒 Files selected for processing (11)
Cargo.tomlcrates/core/Cargo.tomlcrates/core/build.rscrates/core/i18n/en-GB/cadmus_core.ftlcrates/core/i18n/fr/cadmus_core.ftlcrates/core/src/view/input_field.rscrates/core/src/view/key.rscrates/core/src/view/keyboard.rscrates/core/src/view/mod.rscrates/core/src/view/toggleable_keyboard.rskeyboard-layouts/terminal.json
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
crates/core/**/*.ftl
📄 CodeRabbit inference engine (crates/core/REVIEW.md)
crates/core/**/*.ftl: New message IDs must be added tocadmus_core.ftlin the correct sorted section.
Parameterised messages must use Fluent variable syntax in.ftlfiles.
Files:
crates/core/i18n/fr/cadmus_core.ftlcrates/core/i18n/en-GB/cadmus_core.ftl
crates/core/i18n/en-GB/cadmus_core.ftl
📄 CodeRabbit inference engine (crates/core/AGENTS.md)
crates/core/i18n/en-GB/cadmus_core.ftl: Every user-visible string must have a Fluent message ID incrates/core/i18n/en-GB/cadmus_core.ftl.
Parameterized Fluent strings must use Fluent variables such as{ $var }.
Keep Fluent keys sorted alphabetically within each comment section.
Use kebab-case Fluent keys prefixed by the feature area:settings-<category>-<description>,settings-<category>-<description>-input, ornotification-<description>.
Files:
crates/core/i18n/en-GB/cadmus_core.ftl
**/Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
**/Cargo.toml: Define dependency versions in the rootCargo.tomlunder[workspace.dependencies].
Use caret requirements for most dependencies, alphabetize dependencies within logical groups, disable default features when fine-grained control is needed, and keep related crate families at compatible versions.
When adding a feature flag, define it in the relevant Cargo manifest and use workspace-wide or crate-specific feature arguments as appropriate.
Ensurebin/,resources/, andhyphenation-patterns/are present before Kobo builds when compile-time metadata is generated from bundled assets.
Files:
crates/core/Cargo.tomlCargo.toml
**/*.{rs,toml,yml,yaml}
📄 CodeRabbit inference engine (AGENTS.md)
All feature combinations run on
ubuntu-latest; onlydefaultandtestfeatures produce build artifacts.
Files:
crates/core/Cargo.tomlCargo.tomlcrates/core/src/view/input_field.rscrates/core/build.rscrates/core/src/view/toggleable_keyboard.rscrates/core/src/view/mod.rscrates/core/src/view/keyboard.rscrates/core/src/view/key.rs
crates/core/**/*.rs
📄 CodeRabbit inference engine (crates/core/REVIEW.md)
crates/core/**/*.rs: Do not use"string literal".to_string()orformat!("literal")for user-visible text.
Thefl!macro must be used at every call site for user-visible text.
crates/core/**/*.rs: Allhandle_eventandrendermethods in view components must use the specified OpenTelemetry#[cfg_attr(feature = "tracing", tracing::instrument(...))]instrumentation.
Instrumentationskip()names must exactly match the method parameter names, including underscore prefixes.
Verify tracing instrumentation withcargo check --features tracing.
A view'srender()is called only when it has no children or is a background view; container views with children are not rendered directly.
For container decoration, use a background view for content rendered before children, or a dedicated child view for overlays rendered in child order.
Rustdoc examples should prefer fully compilable examples, thenno_run, and useignoreonly for private orpub(crate)items unreachable from the test harness, with an explanatory comment.
Use#to hide Rustdoc example boilerplate and verify examples withcargo test --doc.
Use typed SQLx macros:sqlx::query!,sqlx::query_as!, andsqlx::query_scalar!; do not use untyped query functions.
Use.flatten()onquery_scalar!results for nullable columns producingOption<Option<T>>.
Untyped SQLx queries are permitted for dynamic SQL only when unit tests cover every dynamic path and a comment explains why a typed macro cannot be used.
All user-visible strings must usefl!withuse crate::fl;; do not hardcode labels, buttons, placeholders, notifications, or other user-facing text.
Pass Fluent variable values at call sites usingfl!("id", var = value).
Apply localization toSettingKindlabels,Event::OpenNamedInputlabels, menu entry text, button labels, notification text, and all other user-visible strings.
Files:
crates/core/src/view/input_field.rscrates/core/build.rscrates/core/src/view/toggleable_keyboard.rscrates/core/src/view/mod.rscrates/core/src/view/keyboard.rscrates/core/src/view/key.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: The emulator code path should panic on errors, while the app code path should handle errors gracefully.
Prefer?overunwrap()orexpect()in library and app Rust code.
Usethiserrorfor custom error types andanyhowfor ad-hoc errors.
Prefer iterators over index-based loops.
Use&strinstead ofStringin function parameters when ownership is not needed.
Prefer borrowing over cloning.
Inline expressions directly into struct fields instead of introducing intermediate bindings solely for a struct literal.
Avoidunsafeunless it is required and documented.
Avoid prematurecollect()calls and keep iterators lazy.
Ensure Rust code compiles without warnings.
Use the most restrictive visibility that works: prefer privatemodoverpub mod,pub(crate)overpub, and expose items publicly only when non-documentation code outside the parent module uses them.
Before accepting newpubitems, check for external non-documentation usage with imports or fully qualified paths.
Prefer newtype wrappers over raw primitives when values represent domain concepts.
ImplementDisplay,FromStr, and other appropriate standard traits on newtypes to keep them ergonomic.
Match on enum variants instead of comparing strings.
Comments should explain why, not what; avoid inline comments, commented-out code, changelog comments, and divider comments. ContextualTODO,FIXME,HACK, andNOTEannotations are allowed.
Use structured fields with thetracingcrate; never put log data in formatted strings, use no module prefixes, and do not mix structured fields with format arguments.
Usedebugfor development detail,infofor important runtime events,warnfor recoverable issues, anderrorfor failures requiring attention.
Return domain types directly fromsqlxqueries instead of parsing primitives afterward.
Implementsqlx::Type,Encode, andDecodefor owned newtypes used in SQLite query results, delegating to the inner primitive.
Use sqlx t...
Files:
crates/core/src/view/input_field.rscrates/core/build.rscrates/core/src/view/toggleable_keyboard.rscrates/core/src/view/mod.rscrates/core/src/view/keyboard.rscrates/core/src/view/key.rs
**/*.{rs,tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
When testing strings rendered through
fl!, construct expected strings withfl!as well.
Files:
crates/core/src/view/input_field.rscrates/core/build.rscrates/core/src/view/toggleable_keyboard.rscrates/core/src/view/mod.rscrates/core/src/view/keyboard.rscrates/core/src/view/key.rs
🧠 Learnings (1)
📚 Learning: 2026-05-23T14:57:43.820Z
Learnt from: OGKevin
Repo: OGKevin/cadmus PR: 478
File: crates/core/src/view/settings_editor/refresh_rate_by_kind_editor.rs:91-91
Timestamp: 2026-05-23T14:57:43.820Z
Learning: When reviewing Rust code that calls `ToggleableKeyboard::new(parent_rect: Rectangle, number_mode: bool)` (from `crates/core/src/view/toggleable_keyboard.rs`), treat the second argument as `number_mode` (numeric/number-layout mode), not as an initial visibility flag. In the constructor implementation, `visible` is initialized to `false` and the keyboard starts hidden; only the numeric layout changes based on `number_mode` (e.g., for numeric inputs like `refresh rate u8`). Therefore, do not flag `ToggleableKeyboard::new(rect, true)` as “making the keyboard start visible.”
Applied to files:
crates/core/src/view/input_field.rscrates/core/src/view/toggleable_keyboard.rscrates/core/src/view/mod.rscrates/core/src/view/keyboard.rscrates/core/src/view/key.rs
🔇 Additional comments (11)
crates/core/i18n/fr/cadmus_core.ftl (1)
4-16: 📐 Maintainability & Code Quality | ⚡ Quick winManual French translations may conflict with the "leave it to Crowdin" preference.
These 11
keyboard-key-*translations are hand-written here, but the maintainer previously asked to leave non-English strings for Crowdin to translate rather than hardcoding them in the PR. Worth confirming whether this block should be dropped and left for Crowdin instead.Cargo.toml (1)
23-23: LGTM!crates/core/Cargo.toml (1)
22-22: LGTM!Also applies to: 123-123
crates/core/build.rs (1)
18-18: LGTM! Nice, clean codegen for the layout enum — verified the filename→variant derivation (leading-digit/empty/non-ascii edge cases) and it all holds up. The broader "make build.rs codegen less stringy" ask is already tracked in issue#738, so nothing new to pile on here.Also applies to: 101-101, 212-307
keyboard-layouts/terminal.json (1)
1-50: LGTM! Ran the numbers on rows/widths/outputs alignment (26 letters across all 4 shift levels, special-key aliases all valid) — checks out cleanly.crates/core/src/view/key.rs (1)
24-31: LGTM! Enum, deserializer allowlist, localized labels, and the Control tap-pressure cycling are all wired together consistently. Both prior review threads on this file (alias naming, translatability) are already marked addressed.Also applies to: 67-74, 97-103, 125-125, 137-190, 261-261, 331-331
crates/core/src/view/mod.rs (1)
69-69: LGTM!Also applies to: 734-744
crates/core/i18n/en-GB/cadmus_core.ftl (1)
12-25: LGTM! Keys sorted correctly and slotted into the file's existing loose alphabetical convention.crates/core/src/view/keyboard.rs (1)
12-12: LGTM! Therelease_keyiterator-based helper is a solid cleanup, and the Control modifier cycling mirrors the existing Shift/Alternate pattern consistently. Traced the doublerelease_modifiers()call on the Ctrl+letter path — it's a harmless no-op the second time thanks to the early-return guard, so nothing to fix there.Also applies to: 36-36, 169-202, 231-234, 283-310
crates/core/src/view/toggleable_keyboard.rs (2)
172-187: 🎯 Functional Correctness | 🏗️ Heavy lift
keyboard_height()hardcodes 3 rows, but the new Terminal layout has 4.
keyboard_height()always budgets3 * big_height(plus bar/thickness), regardless of which layout is actually shown.Keyboard::new/resize, by contrast, size themselves to the real row count fromcontext.keyboard_layouts[...].keys.len(). Sinceterminal.json— introduced in this same PR — has 4 key rows, selecting it viawith_layout(Layout::Terminal)(or viacontext.settings.keyboard_layoutdirectly) will reserve the wrong amount of space inshow(), causing the separator/keyboard region to be sized for 3 rows while 4 rows actually render there.This also means
keyboard_height()never consultsself.layoutat all, so its result is identical for every layout no matter how many rows it has.🛠️ Proposed fix: derive row count from the resolved layout
pub fn keyboard_height(&self, context: &AppContext) -> i32 { let dpi = context.device.dpi(); let small_height = scale_by_dpi(SMALL_BAR_HEIGHT, dpi) as i32; let big_height = scale_by_dpi(BIG_BAR_HEIGHT, dpi) as i32; let thickness = scale_by_dpi(THICKNESS_MEDIUM, dpi) as i32; + let layout_name = self + .layout + .map(|layout| layout.to_string()) + .unwrap_or_else(|| context.settings.keyboard_layout.clone()); + let rows_count = context.keyboard_layouts[&layout_name].keys.len() as i32; if self.has_bottom_bar { - small_height + 3 * big_height + thickness + small_height + rows_count * big_height + thickness } else { - 3 * big_height + thickness + rows_count * big_height + thickness } }Also applies to: 199-236
17-18: LGTM! TheLayoutenum +with_layout/with_bottom_barbuilders are a nice, ergonomic follow-through on the maintainer's earlier ask to avoid magic strings, and the new tests cover the builder surface well.Also applies to: 44-45, 70-100, 391-450
This pull request adds support for additional keyboard keys (such as Tab, Escape, Control, and Arrow keys), introduces a new customizable "Terminal" keyboard layout, and enhances the
ToggleableKeyboardcomponent to allow for more flexible configuration. It also improves keyboard event handling and includes comprehensive tests for the new features.Keyboard event and key support improvements:
KeyKindandKeyboardEventenums, including Tab, Escape, Control, and Arrow keys, along with proper deserialization and display labels for each. [1] [2] [3] [4] [5]Custom keyboard layout and configuration:
keyboard-layouts/terminal.json, featuring keys commonly used in terminal environments (e.g., Tab, Esc, Ctrl, arrow keys).ToggleableKeyboardwith builder methodswith_layoutandwith_bottom_barto allow specifying a custom layout and whether to reserve space for a bottom bar. The keyboard now uses the custom layout if provided and can adjust its height accordingly. [1] [2] [3]Testing and reliability: