Conversation
- Map Cmd/Meta to Ctrl on non-macOS platforms in shortcut parsing - Use platform-specific shortcut hint formatting (macOS symbols vs Windows text) - Increase JS API initialization timeout from 2.5s to 10s for WebView2
There was a problem hiding this comment.
Pull request overview
Adds initial cross-platform (Windows) support for Arto by adapting shortcut parsing/formatting and increasing WebView JS-bridge initialization timeouts to accommodate slower WebView2 startup.
Changes:
- Map
Cmd/Meta/Commandmodifiers toCtrlon non-macOS during shortcut parsing. - Render shortcut hints with macOS symbols on macOS and text labels (e.g.,
Ctrl+) on Windows/Linux. - Increase JS readiness retry windows from 2.5s to 10s for keyboard interceptor and search API.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| desktop/src/shortcut.rs | Changes shortcut parsing so Cmd-style presets work as Ctrl on non-macOS. |
| desktop/src/keybindings/hint.rs | Makes shortcut hint rendering platform-specific (symbols on macOS, text on others). |
| desktop/src/components/search_bar.rs | Extends JS search API readiness polling timeout to 10s. |
| desktop/src/components/app/keybinding_engine.rs | Extends JS keyboard interceptor readiness polling timeout to 10s. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // On non-macOS platforms, map Cmd/Command/Meta to CONTROL | ||
| // so that preset keybindings like "Cmd+n" work as "Ctrl+n" on Windows/Linux | ||
| #[cfg(target_os = "macos")] | ||
| "meta" | "cmd" | "command" => modifiers.insert(Modifiers::META), | ||
| #[cfg(not(target_os = "macos"))] | ||
| "meta" | "cmd" | "command" => modifiers.insert(Modifiers::CONTROL), |
There was a problem hiding this comment.
parse_chord now maps "Cmd/Command/Meta" to Modifiers::CONTROL on non-macOS, which will make the existing unit tests that assert Modifiers::META for "Cmd+…" fail on Windows/Linux (e.g., parse_cmd_key, parse_symbol_alias). Update those tests to be OS-aware (e.g., #[cfg(target_os = "macos")] expect META, otherwise expect CONTROL).
| @@ -83,10 +85,46 @@ fn format_chord_hint(chord: &str) -> String { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| #[cfg(not(target_os = "macos"))] | |||
| for modifier in &parts { | |||
| match modifier.to_ascii_lowercase().as_str() { | |||
| "cmd" | "command" | "meta" => out.push_str("Ctrl+"), | |||
| "ctrl" | "control" => out.push_str("Ctrl+"), | |||
| "shift" => out.push_str("Shift+"), | |||
| "alt" | "option" => out.push_str("Alt+"), | |||
| other => { | |||
| out.push_str(other); | |||
| out.push('+'); | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| out.push_str(&format_key_name_hint(key_part)); | |||
There was a problem hiding this comment.
On non-macOS this now formats modifier hints as text (e.g., "Ctrl+") instead of symbols, but the existing tests in this module still assert macOS symbol output (e.g., expecting "⌘⇧O" / "⌃W H"). This will fail when running cargo test on Windows/Linux; make the assertions conditional on target_os (or split into per-platform tests).
| // On non-macOS platforms, map Cmd/Command/Meta to CONTROL | ||
| // so that preset keybindings like "Cmd+n" work as "Ctrl+n" on Windows/Linux | ||
| #[cfg(target_os = "macos")] | ||
| "meta" | "cmd" | "command" => modifiers.insert(Modifiers::META), | ||
| #[cfg(not(target_os = "macos"))] | ||
| "meta" | "cmd" | "command" => modifiers.insert(Modifiers::CONTROL), |
There was a problem hiding this comment.
Mapping "Cmd/Command/Meta" to Modifiers::CONTROL on non-macOS will create duplicate shortcuts in presets/configs that intentionally include both Cmd- and Ctrl-based bindings. Concretely, the Emacs preset global context includes both "Cmd+n" and "Ctrl+n"; after this change both normalize to "Ctrl+n", causing validate_preset_bindings() to panic on Windows/Linux. Consider keeping Cmd as META in config parsing and translating the physical key at runtime, or introduce platform-specific presets / a distinct token (e.g., "Primary") so Cmd-based and Ctrl-based bindings can coexist without collisions.
| // On non-macOS platforms, map Cmd/Command/Meta to CONTROL | |
| // so that preset keybindings like "Cmd+n" work as "Ctrl+n" on Windows/Linux | |
| #[cfg(target_os = "macos")] | |
| "meta" | "cmd" | "command" => modifiers.insert(Modifiers::META), | |
| #[cfg(not(target_os = "macos"))] | |
| "meta" | "cmd" | "command" => modifiers.insert(Modifiers::CONTROL), | |
| // Keep Cmd/Command/Meta as META in the parsed representation on all platforms. | |
| // Platform-specific translation of META (e.g., treating it like Ctrl on non-macOS) | |
| // should be handled at runtime, so that Cmd- and Ctrl-based bindings can coexist | |
| // in presets/configs without colliding. | |
| "meta" | "cmd" | "command" => modifiers.insert(Modifiers::META), |
|
I’m unable to test this since I don’t have a Windows environment, but does this PR alone fully provide Windows support? Please provide screenshots and screencasts. Also, it appears that multiple independent concepts are bundled into a single commit. Please split them into appropriate, logically separated commits. |
|
Got it. I’ve broken the changes down into nine atomic, minimal branches. Since I’ve broken them down into atomic, minimal changes, attempting to build each individual PR will result in a build error. However, by merging them all using this command, you can test the functionality without encountering any build errors. |
|
I'm sorry but it's not what I want. Each individual branches should be independent and meaningful. |
|
Thank you for pointing that out. So, am I correct in understanding that each PR must be atomic, meaningful, and independent, and must pass compilation and CI checks on its own? To put it another way, are you saying that because a single PR labeled “Windows support” contains countless bug fixes unrelated to Windows (such as footnotes and sidebars), and because all of that has been compressed into a single massive commit, you’d like the commits to be split up appropriately? |
|
I've split this PR into three independent, atomic PRs that each pass compilation and CI on their own:
Each PR includes screenshots showing before/after behavior on Windows 11. |
|
It seems all PRs are closed with
So, which one is a PR for addressing that? |
Summary
Adds initial Windows support for Arto. The application builds and runs on Windows with these changes:
Cmd/Metais mapped toCtrlon non-macOS platforms, so preset keybindings likeCmd+nwork asCtrl+non WindowsTest plan
cargo checkpasses on Windows (0 errors, 2 pre-existing warnings)dx bundle --releaseproduces a working Windows executableCtrl+instead of⌘Notes
.ico) is not yet created, sodx bundleshows a non-fatal icon error