Enable to customize keybinds and styles#23
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a TOML-based configuration system so users can customize keybindings and UI styles (editor/listbox/highlight) for both streaming and archived modes.
Changes:
- Introduces
Config+default.tomlfor configurable styles and keybinds, parsed viaserde/toml/termcfg. - Loads (and auto-creates) a config file at
~/.config/sig/config.tomlby default, with--configoverride. - Replaces hardcoded key handling in streaming/archived modes with configurable keybind matching.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/sig.rs | Uses configurable StreamingKeybinds for actions and editor navigation. |
| src/archived.rs | Uses configurable ArchivedKeybinds for exit/retry/navigation and editor controls. |
| src/main.rs | Adds config discovery/creation + loads Config and wires it into streaming/archived runners. |
| src/config.rs | Defines config schema + style/keybind deserialization and keybind matching helper. |
| default.toml | Provides default styles/keybinds used for first-run config generation and fallback. |
| README.md | Documents config file location and --config CLI option. |
| Cargo.toml / Cargo.lock | Adds dependencies for config location + TOML/serde parsing + crossterm style/event serde. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return Ok(()); | ||
| } | ||
|
|
||
| if let Some(parent) = path.parent() { |
There was a problem hiding this comment.
ensure_file_exists: for a path like config.toml (no parent components), path.parent() returns an empty path. Calling create_dir_all on that empty path can fail on some platforms, making --config config.toml unusable. Consider skipping directory creation when the parent is empty (or use Path::parent().filter(|p| !p.as_os_str().is_empty())).
| if let Some(parent) = path.parent() { | |
| if let Some(parent) = path.parent().filter(|p| !p.as_os_str().is_empty()) { |
| if path.exists() { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| if let Some(parent) = path.parent() { | ||
| std::fs::create_dir_all(parent).map_err(|e| anyhow!("Failed to create directory: {e}"))?; | ||
| } | ||
|
|
||
| std::fs::File::create(path)?.write_all(DEFAULT_CONFIG.as_bytes())?; | ||
| Ok(()) |
There was a problem hiding this comment.
ensure_file_exists checks path.exists() and then uses File::create(path), which can truncate an existing file if it appears between the check and the create (TOCTOU race). Using OpenOptions::create_new(true) (and handling the AlreadyExists case) avoids potential data loss.
| let config = determine_config_file(args.config_file.clone()) | ||
| .and_then(|config_file| { | ||
| std::fs::read_to_string(&config_file) | ||
| .map_err(|e| anyhow!("Failed to read configuration file: {e}")) | ||
| }) | ||
| .and_then(|content| Config::load_from(&content)) | ||
| .unwrap_or_else(|_e| { | ||
| Config::load_from(DEFAULT_CONFIG).expect("Failed to load default configuration") | ||
| }); |
There was a problem hiding this comment.
Config loading currently swallows any error (missing file, unreadable file, invalid TOML, etc.) and silently falls back to DEFAULT_CONFIG. That makes it hard for users to notice misconfigurations, especially when --config is explicitly provided. Consider surfacing the error (e.g., log to stderr) and/or failing fast when a user-specified config path cannot be read/parsed.
| let config = determine_config_file(args.config_file.clone()) | |
| .and_then(|config_file| { | |
| std::fs::read_to_string(&config_file) | |
| .map_err(|e| anyhow!("Failed to read configuration file: {e}")) | |
| }) | |
| .and_then(|content| Config::load_from(&content)) | |
| .unwrap_or_else(|_e| { | |
| Config::load_from(DEFAULT_CONFIG).expect("Failed to load default configuration") | |
| }); | |
| let config_result = determine_config_file(args.config_file.clone()) | |
| .and_then(|config_file| { | |
| std::fs::read_to_string(&config_file) | |
| .map_err(|e| anyhow!("Failed to read configuration file: {e}")) | |
| }) | |
| .and_then(|content| Config::load_from(&content)); | |
| let config = match config_result { | |
| Ok(cfg) => cfg, | |
| Err(e) => { | |
| if args.config_file.is_some() { | |
| // Explicitly provided configuration should not be silently ignored. | |
| return Err(e); | |
| } else { | |
| eprintln!( | |
| "Warning: failed to load configuration file, using default configuration: {e}" | |
| ); | |
| Config::load_from(DEFAULT_CONFIG) | |
| .expect("Failed to load default configuration") | |
| } | |
| } | |
| }; |
| pub fn matches_keybind(event: &Event, keybinds: &HashSet<Event>) -> bool { | ||
| let normalized = match event { | ||
| Event::Key(key) => Event::Key(KeyEvent::new(key.code, key.modifiers)), | ||
| Event::Mouse(mouse) => Event::Mouse(MouseEvent { | ||
| kind: mouse.kind, | ||
| column: 0, | ||
| row: 0, | ||
| modifiers: mouse.modifiers, | ||
| }), | ||
| other => other.clone(), | ||
| }; | ||
|
|
||
| keybinds.contains(&normalized) | ||
| } |
There was a problem hiding this comment.
matches_keybind normalizes key events via KeyEvent::new(code, modifiers), which drops kind/state. That means a configured binding may match KeyEventKind::Repeat/Release too, whereas the previous handlers explicitly matched only Press + KeyEventState::NONE. This can cause actions like toggle-pause to fire repeatedly when a key is held. Consider requiring kind == Press (and maybe state == NONE) before matching, or encoding kind/state into the normalized form.
No description provided.