From 07da05e95f5f6c0bf4fa2b2a00d9409c9f60a67d Mon Sep 17 00:00:00 2001 From: Andreas Penz Date: Sat, 10 Jan 2026 10:42:32 +0100 Subject: [PATCH 1/3] replace `ssh` and `config` create in favor of new `settings` crate --- .gitignore | 7 +- Cargo.lock | 34 +-- Cargo.toml | 10 +- crates/config/src/lib.rs | 9 - crates/config/src/loader.rs | 284 ------------------ crates/config/src/schema.rs | 150 ---------- crates/{config => settings}/Cargo.toml | 4 +- crates/settings/src/error.rs | 35 +++ crates/settings/src/lib.rs | 9 + crates/settings/src/settings.rs | 90 ++++++ crates/settings/src/sources/config.rs | 349 +++++++++++++++++++++++ crates/settings/src/sources/mod.rs | 2 + crates/settings/src/sources/ssh.rs | 54 ++++ crates/{config => settings}/src/types.rs | 71 ++--- crates/ssh/Cargo.toml | 11 - crates/ssh/src/lib.rs | 31 -- crates/tray/Cargo.toml | 2 +- crates/tray/src/lib.rs | 14 +- src/xshuttle.rs | 40 +-- 19 files changed, 632 insertions(+), 574 deletions(-) delete mode 100644 crates/config/src/lib.rs delete mode 100644 crates/config/src/loader.rs delete mode 100644 crates/config/src/schema.rs rename crates/{config => settings}/Cargo.toml (74%) create mode 100644 crates/settings/src/error.rs create mode 100644 crates/settings/src/lib.rs create mode 100644 crates/settings/src/settings.rs create mode 100644 crates/settings/src/sources/config.rs create mode 100644 crates/settings/src/sources/mod.rs create mode 100644 crates/settings/src/sources/ssh.rs rename crates/{config => settings}/src/types.rs (62%) delete mode 100644 crates/ssh/Cargo.toml delete mode 100644 crates/ssh/src/lib.rs diff --git a/.gitignore b/.gitignore index c562ead..803c225 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,5 @@ -.claude +.claude/plans +.claude/settings.local.json .idea -/references -/target \ No newline at end of file +**/references +**/target \ No newline at end of file diff --git a/Cargo.lock b/Cargo.lock index dd2c99c..bf4867d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -429,16 +429,6 @@ dependencies = [ "crossbeam-utils", ] -[[package]] -name = "config" -version = "0.0.0" -dependencies = [ - "dirs", - "jsonschema", - "serde", - "serde_json", -] - [[package]] name = "core-foundation" version = "0.9.4" @@ -2548,6 +2538,18 @@ dependencies = [ "serde", ] +[[package]] +name = "settings" +version = "0.0.0" +dependencies = [ + "dirs", + "jsonschema", + "serde", + "serde_json", + "ssh2-config", + "thiserror 2.0.17", +] + [[package]] name = "shlex" version = "1.3.0" @@ -2616,13 +2618,6 @@ dependencies = [ "windows-sys 0.60.2", ] -[[package]] -name = "ssh" -version = "0.0.0" -dependencies = [ - "ssh2-config", -] - [[package]] name = "ssh2-config" version = "0.6.2" @@ -2955,8 +2950,8 @@ dependencies = [ name = "tray" version = "0.0.0" dependencies = [ - "config", "image", + "settings", "tray-icon", ] @@ -3730,11 +3725,10 @@ name = "xshuttle" version = "0.1.0" dependencies = [ "clap", - "config", "gtk", "objc2-core-foundation", "open", - "ssh", + "settings", "terminal", "tray", "winit", diff --git a/Cargo.toml b/Cargo.toml index 99df666..59ba9af 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -9,9 +9,8 @@ description = "Command menu in your system tray" [dependencies] clap = { workspace = true } tray = { workspace = true } -config = { workspace = true } +settings = { workspace = true } terminal = { workspace = true } -ssh = { workspace = true } winit = { workspace = true } open = { workspace = true } @@ -54,11 +53,14 @@ jsonschema = "0.28" dirs = "6.0" open = "5.3.3" +# Logging & Errors +log = "0.4" +thiserror = "2" + # Internal crates tray = { path = "crates/tray" } -config = { path = "crates/config" } terminal = { path = "crates/terminal" } -ssh = { path = "crates/ssh" } +settings = { path = "crates/settings" } [workspace.lints.rust] # Safety diff --git a/crates/config/src/lib.rs b/crates/config/src/lib.rs deleted file mode 100644 index 0939618..0000000 --- a/crates/config/src/lib.rs +++ /dev/null @@ -1,9 +0,0 @@ -mod loader; -mod schema; -mod types; - -pub use loader::{ - LoadError, config_path, ensure_config_exists, load, load_from_path, load_from_str, -}; -pub use schema::{ConfigError, ValidationError, ValidationResult, schema, validate}; -pub use types::{Action, Config, Entry, Group}; diff --git a/crates/config/src/loader.rs b/crates/config/src/loader.rs deleted file mode 100644 index 0faf44f..0000000 --- a/crates/config/src/loader.rs +++ /dev/null @@ -1,284 +0,0 @@ -use crate::schema::{ConfigError, ValidationResult, validate}; -use crate::types::Config; -use serde_json::Value; -use std::fmt; -use std::fs; -use std::io; -use std::path::{Path, PathBuf}; -use std::str::FromStr; - -const DEFAULT_JSON: &str = include_str!("../../../assets/xshuttle.default.json"); - -/// Error type for config loading operations. -#[derive(Debug)] -pub enum LoadError { - NoHomeDir, - Io(io::Error), - Config(ConfigError), -} - -impl fmt::Display for LoadError { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - LoadError::NoHomeDir => write!(f, "could not determine home directory"), - LoadError::Io(e) => write!(f, "failed to read config: {e}"), - LoadError::Config(e) => write!(f, "{e}"), - } - } -} - -impl std::error::Error for LoadError { - fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - match self { - LoadError::Io(e) => Some(e), - LoadError::Config(e) => Some(e), - LoadError::NoHomeDir => None, - } - } -} - -impl From for LoadError { - fn from(e: io::Error) -> Self { - LoadError::Io(e) - } -} - -impl From for LoadError { - fn from(e: ConfigError) -> Self { - LoadError::Config(e) - } -} - -/// Returns the default config file path (~/.xshuttle.json). -pub fn config_path() -> Option { - dirs::home_dir().map(|home| home.join(".xshuttle.json")) -} - -/// Ensures the config file exists, creating a default one if missing. -/// -/// # Errors -/// -/// Returns an error if the home directory cannot be determined or if -/// writing the default config file fails. -pub fn ensure_config_exists() -> Result { - let path = config_path().ok_or(LoadError::NoHomeDir)?; - - if !path.exists() { - fs::write(&path, DEFAULT_JSON)?; - eprintln!("Created default config at {}", path.display()); - } - - Ok(path) -} - -/// Loads config from a string. -/// -/// # Errors -/// -/// Returns an error if the JSON is invalid or fails schema validation. -pub fn load_from_str(s: &str) -> Result { - s.parse() -} - -/// Loads config from a specific path. -/// -/// # Errors -/// -/// Returns an error if the file cannot be read or the config is invalid. -pub fn load_from_path(path: &Path) -> Result { - let contents = fs::read_to_string(path)?; - Ok(load_from_str(&contents)?) -} - -/// Loads config from the default path (~/.xshuttle.json). -/// -/// # Errors -/// -/// Returns an error if the home directory cannot be determined or the config is invalid. -pub fn load() -> Result { - let path = config_path().ok_or(LoadError::NoHomeDir)?; - - if !path.exists() { - return Ok(Config::default()); - } - - load_from_path(&path) -} - -impl FromStr for Config { - type Err = ConfigError; - - fn from_str(s: &str) -> Result { - let value: Value = serde_json::from_str(s).map_err(ConfigError::InvalidJson)?; - - if let ValidationResult::Invalid(errors) = validate(&value) { - return Err(ConfigError::ValidationFailed(errors)); - } - - serde_json::from_value(value).map_err(ConfigError::InvalidJson) - } -} - -#[cfg(test)] -mod tests { - use super::*; - use crate::types::Entry; - - #[test] - fn test_load_from_str_empty() { - let config = load_from_str("{}").unwrap(); - assert_eq!(config.terminal, "default"); - } - - #[test] - fn test_load_from_str_with_terminal() { - let config = load_from_str(r#"{"terminal": "kitty"}"#).unwrap(); - assert_eq!(config.terminal, "kitty"); - } - - #[test] - fn test_load_from_str_rejects_unknown_fields() { - let result = load_from_str(r#"{"terminal": "alacritty", "unknown": true}"#); - assert!(result.is_err()); - } - - #[test] - fn test_load_from_str_invalid_json() { - let result = load_from_str("not valid json"); - assert!(matches!(result, Err(ConfigError::InvalidJson(_)))); - } - - #[test] - fn test_load_from_path_missing_file() { - let result = load_from_path(Path::new("/nonexistent/path/config.json")); - assert!(matches!(result, Err(LoadError::Io(_)))); - } - - #[test] - fn test_flat_actions() { - let config = - load_from_str(r#"{"actions": [{"name": "Test", "cmd": "echo hello"}]}"#).unwrap(); - assert_eq!(config.entries.len(), 1); - match &config.entries[0] { - Entry::Action(c) => { - assert_eq!(c.name, "Test"); - assert_eq!(c.cmd, "echo hello"); - } - Entry::Group(_) => panic!("Expected Action"), - } - } - - #[test] - fn test_nested_actions() { - let config = load_from_str( - r#"{ - "actions": [ - {"name": "Top Level", "cmd": "echo top"}, - {"Production": [ - {"name": "Server 1", "cmd": "ssh server1"} - ]} - ] - }"#, - ) - .unwrap(); - assert_eq!(config.entries.len(), 2); - match &config.entries[0] { - Entry::Action(c) => assert_eq!(c.name, "Top Level"), - Entry::Group(_) => panic!("Expected Action"), - } - match &config.entries[1] { - Entry::Group(group) => { - assert_eq!(group.name, "Production"); - assert_eq!(group.entries.len(), 1); - } - Entry::Action(_) => panic!("Expected Group"), - } - } - - #[test] - fn test_deeply_nested_actions() { - let config = load_from_str( - r#"{ - "actions": [ - {"Level1": [ - {"Level2": [ - {"name": "Deep", "cmd": "echo deep"} - ]} - ]} - ] - }"#, - ) - .unwrap(); - assert_eq!(config.entries.len(), 1); - match &config.entries[0] { - Entry::Group(l1) => { - assert_eq!(l1.name, "Level1"); - match &l1.entries[0] { - Entry::Group(l2) => { - assert_eq!(l2.name, "Level2"); - match &l2.entries[0] { - Entry::Action(c) => assert_eq!(c.name, "Deep"), - Entry::Group(_) => panic!("Expected Action at level 3"), - } - } - Entry::Action(_) => panic!("Expected Group at level 2"), - } - } - Entry::Action(_) => panic!("Expected Group at level 1"), - } - } - - #[test] - fn test_group_with_multiple_children() { - let config = load_from_str( - r#"{ - "actions": [ - {"Servers": [ - {"name": "Server 1", "cmd": "ssh server1"}, - {"name": "Server 2", "cmd": "ssh server2"}, - {"name": "Server 3", "cmd": "ssh server3"} - ]} - ] - }"#, - ) - .unwrap(); - assert_eq!(config.entries.len(), 1); - match &config.entries[0] { - Entry::Group(group) => { - assert_eq!(group.name, "Servers"); - assert_eq!(group.entries.len(), 3); - } - Entry::Action(_) => panic!("Expected Group"), - } - } - - #[test] - fn test_group_serialization_roundtrip() { - let original = r#"{"terminal":"default","editor":"default","actions":[{"Production":[{"name":"Server","cmd":"ssh prod"}]}]}"#; - let config: Config = serde_json::from_str(original).unwrap(); - let serialized = serde_json::to_string(&config).unwrap(); - - // Parse both and compare structure - let orig_value: Value = serde_json::from_str(original).unwrap(); - let ser_value: Value = serde_json::from_str(&serialized).unwrap(); - assert_eq!(orig_value, ser_value); - } - - #[test] - fn test_from_str_valid() { - let config: Config = r#"{"terminal": "kitty"}"#.parse().unwrap(); - assert_eq!(config.terminal, "kitty"); - } - - #[test] - fn test_from_str_invalid_json() { - let result: Result = "not valid json".parse(); - assert!(matches!(result, Err(ConfigError::InvalidJson(_)))); - } - - #[test] - fn test_from_str_validation_failed() { - let result: Result = r#"{"unknown": true}"#.parse(); - assert!(matches!(result, Err(ConfigError::ValidationFailed(_)))); - } -} diff --git a/crates/config/src/schema.rs b/crates/config/src/schema.rs deleted file mode 100644 index 7389b12..0000000 --- a/crates/config/src/schema.rs +++ /dev/null @@ -1,150 +0,0 @@ -use jsonschema::Validator; -use serde_json::Value; -use std::fmt; - -const SCHEMA_JSON: &str = include_str!("../../../assets/xshuttle.schema.json"); - -/// A validation error with path and message. -#[derive(Debug, Clone)] -pub struct ValidationError { - pub path: String, - pub message: String, -} - -impl fmt::Display for ValidationError { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - if self.path.is_empty() { - write!(f, "{}", self.message) - } else { - write!(f, "{}: {}", self.path, self.message) - } - } -} - -/// Result of config validation. -#[derive(Debug)] -pub enum ValidationResult { - Valid, - Invalid(Vec), -} - -/// Error type for config parsing. -#[derive(Debug)] -pub enum ConfigError { - InvalidJson(serde_json::Error), - ValidationFailed(Vec), -} - -impl fmt::Display for ConfigError { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - ConfigError::InvalidJson(e) => write!(f, "invalid JSON: {e}"), - ConfigError::ValidationFailed(errors) => { - writeln!(f, "validation failed:")?; - for error in errors { - writeln!(f, " - {error}")?; - } - Ok(()) - } - } - } -} - -impl std::error::Error for ConfigError { - fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - match self { - ConfigError::InvalidJson(e) => Some(e), - ConfigError::ValidationFailed(_) => None, - } - } -} - -/// Returns the embedded JSON schema as a string. -pub fn schema() -> &'static str { - SCHEMA_JSON -} - -/// Validates a JSON value against the config schema. -/// -/// # Panics -/// -/// Panics if the embedded schema is invalid JSON or not a valid JSON Schema. -/// This should never happen as the schema is compile-time embedded. -pub fn validate(value: &Value) -> ValidationResult { - let schema: Value = - serde_json::from_str(SCHEMA_JSON).expect("embedded schema should be valid JSON"); - - let validator = Validator::new(&schema).expect("embedded schema should be a valid JSON Schema"); - - let errors: Vec = validator - .iter_errors(value) - .map(|e| ValidationError { - path: e.instance_path.to_string(), - message: e.to_string(), - }) - .collect(); - - if errors.is_empty() { - ValidationResult::Valid - } else { - ValidationResult::Invalid(errors) - } -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn test_schema_is_valid_json() { - let schema: Value = serde_json::from_str(schema()).unwrap(); - assert!(schema.is_object()); - assert!(schema.get("$schema").is_some()); - } - - #[test] - fn test_validate_valid_config() { - let config = r#"{"terminal": "kitty", "actions": []}"#; - let value: Value = serde_json::from_str(config).unwrap(); - assert!(matches!(validate(&value), ValidationResult::Valid)); - } - - #[test] - fn test_validate_invalid_action_missing_cmd() { - let config = r#"{"actions": [{"name": "Test"}]}"#; - let value: Value = serde_json::from_str(config).unwrap(); - match validate(&value) { - ValidationResult::Invalid(errors) => { - assert!(!errors.is_empty()); - } - ValidationResult::Valid => panic!("Expected validation error"), - } - } - - #[test] - fn test_validate_invalid_type() { - let config = r#"{"terminal": 123}"#; - let value: Value = serde_json::from_str(config).unwrap(); - assert!(matches!(validate(&value), ValidationResult::Invalid(_))); - } - - #[test] - fn test_validate_unknown_field_rejected() { - let config = r#"{"unknown_field": "value"}"#; - let value: Value = serde_json::from_str(config).unwrap(); - assert!(matches!(validate(&value), ValidationResult::Invalid(_))); - } - - #[test] - fn test_validate_nested_group() { - let config = r#"{ - "actions": [ - {"Production": [ - {"name": "Server", "cmd": "ssh server"} - ]} - ] - }"#; - let value: Value = serde_json::from_str(config).unwrap(); - assert!(matches!(validate(&value), ValidationResult::Valid)); - } -} diff --git a/crates/config/Cargo.toml b/crates/settings/Cargo.toml similarity index 74% rename from crates/config/Cargo.toml rename to crates/settings/Cargo.toml index b1239e7..38bded0 100644 --- a/crates/config/Cargo.toml +++ b/crates/settings/Cargo.toml @@ -1,5 +1,5 @@ [package] -name = "config" +name = "settings" version = "0.0.0" edition.workspace = true license.workspace = true @@ -9,6 +9,8 @@ serde = { workspace = true } serde_json = { workspace = true } jsonschema = { workspace = true } dirs = { workspace = true } +ssh2-config = { workspace = true } +thiserror = { workspace = true } [lints] workspace = true diff --git a/crates/settings/src/error.rs b/crates/settings/src/error.rs new file mode 100644 index 0000000..2614c4a --- /dev/null +++ b/crates/settings/src/error.rs @@ -0,0 +1,35 @@ +use crate::sources::config::ValidationError; +use std::io; +use thiserror::Error; + +/// Error type for settings loading operations. +#[derive(Error, Debug)] +pub enum SettingsError { + /// Home directory not found. + #[error("could not determine home directory")] + NoHomeDir, + + /// Config file I/O error. + #[error("failed to read config: {0}")] + ConfigIo(#[from] io::Error), + + /// Config JSON parse error (not valid JSON at all). + #[error("invalid JSON: {0}")] + ConfigParse(#[from] serde_json::Error), + + /// Config schema validation error. + #[error("config validation failed: {}", format_validation_errors(.0))] + ConfigValidation(Vec), + + /// SSH config parse error (fatal - user should fix their SSH config). + #[error("failed to parse SSH config: {0}")] + SshParse(String), +} + +fn format_validation_errors(errors: &[ValidationError]) -> String { + errors + .iter() + .map(ToString::to_string) + .collect::>() + .join("; ") +} diff --git a/crates/settings/src/lib.rs b/crates/settings/src/lib.rs new file mode 100644 index 0000000..49eca14 --- /dev/null +++ b/crates/settings/src/lib.rs @@ -0,0 +1,9 @@ +mod error; +mod settings; +mod sources; +mod types; + +pub use error::SettingsError; +pub use settings::Settings; +pub use sources::config::{ValidationError, ValidationResult, schema, validate}; +pub use types::{Action, Entry, Group}; diff --git a/crates/settings/src/settings.rs b/crates/settings/src/settings.rs new file mode 100644 index 0000000..354cd3e --- /dev/null +++ b/crates/settings/src/settings.rs @@ -0,0 +1,90 @@ +use crate::error::SettingsError; +use crate::sources::{config, ssh}; +use crate::types::Entry; +use std::io; +use std::path::PathBuf; + +/// Complete application settings loaded from all sources. +#[derive(Debug, Clone)] +pub struct Settings { + /// Terminal emulator to use for commands. + pub terminal: String, + /// Editor for opening config files. + pub editor: String, + /// Actions from ~/.xshuttle.json. + pub actions: Vec, + /// SSH hosts from ~/.ssh/config. + pub hosts: Vec, +} + +impl Settings { + /// Default terminal value when not specified in config. + pub const DEFAULT_TERMINAL: &'static str = "default"; + /// Default editor value when not specified in config. + pub const DEFAULT_EDITOR: &'static str = "default"; + + /// Load settings from all sources. + /// + /// This loads: + /// - Configuration from `~/.xshuttle.json` (uses defaults if missing) + /// - SSH hosts from `~/.ssh/config` (empty if file doesn't exist) + /// + /// Warnings for non-fatal issues are logged via the `log` crate. + /// + /// # Errors + /// + /// Returns an error if: + /// - Home directory cannot be determined + /// - Config file exists but is invalid JSON or fails validation + /// - SSH config file exists but cannot be parsed + pub fn load() -> Result { + let config = config::load()?.unwrap_or_default(); + + Ok(Settings { + terminal: config + .terminal + .unwrap_or_else(|| Self::DEFAULT_TERMINAL.to_string()), + editor: config + .editor + .unwrap_or_else(|| Self::DEFAULT_EDITOR.to_string()), + actions: config.actions.unwrap_or_default(), + hosts: ssh::parse_ssh_config()?, + }) + } + + /// Get the path to the main config file. + pub fn config_path() -> Option { + config::config_path() + } + + /// Ensure the config file exists, creating a default one if missing. + /// + /// # Errors + /// + /// Returns an error if the home directory cannot be determined or if + /// writing the default config file fails. + pub fn ensure_config_exists() -> Result { + config::ensure_config_exists().map_err(|e| match e { + SettingsError::ConfigIo(io_err) => io_err, + SettingsError::NoHomeDir => io::Error::new( + io::ErrorKind::NotFound, + "could not determine home directory", + ), + _ => io::Error::other(e.to_string()), + }) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_config_path_returns_some() { + // This test assumes a home directory exists + let path = Settings::config_path(); + if let Some(p) = path { + assert!(p.ends_with(".xshuttle.json")); + } + } +} diff --git a/crates/settings/src/sources/config.rs b/crates/settings/src/sources/config.rs new file mode 100644 index 0000000..73563c4 --- /dev/null +++ b/crates/settings/src/sources/config.rs @@ -0,0 +1,349 @@ +use crate::error::SettingsError; +use crate::types::Entry; +use jsonschema::Validator; +use serde::Deserialize; +use serde_json::Value; +use std::fmt; +use std::fs; +use std::path::{Path, PathBuf}; + +const DEFAULT_JSON: &str = include_str!("../../../../assets/xshuttle.default.json"); +const SCHEMA_JSON: &str = include_str!("../../../../assets/xshuttle.schema.json"); + +/// Internal configuration structure for JSON deserialization. +/// All fields are optional - defaults are applied by the Settings struct. +#[derive(Debug, Default, Deserialize)] +pub(crate) struct ConfigContent { + pub terminal: Option, + pub editor: Option, + pub actions: Option>, +} + +// ============================================================================ +// Schema Validation +// ============================================================================ + +/// A validation error with path and message. +#[derive(Debug, Clone)] +pub struct ValidationError { + pub path: String, + pub message: String, +} + +impl fmt::Display for ValidationError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + if self.path.is_empty() { + write!(f, "{}", self.message) + } else { + write!(f, "{}: {}", self.path, self.message) + } + } +} + +/// Result of config validation. +#[derive(Debug)] +pub enum ValidationResult { + Valid, + Invalid(Vec), +} + +/// Returns the embedded JSON schema as a string. +pub fn schema() -> &'static str { + SCHEMA_JSON +} + +/// Validates a JSON value against the config schema. +/// +/// # Panics +/// +/// Panics if the embedded schema is invalid JSON or not a valid JSON Schema. +/// This should never happen as the schema is compile-time embedded. +pub fn validate(value: &Value) -> ValidationResult { + let schema: Value = + serde_json::from_str(SCHEMA_JSON).expect("embedded schema should be valid JSON"); + + let validator = Validator::new(&schema).expect("embedded schema should be a valid JSON Schema"); + + let errors: Vec = validator + .iter_errors(value) + .map(|e| ValidationError { + path: e.instance_path.to_string(), + message: e.to_string(), + }) + .collect(); + + if errors.is_empty() { + ValidationResult::Valid + } else { + ValidationResult::Invalid(errors) + } +} + +// ============================================================================ +// Config Loading +// ============================================================================ + +/// Returns the default config file path (~/.xshuttle.json). +pub fn config_path() -> Option { + dirs::home_dir().map(|home| home.join(".xshuttle.json")) +} + +/// Ensures the config file exists, creating a default one if missing. +/// +/// # Errors +/// +/// Returns an error if the home directory cannot be determined or if +/// writing the default config file fails. +pub fn ensure_config_exists() -> Result { + let path = config_path().ok_or(SettingsError::NoHomeDir)?; + + if !path.exists() { + fs::write(&path, DEFAULT_JSON)?; + eprintln!("Created default config at {}", path.display()); + } + + Ok(path) +} + +/// Loads config content from a string. +/// +/// Validates against the schema first, then deserializes. +/// +/// # Errors +/// +/// Returns an error if the JSON is invalid or fails schema validation. +pub fn load_from_str(s: &str) -> Result { + let value: Value = serde_json::from_str(s).map_err(SettingsError::ConfigParse)?; + + // Validate against schema + if let ValidationResult::Invalid(errors) = validate(&value) { + return Err(SettingsError::ConfigValidation(errors)); + } + + // Schema is valid, deserialize + serde_json::from_value(value).map_err(SettingsError::ConfigParse) +} + +/// Loads config content from a specific path. +/// +/// # Errors +/// +/// Returns an error if the file cannot be read or the JSON is unparseable. +pub fn load_from_path(path: &Path) -> Result { + let contents = fs::read_to_string(path)?; + load_from_str(&contents) +} + +/// Loads config content from the default path (~/.xshuttle.json). +/// +/// Returns `None` if the config file doesn't exist (caller should use defaults). +/// +/// # Errors +/// +/// Returns an error if the home directory cannot be determined or the JSON is unparseable. +pub fn load() -> Result, SettingsError> { + let path = config_path().ok_or(SettingsError::NoHomeDir)?; + + if !path.exists() { + return Ok(None); + } + + load_from_path(&path).map(Some) +} + +#[cfg(test)] +mod tests { + use super::*; + + // Schema validation tests + #[test] + fn test_schema_is_valid_json() { + let schema_value: Value = serde_json::from_str(schema()).unwrap(); + assert!(schema_value.is_object()); + assert!(schema_value.get("$schema").is_some()); + } + + #[test] + fn test_validate_valid_config() { + let config = r#"{"terminal": "kitty", "actions": []}"#; + let value: Value = serde_json::from_str(config).unwrap(); + assert!(matches!(validate(&value), ValidationResult::Valid)); + } + + #[test] + fn test_validate_invalid_action_missing_cmd() { + let config = r#"{"actions": [{"name": "Test"}]}"#; + let value: Value = serde_json::from_str(config).unwrap(); + match validate(&value) { + ValidationResult::Invalid(errors) => { + assert!(!errors.is_empty()); + } + ValidationResult::Valid => panic!("Expected validation error"), + } + } + + #[test] + fn test_validate_invalid_type() { + let config = r#"{"terminal": 123}"#; + let value: Value = serde_json::from_str(config).unwrap(); + assert!(matches!(validate(&value), ValidationResult::Invalid(_))); + } + + #[test] + fn test_validate_unknown_field_rejected() { + let config = r#"{"unknown_field": "value"}"#; + let value: Value = serde_json::from_str(config).unwrap(); + assert!(matches!(validate(&value), ValidationResult::Invalid(_))); + } + + #[test] + fn test_validate_nested_group() { + let config = r#"{ + "actions": [ + {"Production": [ + {"name": "Server", "cmd": "ssh server"} + ]} + ] + }"#; + let value: Value = serde_json::from_str(config).unwrap(); + assert!(matches!(validate(&value), ValidationResult::Valid)); + } + + // Config loading tests + #[test] + fn test_load_from_str_empty() { + let content = load_from_str("{}").unwrap(); + assert!(content.terminal.is_none()); + assert!(content.editor.is_none()); + assert!(content.actions.is_none()); + } + + #[test] + fn test_load_from_str_with_terminal() { + let content = load_from_str(r#"{"terminal": "kitty"}"#).unwrap(); + assert_eq!(content.terminal.as_deref(), Some("kitty")); + } + + #[test] + fn test_invalid_unknown_fields() { + // Unknown fields cause validation error + let result = load_from_str(r#"{"terminal": "alacritty", "unknown": true}"#); + assert!(matches!(result, Err(SettingsError::ConfigValidation(_)))); + } + + #[test] + fn test_invalid_terminal_type() { + // Invalid type for terminal causes validation error + let result = load_from_str(r#"{"terminal": 123}"#); + assert!(matches!(result, Err(SettingsError::ConfigValidation(_)))); + } + + #[test] + fn test_invalid_editor_type() { + // Invalid type for editor causes validation error + let result = load_from_str(r#"{"editor": ["vim", "nano"]}"#); + assert!(matches!(result, Err(SettingsError::ConfigValidation(_)))); + } + + #[test] + fn test_invalid_actions_type() { + // Invalid type for actions causes validation error + let result = load_from_str(r#"{"actions": "not an array"}"#); + assert!(matches!(result, Err(SettingsError::ConfigValidation(_)))); + } + + #[test] + fn test_invalid_action_entry() { + // Invalid action entries cause validation error + let result = load_from_str( + r#"{"actions": [ + {"name": "Valid", "cmd": "echo valid"}, + {"name": "Missing cmd"} + ]}"#, + ); + assert!(matches!(result, Err(SettingsError::ConfigValidation(_)))); + } + + #[test] + fn test_load_from_str_invalid_json() { + let result = load_from_str("not valid json"); + assert!(matches!(result, Err(SettingsError::ConfigParse(_)))); + } + + #[test] + fn test_flat_actions() { + let content = + load_from_str(r#"{"actions": [{"name": "Test", "cmd": "echo hello"}]}"#).unwrap(); + let entries = content.actions.unwrap(); + assert_eq!(entries.len(), 1); + match &entries[0] { + Entry::Action(c) => { + assert_eq!(c.name, "Test"); + assert_eq!(c.cmd, "echo hello"); + } + Entry::Group(_) => panic!("Expected Action"), + } + } + + #[test] + fn test_nested_actions() { + let content = load_from_str( + r#"{ + "actions": [ + {"name": "Top Level", "cmd": "echo top"}, + {"Production": [ + {"name": "Server 1", "cmd": "ssh server1"} + ]} + ] + }"#, + ) + .unwrap(); + let entries = content.actions.unwrap(); + assert_eq!(entries.len(), 2); + match &entries[0] { + Entry::Action(c) => assert_eq!(c.name, "Top Level"), + Entry::Group(_) => panic!("Expected Action"), + } + match &entries[1] { + Entry::Group(group) => { + assert_eq!(group.name, "Production"); + assert_eq!(group.entries.len(), 1); + } + Entry::Action(_) => panic!("Expected Group"), + } + } + + #[test] + fn test_deeply_nested_actions() { + let content = load_from_str( + r#"{ + "actions": [ + {"Level1": [ + {"Level2": [ + {"name": "Deep", "cmd": "echo deep"} + ]} + ]} + ] + }"#, + ) + .unwrap(); + let entries = content.actions.unwrap(); + assert_eq!(entries.len(), 1); + match &entries[0] { + Entry::Group(l1) => { + assert_eq!(l1.name, "Level1"); + match &l1.entries[0] { + Entry::Group(l2) => { + assert_eq!(l2.name, "Level2"); + match &l2.entries[0] { + Entry::Action(c) => assert_eq!(c.name, "Deep"), + Entry::Group(_) => panic!("Expected Action at level 3"), + } + } + Entry::Action(_) => panic!("Expected Group at level 2"), + } + } + Entry::Action(_) => panic!("Expected Group at level 1"), + } + } +} diff --git a/crates/settings/src/sources/mod.rs b/crates/settings/src/sources/mod.rs new file mode 100644 index 0000000..a023e8e --- /dev/null +++ b/crates/settings/src/sources/mod.rs @@ -0,0 +1,2 @@ +pub(crate) mod config; +pub(crate) mod ssh; diff --git a/crates/settings/src/sources/ssh.rs b/crates/settings/src/sources/ssh.rs new file mode 100644 index 0000000..48fa26d --- /dev/null +++ b/crates/settings/src/sources/ssh.rs @@ -0,0 +1,54 @@ +use crate::error::SettingsError; +use ssh2_config::{ParseRule, SshConfig}; +use std::path::PathBuf; + +/// Returns the default SSH config file path (~/.ssh/config). +fn ssh_config_path() -> Option { + dirs::home_dir().map(|home| home.join(".ssh").join("config")) +} + +/// Parses SSH config file and returns host names. +/// +/// Returns an empty list if the SSH config file doesn't exist. +/// Returns an error if the file exists but cannot be parsed (user should fix it). +/// +/// # Errors +/// +/// Returns `SettingsError::SshParse` if the SSH config file exists but is malformed. +pub fn parse_ssh_config() -> Result, SettingsError> { + let Some(path) = ssh_config_path() else { + return Err(SettingsError::NoHomeDir); + }; + + // If the file doesn't exist, return empty list (not an error) + if !path.exists() { + return Ok(Vec::new()); + } + + // File exists, so parse errors are fatal + let config = SshConfig::parse_default_file(ParseRule::ALLOW_UNKNOWN_FIELDS) + .map_err(|e| SettingsError::SshParse(e.to_string()))?; + + let mut hosts = Vec::new(); + + for host in config.get_hosts() { + for clause in &host.pattern { + let name = clause.pattern.as_str(); + + // Skip wildcards and patterns + if name.contains('*') || name.contains('?') || name == "!" { + continue; + } + + // Skip negated patterns + if clause.negated { + continue; + } + + hosts.push(name.to_string()); + } + } + + hosts.sort_by_key(|a| a.to_lowercase()); + Ok(hosts) +} diff --git a/crates/config/src/types.rs b/crates/settings/src/types.rs similarity index 62% rename from crates/config/src/types.rs rename to crates/settings/src/types.rs index dbd0d91..149905f 100644 --- a/crates/config/src/types.rs +++ b/crates/settings/src/types.rs @@ -3,6 +3,7 @@ use serde::ser::SerializeMap; use serde::{Deserialize, Serialize}; use std::fmt; +/// A single executable menu item with a display name and command. #[derive(Debug, Clone, Deserialize, Serialize)] pub struct Action { pub name: String, @@ -65,7 +66,7 @@ impl<'de> Deserialize<'de> for Group { } } -/// An entry in the menu - either a action or a group. +/// An entry in the menu - either an action or a group. #[derive(Debug, Clone, Deserialize, Serialize)] #[serde(untagged)] pub enum Entry { @@ -73,45 +74,45 @@ pub enum Entry { Group(Group), } -#[derive(Debug, Deserialize, Serialize)] -pub struct Config { - #[serde(default = "default_terminal")] - pub terminal: String, - - #[serde(default = "default_editor")] - pub editor: String, - - #[serde(rename = "actions", default)] - pub entries: Vec, -} - -fn default_terminal() -> String { - "default".to_string() -} +#[cfg(test)] +mod tests { + use super::*; -fn default_editor() -> String { - "default".to_string() -} + #[test] + fn test_action_serialization() { + let action = Action { + name: "Test".to_string(), + cmd: "echo hello".to_string(), + }; + let json = serde_json::to_string(&action).unwrap(); + assert!(json.contains("Test")); + assert!(json.contains("echo hello")); + } -impl Default for Config { - fn default() -> Self { - Self { - terminal: default_terminal(), - editor: default_editor(), - entries: Vec::new(), - } + #[test] + fn test_group_serialization() { + let group = Group { + name: "Production".to_string(), + entries: vec![Entry::Action(Action { + name: "Server".to_string(), + cmd: "ssh prod".to_string(), + })], + }; + let json = serde_json::to_string(&group).unwrap(); + assert!(json.contains("Production")); } -} -#[cfg(test)] -mod tests { - use super::*; + #[test] + fn test_entry_untagged_action() { + let json = r#"{"name": "Test", "cmd": "echo"}"#; + let entry: Entry = serde_json::from_str(json).unwrap(); + assert!(matches!(entry, Entry::Action(_))); + } #[test] - fn test_default_config() { - let config = Config::default(); - assert_eq!(config.terminal, "default"); - assert_eq!(config.editor, "default"); - assert!(config.entries.is_empty()); + fn test_entry_untagged_group() { + let json = r#"{"MyGroup": [{"name": "Test", "cmd": "echo"}]}"#; + let entry: Entry = serde_json::from_str(json).unwrap(); + assert!(matches!(entry, Entry::Group(_))); } } diff --git a/crates/ssh/Cargo.toml b/crates/ssh/Cargo.toml deleted file mode 100644 index 1dcdd57..0000000 --- a/crates/ssh/Cargo.toml +++ /dev/null @@ -1,11 +0,0 @@ -[package] -name = "ssh" -version = "0.0.0" -edition.workspace = true -license.workspace = true - -[dependencies] -ssh2-config = { workspace = true } - -[lints] -workspace = true diff --git a/crates/ssh/src/lib.rs b/crates/ssh/src/lib.rs deleted file mode 100644 index e741319..0000000 --- a/crates/ssh/src/lib.rs +++ /dev/null @@ -1,31 +0,0 @@ -use ssh2_config::{ParseRule, SshConfig}; - -/// Parses SSH config file and returns host names. -pub fn parse_ssh_config() -> Vec { - let Ok(config) = SshConfig::parse_default_file(ParseRule::ALLOW_UNKNOWN_FIELDS) else { - return Vec::new(); - }; - - let mut hosts = Vec::new(); - - for host in config.get_hosts() { - for clause in &host.pattern { - let name = clause.pattern.as_str(); - - // Skip wildcards and patterns - if name.contains('*') || name.contains('?') || name == "!" { - continue; - } - - // Skip negated patterns - if clause.negated { - continue; - } - - hosts.push(name.to_string()); - } - } - - hosts.sort_by_key(|a| a.to_lowercase()); - hosts -} diff --git a/crates/tray/Cargo.toml b/crates/tray/Cargo.toml index 13af584..4648c5d 100644 --- a/crates/tray/Cargo.toml +++ b/crates/tray/Cargo.toml @@ -5,7 +5,7 @@ edition.workspace = true license.workspace = true [dependencies] -config = { workspace = true } +settings = { workspace = true } tray-icon = { workspace = true } image = { workspace = true } diff --git a/crates/tray/src/lib.rs b/crates/tray/src/lib.rs index 75284e3..55e8e05 100644 --- a/crates/tray/src/lib.rs +++ b/crates/tray/src/lib.rs @@ -1,8 +1,8 @@ use std::collections::HashMap; use std::fmt; -use config::Entry; use image::load_from_memory; +use settings::{Entry, Settings}; use tray_icon::menu::{MenuItem, PredefinedMenuItem, Submenu}; use tray_icon::{Icon, TrayIcon, TrayIconBuilder}; @@ -73,32 +73,32 @@ fn load_icon() -> Icon { Icon::from_rgba(img.into_raw(), width, height).expect("Failed to create icon") } -/// Builds a menu from config entries and SSH hosts. +/// Builds a menu from settings. /// /// Returns the menu and a map from menu IDs to commands. /// /// # Panics /// /// Panics if menu items cannot be appended to the menu. -pub fn build_menu(entries: &[Entry], hosts: &[String]) -> (Menu, HashMap) { +pub fn build_menu(settings: &Settings) -> (Menu, HashMap) { let menu = Menu::new(); let mut menu_id_map = HashMap::new(); let mut id_counter = 0usize; - build_entries(&menu, entries, &mut menu_id_map, &mut id_counter); + build_entries(&menu, &settings.actions, &mut menu_id_map, &mut id_counter); - if !entries.is_empty() && !hosts.is_empty() { + if !settings.actions.is_empty() && !settings.hosts.is_empty() { menu.append(&PredefinedMenuItem::separator()).unwrap(); } - for (index, host) in hosts.iter().enumerate() { + for (index, host) in settings.hosts.iter().enumerate() { let menu_id = format!("ssh_{index}"); menu_id_map.insert(menu_id.clone(), format!("ssh {host}")); let item = MenuItem::with_id(menu_id, host, true, None); menu.append(&item).expect("Failed to append menu item"); } - if !hosts.is_empty() { + if !settings.hosts.is_empty() { menu.append(&PredefinedMenuItem::separator()).unwrap(); } diff --git a/src/xshuttle.rs b/src/xshuttle.rs index 0bc511f..b3f7488 100644 --- a/src/xshuttle.rs +++ b/src/xshuttle.rs @@ -1,7 +1,6 @@ use std::collections::HashMap; -use config::{Config, config_path, ensure_config_exists, load}; -use ssh::parse_ssh_config; +use settings::Settings; use terminal::Terminal; use tray::{MENU_ID_CONFIGURE, MENU_ID_QUIT, MENU_ID_RELOAD, Menu, MenuEvent, Tray, build_menu}; @@ -12,14 +11,14 @@ pub enum UserEvent { #[derive(Default)] pub struct Application { - config: Option, + settings: Option, tray: Tray, menu_id_map: HashMap, } impl Application { pub fn init(&mut self) { - if let Err(e) = ensure_config_exists() { + if let Err(e) = Settings::ensure_config_exists() { eprintln!("Warning: Could not ensure config exists: {e}"); } @@ -28,16 +27,21 @@ impl Application { } fn build(&mut self) -> Menu { - let config = load().unwrap_or_else(|e| { - eprintln!("Warning: {e}"); - Config::default() - }); + let settings = match Settings::load() { + Ok(settings) => settings, + Err(e) => { + eprintln!("Error loading settings: {e}"); + // Return empty menu if settings fail to load + self.settings = None; + self.menu_id_map.clear(); + return Menu::new(); + } + }; - let hosts = parse_ssh_config(); - let (menu, menu_id_map) = build_menu(&config.entries, &hosts); + let (menu, menu_id_map) = build_menu(&settings); self.menu_id_map = menu_id_map; - self.config = Some(config); + self.settings = Some(settings); menu } @@ -62,9 +66,9 @@ impl Application { if let Some(command) = self.menu_id_map.get(menu_id) { let terminal = self - .config + .settings .as_ref() - .map(|c| Terminal::from(c.terminal.as_str())) + .map(|s| Terminal::from(s.terminal.as_str())) .unwrap_or_default(); if let Err(e) = terminal.launch(command) { @@ -76,15 +80,15 @@ impl Application { } fn configure(&self) { - let Some(path) = config_path() else { + let Some(path) = Settings::config_path() else { eprintln!("Error: Could not determine config path"); return; }; let editor = self - .config + .settings .as_ref() - .map_or("default", |c| c.editor.as_str()); + .map_or("default", |s| s.editor.as_str()); let path_display = path.display(); let result = match editor { @@ -92,9 +96,9 @@ impl Application { editor if is_terminal_editor(editor) => { let cmd = format!("{editor} {path_display}"); let terminal = self - .config + .settings .as_ref() - .map(|c| Terminal::from(c.terminal.as_str())) + .map(|s| Terminal::from(s.terminal.as_str())) .unwrap_or_default(); terminal.launch(&cmd).map_err(std::io::Error::other) } From f7597fd8c9f29de11ec700ccde049b37a04ed7ad Mon Sep 17 00:00:00 2001 From: Andreas Penz Date: Sat, 10 Jan 2026 18:17:46 +0100 Subject: [PATCH 2/3] create node structure for actions and hosts --- crates/settings/src/host.rs | 40 +++ crates/settings/src/lib.rs | 4 + crates/settings/src/nodes.rs | 497 ++++++++++++++++++++++++++++++++ crates/settings/src/settings.rs | 17 +- crates/tray/src/lib.rs | 86 +++--- src/xshuttle.rs | 42 ++- 6 files changed, 620 insertions(+), 66 deletions(-) create mode 100644 crates/settings/src/host.rs create mode 100644 crates/settings/src/nodes.rs diff --git a/crates/settings/src/host.rs b/crates/settings/src/host.rs new file mode 100644 index 0000000..5a3dc8f --- /dev/null +++ b/crates/settings/src/host.rs @@ -0,0 +1,40 @@ +//! SSH host entry type. + +/// An SSH host entry. +/// +/// Wraps a hostname string for type safety and future extensibility. +#[derive(Debug, Clone)] +pub struct Host { + /// The SSH hostname to connect to. + pub hostname: String, +} + +impl Host { + /// Returns the command to execute: `ssh {hostname}`. + #[must_use] + pub fn command(&self) -> String { + format!("ssh {}", self.hostname) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_host_command() { + let host = Host { + hostname: "prod-server".into(), + }; + assert_eq!(host.command(), "ssh prod-server"); + } + + #[test] + fn test_host_clone() { + let host = Host { + hostname: "staging".into(), + }; + let cloned = host.clone(); + assert_eq!(host.hostname, cloned.hostname); + } +} diff --git a/crates/settings/src/lib.rs b/crates/settings/src/lib.rs index 49eca14..d8e6108 100644 --- a/crates/settings/src/lib.rs +++ b/crates/settings/src/lib.rs @@ -1,9 +1,13 @@ mod error; +mod host; +mod nodes; mod settings; mod sources; mod types; pub use error::SettingsError; +pub use host::Host; +pub use nodes::{Node, NodeId, Nodes}; pub use settings::Settings; pub use sources::config::{ValidationError, ValidationResult, schema, validate}; pub use types::{Action, Entry, Group}; diff --git a/crates/settings/src/nodes.rs b/crates/settings/src/nodes.rs new file mode 100644 index 0000000..ab4cc0d --- /dev/null +++ b/crates/settings/src/nodes.rs @@ -0,0 +1,497 @@ +//! Indexed node container providing O(1) lookup by ID. +//! +//! This module provides the [`Nodes`] container which stores leaf nodes +//! with assigned IDs while preserving tree structure for menu building. + +use crate::host::Host; +use crate::types::{Action, Entry, Group}; +use std::fmt; + +/// Unique identifier for a leaf node within a [`Nodes`] container. +/// +/// Created during container construction. Use with [`Nodes::get()`] +/// to retrieve the associated value. +/// +/// # Display Format +/// +/// Formats as `"node_{index}"` for use as menu item IDs. +#[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)] +pub struct NodeId(usize); + +impl NodeId { + /// Creates a new `NodeId` from an index. + /// + /// This is used internally during construction and for parsing menu IDs. + #[must_use] + pub fn from_index(index: usize) -> Self { + Self(index) + } + + /// Returns the underlying index. + /// + /// Use this when parsing menu IDs back to `NodeId`. + #[must_use] + pub fn index(&self) -> usize { + self.0 + } +} + +impl fmt::Display for NodeId { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "node_{}", self.0) + } +} + +/// A node in the indexed tree. +/// +/// Use pattern matching to distinguish leaves from groups when +/// building menus or iterating the tree structure. +#[derive(Debug, Clone)] +pub enum Node { + /// A leaf node with its value and assigned ID. + Leaf { + /// Unique identifier for this leaf. + id: NodeId, + /// The leaf value. + value: T, + }, + /// A named group containing nested nodes. + Group { + /// Display name for the submenu. + name: String, + /// Child nodes. + children: Vec>, + }, +} + +impl Node { + /// Returns the ID if this is a `Leaf`, `None` for `Group`. + #[must_use] + pub fn id(&self) -> Option { + match self { + Self::Leaf { id, .. } => Some(*id), + Self::Group { .. } => None, + } + } + + /// Returns `true` if this is a `Leaf` variant. + #[must_use] + pub fn is_leaf(&self) -> bool { + matches!(self, Self::Leaf { .. }) + } + + /// Returns `true` if this is a `Group` variant. + #[must_use] + pub fn is_group(&self) -> bool { + matches!(self, Self::Group { .. }) + } +} + +/// Generic container providing O(1) lookup by [`NodeId`]. +/// +/// Built from config data during settings load. Provides both flat +/// iteration (for lookup) and tree iteration (for menu building). +/// +/// # Type Parameter +/// +/// `T: Clone` - The leaf value type. Clone is required because values +/// are stored in both the tree structure and the flat lookup table. +#[derive(Debug, Clone)] +pub struct Nodes { + /// Tree structure for menu building. + tree: Vec>, + /// Flat list of leaves for O(1) lookup by `NodeId`. + leaves: Vec, +} + +impl Nodes { + /// Build from config entries, assigning IDs during depth-first traversal. + #[must_use] + pub fn from_entries(entries: Vec) -> Self { + let mut leaves = Vec::new(); + let tree = entries + .into_iter() + .map(|e| Self::convert_entry(e, &mut leaves)) + .collect(); + Self { tree, leaves } + } + + fn convert_entry(entry: Entry, leaves: &mut Vec) -> Node { + match entry { + Entry::Action(action) => { + let id = NodeId::from_index(leaves.len()); + leaves.push(action.clone()); + Node::Leaf { id, value: action } + } + Entry::Group(Group { name, entries }) => { + let children = entries + .into_iter() + .map(|e| Self::convert_entry(e, leaves)) + .collect(); + Node::Group { name, children } + } + } + } +} + +impl Nodes { + /// Build from hostnames (flat, no groups). + #[must_use] + pub fn from_hostnames(hostnames: Vec) -> Self { + let mut leaves = Vec::new(); + let tree = hostnames + .into_iter() + .enumerate() + .map(|(i, hostname)| { + let host = Host { hostname }; + leaves.push(host.clone()); + Node::Leaf { + id: NodeId::from_index(i), + value: host, + } + }) + .collect(); + Self { tree, leaves } + } +} + +impl Nodes { + /// O(1) lookup by ID. + /// + /// Returns `Some(&T)` if the ID is valid, `None` otherwise. + #[must_use] + pub fn get(&self, id: NodeId) -> Option<&T> { + self.leaves.get(id.0) + } + + /// Iterate all leaf values with their IDs (flat, depth-first order). + pub fn iter(&self) -> impl Iterator + '_ { + self.leaves + .iter() + .enumerate() + .map(|(i, v)| (NodeId::from_index(i), v)) + } + + /// Access tree structure for menu building. + #[must_use] + pub fn nodes(&self) -> &[Node] { + &self.tree + } + + /// Returns the number of leaf nodes. + #[must_use] + pub fn len(&self) -> usize { + self.leaves.len() + } + + /// Returns `true` if there are no leaf nodes. + #[must_use] + pub fn is_empty(&self) -> bool { + self.leaves.is_empty() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + // === User Story 1: Menu Item Click Lookup (P1) === + + #[test] + fn test_action_lookup_by_id() { + let entries = vec![ + Entry::Action(Action { + name: "Deploy".into(), + cmd: "deploy.sh".into(), + }), + Entry::Group(Group { + name: "Servers".into(), + entries: vec![Entry::Action(Action { + name: "Prod".into(), + cmd: "ssh prod".into(), + })], + }), + ]; + + let nodes = Nodes::from_entries(entries); + + // O(1) lookup by ID + let id0 = NodeId::from_index(0); + let action0 = nodes.get(id0).expect("should find action at index 0"); + assert_eq!(action0.name, "Deploy"); + assert_eq!(action0.cmd, "deploy.sh"); + + let id1 = NodeId::from_index(1); + let action1 = nodes.get(id1).expect("should find action at index 1"); + assert_eq!(action1.name, "Prod"); + assert_eq!(action1.cmd, "ssh prod"); + } + + #[test] + fn test_host_lookup_by_id() { + let hostnames = vec!["staging".into(), "prod".into(), "dev".into()]; + let nodes = Nodes::from_hostnames(hostnames); + + let id0 = NodeId::from_index(0); + assert_eq!(nodes.get(id0).unwrap().hostname, "staging"); + + let id1 = NodeId::from_index(1); + assert_eq!(nodes.get(id1).unwrap().hostname, "prod"); + + let id2 = NodeId::from_index(2); + assert_eq!(nodes.get(id2).unwrap().hostname, "dev"); + } + + #[test] + fn test_invalid_id_returns_none() { + let entries = vec![Entry::Action(Action { + name: "Test".into(), + cmd: "test".into(), + })]; + let nodes = Nodes::from_entries(entries); + + // ID beyond the valid range should return None + let invalid_id = NodeId::from_index(999); + assert!(nodes.get(invalid_id).is_none()); + } + + // === User Story 2: Menu Building with IDs (P2) === + + #[test] + fn test_iter_yields_ids_with_values() { + let entries = vec![ + Entry::Action(Action { + name: "First".into(), + cmd: "first".into(), + }), + Entry::Action(Action { + name: "Second".into(), + cmd: "second".into(), + }), + ]; + + let nodes = Nodes::from_entries(entries); + let items: Vec<_> = nodes.iter().collect(); + + assert_eq!(items.len(), 2); + assert_eq!(items[0].0.index(), 0); + assert_eq!(items[0].1.name, "First"); + assert_eq!(items[1].0.index(), 1); + assert_eq!(items[1].1.name, "Second"); + } + + #[test] + fn test_iter_ids_are_stable() { + let entries = vec![ + Entry::Action(Action { + name: "A".into(), + cmd: "a".into(), + }), + Entry::Action(Action { + name: "B".into(), + cmd: "b".into(), + }), + ]; + + let nodes = Nodes::from_entries(entries); + + // First iteration + let first: Vec<_> = nodes.iter().map(|(id, _)| id.index()).collect(); + + // Second iteration + let second: Vec<_> = nodes.iter().map(|(id, _)| id.index()).collect(); + + // IDs should be identical + assert_eq!(first, second); + } + + #[test] + fn test_independent_id_spaces() { + let actions = vec![Entry::Action(Action { + name: "Action".into(), + cmd: "cmd".into(), + })]; + let hosts = vec!["host1".into()]; + + let action_nodes = Nodes::from_entries(actions); + let host_nodes = Nodes::from_hostnames(hosts); + + // Both start at index 0 + let action_ids: Vec<_> = action_nodes.iter().map(|(id, _)| id.index()).collect(); + let host_ids: Vec<_> = host_nodes.iter().map(|(id, _)| id.index()).collect(); + + assert_eq!(action_ids, vec![0]); + assert_eq!(host_ids, vec![0]); + } + + // === User Story 3: Tree Structure Preservation (P3) === + + #[test] + fn test_nodes_preserves_tree_structure() { + let entries = vec![ + Entry::Action(Action { + name: "Root".into(), + cmd: "root".into(), + }), + Entry::Group(Group { + name: "SubMenu".into(), + entries: vec![Entry::Action(Action { + name: "Child".into(), + cmd: "child".into(), + })], + }), + ]; + + let nodes = Nodes::from_entries(entries); + let tree = nodes.nodes(); + + assert_eq!(tree.len(), 2); + + // First is a leaf + assert!(tree[0].is_leaf()); + if let Node::Leaf { value, .. } = &tree[0] { + assert_eq!(value.name, "Root"); + } + + // Second is a group + assert!(tree[1].is_group()); + if let Node::Group { name, children } = &tree[1] { + assert_eq!(name, "SubMenu"); + assert_eq!(children.len(), 1); + assert!(children[0].is_leaf()); + } + } + + #[test] + fn test_deeply_nested_groups() { + let entries = vec![Entry::Group(Group { + name: "Level1".into(), + entries: vec![Entry::Group(Group { + name: "Level2".into(), + entries: vec![Entry::Group(Group { + name: "Level3".into(), + entries: vec![Entry::Action(Action { + name: "Deep".into(), + cmd: "deep".into(), + })], + })], + })], + })]; + + let nodes = Nodes::from_entries(entries); + let tree = nodes.nodes(); + + // Navigate to the deeply nested action + let level1 = &tree[0]; + assert!(level1.is_group()); + + if let Node::Group { children, .. } = level1 { + let level2 = &children[0]; + assert!(level2.is_group()); + + if let Node::Group { children, .. } = level2 { + let level3 = &children[0]; + assert!(level3.is_group()); + + if let Node::Group { children, .. } = level3 { + let deep = &children[0]; + assert!(deep.is_leaf()); + if let Node::Leaf { value, .. } = deep { + assert_eq!(value.name, "Deep"); + } + } + } + } + + // Still only 1 leaf in flat iteration + assert_eq!(nodes.len(), 1); + } + + #[test] + fn test_flat_hosts_no_groups() { + let hosts = vec!["h1".into(), "h2".into(), "h3".into()]; + let nodes = Nodes::from_hostnames(hosts); + + // All entries at root level are leaves + for node in nodes.nodes() { + assert!(node.is_leaf()); + assert!(!node.is_group()); + } + + assert_eq!(nodes.len(), 3); + } + + // === Edge Cases === + + #[test] + fn test_empty_container() { + let actions: Nodes = Nodes::from_entries(vec![]); + let hosts: Nodes = Nodes::from_hostnames(vec![]); + + assert!(actions.is_empty()); + assert!(hosts.is_empty()); + assert_eq!(actions.len(), 0); + assert_eq!(hosts.len(), 0); + assert!(actions.get(NodeId::from_index(0)).is_none()); + } + + #[test] + fn test_duplicate_names_unique_ids() { + let entries = vec![ + Entry::Action(Action { + name: "Same".into(), + cmd: "cmd1".into(), + }), + Entry::Action(Action { + name: "Same".into(), + cmd: "cmd2".into(), + }), + ]; + + let nodes = Nodes::from_entries(entries); + + // Same names but different IDs + let items: Vec<_> = nodes.iter().collect(); + assert_eq!(items[0].0.index(), 0); + assert_eq!(items[1].0.index(), 1); + assert_eq!(items[0].1.name, items[1].1.name); // Same name + assert_ne!(items[0].1.cmd, items[1].1.cmd); // Different cmd + } + + #[test] + fn test_empty_group() { + let entries = vec![Entry::Group(Group { + name: "EmptyGroup".into(), + entries: vec![], + })]; + + let nodes = Nodes::from_entries(entries); + + // Empty group still exists in tree + assert_eq!(nodes.nodes().len(), 1); + assert!(nodes.nodes()[0].is_group()); + + // But no leaves + assert!(nodes.is_empty()); + assert_eq!(nodes.len(), 0); + } + + // === NodeId Display === + + #[test] + fn test_node_id_display() { + let id = NodeId::from_index(42); + assert_eq!(id.to_string(), "node_42"); + assert_eq!(id.index(), 42); + } + + #[test] + fn test_node_id_equality() { + let id1 = NodeId::from_index(5); + let id2 = NodeId::from_index(5); + let id3 = NodeId::from_index(6); + + assert_eq!(id1, id2); + assert_ne!(id1, id3); + } +} diff --git a/crates/settings/src/settings.rs b/crates/settings/src/settings.rs index 354cd3e..2deb7d9 100644 --- a/crates/settings/src/settings.rs +++ b/crates/settings/src/settings.rs @@ -1,6 +1,8 @@ use crate::error::SettingsError; +use crate::host::Host; +use crate::nodes::Nodes; use crate::sources::{config, ssh}; -use crate::types::Entry; +use crate::types::Action; use std::io; use std::path::PathBuf; @@ -11,10 +13,10 @@ pub struct Settings { pub terminal: String, /// Editor for opening config files. pub editor: String, - /// Actions from ~/.xshuttle.json. - pub actions: Vec, - /// SSH hosts from ~/.ssh/config. - pub hosts: Vec, + /// Actions from ~/.xshuttle.json with O(1) ID-based lookup. + pub actions: Nodes, + /// SSH hosts from ~/.ssh/config with O(1) ID-based lookup. + pub hosts: Nodes, } impl Settings { @@ -39,6 +41,7 @@ impl Settings { /// - SSH config file exists but cannot be parsed pub fn load() -> Result { let config = config::load()?.unwrap_or_default(); + let raw_hosts = ssh::parse_ssh_config()?; Ok(Settings { terminal: config @@ -47,8 +50,8 @@ impl Settings { editor: config .editor .unwrap_or_else(|| Self::DEFAULT_EDITOR.to_string()), - actions: config.actions.unwrap_or_default(), - hosts: ssh::parse_ssh_config()?, + actions: Nodes::from_entries(config.actions.unwrap_or_default()), + hosts: Nodes::from_hostnames(raw_hosts), }) } diff --git a/crates/tray/src/lib.rs b/crates/tray/src/lib.rs index 55e8e05..9bb280b 100644 --- a/crates/tray/src/lib.rs +++ b/crates/tray/src/lib.rs @@ -1,8 +1,7 @@ -use std::collections::HashMap; use std::fmt; use image::load_from_memory; -use settings::{Entry, Settings}; +use settings::{Action, Node, Settings}; use tray_icon::menu::{MenuItem, PredefinedMenuItem, Submenu}; use tray_icon::{Icon, TrayIcon, TrayIconBuilder}; @@ -11,6 +10,8 @@ pub use tray_icon::menu::{Menu, MenuEvent, MenuId}; pub const MENU_ID_CONFIGURE: &str = "configure"; pub const MENU_ID_RELOAD: &str = "reload"; pub const MENU_ID_QUIT: &str = "quit"; +pub const MENU_ID_ACTION_PREFIX: &str = "action_"; +pub const MENU_ID_HOST_PREFIX: &str = "host_"; const ICON_BYTES: &[u8] = include_bytes!("../../../assets/icon.png"); @@ -75,33 +76,38 @@ fn load_icon() -> Icon { /// Builds a menu from settings. /// -/// Returns the menu and a map from menu IDs to commands. +/// Uses the indexed `Nodes` containers for O(1) lookup. +/// Menu item IDs are formatted as `node_{index}` for dynamic entries. /// /// # Panics /// /// Panics if menu items cannot be appended to the menu. -pub fn build_menu(settings: &Settings) -> (Menu, HashMap) { +pub fn build_menu(settings: &Settings) -> Menu { let menu = Menu::new(); - let mut menu_id_map = HashMap::new(); - let mut id_counter = 0usize; - build_entries(&menu, &settings.actions, &mut menu_id_map, &mut id_counter); + // Build action entries (with submenus) + build_action_nodes(&menu, settings.actions.nodes()); + // Add separator if both sections have items if !settings.actions.is_empty() && !settings.hosts.is_empty() { menu.append(&PredefinedMenuItem::separator()).unwrap(); } - for (index, host) in settings.hosts.iter().enumerate() { - let menu_id = format!("ssh_{index}"); - menu_id_map.insert(menu_id.clone(), format!("ssh {host}")); - let item = MenuItem::with_id(menu_id, host, true, None); - menu.append(&item).expect("Failed to append menu item"); + // Build host entries (flat) + for node in settings.hosts.nodes() { + if let Node::Leaf { id, value } = node { + let menu_id = format!("{}{}", MENU_ID_HOST_PREFIX, id.index()); + let item = MenuItem::with_id(menu_id, &value.hostname, true, None); + menu.append(&item).expect("Failed to append menu item"); + } } - if !settings.hosts.is_empty() { + // Add separator before static items + if !settings.hosts.is_empty() || !settings.actions.is_empty() { menu.append(&PredefinedMenuItem::separator()).unwrap(); } + // Add static menu items menu.append(&MenuItem::with_id( MENU_ID_CONFIGURE, "Configure", @@ -114,55 +120,39 @@ pub fn build_menu(settings: &Settings) -> (Menu, HashMap) { menu.append(&MenuItem::with_id(MENU_ID_QUIT, "Quit", true, None)) .unwrap(); - (menu, menu_id_map) + menu } -fn build_entries( - menu: &Menu, - entries: &[Entry], - menu_id_map: &mut HashMap, - id_counter: &mut usize, -) { - for entry in entries { - match entry { - Entry::Action(cmd) => { - let id = *id_counter; - let menu_id = format!("action_{id}"); - *id_counter += 1; - menu_id_map.insert(menu_id.clone(), cmd.cmd.clone()); - let menu_item = MenuItem::with_id(menu_id, &cmd.name, true, None); +fn build_action_nodes(menu: &Menu, nodes: &[Node]) { + for node in nodes { + match node { + Node::Leaf { id, value } => { + let menu_id = format!("{}{}", MENU_ID_ACTION_PREFIX, id.index()); + let menu_item = MenuItem::with_id(menu_id, &value.name, true, None); menu.append(&menu_item).expect("Failed to append menu item"); } - Entry::Group(group) => { - let submenu = Submenu::new(&group.name, true); - build_submenu_entries(&submenu, &group.entries, menu_id_map, id_counter); + Node::Group { name, children } => { + let submenu = Submenu::new(name, true); + build_action_submenu(&submenu, children); menu.append(&submenu).expect("Failed to append submenu"); } } } } -fn build_submenu_entries( - submenu: &Submenu, - entries: &[Entry], - menu_id_map: &mut HashMap, - id_counter: &mut usize, -) { - for entry in entries { - match entry { - Entry::Action(cmd) => { - let id = *id_counter; - let menu_id = format!("action_{id}"); - *id_counter += 1; - menu_id_map.insert(menu_id.clone(), cmd.cmd.clone()); - let menu_item = MenuItem::with_id(menu_id, &cmd.name, true, None); +fn build_action_submenu(submenu: &Submenu, nodes: &[Node]) { + for node in nodes { + match node { + Node::Leaf { id, value } => { + let menu_id = format!("{}{}", MENU_ID_ACTION_PREFIX, id.index()); + let menu_item = MenuItem::with_id(menu_id, &value.name, true, None); submenu .append(&menu_item) .expect("Failed to append menu item"); } - Entry::Group(group) => { - let nested = Submenu::new(&group.name, true); - build_submenu_entries(&nested, &group.entries, menu_id_map, id_counter); + Node::Group { name, children } => { + let nested = Submenu::new(name, true); + build_action_submenu(&nested, children); submenu.append(&nested).expect("Failed to append submenu"); } } diff --git a/src/xshuttle.rs b/src/xshuttle.rs index b3f7488..41b1e58 100644 --- a/src/xshuttle.rs +++ b/src/xshuttle.rs @@ -1,8 +1,9 @@ -use std::collections::HashMap; - -use settings::Settings; +use settings::{NodeId, Settings}; use terminal::Terminal; -use tray::{MENU_ID_CONFIGURE, MENU_ID_QUIT, MENU_ID_RELOAD, Menu, MenuEvent, Tray, build_menu}; +use tray::{ + MENU_ID_ACTION_PREFIX, MENU_ID_CONFIGURE, MENU_ID_HOST_PREFIX, MENU_ID_QUIT, MENU_ID_RELOAD, + Menu, MenuEvent, Tray, build_menu, +}; #[derive(Debug)] pub enum UserEvent { @@ -13,7 +14,6 @@ pub enum UserEvent { pub struct Application { settings: Option, tray: Tray, - menu_id_map: HashMap, } impl Application { @@ -33,14 +33,11 @@ impl Application { eprintln!("Error loading settings: {e}"); // Return empty menu if settings fail to load self.settings = None; - self.menu_id_map.clear(); return Menu::new(); } }; - let (menu, menu_id_map) = build_menu(&settings); - - self.menu_id_map = menu_id_map; + let menu = build_menu(&settings); self.settings = Some(settings); menu @@ -64,14 +61,16 @@ impl Application { return false; } - if let Some(command) = self.menu_id_map.get(menu_id) { + // O(1) lookup for dynamic menu items + let command = self.lookup_command(menu_id); + if let Some(cmd) = command { let terminal = self .settings .as_ref() .map(|s| Terminal::from(s.terminal.as_str())) .unwrap_or_default(); - if let Err(e) = terminal.launch(command) { + if let Err(e) = terminal.launch(&cmd) { eprintln!("Error: {e}"); } } @@ -79,6 +78,27 @@ impl Application { false } + /// O(1) lookup for action and host commands by menu ID. + fn lookup_command(&self, menu_id: &str) -> Option { + let settings = self.settings.as_ref()?; + + // Check for action prefix: "action_{index}" + if let Some(index_str) = menu_id.strip_prefix(MENU_ID_ACTION_PREFIX) { + let index: usize = index_str.parse().ok()?; + let action = settings.actions.get(NodeId::from_index(index))?; + return Some(action.cmd.clone()); + } + + // Check for host prefix: "host_{index}" + if let Some(index_str) = menu_id.strip_prefix(MENU_ID_HOST_PREFIX) { + let index: usize = index_str.parse().ok()?; + let host = settings.hosts.get(NodeId::from_index(index))?; + return Some(host.command()); + } + + None + } + fn configure(&self) { let Some(path) = Settings::config_path() else { eprintln!("Error: Could not determine config path"); From 95971e335ed3c7960b0804eb392696a39c724b2a Mon Sep 17 00:00:00 2001 From: Andreas Penz Date: Sat, 10 Jan 2026 18:46:13 +0100 Subject: [PATCH 3/3] cleanup settings crate --- crates/settings/src/error.rs | 38 ++++++++++++++- crates/settings/src/lib.rs | 6 +-- .../src/{sources => loaders}/config.rs | 27 +---------- .../settings/src/{sources => loaders}/mod.rs | 0 .../settings/src/{sources => loaders}/ssh.rs | 0 crates/settings/src/nodes.rs | 40 +++++++++------- crates/settings/src/settings.rs | 22 ++++++++- crates/tray/src/lib.rs | 46 +++++++++++-------- 8 files changed, 112 insertions(+), 67 deletions(-) rename crates/settings/src/{sources => loaders}/config.rs (94%) rename crates/settings/src/{sources => loaders}/mod.rs (100%) rename crates/settings/src/{sources => loaders}/ssh.rs (100%) diff --git a/crates/settings/src/error.rs b/crates/settings/src/error.rs index 2614c4a..a4aec37 100644 --- a/crates/settings/src/error.rs +++ b/crates/settings/src/error.rs @@ -1,7 +1,43 @@ -use crate::sources::config::ValidationError; +use std::fmt; use std::io; use thiserror::Error; +// ============================================================================ +// Validation Types +// ============================================================================ + +/// A validation error with path and message. +#[derive(Debug, Clone)] +pub struct ValidationError { + /// JSON path to the error location. + pub path: String, + /// Human-readable error description. + pub message: String, +} + +impl fmt::Display for ValidationError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + if self.path.is_empty() { + write!(f, "{}", self.message) + } else { + write!(f, "{}: {}", self.path, self.message) + } + } +} + +/// Result of config validation. +#[derive(Debug)] +pub enum ValidationResult { + /// Validation passed with no errors. + Valid, + /// Validation failed with one or more errors. + Invalid(Vec), +} + +// ============================================================================ +// Settings Error +// ============================================================================ + /// Error type for settings loading operations. #[derive(Error, Debug)] pub enum SettingsError { diff --git a/crates/settings/src/lib.rs b/crates/settings/src/lib.rs index d8e6108..e9d3918 100644 --- a/crates/settings/src/lib.rs +++ b/crates/settings/src/lib.rs @@ -1,13 +1,13 @@ mod error; mod host; +mod loaders; mod nodes; mod settings; -mod sources; mod types; -pub use error::SettingsError; +pub use error::{SettingsError, ValidationError, ValidationResult}; pub use host::Host; +pub use loaders::config::{schema, validate}; pub use nodes::{Node, NodeId, Nodes}; pub use settings::Settings; -pub use sources::config::{ValidationError, ValidationResult, schema, validate}; pub use types::{Action, Entry, Group}; diff --git a/crates/settings/src/sources/config.rs b/crates/settings/src/loaders/config.rs similarity index 94% rename from crates/settings/src/sources/config.rs rename to crates/settings/src/loaders/config.rs index 73563c4..9587d32 100644 --- a/crates/settings/src/sources/config.rs +++ b/crates/settings/src/loaders/config.rs @@ -1,9 +1,8 @@ -use crate::error::SettingsError; +use crate::error::{SettingsError, ValidationError, ValidationResult}; use crate::types::Entry; use jsonschema::Validator; use serde::Deserialize; use serde_json::Value; -use std::fmt; use std::fs; use std::path::{Path, PathBuf}; @@ -23,30 +22,6 @@ pub(crate) struct ConfigContent { // Schema Validation // ============================================================================ -/// A validation error with path and message. -#[derive(Debug, Clone)] -pub struct ValidationError { - pub path: String, - pub message: String, -} - -impl fmt::Display for ValidationError { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - if self.path.is_empty() { - write!(f, "{}", self.message) - } else { - write!(f, "{}: {}", self.path, self.message) - } - } -} - -/// Result of config validation. -#[derive(Debug)] -pub enum ValidationResult { - Valid, - Invalid(Vec), -} - /// Returns the embedded JSON schema as a string. pub fn schema() -> &'static str { SCHEMA_JSON diff --git a/crates/settings/src/sources/mod.rs b/crates/settings/src/loaders/mod.rs similarity index 100% rename from crates/settings/src/sources/mod.rs rename to crates/settings/src/loaders/mod.rs diff --git a/crates/settings/src/sources/ssh.rs b/crates/settings/src/loaders/ssh.rs similarity index 100% rename from crates/settings/src/sources/ssh.rs rename to crates/settings/src/loaders/ssh.rs diff --git a/crates/settings/src/nodes.rs b/crates/settings/src/nodes.rs index ab4cc0d..348eb87 100644 --- a/crates/settings/src/nodes.rs +++ b/crates/settings/src/nodes.rs @@ -45,15 +45,19 @@ impl fmt::Display for NodeId { /// A node in the indexed tree. /// /// Use pattern matching to distinguish leaves from groups when -/// building menus or iterating the tree structure. +/// building menus or iterating the tree structure. For `Leaf` nodes, +/// use [`Nodes::get()`] to retrieve the actual value by ID. #[derive(Debug, Clone)] pub enum Node { - /// A leaf node with its value and assigned ID. + /// A leaf node referencing a value by ID. + /// + /// The actual value is stored in the [`Nodes`] container's `leaves` Vec. + /// Use [`Nodes::get()`] to retrieve it. Leaf { - /// Unique identifier for this leaf. + /// Unique identifier for this leaf, used to look up the value. id: NodeId, - /// The leaf value. - value: T, + /// Marker for the value type (not stored here). + _marker: std::marker::PhantomData, }, /// A named group containing nested nodes. Group { @@ -92,10 +96,9 @@ impl Node { /// Built from config data during settings load. Provides both flat /// iteration (for lookup) and tree iteration (for menu building). /// -/// # Type Parameter -/// -/// `T: Clone` - The leaf value type. Clone is required because values -/// are stored in both the tree structure and the flat lookup table. +/// The tree structure stores only [`NodeId`] references in leaf nodes, +/// while actual values are stored solely in the `leaves` Vec. This +/// eliminates value duplication and reduces memory usage. #[derive(Debug, Clone)] pub struct Nodes { /// Tree structure for menu building. @@ -120,8 +123,11 @@ impl Nodes { match entry { Entry::Action(action) => { let id = NodeId::from_index(leaves.len()); - leaves.push(action.clone()); - Node::Leaf { id, value: action } + leaves.push(action); + Node::Leaf { + id, + _marker: std::marker::PhantomData, + } } Entry::Group(Group { name, entries }) => { let children = entries @@ -144,10 +150,10 @@ impl Nodes { .enumerate() .map(|(i, hostname)| { let host = Host { hostname }; - leaves.push(host.clone()); + leaves.push(host); Node::Leaf { id: NodeId::from_index(i), - value: host, + _marker: std::marker::PhantomData, } }) .collect(); @@ -347,9 +353,10 @@ mod tests { assert_eq!(tree.len(), 2); - // First is a leaf + // First is a leaf - use nodes.get() to access value assert!(tree[0].is_leaf()); - if let Node::Leaf { value, .. } = &tree[0] { + if let Node::Leaf { id, .. } = &tree[0] { + let value = nodes.get(*id).expect("should find value"); assert_eq!(value.name, "Root"); } @@ -396,7 +403,8 @@ mod tests { if let Node::Group { children, .. } = level3 { let deep = &children[0]; assert!(deep.is_leaf()); - if let Node::Leaf { value, .. } = deep { + if let Node::Leaf { id, .. } = deep { + let value = nodes.get(*id).expect("should find value"); assert_eq!(value.name, "Deep"); } } diff --git a/crates/settings/src/settings.rs b/crates/settings/src/settings.rs index 2deb7d9..382994c 100644 --- a/crates/settings/src/settings.rs +++ b/crates/settings/src/settings.rs @@ -1,7 +1,7 @@ use crate::error::SettingsError; use crate::host::Host; +use crate::loaders::{config, ssh}; use crate::nodes::Nodes; -use crate::sources::{config, ssh}; use crate::types::Action; use std::io; use std::path::PathBuf; @@ -19,6 +19,17 @@ pub struct Settings { pub hosts: Nodes, } +impl Default for Settings { + fn default() -> Self { + Self { + terminal: Self::DEFAULT_TERMINAL.to_string(), + editor: Self::DEFAULT_EDITOR.to_string(), + actions: Nodes::from_entries(vec![]), + hosts: Nodes::from_hostnames(vec![]), + } + } +} + impl Settings { /// Default terminal value when not specified in config. pub const DEFAULT_TERMINAL: &'static str = "default"; @@ -90,4 +101,13 @@ mod tests { assert!(p.ends_with(".xshuttle.json")); } } + + #[test] + fn test_settings_default() { + let settings = Settings::default(); + assert_eq!(settings.terminal, Settings::DEFAULT_TERMINAL); + assert_eq!(settings.editor, Settings::DEFAULT_EDITOR); + assert!(settings.actions.is_empty()); + assert!(settings.hosts.is_empty()); + } } diff --git a/crates/tray/src/lib.rs b/crates/tray/src/lib.rs index 9bb280b..fd6a9dc 100644 --- a/crates/tray/src/lib.rs +++ b/crates/tray/src/lib.rs @@ -1,7 +1,7 @@ use std::fmt; use image::load_from_memory; -use settings::{Action, Node, Settings}; +use settings::{Action, Node, Nodes, Settings}; use tray_icon::menu::{MenuItem, PredefinedMenuItem, Submenu}; use tray_icon::{Icon, TrayIcon, TrayIconBuilder}; @@ -86,7 +86,7 @@ pub fn build_menu(settings: &Settings) -> Menu { let menu = Menu::new(); // Build action entries (with submenus) - build_action_nodes(&menu, settings.actions.nodes()); + build_action_nodes(&menu, settings.actions.nodes(), &settings.actions); // Add separator if both sections have items if !settings.actions.is_empty() && !settings.hosts.is_empty() { @@ -95,9 +95,11 @@ pub fn build_menu(settings: &Settings) -> Menu { // Build host entries (flat) for node in settings.hosts.nodes() { - if let Node::Leaf { id, value } = node { + if let Node::Leaf { id, .. } = node + && let Some(host) = settings.hosts.get(*id) + { let menu_id = format!("{}{}", MENU_ID_HOST_PREFIX, id.index()); - let item = MenuItem::with_id(menu_id, &value.hostname, true, None); + let item = MenuItem::with_id(menu_id, &host.hostname, true, None); menu.append(&item).expect("Failed to append menu item"); } } @@ -123,36 +125,40 @@ pub fn build_menu(settings: &Settings) -> Menu { menu } -fn build_action_nodes(menu: &Menu, nodes: &[Node]) { - for node in nodes { +fn build_action_nodes(menu: &Menu, tree: &[Node], actions: &Nodes) { + for node in tree { match node { - Node::Leaf { id, value } => { - let menu_id = format!("{}{}", MENU_ID_ACTION_PREFIX, id.index()); - let menu_item = MenuItem::with_id(menu_id, &value.name, true, None); - menu.append(&menu_item).expect("Failed to append menu item"); + Node::Leaf { id, .. } => { + if let Some(action) = actions.get(*id) { + let menu_id = format!("{}{}", MENU_ID_ACTION_PREFIX, id.index()); + let menu_item = MenuItem::with_id(menu_id, &action.name, true, None); + menu.append(&menu_item).expect("Failed to append menu item"); + } } Node::Group { name, children } => { let submenu = Submenu::new(name, true); - build_action_submenu(&submenu, children); + build_action_submenu(&submenu, children, actions); menu.append(&submenu).expect("Failed to append submenu"); } } } } -fn build_action_submenu(submenu: &Submenu, nodes: &[Node]) { - for node in nodes { +fn build_action_submenu(submenu: &Submenu, tree: &[Node], actions: &Nodes) { + for node in tree { match node { - Node::Leaf { id, value } => { - let menu_id = format!("{}{}", MENU_ID_ACTION_PREFIX, id.index()); - let menu_item = MenuItem::with_id(menu_id, &value.name, true, None); - submenu - .append(&menu_item) - .expect("Failed to append menu item"); + Node::Leaf { id, .. } => { + if let Some(action) = actions.get(*id) { + let menu_id = format!("{}{}", MENU_ID_ACTION_PREFIX, id.index()); + let menu_item = MenuItem::with_id(menu_id, &action.name, true, None); + submenu + .append(&menu_item) + .expect("Failed to append menu item"); + } } Node::Group { name, children } => { let nested = Submenu::new(name, true); - build_action_submenu(&nested, children); + build_action_submenu(&nested, children, actions); submenu.append(&nested).expect("Failed to append submenu"); } }