From 70b68c45000b1f161ab06fa63740b8c4385a26fa Mon Sep 17 00:00:00 2001 From: alberto tirla Date: Sun, 26 May 2024 20:14:42 +0300 Subject: [PATCH 1/9] logging: remove printing to stderr if environment variables for filter errors for other reasons than not found, since we give the values provided in the configuration anyway --- Cargo.lock | 1 - odilia/src/logging.rs | 13 ++++--------- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 089551d5..50819897 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1601,7 +1601,6 @@ dependencies = [ name = "odilia-cache" version = "0.3.0" dependencies = [ - "async-trait", "atspi", "atspi-client", "atspi-common 0.5.0", diff --git a/odilia/src/logging.rs b/odilia/src/logging.rs index 278b7387..3a0120da 100644 --- a/odilia/src/logging.rs +++ b/odilia/src/logging.rs @@ -15,15 +15,10 @@ use tracing_tree::HierarchicalLayer; /// Initialise the logging stack /// this requires an application configuration structure, so configuration must be initialized before logging is pub fn init(config: &ApplicationConfig) -> eyre::Result<()> { - let env_filter = - match env::var("APP_LOG").or_else(|_| env::var("RUST_LOG")) { - Ok(s) => EnvFilter::from(s), - Err(env::VarError::NotPresent) => EnvFilter::from(&config.log.level), - Err(e) => { - eprintln!("Warning: Failed to read log filter from APP_LOG or RUST_LOG: {e}"); - EnvFilter::from(&config.log.level) - } - }; + let env_filter = match env::var("APP_LOG").or_else(|_| env::var("RUST_LOG")) { + Ok(s) => EnvFilter::from(s), + _ => EnvFilter::from(&config.log.level), + }; //this requires boxing because the types returned by this match block would be incompatible otherwise, since we return different layers depending on what we get from the configuration. It is possible to do it otherwise, hopefully, but for now this and a forced dereference at the end would do let output_layer = match &config.log.logger { LoggingKind::File(path) => { From c6ca75e5fd0621efbd79cbf21972ece7b50d0074 Mon Sep 17 00:00:00 2001 From: alberto tirla Date: Sun, 26 May 2024 20:21:47 +0300 Subject: [PATCH 2/9] logging: use simplified `file::create` instead of providing all the options needlessly --- odilia/src/logging.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/odilia/src/logging.rs b/odilia/src/logging.rs index 3a0120da..a02fd627 100644 --- a/odilia/src/logging.rs +++ b/odilia/src/logging.rs @@ -22,13 +22,9 @@ pub fn init(config: &ApplicationConfig) -> eyre::Result<()> { //this requires boxing because the types returned by this match block would be incompatible otherwise, since we return different layers depending on what we get from the configuration. It is possible to do it otherwise, hopefully, but for now this and a forced dereference at the end would do let output_layer = match &config.log.logger { LoggingKind::File(path) => { - let file = std::fs::File::options() - .create_new(true) - .write(true) - .open(path) - .with_context(|| { - format!("creating log file '{}'", path.display()) - })?; + let file = std::fs::File::create(path).with_context(|| { + format!("creating log file '{}'", path.display()) + })?; let fmt = tracing_subscriber::fmt::layer().with_ansi(false).with_writer(file); fmt.boxed() From 5cffde3ca1814db6ba2dd7460713abd826354ea1 Mon Sep 17 00:00:00 2001 From: alberto tirla Date: Wed, 29 May 2024 12:43:23 +0300 Subject: [PATCH 3/9] logging: make output of logs not be sent in two places, both stderr and the logging method described in the configuration file --- odilia/src/logging.rs | 40 ++++++++++++++++++---------------------- 1 file changed, 18 insertions(+), 22 deletions(-) diff --git a/odilia/src/logging.rs b/odilia/src/logging.rs index a02fd627..ca954979 100644 --- a/odilia/src/logging.rs +++ b/odilia/src/logging.rs @@ -3,12 +3,11 @@ //! Not much here yet, but this will get more complex if we decide to add other layers for error //! reporting, tokio-console, etc. -use std::env; +use std::{env, io}; use eyre::Context; use odilia_common::settings::{log::LoggingKind, ApplicationConfig}; use tracing_error::ErrorLayer; -use tracing_log::LogTracer; use tracing_subscriber::{prelude::*, EnvFilter}; use tracing_tree::HierarchicalLayer; @@ -19,34 +18,31 @@ pub fn init(config: &ApplicationConfig) -> eyre::Result<()> { Ok(s) => EnvFilter::from(s), _ => EnvFilter::from(&config.log.level), }; - //this requires boxing because the types returned by this match block would be incompatible otherwise, since we return different layers depending on what we get from the configuration. It is possible to do it otherwise, hopefully, but for now this and a forced dereference at the end would do - let output_layer = match &config.log.logger { + let tree = HierarchicalLayer::new(4) + .with_bracketed_fields(true) + .with_targets(true) + .with_deferred_spans(true) + .with_span_retrace(true) + .with_indent_lines(true) + .with_ansi(false) + .with_wraparound(4); +//this requires boxing because the types returned by this match block would be incompatible otherwise, since we return different layers, or modifications to a layer depending on what we get from the configuration. It is possible to do it otherwise, hopefully, but for now this would do +let final_layer = match &config.log.logger { LoggingKind::File(path) => { let file = std::fs::File::create(path).with_context(|| { format!("creating log file '{}'", path.display()) })?; - let fmt = - tracing_subscriber::fmt::layer().with_ansi(false).with_writer(file); - fmt.boxed() + tree.with_writer(file).boxed() } - LoggingKind::Tty => tracing_subscriber::fmt::layer() - .with_ansi(true) - .with_target(true) + LoggingKind::Tty => tree.with_writer(io::stdout).with_ansi(true).boxed(), + LoggingKind::Syslog => tracing_journald::Layer::new()? + .with_syslog_identifier("odilia".to_owned()) .boxed(), - LoggingKind::Syslog => tracing_journald::layer()?.boxed(), }; - let subscriber = tracing_subscriber::Registry::default() + tracing_subscriber::Registry::default() .with(env_filter) - .with(output_layer) .with(ErrorLayer::default()) - .with(HierarchicalLayer::new(4) - .with_bracketed_fields(true) - .with_targets(true) - .with_deferred_spans(true) - .with_span_retrace(true) - .with_indent_lines(true)); - tracing::subscriber::set_global_default(subscriber) - .wrap_err("unable to init default logging layer")?; - LogTracer::init().wrap_err("unable to init tracing log layer")?; + .with(final_layer) + .init(); Ok(()) } From e83197e1e857cf287149c07ec49935c4df4f6715 Mon Sep 17 00:00:00 2001 From: alberto tirla Date: Wed, 29 May 2024 13:09:58 +0300 Subject: [PATCH 4/9] workspace: make xdg a workspace dependency --- Cargo.toml | 1 + odilia/Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 726d5c16..3541c739 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -53,3 +53,4 @@ tracing-tree = "^0.2.2" zbus = { version = "3.14.1", features = ["tokio"] } serde_plain = "1.0.1" +xdg = "2.5.2" diff --git a/odilia/Cargo.toml b/odilia/Cargo.toml index 794d7a4e..2ce5eb49 100644 --- a/odilia/Cargo.toml +++ b/odilia/Cargo.toml @@ -51,7 +51,7 @@ tracing-log.workspace = true tracing-subscriber.workspace = true tracing-tree.workspace = true tracing.workspace = true -xdg = "2.4.1" +xdg.workspace=true zbus.workspace = true odilia-notify = { version = "0.1.0", path = "../odilia-notify" } clap = { version = "4.5.1", features = ["derive"] } From f94736f1ea357fe50c618da05acc0437af9beb9a Mon Sep 17 00:00:00 2001 From: alberto tirla Date: Wed, 29 May 2024 13:44:13 +0300 Subject: [PATCH 5/9] config: make default logging path be in $XDG_STATE_HOME --- Cargo.lock | 1 + common/Cargo.toml | 1 + common/src/settings/log.rs | 4 +++- 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index 50819897..f66fef1b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1632,6 +1632,7 @@ dependencies = [ "serde_plain", "smartstring", "thiserror", + "xdg", "zbus", ] diff --git a/common/Cargo.toml b/common/Cargo.toml index 65ca163f..391396df 100644 --- a/common/Cargo.toml +++ b/common/Cargo.toml @@ -21,3 +21,4 @@ thiserror = "1.0.37" zbus.workspace = true serde_plain.workspace = true figment = "0.10.15" +xdg.workspace=true \ No newline at end of file diff --git a/common/src/settings/log.rs b/common/src/settings/log.rs index d687f976..a90517f7 100644 --- a/common/src/settings/log.rs +++ b/common/src/settings/log.rs @@ -16,9 +16,11 @@ pub struct LogSettings { } impl Default for LogSettings { fn default() -> Self { + let directories=xdg::BaseDirectories::with_prefix("odilia").expect("can't create required directories according to the xdg directory specification"); + let log_path = directories.place_state_file("odilia.log").expect("can't place log file"); Self { level: "info".to_owned(), - logger: LoggingKind::File("/var/log/odilia.log".into()), + logger: LoggingKind::File(log_path), } } } From 4c35c537d2cd5eba4816ae4253590f34d1ba4418 Mon Sep 17 00:00:00 2001 From: alberto tirla Date: Wed, 29 May 2024 14:10:21 +0300 Subject: [PATCH 6/9] config: make system have priority before user when a user value is not specified and make the toml writer to write all the layers before the user one in the configuration it writes to disk in case of missing configuration --- odilia/src/main.rs | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/odilia/src/main.rs b/odilia/src/main.rs index 427b568f..632675fa 100644 --- a/odilia/src/main.rs +++ b/odilia/src/main.rs @@ -165,7 +165,7 @@ async fn main() -> eyre::Result<()> { fn load_configuration(cli_overide: Option) -> Result { // In order, do a configuration file specified via cli, XDG_CONFIG_HOME, the usual location for system wide configuration(/etc/odilia/config.toml) - // If XDG_CONFIG_HOME based configuration wasn't found, create one with default values for the user to alter, for the next run of odilia + // If XDG_CONFIG_HOME based configuration wasn't found, create one by combining default values with the system provided ones, if available, for the user to alter, for the next run of odilia //default configuration first, because that doesn't affect the priority outlined above let figment = Figment::from(Serialized::defaults(ApplicationConfig::default())); //cli override, if applicable @@ -180,15 +180,16 @@ fn load_configuration(cli_overide: Option) -> Result Date: Wed, 29 May 2024 14:24:06 +0300 Subject: [PATCH 7/9] fix formatting --- common/src/settings/log.rs | 9 ++++----- odilia/src/logging.rs | 18 +++++++++--------- 2 files changed, 13 insertions(+), 14 deletions(-) diff --git a/common/src/settings/log.rs b/common/src/settings/log.rs index a90517f7..13bbf4fb 100644 --- a/common/src/settings/log.rs +++ b/common/src/settings/log.rs @@ -17,11 +17,10 @@ pub struct LogSettings { impl Default for LogSettings { fn default() -> Self { let directories=xdg::BaseDirectories::with_prefix("odilia").expect("can't create required directories according to the xdg directory specification"); - let log_path = directories.place_state_file("odilia.log").expect("can't place log file"); - Self { - level: "info".to_owned(), - logger: LoggingKind::File(log_path), - } + let log_path = directories + .place_state_file("odilia.log") + .expect("can't place log file"); + Self { level: "info".to_owned(), logger: LoggingKind::File(log_path) } } } diff --git a/odilia/src/logging.rs b/odilia/src/logging.rs index ca954979..c79e53c2 100644 --- a/odilia/src/logging.rs +++ b/odilia/src/logging.rs @@ -19,15 +19,15 @@ pub fn init(config: &ApplicationConfig) -> eyre::Result<()> { _ => EnvFilter::from(&config.log.level), }; let tree = HierarchicalLayer::new(4) - .with_bracketed_fields(true) - .with_targets(true) - .with_deferred_spans(true) - .with_span_retrace(true) - .with_indent_lines(true) - .with_ansi(false) - .with_wraparound(4); -//this requires boxing because the types returned by this match block would be incompatible otherwise, since we return different layers, or modifications to a layer depending on what we get from the configuration. It is possible to do it otherwise, hopefully, but for now this would do -let final_layer = match &config.log.logger { + .with_bracketed_fields(true) + .with_targets(true) + .with_deferred_spans(true) + .with_span_retrace(true) + .with_indent_lines(true) + .with_ansi(false) + .with_wraparound(4); + //this requires boxing because the types returned by this match block would be incompatible otherwise, since we return different layers, or modifications to a layer depending on what we get from the configuration. It is possible to do it otherwise, hopefully, but for now this would do + let final_layer = match &config.log.logger { LoggingKind::File(path) => { let file = std::fs::File::create(path).with_context(|| { format!("creating log file '{}'", path.display()) From 44b679e5b279a5c1cf8103743185daa806190507 Mon Sep 17 00:00:00 2001 From: alberto tirla Date: Wed, 29 May 2024 14:34:53 +0300 Subject: [PATCH 8/9] state: remove the ret when it's not required for us to know the value, to reduce log complexity --- odilia/src/state.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/odilia/src/state.rs b/odilia/src/state.rs index 29c6d64a..7fa7bd7e 100644 --- a/odilia/src/state.rs +++ b/odilia/src/state.rs @@ -114,7 +114,7 @@ impl ScreenReaderState { cache, }) } - #[tracing::instrument(level="debug", skip(self) ret, err)] + #[tracing::instrument(level = "debug", skip(self), err)] pub async fn get_or_create_atspi_cache_item_to_cache( &self, atspi_cache_item: atspi_common::CacheItem, @@ -130,7 +130,7 @@ impl ScreenReaderState { } self.cache.get(&prim).ok_or(CacheError::NoItem.into()) } - #[tracing::instrument(level="debug", skip(self) ret, err)] + #[tracing::instrument(level = "debug", skip(self), err)] pub async fn get_or_create_atspi_legacy_cache_item_to_cache( &self, atspi_cache_item: atspi_common::LegacyCacheItem, @@ -146,7 +146,7 @@ impl ScreenReaderState { } self.cache.get(&prim).ok_or(CacheError::NoItem.into()) } - #[tracing::instrument(skip_all, level = "debug", ret, err)] + #[tracing::instrument(skip_all, level = "debug", err)] pub async fn get_or_create_event_object_to_cache<'a, T: GenericEvent<'a>>( &self, event: &T, @@ -307,7 +307,7 @@ impl ScreenReaderState { .get_or_create(&accessible_proxy, Arc::downgrade(&self.cache)) .await } - #[tracing::instrument(skip_all, ret, err)] + #[tracing::instrument(skip_all, err)] pub async fn new_accessible<'a, T: GenericEvent<'a>>( &self, event: &T, @@ -321,7 +321,7 @@ impl ScreenReaderState { .build() .await?) } - #[tracing::instrument(skip_all, ret, err)] + #[tracing::instrument(skip_all, err)] pub async fn add_cache_match_rule(&self) -> OdiliaResult<()> { let cache_rule = MatchRule::builder() .msg_type(MessageType::Signal) From 200e4354043a698a65f9c298b78518b9b29e1ded Mon Sep 17 00:00:00 2001 From: alberto tirla Date: Wed, 29 May 2024 15:04:27 +0300 Subject: [PATCH 9/9] events: remove some unnecesary use of inlines and remove ret, err attributes when their output in the logs would be redundant --- odilia/src/events/object.rs | 34 +++++++++++++--------------------- 1 file changed, 13 insertions(+), 21 deletions(-) diff --git a/odilia/src/events/object.rs b/odilia/src/events/object.rs index 0a8dc7e4..61b28b40 100644 --- a/odilia/src/events/object.rs +++ b/odilia/src/events/object.rs @@ -1,7 +1,7 @@ use crate::state::ScreenReaderState; use atspi_common::events::object::ObjectEvents; -#[tracing::instrument(level = "debug", skip(state), ret, err)] +#[tracing::instrument(level = "debug", skip(state), err)] pub async fn dispatch(state: &ScreenReaderState, event: &ObjectEvents) -> eyre::Result<()> { // Dispatch based on member match event { @@ -36,7 +36,6 @@ mod text_changed { use ssip_client_async::Priority; use std::collections::HashMap; - #[inline] #[tracing::instrument(level = "trace")] pub fn update_string_insert( start_pos: usize, @@ -74,21 +73,18 @@ mod text_changed { } } - #[inline] pub fn append_to_object(original: &str, to_append: &str) -> String { let mut new_text = original.chars().collect::>(); new_text.extend(to_append.chars()); new_text.into_iter().collect() } - #[inline] pub fn insert_at_index(original: &str, to_splice: &str, index: usize) -> String { let mut new_text = original.chars().collect::>(); new_text.splice(index.., to_splice.chars()); new_text.into_iter().collect() } - #[inline] pub fn insert_at_range( original: &str, to_splice: &str, @@ -100,10 +96,9 @@ mod text_changed { new_text.into_iter().collect() } - #[inline] /// Get the live state of a set of attributes. /// Although the function only currently tests one attribute, in the future it may be important to inspect many attributes, compare them, or do additional logic. - #[tracing::instrument(level = "trace", ret, err)] + #[tracing::instrument(level = "trace", ret)] pub fn get_live_state(attributes: &HashMap) -> OdiliaResult { match attributes.get("live") { None => Err(OdiliaError::NoAttributeError("live".to_string())), @@ -111,7 +106,6 @@ mod text_changed { } } - #[inline] /// if the aria-live attribute is set to "polite", then set the priority of the message to speak once all other messages are done /// if the aria-live attribute is set to "assertive", then set the priority of the message to speak immediately, stop all other messages, and do not interrupt that piece of speech /// otherwise, do not continue @@ -123,8 +117,7 @@ mod text_changed { } } - #[inline] - #[tracing::instrument(level = "trace", ret, err)] + #[tracing::instrument(level = "trace", ret)] pub fn get_atomic_state(attributes: &HashMap) -> OdiliaResult { match attributes.get("atomic") { None => Err(OdiliaError::NoAttributeError("atomic".to_string())), @@ -162,7 +155,7 @@ mod text_changed { } } - #[tracing::instrument(level = "debug", skip(state), ret, err)] + #[tracing::instrument(level = "debug", skip(state), err)] pub async fn dispatch( state: &ScreenReaderState, event: &TextChangedEvent, @@ -180,7 +173,7 @@ mod text_changed { Ok(()) } - #[tracing::instrument(level = "debug", skip(state), ret, err)] + #[tracing::instrument(level = "debug", skip(state))] pub async fn speak_insertion( state: &ScreenReaderState, event: &TextChangedEvent, @@ -203,7 +196,7 @@ mod text_changed { /// The `insert` boolean, if set to true, will update the text in the cache. /// If it is set to false, the selection will be removed. /// The [`TextChangedEvent::operation`] value will *NOT* be checked by this function. - #[tracing::instrument(level = "debug", skip(state), ret, err)] + #[tracing::instrument(level = "debug", skip(state), err)] pub async fn insert_or_delete( state: &ScreenReaderState, event: &TextChangedEvent, @@ -258,7 +251,7 @@ mod children_changed { use odilia_common::result::OdiliaResult; use std::sync::Arc; - #[tracing::instrument(level = "debug", skip(state), ret, err)] + #[tracing::instrument(level = "debug", skip(state), err)] pub async fn dispatch( state: &ScreenReaderState, event: &ChildrenChangedEvent, @@ -271,7 +264,7 @@ mod children_changed { } Ok(()) } - #[tracing::instrument(level = "debug", skip(state), ret, err)] + #[tracing::instrument(level = "debug", skip(state), err)] pub async fn add( state: &ScreenReaderState, event: &ChildrenChangedEvent, @@ -286,7 +279,6 @@ mod children_changed { tracing::debug!("Add a single item to cache."); Ok(()) } - #[inline] fn get_child_primitive(event: &ChildrenChangedEvent) -> AccessiblePrimitive { event.child.clone().into() } @@ -395,7 +387,7 @@ mod text_caret_moved { } // TODO: left/right vs. up/down, and use generated speech - #[tracing::instrument(level = "debug", skip(state), ret, err)] + #[tracing::instrument(level = "debug", skip(state), err)] pub async fn text_cursor_moved( state: &ScreenReaderState, event: &TextCaretMovedEvent, @@ -431,7 +423,7 @@ mod text_caret_moved { Ok(()) } - #[tracing::instrument(level = "debug", skip(state), ret, err)] + #[tracing::instrument(level = "debug", skip(state), err)] pub async fn dispatch( state: &ScreenReaderState, event: &TextCaretMovedEvent, @@ -472,7 +464,7 @@ mod state_changed { } } - #[tracing::instrument(level = "debug", skip(state), ret, err)] + #[tracing::instrument(level = "debug", skip(state), err)] pub async fn dispatch( state: &ScreenReaderState, event: &StateChangedEvent, @@ -481,7 +473,7 @@ mod state_changed { // update cache with state of item let a11y_prim = AccessiblePrimitive::from_event(event)?; if update_state(state, &a11y_prim, event.state, state_value)? { - tracing::debug!("Updating of the state was not succesful! The item with id {:?} was not found in the cache.", a11y_prim.id); + tracing::trace!("Updating of the state was not successful! The item with id {:?} was not found in the cache.", a11y_prim.id); } else { tracing::trace!("Updated the state of accessible with ID {:?}, and state {:?} to {state_value}.", a11y_prim.id, event.state); } @@ -497,7 +489,7 @@ mod state_changed { Ok(()) } - #[tracing::instrument(level = "debug", skip(state), ret, err)] + #[tracing::instrument(level = "debug", skip(state), err)] pub async fn focused( state: &ScreenReaderState, event: &StateChangedEvent,