diff --git a/Cargo.lock b/Cargo.lock index 23e5e93b9..d706ef0f9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2030,7 +2030,7 @@ dependencies = [ [[package]] name = "noticenterctl" -version = "1.3.0" +version = "1.3.1" dependencies = [ "anyhow", "blake3", @@ -3702,7 +3702,7 @@ checksum = "ebc1c04c71510c7f702b52b7c350734c9ff1295c464a03335b00bb84fc54f853" [[package]] name = "unixnotis-center" -version = "1.3.0" +version = "1.3.1" dependencies = [ "anyhow", "async-channel", @@ -3737,7 +3737,7 @@ dependencies = [ [[package]] name = "unixnotis-core" -version = "1.3.0" +version = "1.3.1" dependencies = [ "anyhow", "blake3", @@ -3762,7 +3762,7 @@ dependencies = [ [[package]] name = "unixnotis-daemon" -version = "1.3.0" +version = "1.3.1" dependencies = [ "anyhow", "arc-swap", @@ -3793,7 +3793,7 @@ dependencies = [ [[package]] name = "unixnotis-installer" -version = "1.3.0" +version = "1.3.1" dependencies = [ "anyhow", "crossterm", @@ -3814,7 +3814,7 @@ dependencies = [ [[package]] name = "unixnotis-popups" -version = "1.3.0" +version = "1.3.1" dependencies = [ "anyhow", "async-channel", @@ -3828,6 +3828,7 @@ dependencies = [ "image", "proptest", "rustix", + "tempfile", "tokio", "tracing", "tracing-subscriber", @@ -3838,7 +3839,7 @@ dependencies = [ [[package]] name = "unixnotis-ui" -version = "1.3.0" +version = "1.3.1" dependencies = [ "glib-build-tools", "gtk4", diff --git a/Cargo.toml b/Cargo.toml index 878d04219..c1ca9642a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -11,7 +11,7 @@ members = [ resolver = "2" [workspace.package] -version = "1.3.0" +version = "1.3.1" edition = "2021" license = "MIT" diff --git a/crates/noticenterctl/src/app/local.rs b/crates/noticenterctl/src/app/local.rs index 667351431..494d0950a 100644 --- a/crates/noticenterctl/src/app/local.rs +++ b/crates/noticenterctl/src/app/local.rs @@ -1,26 +1,27 @@ //! Local command dispatch that does not require a running daemon -use std::path::PathBuf; - use anyhow::{Context, Result}; -use crate::cli::{Command, PresetCommand, ThemeCommand}; +use crate::cli::{Command, DevCommand, DoctorCommand}; -pub(super) fn handle_local_command( - command: Command, - mut run_css: impl FnMut(Option) -> Result<()>, - mut run_preset: impl FnMut(PresetCommand) -> Result<()>, - mut sync_session: impl FnMut(crate::cli::DoctorServiceManagerArg) -> Result<()>, - mut run_theme: impl FnMut(ThemeCommand) -> Result<()>, -) -> Result<()> { +pub(super) fn handle_local_command(command: Command) -> Result<()> { // Local commands remain available while the session bus or daemon is unavailable match command { - Command::CssCheck { config } => run_css(config), - Command::Preset { command } => run_preset(command).context("preset command failed"), - Command::SyncSessionEnvironment { service_manager } => sync_session(service_manager), - Command::Theme { command } => run_theme(command).context("theme command failed"), - // The caller routes daemon-backed commands before reaching this helper - _ => Ok(()), + Command::CssCheck { config } => crate::css_check::run(config), + Command::Preset { command } => { + crate::preset::run_preset(command).context("preset command failed") + } + Command::Theme { command } => crate::theme::run(command).context("theme command failed"), + Command::Doctor { + command: Some(DoctorCommand::RepairSession), + service_manager, + .. + } => crate::session_environment::sync(service_manager), + Command::Dev { + command: DevCommand::Logs, + } => crate::debug_logs::follow_debug_logs(), + // Incorrect routing must fail instead of reporting a successful no-op + other => anyhow::bail!("internal routing error: {other:?} is not a local command"), } } diff --git a/crates/noticenterctl/src/app/runner.rs b/crates/noticenterctl/src/app/runner.rs index 4b31d3c6f..02b03e667 100644 --- a/crates/noticenterctl/src/app/runner.rs +++ b/crates/noticenterctl/src/app/runner.rs @@ -5,55 +5,53 @@ use clap::Parser; use unixnotis_core::{ensure_control_api_version, log_session_bus_identity, ControlProxy}; use zbus::Connection; -use crate::cli::{Args, Command}; +use crate::cli::{Args, Command, ExecutionKind}; use super::local::handle_local_command; pub fn run() -> Result<()> { // Parse CLI arguments before any daemon work starts let args = Args::parse(); - let command = args.command; + run_command(args.command) +} + +pub(super) fn run_command(command: Command) -> Result<()> { // Semantic checks happen before runtime and D-Bus setup command.validate()?; - if command.is_synchronous() { - // Preset and CSS work should not pay for an unused asynchronous runtime - handle_local_command( - command, - crate::css_check::run, - crate::preset::run_preset, - crate::session_environment::sync, - crate::theme::run, - )?; - return Ok(()); + match command.execution_kind() { + ExecutionKind::LocalSync => handle_local_command(command), + ExecutionKind::LocalAsync | ExecutionKind::Daemon => { + // A current-thread runtime avoids a worker pool for short control commands + let runtime = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .context("build command runtime")?; + runtime.block_on(run_async(command)) + } } - - // A current-thread runtime avoids a worker pool for short control commands - let runtime = tokio::runtime::Builder::new_current_thread() - .enable_all() - .build() - .context("build command runtime")?; - runtime.block_on(run_async(command)) } -async fn run_async(command: Command) -> Result<()> { - if let Command::Doctor { - json, - verbose, - service_manager, - config, - } = command - { - // Doctor owns its D-Bus connection so one failed probe cannot stop later checks - return crate::doctor::run(json, verbose, service_manager, config).await; +pub(super) async fn run_async(command: Command) -> Result<()> { + match command { + Command::Doctor { + command: None, + json, + verbose, + service_manager, + config, + } => { + // Doctor owns its D-Bus connection so one failed probe cannot stop later checks + crate::doctor::run(json, verbose, service_manager, config).await + } + Command::Doctor { + command: Some(_), .. + } => anyhow::bail!("internal routing error: doctor repair reached async dispatcher"), + command => run_daemon(command).await, } +} - // Every remaining command is backed by the running daemon - debug_assert!( - !command.is_local_only(), - "local-only commands must return before D-Bus dispatch" - ); - +async fn run_daemon(command: Command) -> Result<()> { // Control commands need the session bus and the daemon proxy let connection = Connection::session() .await diff --git a/crates/noticenterctl/src/app/tests/local.rs b/crates/noticenterctl/src/app/tests/local.rs index 47f51d70a..74690d982 100644 --- a/crates/noticenterctl/src/app/tests/local.rs +++ b/crates/noticenterctl/src/app/tests/local.rs @@ -1,48 +1,12 @@ -use anyhow::Result; - use super::handle_local_command; use crate::cli::Command; #[test] -fn daemon_command_is_not_dispatched_to_local_handlers() { - let mut css_called = false; - let mut preset_called = false; - - handle_local_command( - Command::OpenPanel { debug: None }, - |_| { - css_called = true; - Ok(()) - }, - |_| { - preset_called = true; - Ok(()) - }, - |_| Ok(()), - |_| Ok(()), - ) - .expect("ignore daemon command in local dispatcher"); - - assert!(!css_called, "daemon command must not invoke CSS checks"); - assert!( - !preset_called, - "daemon command must not invoke preset handling" - ); -} - -#[test] -fn local_handler_error_is_returned_to_the_caller() { - let result = handle_local_command( - Command::CssCheck { config: None }, - |_| anyhow::bail!("CSS check failed"), - |_| -> Result<()> { Ok(()) }, - |_| -> Result<()> { Ok(()) }, - |_| -> Result<()> { Ok(()) }, - ); - - let error = result.expect_err("local command failure should be returned"); +fn daemon_command_fails_closed_in_local_dispatcher() { + let error = handle_local_command(Command::OpenPanel) + .expect_err("daemon command must fail in local dispatcher"); assert!( - error.to_string().contains("CSS check failed"), - "original local command error should remain visible" + error.to_string().contains("internal routing error"), + "routing failure should remain visible" ); } diff --git a/crates/noticenterctl/src/app/tests/runner.rs b/crates/noticenterctl/src/app/tests/runner.rs index 952ba46df..237867ab6 100644 --- a/crates/noticenterctl/src/app/tests/runner.rs +++ b/crates/noticenterctl/src/app/tests/runner.rs @@ -1,54 +1,49 @@ -use std::cell::Cell; +use std::str::FromStr; -use anyhow::Result; - -use crate::cli::{Command, PresetCommand}; +use crate::cli::{Command, DndDuration, DndState, DoctorCommand, DoctorServiceManagerArg}; use super::local::handle_local_command; +use super::runner::{run_async, run_command}; #[test] -fn handle_local_command_runs_css_check_branch() { - let css_called = Cell::new(false); - - handle_local_command( - Command::CssCheck { config: None }, - |config| { - assert!(config.is_none()); - css_called.set(true); - Ok(()) - }, - |_| -> Result<()> { panic!("preset runner should not be called for css check") }, - |_| -> Result<()> { panic!("session runner should not be called for css check") }, - |_| -> Result<()> { panic!("theme runner should not be called for css check") }, - ) - .expect("css check should dispatch"); - - assert!(css_called.get()); +fn async_doctor_report_fails_closed_in_sync_local_dispatcher() { + let command = Command::Doctor { + command: None, + json: false, + verbose: false, + service_manager: DoctorServiceManagerArg::Auto, + config: None, + }; + let error = handle_local_command(command) + .expect_err("async doctor report must fail in synchronous dispatcher"); + + assert!(error.to_string().contains("internal routing error")); } #[test] -fn handle_local_command_runs_preset_branch_with_command_payload() { - let preset_called = Cell::new(false); - - handle_local_command( - Command::Preset { - command: PresetCommand::Inspect { - input: "theme.unixnotis".to_string(), - }, - }, - |_| -> Result<()> { panic!("css runner should not be called for preset command") }, - |command| { - let PresetCommand::Inspect { input } = command else { - panic!("expected inspect preset command"); - }; - assert_eq!(input, "theme.unixnotis"); - preset_called.set(true); - Ok(()) - }, - |_| -> Result<()> { panic!("session runner should not be called for preset command") }, - |_| -> Result<()> { panic!("theme runner should not be called for preset command") }, - ) - .expect("preset should dispatch"); - - assert!(preset_called.get()); +fn run_command_validates_semantics_before_starting_any_runtime_work() { + let command = Command::Dnd { + state: DndState::Off, + for_duration: Some(DndDuration::from_str("30m").expect("valid duration")), + until: None, + }; + let error = run_command(command).expect_err("invalid DND command must fail before dispatch"); + + assert!(error.to_string().contains("valid only with `dnd on`")); +} + +#[tokio::test] +async fn doctor_repair_fails_closed_in_async_dispatcher() { + let command = Command::Doctor { + command: Some(DoctorCommand::RepairSession), + json: false, + verbose: false, + service_manager: DoctorServiceManagerArg::Auto, + config: None, + }; + let error = run_async(command) + .await + .expect_err("synchronous repair must fail in async dispatcher"); + + assert!(error.to_string().contains("internal routing error")); } diff --git a/crates/noticenterctl/src/cli/args.rs b/crates/noticenterctl/src/cli/args.rs index 1312a77b9..25b95551a 100644 --- a/crates/noticenterctl/src/cli/args.rs +++ b/crates/noticenterctl/src/cli/args.rs @@ -68,6 +68,33 @@ pub enum DoctorServiceManagerArg { Manual, } +#[derive(Subcommand, Debug)] +pub enum DoctorCommand { + // Repair the daemon service environment for the active graphical session + RepairSession, +} + +#[derive(Subcommand, Debug)] +pub enum DevCommand { + // Open the panel with daemon diagnostic rendering enabled + OpenPanel { + #[arg(long, value_enum, default_value = "info")] + level: DebugLevelArg, + }, + // Rebuild the daemon desktop application index + RefreshApplications, + // Explain attribution and popup decisions for one active notification + ExplainNotification { + id: u32, + }, + // Print diagnostic detail for active notifications + DumpActive, + // Print diagnostic detail for saved notification history + DumpHistory, + // Follow daemon journal logs until interrupted + Logs, +} + impl InhibitScopeArg { pub(crate) const fn as_scope(self) -> u32 { // Map CLI scope to the daemon bitmask value diff --git a/crates/noticenterctl/src/cli/command.rs b/crates/noticenterctl/src/cli/command.rs index 019c3bc7d..d4c9c59c1 100644 --- a/crates/noticenterctl/src/cli/command.rs +++ b/crates/noticenterctl/src/cli/command.rs @@ -2,23 +2,30 @@ use std::path::PathBuf; use clap::Subcommand; -use super::args::{DndState, DoctorServiceManagerArg, PresetCommand, ThemeCommand}; -use super::{DebugLevelArg, InhibitScopeArg}; +use super::args::{ + DevCommand, DndState, DoctorCommand, DoctorServiceManagerArg, PresetCommand, ThemeCommand, +}; +use super::InhibitScopeArg; use super::{DndClockTime, DndDuration}; +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum ExecutionKind { + // Runs without Tokio or a daemon connection + LocalSync, + // Runs on Tokio but owns any D-Bus connections it needs + LocalAsync, + // Uses the shared daemon control bootstrap and API version check + Daemon, +} + #[derive(Subcommand, Debug)] pub enum Command { // Toggle the panel visibility without changing other state TogglePanel, - // Open the panel, optionally enabling debug logging for live diagnostics - OpenPanel { - #[arg(long, value_enum, num_args = 0..=1, default_missing_value = "info")] - debug: Option, - }, + // Open the panel without enabling diagnostic rendering + OpenPanel, // Close the panel if it is visible ClosePanel, - // Rebuild the daemon's desktop application index immediately - RefreshApplications, // Set or toggle Do Not Disturb mode Dnd { #[arg(value_enum)] @@ -30,8 +37,6 @@ pub enum Command { }, // Clear active notifications and saved history Clear, - // Clear active notifications and saved history - ClearAll, // Clear active notifications without deleting saved history ClearActive, // Clear saved history without closing active notifications @@ -40,20 +45,10 @@ pub enum Command { Dismiss { id: u32, }, - // Explain application identity and popup suppression for one active notification - ExplainNotification { - id: u32, - }, - // List active notifications; full output requires diagnostic mode - ListActive { - #[arg(long)] - full: bool, - }, - // List notification history; full output requires diagnostic mode - ListHistory { - #[arg(long)] - full: bool, - }, + // List active notifications using bounded terminal-safe output + ListActive, + // List notification history using bounded terminal-safe output + ListHistory, // Create a new inhibitor token Inhibit { reason: String, @@ -73,20 +68,17 @@ pub enum Command { }, // Collect independent configuration, theme, bus, service, and log diagnostics Doctor { + #[command(subcommand)] + command: Option, #[arg(long)] json: bool, #[arg(long)] verbose: bool, - #[arg(long, value_enum, default_value = "auto")] + #[arg(long, value_enum, default_value = "auto", global = true)] service_manager: DoctorServiceManagerArg, #[arg(long, value_name = "PATH")] config: Option, }, - // Import the compositor session environment and restart the installed user service - SyncSessionEnvironment { - #[arg(long, value_enum, default_value = "auto")] - service_manager: DoctorServiceManagerArg, - }, // Export, inspect, or import a shareable preset bundle Preset { #[command(subcommand)] @@ -97,6 +89,12 @@ pub enum Command { #[command(subcommand)] command: ThemeCommand, }, + // Keep maintenance commands discoverable only through explicit dev help + #[command(hide = true)] + Dev { + #[command(subcommand)] + command: DevCommand, + }, } impl Command { @@ -114,29 +112,47 @@ impl Command { )); } } - Ok(()) - } - pub(crate) const fn is_local_only(&self) -> bool { - // Local-only commands should not fail just because D-Bus is unavailable - matches!( - self, - Self::CssCheck { .. } - | Self::Doctor { .. } - | Self::Preset { .. } - | Self::Theme { .. } - | Self::SyncSessionEnvironment { .. } - ) + if let Self::Doctor { + command: Some(DoctorCommand::RepairSession), + json, + verbose, + service_manager, + config, + } = self + { + // Report-only options must never be accepted and then ignored during repair + if *json || *verbose || config.is_some() { + return Err(anyhow::anyhow!( + "--json, --verbose, and --config are not valid with `doctor repair-session`" + )); + } + + // Manual mode has no installed service whose environment can be repaired + if matches!(service_manager, DoctorServiceManagerArg::Manual) { + return Err(anyhow::anyhow!( + "--service-manager manual is not valid with `doctor repair-session`" + )); + } + } + Ok(()) } - pub(crate) const fn is_synchronous(&self) -> bool { - // Doctor uses local inputs but still needs asynchronous D-Bus and process timeouts - matches!( - self, + pub(crate) const fn execution_kind(&self) -> ExecutionKind { + // One classification prevents contradictory local and synchronous flags + match self { Self::CssCheck { .. } - | Self::Preset { .. } - | Self::Theme { .. } - | Self::SyncSessionEnvironment { .. } - ) + | Self::Preset { .. } + | Self::Theme { .. } + | Self::Doctor { + command: Some(DoctorCommand::RepairSession), + .. + } + | Self::Dev { + command: DevCommand::Logs, + } => ExecutionKind::LocalSync, + Self::Doctor { command: None, .. } => ExecutionKind::LocalAsync, + _ => ExecutionKind::Daemon, + } } } diff --git a/crates/noticenterctl/src/cli/mod.rs b/crates/noticenterctl/src/cli/mod.rs index 0aa906212..d5206227b 100644 --- a/crates/noticenterctl/src/cli/mod.rs +++ b/crates/noticenterctl/src/cli/mod.rs @@ -4,9 +4,11 @@ mod args; mod command; mod dnd; -pub use args::{Args, DndState, DoctorServiceManagerArg, PresetCommand, ThemeCommand}; -pub use args::{DebugLevelArg, InhibitScopeArg}; -pub use command::Command; +pub use args::InhibitScopeArg; +pub use args::{ + Args, DevCommand, DndState, DoctorCommand, DoctorServiceManagerArg, PresetCommand, ThemeCommand, +}; +pub use command::{Command, ExecutionKind}; pub use dnd::{DndClockTime, DndDuration}; #[cfg(test)] diff --git a/crates/noticenterctl/src/cli/tests/args.rs b/crates/noticenterctl/src/cli/tests/args.rs index 1b1755013..e1d904b6d 100644 --- a/crates/noticenterctl/src/cli/tests/args.rs +++ b/crates/noticenterctl/src/cli/tests/args.rs @@ -1,40 +1,71 @@ use clap::Parser; use unixnotis_core::{PanelDebugLevel, INHIBIT_SCOPE_ALL, INHIBIT_SCOPE_POPUPS}; +use super::super::args::DebugLevelArg; use super::super::{ - Args, Command, DebugLevelArg, DndState, DoctorServiceManagerArg, InhibitScopeArg, PresetCommand, + Args, Command, DevCommand, DndState, DoctorCommand, DoctorServiceManagerArg, InhibitScopeArg, + PresetCommand, }; #[test] -fn parses_open_panel_debug_default() { - // Ensures clap default_missing_value maps --debug to the Info level - let args = - Args::try_parse_from(["noticenterctl", "open-panel", "--debug"]).expect("parse args"); - match args.command { - Command::OpenPanel { debug } => { - assert!(matches!(debug, Some(DebugLevelArg::Info))); - } - other => panic!("unexpected command: {other:?}"), - } +fn parses_normal_open_panel_without_developer_options() { + let args = Args::try_parse_from(["noticenterctl", "open-panel"]).expect("parse open panel"); + assert!(matches!(args.command, Command::OpenPanel)); } #[test] -fn parses_refresh_applications() { - let args = Args::try_parse_from(["noticenterctl", "refresh-applications"]) - .expect("refresh command should parse"); - assert!(matches!(args.command, Command::RefreshApplications)); +fn parses_every_supported_dev_command() { + for level in ["critical", "warn", "info", "verbose"] { + let args = Args::try_parse_from(["noticenterctl", "dev", "open-panel", "--level", level]) + .expect("parse developer panel command"); + assert!(matches!( + args.command, + Command::Dev { + command: DevCommand::OpenPanel { .. } + } + )); + } + + for arguments in [ + vec!["noticenterctl", "dev", "refresh-applications"], + vec!["noticenterctl", "dev", "explain-notification", "42"], + vec!["noticenterctl", "dev", "dump-active"], + vec!["noticenterctl", "dev", "dump-history"], + vec!["noticenterctl", "dev", "logs"], + ] { + Args::try_parse_from(arguments).expect("developer command should parse"); + } } #[test] -fn parses_open_panel_debug_value() { - // Verifies explicit debug values map to the requested verbosity - let args = Args::try_parse_from(["noticenterctl", "open-panel", "--debug", "verbose"]) - .expect("parse args"); - match args.command { - Command::OpenPanel { debug } => { - assert!(matches!(debug, Some(DebugLevelArg::Verbose))); +fn dev_open_panel_defaults_to_info() { + let args = Args::try_parse_from(["noticenterctl", "dev", "open-panel"]) + .expect("parse default developer panel command"); + assert!(matches!( + args.command, + Command::Dev { + command: DevCommand::OpenPanel { + level: DebugLevelArg::Info + } } - other => panic!("unexpected command: {other:?}"), + )); +} + +#[test] +fn removed_root_interfaces_are_rejected_without_compatibility_aliases() { + for arguments in [ + vec!["noticenterctl", "clear-all"], + vec!["noticenterctl", "refresh-applications"], + vec!["noticenterctl", "explain-notification", "42"], + vec!["noticenterctl", "sync-session-environment"], + vec!["noticenterctl", "open-panel", "--debug"], + vec!["noticenterctl", "list-active", "--full"], + vec!["noticenterctl", "list-history", "--full"], + ] { + assert!( + Args::try_parse_from(arguments.clone()).is_err(), + "removed interface unexpectedly parsed: {arguments:?}" + ); } } @@ -104,14 +135,12 @@ fn timed_dnd_options_conflict_and_require_on_state_semantically() { fn parses_explicit_clear_variants() { for (name, expected) in [ ("clear", "clear"), - ("clear-all", "clear-all"), ("clear-active", "clear-active"), ("clear-history", "clear-history"), ] { let args = Args::try_parse_from(["noticenterctl", name]).expect("parse args"); match (args.command, expected) { (Command::Clear, "clear") - | (Command::ClearAll, "clear-all") | (Command::ClearActive, "clear-active") | (Command::ClearHistory, "clear-history") => {} (other, _) => panic!("unexpected command: {other:?}"), @@ -270,6 +299,7 @@ fn parses_doctor_output_and_service_manager_options() { assert!(matches!( args.command, Command::Doctor { + command: None, json: true, verbose: true, service_manager: DoctorServiceManagerArg::Dinit, @@ -279,10 +309,11 @@ fn parses_doctor_output_and_service_manager_options() { } #[test] -fn parses_session_environment_service_manager_without_shell_payloads() { +fn parses_doctor_repair_session_service_manager_without_shell_payloads() { let args = Args::try_parse_from([ "noticenterctl", - "sync-session-environment", + "doctor", + "repair-session", "--service-manager", "runit", ]) @@ -290,12 +321,46 @@ fn parses_session_environment_service_manager_without_shell_payloads() { assert!(matches!( args.command, - Command::SyncSessionEnvironment { + Command::Doctor { + command: Some(DoctorCommand::RepairSession), service_manager: DoctorServiceManagerArg::Runit, + json: false, + verbose: false, + config: None, } )); } +#[test] +fn doctor_repair_session_rejects_report_options_and_manual_service_mode() { + for arguments in [ + vec!["noticenterctl", "doctor", "--json", "repair-session"], + vec!["noticenterctl", "doctor", "--verbose", "repair-session"], + vec![ + "noticenterctl", + "doctor", + "--config", + "config.toml", + "repair-session", + ], + vec![ + "noticenterctl", + "doctor", + "repair-session", + "--service-manager", + "manual", + ], + ] { + let command = Args::try_parse_from(arguments.clone()) + .expect("syntax should parse before semantic validation") + .command; + assert!( + command.validate().is_err(), + "invalid repair options unexpectedly validated: {arguments:?}" + ); + } +} + #[test] fn doctor_and_css_check_accept_explicit_config_paths() { let doctor = Args::try_parse_from([ diff --git a/crates/noticenterctl/src/cli/tests/command.rs b/crates/noticenterctl/src/cli/tests/command.rs index a6c8c6dce..2592fc0dc 100644 --- a/crates/noticenterctl/src/cli/tests/command.rs +++ b/crates/noticenterctl/src/cli/tests/command.rs @@ -1,61 +1,95 @@ use clap::Parser; -use super::super::{Args, Command, DoctorServiceManagerArg, PresetCommand, ThemeCommand}; +use super::super::{ + Args, Command, DevCommand, DoctorCommand, DoctorServiceManagerArg, ExecutionKind, + PresetCommand, ThemeCommand, +}; #[test] -fn local_only_classification_distinguishes_local_and_control_commands() { - assert!(Command::CssCheck { config: None }.is_local_only()); - assert!(Command::Doctor { - json: false, - verbose: false, - service_manager: DoctorServiceManagerArg::Auto, - config: None, - } - .is_local_only()); - assert!(Command::Preset { - command: PresetCommand::Inspect { - input: "bundle.unixnotis".to_string() +fn execution_kind_distinguishes_sync_async_and_daemon_commands() { + assert_eq!( + Command::CssCheck { config: None }.execution_kind(), + ExecutionKind::LocalSync + ); + assert_eq!( + Command::Doctor { + command: None, + json: false, + verbose: false, + service_manager: DoctorServiceManagerArg::Auto, + config: None, } - } - .is_local_only()); - assert!(Command::Theme { - command: ThemeCommand::ExportStock { output: None } - } - .is_local_only()); + .execution_kind(), + ExecutionKind::LocalAsync + ); + assert_eq!( + Command::Doctor { + command: Some(DoctorCommand::RepairSession), + json: false, + verbose: false, + service_manager: DoctorServiceManagerArg::Auto, + config: None, + } + .execution_kind(), + ExecutionKind::LocalSync + ); + assert_eq!( + Command::Dev { + command: DevCommand::Logs, + } + .execution_kind(), + ExecutionKind::LocalSync + ); + assert_eq!( + Command::Dev { + command: DevCommand::DumpActive, + } + .execution_kind(), + ExecutionKind::Daemon + ); + assert_eq!(Command::ClearActive.execution_kind(), ExecutionKind::Daemon); +} - assert!(!Command::ClearActive.is_local_only()); +#[test] +fn preset_and_theme_commands_are_local_sync() { + assert_eq!( + Command::Preset { + command: PresetCommand::Inspect { + input: "bundle.unixnotis".to_string() + } + } + .execution_kind(), + ExecutionKind::LocalSync + ); + assert_eq!( + Command::Theme { + command: ThemeCommand::ExportStock { output: None } + } + .execution_kind(), + ExecutionKind::LocalSync + ); } #[test] -fn preset_commands_are_local_only() { - // Preset commands should bypass D-Bus setup like css-check does +fn parsed_preset_command_bypasses_daemon_bootstrap() { let args = Args::try_parse_from(["noticenterctl", "preset", "inspect", "bundle.unixnotis"]) .expect("parse args"); - assert!(args.command.is_local_only()); + assert_eq!(args.command.execution_kind(), ExecutionKind::LocalSync); } #[test] -fn synchronous_classification_builds_a_runtime_only_when_needed() { - assert!(Command::CssCheck { config: None }.is_synchronous()); - assert!(Command::Preset { - command: PresetCommand::Inspect { - input: "bundle.unixnotis".to_string() +fn doctor_report_is_local_async() { + assert_eq!( + Command::Doctor { + command: None, + json: false, + verbose: false, + service_manager: DoctorServiceManagerArg::Auto, + config: None, } - } - .is_synchronous()); - assert!(Command::Theme { - command: ThemeCommand::ExportStock { output: None } - } - .is_synchronous()); - - assert!(!Command::Doctor { - json: false, - verbose: false, - service_manager: DoctorServiceManagerArg::Auto, - config: None, - } - .is_synchronous()); - assert!(!Command::ClearActive.is_synchronous()); + .execution_kind(), + ExecutionKind::LocalAsync + ); } #[test] diff --git a/crates/noticenterctl/src/cli/tests/help.rs b/crates/noticenterctl/src/cli/tests/help.rs index cdebc3192..236586500 100644 --- a/crates/noticenterctl/src/cli/tests/help.rs +++ b/crates/noticenterctl/src/cli/tests/help.rs @@ -7,22 +7,45 @@ fn root_help_lists_the_supported_command_groups() { let help = Args::command().render_help().to_string(); assert!(help.contains("Usage:")); - assert!(help.contains("css-check")); - assert!(help.contains("doctor")); - assert!(help.contains("preset")); - assert!(help.contains("theme")); + for command in [ + "open-panel", + "dnd", + "clear", + "list-active", + "doctor", + "css-check", + "preset", + "theme", + ] { + assert!(help.contains(command), "missing {command} in {help}"); + } + + for internal_or_removed in [ + "dev", + "refresh-applications", + "explain-notification", + "sync-session-environment", + "clear-all", + ] { + assert!( + !help.contains(internal_or_removed), + "unexpected {internal_or_removed} in {help}" + ); + } } #[test] -fn command_help_lists_output_debug_and_preset_controls() { +fn command_help_lists_user_facing_controls() { for (arguments, expected) in [ ( vec!["noticenterctl", "doctor", "--help"], - vec!["--json", "--verbose", "--service-manager", "manual"], - ), - ( - vec!["noticenterctl", "open-panel", "--help"], - vec!["--debug", "critical", "verbose"], + vec![ + "repair-session", + "--json", + "--verbose", + "--service-manager", + "manual", + ], ), ( vec!["noticenterctl", "preset", "--help"], @@ -42,6 +65,34 @@ fn command_help_lists_output_debug_and_preset_controls() { } } +#[test] +fn dev_help_lists_technical_commands_when_requested_explicitly() { + let error = Args::try_parse_from(["noticenterctl", "dev", "--help"]) + .expect_err("help should stop parsing"); + let help = error.to_string(); + + for command in [ + "open-panel", + "refresh-applications", + "explain-notification", + "dump-active", + "dump-history", + "logs", + ] { + assert!(help.contains(command), "missing {command} in {help}"); + } +} + +#[test] +fn normal_open_panel_help_has_no_diagnostic_options() { + let error = Args::try_parse_from(["noticenterctl", "open-panel", "--help"]) + .expect_err("help should stop parsing"); + let help = error.to_string(); + + assert!(!help.contains("debug"), "unexpected debug option in {help}"); + assert!(!help.contains("level"), "unexpected level option in {help}"); +} + #[test] fn invalid_commands_and_dnd_values_are_rejected_by_the_parser() { let command = Args::try_parse_from(["noticenterctl", "definitely-not-a-command"]) diff --git a/crates/noticenterctl/src/dbus/commands.rs b/crates/noticenterctl/src/dbus/commands.rs index da52d3c32..f45d8a30f 100644 --- a/crates/noticenterctl/src/dbus/commands.rs +++ b/crates/noticenterctl/src/dbus/commands.rs @@ -1,24 +1,15 @@ use anyhow::Result; use unixnotis_core::util; -use crate::cli::{Command, DndState}; -use crate::debug_logs::follow_debug_logs; +use crate::cli::{Command, DevCommand, DndState}; use crate::output::{ - allow_full_output, print_inhibitors, print_notification_diagnostics, print_notifications, - warn_full_requires_diagnostic, write_stderr, write_stdout, + print_inhibitors, print_notification_diagnostics, print_notifications, require_diagnostic_mode, + write_stdout, }; use super::client::ControlClient; pub async fn handle_command(client: &impl ControlClient, command: Command) -> Result<()> { - handle_command_with_debug_logs(client, command, follow_debug_logs).await -} - -pub(super) async fn handle_command_with_debug_logs( - client: &impl ControlClient, - command: Command, - mut follow_logs: impl FnMut() -> Result<()>, -) -> Result<()> { // Keep library-level dispatch safe even when a caller bypasses the CLI runner command.validate()?; // CLI forwards work to the daemon @@ -27,27 +18,16 @@ pub(super) async fn handle_command_with_debug_logs( // Simple toggle keeps the daemon in control of its own visibility rules client.toggle_panel().await?; } - Command::OpenPanel { debug } => { - // Debug mode opens the panel and streams daemon logs for real-time triage - if let Some(level) = debug { - client.open_panel_debug(level.into()).await?; - // Panel open should still succeed when journal follow is unavailable - if let Err(err) = follow_logs() { - write_stderr(&format!("debug log follow unavailable: {err}\n"))?; - } - } else { - client.open_panel().await?; - } + Command::OpenPanel => { + // Normal panel opening never changes daemon diagnostic rendering + client.open_panel().await?; } Command::ClosePanel => { // Explicit close avoids accidental toggles when the panel is hidden client.close_panel().await?; } - Command::RefreshApplications => { - client.refresh_applications().await?; - } - Command::Clear | Command::ClearAll => { - // Clear keeps legacy behavior: remove active notifications and saved history + Command::Clear => { + // Clear removes active notifications and saved history through one daemon call client.clear_all().await?; } Command::ClearActive => { @@ -60,31 +40,15 @@ pub(super) async fn handle_command_with_debug_logs( // Dismiss targets a single notification by id client.dismiss(id).await?; } - Command::ExplainNotification { id } => { - let mut diagnostics = client.notification_diagnostics(id).await?; - let view = diagnostics - .pop() - .ok_or_else(|| anyhow::anyhow!("notification {id} is not active"))?; - print_notification_diagnostics(&view)?; - } - Command::ListActive { full } => { - let diagnostic_mode = util::diagnostic_mode(); - let allow_full = allow_full_output(full, diagnostic_mode); - if warn_full_requires_diagnostic(full, diagnostic_mode) { - // Fall back to the safe view - write_stderr("--full requires UNIXNOTIS_DIAGNOSTIC=1; using redacted output\n")?; - } + Command::ListActive => { + // Normal lists always use the compact bounded formatter let notifications = client.list_active().await?; - print_notifications("active", ¬ifications, allow_full)?; + print_notifications("active", ¬ifications, false)?; } - Command::ListHistory { full } => { - let diagnostic_mode = util::diagnostic_mode(); - let allow_full = allow_full_output(full, diagnostic_mode); - if warn_full_requires_diagnostic(full, diagnostic_mode) { - write_stderr("--full requires UNIXNOTIS_DIAGNOSTIC=1; using redacted output\n")?; - } + Command::ListHistory => { + // History follows the same safe default as the active list let notifications = client.list_history().await?; - print_notifications("history", ¬ifications, allow_full)?; + print_notifications("history", ¬ifications, false)?; } Command::Dnd { state, @@ -129,11 +93,66 @@ pub(super) async fn handle_command_with_debug_logs( let inhibitors = client.list_inhibitors().await?; print_inhibitors(&inhibitors)?; } + Command::Dev { + command: DevCommand::Logs, + } => { + anyhow::bail!("internal routing error: dev logs reached daemon dispatcher"); + } + Command::Dev { command } => { + // Developer D-Bus commands still use the normal client and timeout boundary + handle_dev_command(client, command).await?; + } Command::CssCheck { .. } | Command::Doctor { .. } | Command::Preset { .. } - | Command::Theme { .. } - | Command::SyncSessionEnvironment { .. } => {} + | Command::Theme { .. } => { + anyhow::bail!("internal routing error: local command reached daemon dispatcher"); + } + } + + Ok(()) +} + +async fn handle_dev_command(client: &impl ControlClient, command: DevCommand) -> Result<()> { + // Read diagnostic mode once so each command has one consistent security decision + handle_dev_command_with_diagnostic_mode(client, command, util::diagnostic_mode()).await +} + +pub(super) async fn handle_dev_command_with_diagnostic_mode( + client: &impl ControlClient, + command: DevCommand, + diagnostic_mode: bool, +) -> Result<()> { + match command { + DevCommand::OpenPanel { level } => { + // Debug rendering is independent from journal log following + client.open_panel_debug(level.into()).await?; + } + DevCommand::RefreshApplications => { + client.refresh_applications().await?; + } + DevCommand::ExplainNotification { id } => { + let mut diagnostics = client.notification_diagnostics(id).await?; + let view = diagnostics + .pop() + .ok_or_else(|| anyhow::anyhow!("notification {id} is not active"))?; + // Structured formatting keeps client-controlled text terminal-safe and bounded + print_notification_diagnostics(&view)?; + } + DevCommand::DumpActive => { + // Reject before fetching data so a denied dump has no daemon side effects + require_diagnostic_mode(diagnostic_mode)?; + let notifications = client.list_active().await?; + print_notifications("active", ¬ifications, true)?; + } + DevCommand::DumpHistory => { + require_diagnostic_mode(diagnostic_mode)?; + let notifications = client.list_history().await?; + print_notifications("history", ¬ifications, true)?; + } + DevCommand::Logs => { + anyhow::bail!("internal routing error: dev logs reached daemon dispatcher"); + } } Ok(()) diff --git a/crates/noticenterctl/src/dbus/tests/commands.rs b/crates/noticenterctl/src/dbus/tests/commands.rs index 8de5a8d16..a7de9a28b 100644 --- a/crates/noticenterctl/src/dbus/tests/commands.rs +++ b/crates/noticenterctl/src/dbus/tests/commands.rs @@ -1,18 +1,17 @@ -use anyhow::anyhow; +use clap::Parser; use unixnotis_core::PanelDebugLevel; use crate::cli::{ - Command, DebugLevelArg, DndState, DoctorServiceManagerArg, InhibitScopeArg, PresetCommand, + Args, Command, DevCommand, DndState, DoctorServiceManagerArg, InhibitScopeArg, PresetCommand, }; -use super::super::commands::{handle_command, handle_command_with_debug_logs}; -use super::support::{RecordedCall, RecordedEvent, RecordingControlClient}; +use super::super::commands::{handle_command, handle_dev_command_with_diagnostic_mode}; +use super::support::{RecordedCall, RecordingControlClient}; #[tokio::test] async fn clear_commands_dispatch_to_matching_control_calls() { let cases = [ (Command::Clear, RecordedCall::ClearAll), - (Command::ClearAll, RecordedCall::ClearAll), (Command::ClearActive, RecordedCall::ClearActive), (Command::ClearHistory, RecordedCall::ClearHistory), ]; @@ -30,12 +29,8 @@ async fn clear_commands_dispatch_to_matching_control_calls() { async fn panel_commands_dispatch_to_matching_control_calls() { let cases = [ (Command::TogglePanel, RecordedCall::TogglePanel), - (Command::OpenPanel { debug: None }, RecordedCall::OpenPanel), + (Command::OpenPanel, RecordedCall::OpenPanel), (Command::ClosePanel, RecordedCall::ClosePanel), - ( - Command::RefreshApplications, - RecordedCall::RefreshApplications, - ), ]; for (command, expected) in cases { @@ -48,55 +43,39 @@ async fn panel_commands_dispatch_to_matching_control_calls() { } #[tokio::test] -async fn debug_open_dispatches_debug_panel_and_attempts_log_follow() { - let client = RecordingControlClient::default(); - - handle_command_with_debug_logs( - &client, - Command::OpenPanel { - debug: Some(DebugLevelArg::Verbose), - }, - || { - client.record_debug_log_follow(); - Ok(()) - }, - ) - .await - .expect("dispatch debug open"); - - assert_eq!( - client.take_events(), - vec![ - RecordedEvent::Control(RecordedCall::OpenPanelDebug(PanelDebugLevel::Verbose)), - RecordedEvent::DebugLogFollow, - ] - ); -} - -#[tokio::test] -async fn debug_open_still_succeeds_when_log_follow_fails() { - let client = RecordingControlClient::default(); +async fn dev_commands_dispatch_to_exact_control_calls() { + // Build the debug command through Clap so the test covers the public CLI path + let parsed = Args::try_parse_from(["noticenterctl", "dev", "open-panel", "--level", "verbose"]) + .expect("parse developer panel command"); + let Command::Dev { + command: open_panel, + } = parsed.command + else { + panic!("developer panel command should be selected"); + }; - handle_command_with_debug_logs( - &client, - Command::OpenPanel { - debug: Some(DebugLevelArg::Warn), - }, - || { - client.record_debug_log_follow(); - Err(anyhow!("journal unavailable")) - }, - ) - .await - .expect("debug open should survive log follow errors"); + let cases = [ + ( + open_panel, + RecordedCall::OpenPanelDebug(PanelDebugLevel::Verbose), + ), + ( + DevCommand::RefreshApplications, + RecordedCall::RefreshApplications, + ), + ( + DevCommand::ExplainNotification { id: 8 }, + RecordedCall::NotificationDiagnostics(8), + ), + ]; - assert_eq!( - client.take_events(), - vec![ - RecordedEvent::Control(RecordedCall::OpenPanelDebug(PanelDebugLevel::Warn)), - RecordedEvent::DebugLogFollow, - ] - ); + for (command, expected) in cases { + let client = RecordingControlClient::default(); + handle_command(&client, Command::Dev { command }) + .await + .expect("dispatch developer command"); + assert_eq!(client.take_calls(), vec![expected]); + } } #[tokio::test] @@ -186,18 +165,8 @@ async fn timed_dnd_dispatch_rejects_non_on_state_without_calling_control() { async fn notification_commands_dispatch_to_matching_control_calls() { let cases = [ (Command::Dismiss { id: 7 }, RecordedCall::Dismiss(7)), - ( - Command::ExplainNotification { id: 8 }, - RecordedCall::NotificationDiagnostics(8), - ), - ( - Command::ListActive { full: false }, - RecordedCall::ListActive, - ), - ( - Command::ListHistory { full: false }, - RecordedCall::ListHistory, - ), + (Command::ListActive, RecordedCall::ListActive), + (Command::ListHistory, RecordedCall::ListHistory), ]; for (command, expected) in cases { @@ -236,10 +205,40 @@ async fn inhibitor_commands_dispatch_to_matching_control_calls() { } #[tokio::test] -async fn local_commands_do_not_touch_control_client() { +async fn diagnostic_dumps_require_diagnostic_mode_before_fetching_data() { + for command in [DevCommand::DumpActive, DevCommand::DumpHistory] { + let client = RecordingControlClient::default(); + let error = handle_dev_command_with_diagnostic_mode(&client, command, false) + .await + .expect_err("diagnostic dump should be rejected"); + + assert!(error.to_string().contains("UNIXNOTIS_DIAGNOSTIC=1")); + assert!(client.take_calls().is_empty()); + } +} + +#[tokio::test] +async fn diagnostic_dumps_fetch_matching_data_when_mode_is_enabled() { + let cases = [ + (DevCommand::DumpActive, RecordedCall::ListActive), + (DevCommand::DumpHistory, RecordedCall::ListHistory), + ]; + + for (command, expected) in cases { + let client = RecordingControlClient::default(); + handle_dev_command_with_diagnostic_mode(&client, command, true) + .await + .expect("diagnostic dump should dispatch"); + assert_eq!(client.take_calls(), vec![expected]); + } +} + +#[tokio::test] +async fn local_commands_fail_closed_without_touching_control_client() { let cases = [ Command::CssCheck { config: None }, Command::Doctor { + command: None, json: false, verbose: false, service_manager: DoctorServiceManagerArg::Auto, @@ -254,9 +253,26 @@ async fn local_commands_do_not_touch_control_client() { for command in cases { let client = RecordingControlClient::default(); - handle_command(&client, command) + let error = handle_command(&client, command) .await - .expect("dispatch command"); + .expect_err("local command must fail in daemon dispatcher"); + assert!(error.to_string().contains("internal routing error")); assert!(client.take_calls().is_empty()); } } + +#[tokio::test] +async fn dev_logs_fail_closed_without_touching_control_client() { + let client = RecordingControlClient::default(); + let error = handle_command( + &client, + Command::Dev { + command: DevCommand::Logs, + }, + ) + .await + .expect_err("local log follower must fail in daemon dispatcher"); + + assert!(error.to_string().contains("internal routing error")); + assert!(client.take_calls().is_empty()); +} diff --git a/crates/noticenterctl/src/dbus/tests/support.rs b/crates/noticenterctl/src/dbus/tests/support.rs index f8f6b553f..09f392af2 100644 --- a/crates/noticenterctl/src/dbus/tests/support.rs +++ b/crates/noticenterctl/src/dbus/tests/support.rs @@ -28,41 +28,21 @@ pub(super) enum RecordedCall { ListInhibitors, } -#[derive(Debug, PartialEq, Eq)] -pub(super) enum RecordedEvent { - Control(RecordedCall), - DebugLogFollow, -} - #[derive(Default)] pub(super) struct RecordingControlClient { - events: RefCell>, + calls: RefCell>, } impl RecordingControlClient { fn record<'a, T: 'a>(&'a self, call: RecordedCall, value: T) -> ControlFuture<'a, T> { Box::pin(async move { - self.events.borrow_mut().push(RecordedEvent::Control(call)); + self.calls.borrow_mut().push(call); Ok(value) }) } - pub(super) fn record_debug_log_follow(&self) { - self.events.borrow_mut().push(RecordedEvent::DebugLogFollow); - } - - pub(super) fn take_events(&self) -> Vec { - self.events.replace(Vec::new()) - } - pub(super) fn take_calls(&self) -> Vec { - self.take_events() - .into_iter() - .filter_map(|event| match event { - RecordedEvent::Control(call) => Some(call), - RecordedEvent::DebugLogFollow => None, - }) - .collect() + self.calls.replace(Vec::new()) } } @@ -138,12 +118,10 @@ impl ControlClient for RecordingControlClient { fn inhibit<'a>(&'a self, reason: &'a str, scope: u32) -> ControlFuture<'a, u64> { Box::pin(async move { - self.events - .borrow_mut() - .push(RecordedEvent::Control(RecordedCall::Inhibit { - reason: reason.to_owned(), - scope, - })); + self.calls.borrow_mut().push(RecordedCall::Inhibit { + reason: reason.to_owned(), + scope, + }); Ok(42) }) } diff --git a/crates/noticenterctl/src/output/gate.rs b/crates/noticenterctl/src/output/gate.rs index 772cc1e9a..61aa21ddd 100644 --- a/crates/noticenterctl/src/output/gate.rs +++ b/crates/noticenterctl/src/output/gate.rs @@ -1,7 +1,8 @@ -pub const fn allow_full_output(requested: bool, diagnostic_mode: bool) -> bool { - requested && diagnostic_mode -} +pub fn require_diagnostic_mode(diagnostic_mode: bool) -> anyhow::Result<()> { + // The dev namespace is discoverability only and never grants diagnostic access + if !diagnostic_mode { + anyhow::bail!("diagnostic notification output requires UNIXNOTIS_DIAGNOSTIC=1"); + } -pub const fn warn_full_requires_diagnostic(requested: bool, diagnostic_mode: bool) -> bool { - requested && !diagnostic_mode + Ok(()) } diff --git a/crates/noticenterctl/src/output/mod.rs b/crates/noticenterctl/src/output/mod.rs index 248d34ff0..be78a28f7 100644 --- a/crates/noticenterctl/src/output/mod.rs +++ b/crates/noticenterctl/src/output/mod.rs @@ -8,6 +8,6 @@ mod writer; pub use diagnostics::print_notification_diagnostics; pub use error::format_cli_error; -pub use gate::{allow_full_output, warn_full_requires_diagnostic}; +pub use gate::require_diagnostic_mode; pub use notifications::{print_inhibitors, print_notifications}; pub use writer::{write_stderr, write_stdout}; diff --git a/crates/noticenterctl/src/output/tests/notifications.rs b/crates/noticenterctl/src/output/tests/notifications.rs index 79c47bd18..1a7d39c66 100644 --- a/crates/noticenterctl/src/output/tests/notifications.rs +++ b/crates/noticenterctl/src/output/tests/notifications.rs @@ -1,7 +1,7 @@ use unixnotis_core::{Action, NotificationImage, NotificationView}; use super::{format_inhibitors, format_notifications}; -use crate::output::{allow_full_output, warn_full_requires_diagnostic}; +use crate::output::require_diagnostic_mode; fn sample_notification() -> NotificationView { // Bad bytes on purpose @@ -60,17 +60,9 @@ fn format_inhibitors_sanitizes_reason_and_owner() { } #[test] -fn full_output_requires_request_and_diagnostic_mode() { - assert!(allow_full_output(true, true)); - assert!(!allow_full_output(true, false)); - assert!(!allow_full_output(false, true)); - assert!(!allow_full_output(false, false)); -} +fn diagnostic_output_gate_requires_explicit_diagnostic_mode() { + assert!(require_diagnostic_mode(true).is_ok()); -#[test] -fn full_output_warning_only_when_full_was_requested_without_diagnostic_mode() { - assert!(warn_full_requires_diagnostic(true, false)); - assert!(!warn_full_requires_diagnostic(true, true)); - assert!(!warn_full_requires_diagnostic(false, false)); - assert!(!warn_full_requires_diagnostic(false, true)); + let error = require_diagnostic_mode(false).expect_err("disabled mode must reject full output"); + assert!(error.to_string().contains("UNIXNOTIS_DIAGNOSTIC=1")); } diff --git a/crates/unixnotis-core/src/config/layout/popup.rs b/crates/unixnotis-core/src/config/layout/popup.rs index 115a7c7e4..03f107e1d 100644 --- a/crates/unixnotis-core/src/config/layout/popup.rs +++ b/crates/unixnotis-core/src/config/layout/popup.rs @@ -17,6 +17,7 @@ pub struct PopupConfig { pub max_visible: usize, pub default_timeout_ms: u64, pub critical_timeout_ms: Option, + pub pause_on_hover: bool, pub allow_click_through: bool, pub output: Option, } @@ -36,6 +37,7 @@ impl Default for PopupConfig { max_visible: 3, default_timeout_ms: 5000, critical_timeout_ms: None, + pause_on_hover: true, allow_click_through: false, output: None, } diff --git a/crates/unixnotis-core/src/config/layout/tests/popup.rs b/crates/unixnotis-core/src/config/layout/tests/popup.rs index fc24e3622..ca43d90d6 100644 --- a/crates/unixnotis-core/src/config/layout/tests/popup.rs +++ b/crates/unixnotis-core/src/config/layout/tests/popup.rs @@ -18,3 +18,12 @@ fn popup_defaults_include_edge_clearance_for_card_shadow() { assert_eq!(popup.margin.top, 14); assert_eq!(popup.margin.bottom, 14); } + +#[test] +fn popup_config_without_hover_setting_enables_pause_for_upgrade_compatibility() { + let popup: PopupConfig = toml::from_str("width = 420") + .expect("popup config written before hover pause should parse"); + + assert!(popup.pause_on_hover); + assert_eq!(popup.width, 420); +} diff --git a/crates/unixnotis-core/src/config/loading/io/tests/blue_light.rs b/crates/unixnotis-core/src/config/loading/io/tests/blue_light.rs index 3d443bdf6..67e2115ad 100644 --- a/crates/unixnotis-core/src/config/loading/io/tests/blue_light.rs +++ b/crates/unixnotis-core/src/config/loading/io/tests/blue_light.rs @@ -124,7 +124,7 @@ fn night_mode_falls_through_when_the_first_installed_backend_cannot_stay_running .env("PATH", root.join("bin")) .env("TEST_LOG", &log) .env("TEST_MARKER", &marker) - .env("UNIXNOTIS_BLUE_LIGHT_STARTUP_DELAY", "0.05") + .env("UNIXNOTIS_BLUE_LIGHT_STARTUP_DELAY", "0.5") .status() .expect("start first healthy blue-light backend"); diff --git a/crates/unixnotis-core/src/config/reset/tests/renderer.rs b/crates/unixnotis-core/src/config/reset/tests/renderer.rs index 2684b8168..ec28a2ea8 100644 --- a/crates/unixnotis-core/src/config/reset/tests/renderer.rs +++ b/crates/unixnotis-core/src/config/reset/tests/renderer.rs @@ -6,4 +6,5 @@ fn reset_uses_the_same_annotated_default_renderer() { let rendered = render_default_config_toml(&Config::default()).expect("render defaults"); assert!(rendered.contains("# Exact pixel height override")); assert!(rendered.contains("# Disable panel animation")); + assert!(rendered.contains("pause_on_hover = true")); } diff --git a/crates/unixnotis-installer/src/actions/install/installer_lock.rs b/crates/unixnotis-installer/src/actions/install/installer_lock.rs index c981835e8..5198d9ab0 100644 --- a/crates/unixnotis-installer/src/actions/install/installer_lock.rs +++ b/crates/unixnotis-installer/src/actions/install/installer_lock.rs @@ -6,6 +6,7 @@ use std::path::Path; use anyhow::{anyhow, Context, Result}; use rustix::fs::{flock, open, FlockOperation, Mode, OFlags}; +use rustix::io::retry_on_intr; use rustix::process::geteuid; const INSTALLER_LOCK_FILE: &str = "unixnotis-installer.lock"; @@ -13,6 +14,10 @@ const INSTALLER_LOCK_FILE: &str = "unixnotis-installer.lock"; #[derive(Debug)] pub struct InstallerLock { // Retaining the descriptor retains the kernel lock for the complete action + #[expect( + clippy::used_underscore_binding, + reason = "the underscore documents that the descriptor is retained for lock ownership" + )] _file: File, } @@ -59,6 +64,17 @@ impl InstallerLock { } } +impl Drop for InstallerLock { + fn drop(&mut self) { + // Explicitly release the lock before closing the descriptor + // + // flock locks belong to the open-file description, so a descriptor + // inherited across fork can otherwise keep the lock alive after this + // guard's File is dropped. CLOEXEC only takes effect at exec, not fork + let _ = retry_on_intr(|| flock(&self._file, FlockOperation::Unlock)); + } +} + const fn owned_expected_object(expected_kind: bool, actual_uid: u32, effective_uid: u32) -> bool { expected_kind && actual_uid == effective_uid } diff --git a/crates/unixnotis-installer/src/actions/install/service/tests/readiness.rs b/crates/unixnotis-installer/src/actions/install/service/tests/readiness.rs index a71790ebc..433861080 100644 --- a/crates/unixnotis-installer/src/actions/install/service/tests/readiness.rs +++ b/crates/unixnotis-installer/src/actions/install/service/tests/readiness.rs @@ -12,9 +12,7 @@ async fn installer_rejects_process_start_success_without_dbus_readiness() { .expect("fake service start command should run"); assert!(status.success()); - let probes = AtomicUsize::new(0); let error = wait_until_ready_with_probe(Duration::from_millis(15), || { - probes.fetch_add(1, Ordering::Relaxed); std::future::ready(Err(anyhow!("both required names have no owner"))) }) .await @@ -22,7 +20,6 @@ async fn installer_rejects_process_start_success_without_dbus_readiness() { assert!(error.to_string().contains("readiness timed out")); assert!(error.to_string().contains("both required names")); - assert!(probes.load(Ordering::Relaxed) >= 2); } #[tokio::test] diff --git a/crates/unixnotis-installer/src/actions/install/tests/installer_lock.rs b/crates/unixnotis-installer/src/actions/install/tests/installer_lock.rs index 57f451730..6da63da3d 100644 --- a/crates/unixnotis-installer/src/actions/install/tests/installer_lock.rs +++ b/crates/unixnotis-installer/src/actions/install/tests/installer_lock.rs @@ -24,8 +24,21 @@ fn second_installer_cannot_acquire_the_same_action_lock() { .contains("another UnixNotis installer action is already running"), "unexpected contention error: {error:#}" ); + + // Simulate a descriptor inherited across fork. `dup` refers to the same + // open-file description and therefore shares the flock + let inherited_descriptor = + rustix::io::dup(&first._file).expect("duplicate action-lock descriptor"); + drop(first); - InstallerLock::acquire_at(&lock_path).expect("released action lock"); + + // Dropping the guard must explicitly unlock the open-file description, + // even while another descriptor referring to it remains alive + let reacquired = InstallerLock::acquire_at(&lock_path).expect("released action lock"); + + drop(reacquired); + drop(inherited_descriptor); + std::fs::remove_dir_all(root).expect("remove lock fixture"); } diff --git a/crates/unixnotis-installer/src/service_manager/backends/runit.rs b/crates/unixnotis-installer/src/service_manager/backends/runit.rs index 0e58c229a..25a762be3 100644 --- a/crates/unixnotis-installer/src/service_manager/backends/runit.rs +++ b/crates/unixnotis-installer/src/service_manager/backends/runit.rs @@ -118,7 +118,7 @@ pub fn stop_for_reinstall_command(artifact_root: &Path) -> CommandSpec { } pub fn hyprland_startup_commands(_artifact_root: &Path, _import_vars: &[&str]) -> Vec { - vec!["noticenterctl sync-session-environment --service-manager runit".to_string()] + vec!["noticenterctl doctor repair-session --service-manager runit".to_string()] } pub const fn environment_sync_commands() -> Vec { diff --git a/crates/unixnotis-installer/src/service_manager/backends/s6.rs b/crates/unixnotis-installer/src/service_manager/backends/s6.rs index 51e51ec4f..5ebee81ba 100644 --- a/crates/unixnotis-installer/src/service_manager/backends/s6.rs +++ b/crates/unixnotis-installer/src/service_manager/backends/s6.rs @@ -130,7 +130,7 @@ pub fn hyprland_startup_commands( _live_dir: &Path, _import_vars: &[&str], ) -> Vec { - vec!["noticenterctl sync-session-environment --service-manager s6".to_string()] + vec!["noticenterctl doctor repair-session --service-manager s6".to_string()] } pub const fn environment_sync_commands() -> Vec { diff --git a/crates/unixnotis-installer/src/service_manager/backends/tests/runit.rs b/crates/unixnotis-installer/src/service_manager/backends/tests/runit.rs index 2fc1da56e..81487b1a6 100644 --- a/crates/unixnotis-installer/src/service_manager/backends/tests/runit.rs +++ b/crates/unixnotis-installer/src/service_manager/backends/tests/runit.rs @@ -255,7 +255,7 @@ fn runit_backend_hyprland_startup_lines_update_envdir_and_restart() { assert_eq!(commands.len(), 1); assert_eq!( commands[0], - "noticenterctl sync-session-environment --service-manager runit" + "noticenterctl doctor repair-session --service-manager runit" ); assert!(!commands[0].contains("sh -lc")); } diff --git a/crates/unixnotis-installer/src/service_manager/backends/tests/s6.rs b/crates/unixnotis-installer/src/service_manager/backends/tests/s6.rs index afacca979..2b52724da 100644 --- a/crates/unixnotis-installer/src/service_manager/backends/tests/s6.rs +++ b/crates/unixnotis-installer/src/service_manager/backends/tests/s6.rs @@ -208,7 +208,7 @@ fn s6_backend_hyprland_startup_lines_update_envdir_and_start_service() { assert_eq!(commands.len(), 1); assert_eq!( commands[0], - "noticenterctl sync-session-environment --service-manager s6" + "noticenterctl doctor repair-session --service-manager s6" ); assert!(!commands[0].contains("sh -lc")); } diff --git a/crates/unixnotis-popups/Cargo.toml b/crates/unixnotis-popups/Cargo.toml index 2375055d4..4ec9e0720 100644 --- a/crates/unixnotis-popups/Cargo.toml +++ b/crates/unixnotis-popups/Cargo.toml @@ -25,3 +25,4 @@ unixnotis-ui = { path = "../unixnotis-ui" } [dev-dependencies] proptest.workspace = true +tempfile = "3" diff --git a/crates/unixnotis-popups/src/dbus/tests/types.rs b/crates/unixnotis-popups/src/dbus/tests/types.rs index e65b55944..800d403d3 100644 --- a/crates/unixnotis-popups/src/dbus/tests/types.rs +++ b/crates/unixnotis-popups/src/dbus/tests/types.rs @@ -40,3 +40,16 @@ fn reload_events_remain_distinct() { assert!(matches!(UiEvent::CssReload, UiEvent::CssReload)); assert!(matches!(UiEvent::ConfigReload, UiEvent::ConfigReload)); } + +#[test] +fn popup_hover_events_preserve_notification_generation_and_state() { + let key = NotificationKey { + id: 17, + generation: 23, + }; + + assert!(matches!( + UiEvent::PopupHoverChanged(key, true), + UiEvent::PopupHoverChanged(event_key, hovered) if event_key == key && hovered + )); +} diff --git a/crates/unixnotis-popups/src/dbus/types.rs b/crates/unixnotis-popups/src/dbus/types.rs index 4ddccb4aa..cadf3b8ea 100644 --- a/crates/unixnotis-popups/src/dbus/types.rs +++ b/crates/unixnotis-popups/src/dbus/types.rs @@ -19,6 +19,8 @@ pub enum UiEvent { NotificationClosed(NotificationKey, CloseReason), // Hiding a banner is local UI state and must not close the daemon record PopupHidden(NotificationKey), + // Hover state retains generation identity so stale card callbacks are harmless + PopupHoverChanged(NotificationKey, bool), // Popup gate is split out so panel-only state changes do not wake the popup UI PopupGateChanged(PopupGateState), CssReload, diff --git a/crates/unixnotis-popups/src/ui/entry/build.rs b/crates/unixnotis-popups/src/ui/entry/build.rs index 822fad2b7..756d2af4a 100644 --- a/crates/unixnotis-popups/src/ui/entry/build.rs +++ b/crates/unixnotis-popups/src/ui/entry/build.rs @@ -8,6 +8,7 @@ use gtk::Align; use unixnotis_core::{hooks, NotificationKey, NotificationView}; use unixnotis_ui::CutCorner; +use super::super::popups::PopupHideTimer; use super::super::window::refresh_popup_input_region; use super::super::UiState; use super::activation::connect_default_action; @@ -28,10 +29,8 @@ pub(in crate::ui) struct PopupEntry { pub(in crate::ui) revealer: Option, pub(in crate::ui) root: Option, pub(in crate::ui) visibility: Option, - // The display timer hides only this popup row and never closes the daemon record - pub(in crate::ui) hide_timer: Option, - // GLib has already removed a source when its one-shot callback starts - pub(in crate::ui) hide_timer_fired: Option>>, + // Timer internals stay centralized in the popup timeout module + pub(in crate::ui) hide_timer: PopupHideTimer, } impl PopupEntry { @@ -46,8 +45,7 @@ impl PopupEntry { revealer: None, root: None, visibility: None, - hide_timer: None, - hide_timer_fired: None, + hide_timer: PopupHideTimer::new(), } } @@ -55,17 +53,6 @@ impl PopupEntry { // Both widgets must exist before stack operations can touch this row safely self.revealer.is_some() && self.root.is_some() } - pub(in crate::ui) fn cancel_hide_timer(&mut self) { - let fired = self - .hide_timer_fired - .take() - .is_some_and(|state| state.get()); - if let Some(timer) = self.hide_timer.take() { - if !fired { - timer.remove(); - } - } - } } impl UiState { @@ -84,8 +71,7 @@ impl UiState { revealer: Some(revealer), root: Some(root), visibility: Some(visibility), - hide_timer: None, - hide_timer_fired: None, + hide_timer: PopupHideTimer::new(), } } @@ -122,6 +108,9 @@ impl UiState { connect_close_action(&close, notification.key(), &self.command_tx); connect_default_action(&root, notification.key(), &view, &self.command_tx); + if let Some(sender) = self.popup_event_tx.as_ref() { + connect_hover_events(&root, notification.key(), sender); + } root } @@ -180,6 +169,73 @@ impl UiState { } } +fn connect_hover_events( + root: >k::Box, + key: NotificationKey, + sender: &async_channel::Sender, +) { + // One controller on the full card avoids enter/leave churn between child widgets + let motion = gtk::EventControllerMotion::new(); + let latest_hovered = Rc::new(Cell::new(false)); + let dispatch_pending = Rc::new(Cell::new(false)); + let entered_latest = Rc::clone(&latest_hovered); + let entered_pending = Rc::clone(&dispatch_pending); + let entered_sender = sender.clone(); + motion.connect_enter(move |_, _, _| { + queue_hover_event( + &entered_sender, + key, + &entered_latest, + &entered_pending, + true, + ); + }); + let left_latest = Rc::clone(&latest_hovered); + let left_pending = Rc::clone(&dispatch_pending); + let left_sender = sender.clone(); + motion.connect_leave(move |_| { + queue_hover_event(&left_sender, key, &left_latest, &left_pending, false); + }); + root.add_controller(motion); +} + +fn queue_hover_event( + sender: &async_channel::Sender, + key: NotificationKey, + latest_hovered: &Rc>, + dispatch_pending: &Rc>, + hovered: bool, +) { + // Keep only the newest pointer state while one bounded-channel send is waiting + latest_hovered.set(hovered); + if dispatch_pending.replace(true) { + return; + } + + let sender = sender.clone(); + let latest_hovered = Rc::clone(latest_hovered); + let dispatch_pending = Rc::clone(dispatch_pending); + glib::MainContext::default().spawn_local(async move { + loop { + let hovered = latest_hovered.get(); + if sender + .send(crate::dbus::UiEvent::PopupHoverChanged(key, hovered)) + .await + .is_err() + { + // A closed UI queue cannot accept a final state + dispatch_pending.set(false); + return; + } + if latest_hovered.get() == hovered { + // No state changed while the bounded send was waiting + dispatch_pending.set(false); + return; + } + } + }); +} + fn build_card_root(view: &PopupEntryViewModel) -> gtk::Box { let root = gtk::Box::new(gtk::Orientation::Vertical, 6); root.add_css_class("unixnotis-popup-card"); diff --git a/crates/unixnotis-popups/src/ui/entry/tests/build.rs b/crates/unixnotis-popups/src/ui/entry/tests/build.rs index 02e2e6d8c..259de7f59 100644 --- a/crates/unixnotis-popups/src/ui/entry/tests/build.rs +++ b/crates/unixnotis-popups/src/ui/entry/tests/build.rs @@ -1,4 +1,5 @@ -use super::connect_close_action; +use super::{connect_close_action, connect_hover_events}; +use crate::dbus::UiEvent; use crate::ui::entry::activation::connect_default_action; use gtk::prelude::*; use unixnotis_core::{ @@ -74,6 +75,82 @@ fn nondefault_action_does_not_make_the_whole_card_clickable() { assert_eq!(root.observe_controllers().n_items(), 0); } +#[gtk::test] +fn card_level_motion_controller_emits_generation_keyed_hover_state() { + let root = gtk::Box::new(gtk::Orientation::Vertical, 0); + let notification = notification(); + let (event_tx, event_rx) = async_channel::bounded(2); + connect_hover_events(&root, notification.key(), &event_tx); + + let controllers = root.observe_controllers(); + assert_eq!(controllers.n_items(), 1); + let motion = controllers + .item(0) + .and_downcast::() + .expect("card controller should handle pointer motion"); + motion.emit_by_name::<()>("enter", &[&0.0_f64, &0.0_f64]); + while gtk::glib::MainContext::default().pending() { + gtk::glib::MainContext::default().iteration(false); + } + + assert!(matches!( + event_rx.try_recv().expect("pointer enter event"), + UiEvent::PopupHoverChanged(key, true) if key == notification.key() + )); + + motion.emit_by_name::<()>("leave", &[]); + while gtk::glib::MainContext::default().pending() { + gtk::glib::MainContext::default().iteration(false); + } + assert!(matches!( + event_rx.try_recv().expect("pointer leave event"), + UiEvent::PopupHoverChanged(key, false) if key == notification.key() + )); +} + +#[gtk::test] +fn saturated_hover_queue_coalesces_to_one_final_pointer_state() { + let root = gtk::Box::new(gtk::Orientation::Vertical, 0); + let notification = notification(); + let (event_tx, event_rx) = async_channel::bounded(1); + event_tx + .try_send(UiEvent::CssReload) + .expect("prefill hover event queue"); + connect_hover_events(&root, notification.key(), &event_tx); + let motion = root + .observe_controllers() + .item(0) + .and_downcast::() + .expect("card controller should handle pointer motion"); + + for _ in 0..64 { + motion.emit_by_name::<()>("enter", &[&0.0_f64, &0.0_f64]); + motion.emit_by_name::<()>("leave", &[]); + } + while gtk::glib::MainContext::default().pending() { + gtk::glib::MainContext::default().iteration(false); + } + assert!(matches!( + event_rx.try_recv().expect("prefilled event"), + UiEvent::CssReload + )); + while gtk::glib::MainContext::default().pending() { + gtk::glib::MainContext::default().iteration(false); + } + + assert!(matches!( + event_rx.try_recv().expect("coalesced final hover state"), + UiEvent::PopupHoverChanged(key, false) if key == notification.key() + )); + while gtk::glib::MainContext::default().pending() { + gtk::glib::MainContext::default().iteration(false); + } + assert!( + event_rx.try_recv().is_err(), + "coalescing must not leave more pointer sends waiting" + ); +} + pub(super) fn notification() -> NotificationView { NotificationView { id: 31, diff --git a/crates/unixnotis-popups/src/ui/popups/mod.rs b/crates/unixnotis-popups/src/ui/popups/mod.rs index 81bed33ee..e7ea5fc10 100644 --- a/crates/unixnotis-popups/src/ui/popups/mod.rs +++ b/crates/unixnotis-popups/src/ui/popups/mod.rs @@ -4,3 +4,5 @@ mod mutation; mod reconcile; mod timeout; mod visibility; + +pub(in crate::ui) use timeout::PopupHideTimer; diff --git a/crates/unixnotis-popups/src/ui/popups/mutation.rs b/crates/unixnotis-popups/src/ui/popups/mutation.rs index 3c0e5b649..89df9b9d6 100644 --- a/crates/unixnotis-popups/src/ui/popups/mutation.rs +++ b/crates/unixnotis-popups/src/ui/popups/mutation.rs @@ -215,7 +215,7 @@ impl UiState { pub(super) fn remove_popup_internal(&mut self, id: u32, refresh_visibility: bool) { if let Some(entry) = self.popups.remove(&id) { let mut entry = entry; - entry.cancel_hide_timer(); + entry.clear_hide_state(); if let Some(revealer) = entry.revealer { // Visible rows animate out before leaving the stack revealer.set_reveal_child(false); @@ -328,6 +328,11 @@ impl UiState { pub(super) fn dematerialize_popup(&mut self, id: u32) { // Hidden rows keep only plain Rust data so backlog size does not scale GTK memory + let key = self.popups.get(&id).map(|entry| entry.notification.key()); + if let Some(key) = key { + // Once the card leaves the pointer domain, any active pause must end + self.resume_popup_hide(key); + } let Some(entry) = self.popups.get_mut(&id) else { return; }; diff --git a/crates/unixnotis-popups/src/ui/popups/tests/timeout.rs b/crates/unixnotis-popups/src/ui/popups/tests/timeout.rs index ca394b238..26d6b7fc4 100644 --- a/crates/unixnotis-popups/src/ui/popups/tests/timeout.rs +++ b/crates/unixnotis-popups/src/ui/popups/tests/timeout.rs @@ -1,6 +1,7 @@ -use std::time::Duration; +use std::time::{Duration, Instant}; use super::super::timeout::popup_display_timeout; +use crate::ui::entry::PopupEntry; use unixnotis_core::{Config, NotificationImage, NotificationView, Urgency}; fn notification(timeout_ms: u64, urgency: Urgency) -> NotificationView { @@ -60,3 +61,90 @@ fn zero_display_timeout_disables_local_hiding() { None ); } + +#[test] +fn hover_pause_records_only_the_unconsumed_lifetime() { + let started_at = Instant::now(); + let mut entry = PopupEntry::queued(notification(5_000, Urgency::Normal), 0); + entry.prepare_hide_timer(Duration::from_secs(5), started_at); + + assert!(entry.pause_hide_timer(started_at + Duration::from_secs(2))); + assert!(entry.hide_timer_is_paused()); + assert_eq!(entry.resume_hide_timer(), Some(Duration::from_secs(3))); +} + +#[test] +fn hover_resume_uses_remaining_time_instead_of_original_timeout() { + let started_at = Instant::now(); + let mut entry = PopupEntry::queued(notification(5_000, Urgency::Normal), 0); + entry.prepare_hide_timer(Duration::from_secs(5), started_at); + assert!(entry.pause_hide_timer(started_at + Duration::from_millis(4_800))); + + assert_eq!(entry.resume_hide_timer(), Some(Duration::from_millis(200))); + assert!(!entry.hide_timer_is_paused()); + assert_eq!( + entry.resume_hide_timer(), + None, + "duplicate leave must be a no-op" + ); +} + +#[test] +fn repeated_enter_does_not_consume_or_replace_saved_remaining_time() { + let started_at = Instant::now(); + let mut entry = PopupEntry::queued(notification(5_000, Urgency::Normal), 0); + entry.prepare_hide_timer(Duration::from_secs(5), started_at); + assert!(entry.pause_hide_timer(started_at + Duration::from_secs(1))); + + assert!(!entry.pause_hide_timer(started_at + Duration::from_secs(3))); + assert_eq!(entry.resume_hide_timer(), Some(Duration::from_secs(4))); +} + +#[test] +fn repeated_pause_resume_cycles_only_decrease_lifetime() { + let started_at = Instant::now(); + let mut entry = PopupEntry::queued(notification(5_000, Urgency::Normal), 0); + entry.prepare_hide_timer(Duration::from_secs(5), started_at); + assert!(entry.pause_hide_timer(started_at + Duration::from_secs(1))); + let first_remaining = entry.resume_hide_timer().expect("first resume duration"); + + let resumed_at = started_at + Duration::from_secs(10); + entry.prepare_hide_timer(first_remaining, resumed_at); + assert!(entry.pause_hide_timer(resumed_at + Duration::from_secs(2))); + let second_remaining = entry.resume_hide_timer().expect("second resume duration"); + + assert_eq!(first_remaining, Duration::from_secs(4)); + assert_eq!(second_remaining, Duration::from_secs(2)); + assert!(second_remaining < first_remaining); +} + +#[test] +fn pause_at_or_after_deadline_saturates_to_zero_without_panicking() { + let started_at = Instant::now(); + let mut entry = PopupEntry::queued(notification(1, Urgency::Normal), 0); + entry.prepare_hide_timer(Duration::from_millis(1), started_at); + + assert!(entry.pause_hide_timer(started_at + Duration::from_millis(2))); + assert_eq!(entry.resume_hide_timer(), Some(Duration::ZERO)); +} + +#[test] +fn queued_and_zero_timeout_entries_have_no_timer_state() { + let mut entry = PopupEntry::queued(notification(0, Urgency::Normal), 0); + + assert!(!entry.pause_hide_timer(Instant::now())); + assert_eq!(entry.resume_hide_timer(), None); +} + +#[test] +fn clearing_a_paused_entry_removes_all_timer_state() { + let started_at = Instant::now(); + let mut entry = PopupEntry::queued(notification(5_000, Urgency::Normal), 0); + entry.prepare_hide_timer(Duration::from_secs(5), started_at); + assert!(entry.pause_hide_timer(started_at + Duration::from_secs(1))); + + entry.clear_hide_state(); + + assert!(!entry.hide_timer_is_paused()); + assert_eq!(entry.resume_hide_timer(), None); +} diff --git a/crates/unixnotis-popups/src/ui/popups/timeout.rs b/crates/unixnotis-popups/src/ui/popups/timeout.rs index d49cb94fc..1e6d30301 100644 --- a/crates/unixnotis-popups/src/ui/popups/timeout.rs +++ b/crates/unixnotis-popups/src/ui/popups/timeout.rs @@ -1,6 +1,6 @@ //! Local popup display timers -use std::time::Duration; +use std::time::{Duration, Instant}; use std::{cell::Cell, rc::Rc}; use unixnotis_core::{NotificationKey, NotificationView}; @@ -9,6 +9,28 @@ use crate::dbus::UiEvent; use super::super::UiState; +pub(in crate::ui) struct PopupHideTimer { + // GLib source removal is valid only before its one-shot callback starts + source: Option, + callback_fired: Option>>, + // Monotonic time avoids wall-clock changes while a popup is visible + deadline: Option, + remaining: Option, + paused: bool, +} + +impl PopupHideTimer { + pub(in crate::ui) const fn new() -> Self { + Self { + source: None, + callback_fired: None, + deadline: None, + remaining: None, + paused: false, + } + } +} + pub(super) fn popup_display_timeout(notification: &NotificationView) -> Option { // The daemon has already resolved protocol, urgency, rule, and resident policy let timeout_ms = notification.popup_hide_after_ms; @@ -19,10 +41,10 @@ pub(super) fn popup_display_timeout(notification: &NotificationView) -> Option>(); + for key in paused { + self.resume_popup_hide(key); + } + } +} + +impl super::super::entry::PopupEntry { + pub(in crate::ui) fn clear_hide_state(&mut self) { + self.cancel_hide_source(); + self.hide_timer.deadline = None; + self.hide_timer.remaining = None; + self.hide_timer.paused = false; + } + + pub(super) fn prepare_hide_timer(&mut self, duration: Duration, now: Instant) { + self.clear_hide_state(); + self.hide_timer.deadline = now.checked_add(duration); + } + + fn install_hide_timer(&mut self, source: glib::SourceId, fired: Rc>) { + self.hide_timer.source = Some(source); + self.hide_timer.callback_fired = Some(fired); + } + + pub(super) fn pause_hide_timer(&mut self, now: Instant) -> bool { + if self.hide_timer.paused || self.hide_timer_callback_fired() { + return false; + } + let Some(deadline) = self.hide_timer.deadline.take() else { + return false; + }; + self.hide_timer.remaining = Some(deadline.saturating_duration_since(now)); + self.cancel_hide_source(); + self.hide_timer.paused = true; + true + } + + pub(super) const fn resume_hide_timer(&mut self) -> Option { + if !self.hide_timer.paused { + return None; + } + self.hide_timer.paused = false; + self.hide_timer.remaining.take() + } + + pub(in crate::ui) const fn hide_timer_is_paused(&self) -> bool { + self.hide_timer.paused + } + + fn hide_timer_callback_fired(&self) -> bool { + self.hide_timer + .callback_fired + .as_ref() + .is_some_and(|state| state.get()) + } + + fn cancel_hide_source(&mut self) { + let fired = self + .hide_timer + .callback_fired + .take() + .is_some_and(|state| state.get()); + if let Some(source) = self.hide_timer.source.take() { + if !fired { + source.remove(); + } + } } } diff --git a/crates/unixnotis-popups/src/ui/state/events.rs b/crates/unixnotis-popups/src/ui/state/events.rs index c23308f2d..aa2364f26 100644 --- a/crates/unixnotis-popups/src/ui/state/events.rs +++ b/crates/unixnotis-popups/src/ui/state/events.rs @@ -53,6 +53,15 @@ impl UiState { ); self.hide_popup_if_generation(key); } + UiEvent::PopupHoverChanged(key, hovered) => { + // Timer code validates materialization, config, and exact generation + if hovered { + self.pause_popup_hide(key); + } else { + // Stale leave state cannot resume a replacement generation + self.resume_popup_hide(key); + } + } UiEvent::PopupGateChanged(gate) => { // Gate updates change only policy fields and preserve unrelated daemon state apply_popup_gate(&mut self.control_state, gate); @@ -97,6 +106,8 @@ impl UiState { // Config, generated CSS, and geometry move forward as one accepted snapshot self.config = config.clone(); + // Config can disable pointer handling while a card is still paused + self.resume_ineligible_hover_pauses(); debug!("popup config reloaded"); self.css.update_theme(theme_paths, config.theme.clone()); let report = self.css.reload(css::DEFAULT_CSS); diff --git a/crates/unixnotis-popups/src/ui/state/tests/events.rs b/crates/unixnotis-popups/src/ui/state/tests/events.rs index 2cc8b2213..dd174f393 100644 --- a/crates/unixnotis-popups/src/ui/state/tests/events.rs +++ b/crates/unixnotis-popups/src/ui/state/tests/events.rs @@ -1,6 +1,13 @@ -use unixnotis_core::{ControlState, PopupGateState}; +use gtk::prelude::*; +use unixnotis_core::{ + render_default_config_toml, Config, ControlState, NotificationImage, NotificationView, + PopupGateState, +}; +use unixnotis_ui::css::CssManager; -use super::super::events::apply_popup_gate; +use super::super::{events::apply_popup_gate, UiState}; +use super::support::theme_paths; +use crate::dbus::UiEvent; #[test] fn popup_gate_update_changes_policy_without_replacing_runtime_counts() { @@ -45,3 +52,57 @@ fn popup_gate_update_can_restore_normal_popup_policy() { assert!(!state.dnd_enabled); assert!(!state.inhibited); } + +#[gtk::test] +fn config_reload_disabling_hover_pause_resumes_an_existing_timer() { + let app = gtk::Application::builder() + .application_id("org.unixnotis.PopupHoverReloadEvent") + .flags(gtk::gio::ApplicationFlags::NON_UNIQUE) + .build(); + app.register(None::<>k::gio::Cancellable>) + .expect("register popup config-reload application"); + let config_root = tempfile::tempdir().expect("create popup config fixture"); + let config_path = config_root.path().join("config.toml"); + let mut initial = Config::default(); + initial.popups.max_visible = 1; + let mut reloaded = initial.clone(); + reloaded.popups.pause_on_hover = false; + let rendered = render_default_config_toml(&reloaded).expect("render popup config fixture"); + std::fs::write(&config_path, rendered).expect("write popup config fixture"); + let (command_tx, _command_rx) = tokio::sync::mpsc::channel(4); + let css = CssManager::new_popup(theme_paths(config_root.path()), initial.theme.clone()); + let mut state = UiState::new(&app, initial, config_path, command_tx, css); + let (event_tx, _event_rx) = async_channel::bounded(4); + state.set_popup_event_sender(event_tx); + let notification = hover_notification(); + state.add_popup(notification.clone()); + state.handle_event(UiEvent::PopupHoverChanged(notification.key(), true)); + assert!(state.popups[¬ification.id].hide_timer_is_paused()); + + state.handle_event(UiEvent::ConfigReload); + + assert!(!state.config.popups.pause_on_hover); + assert!(!state.popups[¬ification.id].hide_timer_is_paused()); + state.remove_popup_if_generation(notification.key()); +} + +fn hover_notification() -> NotificationView { + NotificationView { + id: 41, + generation: 1, + app_name: "Example".to_string(), + attribution: unixnotis_core::NotificationAttribution::default(), + summary: "Reload hover policy".to_string(), + body: "Body".to_string(), + actions: Vec::new(), + inline_reply: unixnotis_core::InlineReply::default(), + inline_reply_policy: unixnotis_core::InlineReplyPolicy::Deny, + urgency: 1, + category: String::new(), + is_transient: false, + received_at_unix_seconds: 0, + image: NotificationImage::default(), + popup_decision: unixnotis_core::PopupDecisionRecord::default(), + popup_hide_after_ms: 5_000, + } +} diff --git a/crates/unixnotis-popups/src/ui/state/tests/mutation.rs b/crates/unixnotis-popups/src/ui/state/tests/mutation.rs index 2c969f6c6..7702b108e 100644 --- a/crates/unixnotis-popups/src/ui/state/tests/mutation.rs +++ b/crates/unixnotis-popups/src/ui/state/tests/mutation.rs @@ -361,15 +361,19 @@ fn popup_display_timeout_hides_only_the_local_banner_generation() { assert!(state.popups.contains_key(&first.id)); assert_materialized_and_visible_commands(&mut command_rx, first.key()); - std::thread::sleep(std::time::Duration::from_millis(15)); - while gtk::glib::MainContext::default().pending() { - gtk::glib::MainContext::default().iteration(false); - } - - let hidden = event_rx - .try_recv() + let hidden = wait_for_ui_event(&event_rx, std::time::Duration::from_millis(500)) .expect("display timeout should emit a local hide event"); assert!(matches!(hidden, UiEvent::PopupHidden(key) if key == first.key())); + assert!( + event_rx.try_recv().is_err(), + "one timer must emit exactly one hide event" + ); + // A pointer event racing the fired callback must not remove an expired GLib source + state.handle_event(UiEvent::PopupHoverChanged(first.key(), true)); + assert!( + !state.popups[&first.id].hide_timer_is_paused(), + "a fired timer cannot be paused or rescheduled" + ); state.handle_event(hidden); assert!(!state.popups.contains_key(&first.id)); @@ -405,6 +409,160 @@ fn popup_display_timeout_hides_only_the_local_banner_generation() { ); } +#[gtk::test] +fn hover_events_pause_and_resume_only_the_remaining_timeout() { + let (mut state, _command_rx) = + popup_state_with_commands("org.unixnotis.PopupHoverRemaining", 1); + let (event_tx, _event_rx) = async_channel::bounded(4); + state.set_popup_event_sender(event_tx); + let mut popup = notification(32, 1, "hover timeout"); + popup.popup_hide_after_ms = 5_000; + state.add_popup(popup.clone()); + + state.handle_event(UiEvent::PopupHoverChanged(popup.key(), true)); + assert!(state.popups[&popup.id].hide_timer_is_paused()); + + state.handle_event(UiEvent::PopupHoverChanged(popup.key(), false)); + assert!(!state.popups[&popup.id].hide_timer_is_paused()); + + state.handle_event(UiEvent::PopupHoverChanged(popup.key(), false)); + assert!(!state.popups[&popup.id].hide_timer_is_paused()); + state.remove_popup_if_generation(popup.key()); +} + +#[gtk::test] +fn paused_glib_timer_stays_silent_until_the_saved_remainder_is_resumed() { + let (mut state, _command_rx) = + popup_state_with_commands("org.unixnotis.PopupHoverSourceLifecycle", 1); + let (event_tx, event_rx) = async_channel::bounded(4); + state.set_popup_event_sender(event_tx); + let mut popup = notification(38, 1, "source lifecycle"); + popup.popup_hide_after_ms = 40; + state.add_popup(popup.clone()); + + state.handle_event(UiEvent::PopupHoverChanged(popup.key(), true)); + assert!( + wait_for_ui_event(&event_rx, std::time::Duration::from_millis(80)).is_none(), + "cancelled source must not hide a hovered popup" + ); + + state.handle_event(UiEvent::PopupHoverChanged(popup.key(), false)); + let hidden = wait_for_ui_event(&event_rx, std::time::Duration::from_millis(500)) + .expect("resumed remainder should emit one hide event"); + assert!(matches!(hidden, UiEvent::PopupHidden(key) if key == popup.key())); + assert!(event_rx.try_recv().is_err()); + state.handle_event(hidden); +} + +#[gtk::test] +fn replacement_while_paused_gets_fresh_lifetime_and_rejects_stale_leave() { + let (mut state, _command_rx) = + popup_state_with_commands("org.unixnotis.PopupHoverReplacement", 1); + let (event_tx, _event_rx) = async_channel::bounded(4); + state.set_popup_event_sender(event_tx); + let mut original = notification(33, 1, "original"); + original.popup_hide_after_ms = 5_000; + state.add_popup(original.clone()); + state.handle_event(UiEvent::PopupHoverChanged(original.key(), true)); + assert!(state.popups[&original.id].hide_timer_is_paused()); + + let mut replacement = notification(33, 2, "replacement"); + replacement.popup_hide_after_ms = 7_000; + state.update_popup(replacement.clone(), true); + assert!(!state.popups[&replacement.id].hide_timer_is_paused()); + + state.handle_event(UiEvent::PopupHoverChanged(original.key(), false)); + assert!(!state.popups[&replacement.id].hide_timer_is_paused()); + state.remove_popup_if_generation(replacement.key()); +} + +#[gtk::test] +fn closing_a_paused_popup_removes_its_timer_state_with_the_entry() { + let (mut state, _command_rx) = popup_state_with_commands("org.unixnotis.PopupHoverDismiss", 1); + let (event_tx, _event_rx) = async_channel::bounded(4); + state.set_popup_event_sender(event_tx); + let mut popup = notification(34, 1, "dismiss paused"); + popup.popup_hide_after_ms = 5_000; + state.add_popup(popup.clone()); + state.handle_event(UiEvent::PopupHoverChanged(popup.key(), true)); + assert!(state.popups[&popup.id].hide_timer_is_paused()); + + state.handle_event(UiEvent::NotificationClosed( + popup.key(), + CloseReason::DismissedByUser, + )); + + assert!(!state.popups.contains_key(&popup.id)); +} + +#[gtk::test] +fn disabled_hover_pause_and_click_through_leave_timers_running() { + for (application_id, pause_on_hover, allow_click_through) in [ + ("org.unixnotis.PopupHoverDisabled", false, false), + ("org.unixnotis.PopupHoverClickThrough", true, true), + ] { + let (mut state, _command_rx) = popup_state_with_commands(application_id, 1); + state.config.popups.pause_on_hover = pause_on_hover; + state.config.popups.allow_click_through = allow_click_through; + let (event_tx, _event_rx) = async_channel::bounded(4); + state.set_popup_event_sender(event_tx); + let mut popup = notification(35, 1, "unpaused policy"); + popup.popup_hide_after_ms = 5_000; + state.add_popup(popup.clone()); + state.handle_event(UiEvent::PopupHoverChanged(popup.key(), true)); + + assert!(!state.popups[&popup.id].hide_timer_is_paused()); + state.remove_popup_if_generation(popup.key()); + } +} + +#[gtk::test] +fn config_policy_change_resumes_a_popup_that_was_already_paused() { + let (mut state, _command_rx) = + popup_state_with_commands("org.unixnotis.PopupHoverConfigReload", 1); + let (event_tx, _event_rx) = async_channel::bounded(4); + state.set_popup_event_sender(event_tx); + let mut popup = notification(39, 1, "config reload"); + popup.popup_hide_after_ms = 5_000; + state.add_popup(popup.clone()); + state.handle_event(UiEvent::PopupHoverChanged(popup.key(), true)); + assert!(state.popups[&popup.id].hide_timer_is_paused()); + + state.config.popups.allow_click_through = true; + state.resume_ineligible_hover_pauses(); + + assert!(!state.popups[&popup.id].hide_timer_is_paused()); + state.remove_popup_if_generation(popup.key()); +} + +#[gtk::test] +fn paused_popup_resumes_before_moving_to_hidden_backlog() { + let (mut state, _command_rx) = popup_state_with_commands("org.unixnotis.PopupHoverBacklog", 1); + let (event_tx, _event_rx) = async_channel::bounded(4); + state.set_popup_event_sender(event_tx); + let mut first = notification(36, 1, "first"); + first.popup_hide_after_ms = 5_000; + state.add_popup(first.clone()); + state.handle_event(UiEvent::PopupHoverChanged(first.key(), true)); + assert!(state.popups[&first.id].hide_timer_is_paused()); + + let mut second = notification(37, 1, "second"); + second.popup_hide_after_ms = 5_000; + state.add_popup(second.clone()); + + let backlogged = &state.popups[&first.id]; + assert!(!backlogged.is_materialized()); + assert!(!backlogged.hide_timer_is_paused()); + + state.handle_event(UiEvent::PopupHoverChanged(first.key(), true)); + assert!( + !state.popups[&first.id].hide_timer_is_paused(), + "a backlogged card cannot receive a real pointer hover" + ); + state.remove_popup_if_generation(first.key()); + state.remove_popup_if_generation(second.key()); +} + #[gtk::test] fn visible_popup_materialization_and_rebuild_replace_the_exact_widget_generation() { let (mut state, mut command_rx) = @@ -565,6 +723,34 @@ fn popup_state(application_id: &str) -> UiState { popup_state_with_commands(application_id, 0).0 } +fn wait_for_ui_event( + receiver: &async_channel::Receiver, + timeout: std::time::Duration, +) -> Option { + // Poll GTK work until the expected event arrives or the bounded deadline expires + let Some(deadline) = std::time::Instant::now().checked_add(timeout) else { + panic!("test timeout should fit in monotonic time"); + }; + loop { + while gtk::glib::MainContext::default().pending() { + gtk::glib::MainContext::default().iteration(false); + } + match receiver.try_recv() { + Ok(event) => return Some(event), + Err(async_channel::TryRecvError::Closed) => { + panic!("UI event channel closed while waiting") + } + Err(async_channel::TryRecvError::Empty) => {} + } + let remaining = deadline.saturating_duration_since(std::time::Instant::now()); + if remaining.is_zero() { + return None; + } + // Short parks avoid a busy loop without hiding a slow callback behind a fixed sleep + std::thread::park_timeout(remaining.min(std::time::Duration::from_millis(1))); + } +} + fn popup_state_with_commands( application_id: &str, max_visible: usize,