Skip to content

feat(keyboard): add terminal keyboard layout - #724

Open
KacperLa wants to merge 5 commits into
OGKevin:masterfrom
KacperLa:kl_terminal_keyboard
Open

feat(keyboard): add terminal keyboard layout#724
KacperLa wants to merge 5 commits into
OGKevin:masterfrom
KacperLa:kl_terminal_keyboard

Conversation

@KacperLa

Copy link
Copy Markdown
Contributor

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 ToggleableKeyboard component 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:

  • Added new key variants to KeyKind and KeyboardEvent enums, including Tab, Escape, Control, and Arrow keys, along with proper deserialization and display labels for each. [1] [2] [3] [4] [5]
  • Updated keyboard event handling to process new key events, including sending specific events for Tab, Escape, Control (with character), and Arrow keys, and handling Control+ as a special case. [1] [2] [3]
  • Modified modifier key logic to support Control as a toggleable modifier, similar to Shift and Alternate. [1] [2] [3] [4] Fa234f01L230R264)

Custom keyboard layout and configuration:

  • Added a new "Terminal" keyboard layout in keyboard-layouts/terminal.json, featuring keys commonly used in terminal environments (e.g., Tab, Esc, Ctrl, arrow keys).
  • Enhanced ToggleableKeyboard with builder methods with_layout and with_bottom_bar to 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:

  • Added unit tests for the new builder methods, keyboard height calculation, and layout customization to ensure correct behavior and support for chaining configuration methods.

@github-project-automation github-project-automation Bot moved this to Backlog in Cadmus Jul 17, 2026
@dosubot dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. enhancement New feature or request UI labels Jul 17, 2026
Comment thread crates/core/src/view/key.rs Outdated
Comment thread crates/core/src/view/key.rs Outdated
Comment thread crates/core/src/view/toggleable_keyboard.rs Outdated

@OGKevin OGKevin left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 small revert request of the fr translation file, otherwise looks good 👍

Comment thread crates/core/i18n/fr/cadmus_core.ftl
Comment thread crates/core/build.rs
@OGKevin

OGKevin commented Jul 22, 2026

Copy link
Copy Markdown
Owner

@cursoragent can you open a new PR to not run crowdin sync on forks, as it can't access the secrets.

@OGKevin

OGKevin commented Jul 22, 2026

Copy link
Copy Markdown
Owner

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The PR adds workspace-managed serde_json for the core build dependencies, generates keyboard layouts from JSON files, and adds a Terminal layout. Keyboard key models and events now support control, tab, escape, and arrow keys with localized labels. Keyboard handling tracks control state and emits corresponding events. ToggleableKeyboard gains configurable layouts, bottom-bar behavior, computed height, and related tests.

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
Loading

Suggested labels: dependencies

Suggested reviewers: ogkevin

Poem

JSON keys bloom in a build-time stream,
Control and arrows join the dream.
Labels speak French and English bright,
Terminal layouts snap into sight.
The keyboard grows, calm and keen—
With bottom bar or space between.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title follows Conventional Commits and accurately describes a main part of the change.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 77f6530 and 0c8c62b.

📒 Files selected for processing (11)
  • Cargo.toml
  • crates/core/Cargo.toml
  • crates/core/build.rs
  • crates/core/i18n/en-GB/cadmus_core.ftl
  • crates/core/i18n/fr/cadmus_core.ftl
  • crates/core/src/view/input_field.rs
  • crates/core/src/view/key.rs
  • crates/core/src/view/keyboard.rs
  • crates/core/src/view/mod.rs
  • crates/core/src/view/toggleable_keyboard.rs
  • keyboard-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 to cadmus_core.ftl in the correct sorted section.
Parameterised messages must use Fluent variable syntax in .ftl files.

Files:

  • crates/core/i18n/fr/cadmus_core.ftl
  • crates/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 in crates/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, or notification-<description>.

Files:

  • crates/core/i18n/en-GB/cadmus_core.ftl
**/Cargo.toml

📄 CodeRabbit inference engine (AGENTS.md)

**/Cargo.toml: Define dependency versions in the root Cargo.toml under [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.
Ensure bin/, resources/, and hyphenation-patterns/ are present before Kobo builds when compile-time metadata is generated from bundled assets.

Files:

  • crates/core/Cargo.toml
  • Cargo.toml
**/*.{rs,toml,yml,yaml}

📄 CodeRabbit inference engine (AGENTS.md)

All feature combinations run on ubuntu-latest; only default and test features produce build artifacts.

Files:

  • crates/core/Cargo.toml
  • Cargo.toml
  • crates/core/src/view/input_field.rs
  • crates/core/build.rs
  • crates/core/src/view/toggleable_keyboard.rs
  • crates/core/src/view/mod.rs
  • crates/core/src/view/keyboard.rs
  • crates/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() or format!("literal") for user-visible text.
The fl! macro must be used at every call site for user-visible text.

crates/core/**/*.rs: All handle_event and render methods in view components must use the specified OpenTelemetry #[cfg_attr(feature = "tracing", tracing::instrument(...))] instrumentation.
Instrumentation skip() names must exactly match the method parameter names, including underscore prefixes.
Verify tracing instrumentation with cargo check --features tracing.
A view's render() 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, then no_run, and use ignore only for private or pub(crate) items unreachable from the test harness, with an explanatory comment.
Use # to hide Rustdoc example boilerplate and verify examples with cargo test --doc.
Use typed SQLx macros: sqlx::query!, sqlx::query_as!, and sqlx::query_scalar!; do not use untyped query functions.
Use .flatten() on query_scalar! results for nullable columns producing Option<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 use fl! with use crate::fl;; do not hardcode labels, buttons, placeholders, notifications, or other user-facing text.
Pass Fluent variable values at call sites using fl!("id", var = value).
Apply localization to SettingKind labels, Event::OpenNamedInput labels, menu entry text, button labels, notification text, and all other user-visible strings.

Files:

  • crates/core/src/view/input_field.rs
  • crates/core/build.rs
  • crates/core/src/view/toggleable_keyboard.rs
  • crates/core/src/view/mod.rs
  • crates/core/src/view/keyboard.rs
  • crates/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 ? over unwrap() or expect() in library and app Rust code.
Use thiserror for custom error types and anyhow for ad-hoc errors.
Prefer iterators over index-based loops.
Use &str instead of String in 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.
Avoid unsafe unless it is required and documented.
Avoid premature collect() calls and keep iterators lazy.
Ensure Rust code compiles without warnings.
Use the most restrictive visibility that works: prefer private mod over pub mod, pub(crate) over pub, and expose items publicly only when non-documentation code outside the parent module uses them.
Before accepting new pub items, check for external non-documentation usage with imports or fully qualified paths.
Prefer newtype wrappers over raw primitives when values represent domain concepts.
Implement Display, 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. Contextual TODO, FIXME, HACK, and NOTE annotations are allowed.
Use structured fields with the tracing crate; never put log data in formatted strings, use no module prefixes, and do not mix structured fields with format arguments.
Use debug for development detail, info for important runtime events, warn for recoverable issues, and error for failures requiring attention.
Return domain types directly from sqlx queries instead of parsing primitives afterward.
Implement sqlx::Type, Encode, and Decode for owned newtypes used in SQLite query results, delegating to the inner primitive.
Use sqlx t...

Files:

  • crates/core/src/view/input_field.rs
  • crates/core/build.rs
  • crates/core/src/view/toggleable_keyboard.rs
  • crates/core/src/view/mod.rs
  • crates/core/src/view/keyboard.rs
  • crates/core/src/view/key.rs
**/*.{rs,tsx,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

When testing strings rendered through fl!, construct expected strings with fl! as well.

Files:

  • crates/core/src/view/input_field.rs
  • crates/core/build.rs
  • crates/core/src/view/toggleable_keyboard.rs
  • crates/core/src/view/mod.rs
  • crates/core/src/view/keyboard.rs
  • crates/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.rs
  • crates/core/src/view/toggleable_keyboard.rs
  • crates/core/src/view/mod.rs
  • crates/core/src/view/keyboard.rs
  • crates/core/src/view/key.rs
🔇 Additional comments (11)
crates/core/i18n/fr/cadmus_core.ftl (1)

4-16: 📐 Maintainability & Code Quality | ⚡ Quick win

Manual 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! The release_key iterator-based helper is a solid cleanup, and the Control modifier cycling mirrors the existing Shift/Alternate pattern consistently. Traced the double release_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 budgets 3 * big_height (plus bar/thickness), regardless of which layout is actually shown. Keyboard::new/resize, by contrast, size themselves to the real row count from context.keyboard_layouts[...].keys.len(). Since terminal.json — introduced in this same PR — has 4 key rows, selecting it via with_layout(Layout::Terminal) (or via context.settings.keyboard_layout directly) will reserve the wrong amount of space in show(), causing the separator/keyboard region to be sized for 3 rows while 4 rows actually render there.

This also means keyboard_height() never consults self.layout at 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! The Layout enum + with_layout/with_bottom_bar builders 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

Comment thread crates/core/src/view/input_field.rs Outdated
@KacperLa KacperLa changed the title Kl terminal keyboard feat(keyboard): add terminal keyboard layout Jul 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request size:L This PR changes 100-499 lines, ignoring generated files. UI

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

2 participants