diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6694629f8..508542d86 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -135,6 +135,9 @@ otherwise it reports that optional local check as skipped. The source-coverage g rustc dep-info and rejects tracked or unignored `.rs` files that are outside the supported Cargo target/feature matrix. +The all-feature portal transport tests require `dbus-daemon`. Each fixture owns a private +session bus and connects through its explicit address, leaving the desktop session bus alone. + The canonical gate serializes the Rust test harness because parallel rendering tests have crashed in the native font stack through context-menu, board-picker, and region-capture paths. This changes scheduling, not test selection; tests may still create their own threads. diff --git a/Cargo.lock b/Cargo.lock index 5136bdacf..d0ddfb6d6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1884,6 +1884,7 @@ dependencies = [ "gtk4", "libadwaita", "relm4", + "tempfile", "tokio", "wayscriber", ] diff --git a/config.example.toml b/config.example.toml index c5439cd0c..78d6cb033 100644 --- a/config.example.toml +++ b/config.example.toml @@ -2,6 +2,8 @@ # Location: ~/.config/wayscriber/config.toml # # All settings are optional. If not specified, defaults will be used. +# Floating-point settings must be finite (not nan, inf, or -inf). +# Load-time fallbacks never rewrite this file; see docs/CONFIG.md for the policy. # # Every value here is a configured default: the value Wayscriber starts from. # Edit them in the graphical configurator (`wayscriber-configurator`, or F11 diff --git a/configurator/Cargo.toml b/configurator/Cargo.toml index 90543ede9..7c90f65d0 100644 --- a/configurator/Cargo.toml +++ b/configurator/Cargo.toml @@ -21,3 +21,6 @@ gtk4 = { version = "0.11", features = ["v4_10"] } [features] default = ["tablet-input"] tablet-input = ["wayscriber/tablet-input"] + +[dev-dependencies] +tempfile = "3" diff --git a/configurator/src/app/daemon_setup/hyprland/tests.rs b/configurator/src/app/daemon_setup/hyprland/tests.rs index df94a604e..1ddc9a426 100644 --- a/configurator/src/app/daemon_setup/hyprland/tests.rs +++ b/configurator/src/app/daemon_setup/hyprland/tests.rs @@ -1,10 +1,6 @@ use super::*; -use std::env; -use std::sync::Mutex; use wayscriber::env_vars::HOME_ENV; -static ENV_MUTEX: Mutex<()> = Mutex::new(()); - #[test] fn render_light_controls_quotes_binary_with_spaces() { let rendered = render_light_controls(Path::new("/tmp/My Apps/wayscriber")); @@ -80,15 +76,9 @@ fn has_source_line_matches_quoted_and_inline_commented_targets() { #[test] fn has_source_line_matches_tilde_target() { - let _guard = ENV_MUTEX - .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()); let tmp = crate::test_temp::tempdir().unwrap(); let home = tmp.path(); - let prev_home = env::var_os(HOME_ENV); - unsafe { - env::set_var(HOME_ENV, home); - } + let _env = crate::test_env::EnvGuard::set(&[(HOME_ENV, home.as_os_str())]); let absolute = home .join(".config") @@ -99,11 +89,6 @@ fn has_source_line_matches_tilde_target() { "source = ~/.config/hypr/wayscriber-light.conf # already sourced\n", &source_line )); - - match prev_home { - Some(value) => unsafe { env::set_var(HOME_ENV, value) }, - None => unsafe { env::remove_var(HOME_ENV) }, - } } #[test] diff --git a/configurator/src/app/pages/arrow.rs b/configurator/src/app/pages/arrow.rs index 4a2eb17c6..6f30ca166 100644 --- a/configurator/src/app/pages/arrow.rs +++ b/configurator/src/app/pages/arrow.rs @@ -6,6 +6,7 @@ //! error text instead, on the same `.error` styling every ported page uses. use relm4::prelude::*; +use wayscriber::config::{ARROW_ANGLE_MAX, ARROW_ANGLE_MIN, ARROW_LENGTH_MAX, ARROW_LENGTH_MIN}; use crate::messages::Message; use crate::models::util::format_float; @@ -15,11 +16,6 @@ use super::super::search::SearchArea; use super::super::state::ConfiguratorApp; use super::{BuiltPage, PageBuilder}; -/// `ArrowConfig::length`, in pixels. -const LENGTH_RANGE: (f64, f64) = (5.0, 50.0); -/// `ArrowConfig::angle_degrees`, in degrees. -const ANGLE_RANGE: (f64, f64) = (15.0, 60.0); - pub(super) fn build(sender: &ComponentSender) -> BuiltPage { let mut page = PageBuilder::new(sender, TabId::Arrow); @@ -28,13 +24,13 @@ pub(super) fn build(sender: &ComponentSender) -> BuiltPage { "Arrow length (px)", |app| app.draft.arrow_length.clone(), |value| Message::TextChanged(TextField::ArrowLength, value), - |app| validate_f64_range(&app.draft.arrow_length, LENGTH_RANGE.0, LENGTH_RANGE.1), + |app| validate_f64_range(&app.draft.arrow_length, ARROW_LENGTH_MIN, ARROW_LENGTH_MAX), ) .entry_row_validated( "Arrow angle (deg)", |app| app.draft.arrow_angle.clone(), |value| Message::TextChanged(TextField::ArrowAngle, value), - |app| validate_f64_range(&app.draft.arrow_angle, ANGLE_RANGE.0, ANGLE_RANGE.1), + |app| validate_f64_range(&app.draft.arrow_angle, ARROW_ANGLE_MIN, ARROW_ANGLE_MAX), ) .switch_row( "Place arrowhead at end of line", diff --git a/configurator/src/app/pages/ui/click_highlight.rs b/configurator/src/app/pages/ui/click_highlight.rs index 2089bc0a5..8d0d7711e 100644 --- a/configurator/src/app/pages/ui/click_highlight.rs +++ b/configurator/src/app/pages/ui/click_highlight.rs @@ -1,4 +1,8 @@ use relm4::ComponentSender; +use wayscriber::config::{ + CLICK_HIGHLIGHT_DURATION_MAX_MS, CLICK_HIGHLIGHT_DURATION_MIN_MS, CLICK_HIGHLIGHT_OUTLINE_MAX, + CLICK_HIGHLIGHT_OUTLINE_MIN, CLICK_HIGHLIGHT_RADIUS_MAX, CLICK_HIGHLIGHT_RADIUS_MIN, +}; use crate::messages::Message; use crate::models::{ColorPickerId, TabId, TextField, ToggleField}; @@ -42,19 +46,37 @@ pub(super) fn build(sender: &ComponentSender) -> BuiltPage { "Radius", |app| app.draft.click_highlight_radius.clone(), |value| Message::TextChanged(TextField::HighlightRadius, value), - |app| validate_f64_range(&app.draft.click_highlight_radius, 16.0, 160.0), + |app| { + validate_f64_range( + &app.draft.click_highlight_radius, + CLICK_HIGHLIGHT_RADIUS_MIN, + CLICK_HIGHLIGHT_RADIUS_MAX, + ) + }, ) .entry_row_validated( "Outline thickness", |app| app.draft.click_highlight_outline_thickness.clone(), |value| Message::TextChanged(TextField::HighlightOutlineThickness, value), - |app| validate_f64_range(&app.draft.click_highlight_outline_thickness, 1.0, 12.0), + |app| { + validate_f64_range( + &app.draft.click_highlight_outline_thickness, + CLICK_HIGHLIGHT_OUTLINE_MIN, + CLICK_HIGHLIGHT_OUTLINE_MAX, + ) + }, ) .entry_row_validated( "Duration (ms)", |app| app.draft.click_highlight_duration_ms.clone(), |value| Message::TextChanged(TextField::HighlightDurationMs, value), - |app| validate_u32_range(&app.draft.click_highlight_duration_ms, 150, 1500), + |app| { + validate_u32_range( + &app.draft.click_highlight_duration_ms, + CLICK_HIGHLIGHT_DURATION_MIN_MS as u32, + CLICK_HIGHLIGHT_DURATION_MAX_MS as u32, + ) + }, ); page.group("Colors"); diff --git a/configurator/src/app/session_catalog/duplicate.rs b/configurator/src/app/session_catalog/duplicate.rs index 227db0905..2b80a7474 100644 --- a/configurator/src/app/session_catalog/duplicate.rs +++ b/configurator/src/app/session_catalog/duplicate.rs @@ -82,8 +82,6 @@ fn duplicate_session_catalog_entry_sync( #[cfg(test)] mod tests { use std::ffi::OsString; - use std::path::Path; - use std::sync::MutexGuard; use crate::models::{ DaemonRuntimeStatus, DesktopEnvironment, LightShortcutApplyCapability, @@ -93,50 +91,6 @@ mod tests { use super::*; - struct EnvGuard { - catalog_hooks: Option, - xdg_data_home: Option, - xdg_runtime_dir: Option, - _guard: MutexGuard<'static, ()>, - } - - impl EnvGuard { - fn set_roots(path: &Path) -> Self { - let guard = crate::test_env::lock(); - let catalog_hooks = std::env::var_os(CATALOG_HOOKS_TEST_ENV); - let xdg_data_home = std::env::var_os(XDG_DATA_HOME_ENV); - let xdg_runtime_dir = std::env::var_os(XDG_RUNTIME_DIR_ENV); - unsafe { - std::env::set_var(CATALOG_HOOKS_TEST_ENV, path); - std::env::set_var(XDG_DATA_HOME_ENV, path); - std::env::set_var(XDG_RUNTIME_DIR_ENV, path); - } - Self { - catalog_hooks, - xdg_data_home, - xdg_runtime_dir, - _guard: guard, - } - } - } - - impl Drop for EnvGuard { - fn drop(&mut self) { - match self.catalog_hooks.take() { - Some(value) => unsafe { std::env::set_var(CATALOG_HOOKS_TEST_ENV, value) }, - None => unsafe { std::env::remove_var(CATALOG_HOOKS_TEST_ENV) }, - } - match self.xdg_data_home.take() { - Some(value) => unsafe { std::env::set_var(XDG_DATA_HOME_ENV, value) }, - None => unsafe { std::env::remove_var(XDG_DATA_HOME_ENV) }, - } - match self.xdg_runtime_dir.take() { - Some(value) => unsafe { std::env::set_var(XDG_RUNTIME_DIR_ENV, value) }, - None => unsafe { std::env::remove_var(XDG_RUNTIME_DIR_ENV) }, - } - } - } - fn inactive_status() -> DaemonRuntimeStatus { DaemonRuntimeStatus { desktop: DesktopEnvironment::Unknown, @@ -158,7 +112,11 @@ mod tests { #[test] fn duplicate_session_catalog_entry_copies_primary_and_catalogs_new_entry() { let temp = crate::test_temp::tempdir().unwrap(); - let _env = EnvGuard::set_roots(temp.path()); + let _env = crate::test_env::EnvGuard::set(&[ + (CATALOG_HOOKS_TEST_ENV, temp.path().as_os_str()), + (XDG_DATA_HOME_ENV, temp.path().as_os_str()), + (XDG_RUNTIME_DIR_ENV, temp.path().as_os_str()), + ]); let source = temp.path().join("lecture.wayscriber-session"); let target = temp.path().join("lecture-copy.wayscriber-session"); let source_artifacts = wayscriber::session::named_session_artifact_paths(&source); @@ -199,7 +157,11 @@ mod tests { #[test] fn duplicate_session_catalog_entry_warns_when_catalog_update_fails_after_copy() { let temp = crate::test_temp::tempdir().unwrap(); - let _env = EnvGuard::set_roots(temp.path()); + let _env = crate::test_env::EnvGuard::set(&[ + (CATALOG_HOOKS_TEST_ENV, temp.path().as_os_str()), + (XDG_DATA_HOME_ENV, temp.path().as_os_str()), + (XDG_RUNTIME_DIR_ENV, temp.path().as_os_str()), + ]); let source = temp.path().join("lecture.wayscriber-session"); let target = temp.path().join("lecture-copy.wayscriber-session"); std::fs::write(&source, b"primary").unwrap(); diff --git a/configurator/src/app/session_catalog/move_file.rs b/configurator/src/app/session_catalog/move_file.rs index 83ca9dcf2..5c9cc7ee7 100644 --- a/configurator/src/app/session_catalog/move_file.rs +++ b/configurator/src/app/session_catalog/move_file.rs @@ -129,7 +129,6 @@ fn reject_catalog_target_collision( #[cfg(test)] mod tests { use std::ffi::OsString; - use std::sync::MutexGuard; use crate::models::{ DaemonRuntimeStatus, DesktopEnvironment, LightShortcutApplyCapability, @@ -139,50 +138,6 @@ mod tests { use super::*; - struct EnvGuard { - catalog_hooks: Option, - xdg_data_home: Option, - xdg_runtime_dir: Option, - _guard: MutexGuard<'static, ()>, - } - - impl EnvGuard { - fn set_roots(path: &Path) -> Self { - let guard = crate::test_env::lock(); - let catalog_hooks = std::env::var_os(CATALOG_HOOKS_TEST_ENV); - let xdg_data_home = std::env::var_os(XDG_DATA_HOME_ENV); - let xdg_runtime_dir = std::env::var_os(XDG_RUNTIME_DIR_ENV); - unsafe { - std::env::set_var(CATALOG_HOOKS_TEST_ENV, path); - std::env::set_var(XDG_DATA_HOME_ENV, path); - std::env::set_var(XDG_RUNTIME_DIR_ENV, path); - } - Self { - catalog_hooks, - xdg_data_home, - xdg_runtime_dir, - _guard: guard, - } - } - } - - impl Drop for EnvGuard { - fn drop(&mut self) { - match self.catalog_hooks.take() { - Some(value) => unsafe { std::env::set_var(CATALOG_HOOKS_TEST_ENV, value) }, - None => unsafe { std::env::remove_var(CATALOG_HOOKS_TEST_ENV) }, - } - match self.xdg_data_home.take() { - Some(value) => unsafe { std::env::set_var(XDG_DATA_HOME_ENV, value) }, - None => unsafe { std::env::remove_var(XDG_DATA_HOME_ENV) }, - } - match self.xdg_runtime_dir.take() { - Some(value) => unsafe { std::env::set_var(XDG_RUNTIME_DIR_ENV, value) }, - None => unsafe { std::env::remove_var(XDG_RUNTIME_DIR_ENV) }, - } - } - } - fn inactive_status() -> DaemonRuntimeStatus { DaemonRuntimeStatus { desktop: DesktopEnvironment::Unknown, @@ -204,7 +159,11 @@ mod tests { #[test] fn move_session_catalog_entry_moves_artifacts_and_preserves_catalog_id() { let temp = crate::test_temp::tempdir().unwrap(); - let _env = EnvGuard::set_roots(temp.path()); + let _env = crate::test_env::EnvGuard::set(&[ + (CATALOG_HOOKS_TEST_ENV, temp.path().as_os_str()), + (XDG_DATA_HOME_ENV, temp.path().as_os_str()), + (XDG_RUNTIME_DIR_ENV, temp.path().as_os_str()), + ]); let source = temp.path().join("lecture.wayscriber-session"); let target = temp.path().join("archive.wayscriber-session"); let source_artifacts = wayscriber::session::named_session_artifact_paths(&source); @@ -243,7 +202,11 @@ mod tests { #[test] fn move_session_catalog_entry_failure_keeps_catalog_and_source_artifacts() { let temp = crate::test_temp::tempdir().unwrap(); - let _env = EnvGuard::set_roots(temp.path()); + let _env = crate::test_env::EnvGuard::set(&[ + (CATALOG_HOOKS_TEST_ENV, temp.path().as_os_str()), + (XDG_DATA_HOME_ENV, temp.path().as_os_str()), + (XDG_RUNTIME_DIR_ENV, temp.path().as_os_str()), + ]); let source = temp.path().join("lecture.wayscriber-session"); let target = temp.path().join("archive.wayscriber-session"); let source_artifacts = wayscriber::session::named_session_artifact_paths(&source); @@ -280,7 +243,11 @@ mod tests { #[test] fn move_session_catalog_entry_rejects_catalog_target_collision_before_disk_move() { let temp = crate::test_temp::tempdir().unwrap(); - let _env = EnvGuard::set_roots(temp.path()); + let _env = crate::test_env::EnvGuard::set(&[ + (CATALOG_HOOKS_TEST_ENV, temp.path().as_os_str()), + (XDG_DATA_HOME_ENV, temp.path().as_os_str()), + (XDG_RUNTIME_DIR_ENV, temp.path().as_os_str()), + ]); let source = temp.path().join("lecture.wayscriber-session"); let target = temp.path().join("archive.wayscriber-session"); std::fs::write(&source, b"primary").unwrap(); @@ -308,7 +275,11 @@ mod tests { #[test] fn move_session_catalog_entry_rolls_back_when_catalog_update_fails_after_move() { let temp = crate::test_temp::tempdir().unwrap(); - let _env = EnvGuard::set_roots(temp.path()); + let _env = crate::test_env::EnvGuard::set(&[ + (CATALOG_HOOKS_TEST_ENV, temp.path().as_os_str()), + (XDG_DATA_HOME_ENV, temp.path().as_os_str()), + (XDG_RUNTIME_DIR_ENV, temp.path().as_os_str()), + ]); let source = temp.path().join("lecture.wayscriber-session"); let target = temp.path().join("archive.wayscriber-session"); let source_artifacts = wayscriber::session::named_session_artifact_paths(&source); diff --git a/configurator/src/app/session_catalog/tests.rs b/configurator/src/app/session_catalog/tests.rs index 1dd38095d..ee903c0d9 100644 --- a/configurator/src/app/session_catalog/tests.rs +++ b/configurator/src/app/session_catalog/tests.rs @@ -1,41 +1,11 @@ use super::*; -use std::ffi::OsString; use std::path::PathBuf; -use std::sync::MutexGuard; use crate::models::{ DesktopEnvironment, LightShortcutApplyCapability, ShortcutApplyCapability, ShortcutBackend, }; use wayscriber::env_vars::XDG_RUNTIME_DIR_ENV; -struct RuntimeEnvGuard { - previous: Option, - _guard: MutexGuard<'static, ()>, -} - -impl RuntimeEnvGuard { - fn set_xdg_runtime_dir(path: &Path) -> Self { - let guard = crate::test_env::lock(); - let previous = std::env::var_os(XDG_RUNTIME_DIR_ENV); - unsafe { - std::env::set_var(XDG_RUNTIME_DIR_ENV, path); - } - Self { - previous, - _guard: guard, - } - } -} - -impl Drop for RuntimeEnvGuard { - fn drop(&mut self) { - match self.previous.take() { - Some(value) => unsafe { std::env::set_var(XDG_RUNTIME_DIR_ENV, value) }, - None => unsafe { std::env::remove_var(XDG_RUNTIME_DIR_ENV) }, - } - } -} - fn daemon_status(active: bool) -> DaemonRuntimeStatus { DaemonRuntimeStatus { desktop: DesktopEnvironment::Unknown, @@ -57,7 +27,7 @@ fn daemon_status(active: bool) -> DaemonRuntimeStatus { #[test] fn clear_policy_blocks_running_daemon() { let temp = crate::test_temp::tempdir().unwrap(); - let _env = RuntimeEnvGuard::set_xdg_runtime_dir(temp.path()); + let _env = crate::test_env::EnvGuard::set(&[(XDG_RUNTIME_DIR_ENV, temp.path().as_os_str())]); let status = daemon_status(true); let blocker = SessionCatalogOperation::Clear @@ -70,7 +40,7 @@ fn clear_policy_blocks_running_daemon() { #[test] fn clear_policy_allows_inactive_daemon() { let temp = crate::test_temp::tempdir().unwrap(); - let _env = RuntimeEnvGuard::set_xdg_runtime_dir(temp.path()); + let _env = crate::test_env::EnvGuard::set(&[(XDG_RUNTIME_DIR_ENV, temp.path().as_os_str())]); let status = daemon_status(false); let blocker = SessionCatalogOperation::Clear.cached_status_blocker(Some(&status)); @@ -81,7 +51,7 @@ fn clear_policy_allows_inactive_daemon() { #[test] fn duplicate_policy_uses_duplicate_status_message() { let temp = crate::test_temp::tempdir().unwrap(); - let _env = RuntimeEnvGuard::set_xdg_runtime_dir(temp.path()); + let _env = crate::test_env::EnvGuard::set(&[(XDG_RUNTIME_DIR_ENV, temp.path().as_os_str())]); let blocker = SessionCatalogOperation::Duplicate .cached_status_blocker(None) .expect("unknown status should block duplicate"); @@ -92,7 +62,7 @@ fn duplicate_policy_uses_duplicate_status_message() { #[test] fn move_policy_uses_move_status_message() { let temp = crate::test_temp::tempdir().unwrap(); - let _env = RuntimeEnvGuard::set_xdg_runtime_dir(temp.path()); + let _env = crate::test_env::EnvGuard::set(&[(XDG_RUNTIME_DIR_ENV, temp.path().as_os_str())]); let blocker = SessionCatalogOperation::Move .cached_status_blocker(None) .expect("unknown status should block move"); @@ -103,7 +73,7 @@ fn move_policy_uses_move_status_message() { #[test] fn clear_policy_blocks_unknown_daemon_status() { let temp = crate::test_temp::tempdir().unwrap(); - let _env = RuntimeEnvGuard::set_xdg_runtime_dir(temp.path()); + let _env = crate::test_env::EnvGuard::set(&[(XDG_RUNTIME_DIR_ENV, temp.path().as_os_str())]); let blocker = SessionCatalogOperation::Clear .cached_status_blocker(None) .expect("unknown status should block clear"); @@ -114,7 +84,7 @@ fn clear_policy_blocks_unknown_daemon_status() { #[test] fn clear_tool_state_policy_uses_tool_state_status_message() { let temp = crate::test_temp::tempdir().unwrap(); - let _env = RuntimeEnvGuard::set_xdg_runtime_dir(temp.path()); + let _env = crate::test_env::EnvGuard::set(&[(XDG_RUNTIME_DIR_ENV, temp.path().as_os_str())]); let blocker = SessionCatalogOperation::ClearToolState .cached_status_blocker(None) .expect("unknown status should block tool reset"); @@ -126,7 +96,7 @@ fn clear_tool_state_policy_uses_tool_state_status_message() { #[test] fn session_clear_transaction_guard_blocks_manual_daemon_lock() { let temp = crate::test_temp::tempdir().unwrap(); - let _env = RuntimeEnvGuard::set_xdg_runtime_dir(temp.path()); + let _env = crate::test_env::EnvGuard::set(&[(XDG_RUNTIME_DIR_ENV, temp.path().as_os_str())]); let _daemon_lock = acquire_runtime_lock_for_clear(RuntimeLockKind::Daemon).unwrap(); let error = acquire_runtime_lock_for_inactive_operation( RuntimeLockKind::Daemon, @@ -140,7 +110,7 @@ fn session_clear_transaction_guard_blocks_manual_daemon_lock() { #[test] fn inactive_session_reservation_holds_the_overlay_lock_but_not_the_daemon_lock() { let temp = crate::test_temp::tempdir().unwrap(); - let _env = RuntimeEnvGuard::set_xdg_runtime_dir(temp.path()); + let _env = crate::test_env::EnvGuard::set(&[(XDG_RUNTIME_DIR_ENV, temp.path().as_os_str())]); let reservation = reserve_inactive_session_operation(SessionCatalogOperation::Clear).unwrap(); @@ -161,7 +131,7 @@ fn inactive_session_reservation_holds_the_overlay_lock_but_not_the_daemon_lock() #[test] fn inactive_session_reservation_refuses_a_running_daemon() { let temp = crate::test_temp::tempdir().unwrap(); - let _env = RuntimeEnvGuard::set_xdg_runtime_dir(temp.path()); + let _env = crate::test_env::EnvGuard::set(&[(XDG_RUNTIME_DIR_ENV, temp.path().as_os_str())]); let _daemon_lock = acquire_runtime_lock_for_clear(RuntimeLockKind::Daemon).unwrap(); let error = match reserve_inactive_session_operation(SessionCatalogOperation::Move) { @@ -179,7 +149,7 @@ fn inactive_session_reservation_refuses_a_running_daemon() { #[test] fn cached_status_blocker_does_not_probe_runtime_locks() { let temp = crate::test_temp::tempdir().unwrap(); - let _env = RuntimeEnvGuard::set_xdg_runtime_dir(temp.path()); + let _env = crate::test_env::EnvGuard::set(&[(XDG_RUNTIME_DIR_ENV, temp.path().as_os_str())]); let _daemon_lock = acquire_runtime_lock_for_clear(RuntimeLockKind::Daemon).unwrap(); let _overlay_lock = acquire_runtime_lock_for_clear(RuntimeLockKind::Overlay).unwrap(); let status = daemon_status(false); @@ -195,7 +165,7 @@ fn cached_status_blocker_does_not_probe_runtime_locks() { #[test] fn clear_runtime_guards_hold_daemon_and_overlay_locks() { let temp = crate::test_temp::tempdir().unwrap(); - let _env = RuntimeEnvGuard::set_xdg_runtime_dir(temp.path()); + let _env = crate::test_env::EnvGuard::set(&[(XDG_RUNTIME_DIR_ENV, temp.path().as_os_str())]); let _daemon_lock = acquire_runtime_lock_for_clear(RuntimeLockKind::Daemon).unwrap(); let _overlay_lock = acquire_runtime_lock_for_clear(RuntimeLockKind::Overlay).unwrap(); diff --git a/configurator/src/app/update/config/tests.rs b/configurator/src/app/update/config/tests.rs index 3e3784c79..4a5e0b437 100644 --- a/configurator/src/app/update/config/tests.rs +++ b/configurator/src/app/update/config/tests.rs @@ -1,5 +1,4 @@ use std::path::{Path, PathBuf}; -use std::sync::atomic::{AtomicU64, Ordering}; use wayscriber::config::{Action, CURRENT_CONFIG_REVISION, Config, ConfigDocument}; @@ -14,17 +13,12 @@ fn status_contains(status: &StatusMessage, needle: &str) -> bool { status.text().is_some_and(|text| text.contains(needle)) } -static TEMP_SEQUENCE: AtomicU64 = AtomicU64::new(0); - -fn temp_config_document(name: &str, contents: &str) -> (PathBuf, Box) { - let sequence = TEMP_SEQUENCE.fetch_add(1, Ordering::Relaxed); - let path = std::env::temp_dir().join(format!( - "wayscriber-configurator-update-config-{}-{sequence}-{name}.toml", - std::process::id(), - )); +fn temp_config_document(name: &str, contents: &str) -> (TempDir, Box) { + let temp = crate::test_temp::tempdir().expect("temporary config directory"); + let path = temp.path().join(format!("{name}.toml")); std::fs::write(&path, contents).expect("write test config"); let document = ConfigDocument::load_from_path(&path).expect("load test config document"); - (path, Box::new(document)) + (temp, Box::new(document)) } #[test] @@ -32,7 +26,7 @@ fn handle_config_loaded_success_resets_loading_and_dirty_state() { let (mut app, _effects) = ConfiguratorApp::new_app(); app.is_dirty = true; - let (path, document) = temp_config_document("loaded", ""); + let (_path, document) = temp_config_document("loaded", ""); let _ = app.handle_config_loaded(Ok((document, None))); assert!(!app.document.is_loading()); @@ -42,14 +36,13 @@ fn handle_config_loaded_success_resets_loading_and_dirty_state() { &app.status, "Configuration loaded from disk." )); - let _ = std::fs::remove_file(path); } #[test] fn handle_config_loaded_uses_startup_search_focus_fallback_once() { let (mut app, _effects) = ConfiguratorApp::new_app(); - let (first_path, first) = temp_config_document("focus-first", ""); + let (_first_path, first) = temp_config_document("focus-first", ""); let _ = app.handle_config_loaded(Ok((first, None))); assert_eq!(app.search_focus_serial, 1); @@ -57,18 +50,16 @@ fn handle_config_loaded_uses_startup_search_focus_fallback_once() { // A reload is not a relaunch: the offer was answered by the first load, // so the caret stays wherever the user put it. - let (second_path, second) = temp_config_document("focus-second", ""); + let (_second_path, second) = temp_config_document("focus-second", ""); let _ = app.handle_config_loaded(Ok((second, None))); assert_eq!(app.search_focus_serial, 1); - let _ = std::fs::remove_file(first_path); - let _ = std::fs::remove_file(second_path); } #[test] fn handle_config_loaded_error_preserves_the_last_good_document_and_draft() { let (mut app, _effects) = ConfiguratorApp::new_app(); - let (path, document) = temp_config_document("before-reload-error", ""); + let (_path, document) = temp_config_document("before-reload-error", ""); let destination = document.destination().to_path_buf(); let _ = app.handle_config_loaded(Ok((document, None))); app.draft.capture.enabled = !app.draft.capture.enabled; @@ -90,13 +81,12 @@ fn handle_config_loaded_error_preserves_the_last_good_document_and_draft() { &app.status, "Failed to load config from disk: broken" )); - let _ = std::fs::remove_file(path); } #[test] fn handle_config_loaded_repair_document_allows_saving() { let (mut app, _effects) = ConfiguratorApp::new_app(); - let (path, document) = temp_config_document("repair", ""); + let (_path, document) = temp_config_document("repair", ""); let _ = app.handle_config_loaded(Ok(( document, @@ -112,20 +102,18 @@ fn handle_config_loaded_repair_document_allows_saving() { )); let _ = app.handle_save_requested(); assert!(app.document.is_saving()); - let _ = std::fs::remove_file(path); } #[test] fn handle_config_loaded_surfaces_preserved_unknown_settings() { let (mut app, _effects) = ConfiguratorApp::new_app(); - let (path, document) = temp_config_document("unknown", "future_configurator_option = true\n"); + let (_path, document) = temp_config_document("unknown", "future_configurator_option = true\n"); let _ = app.handle_config_loaded(Ok((document, None))); assert!(matches!(app.status, StatusMessage::Warning(_))); assert!(status_contains(&app.status, "future_configurator_option")); assert!(status_contains(&app.status, "were preserved")); - let _ = std::fs::remove_file(path); } /// A resolved shortcut conflict is never written back, so the editor is @@ -134,7 +122,7 @@ fn handle_config_loaded_surfaces_preserved_unknown_settings() { #[test] fn handle_config_loaded_surfaces_resolved_shortcut_conflicts() { let (mut app, _effects) = ConfiguratorApp::new_app(); - let (path, document) = temp_config_document( + let (_path, document) = temp_config_document( "shortcut-conflict", &format!( "config_revision = {}\n\n[keybindings]\ntoggle_toolbar = [\"F2\"]\ncycle_toolbar_display = [\"F2\"]\n", @@ -153,7 +141,6 @@ fn handle_config_loaded_surfaces_resolved_shortcut_conflicts() { !status_contains(&app.status, "Unrecognized settings"), "a conflict is not an unknown setting" ); - let _ = std::fs::remove_file(path); } /// A default this build added and the file never mentions gets its own @@ -162,7 +149,7 @@ fn handle_config_loaded_surfaces_resolved_shortcut_conflicts() { #[test] fn handle_config_loaded_surfaces_skipped_default_shortcuts() { let (mut app, _effects) = ConfiguratorApp::new_app(); - let (path, document) = temp_config_document( + let (_path, document) = temp_config_document( "skipped-default", &format!( "config_revision = {}\n\n[keybindings]\ntoggle_toolbar = [\"F2\", \"F9\"]\n", @@ -184,7 +171,6 @@ fn handle_config_loaded_surfaces_skipped_default_shortcuts() { && !status_contains(&app.status, "Unrecognized settings"), "a skipped default is neither a conflict nor an unknown setting" ); - let _ = std::fs::remove_file(path); } /// A string the parser rejects is dropped for the session and kept by the @@ -193,7 +179,7 @@ fn handle_config_loaded_surfaces_skipped_default_shortcuts() { #[test] fn handle_config_loaded_surfaces_shortcuts_that_could_not_be_parsed() { let (mut app, _effects) = ConfiguratorApp::new_app(); - let (path, document) = temp_config_document( + let (_path, document) = temp_config_document( "invalid-shortcut", &format!( "config_revision = {}\n\n[keybindings]\nclear_canvas = [\"Ctrl+Shift\"]\n", @@ -212,7 +198,6 @@ fn handle_config_loaded_surfaces_shortcuts_that_could_not_be_parsed() { && !status_contains(&app.status, "Conflicting shortcuts"), "an unparseable shortcut is neither an unknown setting nor a conflict" ); - let _ = std::fs::remove_file(path); } /// All three keybinding kinds can land in one file, and each gets its own @@ -220,7 +205,7 @@ fn handle_config_loaded_surfaces_shortcuts_that_could_not_be_parsed() { #[test] fn handle_config_loaded_separates_every_keybinding_diagnostic_kind() { let (mut app, _effects) = ConfiguratorApp::new_app(); - let (path, document) = temp_config_document( + let (_path, document) = temp_config_document( "invalid-and-conflicting", &format!( "config_revision = {}\n\n[keybindings]\nclear_canvas = [\"Ctrl+Shift\"]\ntoggle_toolbar = [\"F2\", \"F9\"]\nundo = [\"Ctrl+Alt+U\"]\nredo = [\"Ctrl+Alt+U\"]\n", @@ -240,7 +225,6 @@ fn handle_config_loaded_separates_every_keybinding_diagnostic_kind() { assert!(status_contains(&app.status, "Ctrl+Shift")); assert!(status_contains(&app.status, "Ctrl+Alt+U")); assert!(status_contains(&app.status, "F2")); - let _ = std::fs::remove_file(path); } #[test] @@ -264,7 +248,7 @@ fn handle_save_requested_blocks_without_loaded_document() { fn handle_save_requested_sets_saving_for_valid_draft() { let (mut app, _effects) = ConfiguratorApp::new_app(); app.document.set_saving_for_test(false); - let (path, document) = temp_config_document("save-request", ""); + let (_path, document) = temp_config_document("save-request", ""); let _ = app.handle_config_loaded(Ok((document, None))); let effects = app.handle_save_requested(); @@ -272,7 +256,6 @@ fn handle_save_requested_sets_saving_for_valid_draft() { assert!(matches!(effects.as_slice(), [Effect::SaveConfig { .. }])); assert!(app.document.is_saving()); assert!(status_contains(&app.status, "Saving configuration...")); - let _ = std::fs::remove_file(path); } /// The document is the model's only copy, and the write needs it moved. It @@ -868,7 +851,7 @@ fn a_typed_shortcut_the_parser_rejects_is_reported_by_the_save() { #[test] fn save_is_refused_while_a_reload_is_in_flight() { let (mut app, _effects) = ConfiguratorApp::new_app(); - let (path, document) = temp_config_document("save-during-reload", ""); + let (_path, document) = temp_config_document("save-during-reload", ""); app.document.finish_load(Some(*document)); app.document.set_loading_for_test(true); app.is_dirty = true; @@ -886,7 +869,6 @@ fn save_is_refused_while_a_reload_is_in_flight() { format!("{before:?}"), "a refused save must not claim it is saving" ); - let _ = std::fs::remove_file(path); } #[test] @@ -896,7 +878,7 @@ fn handle_config_saved_success_clears_dirty_and_records_backup() { app.is_dirty = true; app.draft.capture.enabled = !app.draft.capture.enabled; let backup = PathBuf::from("/tmp/wayscriber-config.bak"); - let (path, document) = temp_config_document("saved", ""); + let (_path, document) = temp_config_document("saved", ""); let _ = app.handle_config_saved(Ok((Some(backup.clone()), document))); assert!(app.document.loaded().is_some()); @@ -909,7 +891,6 @@ fn handle_config_saved_success_clears_dirty_and_records_backup() { &app.status, "Configuration saved successfully." )); - let _ = std::fs::remove_file(path); } const LEGACY_REVISION_ZERO_CONFIG: &str = "config_revision = 0\n\n[drawing]\ndefault_thickness = 3.0\n\n[keybindings]\ntoggle_command_palette = [\"Ctrl+K\"]\ncapture_full_screen = [\"Ctrl+Shift+P\"]\n"; @@ -1455,7 +1436,8 @@ fn document_operations_freeze_queued_edits_until_completion() { for saving in [false, true] { for success in [false, true] { let (mut app, _) = ConfiguratorApp::new_app(); - let (path, document) = temp_config_document("busy-edits", ""); + let (_temp, document) = temp_config_document("busy-edits", ""); + let path = document.destination().to_path_buf(); app.update_command(CommandMessage::ConfigLoaded(Ok((document, None)))); let original = app.draft.clone(); let effects = app.update_message(if saving { @@ -1505,7 +1487,6 @@ fn document_operations_freeze_queued_edits_until_completion() { !original.capture.enabled, )); assert_ne!(app.draft, original); - std::fs::remove_file(path).unwrap(); } } } @@ -1522,7 +1503,7 @@ fn leave_dirty_draft_requires_a_current_decision_and_successful_save() { fn check_leave_dirty_draft(close: bool, save_succeeds: bool) { use crate::messages::{CommandMessage, Message}; let (mut app, _) = ConfiguratorApp::new_app(); - let (path, document) = temp_config_document("leave-draft", ""); + let (_path, document) = temp_config_document("leave-draft", ""); app.update_command(CommandMessage::ConfigLoaded(Ok((document, None)))); let request = if close { Message::CloseRequested @@ -1580,7 +1561,6 @@ fn check_leave_dirty_draft(close: bool, save_succeeds: bool) { [Effect::CloseWindow] | [Effect::LoadConfig] )); } - std::fs::remove_file(path).unwrap(); } #[test] @@ -1589,7 +1569,7 @@ fn leave_protects_raw_color_and_unapplied_shortcut_input() { for shortcut in [false, true] { let (mut app, _) = ConfiguratorApp::new_app(); - let (path, document) = temp_config_document("leave-editor", ""); + let (_path, document) = temp_config_document("leave-editor", ""); app.update_command(CommandMessage::ConfigLoaded(Ok((document, None)))); if shortcut { app.update_message(Message::ShortcutTextEditStarted( @@ -1609,6 +1589,5 @@ fn leave_protects_raw_color_and_unapplied_shortcut_input() { assert!(app.has_unresolved_editor()); assert!(!app.document.is_saving()); assert!(matches!(app.status, StatusMessage::Error(_))); - std::fs::remove_file(path).unwrap(); } } diff --git a/configurator/src/app/update/session_catalog/tests.rs b/configurator/src/app/update/session_catalog/tests.rs index cad776983..d87b7cae2 100644 --- a/configurator/src/app/update/session_catalog/tests.rs +++ b/configurator/src/app/update/session_catalog/tests.rs @@ -1,6 +1,4 @@ -use std::ffi::OsString; -use std::path::{Path, PathBuf}; -use std::sync::MutexGuard; +use std::path::PathBuf; use super::*; use crate::models::{ @@ -9,34 +7,6 @@ use crate::models::{ }; use wayscriber::env_vars::XDG_RUNTIME_DIR_ENV; -struct RuntimeEnvGuard { - previous: Option, - _guard: MutexGuard<'static, ()>, -} - -impl RuntimeEnvGuard { - fn set_xdg_runtime_dir(path: &Path) -> Self { - let guard = crate::test_env::lock(); - let previous = std::env::var_os(XDG_RUNTIME_DIR_ENV); - unsafe { - std::env::set_var(XDG_RUNTIME_DIR_ENV, path); - } - Self { - previous, - _guard: guard, - } - } -} - -impl Drop for RuntimeEnvGuard { - fn drop(&mut self) { - match self.previous.take() { - Some(value) => unsafe { std::env::set_var(XDG_RUNTIME_DIR_ENV, value) }, - None => unsafe { std::env::remove_var(XDG_RUNTIME_DIR_ENV) }, - } - } -} - fn catalog_item(id: &str, display_name: &str) -> SessionCatalogItem { SessionCatalogItem { id: id.to_string(), @@ -153,7 +123,7 @@ fn catalog_load_preserves_a_defaults_confirmation() { #[test] fn duplicate_request_blocks_without_daemon_status() { let temp = crate::test_temp::tempdir().unwrap(); - let _env = RuntimeEnvGuard::set_xdg_runtime_dir(temp.path()); + let _env = crate::test_env::EnvGuard::set(&[(XDG_RUNTIME_DIR_ENV, temp.path().as_os_str())]); let (mut app, _effects) = ConfiguratorApp::new_app(); app.session_catalog = SessionCatalogState::loading(); app.session_catalog @@ -170,7 +140,7 @@ fn duplicate_request_blocks_without_daemon_status() { #[test] fn duplicate_request_sets_busy_when_safe() { let temp = crate::test_temp::tempdir().unwrap(); - let _env = RuntimeEnvGuard::set_xdg_runtime_dir(temp.path()); + let _env = crate::test_env::EnvGuard::set(&[(XDG_RUNTIME_DIR_ENV, temp.path().as_os_str())]); let (mut app, _effects) = ConfiguratorApp::new_app(); app.session_catalog = SessionCatalogState::loading(); app.session_catalog @@ -190,7 +160,7 @@ fn duplicate_request_sets_busy_when_safe() { #[test] fn move_request_blocks_without_daemon_status() { let temp = crate::test_temp::tempdir().unwrap(); - let _env = RuntimeEnvGuard::set_xdg_runtime_dir(temp.path()); + let _env = crate::test_env::EnvGuard::set(&[(XDG_RUNTIME_DIR_ENV, temp.path().as_os_str())]); let (mut app, _effects) = ConfiguratorApp::new_app(); app.session_catalog = SessionCatalogState::loading(); app.session_catalog @@ -207,7 +177,7 @@ fn move_request_blocks_without_daemon_status() { #[test] fn move_request_sets_busy_when_safe() { let temp = crate::test_temp::tempdir().unwrap(); - let _env = RuntimeEnvGuard::set_xdg_runtime_dir(temp.path()); + let _env = crate::test_env::EnvGuard::set(&[(XDG_RUNTIME_DIR_ENV, temp.path().as_os_str())]); let (mut app, _effects) = ConfiguratorApp::new_app(); app.session_catalog = SessionCatalogState::loading(); app.session_catalog @@ -227,7 +197,7 @@ fn move_request_sets_busy_when_safe() { #[test] fn clear_request_blocks_without_daemon_status() { let temp = crate::test_temp::tempdir().unwrap(); - let _env = RuntimeEnvGuard::set_xdg_runtime_dir(temp.path()); + let _env = crate::test_env::EnvGuard::set(&[(XDG_RUNTIME_DIR_ENV, temp.path().as_os_str())]); let (mut app, _effects) = ConfiguratorApp::new_app(); app.session_catalog = SessionCatalogState::loading(); app.session_catalog @@ -243,7 +213,7 @@ fn clear_request_blocks_without_daemon_status() { #[test] fn clear_tool_state_request_blocks_without_daemon_status() { let temp = crate::test_temp::tempdir().unwrap(); - let _env = RuntimeEnvGuard::set_xdg_runtime_dir(temp.path()); + let _env = crate::test_env::EnvGuard::set(&[(XDG_RUNTIME_DIR_ENV, temp.path().as_os_str())]); let (mut app, _effects) = ConfiguratorApp::new_app(); app.session_catalog = SessionCatalogState::loading(); app.session_catalog @@ -260,7 +230,7 @@ fn clear_tool_state_request_blocks_without_daemon_status() { #[test] fn clear_tool_state_request_sets_busy_when_safe() { let temp = crate::test_temp::tempdir().unwrap(); - let _env = RuntimeEnvGuard::set_xdg_runtime_dir(temp.path()); + let _env = crate::test_env::EnvGuard::set(&[(XDG_RUNTIME_DIR_ENV, temp.path().as_os_str())]); let (mut app, _effects) = ConfiguratorApp::new_app(); app.session_catalog = SessionCatalogState::loading(); app.session_catalog @@ -281,7 +251,7 @@ fn clear_tool_state_request_sets_busy_when_safe() { #[test] fn clear_request_sets_pending_confirmation_when_safe() { let temp = crate::test_temp::tempdir().unwrap(); - let _env = RuntimeEnvGuard::set_xdg_runtime_dir(temp.path()); + let _env = crate::test_env::EnvGuard::set(&[(XDG_RUNTIME_DIR_ENV, temp.path().as_os_str())]); let (mut app, _effects) = ConfiguratorApp::new_app(); app.session_catalog = SessionCatalogState::loading(); app.session_catalog @@ -432,7 +402,7 @@ fn stale_clear_cancel_does_not_disarm_a_newer_confirmation() { #[test] fn clear_confirmed_consumes_the_pending_confirmation() { let temp = crate::test_temp::tempdir().expect("temporary test directory"); - let _env = RuntimeEnvGuard::set_xdg_runtime_dir(temp.path()); + let _env = crate::test_env::EnvGuard::set(&[(XDG_RUNTIME_DIR_ENV, temp.path().as_os_str())]); let (mut app, _effects) = ConfiguratorApp::new_app(); app.session_catalog = SessionCatalogState::loading(); app.session_catalog @@ -456,7 +426,7 @@ fn clear_confirmed_consumes_the_pending_confirmation() { #[test] fn clear_confirmed_twice_starts_only_one_clear() { let temp = crate::test_temp::tempdir().expect("temporary test directory"); - let _env = RuntimeEnvGuard::set_xdg_runtime_dir(temp.path()); + let _env = crate::test_env::EnvGuard::set(&[(XDG_RUNTIME_DIR_ENV, temp.path().as_os_str())]); let (mut app, _effects) = ConfiguratorApp::new_app(); app.session_catalog = SessionCatalogState::loading(); app.session_catalog @@ -478,7 +448,7 @@ fn clear_confirmed_twice_starts_only_one_clear() { #[test] fn clear_confirmed_for_another_row_leaves_the_pending_one_armed() { let temp = crate::test_temp::tempdir().expect("temporary test directory"); - let _env = RuntimeEnvGuard::set_xdg_runtime_dir(temp.path()); + let _env = crate::test_env::EnvGuard::set(&[(XDG_RUNTIME_DIR_ENV, temp.path().as_os_str())]); let (mut app, _effects) = ConfiguratorApp::new_app(); app.session_catalog = SessionCatalogState::loading(); app.session_catalog.replace_items(vec![ diff --git a/configurator/src/models/config/tests.rs b/configurator/src/models/config/tests.rs index 1cf5dc39b..063cf139a 100644 --- a/configurator/src/models/config/tests.rs +++ b/configurator/src/models/config/tests.rs @@ -1824,3 +1824,50 @@ fn compiled_defaults_are_not_shown_as_authored_keybindings() { "Config::default() is AllExplicit for validation, not for UI source badges" ); } + +#[test] +fn config_draft_numeric_policy_accepts_bounds_and_rejects_invalid_values() { + let config = Config::default(); + for (length, angle, radius, outline, duration) in [ + ("5", "15", "16", "1", "150"), + ("50", "60", "160", "12", "1500"), + ] { + let mut draft = ConfigDraft::from_config(&config); + draft.arrow_length = length.into(); + draft.arrow_angle = angle.into(); + draft.click_highlight_radius = radius.into(); + draft.click_highlight_outline_thickness = outline.into(); + draft.click_highlight_duration_ms = duration.into(); + let converted = draft.to_config(&config).unwrap(); + assert!(converted.validate_for_save().is_ok()); + } + + for (length, angle, radius, outline, duration) in [ + ("4.9", "14.9", "15.9", "0.9", "149"), + ("50.1", "60.1", "160.1", "12.1", "1501"), + ("NaN", "NaN", "NaN", "NaN", "NaN"), + ("inf", "inf", "inf", "inf", "inf"), + ("-inf", "-inf", "-inf", "-inf", "-inf"), + ] { + let mut draft = ConfigDraft::from_config(&config); + draft.arrow_length = length.into(); + draft.arrow_angle = angle.into(); + draft.click_highlight_radius = radius.into(); + draft.click_highlight_outline_thickness = outline.into(); + draft.click_highlight_duration_ms = duration.into(); + let errors = draft.to_config(&config).unwrap_err(); + for field in [ + "arrow.length", + "arrow.angle_degrees", + "ui.click_highlight.radius", + "ui.click_highlight.outline_thickness", + "ui.click_highlight.duration_ms", + ] { + assert!( + errors.iter().any(|error| error.field == field), + "{field} for {radius}" + ); + } + assert_eq!(draft.click_highlight_radius, radius); + } +} diff --git a/configurator/src/models/config/to_config/drawing.rs b/configurator/src/models/config/to_config/drawing.rs index 4439e743e..ec97f6f9e 100644 --- a/configurator/src/models/config/to_config/drawing.rs +++ b/configurator/src/models/config/to_config/drawing.rs @@ -3,8 +3,10 @@ use super::super::parse::{ parse_field_in_range, parse_u8_in_range, parse_usize_at_least, parse_usize_in_range, }; use crate::models::error::FormError; -use wayscriber::config::Config; use wayscriber::config::MAX_SHAPE_RECOGNITION_SENSITIVITY; +use wayscriber::config::{ + ARROW_ANGLE_MAX, ARROW_ANGLE_MIN, ARROW_LENGTH_MAX, ARROW_LENGTH_MIN, Config, +}; use wayscriber::domain::{DragBindableTool, DragTool}; use wayscriber::domain::{MAX_STROKE_THICKNESS, MIN_STROKE_THICKNESS}; use wayscriber::draw::{MAX_PEN_SMOOTHING, REGULAR_POLYGON_MAX_SIDES, REGULAR_POLYGON_MIN_SIDES}; @@ -135,16 +137,16 @@ impl ConfigDraft { parse_field_in_range( &self.arrow_length, "arrow.length", - 5.0, - 50.0, + ARROW_LENGTH_MIN, + ARROW_LENGTH_MAX, errors, |value| config.arrow.length = value, ); parse_field_in_range( &self.arrow_angle, "arrow.angle_degrees", - 15.0, - 60.0, + ARROW_ANGLE_MIN, + ARROW_ANGLE_MAX, errors, |value| config.arrow.angle_degrees = value, ); diff --git a/configurator/src/models/config/to_config/ui.rs b/configurator/src/models/config/to_config/ui.rs index 6dda4af36..b57a88fcd 100644 --- a/configurator/src/models/config/to_config/ui.rs +++ b/configurator/src/models/config/to_config/ui.rs @@ -1,7 +1,13 @@ use super::super::draft::ConfigDraft; -use super::super::parse::{parse_field, parse_u64_field, parse_usize_field}; +use super::super::parse::{ + parse_field, parse_field_in_range, parse_u64_field, parse_u64_in_range, parse_usize_field, +}; use crate::models::error::FormError; -use wayscriber::config::{Config, XdgFocusLossBehavior}; +use wayscriber::config::{ + CLICK_HIGHLIGHT_DURATION_MAX_MS, CLICK_HIGHLIGHT_DURATION_MIN_MS, CLICK_HIGHLIGHT_OUTLINE_MAX, + CLICK_HIGHLIGHT_OUTLINE_MIN, CLICK_HIGHLIGHT_RADIUS_MAX, CLICK_HIGHLIGHT_RADIUS_MIN, Config, + XdgFocusLossBehavior, +}; impl ConfigDraft { pub(super) fn apply_ui(&self, config: &mut Config, errors: &mut Vec) { @@ -126,21 +132,27 @@ impl ConfigDraft { self.click_highlight_show_on_highlight_tool; config.ui.click_highlight.use_pen_color = self.click_highlight_use_pen_color; config.ui.click_highlight.force_in_light_mode = self.click_highlight_force_in_light_mode; - parse_field( + parse_field_in_range( &self.click_highlight_radius, "ui.click_highlight.radius", + CLICK_HIGHLIGHT_RADIUS_MIN, + CLICK_HIGHLIGHT_RADIUS_MAX, errors, |value| config.ui.click_highlight.radius = value, ); - parse_field( + parse_field_in_range( &self.click_highlight_outline_thickness, "ui.click_highlight.outline_thickness", + CLICK_HIGHLIGHT_OUTLINE_MIN, + CLICK_HIGHLIGHT_OUTLINE_MAX, errors, |value| config.ui.click_highlight.outline_thickness = value, ); - parse_u64_field( + parse_u64_in_range( &self.click_highlight_duration_ms, "ui.click_highlight.duration_ms", + CLICK_HIGHLIGHT_DURATION_MIN_MS, + CLICK_HIGHLIGHT_DURATION_MAX_MS, errors, |value| config.ui.click_highlight.duration_ms = value, ); diff --git a/configurator/src/test_env.rs b/configurator/src/test_env.rs index 78538025d..e61bd1217 100644 --- a/configurator/src/test_env.rs +++ b/configurator/src/test_env.rs @@ -7,3 +7,60 @@ pub(crate) fn lock() -> MutexGuard<'static, ()> { .lock() .unwrap_or_else(|poisoned| poisoned.into_inner()) } + +/// Holds the shared environment lock until every changed variable is restored. +pub(crate) struct EnvGuard { + previous: Vec<(&'static str, Option)>, + _lock: MutexGuard<'static, ()>, +} + +impl EnvGuard { + pub(crate) fn set(values: &[(&'static str, &std::ffi::OsStr)]) -> Self { + let lock = lock(); + let previous = values + .iter() + .map(|(key, _)| (*key, std::env::var_os(key))) + .collect(); + for (key, value) in values { + // SAFETY: mutations are serialized by the shared test lock. + unsafe { + std::env::set_var(key, value); + } + } + Self { + previous, + _lock: lock, + } + } +} + +impl Drop for EnvGuard { + fn drop(&mut self) { + for (key, value) in self.previous.drain(..) { + // SAFETY: the lock remains held until restoration completes. + unsafe { + match value { + Some(value) => std::env::set_var(key, value), + None => std::env::remove_var(key), + } + } + } + } +} + +#[test] +fn guard_restores_environment_after_a_panic() { + const KEY: &str = "WAYSCRIBER_CONFIGURATOR_TEST_PANIC_RESTORE"; + let before = std::env::var_os(KEY); + let result = std::panic::catch_unwind(|| { + let _env = EnvGuard::set(&[(KEY, std::ffi::OsStr::new("during"))]); + assert_eq!( + std::env::var_os(KEY).as_deref(), + Some(std::ffi::OsStr::new("during")) + ); + panic!("exercise fixture unwind"); + }); + assert!(result.is_err()); + let _lock = lock(); + assert_eq!(std::env::var_os(KEY), before); +} diff --git a/configurator/src/test_temp.rs b/configurator/src/test_temp.rs index 154db697b..15be825cc 100644 --- a/configurator/src/test_temp.rs +++ b/configurator/src/test_temp.rs @@ -1,41 +1 @@ -use std::path::{Path, PathBuf}; -use std::sync::atomic::{AtomicU64, Ordering}; -use std::{fs, io}; - -static NEXT_TEMP_ID: AtomicU64 = AtomicU64::new(0); - -pub(crate) struct TempDir { - path: PathBuf, -} - -impl TempDir { - pub(crate) fn path(&self) -> &Path { - &self.path - } -} - -impl Drop for TempDir { - fn drop(&mut self) { - let _ = fs::remove_dir_all(&self.path); - } -} - -pub(crate) fn tempdir() -> io::Result { - let base = std::env::temp_dir(); - let pid = std::process::id(); - - for _ in 0..100 { - let id = NEXT_TEMP_ID.fetch_add(1, Ordering::Relaxed); - let path = base.join(format!("wayscriber-configurator-test-{pid}-{id}")); - match fs::create_dir(&path) { - Ok(()) => return Ok(TempDir { path }), - Err(err) if err.kind() == io::ErrorKind::AlreadyExists => continue, - Err(err) => return Err(err), - } - } - - Err(io::Error::new( - io::ErrorKind::AlreadyExists, - "failed to create a unique temporary test directory", - )) -} +pub(crate) use tempfile::{TempDir, tempdir}; diff --git a/docs/CONFIG.md b/docs/CONFIG.md index 184ff615e..bfe2fe2f0 100644 --- a/docs/CONFIG.md +++ b/docs/CONFIG.md @@ -9,6 +9,20 @@ wayscriber supports customization through a TOML configuration file located at: All settings are optional. If the configuration file doesn't exist or settings are missing, sensible defaults will be used. +### Non-finite numbers + +Floating-point settings must be finite. When a hand-edited file contains `nan`, `inf`, or +`-inf`, loading logs a warning and uses the field's built-in default before applying its +normal range checks. Exceptions are preset sizes (which use the minimum size, 1.0), optional +preset numeric overrides (which are ignored), and custom board RGB components (which use +0.0). Each color component is checked independently. Tablet thickness defaults are restored +before ordering the minimum and maximum. + +Finite values keep their existing range policy; fields without load-time bounds, such as +help/status styling and toolbar offsets, retain finite authored values. Loading does not +rewrite the file. Configurator saves still report invalid numeric input rather than silently +saving corrected values. + ### Configured defaults and runtime UI preferences `config.toml` is the authored source for configured defaults. Some direct overlay customizations are @@ -1083,6 +1097,12 @@ font_size = 18.0 enabled = true ``` +Click-highlight radius accepts 16–160 pixels, outline thickness 1–12 pixels, +and duration 150–1500 milliseconds. Finite out-of-range values are clamped on +load. Non-finite radius, thickness, and RGBA components reset to their respective +defaults shown above. The configurator keeps invalid input visible and refuses +to save it. + **Status Bar:** - Shows current color, pen thickness, and active tool - Press F1/F10 to toggle help overlay @@ -2045,6 +2065,10 @@ persist_whiteboard = true persist_blackboard = true persist_history = true restore_tool_state = true +autosave_enabled = true +autosave_idle_ms = 5000 +autosave_interval_ms = 45000 +autosave_failure_backoff_ms = 5000 storage = "auto" # custom_directory = "/absolute/path" per_output = true @@ -2059,6 +2083,10 @@ backup_retention = 1 - `persist_*` — choose which boards survive restarts (`persist_transparent` for overlay, `persist_whiteboard`/`persist_blackboard` gate non-transparent boards for legacy compatibility) - `persist_history` — when `true`, persist undo/redo stacks so that history survives restarts; set to `false` to save only visible drawings - `restore_tool_state` — save pen colour, thickness, font size, arrow settings (including head placement), and the starting Spotlight magnification; when `true`, the last-used tool state overrides config defaults at startup. Chrome is not tool state: status bar and badge visibility come from `[ui]` on every start, and an overlay toggle of them applies to that run only. Sessions written by older releases still carry a `show_status_bar` value; it is ignored on load and no longer written +- `autosave_enabled` — when `true` (default), save dirty session data in the background while the overlay is running; at least one board, history, or tool-state persistence option must be enabled. Set to `false` to disable these periodic writes; explicit saves and normal exit persistence still apply. +- `autosave_idle_ms` — idle debounce in milliseconds, default `5000`: an edit restarts this timer. +- `autosave_interval_ms` — maximum dirty scheduling interval in milliseconds, default `45000`: autosave becomes due when either the idle debounce or this interval elapses, so continuous edits do not restart both timers. Active gestures, an in-flight write, or a session operation may defer the write. +- `autosave_failure_backoff_ms` — wait before retrying a failed autosave, in milliseconds, default `5000`. Failed writes keep the session dirty. All three timing values have a minimum of `1000` milliseconds on load; the configurator rejects invalid drafts. - `storage` — `auto` (XDG data dir, e.g. `~/.local/share/wayscriber`), `config` (same directory as `config.toml`), or `custom` - `custom_directory` — absolute path used when `storage = "custom"`; supports `~` - `per_output` — when `true` (default) keep a separate session file for each monitor; set to `false` to share one file per Wayland display as in earlier releases diff --git a/docs/codebase-overview.md b/docs/codebase-overview.md index d7d06805a..489a2b7cc 100644 --- a/docs/codebase-overview.md +++ b/docs/codebase-overview.md @@ -666,3 +666,12 @@ the board picker owns only its draft. Session format 7 carries appearance and its explicit-override provenance independently of page history. The regression and performance evidence for this work is kept with the internal documentation rather than in this repository. + +Action metadata stores `ActionIcon` identities in `ActionMeta.icon`. Rust callers +that previously invoked that field as a Cairo painter now resolve it with +`toolbar_icons::action_icon_painter(icon)`. Config serialization and action labels +are unchanged. Pressure thickness policy lives in `domain`, with the established +`input::state` re-exports retained. Preset runtime conversion methods and +`UiTheme::to_theme_mode` keep their public signatures but live beside their +runtime consumers. Advisory file locks live in `durable_io`; established session +lock paths remain available. diff --git a/src/backend/wayland/clipboard/image.rs b/src/backend/wayland/clipboard/image.rs index b3a0906a4..8eafe9226 100644 --- a/src/backend/wayland/clipboard/image.rs +++ b/src/backend/wayland/clipboard/image.rs @@ -67,18 +67,3 @@ fn canonical_image_mime_type(format: EncodedImageFormat) -> &'static str { EncodedImageFormat::Jpeg => "image/jpeg", } } - -#[cfg(test)] -mod tests { - use crate::screen_pixels::EmbeddedImageLimits; - - #[test] - fn image_byte_cap_leaves_room_for_default_persisted_create_history() { - let encoded_len = EmbeddedImageLimits::default().max_bytes().div_ceil(3) * 4; - let duplicated_history_len = encoded_len * 2; - let default_session_budget = 50 * 1024 * 1024; - let json_margin = 512 * 1024; - - assert!(duplicated_history_len + json_margin < default_session_budget); - } -} diff --git a/src/backend/wayland/runtime_ui_state/tests.rs b/src/backend/wayland/runtime_ui_state/tests.rs index 4d10b644c..db5a9e921 100644 --- a/src/backend/wayland/runtime_ui_state/tests.rs +++ b/src/backend/wayland/runtime_ui_state/tests.rs @@ -49,7 +49,7 @@ fn test_runtime_allow_startup_incident(config: &Config, path: &Path) -> ToolbarR lifecycle: RuntimeUiLifecycleState::startup(bootstrap.startup_incident), board_pin_seeds, deferred_board_pin_restores: BTreeMap::new(), - writer: Some(RuntimeUiStateWriter::spawn(store).unwrap()), + writer: Some(RuntimeUiStateWriter::spawn_with_completion_notifier(store, || {}).unwrap()), pending_writer_command: None, live_rebuild_pending: false, item_drag: None, diff --git a/src/backend/wayland/state/helper_launch.rs b/src/backend/wayland/state/helper_launch.rs index 711881608..d921acbc7 100644 --- a/src/backend/wayland/state/helper_launch.rs +++ b/src/backend/wayland/state/helper_launch.rs @@ -25,7 +25,7 @@ fn spawn_detached( } fn launch_deferred_by_busy_broker(error: &anyhow::Error) -> bool { - format!("{error:#}").contains(crate::process_broker::BROKER_BUSY) + crate::process_broker::error_kind(error) == Some(crate::process_broker::BrokerErrorKind::Busy) } fn launch_failure_message(error: &anyhow::Error, failed: &'static str) -> &'static str { @@ -131,20 +131,12 @@ impl WaylandState { mod tests { use super::*; - #[test] - fn busy_broker_failure_is_retryable() { - let error = anyhow::anyhow!("transport failed: {}", crate::process_broker::BROKER_BUSY); - - assert!(launch_deferred_by_busy_broker(&error)); - assert_eq!( - launch_failure_message(&error, "failed"), - "Busy with another task. Try again in a moment." - ); - } - #[test] fn ordinary_launch_failure_keeps_specific_notice() { - let error = anyhow::anyhow!("executable not found"); + let error = anyhow::anyhow!( + "unrelated diagnostic: process broker is busy running another helper; No such file" + ) + .context("launch context"); assert!(!launch_deferred_by_busy_broker(&error)); assert_eq!( diff --git a/src/backend/wayland/state/toolbar/events/session/dialog.rs b/src/backend/wayland/state/toolbar/events/session/dialog.rs index 817fc896e..14fadc543 100644 --- a/src/backend/wayland/state/toolbar/events/session/dialog.rs +++ b/src/backend/wayland/state/toolbar/events/session/dialog.rs @@ -1,7 +1,10 @@ -use anyhow::{Context, Result, anyhow}; +use crate::backend::wayland::runtime_operation::{ + RuntimeOperationController, RuntimeOperationIdSource, RuntimeOperationPoll, +}; + +use anyhow::{Result, anyhow}; use std::ffi::{OsStr, OsString}; use std::path::{Path, PathBuf}; -use std::sync::mpsc; use std::time::Duration; const SESSION_FILE_EXTENSION: &str = "wayscriber-session"; @@ -24,14 +27,17 @@ pub(in crate::backend::wayland::state) struct SessionFileDialogCompletion { pub(in crate::backend::wayland::state) result: Result, String>, } -type SessionFileDialogMessage = (u64, SessionFileDialogMode, Result, String>); - -#[derive(Debug)] pub(in crate::backend::wayland::state) struct SessionFileDialogController { - next_id: u64, - active: Option<(u64, SessionFileDialogMode)>, - receiver: Option>, - runtime_wake: crate::backend::wayland::RuntimeWakeHandle, + operation: RuntimeOperationController, String>>, +} + +impl std::fmt::Debug for SessionFileDialogController { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter + .debug_struct("SessionFileDialogController") + .field("active", &self.operation.is_active()) + .finish() + } } impl SessionFileDialogController { @@ -39,10 +45,10 @@ impl SessionFileDialogController { runtime_wake: crate::backend::wayland::RuntimeWakeHandle, ) -> Self { Self { - next_id: 1, - active: None, - receiver: None, - runtime_wake, + operation: RuntimeOperationController::new( + RuntimeOperationIdSource::new(), + runtime_wake, + ), } } @@ -51,58 +57,49 @@ impl SessionFileDialogController { mode: SessionFileDialogMode, current_path: Option, ) -> Result<()> { - if self.active.is_some() { - return Err(anyhow!("a session file dialog is already active")); - } - let id = self.next_id; - self.next_id = self - .next_id - .checked_add(1) - .ok_or_else(|| anyhow!("session dialog identity exhausted"))?; - let (sender, receiver) = mpsc::sync_channel(1); - let wake = self.runtime_wake.clone(); - std::thread::Builder::new() - .name(format!("wayscriber-session-dialog-{id}")) - .spawn(move || { - let result = choose_session_file(mode, current_path.as_deref()) - .map_err(|error| format!("{error:#}")); - let _ = sender.send((id, mode, result)); - if let Err(error) = wake.wake() { - log::error!("Failed to wake runtime for session dialog completion: {error}"); - } - }) - .context("failed to start session dialog worker")?; - self.active = Some((id, mode)); - self.receiver = Some(receiver); - Ok(()) + self.submit(mode, move || { + choose_session_file(mode, current_path.as_deref()).map_err(|error| format!("{error:#}")) + }) + } + + fn submit( + &mut self, + mode: SessionFileDialogMode, + chooser: impl FnOnce() -> Result, String> + Send + 'static, + ) -> Result<()> { + self.operation + .try_submit(mode, "wayscriber-session-dialog", chooser) + .map(|_| ()) + .map_err(|failure| anyhow!(failure.into_parts().0)) } pub(in crate::backend::wayland::state) fn try_receive( &mut self, ) -> Result> { - let Some((expected_id, expected_mode)) = self.active else { - return Ok(None); - }; - let receiver = self - .receiver - .as_ref() - .ok_or_else(|| anyhow!("active session dialog has no completion receiver"))?; - let received = match receiver.try_recv() { - Ok(received) => received, - Err(mpsc::TryRecvError::Empty) => return Ok(None), - Err(mpsc::TryRecvError::Disconnected) => ( - expected_id, - expected_mode, - Err("session dialog worker exited without a completion".into()), - ), + let completion = match self.operation.poll() { + RuntimeOperationPoll::Idle | RuntimeOperationPoll::Pending { .. } => return Ok(None), + RuntimeOperationPoll::Ready { + context: mode, + outcome: result, + .. + } => SessionFileDialogCompletion { mode, result }, + RuntimeOperationPoll::ProducerFailed { + context: mode, + reason, + .. + } => SessionFileDialogCompletion { + mode, + result: Err(reason), + }, + RuntimeOperationPoll::Disconnected { context: mode, .. } => { + SessionFileDialogCompletion { + mode, + result: Err("session dialog worker exited without a completion".into()), + } + } }; - self.active = None; - self.receiver = None; - let (id, mode, result) = received; - if id != expected_id || mode != expected_mode { - return Err(anyhow!("session dialog completion identity mismatch")); - } - Ok(Some(SessionFileDialogCompletion { mode, result })) + + Ok(Some(completion)) } } @@ -237,7 +234,12 @@ fn run_session_file_dialog_command( ) }) { Ok(output) => output, - Err(err) if err.to_string().contains("No such file") => return Ok(None), + Err(err) + if crate::process_broker::error_kind(&err) + == Some(crate::process_broker::BrokerErrorKind::MissingExecutable) => + { + return Ok(None); + } Err(err) => return Err(anyhow!("failed to launch {program}: {err:#}")), }; @@ -319,59 +321,75 @@ pub(in crate::backend::wayland::state::toolbar::events) fn ensure_save_as_extens #[cfg(test)] mod controller_tests { use super::*; - - fn controller() -> SessionFileDialogController { - let wake = crate::backend::wayland::RuntimeWakeSource::new().unwrap(); - SessionFileDialogController::new(wake.handle()) - } + use crate::backend::wayland::RuntimeWakeSource; #[test] - fn completion_identity_mismatch_is_terminal_and_consumed_once() { - let mut controller = controller(); - let (sender, receiver) = mpsc::sync_channel(1); - controller.active = Some((7, SessionFileDialogMode::Open)); - controller.receiver = Some(receiver); - sender - .send(( - 8, - SessionFileDialogMode::Open, - Ok(Some(PathBuf::from("/tmp/session"))), - )) - .unwrap(); - - assert!(controller.try_receive().is_err()); - assert!(controller.try_receive().unwrap().is_none()); + fn every_dialog_terminal_outcome_wakes_and_is_consumed_once() { + let outcomes = [ + Ok(Some(PathBuf::from("/tmp/session"))), + Ok(None), + Err("chooser error".into()), + ]; + for expected in outcomes { + let wake = RuntimeWakeSource::new().unwrap(); + let mut controller = SessionFileDialogController::new(wake.handle()); + let result = expected.clone(); + controller + .submit(SessionFileDialogMode::SaveAs, move || result) + .unwrap(); + + assert!(wake.wait_readable(Some(Duration::from_secs(1))).unwrap()); + let completion = controller.try_receive().unwrap().unwrap(); + assert_eq!(completion.mode, SessionFileDialogMode::SaveAs); + assert_eq!(completion.result, expected); + assert!(controller.try_receive().unwrap().is_none()); + assert!(!wake.drain().unwrap()); + } } #[test] - fn worker_disconnect_produces_one_identified_failure() { - let mut controller = controller(); - let (sender, receiver) = mpsc::sync_channel(1); - controller.active = Some((9, SessionFileDialogMode::SaveAs)); - controller.receiver = Some(receiver); - drop(sender); + fn panicking_dialog_wakes_idle_runtime_with_one_terminal_failure() { + let wake = RuntimeWakeSource::new().unwrap(); + let mut controller = SessionFileDialogController::new(wake.handle()); + controller + .submit(SessionFileDialogMode::Open, || panic!("chooser panicked")) + .unwrap(); + assert!(wake.wait_readable(Some(Duration::from_secs(1))).unwrap()); let completion = controller.try_receive().unwrap().unwrap(); - assert_eq!(completion.mode, SessionFileDialogMode::SaveAs); - assert!( - completion - .result - .unwrap_err() - .contains("without a completion") - ); + assert_eq!(completion.mode, SessionFileDialogMode::Open); + assert!(completion.result.unwrap_err().contains("chooser panicked")); assert!(controller.try_receive().unwrap().is_none()); + assert!(!wake.drain().unwrap()); } #[test] fn active_dialog_rejects_overlap_before_spawning_worker() { - let mut controller = controller(); - controller.active = Some((1, SessionFileDialogMode::Open)); + let wake = RuntimeWakeSource::new().unwrap(); + let mut controller = SessionFileDialogController::new(wake.handle()); + let (release, wait) = std::sync::mpsc::channel(); + controller + .submit(SessionFileDialogMode::Open, move || { + wait.recv().unwrap(); + Ok(None) + }) + .unwrap(); + assert!( controller .start(SessionFileDialogMode::SaveAs, None) - .unwrap_err() - .to_string() - .contains("already active") + .is_err() + ); + release.send(()).unwrap(); + assert!(wake.wait_readable(Some(Duration::from_secs(1))).unwrap()); + assert!( + controller + .try_receive() + .unwrap() + .unwrap() + .result + .unwrap() + .is_none() ); } } diff --git a/src/canvas_export/pdf/tests.rs b/src/canvas_export/pdf/tests.rs index 610393ab2..16ccd1740 100644 --- a/src/canvas_export/pdf/tests.rs +++ b/src/canvas_export/pdf/tests.rs @@ -124,10 +124,7 @@ fn auto_orientation_matches_source_for_standard_pages() { } #[test] -fn rendered_pdf_reports_page_count_and_sizes_when_pdfinfo_is_available() { - if Command::new("pdfinfo").arg("-v").output().is_err() { - return; - } +fn rendered_pdf_reports_exact_page_count_and_ordered_sizes() { let source = CanvasExportRect::new(0.0, 0.0, 100.0, 100.0).expect("source"); let pages = vec![ pdf_page(300.0, 200.0, source, 0, 2), @@ -143,6 +140,7 @@ fn rendered_pdf_reports_page_count_and_sizes_when_pdfinfo_is_available() { std::fs::write(&path, bytes).expect("write pdf"); let output = Command::new("pdfinfo") + .env("LC_ALL", "C") .arg("-f") .arg("1") .arg("-l") @@ -150,14 +148,10 @@ fn rendered_pdf_reports_page_count_and_sizes_when_pdfinfo_is_available() { .arg(&path) .output() .expect("pdfinfo"); - if !output.status.success() { - return; - } - let text = String::from_utf8_lossy(&output.stdout); - - assert!(text.contains("Pages:")); - assert!(text.contains('2')); - assert!(text.contains("300 x 200") || text.contains("200 x 300")); + let text = checked_pdf_output(output); + assert_eq!(pdf_field(&text, "Pages"), "2"); + assert_eq!(pdf_field(&text, "Page 1 size"), "300 x 200 pts"); + assert_eq!(pdf_field(&text, "Page 2 size"), "200 x 300 pts"); } fn text_pdf(text_halo_enabled: bool) -> Vec { @@ -184,13 +178,52 @@ fn text_pdf(text_halo_enabled: bool) -> Vec { #[test] fn pdf_export_honours_the_text_halo_setting() { - let disabled = text_pdf(false); - let enabled = text_pdf(true); - assert_ne!( - disabled.len(), - enabled.len(), - "vector PDF output must contain the configured text rendering", - ); + let temp = crate::test_temp::tempdir().expect("tempdir"); + for enabled in [false, true] { + let path = temp.path().join(format!("halo-{enabled}.pdf")); + let prefix = temp.path().join(format!("halo-{enabled}")); + std::fs::write(&path, text_pdf(enabled)).expect("write text PDF"); + checked_pdf_output( + Command::new("pdftoppm") + .env("LC_ALL", "C") + .args(["-png", "-singlefile", "-r", "72"]) + .arg(&path) + .arg(&prefix) + .output() + .expect("pdftoppm is required for PDF artifact tests (install poppler)"), + ); + let bytes = std::fs::read(prefix.with_extension("png")).expect("rasterized PDF"); + let image = crate::image_decode::decode_rgba( + crate::image_decode::EncodedImageFormat::Png, + &bytes, + crate::screen_pixels::EmbeddedImageLimits::default().into(), + ) + .expect("decode rasterized PDF"); + assert_eq!((image.width, image.height), (400, 120)); + let red = image + .rgba + .as_chunks::<4>() + .0 + .iter() + .filter(|p| p[0] > 180 && p[1] < 80 && p[2] < 80) + .count(); + let dark = image + .rgba + .as_chunks::<4>() + .0 + .iter() + .filter(|p| p[0] < 80 && p[1] < 80 && p[2] < 80) + .count(); + assert!(red > 100, "red glyphs must remain visible: {red}"); + if enabled { + assert!( + dark > 100, + "enabled halo must outline the red glyphs: {dark}" + ); + } else { + assert_eq!(dark, 0, "disabled halo must not add a dark outline"); + } + } } fn pdf_page( @@ -267,20 +300,34 @@ fn worker_exports_three_page_pdf_from_unicode_metadata() { let temp = crate::test_temp::tempdir().expect("tempdir"); let path = temp.path().join("worker-pages.pdf"); std::fs::write(&path, bytes).expect("write worker PDF"); - let output = match Command::new("pdfinfo").arg(&path).output() { - Ok(output) => output, - Err(error) if error.kind() == std::io::ErrorKind::NotFound => return, - Err(error) => panic!("failed to run pdfinfo: {error}"), - }; + let info = checked_pdf_output( + Command::new("pdfinfo") + .env("LC_ALL", "C") + .arg(&path) + .output() + .expect("pdfinfo is required for PDF artifact tests (install poppler)"), + ); + assert_eq!( + pdf_field(&info, "Pages"), + "3", + "worker PDF must contain all three pages" + ); +} + +fn checked_pdf_output(output: std::process::Output) -> String { assert!( output.status.success(), - "pdfinfo failed: {}", + "PDF reader failed: {}", String::from_utf8_lossy(&output.stderr) ); - let info = String::from_utf8_lossy(&output.stdout); - let pages = info - .lines() - .find_map(|line| line.strip_prefix("Pages:")) - .expect("pdfinfo page count"); - assert_eq!(pages.trim(), "3", "worker PDF must contain all three pages"); + String::from_utf8(output.stdout).expect("PDF reader output is UTF-8") +} + +fn pdf_field<'a>(info: &'a str, name: &str) -> &'a str { + info.lines() + .find_map(|line| { + let (key, value) = line.split_once(':')?; + (key == name).then(|| value.trim()) + }) + .unwrap_or_else(|| panic!("missing PDF field {name:?}: {info}")) } diff --git a/src/capture/portal.rs b/src/capture/portal.rs index 55bcd9197..302391934 100644 --- a/src/capture/portal.rs +++ b/src/capture/portal.rs @@ -660,3 +660,7 @@ mod tests { assert!(matches!(error, CaptureError::DBusError(_)), "{error}"); } } + +#[cfg(test)] +#[path = "portal/tests/transport.rs"] +mod transport_tests; diff --git a/src/capture/portal/tests/transport.rs b/src/capture/portal/tests/transport.rs new file mode 100644 index 000000000..719aabb0f --- /dev/null +++ b/src/capture/portal/tests/transport.rs @@ -0,0 +1,295 @@ +//! Real subscription transactions on an owned private session bus. + +use super::*; +use std::io::{BufRead, BufReader}; +use std::process::{Child, Command, Stdio}; +use std::sync::{ + Arc, + atomic::{AtomicUsize, Ordering}, +}; +use zbus::message::Header; + +struct PrivateBus { + child: Child, + address: String, +} + +impl PrivateBus { + fn start() -> Self { + let mut child = Command::new("dbus-daemon") + .args(["--session", "--nofork", "--print-address=1"]) + .stdout(Stdio::piped()) + .stderr(Stdio::null()) + .spawn() + .expect("portal transport tests require dbus-daemon"); + let mut address = String::new(); + BufReader::new(child.stdout.take().unwrap()) + .read_line(&mut address) + .unwrap(); + assert!( + !address.trim().is_empty(), + "private bus must publish its address" + ); + Self { + child, + address: address.trim().to_owned(), + } + } +} + +impl Drop for PrivateBus { + fn drop(&mut self) { + let _ = self.child.kill(); + let _ = self.child.wait(); + } +} + +#[derive(Clone, Copy)] +enum ResponseMode { + Immediate, + Deferred, + Cancelled, + Compatibility, + Silent, +} + +struct ScreenshotFixture { + mode: ResponseMode, + requested: tokio::sync::mpsc::UnboundedSender, + closed: Arc, +} + +struct RequestFixture { + closed: Arc, +} + +#[zbus::interface(name = "org.freedesktop.portal.Request")] +impl RequestFixture { + fn close(&self) { + self.closed.fetch_add(1, Ordering::SeqCst); + } +} + +#[zbus::interface(name = "org.freedesktop.portal.Screenshot")] +impl ScreenshotFixture { + async fn screenshot( + &self, + _parent_window: &str, + mut options: HashMap, + #[zbus(header)] header: Header<'_>, + #[zbus(connection)] connection: &Connection, + ) -> zbus::fdo::Result { + let token = + String::try_from(options.remove(PORTAL_OPTION_HANDLE_TOKEN_KEY).unwrap()).unwrap(); + let sender = header + .sender() + .unwrap() + .as_str() + .trim_start_matches(':') + .replace('.', "_"); + let path = if matches!(self.mode, ResponseMode::Compatibility) { + format!("{PORTAL_REQUEST_PATH_PREFIX}/{sender}/compatibility") + } else { + format!("{PORTAL_REQUEST_PATH_PREFIX}/{sender}/{token}") + }; + let path = OwnedObjectPath::try_from(path).unwrap(); + connection + .object_server() + .at( + path.clone(), + RequestFixture { + closed: self.closed.clone(), + }, + ) + .await + .map_err(|error| zbus::fdo::Error::Failed(error.to_string()))?; + if matches!(self.mode, ResponseMode::Immediate) { + emit_response(connection, &path, 0).await; + } + self.requested.send(path.clone()).unwrap(); + Ok(path) + } +} + +async fn emit_response(connection: &Connection, path: &OwnedObjectPath, code: u32) { + let results: HashMap = if code == 0 { + HashMap::from([( + PORTAL_RESULT_URI_KEY.into(), + zbus::zvariant::Str::from("file:///tmp/private-portal.png").into(), + )]) + } else { + HashMap::new() + }; + connection + .emit_signal( + None::<&str>, + path.clone(), + "org.freedesktop.portal.Request", + "Response", + &(code, results), + ) + .await + .unwrap(); +} + +async fn fixture( + bus: &PrivateBus, + mode: ResponseMode, +) -> ( + Connection, + Connection, + tokio::sync::mpsc::UnboundedReceiver, + Arc, +) { + let (requested, receiver) = tokio::sync::mpsc::unbounded_channel(); + let closed = Arc::new(AtomicUsize::new(0)); + let service = zbus::connection::Builder::address(bus.address.as_str()) + .unwrap() + .name(PORTAL_DESTINATION) + .unwrap() + .serve_at( + "/org/freedesktop/portal/desktop", + ScreenshotFixture { + mode, + requested, + closed: closed.clone(), + }, + ) + .unwrap() + .build() + .await + .unwrap(); + let client = zbus::connection::Builder::address(bus.address.as_str()) + .unwrap() + .build() + .await + .unwrap(); + (service, client, receiver, closed) +} + +async fn capture(client: Connection) -> Result { + let proxy = ScreenshotProxy::new(&client).await.unwrap(); + capture_once( + &client, + &proxy, + build_portal_options(CaptureType::FullScreen), + ) + .await +} + +/// Inspect the private bus's actual match rules before a deferred signal. +/// This avoids timing guesses, especially after a compatibility path reply. +async fn wait_for_response_subscription( + service: &Connection, + client: &Connection, + path: &OwnedObjectPath, +) { + let deadline = tokio::time::Instant::now() + Duration::from_secs(2); + loop { + let reply = service + .call_method( + Some("org.freedesktop.DBus"), + "/org/freedesktop/DBus", + Some("org.freedesktop.DBus.Debug.Stats"), + "GetAllMatchRules", + &(), + ) + .await + .unwrap(); + let rules: HashMap> = reply.body().deserialize().unwrap(); + let matched = rules + .get(client.unique_name().unwrap().as_str()) + .is_some_and(|rules| { + rules + .iter() + .any(|rule| rule.contains(path.as_str()) && rule.contains("Response")) + }); + if matched { + return; + } + assert!( + tokio::time::Instant::now() < deadline, + "client did not subscribe at {path}" + ); + tokio::task::yield_now().await; + } +} + +#[tokio::test] +async fn portal_transport_immediate_response_before_method_reply_is_not_lost() { + let bus = PrivateBus::start(); + let (_service, client, _requests, closed) = fixture(&bus, ResponseMode::Immediate).await; + let result = tokio::time::timeout(Duration::from_secs(2), capture(client)) + .await + .unwrap() + .unwrap(); + assert_eq!(result, "file:///tmp/private-portal.png"); + assert_eq!(closed.load(Ordering::SeqCst), 0); +} + +async fn deferred_case(mode: ResponseMode, code: u32) -> Result { + let bus = PrivateBus::start(); + let (service, client, mut requests, closed) = fixture(&bus, mode).await; + let operation = tokio::spawn(capture(client.clone())); + let path = requests.recv().await.unwrap(); + wait_for_response_subscription(&service, &client, &path).await; + emit_response(&service, &path, code).await; + let result = tokio::time::timeout(Duration::from_secs(2), operation) + .await + .unwrap() + .unwrap(); + assert_eq!(closed.load(Ordering::SeqCst), 0); + result +} + +#[tokio::test] +async fn portal_transport_delayed_response_reaches_the_original_subscription() { + assert_eq!( + deferred_case(ResponseMode::Deferred, 0).await.unwrap(), + "file:///tmp/private-portal.png" + ); +} + +#[tokio::test] +async fn portal_transport_compatibility_path_installs_a_new_subscription() { + assert_eq!( + deferred_case(ResponseMode::Compatibility, 0).await.unwrap(), + "file:///tmp/private-portal.png" + ); +} + +#[tokio::test] +async fn portal_transport_cancellation_remains_distinct_from_failure() { + assert!(matches!( + deferred_case(ResponseMode::Cancelled, 1).await, + Err(CaptureError::Cancelled(_)) + )); +} + +#[tokio::test] +async fn portal_transport_silent_request_times_out_and_closes_once() { + let bus = PrivateBus::start(); + let (_service, client, _requests, closed) = fixture(&bus, ResponseMode::Silent).await; + assert!( + matches!(capture(client).await, Err(CaptureError::PortalTimeout(limit)) if limit == PORTAL_RESPONSE_TIMEOUT) + ); + assert_eq!(closed.load(Ordering::SeqCst), 1); +} + +#[tokio::test] +async fn portal_transport_bus_disconnect_is_a_terminal_failure() { + let mut bus = PrivateBus::start(); + let (_service, client, mut requests, _closed) = fixture(&bus, ResponseMode::Silent).await; + let operation = tokio::spawn(capture(client)); + requests.recv().await.unwrap(); + bus.child.kill().unwrap(); + bus.child.wait().unwrap(); + let outcome = tokio::time::timeout(Duration::from_secs(2), operation) + .await + .unwrap() + .unwrap(); + assert!(matches!( + outcome, + Err(CaptureError::DBusError(_) | CaptureError::InvalidResponse(_)) + )); +} diff --git a/src/config/action_meta/entries/tools.rs b/src/config/action_meta/entries/tools.rs index 558500e0e..2b895fb07 100644 --- a/src/config/action_meta/entries/tools.rs +++ b/src/config/action_meta/entries/tools.rs @@ -10,7 +10,7 @@ pub const ENTRIES: &[ActionMeta] = &[ true, true, true, - icon: crate::toolbar_icons::draw_icon_text + icon: crate::config::action_meta::ActionIcon::Text ), meta!( EnterStickyNoteMode, @@ -21,7 +21,7 @@ pub const ENTRIES: &[ActionMeta] = &[ true, true, true, - icon: crate::toolbar_icons::draw_icon_note + icon: crate::config::action_meta::ActionIcon::StickyNote ), meta!( SelectSelectionTool, @@ -32,7 +32,7 @@ pub const ENTRIES: &[ActionMeta] = &[ true, true, true, - icon: crate::toolbar_icons::draw_icon_select + icon: crate::config::action_meta::ActionIcon::Select ), meta!( SelectPenTool, @@ -43,7 +43,7 @@ pub const ENTRIES: &[ActionMeta] = &[ true, true, true, - icon: crate::toolbar_icons::draw_icon_pen + icon: crate::config::action_meta::ActionIcon::Pen ), meta!( SelectLiveShapeTool, @@ -64,7 +64,7 @@ pub const ENTRIES: &[ActionMeta] = &[ true, true, true, - icon: crate::toolbar_icons::draw_icon_line + icon: crate::config::action_meta::ActionIcon::Line ), meta!( SelectRectTool, @@ -75,7 +75,7 @@ pub const ENTRIES: &[ActionMeta] = &[ true, true, true, - icon: crate::toolbar_icons::draw_icon_rect + icon: crate::config::action_meta::ActionIcon::Rect ), meta!( SelectEllipseTool, @@ -86,7 +86,7 @@ pub const ENTRIES: &[ActionMeta] = &[ true, true, true, - icon: crate::toolbar_icons::draw_icon_circle + icon: crate::config::action_meta::ActionIcon::Ellipse ), meta!( SelectTriangleTool, @@ -127,7 +127,7 @@ pub const ENTRIES: &[ActionMeta] = &[ true, true, true, - icon: crate::toolbar_icons::draw_icon_polygon + icon: crate::config::action_meta::ActionIcon::FreeformPolygon ), meta!( SelectFreeformPolygonTool, @@ -148,7 +148,7 @@ pub const ENTRIES: &[ActionMeta] = &[ true, true, true, - icon: crate::toolbar_icons::draw_icon_arrow + icon: crate::config::action_meta::ActionIcon::Arrow ), meta!( SelectBlurTool, @@ -159,7 +159,7 @@ pub const ENTRIES: &[ActionMeta] = &[ true, true, true, - icon: crate::toolbar_icons::draw_icon_blur + icon: crate::config::action_meta::ActionIcon::Blur ), meta!( SelectHighlightTool, @@ -208,7 +208,7 @@ pub const ENTRIES: &[ActionMeta] = &[ true, true, true, - icon: crate::toolbar_icons::draw_icon_marker + icon: crate::config::action_meta::ActionIcon::Marker ), meta!( SelectStepMarkerTool, @@ -219,7 +219,7 @@ pub const ENTRIES: &[ActionMeta] = &[ true, true, true, - icon: crate::toolbar_icons::draw_icon_step_marker + icon: crate::config::action_meta::ActionIcon::StepMarker ), meta!( SelectEraserTool, @@ -230,7 +230,7 @@ pub const ENTRIES: &[ActionMeta] = &[ true, true, true, - icon: crate::toolbar_icons::draw_icon_eraser + icon: crate::config::action_meta::ActionIcon::Eraser ), meta!( ToggleEraserMode, diff --git a/src/config/action_meta/mod.rs b/src/config/action_meta/mod.rs index 35508bba7..ecc291f04 100644 --- a/src/config/action_meta/mod.rs +++ b/src/config/action_meta/mod.rs @@ -15,6 +15,25 @@ pub enum ActionCategory { Presets, } +/// Semantic glyph identity; rendering owners choose the Cairo painter. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ActionIcon { + Text, + StickyNote, + Select, + Pen, + Line, + Rect, + Ellipse, + FreeformPolygon, + Arrow, + Blur, + Marker, + StepMarker, + Eraser, + Undo, +} + #[derive(Debug, Clone, Copy)] pub struct ActionMeta { pub action: Action, @@ -27,11 +46,8 @@ pub struct ActionMeta { pub in_command_palette: bool, pub in_help: bool, pub in_toolbar: bool, - /// Shared Cairo glyph painter for surfaces that render this action as an - /// icon (the radial compass). Reuses the toolbar's semantic painters so - /// the same action can never draw two different glyphs. `None` for - /// actions no icon surface needs. - pub icon: Option, + /// Semantic icon for the radial menu and command palette. + pub icon: Option, } impl ActionMeta { diff --git a/src/config/action_meta/tests.rs b/src/config/action_meta/tests.rs index 3898e13da..ae7f00be6 100644 --- a/src/config/action_meta/tests.rs +++ b/src/config/action_meta/tests.rs @@ -422,33 +422,6 @@ fn icon_coverage_matches_the_radial_contract() { } } -#[test] -fn icons_reuse_the_shared_semantic_tool_painters() { - use crate::input::Tool; - use crate::toolbar_icons::top_toolbar_icon_painter; - use crate::ui::toolbar::model::{TopToolbarIcon, semantic_icon_for_tool}; - - for action in EXPECTED_ICON_ACTIONS { - let icon = action_meta(*action) - .and_then(|meta| meta.icon) - .unwrap_or_else(|| panic!("missing icon for {:?}", action)); - let expected = match action { - Action::EnterTextMode => top_toolbar_icon_painter(TopToolbarIcon::Text), - Action::EnterStickyNoteMode => top_toolbar_icon_painter(TopToolbarIcon::StickyNote), - _ => { - let tool = Tool::from_select_action(*action) - .unwrap_or_else(|| panic!("{:?} should select a tool", action)); - top_toolbar_icon_painter(TopToolbarIcon::Tool(semantic_icon_for_tool(tool))) - } - }; - assert!( - std::ptr::fn_addr_eq(icon, expected), - "icon painter for {:?} drifted from the shared semantic painter", - action - ); - } -} - #[test] fn meta_macro_arms_default_aliases_and_icon() { const PLAIN: ActionMeta = meta!(Undo, "Undo", None, "d", History, false, false, false); @@ -472,7 +445,7 @@ fn meta_macro_arms_default_aliases_and_icon() { false, false, false, - icon: crate::toolbar_icons::draw_icon_undo + icon: crate::config::action_meta::ActionIcon::Undo ); const FULL: ActionMeta = meta!( Undo, @@ -484,7 +457,7 @@ fn meta_macro_arms_default_aliases_and_icon() { false, false, &["back"], - icon: crate::toolbar_icons::draw_icon_undo + icon: crate::config::action_meta::ActionIcon::Undo ); assert!(PLAIN.icon.is_none() && PLAIN.search_aliases.is_empty()); diff --git a/src/config/document.rs b/src/config/document.rs index 35f96cdcb..e807a5a7c 100644 --- a/src/config/document.rs +++ b/src/config/document.rs @@ -29,6 +29,27 @@ use merge::{ serialize_config_document, unreadable_repair_source_document, }; +/// The source revision or destination changed after a config document was loaded. +/// Narrow edits may reload and reapply once; other errors must not trigger retry. +#[derive(Debug)] +pub struct ConfigSourceChanged { + diagnostic: String, +} + +impl ConfigSourceChanged { + pub(super) fn new(diagnostic: String) -> Self { + Self { diagnostic } + } +} + +impl fmt::Display for ConfigSourceChanged { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str(&self.diagnostic) + } +} + +impl std::error::Error for ConfigSourceChanged {} + #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum ConfigDiagnosticKind { UnknownSetting, @@ -848,9 +869,7 @@ impl ConfigDocument { /// somewhere else, which is not a smaller version of either: a retargeted /// link means the bytes being compared are a different file's, and the /// document's edit was merged into text that path no longer holds. Each is - /// reported on its own terms, with the same "changed on disk" wording the - /// editors' reload-and-reapply retry recognises, because the recovery is the - /// same — load what the path names now and reapply the edit onto it. + /// reported as `ConfigSourceChanged`, because the recovery is the same — load what the path names now and reapply the edit onto it. /// /// The destination is re-derived here rather than re-read from the loaded /// revision, and it is derived the same way the load derived it: whole path, @@ -862,27 +881,27 @@ impl ConfigDocument { fn ensure_source_unchanged(&self) -> Result<()> { let current = SourceRevision::read(&self.source_path)?; if current.destination() != self.revision.destination() { - bail!( + bail!(ConfigSourceChanged::new(format!( "Configuration changed on disk at {}: it now resolves to {} rather than {}. \ Reload before saving.", self.source_path.display(), current.destination().display(), self.revision.destination().display(), - ); + ))); } if current.followed_links() != self.revision.followed_links() { - bail!( + bail!(ConfigSourceChanged::new(format!( "Configuration changed on disk at {}: it reaches {} through different links \ than it did. Reload before saving.", self.source_path.display(), self.revision.destination().display(), - ); + ))); } if current != self.revision { - bail!( + bail!(ConfigSourceChanged::new(format!( "Configuration changed on disk at {}. Reload before saving.", self.source_path.display() - ); + ))); } Ok(()) } diff --git a/src/config/document/lock.rs b/src/config/document/lock.rs index 04f2acc24..92c5d69c4 100644 --- a/src/config/document/lock.rs +++ b/src/config/document/lock.rs @@ -94,7 +94,7 @@ impl Drop for ConfigWriteLock { // Closing the descriptor would release it anyway; unlocking first // says so at the point where the window ends. A failure here leaves // the close to do it, so there is nothing for the caller to act on. - let _ = crate::session::unlock(file); + let _ = crate::durable_io::unlock(file); } } } @@ -115,7 +115,7 @@ pub(super) fn acquire_at(path: &Path, timeout: Duration) -> Result return Ok(ConfigWriteLock { file: Some(file) }), Err(error) if error.kind() == ErrorKind::WouldBlock => { let waited = started.elapsed(); diff --git a/src/config/document/merge.rs b/src/config/document/merge.rs index 8f840fbe9..f9cd21016 100644 --- a/src/config/document/merge.rs +++ b/src/config/document/merge.rs @@ -4,7 +4,7 @@ use anyhow::{Context, Result}; use toml_edit::{Array, ArrayOfTables, DocumentMut, Item, Key, Table, TableLike, Value}; use super::super::Config; -use crate::input::boards::BoundaryBoardIdSet; +use crate::domain::BoundaryBoardIdSet; use crate::render_profiles::normalize_profile_id; pub(super) fn merge_config_document( diff --git a/src/config/enums.rs b/src/config/enums.rs index 4f2919ad0..765d00e90 100644 --- a/src/config/enums.rs +++ b/src/config/enums.rs @@ -74,17 +74,6 @@ pub enum UiTheme { Light, } -impl UiTheme { - /// Maps the config value onto the runtime theme mode. - pub fn to_theme_mode(self) -> crate::ui::theme::ThemeMode { - match self { - UiTheme::Auto => crate::ui::theme::ThemeMode::Auto, - UiTheme::Dark => crate::ui::theme::ThemeMode::Dark, - UiTheme::Light => crate::ui::theme::ThemeMode::Light, - } - } -} - /// Reduced-motion preference (`[ui] reduced_motion`). /// /// `on` disables UI animations. `auto` is reserved for a future desktop-portal diff --git a/src/config/io.rs b/src/config/io.rs index 4665a2c31..6dc75014b 100644 --- a/src/config/io.rs +++ b/src/config/io.rs @@ -328,21 +328,9 @@ impl std::fmt::Display for QuickColorSlotMissing { impl std::error::Error for QuickColorSlotMissing {} -/// The substring a save uses to report that the file moved under a loaded -/// document. -/// -/// Two places produce it: `ensure_source_unchanged`, where the document -/// compares what it loaded against what the path holds now, and -/// [`write_config_text_atomic`], where the rename finds a different file than -/// the one those comparisons were about. Both are already pinned by the -/// document suite; `a_stale_document_is_recognised_and_retried` below re-checks -/// the coupling against a real stale save rather than trusting it. -const STALE_SOURCE_MARKER: &str = "changed on disk"; - +/// Whether retry can recover by reloading the changed source revision. pub(crate) fn is_stale_source_error(error: &anyhow::Error) -> bool { - error - .chain() - .any(|cause| cause.to_string().contains(STALE_SOURCE_MARKER)) + error.downcast_ref::().is_some() } /// The shared shape of every narrow config editor. @@ -765,9 +753,8 @@ pub(super) fn create_config_backup(path: &Path) -> Result { /// the complete revision still being there; the identity coming back names the /// file this write created, which is what the caller's next comparison expects. /// -/// A destination that moved is reported in the wording -/// [`is_stale_source_error`] recognises, because the recovery is the editors' -/// ordinary one: load what the path holds now and reapply the edit onto it. +/// A destination that moved is reported as `ConfigSourceChanged`, because +/// the recovery is the editors' ordinary one: load what the path holds now and reapply the edit onto it. pub(super) fn write_config_text_atomic( destination: &Path, contents: &str, @@ -784,10 +771,13 @@ pub(super) fn write_config_text_atomic( Some(expected), ) { Ok(identity) => Ok(identity), - Err(error) if matches!(error, DurableIoError::DestinationChanged { .. }) => Err(anyhow!( - "Configuration changed on disk at {}: {error}. Reload before saving.", - destination.display() - )), + Err(error) if matches!(error, DurableIoError::DestinationChanged { .. }) => { + let diagnostic = format!( + "Configuration changed on disk at {}: {error}. Reload before saving.", + destination.display() + ); + Err(error).context(super::ConfigSourceChanged::new(diagnostic)) + } Err(error) => Err(error) .with_context(|| format!("Failed to write config to {}", destination.display())), } @@ -797,6 +787,78 @@ pub(super) fn write_config_text_atomic( mod tests { use super::*; + #[test] + fn unrelated_failure_with_stale_wording_does_not_reapply_the_edit() { + let temp = crate::test_temp::tempdir().unwrap(); + let path = temp.path().join("config.toml"); + fs::write(&path, "[ui]\nshow_status_bar = true\n").unwrap(); + let invocations = std::cell::Cell::new(0); + + let error = edit_one_config_key_with_retry( + &path, + "status", + &|_| { + invocations.set(invocations.get() + 1); + Err(std::io::Error::other("unrelated file changed on disk")).context("edit failed") + }, + &|_| false, + ) + .unwrap_err(); + + assert_eq!(invocations.get(), 1); + assert!(error.downcast_ref::().is_some()); + assert_eq!( + fs::read_to_string(path).unwrap(), + "[ui]\nshow_status_bar = true\n" + ); + } + + #[test] + fn changed_source_reapplies_once_and_preserves_the_other_edit() { + for collide_twice in [false, true] { + let temp = crate::test_temp::tempdir().unwrap(); + let path = temp.path().join("config.toml"); + fs::write(&path, "[ui]\nshow_status_bar = true\n").unwrap(); + let invocations = std::cell::Cell::new(0); + + let result = edit_one_config_key_with_retry( + &path, + "status", + &|config| { + let invocation = invocations.get() + 1; + invocations.set(invocation); + config.ui.show_status_bar = false; + if invocation == 1 || collide_twice { + fs::write( + &path, + format!( + "# concurrent edit {invocation}\n[ui]\nshow_status_bar = true\ntheme = 'light'\n" + ), + )?; + } + Ok(()) + }, + &|config| !config.ui.show_status_bar, + ); + + assert_eq!(invocations.get(), 2, "retry must be bounded to one reapply"); + let document = super::super::ConfigDocument::load_from_path(&path).unwrap(); + assert_eq!(document.config().ui.theme, super::super::UiTheme::Light); + assert!( + fs::read_to_string(&path) + .unwrap() + .contains("# concurrent edit") + ); + if collide_twice { + assert!(is_stale_source_error(&result.unwrap_err())); + assert!(document.config().ui.show_status_bar); + } else { + assert!(result.is_ok()); + assert!(!document.config().ui.show_status_bar); + } + } + } + /// The load-bearing half of the narrow editors' one-key property. /// /// `edit_one_config_key` builds `updated` from `document.config()` and lets diff --git a/src/config/mod.rs b/src/config/mod.rs index c1480fa35..dc29a0b6d 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -36,7 +36,7 @@ pub use action_meta::{ pub use core::{CURRENT_CONFIG_REVISION, Config}; pub use document::{ ConfigDiagnostic, ConfigDiagnosticKind, ConfigDocument, ConfigDocumentSaveOutcome, - ConfigWriteLockTimeout, + ConfigSourceChanged, ConfigWriteLockTimeout, }; pub use enums::{ RadialMenuMouseBinding, ReducedMotion, RegionPicker, StatusPosition, UiTheme, @@ -63,12 +63,14 @@ pub use migration::{MigrationChange, MigrationPreview}; pub use types::{ ARROW_ANGLE_MAX, ARROW_ANGLE_MIN, ARROW_LENGTH_MAX, ARROW_LENGTH_MIN, ArrowConfig, BoardBackgroundConfig, BoardColorConfig, BoardConfig, BoardGridConfig, BoardGridKindConfig, - BoardItemConfig, BoardsConfig, CaptureConfig, ClickHighlightConfig, DEFAULT_OCR_LANGUAGES, - DEFAULT_PEN_SMOOTHING, DEFAULT_SHAPE_RECOGNITION_SENSITIVITY, DragButtonConfig, DrawingConfig, - ExportConfig, HelpOverlayStyle, HistoryConfig, InputHudConfig, InputHudMode, InputHudPosition, - LASER_FADE_MS_MAX, LASER_HOLD_MS_MAX, LASER_WIDTH_MAX, LASER_WIDTH_MIN, LaserConfig, - MAX_SHAPE_RECOGNITION_SENSITIVITY, MouseDragToolsConfig, PDF_LABEL_APP_BOARD, - PDF_LABEL_APP_BOARDS, PDF_LABEL_BOARD_NAME, PDF_LABEL_DEFAULT_TEMPLATE, + BoardItemConfig, BoardsConfig, CLICK_HIGHLIGHT_DURATION_MAX_MS, + CLICK_HIGHLIGHT_DURATION_MIN_MS, CLICK_HIGHLIGHT_OUTLINE_MAX, CLICK_HIGHLIGHT_OUTLINE_MIN, + CLICK_HIGHLIGHT_RADIUS_MAX, CLICK_HIGHLIGHT_RADIUS_MIN, CaptureConfig, ClickHighlightConfig, + DEFAULT_OCR_LANGUAGES, DEFAULT_PEN_SMOOTHING, DEFAULT_SHAPE_RECOGNITION_SENSITIVITY, + DragButtonConfig, DrawingConfig, ExportConfig, HelpOverlayStyle, HistoryConfig, InputHudConfig, + InputHudMode, InputHudPosition, LASER_FADE_MS_MAX, LASER_HOLD_MS_MAX, LASER_WIDTH_MAX, + LASER_WIDTH_MIN, LaserConfig, MAX_SHAPE_RECOGNITION_SENSITIVITY, MouseDragToolsConfig, + PDF_LABEL_APP_BOARD, PDF_LABEL_APP_BOARDS, PDF_LABEL_BOARD_NAME, PDF_LABEL_DEFAULT_TEMPLATE, PDF_LABEL_DOCUMENT_PAGE, PDF_LABEL_DOCUMENT_PAGES, PDF_LABEL_EXPORT_BOARD, PDF_LABEL_EXPORT_BOARDS, PDF_LABEL_PAGE, PDF_LABEL_PAGE_NAME, PDF_LABEL_PAGES, PDF_LABEL_PLACEHOLDERS, PRESET_SLOTS_MAX, PRESET_SLOTS_MIN, PdfExportConfig, PdfFitMode, diff --git a/src/config/tests/document.rs b/src/config/tests/document.rs index 7247ad6ad..a5c922855 100644 --- a/src/config/tests/document.rs +++ b/src/config/tests/document.rs @@ -2573,3 +2573,43 @@ fn default_config_omits_retired_toolbar_keys() { ); } } + +#[test] +fn click_highlight_load_repairs_nonfinite_values_and_preserves_finite_bounds() { + for (value, radius, thickness, channel) in [ + ("nan", 24.0, 4.0, None), + ("inf", 24.0, 4.0, None), + ("-inf", 24.0, 4.0, None), + ("-1.0", 16.0, 1.0, Some(0.0)), + ("200.0", 160.0, 12.0, Some(1.0)), + ("16.0", 16.0, 12.0, Some(1.0)), + ("160.0", 160.0, 12.0, Some(1.0)), + ("1.0", 16.0, 1.0, Some(1.0)), + ("12.0", 16.0, 12.0, Some(1.0)), + ("0.0", 16.0, 1.0, Some(0.0)), + ("0.5", 16.0, 1.0, Some(0.5)), + ] { + let temp = TempConfig::new("click-highlight-finite"); + temp.write(&format!( + "[ui.click_highlight]\nradius = {value}\noutline_thickness = {value}\nfill_color = [{value}, {value}, {value}, {value}]\noutline_color = [{value}, {value}, {value}, {value}]\n" + )); + + let document = ConfigDocument::load_from_path(&temp.path).unwrap(); + let highlight = &document.config().ui.click_highlight; + assert_eq!(highlight.radius, radius, "radius for {value}"); + assert_eq!( + highlight.outline_thickness, thickness, + "thickness for {value}" + ); + assert_eq!( + highlight.fill_color, + channel.map_or([1.0, 0.8, 0.0, 0.35], |v| [v; 4]), + "fill for {value}" + ); + assert_eq!( + highlight.outline_color, + channel.map_or([1.0, 0.6, 0.0, 0.9], |v| [v; 4]), + "outline for {value}" + ); + } +} diff --git a/src/config/tests/finite.rs b/src/config/tests/finite.rs new file mode 100644 index 000000000..a85532ebc --- /dev/null +++ b/src/config/tests/finite.rs @@ -0,0 +1,414 @@ +//! Load-policy contracts, inventoried independently against the declaration-derived schema. +use super::{Config, ConfigDocument}; +use toml::Value; + +#[derive(Clone)] +struct Case { + path: String, + fallback: Option, + bounds: Option<(f64, f64)>, +} + +fn cases() -> Vec { + let mut cases = Vec::new(); + for (path, default, min, max) in [ + ("drawing.default_thickness", 3.0, 1.0, 50.0), + ("drawing.default_eraser_size", 12.0, 1.0, 50.0), + ("drawing.marker_opacity", 0.32, 0.05, 0.9), + ("drawing.default_font_size", 32.0, 8.0, 72.0), + ("drawing.hit_test_tolerance", 6.0, 1.0, 20.0), + ("arrow.length", 20.0, 5.0, 50.0), + ("arrow.angle_degrees", 26.0, 15.0, 60.0), + ("spotlight.dim_opacity", 0.6, 0.1, 0.95), + ("spotlight.feather", 0.35, 0.0, 0.9), + ("spotlight.magnification", 1.0, 1.0, 4.0), + ("laser.width", 6.0, 2.0, 30.0), + ("ui.toolbar.scale", 1.0, 0.5, 3.0), + ("ui.click_highlight.radius", 24.0, 16.0, 160.0), + ("ui.click_highlight.outline_thickness", 4.0, 1.0, 12.0), + ("ui.input_hud.font_size", 18.0, 6.0, 72.0), + ("export.pdf.custom_width", 800.0, 1.0, 14_400.0), + ("export.pdf.custom_height", 600.0, 1.0, 14_400.0), + ("export.pdf.content_source_padding", 24.0, 0.0, 4_096.0), + ("export.pdf.labels.font_size", 10.0, 1.0, 72.0), + ("export.pdf.labels.margin", 12.0, 0.0, 240.0), + ("export.pdf.labels.padding_x", 6.0, 0.0, 120.0), + ("export.pdf.labels.padding_y", 3.0, 0.0, 120.0), + ] { + cases.push(Case { + path: path.into(), + fallback: Some(default), + bounds: Some((min, max)), + }); + } + #[cfg(feature = "tablet-input")] + for (field, default, min, max) in [ + ("min_thickness", 1.0, 1.0, 50.0), + ("max_thickness", 8.0, 1.0, 50.0), + ("pressure_variation_threshold", 0.1, 0.0, f64::MAX), + ("pressure_thickness_scale_step", 0.1, 0.0, 1.0), + ] { + cases.push(Case { + path: format!("tablet.{field}"), + fallback: Some(default), + bounds: Some((min, max)), + }); + } + for (path, default) in [ + ("ui.toolbar.top_offset", 0.0), + ("ui.toolbar.top_offset_y", 0.0), + ("ui.status_bar_style.font_size", 15.0), + ("ui.status_bar_style.padding", 11.0), + ("ui.status_bar_style.dot_radius", 6.0), + ("ui.help_overlay_style.font_size", 14.0), + ("ui.help_overlay_style.line_height", 22.0), + ("ui.help_overlay_style.padding", 32.0), + ("ui.help_overlay_style.border_width", 2.0), + ] { + cases.push(Case { + path: path.into(), + fallback: Some(default), + bounds: None, + }); + } + for (path, defaults, bounded) in [ + ("board.whiteboard_color", vec![0.992; 3], true), + ("board.blackboard_color", vec![0.067; 3], true), + ( + "board.whiteboard_pen_color", + vec![36.0 / 255.0, 31.0 / 255.0, 49.0 / 255.0], + true, + ), + ("board.blackboard_pen_color", vec![1.0; 3], true), + ("boards.items.0.background", vec![0.0; 3], true), + ("boards.items.0.background.rgb", vec![0.0; 3], true), + ("boards.items.0.default_pen_color", vec![0.0; 3], true), + ("boards.items.0.default_pen_color.rgb", vec![0.0; 3], true), + ("laser.color", vec![1.0, 0.16, 0.12, 1.0], true), + ( + "ui.click_highlight.fill_color", + vec![1.0, 0.8, 0.0, 0.35], + true, + ), + ( + "ui.click_highlight.outline_color", + vec![1.0, 0.6, 0.0, 0.9], + true, + ), + ( + "ui.status_bar_style.bg_color", + vec![0.0, 0.0, 0.0, 0.85], + false, + ), + ("ui.status_bar_style.text_color", vec![1.0; 4], false), + ( + "ui.help_overlay_style.bg_color", + vec![0.09, 0.1, 0.13, 1.0], + false, + ), + ( + "ui.help_overlay_style.border_color", + vec![0.33, 0.39, 0.52, 0.88], + false, + ), + ( + "ui.help_overlay_style.text_color", + vec![0.95, 0.96, 0.98, 1.0], + false, + ), + ( + "export.pdf.labels.text_color", + vec![0.1, 0.1, 0.1, 1.0], + true, + ), + ( + "export.pdf.labels.background_color", + vec![1.0, 1.0, 1.0, 0.85], + true, + ), + ] { + for (index, default) in defaults.into_iter().enumerate() { + cases.push(Case { + path: format!("{path}.{index}"), + fallback: Some(default), + bounds: bounded.then_some((0.0, 1.0)), + }); + } + } + for slot in 1..=5 { + let prefix = format!("presets.slot_{slot}"); + for field in [ + "size", + "tool_settings.pen.size", + "tool_settings.line.size", + "tool_settings.rect.size", + "tool_settings.ellipse.size", + "tool_settings.arrow.size", + "tool_settings.blur.size", + "tool_settings.marker.size", + "tool_settings.step_marker.size", + "tool_settings.eraser_size", + ] { + cases.push(Case { + path: format!("{prefix}.{field}"), + fallback: Some(1.0), + bounds: Some((1.0, 50.0)), + }); + } + for (field, min, max) in [ + ("marker_opacity", 0.05, 0.9), + ("font_size", 8.0, 72.0), + ("arrow_length", 5.0, 50.0), + ("arrow_angle", 15.0, 60.0), + ] { + cases.push(Case { + path: format!("{prefix}.{field}"), + fallback: None, + bounds: Some((min, max)), + }); + } + } + cases +} + +fn set(value: &mut Value, path: &str, replacement: Value) { + let (head, tail) = path.split_once('.').unwrap_or((path, "")); + let child = if let Ok(index) = head.parse::() { + &mut value.as_array_mut().unwrap()[index] + } else { + value + .as_table_mut() + .unwrap() + .entry(head.to_string()) + .or_insert_with(|| Value::Table(Default::default())) + }; + if tail.is_empty() { + *child = replacement; + } else { + set(child, tail, replacement); + } +} + +fn get<'a>(value: &'a Value, path: &str) -> Option<&'a Value> { + path.split('.').try_fold(value, |value, part| { + if let Ok(index) = part.parse::() { + value.as_array()?.get(index) + } else { + value.get(part) + } + }) +} + +fn fixture(path: &str) -> Value { + let mut value = Value::try_from(Config::default()).unwrap(); + if path.starts_with("presets.") { + let slot = path.split('.').nth(1).unwrap(); + let preset: Value = toml::from_str("tool = 'pen'\ncolor = 'red'\nsize = 3.0").unwrap(); + set(&mut value, &format!("presets.{slot}"), preset); + for tool in [ + "pen", + "line", + "rect", + "ellipse", + "arrow", + "blur", + "marker", + "step_marker", + ] { + set( + &mut value, + &format!("presets.{slot}.tool_settings.{tool}"), + toml::from_str("color = 'red'\nsize = 3.0").unwrap(), + ); + } + set( + &mut value, + &format!("presets.{slot}.tool_settings.eraser_size"), + Value::Float(12.0), + ); + } + if path.starts_with("boards.") { + let boards: Value = toml::from_str("[[items]]\nid = 'sample'\nname = 'Sample'\nbackground = [0.5, 0.5, 0.5]\ndefault_pen_color = [0.5, 0.5, 0.5]\n[[items]]\nid = 'transparent'\nname = 'Overlay'\nbackground = 'transparent'").unwrap(); + set(&mut value, "boards", boards); + if let Some((base, _)) = path.split_once(".rgb.") { + set( + &mut value, + base, + toml::from_str("rgb = [0.5, 0.5, 0.5]").unwrap(), + ); + } + } + // Isolate scalar bounds from the tablet min <= max relationship, tested separately. + if path == "tablet.min_thickness" { + set(&mut value, "tablet.max_thickness", Value::Float(50.0)); + } + value +} + +#[test] +fn document_load_enforces_finite_policy_for_every_float_and_array_component() { + let temp = crate::test_temp::tempdir().unwrap(); + let file = temp.path().join("config.toml"); + for case in cases() { + let mut inputs = vec![f64::NAN, f64::INFINITY, f64::NEG_INFINITY]; + if let Some((min, max)) = case.bounds { + inputs.extend([min.next_down(), min, min.next_up(), max.next_down(), max]); + if max < f64::MAX { + inputs.push(max.next_up()); + } + } else { + inputs.extend([-1.0, 0.0, 1.0, 1e9]); + } + for input in inputs { + let mut raw = fixture(&case.path); + set(&mut raw, &case.path, Value::Float(input)); + let source = toml::to_string(&raw).unwrap(); + std::fs::write(&file, &source).unwrap(); + let document = ConfigDocument::load_from_path(&file).unwrap(); + assert!( + document.section_errors().is_empty(), + "{}: {:?}", + case.path, + document.source() + ); + let loaded = Value::try_from(document.config()).unwrap(); + // Board RGB maps normalize to the canonical array representation. + let output_path = case.path.replace(".rgb.", "."); + let actual = get(&loaded, &output_path).and_then(Value::as_float); + let expected = if !input.is_finite() { + case.fallback + } else { + Some(match case.bounds { + Some((min, _)) if input < min => min, + Some((_, max)) if input > max => max, + _ => input, + }) + }; + assert_eq!(actual, expected, "{} input={input}", case.path); + assert_eq!( + std::fs::read_to_string(&file).unwrap(), + source, + "load must not rewrite authored non-finite values" + ); + } + } +} + +#[cfg(feature = "tablet-input")] +#[test] +fn tablet_normalizes_nonfinite_values_before_ordering_and_clamping() { + let temp = crate::test_temp::tempdir().unwrap(); + let file = temp.path().join("config.toml"); + for (min, max, expected) in [ + ("nan", "inf", (1.0, 8.0)), + ("60.0", "-10.0", (1.0, 50.0)), + ("45.0", "nan", (8.0, 45.0)), + ] { + std::fs::write( + &file, + format!("[tablet]\nmin_thickness = {min}\nmax_thickness = {max}\n"), + ) + .unwrap(); + let document = ConfigDocument::load_from_path(&file).unwrap(); + let tablet = &document.config().tablet; + assert_eq!((tablet.min_thickness, tablet.max_thickness), expected); + } +} + +#[test] +fn authored_save_rejects_nonfinite_float_with_a_field_level_correction() { + let mut edited = Config::default(); + edited.drawing.default_thickness = f64::NAN; + let Err(crate::config::SaveValidationError::CorrectedValues(corrections)) = + edited.validate_for_save() + else { + panic!("authored save must report the invalid numeric input"); + }; + assert_eq!(corrections.len(), 1); + assert_eq!(corrections[0].path, "drawing.default_thickness"); + assert!( + corrections[0] + .authored + .as_ref() + .and_then(Value::as_float) + .unwrap() + .is_nan() + ); + assert_eq!(corrections[0].corrected, Some(Value::Float(3.0))); +} + +#[cfg(feature = "config-schema")] +#[test] +fn float_load_cases_cover_the_independent_schema_inventory() { + use serde_json::Value as Json; + use std::collections::BTreeSet; + + fn inventory(root: &Json, node: &Json, path: &str, found: &mut BTreeSet) { + if let Some(reference) = node.get("$ref").and_then(Json::as_str) { + inventory( + root, + root.pointer(reference.strip_prefix('#').unwrap()).unwrap(), + path, + found, + ); + return; + } + let kind = node.get("type"); + if kind == Some(&Json::String("number".into())) + || kind + .and_then(Json::as_array) + .is_some_and(|types| types.iter().any(|kind| kind == "number")) + { + found.insert(path.into()); + } + if let Some(properties) = node.get("properties").and_then(Json::as_object) { + for (key, child) in properties { + let next = if path.is_empty() { + key.clone() + } else { + format!("{path}.{key}") + }; + inventory(root, child, &next, found); + } + } + // A newly introduced map can be empty in defaults and fixtures too. + if let Some(values) = node + .get("additionalProperties") + .filter(|value| value.is_object()) + { + inventory(root, values, &format!("{path}.{{key}}"), found); + } + if let Some(patterns) = node.get("patternProperties").and_then(Json::as_object) { + for values in patterns.values() { + inventory(root, values, &format!("{path}.{{key}}"), found); + } + } + if let Some(items) = node.get("prefixItems").and_then(Json::as_array) { + for (index, child) in items.iter().enumerate() { + inventory(root, child, &format!("{path}.{index}"), found); + } + } else if let Some(items) = node.get("items") { + let count = node.get("minItems").and_then(Json::as_u64).unwrap_or(1); + for index in 0..count { + inventory(root, items, &format!("{path}.{index}"), found); + } + } + for branch in ["anyOf", "oneOf", "allOf"] { + if let Some(children) = node.get(branch).and_then(Json::as_array) { + for child in children { + inventory(root, child, path, found); + } + } + } + } + + let schema = Config::json_schema(); + let mut declared = BTreeSet::new(); + inventory(&schema, &schema, "", &mut declared); + let exercised: BTreeSet<_> = cases().into_iter().map(|case| case.path).collect(); + assert!(!declared.is_empty()); + assert_eq!( + declared, exercised, + "new floats (including absent optionals and collections) require load-policy cases" + ); +} diff --git a/src/config/tests/mod.rs b/src/config/tests/mod.rs index 1af6e12ef..44f5f521d 100644 --- a/src/config/tests/mod.rs +++ b/src/config/tests/mod.rs @@ -1,6 +1,7 @@ mod board_grid; mod document; mod file_io; +mod finite; mod immutability; mod load; mod migration; diff --git a/src/config/tests/write_target.rs b/src/config/tests/write_target.rs index b75696cdb..7db588656 100644 --- a/src/config/tests/write_target.rs +++ b/src/config/tests/write_target.rs @@ -241,7 +241,7 @@ fn a_retarget_after_the_last_check_still_writes_the_file_the_window_was_about() /// file's text over the new one, destroying an edit nobody here ever read, and /// report a clean save. The identity of the file that was checked is what the /// rename is made conditional on, so the save is refused instead — and refused -/// in the wording that sends the editors round again, because reloading and +/// as the typed source-change error that sends editors round again, because reloading and /// reapplying onto the file that is there now is exactly the right recovery. #[test] fn a_file_swapped_in_under_the_checked_name_is_refused_rather_than_overwritten() { diff --git a/src/config/types/click_highlight.rs b/src/config/types/click_highlight.rs index 5c91d52b4..139eca0a1 100644 --- a/src/config/types/click_highlight.rs +++ b/src/config/types/click_highlight.rs @@ -1,5 +1,15 @@ use serde::{Deserialize, Serialize}; +/// Accepted click-highlight radius in logical pixels. +pub const CLICK_HIGHLIGHT_RADIUS_MIN: f64 = 16.0; +pub const CLICK_HIGHLIGHT_RADIUS_MAX: f64 = 160.0; +/// Accepted click-highlight outline thickness in logical pixels. +pub const CLICK_HIGHLIGHT_OUTLINE_MIN: f64 = 1.0; +pub const CLICK_HIGHLIGHT_OUTLINE_MAX: f64 = 12.0; +/// Accepted click-highlight lifetime in milliseconds. +pub const CLICK_HIGHLIGHT_DURATION_MIN_MS: u64 = 150; +pub const CLICK_HIGHLIGHT_DURATION_MAX_MS: u64 = 1500; + /// Click highlight configuration for mouse press indicator. #[cfg_attr(feature = "config-schema", derive(schemars::JsonSchema))] #[derive(Debug, Clone, Serialize, Deserialize)] diff --git a/src/config/types/mod.rs b/src/config/types/mod.rs index fdebb73e1..7a09a868b 100644 --- a/src/config/types/mod.rs +++ b/src/config/types/mod.rs @@ -37,7 +37,11 @@ pub use capture::{ CaptureConfig, DEFAULT_OCR_LANGUAGES, RegionCaptureConfig, validate_capture_format, validate_filename_template, validate_ocr_languages, }; -pub use click_highlight::ClickHighlightConfig; +pub use click_highlight::{ + CLICK_HIGHLIGHT_DURATION_MAX_MS, CLICK_HIGHLIGHT_DURATION_MIN_MS, CLICK_HIGHLIGHT_OUTLINE_MAX, + CLICK_HIGHLIGHT_OUTLINE_MIN, CLICK_HIGHLIGHT_RADIUS_MAX, CLICK_HIGHLIGHT_RADIUS_MIN, + ClickHighlightConfig, +}; pub use context_menu::ContextMenuUiConfig; pub(crate) use drawing::DEFAULT_HIT_TEST_TOLERANCE; pub use drawing::DEFAULT_PEN_SMOOTHING; diff --git a/src/config/types/presets.rs b/src/config/types/presets.rs index 7c8bd08ef..6beaa9974 100644 --- a/src/config/types/presets.rs +++ b/src/config/types/presets.rs @@ -1,9 +1,6 @@ use crate::config::{MouseDragToolsConfig, enums::ColorSpec}; use crate::domain::{Color, EraserMode, Tool}; use crate::draw::EraserKind; -use crate::input::tool::{ - PerToolDrawingSettings, ToolDrawingSettings, ToolSettingsSlot, ToolSizeSource, -}; use serde::{Deserialize, Serialize}; pub const PRESET_SLOTS_MIN: usize = 3; @@ -114,19 +111,6 @@ pub struct PresetToolSettingConfig { pub size: f64, } -impl PresetToolSettingConfig { - pub fn from_runtime(settings: ToolDrawingSettings) -> Self { - Self { - color: settings.color.into(), - size: settings.thickness, - } - } - - pub fn to_runtime(&self) -> ToolDrawingSettings { - ToolDrawingSettings::new(self.color.to_color(), self.size) - } -} - /// Full drawing tool profile captured by a preset. #[cfg_attr(feature = "config-schema", derive(schemars::JsonSchema))] #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] @@ -142,85 +126,6 @@ pub struct PresetToolStatesConfig { pub eraser_size: f64, } -impl PresetToolStatesConfig { - pub fn from_runtime(settings: &PerToolDrawingSettings, eraser_size: f64) -> Self { - Self { - pen: PresetToolSettingConfig::from_runtime(settings.pen), - line: PresetToolSettingConfig::from_runtime(settings.line), - rect: PresetToolSettingConfig::from_runtime(settings.rect), - ellipse: PresetToolSettingConfig::from_runtime(settings.ellipse), - arrow: PresetToolSettingConfig::from_runtime(settings.arrow), - blur: PresetToolSettingConfig::from_runtime(settings.blur), - marker: PresetToolSettingConfig::from_runtime(settings.marker), - step_marker: PresetToolSettingConfig::from_runtime(settings.step_marker), - eraser_size, - } - } - - pub fn to_runtime(&self) -> PerToolDrawingSettings { - PerToolDrawingSettings { - pen: self.pen.to_runtime(), - line: self.line.to_runtime(), - rect: self.rect.to_runtime(), - ellipse: self.ellipse.to_runtime(), - arrow: self.arrow.to_runtime(), - blur: self.blur.to_runtime(), - marker: self.marker.to_runtime(), - step_marker: self.step_marker.to_runtime(), - } - } - - pub fn color_spec_for_tool(&self, tool: Tool) -> ColorSpec { - self.setting_for_slot(tool.settings_slot()).color.clone() - } - - pub fn size_for_tool(&self, tool: Tool) -> f64 { - let profile = tool.profile(); - match profile.size_source { - ToolSizeSource::EraserSize => self.eraser_size, - ToolSizeSource::DrawingThickness => self.setting_for_slot(profile.settings_slot).size, - } - } - - #[allow(dead_code)] - pub fn set_preview_tool(&mut self, tool: Tool, color: ColorSpec, size: f64) { - let profile = tool.profile(); - self.setting_for_slot_mut(profile.settings_slot).color = color; - match profile.size_source { - ToolSizeSource::EraserSize => self.eraser_size = size, - ToolSizeSource::DrawingThickness => { - self.setting_for_slot_mut(profile.settings_slot).size = size; - } - } - } - - fn setting_for_slot(&self, slot: ToolSettingsSlot) -> &PresetToolSettingConfig { - match slot { - ToolSettingsSlot::Pen => &self.pen, - ToolSettingsSlot::Line => &self.line, - ToolSettingsSlot::Rect => &self.rect, - ToolSettingsSlot::Ellipse => &self.ellipse, - ToolSettingsSlot::Arrow => &self.arrow, - ToolSettingsSlot::Blur => &self.blur, - ToolSettingsSlot::Marker => &self.marker, - ToolSettingsSlot::StepMarker => &self.step_marker, - } - } - - fn setting_for_slot_mut(&mut self, slot: ToolSettingsSlot) -> &mut PresetToolSettingConfig { - match slot { - ToolSettingsSlot::Pen => &mut self.pen, - ToolSettingsSlot::Line => &mut self.line, - ToolSettingsSlot::Rect => &mut self.rect, - ToolSettingsSlot::Ellipse => &mut self.ellipse, - ToolSettingsSlot::Arrow => &mut self.arrow, - ToolSettingsSlot::Blur => &mut self.blur, - ToolSettingsSlot::Marker => &mut self.marker, - ToolSettingsSlot::StepMarker => &mut self.step_marker, - } - } -} - /// Preset slot configuration for quick tool switching. #[cfg_attr(feature = "config-schema", derive(schemars::JsonSchema))] #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] diff --git a/src/config/types/tablet.rs b/src/config/types/tablet.rs index aca01c2a9..3b15cad7b 100644 --- a/src/config/types/tablet.rs +++ b/src/config/types/tablet.rs @@ -1,7 +1,7 @@ use serde::{Deserialize, Serialize}; use crate::config::keybindings::Action; -use crate::input::state::{PressureThicknessEditMode, PressureThicknessEntryMode}; +use crate::domain::{PressureThicknessEditMode, PressureThicknessEntryMode}; /// Binding for a single stylus barrel button. /// diff --git a/src/config/validate/arrow.rs b/src/config/validate/arrow.rs index f28793c9a..82fbf33d4 100644 --- a/src/config/validate/arrow.rs +++ b/src/config/validate/arrow.rs @@ -1,8 +1,16 @@ -use super::Config; +use super::{Config, float::finite_or_default}; use crate::config::{ARROW_ANGLE_MAX, ARROW_ANGLE_MIN, ARROW_LENGTH_MAX, ARROW_LENGTH_MIN}; impl Config { pub(super) fn validate_arrow(&mut self) { + let defaults = crate::config::ArrowConfig::default(); + self.arrow.length = finite_or_default(self.arrow.length, defaults.length, "arrow.length"); + self.arrow.angle_degrees = finite_or_default( + self.arrow.angle_degrees, + defaults.angle_degrees, + "arrow.angle_degrees", + ); + if !(ARROW_LENGTH_MIN..=ARROW_LENGTH_MAX).contains(&self.arrow.length) { log::warn!( "Invalid arrow length {:.1}, clamping to {ARROW_LENGTH_MIN:.1}-{ARROW_LENGTH_MAX:.1} range", diff --git a/src/config/validate/board.rs b/src/config/validate/board.rs index 1c3c64285..9f2326e9b 100644 --- a/src/config/validate/board.rs +++ b/src/config/validate/board.rs @@ -1,4 +1,4 @@ -use super::Config; +use super::{Config, float::finite_or_default}; impl Config { pub(super) fn validate_board(&mut self) { @@ -14,6 +14,34 @@ impl Config { self.board.default_mode = "transparent".to_string(); } + let defaults = crate::config::BoardConfig::default(); + for (field, color, fallback) in [ + ( + "board.whiteboard_color", + &mut self.board.whiteboard_color, + defaults.whiteboard_color, + ), + ( + "board.blackboard_color", + &mut self.board.blackboard_color, + defaults.blackboard_color, + ), + ( + "board.whiteboard_pen_color", + &mut self.board.whiteboard_pen_color, + defaults.whiteboard_pen_color, + ), + ( + "board.blackboard_pen_color", + &mut self.board.blackboard_pen_color, + defaults.blackboard_pen_color, + ), + ] { + for (index, (component, fallback)) in color.iter_mut().zip(fallback).enumerate() { + *component = finite_or_default(*component, fallback, &format!("{field}[{index}]")); + } + } + // Validate board color RGB values (0.0-1.0) for i in 0..3 { if !(0.0..=1.0).contains(&self.board.whiteboard_color[i]) { diff --git a/src/config/validate/boards.rs b/src/config/validate/boards.rs index c92dbdf5c..5e3fca0b0 100644 --- a/src/config/validate/boards.rs +++ b/src/config/validate/boards.rs @@ -2,7 +2,7 @@ use crate::config::types::{BoardBackgroundConfig, BoardColorConfig, BoardsConfig use crate::domain::{BoundaryBoardIdSet, clamp_board_rgb}; use log::warn; -use super::Config; +use super::{Config, float::finite_or_default}; impl Config { pub(super) fn validate_boards(&mut self) { @@ -143,10 +143,14 @@ fn normalize_background(background: &mut BoardBackgroundConfig, id: &str) { } fn clamp_color(color: &mut BoardColorConfig, label: &str) { - let original = color.rgb(); - let (rgb, clamped) = clamp_board_rgb(original); + let mut finite = color.rgb(); + for (index, component) in finite.iter_mut().enumerate() { + *component = finite_or_default(*component, 0.0, &format!("{label}[{index}]")); + } + let (rgb, clamped) = clamp_board_rgb(finite); + if clamped { - for (i, (before, after)) in original.iter().zip(rgb.iter()).enumerate() { + for (i, (before, after)) in finite.iter().zip(rgb.iter()).enumerate() { if before != after { warn!( "Invalid {}[{}] = {:.3}, clamping to 0.0-1.0", @@ -155,5 +159,6 @@ fn clamp_color(color: &mut BoardColorConfig, label: &str) { } } } + *color = BoardColorConfig::Rgb(rgb); } diff --git a/src/config/validate/drawing.rs b/src/config/validate/drawing.rs index b02271403..9f8204384 100644 --- a/src/config/validate/drawing.rs +++ b/src/config/validate/drawing.rs @@ -1,13 +1,38 @@ -use super::Config; +use super::{Config, float::finite_or_default}; use crate::config::types::{DEFAULT_HIT_TEST_TOLERANCE, MAX_SHAPE_RECOGNITION_SENSITIVITY}; use crate::domain::{MAX_STROKE_THICKNESS, MIN_STROKE_THICKNESS}; use crate::draw::shape::{MAX_PEN_SMOOTHING, REGULAR_POLYGON_MAX_SIDES, REGULAR_POLYGON_MIN_SIDES}; impl Config { pub(super) fn validate_drawing(&mut self) { + let defaults = crate::config::DrawingConfig::default(); + for (field, value, fallback) in [ + ( + "drawing.default_thickness", + &mut self.drawing.default_thickness, + defaults.default_thickness, + ), + ( + "drawing.default_eraser_size", + &mut self.drawing.default_eraser_size, + defaults.default_eraser_size, + ), + ( + "drawing.marker_opacity", + &mut self.drawing.marker_opacity, + defaults.marker_opacity, + ), + ( + "drawing.default_font_size", + &mut self.drawing.default_font_size, + defaults.default_font_size, + ), + ] { + *value = finite_or_default(*value, fallback, field); + } + self.validate_stroke_sizes(); - // Marker opacity: 0.05 - 0.9 // Font cycle: drop blank entries and repeats, keeping the given order. // A repeat would make the action appear to skip, and a blank name would // resolve to whatever the font system falls back to. @@ -49,6 +74,7 @@ impl Config { self.drawing.shape_recognition_sensitivity = MAX_SHAPE_RECOGNITION_SENSITIVITY; } + // Marker opacity: 0.05 - 0.9 if !(0.05..=0.9).contains(&self.drawing.marker_opacity) { log::warn!( "Invalid marker_opacity {:.2}, clamping to 0.05-0.90 range", diff --git a/src/config/validate/export.rs b/src/config/validate/export.rs index 3ee4dd5a8..a0f08aa32 100644 --- a/src/config/validate/export.rs +++ b/src/config/validate/export.rs @@ -113,10 +113,7 @@ fn validate_pdf_labels(labels: &mut PdfLabelConfig) { } fn sanitize_range(value: f64, min: f64, max: f64, fallback: f64, field: &str) -> f64 { - if !value.is_finite() { - log::warn!("Invalid {field}: {value}; resetting to {fallback}"); - return fallback; - } + let value = super::float::finite_or_default(value, fallback, field); let clamped = value.clamp(min, max); if (clamped - value).abs() > f64::EPSILON { log::warn!("Clamping {field} from {value} to {clamped}"); diff --git a/src/config/validate/float.rs b/src/config/validate/float.rs new file mode 100644 index 000000000..16497012a --- /dev/null +++ b/src/config/validate/float.rs @@ -0,0 +1,10 @@ +/// Normalize before comparing or clamping: neither operation repairs NaN. +/// Callers supply their own finite default; finite inputs retain the owner's policy. +pub(super) fn finite_or_default(value: f64, fallback: f64, field: &str) -> f64 { + if value.is_finite() { + value + } else { + log::warn!("Non-finite {field}: {value}; resetting to {fallback}"); + fallback + } +} diff --git a/src/config/validate/mod.rs b/src/config/validate/mod.rs index 7b292c948..b17ba6e94 100644 --- a/src/config/validate/mod.rs +++ b/src/config/validate/mod.rs @@ -6,6 +6,7 @@ mod boards; mod capture; mod drawing; mod export; +mod float; mod fonts; mod history; mod keybindings; diff --git a/src/config/validate/presets.rs b/src/config/validate/presets.rs index 1130db488..029a10b92 100644 --- a/src/config/validate/presets.rs +++ b/src/config/validate/presets.rs @@ -1,4 +1,4 @@ -use super::Config; +use super::{Config, float::finite_or_default}; use crate::domain::{MAX_STROKE_THICKNESS, MIN_STROKE_THICKNESS}; use crate::draw::{REGULAR_POLYGON_MAX_SIDES, REGULAR_POLYGON_MIN_SIDES, clamp_regular_sides}; @@ -93,6 +93,11 @@ fn validate_preset(slot: usize, preset: &mut ToolPresetConfig) { } fn clamp_stroke_size(slot: usize, label: &str, value: &mut f64) { + *value = finite_or_default( + *value, + MIN_STROKE_THICKNESS, + &format!("presets.slot_{slot}.{label}"), + ); if (MIN_STROKE_THICKNESS..=MAX_STROKE_THICKNESS).contains(value) { return; } @@ -108,6 +113,15 @@ fn clamp_stroke_size(slot: usize, label: &str, value: &mut f64) { } fn clamp_optional_float(slot: usize, value: &mut Option, range: PresetFloatRange) { + if value.is_some_and(|value| !value.is_finite()) { + log::warn!( + "Non-finite {} in preset slot {slot}; ignoring the override", + range.label + ); + *value = None; + return; + } + let Some(value) = value.as_mut() else { return; }; diff --git a/src/config/validate/tablet.rs b/src/config/validate/tablet.rs index 3b983a0e2..203c87655 100644 --- a/src/config/validate/tablet.rs +++ b/src/config/validate/tablet.rs @@ -1,14 +1,41 @@ -use super::Config; +use super::{Config, float::finite_or_default}; use crate::domain::{MAX_STROKE_THICKNESS, MIN_STROKE_THICKNESS}; impl Config { pub(super) fn validate_tablet(&mut self) { + let defaults = crate::config::TabletInputConfig::default(); + for (field, value, fallback) in [ + ( + "tablet.min_thickness", + &mut self.tablet.min_thickness, + defaults.min_thickness, + ), + ( + "tablet.max_thickness", + &mut self.tablet.max_thickness, + defaults.max_thickness, + ), + ( + "tablet.pressure_variation_threshold", + &mut self.tablet.pressure_variation_threshold, + defaults.pressure_variation_threshold, + ), + ( + "tablet.pressure_thickness_scale_step", + &mut self.tablet.pressure_thickness_scale_step, + defaults.pressure_thickness_scale_step, + ), + ] { + *value = finite_or_default(*value, fallback, field); + } + if self.tablet.min_thickness > self.tablet.max_thickness { std::mem::swap( &mut self.tablet.min_thickness, &mut self.tablet.max_thickness, ); } + self.tablet.min_thickness = self .tablet .min_thickness @@ -17,6 +44,7 @@ impl Config { .tablet .max_thickness .clamp(MIN_STROKE_THICKNESS, MAX_STROKE_THICKNESS); + if self.tablet.pressure_variation_threshold < 0.0 { self.tablet.pressure_variation_threshold = 0.0; } diff --git a/src/config/validate/ui.rs b/src/config/validate/ui.rs index ca575971b..392643f3a 100644 --- a/src/config/validate/ui.rs +++ b/src/config/validate/ui.rs @@ -1,33 +1,16 @@ use super::Config; +use crate::config::{ + CLICK_HIGHLIGHT_DURATION_MAX_MS, CLICK_HIGHLIGHT_DURATION_MIN_MS, CLICK_HIGHLIGHT_OUTLINE_MAX, + CLICK_HIGHLIGHT_OUTLINE_MIN, CLICK_HIGHLIGHT_RADIUS_MAX, CLICK_HIGHLIGHT_RADIUS_MIN, + ClickHighlightConfig, +}; + +mod styles; impl Config { pub(super) fn validate_ui(&mut self) { - // Validate click highlight settings - if !(16.0..=160.0).contains(&self.ui.click_highlight.radius) { - log::warn!( - "Invalid click highlight radius {:.1}, clamping to 16.0-160.0 range", - self.ui.click_highlight.radius - ); - self.ui.click_highlight.radius = self.ui.click_highlight.radius.clamp(16.0, 160.0); - } - - if !(1.0..=12.0).contains(&self.ui.click_highlight.outline_thickness) { - log::warn!( - "Invalid click highlight outline thickness {:.1}, clamping to 1.0-12.0 range", - self.ui.click_highlight.outline_thickness - ); - self.ui.click_highlight.outline_thickness = - self.ui.click_highlight.outline_thickness.clamp(1.0, 12.0); - } - - if !(150..=1500).contains(&self.ui.click_highlight.duration_ms) { - log::warn!( - "Invalid click highlight duration {}ms, clamping to 150-1500ms range", - self.ui.click_highlight.duration_ms - ); - self.ui.click_highlight.duration_ms = - self.ui.click_highlight.duration_ms.clamp(150, 1500); - } + self.validate_ui_styles(); + self.validate_click_highlight(); if !(300..=5000).contains(&self.ui.command_palette_toast_duration_ms) { log::warn!( @@ -67,13 +50,90 @@ impl Config { ); } - self.validate_click_highlight_colors(); - self.validate_input_hud(); } + fn validate_click_highlight(&mut self) { + // Non-finite values use the field's default; finite values retain clamping. + let defaults = ClickHighlightConfig::default(); + if !self.ui.click_highlight.radius.is_finite() { + log::warn!( + "Non-finite click highlight radius, resetting to {}", + defaults.radius + ); + self.ui.click_highlight.radius = defaults.radius; + } + if !self.ui.click_highlight.outline_thickness.is_finite() { + log::warn!( + "Non-finite click highlight outline thickness, resetting to {}", + defaults.outline_thickness + ); + self.ui.click_highlight.outline_thickness = defaults.outline_thickness; + } + + // Validate click highlight settings + if !(CLICK_HIGHLIGHT_RADIUS_MIN..=CLICK_HIGHLIGHT_RADIUS_MAX) + .contains(&self.ui.click_highlight.radius) + { + log::warn!( + "Invalid click highlight radius {:.1}, clamping to 16.0-160.0 range", + self.ui.click_highlight.radius + ); + self.ui.click_highlight.radius = self + .ui + .click_highlight + .radius + .clamp(CLICK_HIGHLIGHT_RADIUS_MIN, CLICK_HIGHLIGHT_RADIUS_MAX); + } + + if !(CLICK_HIGHLIGHT_OUTLINE_MIN..=CLICK_HIGHLIGHT_OUTLINE_MAX) + .contains(&self.ui.click_highlight.outline_thickness) + { + log::warn!( + "Invalid click highlight outline thickness {:.1}, clamping to 1.0-12.0 range", + self.ui.click_highlight.outline_thickness + ); + self.ui.click_highlight.outline_thickness = self + .ui + .click_highlight + .outline_thickness + .clamp(CLICK_HIGHLIGHT_OUTLINE_MIN, CLICK_HIGHLIGHT_OUTLINE_MAX); + } + + if !(CLICK_HIGHLIGHT_DURATION_MIN_MS..=CLICK_HIGHLIGHT_DURATION_MAX_MS) + .contains(&self.ui.click_highlight.duration_ms) + { + log::warn!( + "Invalid click highlight duration {}ms, clamping to 150-1500ms range", + self.ui.click_highlight.duration_ms + ); + self.ui.click_highlight.duration_ms = self.ui.click_highlight.duration_ms.clamp( + CLICK_HIGHLIGHT_DURATION_MIN_MS, + CLICK_HIGHLIGHT_DURATION_MAX_MS, + ); + } + + self.validate_click_highlight_colors(); + } + fn validate_click_highlight_colors(&mut self) { + let defaults = ClickHighlightConfig::default(); for i in 0..4 { + if !self.ui.click_highlight.fill_color[i].is_finite() { + log::warn!( + "Non-finite click highlight fill_color[{i}], resetting to {}", + defaults.fill_color[i] + ); + self.ui.click_highlight.fill_color[i] = defaults.fill_color[i]; + } + if !self.ui.click_highlight.outline_color[i].is_finite() { + log::warn!( + "Non-finite click highlight outline_color[{i}], resetting to {}", + defaults.outline_color[i] + ); + self.ui.click_highlight.outline_color[i] = defaults.outline_color[i]; + } + if !(0.0..=1.0).contains(&self.ui.click_highlight.fill_color[i]) { log::warn!( "Invalid click highlight fill_color[{}] = {:.3}, clamping to 0.0-1.0", diff --git a/src/config/validate/ui/styles.rs b/src/config/validate/ui/styles.rs new file mode 100644 index 000000000..8a8ef79f1 --- /dev/null +++ b/src/config/validate/ui/styles.rs @@ -0,0 +1,90 @@ +use super::super::{Config, float::finite_or_default}; + +impl Config { + /// These styling fields have no load-time finite bounds. Preserve authored + /// finite values, but keep NaN/infinity out of layout and Cairo consumers. + pub(super) fn validate_ui_styles(&mut self) { + let defaults = crate::config::UiConfig::default(); + for (field, value, fallback) in [ + ( + "ui.toolbar.top_offset", + &mut self.ui.toolbar.top_offset, + defaults.toolbar.top_offset, + ), + ( + "ui.toolbar.top_offset_y", + &mut self.ui.toolbar.top_offset_y, + defaults.toolbar.top_offset_y, + ), + ( + "ui.status_bar_style.font_size", + &mut self.ui.status_bar_style.font_size, + defaults.status_bar_style.font_size, + ), + ( + "ui.status_bar_style.padding", + &mut self.ui.status_bar_style.padding, + defaults.status_bar_style.padding, + ), + ( + "ui.status_bar_style.dot_radius", + &mut self.ui.status_bar_style.dot_radius, + defaults.status_bar_style.dot_radius, + ), + ( + "ui.help_overlay_style.font_size", + &mut self.ui.help_overlay_style.font_size, + defaults.help_overlay_style.font_size, + ), + ( + "ui.help_overlay_style.line_height", + &mut self.ui.help_overlay_style.line_height, + defaults.help_overlay_style.line_height, + ), + ( + "ui.help_overlay_style.padding", + &mut self.ui.help_overlay_style.padding, + defaults.help_overlay_style.padding, + ), + ( + "ui.help_overlay_style.border_width", + &mut self.ui.help_overlay_style.border_width, + defaults.help_overlay_style.border_width, + ), + ] { + *value = finite_or_default(*value, fallback, field); + } + + for (field, color, fallback) in [ + ( + "ui.status_bar_style.bg_color", + &mut self.ui.status_bar_style.bg_color, + defaults.status_bar_style.bg_color, + ), + ( + "ui.status_bar_style.text_color", + &mut self.ui.status_bar_style.text_color, + defaults.status_bar_style.text_color, + ), + ( + "ui.help_overlay_style.bg_color", + &mut self.ui.help_overlay_style.bg_color, + defaults.help_overlay_style.bg_color, + ), + ( + "ui.help_overlay_style.border_color", + &mut self.ui.help_overlay_style.border_color, + defaults.help_overlay_style.border_color, + ), + ( + "ui.help_overlay_style.text_color", + &mut self.ui.help_overlay_style.text_color, + defaults.help_overlay_style.text_color, + ), + ] { + for (index, (component, fallback)) in color.iter_mut().zip(fallback).enumerate() { + *component = finite_or_default(*component, fallback, &format!("{field}[{index}]")); + } + } + } +} diff --git a/src/domain/board_validation.rs b/src/domain/board_validation.rs index 6832de620..f989773f9 100644 --- a/src/domain/board_validation.rs +++ b/src/domain/board_validation.rs @@ -1,6 +1,8 @@ //! Pure normalization at configuration and persistence boundaries. use std::collections::HashSet; +/// Clamp RGB components to [0, 1]. Callers must replace NaN before calling; +/// range clamping alone does not turn NaN into a usable color component. pub fn clamp_board_rgb(mut rgb: [f64; 3]) -> ([f64; 3], bool) { let mut clamped = false; for component in &mut rgb { diff --git a/src/domain/mod.rs b/src/domain/mod.rs index 36d8105f6..54dd73cbd 100644 --- a/src/domain/mod.rs +++ b/src/domain/mod.rs @@ -11,6 +11,7 @@ mod board_validation; pub mod color; mod drawing; mod onboarding; +mod pressure; mod tool; pub use action::Action; @@ -27,6 +28,7 @@ pub use board_validation::{ pub use color::Color; pub use drawing::{MAX_STROKE_THICKNESS, MIN_STROKE_THICKNESS, step_stroke_thickness}; pub use onboarding::OnboardingTip; +pub use pressure::{PressureThicknessEditMode, PressureThicknessEntryMode}; pub use tool::{DragBindableTool, DragTool, EraserMode, Tool}; #[cfg(test)] diff --git a/src/domain/pressure.rs b/src/domain/pressure.rs new file mode 100644 index 000000000..f95f356a1 --- /dev/null +++ b/src/domain/pressure.rs @@ -0,0 +1,23 @@ +//! Stable pressure-stroke thickness editing preferences. + +use serde::{Deserialize, Serialize}; + +#[cfg_attr(feature = "config-schema", derive(schemars::JsonSchema))] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Default)] +#[serde(rename_all = "snake_case")] +pub enum PressureThicknessEditMode { + #[default] + Disabled, + Add, + Scale, +} + +#[cfg_attr(feature = "config-schema", derive(schemars::JsonSchema))] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Default)] +#[serde(rename_all = "snake_case")] +pub enum PressureThicknessEntryMode { + Never, + #[default] + PressureOnly, + AnyPressure, +} diff --git a/src/domain/tests.rs b/src/domain/tests.rs index 378b60925..7fb5a6c81 100644 --- a/src/domain/tests.rs +++ b/src/domain/tests.rs @@ -1,6 +1,6 @@ use std::fmt::Debug; use std::fs; -use std::path::Path; +use std::path::{Path, PathBuf}; use serde::Serialize; use serde::de::DeserializeOwned; @@ -388,13 +388,24 @@ fn established_public_paths_reexport_domain_types() { let _: BoardSpec = board; } -#[test] -fn production_domain_sources_have_no_upward_crate_dependencies() { - let domain_dir = Path::new(env!("CARGO_MANIFEST_DIR")).join("src/domain"); +fn check_domain_sources(domain_dir: &Path) -> Result { let mut checked = 0; - for entry in fs::read_dir(&domain_dir).expect("read src/domain") { - let path = entry.expect("read domain entry").path(); + for entry in fs::read_dir(domain_dir).expect("read domain directory") { + let entry = entry.expect("read domain entry"); + let path = entry.path(); + let file_type = entry.file_type().expect("read domain entry type"); + + // Do not recurse through directory aliases, but still inspect symlinked Rust files. + if file_type.is_symlink() && path.is_dir() { + continue; + } + if file_type.is_dir() { + if path.file_name().and_then(|name| name.to_str()) != Some("tests") { + checked += check_domain_sources(&path)?; + } + continue; + } if path.extension().and_then(|extension| extension.to_str()) != Some("rs") || path.file_name().and_then(|name| name.to_str()) == Some("tests.rs") { @@ -402,20 +413,62 @@ fn production_domain_sources_have_no_upward_crate_dependencies() { } let source = fs::read_to_string(&path).expect("read domain source"); - assert!( - !source.contains("crate::"), - "{} contains an upward crate dependency", - path.display() - ); + if source.contains("crate::") { + return Err(path); + } checked += 1; } - assert_eq!( - checked, 9, - "architecture test must cover every domain source" + Ok(checked) +} + +#[test] +fn production_domain_sources_have_no_upward_crate_dependencies() { + let domain_dir = Path::new(env!("CARGO_MANIFEST_DIR")).join("src/domain"); + let checked = check_domain_sources(&domain_dir) + .unwrap_or_else(|path| panic!("{} contains an upward crate dependency", path.display())); + + assert!( + checked > 0, + "architecture test must inspect production domain sources" ); } +#[test] +fn nested_domain_sources_are_checked_for_upward_dependencies() { + let temp = crate::test_temp::tempdir().unwrap(); + let nested = temp.path().join("nested/deeper"); + fs::create_dir_all(&nested).unwrap(); + let source = nested.join("value.rs"); + fs::write(&source, "use crate::config::Config;\n").unwrap(); + + assert_eq!(check_domain_sources(temp.path()).unwrap_err(), source); + + fs::write(&source, "pub struct Value;\n").unwrap(); + + let tests = nested.join("tests"); + fs::create_dir(&tests).unwrap(); + fs::write(tests.join("fixture.rs"), "use crate::config::Config;\n").unwrap(); + #[cfg(unix)] + std::os::unix::fs::symlink(temp.path(), nested.join("cycle")).unwrap(); + + assert_eq!(check_domain_sources(temp.path()).unwrap(), 1); + + #[cfg(unix)] + { + let linked = crate::test_temp::tempdir().unwrap(); + fs::write( + linked.path().join("source.rs"), + "use crate::config::Config;\n", + ) + .unwrap(); + let alias = nested.join("alias.rs"); + std::os::unix::fs::symlink(linked.path().join("source.rs"), &alias).unwrap(); + + assert_eq!(check_domain_sources(temp.path()).unwrap_err(), alias); + } +} + #[test] fn region_capture_action_classification_is_complete_and_narrow() { for action in [ @@ -439,3 +492,23 @@ fn region_capture_action_classification_is_complete_and_narrow() { assert!(!action.is_region_capture(), "action={action:?}"); } } + +#[test] +fn pressure_preferences_preserve_public_paths_and_serialized_names() { + use super::{PressureThicknessEditMode as Edit, PressureThicknessEntryMode as Entry}; + + assert_json_names(&[ + (Edit::Disabled, "disabled"), + (Edit::Add, "add"), + (Edit::Scale, "scale"), + ]); + assert_json_names(&[ + (Entry::Never, "never"), + (Entry::PressureOnly, "pressure_only"), + (Entry::AnyPressure, "any_pressure"), + ]); + let legacy_edit: crate::input::state::PressureThicknessEditMode = Edit::Scale; + let legacy_entry: crate::input::state::PressureThicknessEntryMode = Entry::AnyPressure; + assert_eq!(legacy_edit, Edit::Scale); + assert_eq!(legacy_entry, Entry::AnyPressure); +} diff --git a/src/durable_io.rs b/src/durable_io.rs index 16aa43c05..39f5f19b7 100644 --- a/src/durable_io.rs +++ b/src/durable_io.rs @@ -1,3 +1,7 @@ +mod lock; + +pub use lock::{lock_exclusive, lock_shared, try_lock_exclusive, unlock}; + use std::fs::{self, File, OpenOptions}; use std::io::{self, ErrorKind, Read, Write}; use std::path::{Path, PathBuf}; @@ -525,8 +529,8 @@ fn finalize_temp_file( /// expectation found a file it was never told about, and `AlreadyExists` is the /// answer to its own question. A write carrying one was told the name was free /// and is now finding out that it is not — the caller's expectation broke, the -/// same as every other way this window ends, and only that wording sends an -/// editor round to reload and reapply instead of reporting a failed save. +/// same as every other way this window ends. The typed destination-change result +/// lets an editor reload and reapply instead of reporting a failed save. fn finalize_rename_error( overwrite: OverwriteMode, expected: Option>, diff --git a/src/durable_io/lock.rs b/src/durable_io/lock.rs new file mode 100644 index 000000000..f8dab3f74 --- /dev/null +++ b/src/durable_io/lock.rs @@ -0,0 +1,72 @@ +//! Advisory locks shared by durable file owners. + +use std::fs::File; +use std::io; + +#[cfg(unix)] +use std::os::unix::io::AsRawFd; + +#[cfg(unix)] +fn flock(file: &File, op: libc::c_int) -> io::Result<()> { + let fd = file.as_raw_fd(); + // SAFETY: `fd` stays open for the borrowed `file`, and flock takes no + // pointers or ownership of the descriptor. + let result = unsafe { libc::flock(fd, op) }; + if result == 0 { + Ok(()) + } else { + Err(io::Error::last_os_error()) + } +} + +#[cfg(not(unix))] +fn flock(_file: &File, _op: i32) -> io::Result<()> { + Err(io::Error::new( + io::ErrorKind::Unsupported, + "file locking is not supported on this platform", + )) +} + +pub fn lock_shared(file: &File) -> io::Result<()> { + #[cfg(unix)] + { + flock(file, libc::LOCK_SH) + } + #[cfg(not(unix))] + { + flock(file, 0) + } +} + +pub fn lock_exclusive(file: &File) -> io::Result<()> { + #[cfg(unix)] + { + flock(file, libc::LOCK_EX) + } + #[cfg(not(unix))] + { + flock(file, 0) + } +} + +pub fn try_lock_exclusive(file: &File) -> io::Result<()> { + #[cfg(unix)] + { + flock(file, libc::LOCK_EX | libc::LOCK_NB) + } + #[cfg(not(unix))] + { + flock(file, 0) + } +} + +pub fn unlock(file: &File) -> io::Result<()> { + #[cfg(unix)] + { + flock(file, libc::LOCK_UN) + } + #[cfg(not(unix))] + { + flock(file, 0) + } +} diff --git a/src/image_decode.rs b/src/image_decode.rs index bd7516614..a4bd64872 100644 --- a/src/image_decode.rs +++ b/src/image_decode.rs @@ -261,6 +261,80 @@ mod tests { bytes } + #[test] + fn png_normalization_preserves_color_and_alpha_across_formats() { + use png::{BitDepth, ColorType}; + + let cases: &[(ColorType, BitDepth, &[u8], &[u8])] = &[ + ( + ColorType::Rgb, + BitDepth::Eight, + &[7, 83, 219, 241, 32, 18], + &[7, 83, 219, 255, 241, 32, 18, 255], + ), + ( + ColorType::Rgba, + BitDepth::Eight, + &[7, 83, 219, 0, 241, 32, 18, 127, 2, 9, 74, 255], + &[7, 83, 219, 0, 241, 32, 18, 127, 2, 9, 74, 255], + ), + ( + ColorType::Grayscale, + BitDepth::Eight, + &[17, 203], + &[17, 17, 17, 255, 203, 203, 203, 255], + ), + ( + ColorType::GrayscaleAlpha, + BitDepth::Eight, + &[17, 0, 203, 127, 58, 255], + &[17, 17, 17, 0, 203, 203, 203, 127, 58, 58, 58, 255], + ), + ( + ColorType::Indexed, + BitDepth::Eight, + &[0, 1, 2], + &[7, 83, 219, 0, 241, 32, 18, 127, 2, 9, 74, 255], + ), + ( + ColorType::Rgba, + BitDepth::Sixteen, + &[ + 7, 250, 83, 12, 219, 34, 0, 255, 241, 9, 32, 8, 18, 7, 127, 1, + ], + &[7, 83, 219, 0, 241, 32, 18, 127], + ), + ]; + + for &(color, depth, data, expected) in cases { + let width = (expected.len() / 4) as u32; + let mut bytes = Vec::new(); + let mut encoder = png::Encoder::new(&mut bytes, width, 1); + encoder.set_color(color); + encoder.set_depth(depth); + if color == ColorType::Indexed { + encoder.set_palette(vec![7, 83, 219, 241, 32, 18, 2, 9, 74]); + encoder.set_trns(vec![0, 127, 255]); + } + let mut writer = encoder.write_header().unwrap(); + writer.write_image_data(data).unwrap(); + writer.finish().unwrap(); + + let image = decode_rgba( + EncodedImageFormat::Png, + &bytes, + DecodeLimits { max_pixels: 3 }, + ) + .unwrap(); + assert_eq!( + (image.width, image.height), + (width, 1), + "{color:?}/{depth:?}" + ); + assert_eq!(image.rgba, expected, "{color:?}/{depth:?}"); + } + } + /// A PNG signature and IHDR declaring `width`x`height`, with no image data. fn png_header_only(width: u32, height: u32) -> Vec { let mut bytes = Vec::new(); diff --git a/src/input/state/core/base/types.rs b/src/input/state/core/base/types.rs index 3a94b97b9..9824a86aa 100644 --- a/src/input/state/core/base/types.rs +++ b/src/input/state/core/base/types.rs @@ -256,25 +256,7 @@ pub(crate) struct TextPasteEdit { pub(crate) inserted_len: usize, } -#[cfg_attr(feature = "config-schema", derive(schemars::JsonSchema))] -#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Default)] -#[serde(rename_all = "snake_case")] -pub enum PressureThicknessEditMode { - #[default] - Disabled, - Add, - Scale, -} - -#[cfg_attr(feature = "config-schema", derive(schemars::JsonSchema))] -#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Default)] -#[serde(rename_all = "snake_case")] -pub enum PressureThicknessEntryMode { - Never, - #[default] - PressureOnly, - AnyPressure, -} +pub use crate::domain::{PressureThicknessEditMode, PressureThicknessEntryMode}; #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum SelectionAxis { diff --git a/src/input/state/core/board_picker/search.rs b/src/input/state/core/board_picker/search.rs index 6e855e95b..d20badbdf 100644 --- a/src/input/state/core/board_picker/search.rs +++ b/src/input/state/core/board_picker/search.rs @@ -160,9 +160,6 @@ mod tests { fn make_state() -> InputState { let keybindings = KeybindingsConfig::default(); - let _action_map = keybindings - .build_action_map() - .expect("default keybindings map"); let action_bindings = keybindings .build_action_bindings() .expect("default keybindings bindings"); diff --git a/src/input/state/core/command_palette/mod.rs b/src/input/state/core/command_palette/mod.rs index ab7918154..85bcfa80d 100644 --- a/src/input/state/core/command_palette/mod.rs +++ b/src/input/state/core/command_palette/mod.rs @@ -120,9 +120,6 @@ mod tests { fn make_state() -> InputState { let keybindings = KeybindingsConfig::default(); - let _action_map = keybindings - .build_action_map() - .expect("default keybindings map"); let action_bindings = keybindings .build_action_bindings() .expect("default keybindings bindings"); diff --git a/src/input/state/core/help_overlay/tests.rs b/src/input/state/core/help_overlay/tests.rs index 306fb1848..3ad0c272c 100644 --- a/src/input/state/core/help_overlay/tests.rs +++ b/src/input/state/core/help_overlay/tests.rs @@ -1,15 +1,9 @@ -use crate::config::KeybindingsConfig; use crate::input::state::{ HelpOverlayClick, HelpOverlayCursorHint, HelpOverlayPressSource, HelpOverlayReleaseOutcome, InputState, }; fn make_state() -> InputState { - let keybindings = KeybindingsConfig::default(); - let _action_map = keybindings - .build_action_map() - .expect("default keybindings map"); - crate::input::state::test_support::make_test_input_state() } diff --git a/src/input/state/core/history.rs b/src/input/state/core/history.rs index ff3f96cf5..872200c77 100644 --- a/src/input/state/core/history.rs +++ b/src/input/state/core/history.rs @@ -113,15 +113,9 @@ impl InputState { #[cfg(test)] mod tests { use super::*; - use crate::config::KeybindingsConfig; use crate::draw::{Color, Shape, frame::ShapeSnapshot}; fn make_state() -> InputState { - let keybindings = KeybindingsConfig::default(); - let _action_map = keybindings - .build_action_map() - .expect("default keybindings map"); - let mut state = crate::input::state::test_support::make_test_input_state(); state.update_screen_dimensions(200, 120); let _ = state.take_dirty_regions(); diff --git a/src/input/state/core/properties/apply_selection/actions/arrow.rs b/src/input/state/core/properties/apply_selection/actions/arrow.rs index 42bb15f90..173ea7be3 100644 --- a/src/input/state/core/properties/apply_selection/actions/arrow.rs +++ b/src/input/state/core/properties/apply_selection/actions/arrow.rs @@ -226,14 +226,8 @@ impl InputState { #[cfg(test)] mod tests { use super::*; - use crate::config::KeybindingsConfig; fn make_state() -> InputState { - let keybindings = KeybindingsConfig::default(); - let _action_map = keybindings - .build_action_map() - .expect("default keybindings map"); - crate::input::state::test_support::make_test_input_state() } diff --git a/src/input/state/core/properties/apply_selection/actions/color.rs b/src/input/state/core/properties/apply_selection/actions/color.rs index bc169e3d3..10458fea6 100644 --- a/src/input/state/core/properties/apply_selection/actions/color.rs +++ b/src/input/state/core/properties/apply_selection/actions/color.rs @@ -238,15 +238,9 @@ impl InputState { #[cfg(test)] mod tests { use super::*; - use crate::config::KeybindingsConfig; use crate::draw::RED; fn make_state() -> InputState { - let keybindings = KeybindingsConfig::default(); - let _action_map = keybindings - .build_action_map() - .expect("default keybindings map"); - crate::input::state::test_support::make_test_input_state() } diff --git a/src/input/state/core/properties/apply_selection/actions/fill.rs b/src/input/state/core/properties/apply_selection/actions/fill.rs index 8058b062b..d99e4276d 100644 --- a/src/input/state/core/properties/apply_selection/actions/fill.rs +++ b/src/input/state/core/properties/apply_selection/actions/fill.rs @@ -197,14 +197,8 @@ fn fill_fields(shape: &mut Shape) -> Option<(&mut bool, &mut Option)> { #[cfg(test)] mod tests { use super::*; - use crate::config::KeybindingsConfig; fn make_state() -> InputState { - let keybindings = KeybindingsConfig::default(); - let _action_map = keybindings - .build_action_map() - .expect("default keybindings map"); - crate::input::state::test_support::make_test_input_state() } diff --git a/src/input/state/core/properties/apply_selection/actions/text.rs b/src/input/state/core/properties/apply_selection/actions/text.rs index e29c74b24..59bb4746d 100644 --- a/src/input/state/core/properties/apply_selection/actions/text.rs +++ b/src/input/state/core/properties/apply_selection/actions/text.rs @@ -79,14 +79,8 @@ impl InputState { #[cfg(test)] mod tests { use super::*; - use crate::config::KeybindingsConfig; fn make_state() -> InputState { - let keybindings = KeybindingsConfig::default(); - let _action_map = keybindings - .build_action_map() - .expect("default keybindings map"); - crate::input::state::test_support::make_test_input_state() } diff --git a/src/input/state/core/properties/apply_selection/helpers/tests.rs b/src/input/state/core/properties/apply_selection/helpers/tests.rs index f0457eec4..2c98edd9c 100644 --- a/src/input/state/core/properties/apply_selection/helpers/tests.rs +++ b/src/input/state/core/properties/apply_selection/helpers/tests.rs @@ -1,13 +1,7 @@ use super::*; -use crate::config::KeybindingsConfig; use crate::draw::Color; fn make_state() -> InputState { - let keybindings = KeybindingsConfig::default(); - let _action_map = keybindings - .build_action_map() - .expect("default keybindings map"); - let mut state = crate::input::state::test_support::make_test_input_state(); state.update_screen_dimensions(200, 120); let _ = state.take_dirty_regions(); diff --git a/src/input/state/core/properties/entries.rs b/src/input/state/core/properties/entries.rs index d49ddd493..b1a3d0db3 100644 --- a/src/input/state/core/properties/entries.rs +++ b/src/input/state/core/properties/entries.rs @@ -1,12 +1,13 @@ use super::super::base::InputState; use super::summary::{ - PropertySummary, shape_arrow_angle, shape_arrow_head, shape_arrow_length, shape_arrow_style, - shape_color, shape_fill_paint, shape_font_size, shape_opacity, shape_spotlight_magnification, - shape_text_background, shape_thickness, summarize_property, + PropertySummary, resolve_selected_shapes, shape_arrow_angle, shape_arrow_head, + shape_arrow_length, shape_arrow_style, shape_color, shape_fill_paint, shape_font_size, + shape_opacity, shape_spotlight_magnification, shape_text_background, shape_thickness, + summarize_property, }; use super::types::{SelectionPropertyEntry, SelectionPropertyKind, SelectionPropertyValue}; use super::utils::{approx_eq, color_label, color_rgba_eq}; -use crate::draw::{Shape, ShapeId}; +use crate::draw::{DrawnShape, Shape, ShapeId}; use crate::input::state::{PressureThicknessEditMode, PressureThicknessEntryMode}; /// Renders one summary the way every popup row shows it: a locked row reads @@ -61,7 +62,12 @@ impl InputState { /// Whether some selected shape has a color that is not locked. pub(crate) fn selection_has_editable_color(&self) -> bool { let frame = self.boards.active_frame(); - summarize_property(frame, self.selected_shape_ids(), shape_color, color_rgba_eq).editable + summarize_property( + &resolve_selected_shapes(frame, self.selected_shape_ids()), + shape_color, + color_rgba_eq, + ) + .editable } pub(super) fn build_selection_property_entries( @@ -69,11 +75,20 @@ impl InputState { ids: &[ShapeId], ) -> Vec { let frame = self.boards.active_frame(); + let selected = resolve_selected_shapes(frame, ids); + self.build_selection_property_entries_from_resolved(ids, &selected) + } + + pub(crate) fn build_selection_property_entries_from_resolved( + &self, + ids: &[ShapeId], + selected: &[&DrawnShape], + ) -> Vec { let palette = self.style.quick_colors.rendered_entries(); let mut entries = Vec::new(); // Opacity counts: two reds at different opacities are a mixed color. - let color_summary = summarize_property(frame, ids, shape_color, color_rgba_eq); + let color_summary = summarize_property(selected, shape_color, color_rgba_eq); if color_summary.applicable { entries.push(entry( "Color", @@ -84,7 +99,7 @@ impl InputState { )); } - let thickness_summary = summarize_property(frame, ids, shape_thickness, approx_eq); + let thickness_summary = summarize_property(selected, shape_thickness, approx_eq); if thickness_summary.applicable { entries.push(entry( "Thickness", @@ -95,13 +110,9 @@ impl InputState { )); } else { let mut any_pressure = false; - let mut all_pressure = !ids.is_empty(); + let mut all_pressure = !ids.is_empty() && selected.len() == ids.len(); let mut any_pressure_editable = false; - for id in ids { - let Some(drawn) = frame.shape(*id) else { - all_pressure = false; - continue; - }; + for drawn in selected { if matches!(&drawn.shape, Shape::FreehandPressure { .. }) { any_pressure = true; if !drawn.locked { @@ -135,7 +146,7 @@ impl InputState { } } - let opacity_summary = summarize_property(frame, ids, shape_opacity, approx_eq); + let opacity_summary = summarize_property(selected, shape_opacity, approx_eq); if opacity_summary.applicable { entries.push(entry( "Opacity", @@ -146,7 +157,7 @@ impl InputState { )); } - let fill_summary = summarize_property(frame, ids, shape_fill_paint, |a, b| match (a, b) { + let fill_summary = summarize_property(selected, shape_fill_paint, |a, b| match (a, b) { (Some(a), Some(b)) => color_rgba_eq(a, b), (a, b) => a.is_none() && b.is_none(), }); @@ -162,7 +173,7 @@ impl InputState { )); } - let font_summary = summarize_property(frame, ids, shape_font_size, approx_eq); + let font_summary = summarize_property(selected, shape_font_size, approx_eq); if font_summary.applicable { entries.push(entry( "Font size", @@ -173,7 +184,7 @@ impl InputState { )); } - let head_summary = summarize_property(frame, ids, shape_arrow_head, |a, b| a == b); + let head_summary = summarize_property(selected, shape_arrow_head, |a, b| a == b); if head_summary.applicable { entries.push(entry( "Arrow head", @@ -184,7 +195,7 @@ impl InputState { )); } - let style_summary = summarize_property(frame, ids, shape_arrow_style, |a, b| a == b); + let style_summary = summarize_property(selected, shape_arrow_style, |a, b| a == b); if style_summary.applicable { entries.push(entry( "Arrow style", @@ -195,7 +206,7 @@ impl InputState { )); } - let length_summary = summarize_property(frame, ids, shape_arrow_length, approx_eq); + let length_summary = summarize_property(selected, shape_arrow_length, approx_eq); if length_summary.applicable { entries.push(entry( "Arrow length", @@ -206,7 +217,7 @@ impl InputState { )); } - let angle_summary = summarize_property(frame, ids, shape_arrow_angle, approx_eq); + let angle_summary = summarize_property(selected, shape_arrow_angle, approx_eq); if angle_summary.applicable { entries.push(entry( "Arrow angle", @@ -217,7 +228,7 @@ impl InputState { )); } - let text_bg_summary = summarize_property(frame, ids, shape_text_background, |a, b| a == b); + let text_bg_summary = summarize_property(selected, shape_text_background, |a, b| a == b); if text_bg_summary.applicable { entries.push(entry( "Text background", @@ -229,7 +240,7 @@ impl InputState { } let spotlight_summary = - summarize_property(frame, ids, shape_spotlight_magnification, approx_eq); + summarize_property(selected, shape_spotlight_magnification, approx_eq); if spotlight_summary.applicable { entries.push(entry( "Magnification", @@ -264,6 +275,36 @@ mod tests { .expect(label) } + #[test] + fn selection_pill_resolves_sparse_and_all_selected_frames_without_id_rescans() { + let mut state = make_state(); + let ids: Vec<_> = (0..2_048) + .map(|index| { + state.boards.active_frame_mut().add_shape(Shape::Rect { + x: index * 10, + y: 0, + w: 10, + h: 10, + fill: false, + fill_color: None, + color: PALETTE_RED, + thick: 3.0, + }) + }) + .collect(); + + for selected in [vec![ids[2], ids[1_000], ids[2_047]], ids] { + state.set_selection(selected); + crate::draw::Frame::reset_linear_id_lookup_count(); + let entries = state.selection_pill_entries(); + + assert_eq!(entry(&entries, "Thickness").value, "3.0px"); + assert_eq!(entry(&entries, "Opacity").value, "100%"); + assert!(!entry(&entries, "Color").disabled); + assert_eq!(crate::draw::Frame::linear_id_lookup_count(), 0); + } + } + #[test] fn property_entries_report_mixed_color_for_different_rectangles() { let mut state = make_state(); diff --git a/src/input/state/core/properties/panel.rs b/src/input/state/core/properties/panel.rs index b1c4954fb..b487f11a1 100644 --- a/src/input/state/core/properties/panel.rs +++ b/src/input/state/core/properties/panel.rs @@ -289,15 +289,9 @@ impl InputState { #[cfg(test)] mod tests { use super::*; - use crate::config::KeybindingsConfig; use crate::draw::{Shape, ShapeId}; fn make_state() -> InputState { - let keybindings = KeybindingsConfig::default(); - let _action_map = keybindings - .build_action_map() - .expect("default keybindings map"); - crate::input::state::test_support::make_test_input_state() } diff --git a/src/input/state/core/properties/panel_layout/focus.rs b/src/input/state/core/properties/panel_layout/focus.rs index 31b060ad4..b80c062fa 100644 --- a/src/input/state/core/properties/panel_layout/focus.rs +++ b/src/input/state/core/properties/panel_layout/focus.rs @@ -118,15 +118,9 @@ impl InputState { mod tests { use super::super::super::types::PropertiesPanelHit; use super::*; - use crate::config::KeybindingsConfig; use crate::draw::Shape; fn make_state() -> InputState { - let keybindings = KeybindingsConfig::default(); - let _action_map = keybindings - .build_action_map() - .expect("default keybindings map"); - crate::input::state::test_support::make_test_input_state() } diff --git a/src/input/state/core/properties/summary.rs b/src/input/state/core/properties/summary.rs index 0511bfbe3..1dbf56c3f 100644 --- a/src/input/state/core/properties/summary.rs +++ b/src/input/state/core/properties/summary.rs @@ -1,4 +1,27 @@ -use crate::draw::{ArrowStyle, Color, Frame, Shape, ShapeId}; +use std::collections::HashMap; + +use crate::draw::{ArrowStyle, Color, DrawnShape, Frame, Shape, ShapeId}; + +#[cfg(test)] +thread_local! { + static SELECTION_RESOLUTIONS: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + +impl super::super::InputState { + pub(crate) fn resolved_selected_shapes(&self) -> Vec<&DrawnShape> { + resolve_selected_shapes(self.boards.active_frame(), self.selected_shape_ids()) + } + + #[cfg(test)] + pub(crate) fn reset_selection_resolution_count() { + SELECTION_RESOLUTIONS.with(|count| count.set(0)); + } + + #[cfg(test)] + pub(crate) fn selection_resolution_count() -> usize { + SELECTION_RESOLUTIONS.with(std::cell::Cell::get) + } +} #[derive(Debug)] pub(super) struct PropertySummary { @@ -8,59 +31,81 @@ pub(super) struct PropertySummary { pub(super) value: Option, } -pub(super) fn summarize_property( - frame: &Frame, +/// Resolve IDs with one frame pass while retaining selection order. +/// Missing IDs are omitted; callers can compare lengths when absence matters. +pub(super) fn resolve_selected_shapes<'a>( + frame: &'a Frame, ids: &[ShapeId], +) -> Vec<&'a DrawnShape> { + #[cfg(test)] + SELECTION_RESOLUTIONS.with(|count| count.set(count.get() + 1)); + + if ids.is_empty() { + return Vec::new(); + } + + // Tiny selections need only a few comparisons per shape. Avoid hashing the + // whole frame for the common one-shape selection; the bound stays constant. + if ids.len() <= 4 { + let mut selected = vec![None; ids.len()]; + let mut remaining = ids.len(); + for drawn in &frame.shapes { + for (id, slot) in ids.iter().zip(&mut selected) { + if *id == drawn.id { + if slot.is_none() { + remaining -= 1; + } + *slot = Some(drawn); + } + } + if remaining == 0 { + break; + } + } + return selected.into_iter().flatten().collect(); + } + + let mut selected: HashMap<_, Option<&DrawnShape>> = ids.iter().map(|id| (*id, None)).collect(); + for drawn in &frame.shapes { + if let Some(slot) = selected.get_mut(&drawn.id) { + *slot = Some(drawn); + } + } + + ids.iter().filter_map(|id| selected[id]).collect() +} + +pub(super) fn summarize_property( + selected: &[&DrawnShape], mut extract: F, mut eq: Eq, ) -> PropertySummary where - T: Clone, F: FnMut(&Shape) -> Option, Eq: FnMut(&T, &T) -> bool, { - let mut values = Vec::new(); - let mut applicable = 0; - for id in ids { - let Some(drawn) = frame.shape(*id) else { - continue; - }; + let mut applicable = false; + let mut first = None; + let mut mixed = false; + for drawn in selected { let Some(value) = extract(&drawn.shape) else { continue; }; - applicable += 1; + applicable = true; if drawn.locked { continue; } - values.push(value); - } - - if applicable == 0 { - return PropertySummary { - applicable: false, - editable: false, - mixed: false, - value: None, - }; - } - - if values.is_empty() { - return PropertySummary { - applicable: true, - editable: false, - mixed: false, - value: None, - }; + match &first { + Some(first) => mixed |= !eq(first, &value), + None => first = Some(value), + } } - let first = values[0].clone(); - let mixed = values.iter().skip(1).any(|value| !eq(&first, value)); - PropertySummary { - applicable: true, - editable: true, + applicable, + editable: first.is_some(), mixed, - value: Some(first), + value: first, } } @@ -221,7 +266,11 @@ mod tests { wrap_width: None, }); - let summary = summarize_property(&frame, &[text_id], shape_fill_paint, |a, b| a == b); + let summary = summarize_property( + &resolve_selected_shapes(&frame, &[text_id]), + shape_fill_paint, + |a, b| a == b, + ); assert!(!summary.applicable); assert!(!summary.editable); @@ -244,7 +293,11 @@ mod tests { )); frame.shape_mut(id).expect("locked shape").locked = true; - let summary = summarize_property(&frame, &[id], shape_color, color_eq); + let summary = summarize_property( + &resolve_selected_shapes(&frame, &[id]), + shape_color, + color_eq, + ); assert!(summary.applicable); assert!(!summary.editable); @@ -276,7 +329,11 @@ mod tests { 2.0, )); - let summary = summarize_property(&frame, &[first, second], shape_color, color_eq); + let summary = summarize_property( + &resolve_selected_shapes(&frame, &[first, second]), + shape_color, + color_eq, + ); assert!(summary.applicable); assert!(summary.editable); @@ -317,7 +374,11 @@ mod tests { )); frame.shape_mut(locked).expect("locked shape").locked = true; - let summary = summarize_property(&frame, &[unlocked, locked], shape_color, color_eq); + let summary = summarize_property( + &resolve_selected_shapes(&frame, &[unlocked, locked]), + shape_color, + color_eq, + ); assert!(summary.applicable); assert!(summary.editable); diff --git a/src/input/state/core/text_font.rs b/src/input/state/core/text_font.rs index 33d101072..cb0994e9a 100644 --- a/src/input/state/core/text_font.rs +++ b/src/input/state/core/text_font.rs @@ -8,7 +8,7 @@ use super::InputState; use crate::draw::TextMeasurer; -use crate::draw::{FontDescriptor, Shape, families_match}; +use crate::draw::{DrawnShape, FontDescriptor, Shape, families_match}; fn text_font_descriptor(shape: &Shape) -> Option<&FontDescriptor> { match shape { @@ -37,13 +37,13 @@ fn text_font_descriptor_mut(shape: &mut Shape) -> Option<&mut FontDescriptor> { impl InputState { /// Whether the selection holds anything a font applies to. pub(crate) fn selection_has_text(&self) -> bool { - let frame = self.boards.active_frame(); - self.selected_shape_ids().iter().any(|id| { - frame - .shape(*id) - .and_then(|drawn| text_font_descriptor(&drawn.shape)) - .is_some() - }) + Self::resolved_selection_has_text(&self.resolved_selected_shapes()) + } + + pub(crate) fn resolved_selection_has_text(selected: &[&DrawnShape]) -> bool { + selected + .iter() + .any(|drawn| text_font_descriptor(&drawn.shape).is_some()) } fn first_selected_text_descriptor(&self) -> Option<&FontDescriptor> { @@ -55,19 +55,6 @@ impl InputState { }) } - fn first_editable_selected_text_descriptor(&self) -> Option<&FontDescriptor> { - let frame = self.boards.active_frame(); - self.selected_shape_ids().iter().find_map(|id| { - frame.shape(*id).and_then(|drawn| { - if drawn.locked { - None - } else { - text_font_descriptor(&drawn.shape) - } - }) - }) - } - /// The family of the first selected text shape, if any. /// /// One shape decides for the whole selection. A mixed selection then @@ -82,9 +69,14 @@ impl InputState { /// /// A mixed selection converges when the user clicks rather than borrowing /// state from either a locked shape or the unrelated tool default. - pub(crate) fn first_editable_selected_text_is_bold(&self) -> Option { - self.first_editable_selected_text_descriptor() - .map(FontDescriptor::is_bold) + pub(crate) fn resolved_selected_text_is_bold(selected: &[&DrawnShape]) -> Option { + selected.iter().find_map(|drawn| { + if drawn.locked { + None + } else { + text_font_descriptor(&drawn.shape).map(FontDescriptor::is_bold) + } + }) } /// Turn bold on or off, on selected text when there is any and on the tool diff --git a/src/input/state/core/toolbar/apply/delays.rs b/src/input/state/core/toolbar/apply/delays.rs index 5f758a1ff..54eed1c2b 100644 --- a/src/input/state/core/toolbar/apply/delays.rs +++ b/src/input/state/core/toolbar/apply/delays.rs @@ -33,14 +33,8 @@ impl InputState { #[cfg(test)] mod tests { use super::*; - use crate::config::KeybindingsConfig; fn make_state() -> InputState { - let keybindings = KeybindingsConfig::default(); - let _action_map = keybindings - .build_action_map() - .expect("default keybindings map"); - crate::input::state::test_support::make_test_input_state() } diff --git a/src/input/state/core/utility/frozen_zoom.rs b/src/input/state/core/utility/frozen_zoom.rs index c88bab3e0..5bba99b8e 100644 --- a/src/input/state/core/utility/frozen_zoom.rs +++ b/src/input/state/core/utility/frozen_zoom.rs @@ -58,14 +58,8 @@ impl InputState { #[cfg(test)] mod tests { use super::*; - use crate::config::KeybindingsConfig; fn make_state() -> InputState { - let keybindings = KeybindingsConfig::default(); - let _action_map = keybindings - .build_action_map() - .expect("default keybindings map"); - crate::input::state::test_support::make_test_input_state() } diff --git a/src/input/state/core/utility/pending.rs b/src/input/state/core/utility/pending.rs index f484187cf..1da80f872 100644 --- a/src/input/state/core/utility/pending.rs +++ b/src/input/state/core/utility/pending.rs @@ -337,17 +337,12 @@ impl InputState { #[cfg(test)] mod tests { use super::*; - use crate::config::{Action, KeybindingsConfig}; + use crate::config::Action; use crate::draw::{BLACK, WHITE}; use crate::input::state::KeybindingEditOperation; use crate::input::state::core::base::{InputEffect, InputEffectDrain}; fn make_state() -> InputState { - let keybindings = KeybindingsConfig::default(); - let _action_map = keybindings - .build_action_map() - .expect("default keybindings map"); - crate::input::state::test_support::make_test_input_state() } @@ -457,18 +452,6 @@ mod tests { assert_eq!(state.take_pending_zoom_action(), None); } - #[test] - fn pending_preset_action_is_taken_once() { - let mut state = make_state(); - state.emit_input_effect(InputEffect::Preset(PresetAction::Clear { slot: 2 })); - - assert!(matches!( - state.take_pending_preset_action(), - Some(PresetAction::Clear { slot: 2 }) - )); - assert!(state.take_pending_preset_action().is_none()); - } - #[test] fn runtime_effect_drain_orders_region_capture_before_freeze_without_dropping_other_work() { let mut state = make_state(); diff --git a/src/input/state/core/utility/toasts.rs b/src/input/state/core/utility/toasts.rs index 6729b9931..313c4b791 100644 --- a/src/input/state/core/utility/toasts.rs +++ b/src/input/state/core/utility/toasts.rs @@ -122,11 +122,6 @@ impl InputState { result } - #[cfg(test)] - pub(crate) fn toast_contains(&self, x: i32, y: i32) -> bool { - self.feedback.contains(x, y) - } - pub(crate) fn note_capability_toast(&mut self, caps: CompositorCapabilities) -> Option { self.feedback.note_capability_toast(caps) } @@ -237,7 +232,6 @@ impl InputState { #[cfg(test)] mod tests { use super::*; - use crate::config::KeybindingsConfig; use crate::domain::OnboardingTip; use crate::draw::{Color, Shape}; use crate::input::state::core::base::UiToastKind; @@ -249,11 +243,6 @@ mod tests { use std::time::Duration; fn make_state() -> InputState { - let keybindings = KeybindingsConfig::default(); - let _action_map = keybindings - .build_action_map() - .expect("default keybindings map"); - crate::input::state::test_support::make_test_input_state() } @@ -533,12 +522,12 @@ mod tests { } #[test] - fn toast_contains_reports_hit_without_dismissing() { + fn toast_press_reports_hit_without_dismissing() { let mut state = make_state(); state.push_toast(ToastPriority::Info, "test", Toast::info("Saved")); state.set_toast_geometry(Some((10.0, 20.0, 100.0, 40.0)), [None, None]); - assert!(state.toast_contains(50, 40)); + assert!(state.toast_press_at(50, 40).is_some()); assert!(state.active_toast().is_some()); assert!(state.test_toast_geometry().is_some()); } @@ -560,7 +549,7 @@ mod tests { let toast = state.active_toast().expect("preempting toast visible"); assert_eq!(toast.message, "Delete page?"); assert!(state.test_toast_geometry().is_none()); - assert!(!state.toast_contains(50, 40)); + assert!(state.toast_press_at(50, 40).is_none()); let stale_press = ToastPress::body(0); assert_eq!( state.resolve_toast_release(stale_press, 50, 40), @@ -755,51 +744,6 @@ mod tests { assert!(state.test_blocked_feedback_active()); } - /// Producer-migration completeness: every toast producer goes through - /// `push_toast(priority, key, toast)`. The legacy `set_ui_toast*` shims - /// have been removed; no module may reintroduce them. - #[test] - fn all_toast_producers_use_the_priority_queue_api() { - let src_root = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("src"); - let allowlist = [ - // This file names the retired shims in the assertion string below. - "input/state/core/utility/toasts.rs", - ]; - - let mut offenders = Vec::new(); - let mut stack = vec![src_root.clone()]; - while let Some(dir) = stack.pop() { - for entry in std::fs::read_dir(&dir).expect("read src dir") { - let entry = entry.expect("dir entry"); - let path = entry.path(); - if path.is_dir() { - stack.push(path); - continue; - } - if path.extension().and_then(|e| e.to_str()) != Some("rs") { - continue; - } - let rel = path - .strip_prefix(&src_root) - .expect("path under src") - .to_string_lossy() - .replace('\\', "/"); - if allowlist.contains(&rel.as_str()) { - continue; - } - let contents = std::fs::read_to_string(&path).expect("read source file"); - if contents.contains(".set_ui_toast") { - offenders.push(rel); - } - } - } - - assert!( - offenders.is_empty(), - "files still using legacy set_ui_toast* instead of push_toast: {offenders:?}" - ); - } - #[test] fn advance_text_edit_entry_feedback_clears_expired_feedback() { let mut state = make_state(); diff --git a/src/input/state/spotlight.rs b/src/input/state/spotlight.rs index 0916cdf03..8a8d64d4e 100644 --- a/src/input/state/spotlight.rs +++ b/src/input/state/spotlight.rs @@ -9,6 +9,8 @@ use crate::input::Tool; use super::{DrawingState, InputState}; +mod selection; + /// Every spotlight one frame must dim, collected in a single pass. pub(crate) struct SpotlightFrameRegions { /// Committed regions first, then the in-progress drag when there is one. @@ -159,6 +161,25 @@ fn snapshot_magnification(snapshot: &crate::draw::frame::ShapeSnapshot) -> Optio } impl InputState { + /// Whether anything on the active page dims the canvas. + /// + /// Drives the full-damage decision: a spotlight changes every pixel outside + /// itself, so partial damage cannot describe adding, moving, or removing one. + pub(crate) fn has_spotlight(&self) -> bool { + self.boards + .active_frame() + .shapes + .iter() + .any(|drawn| matches!(drawn.shape, Shape::Spotlight { .. })) + || matches!( + &self.state, + DrawingState::Drawing { + tool: Tool::Spotlight, + .. + } + ) + } + /// Whether a live wheel burst still owes its single undo entry. pub(crate) fn has_pending_spotlight_magnification_gesture(&self) -> bool { self.spotlight_wheel.is_pending() @@ -500,44 +521,6 @@ impl InputState { ), }) } - - /// Highest magnification among the currently selected Spotlights. - /// - /// `None` when the selection holds no Spotlight at all. The docked - /// selection control reports availability against this rather than the - /// next-shape default, which is a different number whenever the user - /// selects an existing shape. - pub fn selection_spotlight_magnification(&self) -> Option { - let frame = self.boards.active_frame(); - self.selected_shape_ids() - .iter() - .filter_map(|id| match frame.shape(*id)?.shape { - Shape::Spotlight { magnification, .. } => Some( - crate::draw::normalize_spotlight_magnification(magnification), - ), - _ => None, - }) - .reduce(f64::max) - } - - /// Whether anything on the active page dims the canvas. - /// - /// Drives the full-damage decision: a spotlight changes every pixel outside - /// itself, so partial damage cannot describe adding, moving, or removing one. - pub(crate) fn has_spotlight(&self) -> bool { - self.boards - .active_frame() - .shapes - .iter() - .any(|drawn| matches!(drawn.shape, Shape::Spotlight { .. })) - || matches!( - &self.state, - DrawingState::Drawing { - tool: Tool::Spotlight, - .. - } - ) - } } #[cfg(test)] diff --git a/src/input/state/spotlight/selection.rs b/src/input/state/spotlight/selection.rs new file mode 100644 index 000000000..bbf8d8df0 --- /dev/null +++ b/src/input/state/spotlight/selection.rs @@ -0,0 +1,29 @@ +//! Spotlight queries for selection controls. +use super::InputState; +use crate::draw::{DrawnShape, Shape}; + +impl InputState { + /// Highest normalized magnification among selected Spotlights, if any. + /// + /// `None` when the selection holds no Spotlight at all. The selection + /// control reports availability against this rather than the next-shape + /// default, which differs when an existing shape is selected. + pub fn selection_spotlight_magnification(&self) -> Option { + Self::resolved_selection_spotlight_magnification(&self.resolved_selected_shapes()) + } + + /// Same query over a selection already resolved by the caller. + pub(crate) fn resolved_selection_spotlight_magnification( + selected: &[&DrawnShape], + ) -> Option { + selected + .iter() + .filter_map(|drawn| match drawn.shape { + Shape::Spotlight { magnification, .. } => Some( + crate::draw::normalize_spotlight_magnification(magnification), + ), + _ => None, + }) + .reduce(f64::max) + } +} diff --git a/src/input/state/tests/properties_panel_controls.rs b/src/input/state/tests/properties_panel_controls.rs index 3388db648..e5bcfe1b5 100644 --- a/src/input/state/tests/properties_panel_controls.rs +++ b/src/input/state/tests/properties_panel_controls.rs @@ -1162,8 +1162,9 @@ fn a_selection_saves_its_style_into_a_preset_slot_and_another_takes_it_on() { assert_eq!(saved.size, 8.0); assert_eq!(saved.fill_enabled, Some(true)); assert!(matches!( - state.take_pending_preset_action(), - Some(crate::input::state::PresetAction::Save { slot, .. }) if slot == empty_slot + state.drain_input_effects(crate::input::state::InputEffectDrain::Runtime) + .into_iter().filter(|effect| matches!(effect, crate::input::state::InputEffect::Preset(_))).collect::>().as_slice(), + [crate::input::state::InputEffect::Preset(crate::input::state::PresetAction::Save { slot, .. })] if *slot == empty_slot )); let tool_before = state.active_tool(); diff --git a/src/input/state/tests/tool_controls.rs b/src/input/state/tests/tool_controls.rs index cc38d354c..f82297724 100644 --- a/src/input/state/tests/tool_controls.rs +++ b/src/input/state/tests/tool_controls.rs @@ -5,6 +5,18 @@ use crate::input::{DragBinding, DragToolBindings, PerToolDrawingSettings}; use crate::ui::toolbar::model::{StylePillControl, StylePillSpec, TopStripPlan}; use crate::ui::toolbar::{ToolContext, ToolOptionsKind, ToolbarEvent, ToolbarSnapshot}; +/// Pointer releases publish config edits through the live runtime inventory. +fn runtime_quick_color_edits(state: &mut InputState) -> Vec { + state + .drain_input_effects(crate::input::state::InputEffectDrain::Runtime) + .into_iter() + .filter_map(|effect| match effect { + crate::input::state::InputEffect::QuickColor(edit) => Some(edit), + _ => None, + }) + .collect() +} + #[test] fn set_tool_override_clears_active_preset_and_resets_drawing_state() { let mut state = create_test_input_state(); @@ -783,11 +795,11 @@ fn accepting_a_recolor_keeps_the_swatch_and_queues_the_durable_write() { assert!(!state.is_color_picker_popup_open()); assert_eq!(state.style.quick_colors.color_for_index(2), Some(picked)); assert_eq!( - state.take_pending_quick_color_edit(), - Some(crate::input::state::QuickColorEdit { + runtime_quick_color_edits(&mut state), + vec![crate::input::state::QuickColorEdit { index: 2, color: picked - }) + }] ); assert!( state.active_toast().is_none(), @@ -977,11 +989,11 @@ fn default_button_stages_the_shipped_color_for_ok_to_accept() { state.apply_color_picker_popup(); assert_eq!(state.style.quick_colors.color_for_index(1), Some(shipped)); assert_eq!( - state.take_pending_quick_color_edit(), - Some(crate::input::state::QuickColorEdit { + runtime_quick_color_edits(&mut state), + vec![crate::input::state::QuickColorEdit { index: 1, color: shipped - }) + }] ); } @@ -1034,11 +1046,11 @@ fn accepting_a_recolor_from_the_popup_release_queues_the_durable_write() { assert!(!state.is_color_picker_popup_open()); assert_eq!(state.style.quick_colors.color_for_index(0), Some(picked)); assert_eq!( - state.take_pending_quick_color_edit(), - Some(crate::input::state::QuickColorEdit { + runtime_quick_color_edits(&mut state), + vec![crate::input::state::QuickColorEdit { index: 0, color: picked - }) + }] ); } @@ -2355,11 +2367,11 @@ fn a_quick_color_recolor_queues_the_write_without_touching_the_file_itself() { assert_eq!(state.style.quick_colors.color_for_index(0), Some(picked)); assert_eq!( - state.take_pending_quick_color_edit(), - Some(crate::input::state::QuickColorEdit { + runtime_quick_color_edits(&mut state), + vec![crate::input::state::QuickColorEdit { index: 0, color: picked - }) + }] ); snapshot.assert_unchanged("accepting a quick-color recolor in InputState"); diff --git a/src/input/tablet/mod.rs b/src/input/tablet/mod.rs index eee17c73a..383e96afd 100644 --- a/src/input/tablet/mod.rs +++ b/src/input/tablet/mod.rs @@ -69,16 +69,10 @@ pub(crate) fn try_apply_pressure_to_state_with( #[cfg(test)] mod tests { use super::*; - use crate::config::KeybindingsConfig; use crate::draw::Shape; use crate::input::{DrawingState, MouseButton, Tool}; fn make_state() -> InputState { - let keybindings = KeybindingsConfig::default(); - let _action_map = keybindings - .build_action_map() - .expect("default keybindings map"); - crate::input::state::test_support::make_test_input_state() } diff --git a/src/input/tool/mod.rs b/src/input/tool/mod.rs index 4c7d49163..fcafe1601 100644 --- a/src/input/tool/mod.rs +++ b/src/input/tool/mod.rs @@ -3,6 +3,7 @@ mod catalog; mod drawing; mod live_shape; +mod presets; mod profile; mod settings; diff --git a/src/input/tool/presets.rs b/src/input/tool/presets.rs new file mode 100644 index 000000000..f1bd34eff --- /dev/null +++ b/src/input/tool/presets.rs @@ -0,0 +1,97 @@ +//! Adapters between persisted preset values and live per-tool drawing settings. + +use super::{PerToolDrawingSettings, ToolDrawingSettings, ToolSettingsSlot, ToolSizeSource}; +use crate::config::{ColorSpec, PresetToolSettingConfig, PresetToolStatesConfig}; +use crate::domain::Tool; + +impl PresetToolSettingConfig { + pub fn from_runtime(settings: ToolDrawingSettings) -> Self { + Self { + color: settings.color.into(), + size: settings.thickness, + } + } + + pub fn to_runtime(&self) -> ToolDrawingSettings { + ToolDrawingSettings::new(self.color.to_color(), self.size) + } +} + +impl PresetToolStatesConfig { + pub fn from_runtime(settings: &PerToolDrawingSettings, eraser_size: f64) -> Self { + Self { + pen: PresetToolSettingConfig::from_runtime(settings.pen), + line: PresetToolSettingConfig::from_runtime(settings.line), + rect: PresetToolSettingConfig::from_runtime(settings.rect), + ellipse: PresetToolSettingConfig::from_runtime(settings.ellipse), + arrow: PresetToolSettingConfig::from_runtime(settings.arrow), + blur: PresetToolSettingConfig::from_runtime(settings.blur), + marker: PresetToolSettingConfig::from_runtime(settings.marker), + step_marker: PresetToolSettingConfig::from_runtime(settings.step_marker), + eraser_size, + } + } + + pub fn to_runtime(&self) -> PerToolDrawingSettings { + PerToolDrawingSettings { + pen: self.pen.to_runtime(), + line: self.line.to_runtime(), + rect: self.rect.to_runtime(), + ellipse: self.ellipse.to_runtime(), + arrow: self.arrow.to_runtime(), + blur: self.blur.to_runtime(), + marker: self.marker.to_runtime(), + step_marker: self.step_marker.to_runtime(), + } + } + + pub fn color_spec_for_tool(&self, tool: Tool) -> ColorSpec { + self.setting_for_slot(tool.settings_slot()).color.clone() + } + + pub fn size_for_tool(&self, tool: Tool) -> f64 { + let profile = tool.profile(); + match profile.size_source { + ToolSizeSource::EraserSize => self.eraser_size, + ToolSizeSource::DrawingThickness => self.setting_for_slot(profile.settings_slot).size, + } + } + + #[allow(dead_code)] + pub fn set_preview_tool(&mut self, tool: Tool, color: ColorSpec, size: f64) { + let profile = tool.profile(); + self.setting_for_slot_mut(profile.settings_slot).color = color; + match profile.size_source { + ToolSizeSource::EraserSize => self.eraser_size = size, + ToolSizeSource::DrawingThickness => { + self.setting_for_slot_mut(profile.settings_slot).size = size; + } + } + } + + fn setting_for_slot(&self, slot: ToolSettingsSlot) -> &PresetToolSettingConfig { + match slot { + ToolSettingsSlot::Pen => &self.pen, + ToolSettingsSlot::Line => &self.line, + ToolSettingsSlot::Rect => &self.rect, + ToolSettingsSlot::Ellipse => &self.ellipse, + ToolSettingsSlot::Arrow => &self.arrow, + ToolSettingsSlot::Blur => &self.blur, + ToolSettingsSlot::Marker => &self.marker, + ToolSettingsSlot::StepMarker => &self.step_marker, + } + } + + fn setting_for_slot_mut(&mut self, slot: ToolSettingsSlot) -> &mut PresetToolSettingConfig { + match slot { + ToolSettingsSlot::Pen => &mut self.pen, + ToolSettingsSlot::Line => &mut self.line, + ToolSettingsSlot::Rect => &mut self.rect, + ToolSettingsSlot::Ellipse => &mut self.ellipse, + ToolSettingsSlot::Arrow => &mut self.arrow, + ToolSettingsSlot::Blur => &mut self.blur, + ToolSettingsSlot::Marker => &mut self.marker, + ToolSettingsSlot::StepMarker => &mut self.step_marker, + } + } +} diff --git a/src/lib.rs b/src/lib.rs index dc9a947e0..0d86668d9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -36,9 +36,7 @@ mod process_broker; pub mod render_profiles; pub mod runtime_capabilities; pub(crate) mod screen_pixels; -// Phases 2-3 establish the controller and storage contracts before later -// phases route live UI producers through them. -#[allow(dead_code)] +// Runtime UI controllers coordinate the live toolbar and other UI operations. pub(crate) mod runtime_ui_state; pub mod session; mod session_override; diff --git a/src/process_broker/client.rs b/src/process_broker/client.rs index 77dc9c8cd..a73d1d0f9 100644 --- a/src/process_broker/client.rs +++ b/src/process_broker/client.rs @@ -7,6 +7,7 @@ use std::time::Duration; use anyhow::{Context, Result, anyhow, bail}; +use super::error::{BrokerError, BrokerErrorKind}; use super::execution::supports_retained_publication; use super::transport::{ GRACEFUL_SHUTDOWN_BYTE, decode_blob, encode_blob, recv_packet, send_packet, set_socket_timeout, @@ -57,10 +58,6 @@ struct RunOptions { /// is killed. const UNHEALTHY_BROKER_EXIT_GRACE: Duration = Duration::from_secs(3); -/// Failure text used when a caller declines to wait for the transport. -/// Callers match on it to offer "try again" rather than a generic failure. -pub(crate) const BROKER_BUSY: &str = "process broker is busy running another helper"; - /// How a caller wants the single-socket transport acquired. /// /// One socket serializes every exchange, and a `Run` can hold it for as long as @@ -220,7 +217,11 @@ impl ProcessBroker { ExchangeWait::Immediate => match self.inner.exchange_lock.try_lock() { Ok(guard) => Ok(guard), Err(TryLockError::Poisoned(poisoned)) => Ok(poisoned.into_inner()), - Err(TryLockError::WouldBlock) => bail!(BROKER_BUSY), + Err(TryLockError::WouldBlock) => Err(BrokerError::new( + BrokerErrorKind::Busy, + "process broker is busy running another helper", + ) + .into()), }, } } @@ -268,11 +269,14 @@ impl ProcessBroker { Ok(response) => response, Err(error) => { self.inner.healthy.store(false, Ordering::Release); - return Err(error).context("process broker exchange failed"); + return Err(error).context(BrokerError::new( + BrokerErrorKind::Transport, + "process broker exchange failed", + )); } }; - if let BrokerOutcome::Error { message } = response.outcome { - bail!("process broker rejected request: {message}"); + if let BrokerOutcome::Error { kind, message } = response.outcome { + return Err(BrokerError::new(kind, message).into()); } Ok((response.outcome, descriptors)) } @@ -472,7 +476,7 @@ impl ProcessBroker { ) } - /// Spawn without waiting for the transport, failing with [`BROKER_BUSY`] + /// Spawn without waiting for the transport, failing with a typed busy error /// when another exchange holds it. /// /// For the Wayland callback thread only. Everything else — the daemon's diff --git a/src/process_broker/error.rs b/src/process_broker/error.rs new file mode 100644 index 000000000..99783e85a --- /dev/null +++ b/src/process_broker/error.rs @@ -0,0 +1,51 @@ +use std::fmt; + +use serde::{Deserialize, Serialize}; + +/// Stable broker policy categories; diagnostic wording is never a protocol. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Default)] +#[serde(rename_all = "snake_case")] +pub(crate) enum BrokerErrorKind { + Busy, + MissingExecutable, + Denied, + Transport, + #[default] + Rejected, +} + +#[derive(Debug)] +pub(crate) struct BrokerError { + pub(crate) kind: BrokerErrorKind, + message: String, +} + +impl BrokerError { + pub(super) fn new(kind: BrokerErrorKind, message: impl Into) -> Self { + Self { + kind, + message: message.into(), + } + } + + pub(super) fn spawn(error: std::io::Error) -> anyhow::Error { + let kind = match error.kind() { + std::io::ErrorKind::NotFound => BrokerErrorKind::MissingExecutable, + std::io::ErrorKind::PermissionDenied => BrokerErrorKind::Denied, + _ => BrokerErrorKind::Rejected, + }; + anyhow::Error::new(error).context(Self::new(kind, "broker helper spawn failed")) + } +} + +impl fmt::Display for BrokerError { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str(&self.message) + } +} + +impl std::error::Error for BrokerError {} + +pub(crate) fn error_kind(error: &anyhow::Error) -> Option { + error.downcast_ref::().map(|error| error.kind) +} diff --git a/src/process_broker/execution.rs b/src/process_broker/execution.rs index 511b99f1f..ab1052d9a 100644 --- a/src/process_broker/execution.rs +++ b/src/process_broker/execution.rs @@ -195,6 +195,7 @@ pub(super) fn publish_bounded( let mut child = OwnedProcess::process_group( command .spawn() + .map_err(super::error::BrokerError::spawn) .context("broker publication helper spawn failed")?, ); let stdin = child.child_mut().stdin.take(); @@ -305,7 +306,7 @@ pub(super) fn run_bounded( .stdout(Stdio::piped()) .stderr(Stdio::piped()); let mut child = - OwnedProcess::process_group(command.spawn().context("broker helper spawn failed")?); + OwnedProcess::process_group(command.spawn().map_err(super::error::BrokerError::spawn)?); let mut stdin = child.child_mut().stdin.take(); let stdout = child .child_mut() diff --git a/src/process_broker/mod.rs b/src/process_broker/mod.rs index 9b424ce90..a5ec80db4 100644 --- a/src/process_broker/mod.rs +++ b/src/process_broker/mod.rs @@ -6,6 +6,7 @@ mod bootstrap; mod client; +mod error; mod execution; mod manifest; mod server; @@ -15,7 +16,8 @@ mod wire; #[cfg(test)] mod tests; -pub(crate) use client::{BROKER_BUSY, BrokerChild, ProcessBroker, current, start_for_runtime}; +pub(crate) use client::{BrokerChild, ProcessBroker, current, start_for_runtime}; +pub(crate) use error::{BrokerErrorKind, error_kind}; pub(crate) use server::run_internal_broker_if_requested; pub(crate) use wire::{BrokerOutput, HelperKind, HelperLifetime, STDOUT_CAP_EXCEEDED}; diff --git a/src/process_broker/server.rs b/src/process_broker/server.rs index 3a96d193a..b70d81522 100644 --- a/src/process_broker/server.rs +++ b/src/process_broker/server.rs @@ -95,6 +95,7 @@ fn broker_loop(socket: RawFd, shutdown_fd: RawFd, token: &str) -> Result<()> { let response = BrokerResponse { request_id: String::new(), outcome: BrokerOutcome::Error { + kind: super::error::BrokerErrorKind::Rejected, message: format!("malformed broker request: {error}"), }, }; @@ -118,6 +119,7 @@ fn broker_loop(socket: RawFd, shutdown_fd: RawFd, token: &str) -> Result<()> { ) .unwrap_or_else(|error| BrokerWireResponse { outcome: BrokerOutcome::Error { + kind: super::error::error_kind(&error).unwrap_or_default(), message: truncate_reason(&format!("{error:#}"), 2048), }, descriptors: Vec::new(), @@ -463,7 +465,7 @@ fn spawn_helper( if shutdown_requested(shutdown_fd)? { bail!("broker spawn cancelled during shutdown"); } - let child = command.spawn().context("broker helper spawn failed")?; + let child = command.spawn().map_err(super::error::BrokerError::spawn)?; let child = if initial_detach { // The execed overlay calls setsid(). It must not be a process-group leader // at that point or setsid() deterministically fails with EPERM. diff --git a/src/process_broker/tests.rs b/src/process_broker/tests.rs index acba6479f..a57553c5f 100644 --- a/src/process_broker/tests.rs +++ b/src/process_broker/tests.rs @@ -472,7 +472,8 @@ fn only_the_callback_thread_spawn_declines_to_wait_for_the_transport() { let error = declined.expect_err("try_spawn must not queue behind the long helper"); assert!( - format!("{error:#}").contains(crate::process_broker::BROKER_BUSY), + crate::process_broker::error_kind(&error) + == Some(crate::process_broker::BrokerErrorKind::Busy), "unexpected try_spawn failure: {error:#}" ); assert!( @@ -1361,3 +1362,49 @@ fn wl_copy_publication_accepts_capture_sized_input() { PUBLICATION_BYTES.to_string() ); } + +#[test] +fn missing_executable_category_survives_real_broker_transport_and_context() { + let guard = start_for_runtime().unwrap(); + let error = guard + .broker() + .run( + HelperKind::SessionZenity, + OsStr::new("/definitely-missing-wayscriber-test/zenity"), + [OsStr::new("--file-selection")], + Vec::new(), + Duration::from_secs(1), + 1024, + ) + .unwrap_err() + .context("caller can add diagnostic context"); + + assert_eq!( + crate::process_broker::error_kind(&error), + Some(crate::process_broker::BrokerErrorKind::MissingExecutable) + ); + assert!(format!("{error:#}").contains("broker helper spawn failed")); +} + +#[test] +fn rejected_diagnostic_with_missing_file_words_is_not_a_missing_executable() { + let guard = start_for_runtime().unwrap(); + let error = guard + .broker() + .run( + HelperKind::SessionZenity, + OsStr::new("No such file"), + [OsStr::new("--file-selection")], + Vec::new(), + Duration::from_secs(1), + 1024, + ) + .unwrap_err() + .context("chooser context"); + + assert!(format!("{error:#}").contains("No such file")); + assert_eq!( + crate::process_broker::error_kind(&error), + Some(crate::process_broker::BrokerErrorKind::Rejected) + ); +} diff --git a/src/process_broker/wire.rs b/src/process_broker/wire.rs index 34946015a..0226a6cea 100644 --- a/src/process_broker/wire.rs +++ b/src/process_broker/wire.rs @@ -151,6 +151,8 @@ pub(super) enum BrokerOutcome { }, Acknowledged, Error { + #[serde(default)] + kind: super::error::BrokerErrorKind, message: String, }, } diff --git a/src/runtime_ui_state.rs b/src/runtime_ui_state.rs index 3c56f7cbe..050c7d9b3 100644 --- a/src/runtime_ui_state.rs +++ b/src/runtime_ui_state.rs @@ -1,8 +1,9 @@ //! Seed-guarded runtime UI state authority and persistence coordination. //! //! This module deliberately owns no toolbar event routing. It contains the -//! serialized controller boundary plus the isolated runtime-state wire, store, -//! and writer machinery used to exercise that boundary before UI integration. +//! serialized controller boundary plus runtime-state wire, store, and writer +//! machinery. The backend toolbar adapter routes live mutations and completions +//! through these owners. mod controller; mod model; diff --git a/src/runtime_ui_state/controller.rs b/src/runtime_ui_state/controller.rs index 72870e5fa..1221fe772 100644 --- a/src/runtime_ui_state/controller.rs +++ b/src/runtime_ui_state/controller.rs @@ -31,17 +31,29 @@ pub(crate) enum CommitResult { }, NoChange, RejectedStaleAuthorityEpoch, + #[allow( + dead_code, + reason = "Typed rejection evidence is retained for diagnostics and controller contract tests" + )] RejectedSeedChanged { targets: Vec, }, RejectedWrongController, RejectedUnsupportedVersion, RejectedShuttingDown, + #[allow( + dead_code, + reason = "Typed rejection evidence is retained for diagnostics and controller contract tests" + )] RejectedInvalidValues(MutationShapeError), RejectedControllerBusy { permit: RuntimeUiMutationPermit, barrier: ControllerBarrierId, }, + #[allow( + dead_code, + reason = "Typed rejection evidence is retained for diagnostics and controller contract tests" + )] RejectedPersistence(PipelineProtocolError), } @@ -57,6 +69,10 @@ pub(crate) enum UpdateSeedsResult { }, RejectedShuttingDown, Rejected(SeedRegistryError), + #[allow( + dead_code, + reason = "Typed rejection evidence is retained for diagnostics and controller contract tests" + )] RejectedPersistence(PipelineProtocolError), } @@ -198,7 +214,6 @@ pub(crate) enum ExternalAuthorityInstallError { FileStatusMismatch, UnexpectedDecodedAuthority, AuthorityEpochExhausted, - Seed(SeedRegistryError), Persistence(PipelineProtocolError), } diff --git a/src/runtime_ui_state/controller/construction.rs b/src/runtime_ui_state/controller/construction.rs index b3a8764d8..cd297cb53 100644 --- a/src/runtime_ui_state/controller/construction.rs +++ b/src/runtime_ui_state/controller/construction.rs @@ -138,6 +138,10 @@ impl RuntimeUiStateController { &self.seeds } + #[allow( + dead_code, + reason = "Controller state inspection supports independent mutation and persistence contract tests" + )] pub(crate) fn model(&self) -> &RuntimeUiModel { &self.model } @@ -204,6 +208,10 @@ impl RuntimeUiStateController { self.preview_resolution_outbox.extend(resolved); } + #[allow( + dead_code, + reason = "Controller state inspection supports independent mutation and persistence contract tests" + )] pub(crate) fn pipeline(&self) -> &PersistencePipeline { &self.pipeline } diff --git a/src/runtime_ui_state/controller/lifecycle.rs b/src/runtime_ui_state/controller/lifecycle.rs index 0a5e7622e..897f47dff 100644 --- a/src/runtime_ui_state/controller/lifecycle.rs +++ b/src/runtime_ui_state/controller/lifecycle.rs @@ -6,6 +6,10 @@ impl RuntimeUiStateController { self.pipeline.receipt(revision) } + #[allow( + dead_code, + reason = "Receipt and flush endpoints preserve the persistence protocol; live toolbar currently uses completion integration" + )] pub(crate) fn take_receipt( &mut self, revision: AcceptedStateRevision, @@ -13,6 +17,10 @@ impl RuntimeUiStateController { self.pipeline.take_receipt(revision) } + #[allow( + dead_code, + reason = "Receipt and flush endpoints preserve the persistence protocol; live toolbar currently uses completion integration" + )] pub(crate) fn request_flush( &mut self, through: AcceptedStateRevision, @@ -34,6 +42,10 @@ impl RuntimeUiStateController { self.pipeline.flush_outcome(id) } + #[allow( + dead_code, + reason = "Receipt and flush endpoints preserve the persistence protocol; live toolbar currently uses completion integration" + )] pub(crate) fn take_flush_outcome(&mut self, id: FlushRequestId) -> Option { self.pipeline.take_flush_outcome(id) } diff --git a/src/runtime_ui_state/controller/resets.rs b/src/runtime_ui_state/controller/resets.rs index df97a9358..86e20a050 100644 --- a/src/runtime_ui_state/controller/resets.rs +++ b/src/runtime_ui_state/controller/resets.rs @@ -1,6 +1,10 @@ use super::*; impl RuntimeUiStateController { + #[allow( + dead_code, + reason = "Reset policy is exercised by recovery contracts; live toolbar currently exposes reconciliation" + )] pub(crate) fn request_supported_reset(&mut self) -> RequestResetResult { self.request_runtime_ui_reset() } diff --git a/src/runtime_ui_state/model.rs b/src/runtime_ui_state/model.rs index 3632d2bfe..f22624308 100644 --- a/src/runtime_ui_state/model.rs +++ b/src/runtime_ui_state/model.rs @@ -127,6 +127,10 @@ pub(crate) struct RuntimeUiLiveOnlyOverlay { } impl RuntimeUiLiveOnlyOverlay { + #[allow( + dead_code, + reason = "Preview overlay inspection is used by mutation and rollback contract tests" + )] pub(crate) fn get(&self, target: &InteractionSeedTarget) -> Option<&InteractionSeedValue> { self.values.get(target) } @@ -270,6 +274,10 @@ impl RuntimeUiMutationValues { pub(crate) struct RuntimeUiMutationPermit { pub(crate) controller_id: ControllerId, pub(crate) authority_epoch: u64, + #[allow( + dead_code, + reason = "Mutation identity is retained as permit evidence for protocol assertions" + )] pub(crate) mutation_id: u64, pub(crate) guards: Vec, } diff --git a/src/runtime_ui_state/pipeline.rs b/src/runtime_ui_state/pipeline.rs index aece042c4..317f4ea0d 100644 --- a/src/runtime_ui_state/pipeline.rs +++ b/src/runtime_ui_state/pipeline.rs @@ -120,11 +120,23 @@ pub(crate) enum PipelineProtocolError { InvalidPublishEpoch, RevisionExhausted, MutationIdExhausted, + #[allow( + dead_code, + reason = "Flush protocol errors remain part of the controller receipt contract" + )] FlushIdExhausted, + #[allow( + dead_code, + reason = "Flush protocol errors remain part of the controller receipt contract" + )] FlushBeyondAccepted { requested: AcceptedStateRevision, latest: AcceptedStateRevision, }, + #[allow( + dead_code, + reason = "Flush protocol errors remain part of the controller receipt contract" + )] ControllerBarrierActive { barrier: ControllerBarrierId, }, @@ -185,6 +197,10 @@ impl From for PendingReplacement { #[derive(Debug, Clone, PartialEq, Eq)] enum PendingStage { Replace(PendingReplacement), + #[allow( + dead_code, + reason = "Flush sequencing is retained for the persistence protocol; live toolbar uses direct completions" + )] Flush { id: FlushRequestId, through: AcceptedStateRevision, @@ -214,6 +230,10 @@ pub(crate) struct PersistencePipeline { settled_through: AcceptedStateRevision, next_accepted: AcceptedStateRevision, next_mutation_id: u64, + #[allow( + dead_code, + reason = "Flush sequencing is retained for the persistence protocol; live toolbar uses direct completions" + )] next_flush_id: u64, in_flight: Option, outbound: Option, @@ -262,6 +282,10 @@ impl PersistencePipeline { self.settled_through } + #[allow( + dead_code, + reason = "Accepted-revision inspection is used by independent pipeline contract tests" + )] pub(crate) fn latest_accepted(&self) -> AcceptedStateRevision { self.next_accepted } diff --git a/src/runtime_ui_state/pipeline/integration.rs b/src/runtime_ui_state/pipeline/integration.rs index 35df798fa..1117180fd 100644 --- a/src/runtime_ui_state/pipeline/integration.rs +++ b/src/runtime_ui_state/pipeline/integration.rs @@ -127,6 +127,10 @@ impl PersistencePipeline { /// Consume a terminal durability outcome after it has been delivered. /// Pending receipts remain registered and return `None`. + #[allow( + dead_code, + reason = "Receipt extraction is retained for controller flush and integration contracts" + )] pub(crate) fn take_receipt( &mut self, revision: AcceptedStateRevision, @@ -202,6 +206,10 @@ impl PersistencePipeline { /// Consume a terminal flush outcome after it has been delivered. /// Pending flushes remain registered and return `None`. + #[allow( + dead_code, + reason = "Receipt extraction is retained for controller flush and integration contracts" + )] pub(crate) fn take_flush_outcome(&mut self, id: FlushRequestId) -> Option { if !self.flushes.get(&id).is_some_and(Option::is_some) { return None; diff --git a/src/runtime_ui_state/pipeline/queue.rs b/src/runtime_ui_state/pipeline/queue.rs index c4428d397..004ab89e4 100644 --- a/src/runtime_ui_state/pipeline/queue.rs +++ b/src/runtime_ui_state/pipeline/queue.rs @@ -2,6 +2,10 @@ use super::integration::durability_satisfies_flush; use super::*; impl PersistencePipeline { + #[allow( + dead_code, + reason = "Flush queuing is retained for controller receipt and ordering contracts" + )] pub(crate) fn request_flush( &mut self, through: AcceptedStateRevision, diff --git a/src/runtime_ui_state/preview.rs b/src/runtime_ui_state/preview.rs index 4b52da152..426896fbe 100644 --- a/src/runtime_ui_state/preview.rs +++ b/src/runtime_ui_state/preview.rs @@ -6,6 +6,10 @@ use super::*; pub(crate) struct RuntimeUiLiveOnlyGuard { pub(crate) controller_id: ControllerId, pub(crate) authority_epoch: u64, + #[allow( + dead_code, + reason = "Preview identity and scope are retained as rollback and reconciliation evidence" + )] pub(crate) session_id: u64, pub(crate) guards: Vec, } @@ -20,6 +24,10 @@ pub(crate) struct RuntimePersistentPreviewSession { #[derive(Debug)] pub(crate) struct RuntimeLiveOnlyPreviewSession { pub(crate) guard: RuntimeUiLiveOnlyGuard, + #[allow( + dead_code, + reason = "Preview identity and scope are retained as rollback and reconciliation evidence" + )] pub(crate) scope: RuntimeUiMutationScope, pub(crate) rollback: PreviewRollbackSnapshot, } diff --git a/src/runtime_ui_state/recovery.rs b/src/runtime_ui_state/recovery.rs index af7a7f016..c6b30dbfb 100644 --- a/src/runtime_ui_state/recovery.rs +++ b/src/runtime_ui_state/recovery.rs @@ -86,6 +86,10 @@ impl PersistenceRecoveryHandle { self.incident } + #[allow( + dead_code, + reason = "Recovery handle identity inspection supports stale-completion contract tests" + )] pub(crate) fn handle_id(&self) -> RecoveryHandleId { self.handle_id } @@ -363,6 +367,10 @@ pub(crate) struct PersistenceRecoveryEvidence { } #[derive(Debug)] +#[allow( + dead_code, + reason = "Structured recovery evidence is retained even when a live consumer only inspects the outcome" +)] pub(crate) enum PersistenceRecoveryResult { Recovered { incident: PersistenceIncidentId, @@ -416,6 +424,10 @@ pub(crate) enum PersistenceRecoveryResult { } #[derive(Debug)] +#[allow( + dead_code, + reason = "Structured recovery evidence is retained even when a live consumer only inspects the outcome" +)] pub(crate) enum BeginPersistenceRecoveryResult { Started { client: RecoveryAttemptClient, @@ -436,6 +448,10 @@ pub(crate) enum CheckoutPersistenceRecoveryHandleResult { } #[derive(Debug)] +#[allow( + dead_code, + reason = "Structured recovery evidence is retained even when a live consumer only inspects the outcome" +)] pub(crate) enum SubmitPersistenceRecoveryResult { Continue { dispatched: RecoveryCommandId, @@ -463,6 +479,10 @@ pub(crate) enum SubmitPersistenceRecoveryResult { } #[derive(Debug)] +#[allow( + dead_code, + reason = "Structured recovery evidence is retained even when a live consumer only inspects the outcome" +)] pub(crate) enum CancelPersistenceRecoveryResult { Cancelled, PendingIrrevocableIo { diff --git a/src/runtime_ui_state/seeds.rs b/src/runtime_ui_state/seeds.rs index bc26629ee..d4f9fd56a 100644 --- a/src/runtime_ui_state/seeds.rs +++ b/src/runtime_ui_state/seeds.rs @@ -9,6 +9,10 @@ pub(crate) struct SeedState { } impl SeedState { + #[allow( + dead_code, + reason = "Seed inspection and construction support config reload and identity contract tests" + )] pub(crate) fn generation(&self) -> u64 { self.generation } @@ -50,6 +54,10 @@ impl ValidatedInteractionSeeds { self.values.get(target) } + #[allow( + dead_code, + reason = "Seed inspection and construction support config reload and identity contract tests" + )] pub(crate) fn remove( &mut self, target: &InteractionSeedTarget, @@ -57,12 +65,20 @@ impl ValidatedInteractionSeeds { self.values.remove(target) } + #[allow( + dead_code, + reason = "Seed inspection and construction support config reload and identity contract tests" + )] pub(crate) fn iter( &self, ) -> impl Iterator { self.values.iter() } + #[allow( + dead_code, + reason = "Seed inspection and construction support config reload and identity contract tests" + )] pub(crate) fn is_empty(&self) -> bool { self.values.is_empty() } @@ -111,6 +127,10 @@ impl StagedSeedReload { }) } + #[allow( + dead_code, + reason = "Seed inspection and construction support config reload and identity contract tests" + )] pub(crate) fn registry(&self) -> &InteractionSeedRegistry { &self.registry } @@ -155,6 +175,10 @@ impl InteractionSeedRegistry { self.state(target).and_then(SeedState::normalized_value) } + #[allow( + dead_code, + reason = "Seed inspection and construction support config reload and identity contract tests" + )] pub(crate) fn contains_current(&self, target: &InteractionSeedTarget) -> bool { self.current_value(target).is_some() } diff --git a/src/runtime_ui_state/source_revision.rs b/src/runtime_ui_state/source_revision.rs index 67f8d6883..3696ea47e 100644 --- a/src/runtime_ui_state/source_revision.rs +++ b/src/runtime_ui_state/source_revision.rs @@ -67,6 +67,10 @@ impl RuntimeStatePathIdentity { &self.source_path } + #[allow( + dead_code, + reason = "Source revision inspection and synthetic revisions support durable-store identity contract tests" + )] pub(crate) fn followed_links(&self) -> &[(PathBuf, PathBuf)] { &self.followed_links } @@ -94,6 +98,10 @@ impl RuntimeStateSourceRevision { Self::Missing { path } } + #[allow( + dead_code, + reason = "Source revision inspection and synthetic revisions support durable-store identity contract tests" + )] pub(crate) fn present(path: RuntimeStatePathIdentity, bytes: impl Into>) -> Self { Self::Present { path, @@ -127,6 +135,10 @@ impl RuntimeStateSourceRevision { } } + #[allow( + dead_code, + reason = "Source revision inspection and synthetic revisions support durable-store identity contract tests" + )] pub(crate) fn file_identity(&self) -> Option { match self { Self::Missing { .. } => None, diff --git a/src/runtime_ui_state/types.rs b/src/runtime_ui_state/types.rs index 0c1a20fc4..3cf692e37 100644 --- a/src/runtime_ui_state/types.rs +++ b/src/runtime_ui_state/types.rs @@ -9,6 +9,7 @@ macro_rules! id_type { pub(crate) struct $name(pub(crate) u64); impl $name { + #[allow(dead_code, reason = "Some protocol identities only need their numeric projection in serialization or contract tests")] pub(crate) const fn get(self) -> u64 { self.0 } @@ -209,6 +210,10 @@ pub(crate) enum RuntimeUiFileStatus { #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) enum ControllerBarrierOperation { + #[allow( + dead_code, + reason = "Explicit reset barrier is retained for controller reset contracts" + )] RequestRuntimeUiReset, ResetSupported, ConfirmUnsupportedReset, @@ -223,7 +228,6 @@ pub(crate) enum RecoveryAttemptStep { AwaitingControllerDecision, SourceMutationInFlight(RecoveryCommandId), ProtocolFailureAwaitingSourceMutation(RecoveryCommandId), - CleanupInFlight(RecoveryCommandId), CancellationPending(RecoveryCommandId), } @@ -233,8 +237,6 @@ pub(crate) enum ControllerBarrierPhase { WaitingForPrerequisite(SourceMutationId), Writing(SourceMutationId), Reinspecting, - InstallingAuthority, - ResolvingPreviews, PersistenceUnhealthy { incident: PersistenceIncidentId, }, diff --git a/src/runtime_ui_state/writer.rs b/src/runtime_ui_state/writer.rs index 3405a6c43..bb44ad6db 100644 --- a/src/runtime_ui_state/writer.rs +++ b/src/runtime_ui_state/writer.rs @@ -37,10 +37,6 @@ pub(crate) struct RuntimeUiStateWriter { } impl RuntimeUiStateWriter { - pub(crate) fn spawn(store: RuntimeUiStateStore) -> std::io::Result { - Self::spawn_with_completion_notifier(store, || {}) - } - pub(crate) fn spawn_with_completion_notifier( store: RuntimeUiStateStore, notify_completion: impl Fn() + Send + 'static, diff --git a/src/runtime_ui_state/writer/tests.rs b/src/runtime_ui_state/writer/tests.rs index 4e6b85bf8..008a53a88 100644 --- a/src/runtime_ui_state/writer/tests.rs +++ b/src/runtime_ui_state/writer/tests.rs @@ -24,7 +24,7 @@ fn every_accepted_command_produces_one_typed_completion() { let path = temp.path().join("runtime-ui.toml"); let store = RuntimeUiStateStore::new(&path); let expected = store.inspect().unwrap().observation.revision; - let writer = RuntimeUiStateWriter::spawn(store).unwrap(); + let writer = RuntimeUiStateWriter::spawn_with_completion_notifier(store, || {}).unwrap(); let request = SourceMutationRequest { id: SourceMutationId(1), accepted_through: AcceptedStateRevision(1), @@ -109,7 +109,8 @@ fn source_commands_are_serialized_against_the_previous_completion_source() { let path = temp.path().join("runtime-ui.toml"); let store = RuntimeUiStateStore::new(&path); let missing = store.inspect().unwrap().observation.revision; - let writer = RuntimeUiStateWriter::spawn(store.clone()).unwrap(); + let writer = + RuntimeUiStateWriter::spawn_with_completion_notifier(store.clone(), || {}).unwrap(); let first = SourceMutationRequest { id: SourceMutationId(1), accepted_through: AcceptedStateRevision(1), @@ -154,7 +155,7 @@ fn writer_reports_filesystem_failures_without_dropping_completion() { let path = temp.path().join("gone/runtime-ui.toml"); let store = RuntimeUiStateStore::new(&path); let expected = store.inspect().unwrap().observation.revision; - let writer = RuntimeUiStateWriter::spawn(store).unwrap(); + let writer = RuntimeUiStateWriter::spawn_with_completion_notifier(store, || {}).unwrap(); writer .submit(RuntimeStateWriterCommand::SourceMutation( SourceMutationRequest { @@ -183,7 +184,9 @@ fn inspection_command_returns_exact_unsupported_bytes() { let path = temp.path().join("runtime-ui.toml"); let bytes = b"version = 22\nfuture = true\n"; fs::write(&path, bytes).unwrap(); - let writer = RuntimeUiStateWriter::spawn(RuntimeUiStateStore::new(path)).unwrap(); + let writer = + RuntimeUiStateWriter::spawn_with_completion_notifier(RuntimeUiStateStore::new(path), || {}) + .unwrap(); let command = RecoveryIoCommand { controller_id: ControllerId(1), incident: PersistenceIncidentId(2), @@ -218,7 +221,9 @@ fn inspection_command_returns_exact_unsupported_bytes() { fn a_batch_of_accepted_inspections_has_no_missing_or_duplicate_completion() { let temp = crate::test_temp::tempdir().unwrap(); let path = temp.path().join("runtime-ui.toml"); - let writer = RuntimeUiStateWriter::spawn(RuntimeUiStateStore::new(path)).unwrap(); + let writer = + RuntimeUiStateWriter::spawn_with_completion_notifier(RuntimeUiStateStore::new(path), || {}) + .unwrap(); for id in 1..=12 { writer .submit(RuntimeStateWriterCommand::Recovery(RecoveryIoCommand { @@ -256,7 +261,8 @@ fn malformed_startup_is_preserved_and_reset_through_real_recovery_io() { .into_controller_bootstrap(startup_seeds()); let incident = bootstrap.startup_incident.unwrap(); let mut controller = bootstrap.controller; - let writer = RuntimeUiStateWriter::spawn(store.clone()).unwrap(); + let writer = + RuntimeUiStateWriter::spawn_with_completion_notifier(store.clone(), || {}).unwrap(); let recovery = match controller.checkout_persistence_recovery_handle(incident) { CheckoutPersistenceRecoveryHandleResult::CheckedOut(handle) => handle, diff --git a/src/session/lock.rs b/src/session/lock.rs index 60f0326c2..b323eb551 100644 --- a/src/session/lock.rs +++ b/src/session/lock.rs @@ -2,75 +2,10 @@ use std::fs::{File, OpenOptions}; use std::io; use std::path::Path; +pub(crate) use crate::durable_io::unlock; +pub use crate::durable_io::{lock_exclusive, lock_shared, try_lock_exclusive}; #[cfg(unix)] use std::os::unix::fs::OpenOptionsExt; -#[cfg(unix)] -use std::os::unix::io::AsRawFd; - -#[cfg(unix)] -fn flock(file: &File, op: libc::c_int) -> io::Result<()> { - let fd = file.as_raw_fd(); - // SAFETY: `fd` stays open for the borrowed `file`, and flock takes no - // pointers or ownership of the descriptor. - let result = unsafe { libc::flock(fd, op) }; - if result == 0 { - Ok(()) - } else { - Err(io::Error::last_os_error()) - } -} - -#[cfg(not(unix))] -fn flock(_file: &File, _op: i32) -> io::Result<()> { - Err(io::Error::new( - io::ErrorKind::Unsupported, - "file locking is not supported on this platform", - )) -} - -pub fn lock_shared(file: &File) -> io::Result<()> { - #[cfg(unix)] - { - flock(file, libc::LOCK_SH) - } - #[cfg(not(unix))] - { - flock(file, 0) - } -} - -pub fn lock_exclusive(file: &File) -> io::Result<()> { - #[cfg(unix)] - { - flock(file, libc::LOCK_EX) - } - #[cfg(not(unix))] - { - flock(file, 0) - } -} - -pub fn try_lock_exclusive(file: &File) -> io::Result<()> { - #[cfg(unix)] - { - flock(file, libc::LOCK_EX | libc::LOCK_NB) - } - #[cfg(not(unix))] - { - flock(file, 0) - } -} - -pub fn unlock(file: &File) -> io::Result<()> { - #[cfg(unix)] - { - flock(file, libc::LOCK_UN) - } - #[cfg(not(unix))] - { - flock(file, 0) - } -} pub(crate) fn open_runtime_lock_file(lock_path: &Path, named: bool) -> io::Result { if named { diff --git a/src/session/snapshot/tests.rs b/src/session/snapshot/tests.rs index 24e641c85..bdebf1ba7 100644 --- a/src/session/snapshot/tests.rs +++ b/src/session/snapshot/tests.rs @@ -2686,3 +2686,95 @@ fn a_failed_save_leaves_no_temporary_file_behind() { "temporary files were left behind: {strays:?}" ); } + +#[test] +fn maximum_admitted_image_and_create_history_fit_actual_default_session_budget() { + use super::save::{SaveSnapshotOutcome, estimate_snapshot_save, save_snapshot_with_report}; + use crate::draw::EmbeddedImage; + use crate::screen_pixels::EmbeddedImageLimits; + + let limits = EmbeddedImageLimits::default(); + let mut seed = 0x1234_5678_u32; + let mut random_byte = || { + seed ^= seed << 13; + seed ^= seed >> 17; + seed ^= seed << 5; + seed as u8 + }; + let pixels: Vec = (0..3_000_000).map(|_| random_byte()).collect(); + let mut bytes = Vec::new(); + let mut encoder = png::Encoder::new(&mut bytes, 1000, 1000); + encoder.set_color(png::ColorType::Rgb); + encoder.set_depth(png::BitDepth::Eight); + let mut writer = encoder.write_header().unwrap(); + writer.write_image_data(&pixels).unwrap(); + writer.finish().unwrap(); + assert!(bytes.len() <= limits.max_bytes()); + // PNG readers permit trailing bytes after IEND; use them to reach the + // admission cap exactly without compressible zero padding. + while bytes.len() < limits.max_bytes() { + bytes.push(random_byte()); + } + assert!(limits.allows_bytes(bytes.len())); + crate::image_decode::decode_rgba( + crate::image_decode::EncodedImageFormat::Png, + &bytes, + limits.into(), + ) + .unwrap(); + + let mut snapshot = sample_snapshot(); + let mut frame = Frame::new(); + let id = frame.add_shape(Shape::Image { + x: 0, + y: 0, + w: 1000, + h: 1000, + data: EmbeddedImage { + mime_type: "image/png".to_string(), + width: 1000, + height: 1000, + bytes: bytes.into(), + }, + }); + frame.push_undo_action( + UndoAction::Create { + shapes: vec![(0, frame.shape(id).unwrap().clone())], + }, + 100, + ); + snapshot.boards[0].pages.pages[0] = frame; + let temp = tempdir().unwrap(); + let options = crate::session::options::options_from_config_for_named_file( + &crate::config::SessionConfig::default(), + temp.path().join("maximum-image.wayscriber-session"), + Some("test"), + ); + assert!(options.persist_history); + let estimate = estimate_snapshot_save(&snapshot, &options).unwrap(); + assert_eq!(estimate.full.limit_exceeded, None); + assert!( + estimate.full.raw_size as u64 <= options.max_file_size_bytes, + "full history must fit without relying on compression" + ); + let report = save_snapshot_with_report(&snapshot, &options) + .unwrap() + .unwrap(); + assert_eq!(report.outcome, SaveSnapshotOutcome::Full); + let mut loaded = load_snapshot(&options).unwrap().unwrap(); + let restored = &mut loaded.boards[0].pages.pages[0]; + let Shape::Image { data, .. } = &restored.shape(id).expect("restored image").shape else { + panic!("restored shape is an image"); + }; + assert_eq!(data.bytes.len(), limits.max_bytes()); + assert_eq!( + restored.shape(id).unwrap().shape, + snapshot.boards[0].pages.pages[0].shape(id).unwrap().shape + ); + assert_eq!(restored.undo_stack_len(), 1); + assert!(restored.undo_last().is_some()); + assert!( + restored.shapes.is_empty(), + "create history survives full save/load" + ); +} diff --git a/src/test_temp.rs b/src/test_temp.rs index a23808c99..15be825cc 100644 --- a/src/test_temp.rs +++ b/src/test_temp.rs @@ -1,45 +1 @@ -use std::path::{Path, PathBuf}; -use std::sync::atomic::{AtomicU64, Ordering}; -use std::{fs, io}; - -static NEXT_TEMP_ID: AtomicU64 = AtomicU64::new(0); - -pub(crate) struct TempDir { - path: PathBuf, -} - -impl TempDir { - pub(crate) fn new() -> io::Result { - tempdir() - } - - pub(crate) fn path(&self) -> &Path { - &self.path - } -} - -impl Drop for TempDir { - fn drop(&mut self) { - let _ = fs::remove_dir_all(&self.path); - } -} - -pub(crate) fn tempdir() -> io::Result { - let base = std::env::temp_dir(); - let pid = std::process::id(); - - for _ in 0..100 { - let id = NEXT_TEMP_ID.fetch_add(1, Ordering::Relaxed); - let path = base.join(format!("wayscriber-test-{pid}-{id}")); - match fs::create_dir(&path) { - Ok(()) => return Ok(TempDir { path }), - Err(err) if err.kind() == io::ErrorKind::AlreadyExists => continue, - Err(err) => return Err(err), - } - } - - Err(io::Error::new( - io::ErrorKind::AlreadyExists, - "failed to create a unique temporary test directory", - )) -} +pub(crate) use tempfile::{TempDir, tempdir}; diff --git a/src/toolbar_icons/mod.rs b/src/toolbar_icons/mod.rs index 6dc8dd175..7e1c68332 100644 --- a/src/toolbar_icons/mod.rs +++ b/src/toolbar_icons/mod.rs @@ -29,6 +29,30 @@ pub(crate) use smoothing_preview::draw_smoothing_preview; pub(crate) type ToolbarIconPainter = fn(&cairo::Context, f64, f64, f64); +/// Resolve action metadata to the shared Cairo glyph used by UI surfaces. +pub fn action_icon_painter( + icon: crate::config::action_meta::ActionIcon, +) -> fn(&cairo::Context, f64, f64, f64) { + use crate::config::action_meta::ActionIcon; + + match icon { + ActionIcon::Text => draw_icon_text, + ActionIcon::StickyNote => draw_icon_note, + ActionIcon::Select => draw_icon_select, + ActionIcon::Pen => draw_icon_pen, + ActionIcon::Line => draw_icon_line, + ActionIcon::Rect => draw_icon_rect, + ActionIcon::Ellipse => draw_icon_circle, + ActionIcon::FreeformPolygon => draw_icon_polygon, + ActionIcon::Arrow => draw_icon_arrow, + ActionIcon::Blur => draw_icon_blur, + ActionIcon::Marker => draw_icon_marker, + ActionIcon::StepMarker => draw_icon_step_marker, + ActionIcon::Eraser => draw_icon_eraser, + ActionIcon::Undo => draw_icon_undo, + } +} + /// Paint inputs of the micro-mode chip that vary with live state. pub(crate) struct MicroChipStyle { /// Ring stroke color: the current drawing color. @@ -235,6 +259,39 @@ mod painter_tests { ("zoom_reset", draw_icon_zoom_reset), ]; + #[test] + fn icons_reuse_the_shared_semantic_tool_painters() { + use crate::input::Tool; + use crate::toolbar_icons::top_toolbar_icon_painter; + use crate::ui::toolbar::model::{TopToolbarIcon, semantic_icon_for_tool}; + + for meta in crate::config::action_meta_iter().filter(|meta| meta.icon.is_some()) { + let action = meta.action; + let icon = crate::config::action_meta(action) + .and_then(|meta| meta.icon) + .map(action_icon_painter) + .unwrap_or_else(|| panic!("missing icon for {:?}", action)); + let expected = match action { + crate::config::Action::EnterTextMode => { + top_toolbar_icon_painter(TopToolbarIcon::Text) + } + crate::config::Action::EnterStickyNoteMode => { + top_toolbar_icon_painter(TopToolbarIcon::StickyNote) + } + _ => { + let tool = Tool::from_select_action(action) + .unwrap_or_else(|| panic!("{:?} should select a tool", action)); + top_toolbar_icon_painter(TopToolbarIcon::Tool(semantic_icon_for_tool(tool))) + } + }; + assert!( + std::ptr::fn_addr_eq(icon, expected), + "icon painter for {:?} drifted from the shared semantic painter", + action + ); + } + } + #[test] fn every_public_icon_paints_something_at_every_chrome_size() { for (name, paint) in PAINTERS { diff --git a/src/ui/command_palette/command_palette_row.rs b/src/ui/command_palette/command_palette_row.rs index 07c6ee9f2..e1263cbc9 100644 --- a/src/ui/command_palette/command_palette_row.rs +++ b/src/ui/command_palette/command_palette_row.rs @@ -80,7 +80,7 @@ pub(super) fn render_command_row( if let Some(icon) = cmd.icon { let icon_alpha = if is_selected { 0.95 } else { 0.7 }; constants::set_color(ctx, constants::with_alpha(theme.text_primary, icon_alpha)); - icon( + crate::toolbar_icons::action_icon_painter(icon)( ctx, inner_x + 10.0, item_y + (COMMAND_PALETTE_ITEM_HEIGHT - COMMAND_PALETTE_ROW_ICON_SIZE) / 2.0, diff --git a/src/ui/command_palette/tests/engine.rs b/src/ui/command_palette/tests/engine.rs index 457b603a4..cc8ee8bd9 100644 --- a/src/ui/command_palette/tests/engine.rs +++ b/src/ui/command_palette/tests/engine.rs @@ -312,3 +312,56 @@ fn row_shortcut_controls_show_only_on_the_selected_and_hovered_rows() { "hovering a row reveals its controls" ); } + +#[test] +fn palette_rows_highlight_literal_label_matches_but_not_fuzzy_matches() { + let engine = UiTextEngine::default(); + let mut input = crate::input::state::test_support::make_test_input_state(); + input.toggle_command_palette(); + input.command_palette.set_query("tool"); + let mut view = CommandPaletteView::prepare(&input, 800, 600); + let CommandPaletteView::List(list) = &view else { + panic!("open palette"); + }; + let (x, y, width, height) = list.geometry; + let rows_top = + y + COMMAND_PALETTE_PADDING + COMMAND_PALETTE_INPUT_HEIGHT + COMMAND_PALETTE_LIST_GAP; + let mut theme = crate::ui::theme::Theme::dark(); + theme.accent = (1.0, 0.0, 1.0, 1.0); + + let count_highlight = |query_view: &CommandPaletteView| { + let rgba = pixels(1, |ctx| { + paint_command_palette(&theme, &engine, ctx, query_view, 800, 600) + }); + rgba.as_chunks::<4>() + .0 + .iter() + .enumerate() + .filter(|(index, pixel)| { + let px = (index % 800) as f64; + let py = (index / 800) as f64; + // Exclude the query box, icons and shortcut controls; only the + // command-label column can contribute these magenta backdrops. + px >= x + 44.0 + && px < x + width * 0.65 + && py >= rows_top + && py < y + height - 48.0 + && pixel[0] > pixel[1].saturating_add(35) + && pixel[2] > pixel[1].saturating_add(35) + }) + .count() + }; + let literal = count_highlight(&view); + let CommandPaletteView::List(list) = &mut view else { + unreachable!(); + }; + // Keep the exact prepared rows, selection and layout for the control. + // "tl" is a subsequence of "tool", but not a literal label substring. + list.query = "tl".to_string(); + let fuzzy = count_highlight(&view); + + assert!( + literal > fuzzy + 100, + "literal matches add visible accent pixels beyond the selected row: literal={literal}, fuzzy={fuzzy}" + ); +} diff --git a/src/ui/radial_menu/mod.rs b/src/ui/radial_menu/mod.rs index d0738a3f7..c198b05ac 100644 --- a/src/ui/radial_menu/mod.rs +++ b/src/ui/radial_menu/mod.rs @@ -390,7 +390,9 @@ fn draw_sub_ring( let lx = cx + mid_r * mid_angle.cos(); let ly = cy + mid_r * mid_angle.sin(); let color = wedge_content_color(theme, is_hovered, is_active); - let icon = action_meta(*action).and_then(|meta| meta.icon); + let icon = action_meta(*action) + .and_then(|meta| meta.icon) + .map(crate::toolbar_icons::action_icon_painter); let label = action_short_label(*action); match icon { Some(_) if show_labels => { @@ -672,7 +674,9 @@ fn slice_label(slice: &RadialSlice) -> &'static str { /// family glyphs for parents. fn slice_icon(slice: &RadialSlice) -> Option { match slice.kind { - RadialSliceKind::Action(action) => action_meta(action).and_then(|meta| meta.icon), + RadialSliceKind::Action(action) => action_meta(action) + .and_then(|meta| meta.icon) + .map(crate::toolbar_icons::action_icon_painter), RadialSliceKind::Parent(RadialParent::Shapes) => Some(draw_icon_shape_picker), RadialSliceKind::Parent(RadialParent::Notes) => Some(draw_icon_note), } diff --git a/src/ui/status/bar/tests.rs b/src/ui/status/bar/tests.rs index ab76f7c86..29e5db0c6 100644 --- a/src/ui/status/bar/tests.rs +++ b/src/ui/status/bar/tests.rs @@ -1,5 +1,5 @@ use super::*; -use crate::config::{KeybindingsConfig, StatusBarItem, StatusBarStyle}; +use crate::config::{StatusBarItem, StatusBarStyle}; use crate::draw::{Color, Shape}; /// Worst-case prefix: selection info plus a long output label on a @@ -12,11 +12,6 @@ const CLUSTER_LINE_HEIGHT: f64 = 21.0; const DOT_DIAMETER: f64 = 12.0; fn make_state() -> InputState { - let keybindings = KeybindingsConfig::default(); - let _action_map = keybindings - .build_action_map() - .expect("default keybindings map"); - crate::input::state::test_support::make_test_input_state() } diff --git a/src/ui/status/zoom_chip.rs b/src/ui/status/zoom_chip.rs index 08ea193c0..b808a522d 100644 --- a/src/ui/status/zoom_chip.rs +++ b/src/ui/status/zoom_chip.rs @@ -722,13 +722,9 @@ fn draw_zoom_sign(ctx: &cairo::Context, m: &ZoomGlyphMetrics, run_x: f64, plus: #[cfg(test)] mod tests { use super::*; - use crate::config::{KeybindingsConfig, StatusBarStyle}; + use crate::config::StatusBarStyle; fn make_state() -> InputState { - let keybindings = KeybindingsConfig::default(); - let _action_map = keybindings - .build_action_map() - .expect("default keybindings map"); crate::input::state::test_support::make_test_input_state() } diff --git a/src/ui/theme.rs b/src/ui/theme.rs index 3c88358d8..cf367972a 100644 --- a/src/ui/theme.rs +++ b/src/ui/theme.rs @@ -946,6 +946,17 @@ pub fn lerp_color(from: Rgba, to: Rgba, t: f64) -> Rgba { ) } +impl crate::config::UiTheme { + /// Maps the config value onto the runtime theme mode. + pub fn to_theme_mode(self) -> crate::ui::theme::ThemeMode { + match self { + crate::config::UiTheme::Auto => crate::ui::theme::ThemeMode::Auto, + crate::config::UiTheme::Dark => crate::ui::theme::ThemeMode::Dark, + crate::config::UiTheme::Light => crate::ui::theme::ThemeMode::Light, + } + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/src/ui/toolbar/snapshot.rs b/src/ui/toolbar/snapshot.rs index 85ea49613..d85a0ece6 100644 --- a/src/ui/toolbar/snapshot.rs +++ b/src/ui/toolbar/snapshot.rs @@ -3,6 +3,9 @@ pub mod fade; mod text_controls; mod types; +#[cfg(test)] +mod selection_tests; + pub use types::{ PresetFeedbackSnapshot, PresetSlotSnapshot, RuntimeUiPersistenceMode, RuntimeUiPersistenceSnapshot, SessionRecentSnapshot, ToolContext, ToolOptionsKind, diff --git a/src/ui/toolbar/snapshot/build.rs b/src/ui/toolbar/snapshot/build.rs index d60dba5ac..af3e88e12 100644 --- a/src/ui/toolbar/snapshot/build.rs +++ b/src/ui/toolbar/snapshot/build.rs @@ -19,6 +19,7 @@ impl ToolbarSnapshot { ) -> Self { let frame = state.boards.active_frame(); let active_tool = state.active_tool(); + let selected = state.resolved_selected_shapes(); let board_count = state.boards.board_count(); let board_index = state.boards.active_index(); let board_name = state.board_name().to_string(); @@ -120,10 +121,11 @@ impl ToolbarSnapshot { spotlight_magnification: state.style.spotlight_magnification, // Filled in by the backend that renders the canvas; see the field. spotlight_magnifier_source: None, - selection_spotlight_magnification: state.selection_spotlight_magnification(), + selection_spotlight_magnification: + InputState::resolved_selection_spotlight_magnification(&selected), font: state.style.font_descriptor.clone(), - selection_has_text: state.selection_has_text(), - selected_text_bold: state.first_editable_selected_text_is_bold(), + selection_has_text: InputState::resolved_selection_has_text(&selected), + selected_text_bold: InputState::resolved_selected_text_is_bold(&selected), font_size: state.style.current_font_size, text_active, note_active, @@ -220,7 +222,10 @@ impl ToolbarSnapshot { // the backend publishes the animated value. top_fade: 1.0, selection_properties: if active_tool == crate::input::Tool::Select { - state.selection_pill_entries() + state.build_selection_property_entries_from_resolved( + state.selected_shape_ids(), + &selected, + ) } else { Vec::new() }, diff --git a/src/ui/toolbar/snapshot/selection_tests.rs b/src/ui/toolbar/snapshot/selection_tests.rs new file mode 100644 index 000000000..2d9c68497 --- /dev/null +++ b/src/ui/toolbar/snapshot/selection_tests.rs @@ -0,0 +1,220 @@ +use super::ToolbarSnapshot; + +#[test] +fn shared_selection_preserves_text_order_locked_notes_and_normalized_spotlights() { + let mut state = make_test_input_state(); + state.apply_toolbar_event(crate::ui::toolbar::ToolbarEvent::SelectTool(Tool::Select)); + let mut bold = state.style.font_descriptor.clone(); + bold.weight = "bold".to_string(); + let mut normal = state.style.font_descriptor.clone(); + normal.weight = "normal".to_string(); + let color = state.style.current_color; + let frame = state.boards.active_frame_mut(); + let note = frame.add_shape(Shape::StickyNote { + x: 0, + y: 0, + text: "Locked".into(), + background: color, + size: 18.0, + font_descriptor: bold.clone(), + wrap_width: None, + }); + frame.shapes.last_mut().unwrap().locked = true; + let deleted = frame.add_shape(Shape::Text { + x: 0, + y: 0, + text: "Deleted bold text".into(), + color, + size: 18.0, + font_descriptor: bold.clone(), + background_enabled: false, + wrap_width: None, + }); + frame.remove_shape_by_id(deleted).unwrap(); + + let text = frame.add_shape(Shape::Text { + x: 10, + y: 0, + text: "Normal".into(), + color, + size: 18.0, + font_descriptor: normal, + background_enabled: false, + wrap_width: None, + }); + let bold_text = frame.add_shape(Shape::Text { + x: 20, + y: 0, + text: "Bold".into(), + color, + size: 18.0, + font_descriptor: bold, + background_enabled: false, + wrap_width: None, + }); + let spotlights: Vec<_> = [f64::NAN, 2.5, 8.0] + .into_iter() + .map(|magnification| { + frame.add_shape(Shape::Spotlight { + cx: 0, + cy: 0, + rx: 10, + ry: 10, + magnification, + }) + }) + .collect(); + + for (texts, expected_bold) in [ + (vec![note, text, bold_text], Some(false)), + (vec![note, bold_text, text], Some(true)), + ] { + let mut selected = vec![deleted, u64::MAX]; + selected.extend(texts); + selected.extend(&spotlights); + state.set_selection(selected); + Frame::reset_linear_id_lookup_count(); + InputState::reset_selection_resolution_count(); + let snapshot = ToolbarSnapshot::from_input(&state); + assert_eq!(Frame::linear_id_lookup_count(), 0); + assert_eq!(InputState::selection_resolution_count(), 1); + assert!(snapshot.selection_has_text); + assert_eq!(snapshot.selected_text_bold, expected_bold); + assert_eq!(snapshot.selection_spotlight_magnification, Some(4.0)); + } + + // Missing IDs must also be skipped by the tiny-selection path. + for (selected, has_text, bold) in [ + (vec![deleted, u64::MAX, text], true, Some(false)), + (vec![deleted, u64::MAX], false, None), + ] { + state.set_selection(selected); + Frame::reset_linear_id_lookup_count(); + InputState::reset_selection_resolution_count(); + let snapshot = ToolbarSnapshot::from_input(&state); + + assert_eq!(Frame::linear_id_lookup_count(), 0); + assert_eq!(InputState::selection_resolution_count(), 1); + assert_eq!(snapshot.selection_has_text, has_text); + assert_eq!(snapshot.selected_text_bold, bold); + assert_eq!(snapshot.selection_spotlight_magnification, None); + assert_eq!(snapshot.selection_properties.is_empty(), !has_text); + } +} + +use crate::{ + draw::{Frame, Shape}, + input::state::SelectionPropertyKind, + input::{InputState, Tool, state::test_support::make_test_input_state}, +}; + +#[test] +fn complete_snapshot_resolves_selection_once_without_per_id_lookups() { + #[derive(Clone, Copy, Debug, PartialEq, Eq)] + enum Fixture { + Rectangles, + TextLast, + LockedText, + } + + for case in [Fixture::Rectangles, Fixture::TextLast, Fixture::LockedText] { + let mut state = make_test_input_state(); + state.apply_toolbar_event(crate::ui::toolbar::ToolbarEvent::SelectTool(Tool::Select)); + let mut font = state.style.font_descriptor.clone(); + font.weight = "bold".to_string(); + let color = state.style.current_color; + let ids: Vec<_> = (0..2_048) + .map(|index| { + let text = + case == Fixture::LockedText || (case == Fixture::TextLast && index == 2_047); + let shape = if text { + Shape::Text { + x: index, + y: 0, + text: "Text".to_string(), + color, + size: 18.0, + font_descriptor: font.clone(), + background_enabled: false, + wrap_width: None, + } + } else { + Shape::Rect { + x: index, + y: 0, + w: 10, + h: 10, + color, + thick: 3.0, + fill: false, + fill_color: None, + } + }; + let frame = state.boards.active_frame_mut(); + let id = frame.add_shape(shape); + frame.shapes.last_mut().unwrap().locked = case == Fixture::LockedText; + id + }) + .collect(); + + for selected in [ + ids.clone(), + vec![ids[0]], + vec![ids[2_047]], + vec![ids[1], ids[700], ids[2_047]], + ] { + let contains_text = case == Fixture::LockedText + || (case == Fixture::TextLast && selected.contains(&ids[2_047])); + state.set_selection(selected); + // Fixture/setup scans do not count toward the immutable snapshot contract. + Frame::reset_linear_id_lookup_count(); + InputState::reset_selection_resolution_count(); + let snapshot = ToolbarSnapshot::from_input(&state); + + assert_eq!(Frame::linear_id_lookup_count(), 0, "{case:?}"); + assert_eq!(InputState::selection_resolution_count(), 1, "{case:?}"); + assert_eq!(snapshot.selection_has_text, contains_text, "{case:?}"); + assert_eq!( + snapshot.selected_text_bold, + if case == Fixture::TextLast && contains_text { + Some(true) + } else { + None + }, + "{case:?}" + ); + assert_eq!(snapshot.selection_spotlight_magnification, None); + let properties = &snapshot.selection_properties; + assert!(!properties.is_empty()); + if case == Fixture::LockedText { + assert!(properties.iter().all(|entry| entry.disabled)); + assert_eq!( + properties + .iter() + .find(|entry| entry.kind == SelectionPropertyKind::FontSize) + .unwrap() + .value, + "Locked" + ); + } else if contains_text { + assert_eq!( + properties + .iter() + .find(|entry| entry.kind == SelectionPropertyKind::FontSize) + .unwrap() + .value, + "18pt" + ); + } else { + assert_eq!( + properties + .iter() + .find(|entry| entry.kind == SelectionPropertyKind::Thickness) + .unwrap() + .value, + "3.0px" + ); + } + } + } +} diff --git a/tests/cli.rs b/tests/cli.rs index cbcbed800..b3e674681 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -5,7 +5,6 @@ use std::path::{Path, PathBuf}; #[cfg(unix)] use std::process::Stdio; use std::process::{Command, Output}; -use std::sync::atomic::{AtomicU64, Ordering}; #[cfg(unix)] use std::thread; #[cfg(unix)] @@ -20,43 +19,7 @@ use wayscriber::env_vars::{ }; use wayscriber::runtime_capabilities::RUNTIME_CAPABILITIES_FLAG; -static NEXT_TEMP_ID: AtomicU64 = AtomicU64::new(0); - -struct TempDir { - path: PathBuf, -} - -impl TempDir { - fn new() -> std::io::Result { - let base = std::env::temp_dir(); - let pid = std::process::id(); - - for _ in 0..100 { - let id = NEXT_TEMP_ID.fetch_add(1, Ordering::Relaxed); - let path = base.join(format!("wayscriber-cli-test-{pid}-{id}")); - match fs::create_dir(&path) { - Ok(()) => return Ok(Self { path }), - Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => continue, - Err(err) => return Err(err), - } - } - - Err(std::io::Error::new( - std::io::ErrorKind::AlreadyExists, - "failed to create a unique temporary test directory", - )) - } - - fn path(&self) -> &Path { - &self.path - } -} - -impl Drop for TempDir { - fn drop(&mut self) { - let _ = fs::remove_dir_all(&self.path); - } -} +use tempfile::TempDir; struct CommandOutput { output: Output, diff --git a/tests/daemon_v1_fixture.rs b/tests/daemon_v1_fixture.rs index e9da35234..53810141f 100644 --- a/tests/daemon_v1_fixture.rs +++ b/tests/daemon_v1_fixture.rs @@ -1,44 +1,10 @@ use std::fs; use std::path::{Path, PathBuf}; use std::process::{Child, Command, Output}; -use std::sync::atomic::{AtomicU64, Ordering}; use std::time::{Duration, Instant}; const FROZEN_V1_SOURCE: &str = include_str!("fixtures/frozen_daemon_v1.rs"); -static NEXT_TEMP_ID: AtomicU64 = AtomicU64::new(0); - -struct TempDir(PathBuf); - -impl TempDir { - fn new() -> std::io::Result { - for _ in 0..100 { - let id = NEXT_TEMP_ID.fetch_add(1, Ordering::Relaxed); - let path = std::env::temp_dir().join(format!( - "wayscriber-daemon-v1-fixture-{}-{id}", - std::process::id() - )); - match fs::create_dir(&path) { - Ok(()) => return Ok(Self(path)), - Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => continue, - Err(error) => return Err(error), - } - } - Err(std::io::Error::new( - std::io::ErrorKind::AlreadyExists, - "failed to create a unique fixture directory", - )) - } - - fn path(&self) -> &Path { - &self.0 - } -} - -impl Drop for TempDir { - fn drop(&mut self) { - let _ = fs::remove_dir_all(&self.0); - } -} +use tempfile::TempDir; struct ChildGuard(Child); diff --git a/tests/ui.rs b/tests/ui.rs index 321d9863b..80eb79bf7 100644 --- a/tests/ui.rs +++ b/tests/ui.rs @@ -125,9 +125,8 @@ fn render_help_overlay_without_frozen_shortcuts_draws_content() { #[test] fn render_command_palette_with_query_draws_content() { - // Exercises the match-highlight path: an open palette with a query that - // literally appears in some command labels must render without panicking - // and draw pixels (highlight boxes + rows). + // Public API smoke coverage; the palette row artifact test owns highlight + // color and fuzzy-match behavior. let (mut surface, ctx) = surface_with_context(900, 700); let mut input = make_input_state(); input.command_palette.open(); diff --git a/tools/check-shared-dependencies.py b/tools/check-shared-dependencies.py index b5073bef6..f7852f63c 100755 --- a/tools/check-shared-dependencies.py +++ b/tools/check-shared-dependencies.py @@ -1,29 +1,108 @@ #!/usr/bin/env python3 -"""Guard explicit upward Rust paths in the agreed shared-layer source boundary. +"""Guard explicit upward Rust paths in shared layers. -This is a source-path guard, not a full Rust dependency graph. Domain compatibility -path tests deliberately mention old public paths. Pure board normalization lives -in the domain layer; runtime board lifecycle stays in input. +This partial source guard understands rooted/parent-relative paths and grouped +use trees, and ignores comments and literals. It does not resolve aliases, +macro expansion, reexports, or Rust's complete module/dependency graph. """ from pathlib import Path import re import sys -ROOT = Path(__file__).resolve().parent.parent -errors = [] -for directory, forbidden in [ - ("src/domain", "config|input|draw|backend|ui|session"), - ("src/config/validate", "input|backend"), -]: - for path in (ROOT / directory).rglob("*.rs"): - if path == ROOT / "src/domain/tests.rs": - continue # #[cfg(test)] public-path compatibility assertions. - source = re.sub(r"/\*.*?\*/|//[^\n]*", "", path.read_text(), flags=re.S) - # Also catch multiline grouped imports such as use crate::{input::...}. - pattern = rf"crate\s*::\s*(?:{forbidden})\b|use\s+crate\s*::\s*\{{[^;]*\b(?:{forbidden})\s*::" - if re.search(pattern, source): - errors.append(f"{path.relative_to(ROOT)}: upward dependency in shared layer") -if errors: - print("\n".join(errors), file=sys.stderr) - sys.exit(1) -print("Shared domain and configuration-validation dependency paths passed.") +NON_CODE = re.compile( + r'r(?P#{0,16})".*?"(?P=hashes)|"(?:\\.|[^"\\])*"|' + r"'(?:\\.|[^'\\\n])'|//[^\n]*|/\*", re.S +) +TOKENS = re.compile(r'r#[A-Za-z_][A-Za-z_0-9]*|[A-Za-z_][A-Za-z_0-9]*|::|[{},;*]') +BOUNDARIES = [ + ("src/domain", {"config", "input", "draw", "backend", "ui", "session"}), + ("src/config/validate", {"input", "backend"}), +] + + +def strip_non_code(source): + pieces = [] + position = 0 + while match := NON_CODE.search(source, position): + pieces.append(source[position:match.start()]) + position = match.end() + if match.group() == "/*": + depth = 1 + while depth and (marker := re.search(r'/\*|\*/', source[position:])): + depth += 1 if marker.group() == "/*" else -1 + position += marker.end() + if depth: + position = len(source) + pieces.append(" ") + return ''.join(pieces) + source[position:] + + +def module_path(relative_path): + parts = list(Path(relative_path).with_suffix('').parts[1:]) + if parts[-1] == "mod": + parts.pop() + return parts + + +def has_upward_path(source, relative_path, forbidden): + tokens = [token.removeprefix("r#") for token in TOKENS.findall(strip_non_code(source))] + module = module_path(relative_path) + + def tree(index, prefix): + path = list(prefix) + while index < len(tokens): + token = tokens[index] + if token in {",", ";", "}", "as"}: + break + if token == "{": + index += 1 + while index < len(tokens) and tokens[index] != "}": + rejected, index = tree(index, path) + if rejected: + return True, index + if index < len(tokens) and tokens[index] == "as": + index += 2 + if index < len(tokens) and tokens[index] == ",": + index += 1 + elif index < len(tokens) and tokens[index] != "}": + break + return False, index + 1 + if token == "crate": + path = [] + elif token == "super": + path = path[:-1] + elif token not in {"self", "::", "*"}: + path.append(token) + if path and path[0] in forbidden: + return True, index + index += 1 + if index >= len(tokens) or tokens[index] != "::": + break + index += 1 + return False, index + + return any( + tree(index, module)[0] + for index, token in enumerate(tokens[:-1]) + if token in {"crate", "super", "self"} and tokens[index + 1] == "::" + ) + + +def check(root): + errors = [] + for directory, forbidden in BOUNDARIES: + for path in (root / directory).rglob("*.rs"): + relative = path.relative_to(root).as_posix() + if relative == "src/domain/tests.rs": + continue # Public-path compatibility assertions only. + if has_upward_path(path.read_text(), relative, forbidden): + errors.append(f"{relative}: upward dependency in shared layer") + return errors + + +if __name__ == "__main__": + errors = check(Path(__file__).resolve().parent.parent) + if errors: + print("\n".join(errors), file=sys.stderr) + sys.exit(1) + print("Shared domain and configuration-validation dependency paths passed.") diff --git a/tools/csharp-tests/ReleaseParityRegressionTests.cs b/tools/csharp-tests/ReleaseParityRegressionTests.cs index 9b2b45a9e..fcbf1948f 100644 --- a/tools/csharp-tests/ReleaseParityRegressionTests.cs +++ b/tools/csharp-tests/ReleaseParityRegressionTests.cs @@ -320,7 +320,7 @@ private static TemporaryDirectory CreateStandaloneGateFixture( bool dotnetSdkAva fi """ ); foreach ( var check in new[] { "check-nixpkgs-recipe.py", "check-rust-source-coverage.py", "check-process-sites.py", - "check-config-writers.py", "check-shared-dependencies.py" } ) + "check-config-writers.py", "check-shared-dependencies.py", "test-shared-dependencies.py" } ) { WriteExecutable( Path.Combine( tools, check ), "#!/usr/bin/true\n" ); } diff --git a/tools/csharp-tests/RepositoryContractTests.cs b/tools/csharp-tests/RepositoryContractTests.cs index e8668c334..d6395f950 100644 --- a/tools/csharp-tests/RepositoryContractTests.cs +++ b/tools/csharp-tests/RepositoryContractTests.cs @@ -6,6 +6,35 @@ namespace Wayscriber.Tools.Tests; public sealed class RepositoryContractTests { + [Fact] + public async Task SharedDependencyCommandsEnforceTheSharedSyntaxCorpus( ) + { + var root = FindRepository( ); + using var corpus = JsonDocument.Parse( File.ReadAllText( Path.Combine( root, "tools/shared-dependency-fixtures.json" ) ) ); + var command = ChecksCommand.Commands.Single( command => command.Name == CommandNames.SharedDependencies ); + foreach ( var fixture in corpus.RootElement.EnumerateArray( ) ) + { + using var directory = new TemporaryDirectory( "wayscriber-shared-dependency-test" ); + Directory.CreateDirectory( Path.Combine( directory.Path, "src/domain" ) ); + Directory.CreateDirectory( Path.Combine( directory.Path, "src/config/validate" ) ); + var sourcePath = Path.Combine( directory.Path, fixture.GetProperty( "path" ).GetString( )! ); + Directory.CreateDirectory( Path.GetDirectoryName( sourcePath )! ); + File.WriteAllText( sourcePath, fixture.GetProperty( "source" ).GetString( ) ); + var context = new ToolContext( directory.Path, TextWriter.Null, TextWriter.Null, + new ProcessRunner( TextWriter.Null, TextWriter.Null ), CancellationToken.None ); + + var error = await Record.ExceptionAsync( ( ) => command.Handler( context, [] ) ); + Assert.True( (error is ToolException) == fixture.GetProperty( "reject" ).GetBoolean( ), + $"C# fixture {fixture.GetProperty( "name" ).GetString( )}: {error}" ); + Assert.True( error is null or ToolException ); + } + + var request = new ProcessRequest( "python3", [Path.Combine( root, "tools/test-shared-dependencies.py" )], root, + CaptureOutput: true, Trace: false ); + var result = await new ProcessRunner( TextWriter.Null, TextWriter.Null ).RunAsync( request, CancellationToken.None ); + Assert.Equal( ExitCodes.Success, result.ExitCode ); + } + [Fact] public void StandaloneInstallersRemainAvailableWithoutDotnet( ) { diff --git a/tools/csharp/Commands/ChecksCommand.cs b/tools/csharp/Commands/ChecksCommand.cs index 2420192fe..68228beb9 100644 --- a/tools/csharp/Commands/ChecksCommand.cs +++ b/tools/csharp/Commands/ChecksCommand.cs @@ -11,7 +11,7 @@ internal static class ChecksCommand [ "build-package-repos.sh", "build.sh", "bump-version.sh", "check-arch-installer-manifest.sh", "check-config-writers.py", "check-nixpkgs-recipe.py", "check-process-sites.py", - "check-rust-source-coverage.py", "check-shared-dependencies.py", "check-version-consistency.sh", + "check-rust-source-coverage.py", "check-shared-dependencies.py", "test-shared-dependencies.py", "check-version-consistency.sh", "code-health-report.sh", "create-release-tag.sh", "fetch-all-deps.sh", "install-configurator.sh", "install-gtk4-layer-shell.sh", "install.sh", "lint-and-test.sh", "package.sh", "publish-release-tag.sh", "reload-daemon.sh", "run.sh", "set-portal-shortcut.sh", "test-aur-desktop-assets.sh", "test-gtk-widgets.sh", @@ -38,8 +38,8 @@ private static Task SharedDependencies( ToolContext context, string[] args var errors = new List( ); foreach ( var (directory, forbidden) in new[] { - ("src/domain", "config|input|draw|backend|ui|session"), - ("src/config/validate", "input|backend"), + ("src/domain", new HashSet( ["config", "input", "draw", "backend", "ui", "session"] )), + ("src/config/validate", new HashSet( ["input", "backend"] )), } ) { foreach ( var path in Directory.EnumerateFiles( context.Path( directory.Split( '/' ) ), "*.rs", SearchOption.AllDirectories ) ) @@ -48,9 +48,8 @@ private static Task SharedDependencies( ToolContext context, string[] args { continue; } - var source = Regex.Replace( Files.Read( path ), @"/\*.*?\*/|//[^\n]*", string.Empty, RegexOptions.Singleline ); - var pattern = $@"crate\s*::\s*(?:{forbidden})\b|use\s+crate\s*::\s*\{{[^;]*\b(?:{forbidden})\s*::"; - if ( Regex.IsMatch( source, pattern ) ) + var relative = Path.GetRelativePath( context.RepositoryRoot, path ).Replace( Path.DirectorySeparatorChar, '/' ); + if ( SharedDependencyGuard.HasUpwardPath( Files.Read( path ), relative, forbidden ) ) { errors.Add( $"{Path.GetRelativePath( context.RepositoryRoot, path )}: upward dependency in shared layer" ); } diff --git a/tools/csharp/Commands/DevelopmentCommands.cs b/tools/csharp/Commands/DevelopmentCommands.cs index d614de18c..5fc724c12 100644 --- a/tools/csharp/Commands/DevelopmentCommands.cs +++ b/tools/csharp/Commands/DevelopmentCommands.cs @@ -233,7 +233,7 @@ private static async Task InstallDependencies( ToolContext context, string[ var repositoryPackages = new[] { "apt-utils", "dpkg-dev", "rpm", "createrepo-c", "rsync", "gnupg" }; IEnumerable? packages = profile switch { - "checks" => common.Concat( ["clang", "cmake", "libxkbcommon-x11-dev", "libegl1-mesa-dev", "libgles2-mesa-dev", "libdbus-1-dev", "libinput-dev", "libudev-dev", "libpixman-1-dev", "libxcb-randr0-dev"] ) + "checks" => common.Concat( ["poppler-utils", "dbus-daemon", "clang", "cmake", "libxkbcommon-x11-dev", "libegl1-mesa-dev", "libgles2-mesa-dev", "libdbus-1-dev", "libinput-dev", "libudev-dev", "libpixman-1-dev", "libxcb-randr0-dev"] ) .Concat( repositoryPackages ), "widgets" => common.Concat( ["weston", "dbus-x11", "python3", "libgl1-mesa-dri", "fonts-dejavu-core", "clang", "cmake", "libxkbcommon-x11-dev", "libegl1-mesa-dev", "libgles2-mesa-dev", "libdbus-1-dev", "libinput-dev", "libudev-dev", "libpixman-1-dev", "libxcb-randr0-dev"] ), "package" => common.Concat( ["rpm"] ), diff --git a/tools/csharp/Infrastructure/SharedDependencyGuard.cs b/tools/csharp/Infrastructure/SharedDependencyGuard.cs new file mode 100644 index 000000000..d63a81a35 --- /dev/null +++ b/tools/csharp/Infrastructure/SharedDependencyGuard.cs @@ -0,0 +1,136 @@ +using System.Text; +using System.Text.RegularExpressions; + +namespace Wayscriber.Tools; + +// Partial source guard: no alias resolution, macro expansion, or dependency graph. +internal static class SharedDependencyGuard +{ + private static readonly Regex NonCode = new( + "r(?#{0,16})\".*?\"\\k|\"(?:\\\\.|[^\"\\\\])*\"|'(?:\\\\.|[^'\\\\\\n])'|//[^\\n]*|/\\*", + RegexOptions.Singleline ); + private static readonly Regex Tokens = new( @"r#[A-Za-z_][A-Za-z_0-9]*|[A-Za-z_][A-Za-z_0-9]*|::|[{},;*]" ); + private static readonly Regex CommentMarkers = new( @"/\*|\*/" ); + + public static bool HasUpwardPath( string source, string relativePath, IReadOnlySet forbidden ) + { + var tokens = Tokens.Matches( StripNonCode( source ) ) + .Select( match => match.Value.StartsWith( "r#", StringComparison.Ordinal ) ? match.Value[2..] : match.Value ).ToArray( ); + var module = relativePath[..^3].Split( '/' ).Skip( 1 ).ToList( ); + if ( module[^1] == "mod" ) + { + module.RemoveAt( module.Count - 1 ); + } + + for ( var index = 0; index + 1 < tokens.Length; index++ ) + { + if ( tokens[index] is "crate" or "super" or "self" && tokens[index + 1] == "::" && + Traverse( tokens, index, module, forbidden ).Rejected ) + { + return true; + } + } + return false; + } + + private static (bool Rejected, int Index) Traverse( + string[] tokens, int index, List prefix, IReadOnlySet forbidden ) + { + var path = new List( prefix ); + while ( index < tokens.Length ) + { + var token = tokens[index]; + if ( token is "," or ";" or "}" or "as" ) + { + break; + } + if ( token == "{" ) + { + return TraverseGroup( tokens, index + 1, path, forbidden ); + } + if ( token == "crate" ) + { + path.Clear( ); + } + else if ( token == "super" && path.Count > 0 ) + { + path.RemoveAt( path.Count - 1 ); + } + else if ( token is not ("super" or "self" or "::" or "*") ) + { + path.Add( token ); + } + if ( path.Count > 0 && forbidden.Contains( path[0] ) ) + { + return (true, index); + } + index++; + if ( index >= tokens.Length || tokens[index] != "::" ) + { + break; + } + index++; + } + return (false, index); + } + + private static (bool Rejected, int Index) TraverseGroup( + string[] tokens, int index, List prefix, IReadOnlySet forbidden ) + { + while ( index < tokens.Length && tokens[index] != "}" ) + { + var result = Traverse( tokens, index, prefix, forbidden ); + if ( result.Rejected ) + { + return result; + } + index = result.Index; + if ( index < tokens.Length && tokens[index] == "as" ) + { + index += 2; + } + if ( index < tokens.Length && tokens[index] == "," ) + { + index++; + } + else if ( index < tokens.Length && tokens[index] != "}" ) + { + break; + } + } + return (false, index + 1); + } + + private static string StripNonCode( string source ) + { + var pieces = new StringBuilder( ); + var position = 0; + var match = NonCode.Match( source, position ); + while ( match.Success ) + { + pieces.Append( source, position, match.Index - position ); + position = match.Index + match.Length; + if ( match.Value == "/*" ) + { + position = SkipBlockComment( source, position ); + } + pieces.Append( ' ' ); + match = NonCode.Match( source, position ); + } + pieces.Append( source, position, source.Length - position ); + return pieces.ToString( ); + } + + private static int SkipBlockComment( string source, int position ) + { + var depth = 1; + var marker = CommentMarkers.Match( source, position ); + while ( depth > 0 && marker.Success ) + { + depth += marker.Value == "/*" ? 1 : -1; + position = marker.Index + marker.Length; + marker = CommentMarkers.Match( source, position ); + } + return depth > 0 ? source.Length : position; + } +} diff --git a/tools/csharp/includes.cs b/tools/csharp/includes.cs index bdd12ab74..307e21582 100644 --- a/tools/csharp/includes.cs +++ b/tools/csharp/includes.cs @@ -12,3 +12,5 @@ #:include Commands/PackagingCommands.cs #:include Commands/ReleaseAurCommands.cs #:include Commands/ReportCommands.cs + +#:include Infrastructure/SharedDependencyGuard.cs diff --git a/tools/lint-and-test.sh b/tools/lint-and-test.sh index abefd2f56..eaf298f6c 100755 --- a/tools/lint-and-test.sh +++ b/tools/lint-and-test.sh @@ -34,6 +34,7 @@ run_check ./tools/check-rust-source-coverage.py run_check ./tools/check-process-sites.py run_check ./tools/check-config-writers.py run_check ./tools/check-shared-dependencies.py +run_check ./tools/test-shared-dependencies.py run_check cargo fmt --all -- --check run_check cargo clippy --locked --workspace --all-targets --all-features -- -D warnings run_check cargo build --locked --workspace --all-features --bins diff --git a/tools/shared-dependency-fixtures.json b/tools/shared-dependency-fixtures.json new file mode 100644 index 000000000..df4c4523a --- /dev/null +++ b/tools/shared-dependency-fixtures.json @@ -0,0 +1,176 @@ +[ + { + "name": "direct", + "path": "src/domain/probe.rs", + "source": "use crate::input::Tool;", + "reject": true + }, + { + "name": "grouped leaf", + "path": "src/domain/probe.rs", + "source": "use crate::{input::Tool};", + "reject": true + }, + { + "name": "grouped module", + "path": "src/domain/probe.rs", + "source": "use crate::{input};", + "reject": true + }, + { + "name": "multiline alias", + "path": "src/domain/probe.rs", + "source": "use crate::{domain::Color,\n input as runtime};", + "reject": true + }, + { + "name": "nested group", + "path": "src/domain/probe.rs", + "source": "use crate::{domain::Color, input::{Tool, InputState}};", + "reject": true + }, + { + "name": "parent relative", + "path": "src/domain/probe.rs", + "source": "use super::super::input::Tool;", + "reject": true + }, + { + "name": "grouped relative", + "path": "src/domain/probe.rs", + "source": "use super::{super::input::Tool};", + "reject": true + }, + { + "name": "expression path", + "path": "src/domain/probe.rs", + "source": "fn f() { crate::backend::run(); }", + "reject": true + }, + { + "name": "allowed domain", + "path": "src/domain/probe.rs", + "source": "use crate::domain::{Color, tool::Tool};", + "reject": false + }, + { + "name": "allowed nested name", + "path": "src/domain/probe.rs", + "source": "use crate::domain::{input::Tool};", + "reject": false + }, + { + "name": "allowed sibling", + "path": "src/domain/probe.rs", + "source": "use super::input::Tool;", + "reject": false + }, + { + "name": "allowed self", + "path": "src/domain/probe.rs", + "source": "use self::input::Tool;", + "reject": false + }, + { + "name": "line comment", + "path": "src/domain/probe.rs", + "source": "// use crate::{input};\nuse crate::domain::Color;", + "reject": false + }, + { + "name": "block comment", + "path": "src/domain/probe.rs", + "source": "/* use super::super::input::Tool; */", + "reject": false + }, + { + "name": "nested comment", + "path": "src/domain/probe.rs", + "source": "/* outer /* inner */ use crate::input::Tool; */", + "reject": false + }, + { + "name": "string", + "path": "src/domain/probe.rs", + "source": "const S: &str = \"use crate::{input}; //\";", + "reject": false + }, + { + "name": "escaped string", + "path": "src/domain/probe.rs", + "source": "const S: &str = \"\\\" crate::input::Tool\";", + "reject": false + }, + { + "name": "raw string", + "path": "src/domain/probe.rs", + "source": "const S: &str = r#\"crate::input::Tool; /*\"#;", + "reject": false + }, + { + "name": "raw string hashes", + "path": "src/domain/probe.rs", + "source": "const S: &str = r###\"a \"# crate::input::Tool\"###;", + "reject": false + }, + { + "name": "byte string", + "path": "src/domain/probe.rs", + "source": "const S: &[u8] = b\"crate::input::Tool\";", + "reject": false + }, + { + "name": "lifetime and char", + "path": "src/domain/probe.rs", + "source": "fn f<'a>() { let c = '\"'; let t: &'a str = \"ok\"; }", + "reject": false + }, + { + "name": "nested parent relative", + "path": "src/domain/nested/probe.rs", + "source": "use super::super::super::{ui};", + "reject": true + }, + { + "name": "mod relative", + "path": "src/domain/nested/mod.rs", + "source": "use super::super::input::Tool;", + "reject": true + }, + { + "name": "validation upward", + "path": "src/config/validate/probe.rs", + "source": "use super::super::super::{input};", + "reject": true + }, + { + "name": "validation allowed config", + "path": "src/config/validate/probe.rs", + "source": "use crate::config::Config;", + "reject": false + }, + { + "name": "compatibility exception", + "path": "src/domain/tests.rs", + "source": "use crate::input::Tool;", + "reject": false + }, + { + "name": "raw module name", + "path": "src/domain/probe.rs", + "source": "use crate::r#input::Tool;", + "reject": true + }, + { + "name": "grouped raw module name", + "path": "src/domain/probe.rs", + "source": "use crate::{r#input};", + "reject": true + }, + { + "name": "allowed raw nested name", + "path": "src/domain/probe.rs", + "source": "use crate::domain::{r#input};", + "reject": false + } +] diff --git a/tools/test-shared-dependencies.py b/tools/test-shared-dependencies.py new file mode 100755 index 000000000..00d17fc03 --- /dev/null +++ b/tools/test-shared-dependencies.py @@ -0,0 +1,23 @@ +#!/usr/bin/env python3 +"""Exercise the standalone guard's actual command against shared syntax fixtures.""" +import json +from pathlib import Path +import shutil +import subprocess +import tempfile + +TOOLS = Path(__file__).resolve().parent +fixtures = json.loads((TOOLS / "shared-dependency-fixtures.json").read_text()) +for fixture in fixtures: + with tempfile.TemporaryDirectory(prefix="wayscriber-shared-dependencies-") as directory: + root = Path(directory) + (root / "tools").mkdir() + checker = root / "tools/check-shared-dependencies.py" + shutil.copyfile(TOOLS / checker.name, checker) + source = root / fixture["path"] + source.parent.mkdir(parents=True) + source.write_text(fixture["source"]) + result = subprocess.run(["python3", str(checker)], capture_output=True, text=True) + expected = 1 if fixture["reject"] else 0 + assert result.returncode == expected, (fixture["name"], result.returncode, result.stderr) +print(f"Standalone shared-dependency syntax fixtures passed ({len(fixtures)}).")