From 459e414a09ad0098e29f9ce4865f0c9a3fb87909 Mon Sep 17 00:00:00 2001 From: Federico Terzi Date: Sun, 4 Apr 2021 22:01:59 +0200 Subject: [PATCH] feat(config): refactor id field and introduce deduplication on legacy loader --- espanso-config/src/legacy/mod.rs | 118 +++++++++++++++++- .../src/matches/group/loader/yaml/mod.rs | 22 +++- espanso-config/src/matches/mod.rs | 45 +++---- espanso-config/src/matches/store/default.rs | 47 +++++-- 4 files changed, 182 insertions(+), 50 deletions(-) diff --git a/espanso-config/src/legacy/mod.rs b/espanso-config/src/legacy/mod.rs index a6863c0..4d2e3b0 100644 --- a/espanso-config/src/legacy/mod.rs +++ b/espanso-config/src/legacy/mod.rs @@ -22,8 +22,8 @@ use regex::Regex; use std::{collections::HashMap, path::Path}; use self::config::LegacyConfig; -use crate::config::store::DefaultConfigStore; use crate::matches::group::loader::yaml::parse::{YAMLMatch, YAMLVariable}; +use crate::{config::store::DefaultConfigStore, counter::StructId}; use crate::{ config::Config, config::{AppProperties, ConfigStore}, @@ -43,13 +43,28 @@ pub fn load( ) -> Result<(Box, Box)> { let config_set = config::LegacyConfigSet::load(base_dir, package_dir)?; - let (default_config, default_match_group) = split_config(config_set.default); + let mut match_deduplicate_map = HashMap::new(); + let mut var_deduplicate_map = HashMap::new(); + + let (default_config, mut default_match_group) = split_config(config_set.default); + deduplicate_ids( + &mut default_match_group, + &mut match_deduplicate_map, + &mut var_deduplicate_map, + ); + let mut match_groups = HashMap::new(); match_groups.insert("default".to_string(), default_match_group); let mut custom_configs: Vec> = Vec::new(); for custom in config_set.specific { - let (custom_config, custom_match_group) = split_config(custom); + let (custom_config, mut custom_match_group) = split_config(custom); + deduplicate_ids( + &mut custom_match_group, + &mut match_deduplicate_map, + &mut var_deduplicate_map, + ); + match_groups.insert(custom_config.name.clone(), custom_match_group); custom_configs.push(Box::new(custom_config)); } @@ -90,6 +105,34 @@ fn split_config(config: LegacyConfig) -> (LegacyInteropConfig, LegacyMatchGroup) (config, match_group) } +/// Due to the way the legacy configs are loaded (matches are copied multiple times in the various configs) +/// we need to deduplicate the ids of those matches (and global vars). +fn deduplicate_ids( + match_group: &mut LegacyMatchGroup, + match_map: &mut HashMap, + var_map: &mut HashMap, +) { + for m in match_group.matches.iter_mut() { + let mut m_without_id = m.clone(); + m_without_id.id = 0; + if let Some(id) = match_map.get(&m_without_id) { + m.id = *id; + } else { + match_map.insert(m_without_id, m.id); + } + } + + for v in match_group.global_vars.iter_mut() { + let mut v_without_id = v.clone(); + v_without_id.id = 0; + if let Some(id) = var_map.get(&v_without_id) { + v.id = *id; + } else { + var_map.insert(v_without_id, v.id); + } + } +} + struct LegacyInteropConfig { pub name: String, match_paths: Vec, @@ -352,6 +395,75 @@ mod tests { }); } + #[test] + fn load_legacy_deduplicates_ids_correctly() { + use_test_directory(|base, user, packages| { + std::fs::write( + base.join("default.yml"), + r#" + backend: Clipboard + + global_vars: + - name: var1 + type: test + + matches: + - trigger: "hello" + replace: "world" + "#, + ) + .unwrap(); + + std::fs::write( + user.join("specific.yml"), + r#" + name: specific + filter_title: "Google" + "#, + ) + .unwrap(); + + let (config_store, match_store) = load(base, packages).unwrap(); + + let default_config = config_store.default(); + let active_config = config_store.active(&AppProperties { + title: Some("Google"), + class: None, + exec: None, + }); + + assert_eq!( + match_store + .query(default_config.match_paths()) + .matches + .first() + .unwrap() + .id, + match_store + .query(active_config.match_paths()) + .matches + .first() + .unwrap() + .id, + ); + + assert_eq!( + match_store + .query(default_config.match_paths()) + .global_vars + .first() + .unwrap() + .id, + match_store + .query(active_config.match_paths()) + .global_vars + .first() + .unwrap() + .id, + ); + }); + } + #[test] fn load_legacy_with_packages() { use_test_directory(|base, _, packages| { diff --git a/espanso-config/src/matches/group/loader/yaml/mod.rs b/espanso-config/src/matches/group/loader/yaml/mod.rs index 5b46238..26717f9 100644 --- a/espanso-config/src/matches/group/loader/yaml/mod.rs +++ b/espanso-config/src/matches/group/loader/yaml/mod.rs @@ -17,10 +17,10 @@ * along with espanso. If not, see . */ -use crate::matches::{ +use crate::{counter::next_id, matches::{ group::{path::resolve_imports, MatchGroup}, Match, Variable, -}; +}}; use anyhow::Result; use log::warn; use parse::YAMLMatchGroup; @@ -138,7 +138,7 @@ impl TryFrom for Match { cause, effect, label: None, - ..Default::default() + id: next_id(), }) } } @@ -151,7 +151,7 @@ impl TryFrom for Variable { name: yaml_var.name, var_type: yaml_var.var_type, params: yaml_var.params, - ..Default::default() + id: next_id(), }) } } @@ -165,7 +165,14 @@ mod tests { fn create_match(yaml: &str) -> Result { let yaml_match: YAMLMatch = serde_yaml::from_str(yaml)?; - let m: Match = yaml_match.try_into()?; + let mut m: Match = yaml_match.try_into()?; + + // Reset the IDs to correctly compare them + m.id = 0; + if let MatchEffect::Text(e) = &mut m.effect { + e.vars.iter_mut().for_each(|v| v.id = 0); + } + Ok(m) } @@ -429,7 +436,10 @@ mod tests { std::fs::write(&sub_file, "").unwrap(); let importer = YAMLImporter::new(); - let group = importer.load_group(&base_file).unwrap(); + let mut group = importer.load_group(&base_file).unwrap(); + // Reset the ids to compare them correctly + group.matches.iter_mut().for_each(|mut m| m.id = 0); + group.global_vars.iter_mut().for_each(|mut v| v.id = 0); let vars = vec![Variable { name: "var1".to_string(), diff --git a/espanso-config/src/matches/mod.rs b/espanso-config/src/matches/mod.rs index f5208b8..c7c88ea 100644 --- a/espanso-config/src/matches/mod.rs +++ b/espanso-config/src/matches/mod.rs @@ -19,21 +19,20 @@ use serde_yaml::Mapping; -use crate::counter::{next_id, StructId}; +use crate::counter::{StructId}; pub(crate) mod group; pub mod store; -#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct Match { - cause: MatchCause, - effect: MatchEffect, + pub id: StructId, + + pub cause: MatchCause, + pub effect: MatchEffect, // Metadata - label: Option, - - // Internals - _id: StructId, + pub label: Option, } impl Default for Match { @@ -42,20 +41,14 @@ impl Default for Match { cause: MatchCause::None, effect: MatchEffect::None, label: None, - _id: next_id(), + id: 0, } } } -impl PartialEq for Match { - fn eq(&self, other: &Self) -> bool { - self.cause == other.cause && self.effect == other.effect && self.label == other.label - } -} - // Causes -#[derive(Debug, Clone, PartialEq)] +#[derive(Debug, Clone, Eq, Hash, PartialEq)] pub enum MatchCause { None, Trigger(TriggerCause), @@ -63,7 +56,7 @@ pub enum MatchCause { // TODO: shortcut } -#[derive(Debug, Clone, PartialEq)] +#[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct TriggerCause { pub triggers: Vec, @@ -86,7 +79,7 @@ impl Default for TriggerCause { // Effects -#[derive(Debug, Clone, PartialEq)] +#[derive(Debug, Clone, PartialEq, Eq, Hash)] pub enum MatchEffect { None, Text(TextEffect), @@ -94,7 +87,7 @@ pub enum MatchEffect { // TODO: rich text } -#[derive(Debug, Clone, PartialEq)] +#[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct TextEffect { pub replace: String, pub vars: Vec, @@ -109,29 +102,21 @@ impl Default for TextEffect { } } -#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct Variable { + pub id: StructId, pub name: String, pub var_type: String, pub params: Mapping, - - // Internals - _id: StructId, } impl Default for Variable { fn default() -> Self { Self { + id: 0, name: String::new(), var_type: String::new(), params: Mapping::new(), - _id: next_id(), } } } - -impl PartialEq for Variable { - fn eq(&self, other: &Self) -> bool { - self.name == other.name && self.var_type == other.var_type && self.params == other.params - } -} diff --git a/espanso-config/src/matches/store/default.rs b/espanso-config/src/matches/store/default.rs index 8b3ea6a..3206585 100644 --- a/espanso-config/src/matches/store/default.rs +++ b/espanso-config/src/matches/store/default.rs @@ -42,9 +42,7 @@ impl DefaultMatchStore { // top-level ones. load_match_groups_recursively(&mut groups, paths); - Self { - groups, - } + Self { groups } } } @@ -117,16 +115,16 @@ fn query_matches_for_paths<'a>( ); for m in group.matches.iter() { - if !visited_matches.contains(&m._id) { + if !visited_matches.contains(&m.id) { matches.push(m); - visited_matches.insert(m._id); + visited_matches.insert(m.id); } } for var in group.global_vars.iter() { - if !visited_global_vars.contains(&var._id) { + if !visited_global_vars.contains(&var.id) { global_vars.push(var); - visited_global_vars.insert(var._id); + visited_global_vars.insert(var.id); } } } @@ -232,16 +230,27 @@ mod tests { .get(&base_file.to_string_lossy().to_string()) .unwrap() .matches; - assert_eq!(base_group, &create_matches(&[("hello", "world")])); + let base_group: Vec = base_group.iter().map(|m| { + let mut copy = m.clone(); + copy.id = 0; + copy + }).collect(); + + assert_eq!(base_group, create_matches(&[("hello", "world")])); let another_group = &match_store .groups .get(&another_file.to_string_lossy().to_string()) .unwrap() .matches; + let another_group: Vec = another_group.iter().map(|m| { + let mut copy = m.clone(); + copy.id = 0; + copy + }).collect(); assert_eq!( another_group, - &create_matches(&[("hello", "world2"), ("foo", "bar")]) + create_matches(&[("hello", "world2"), ("foo", "bar")]) ); let sub_group = &match_store @@ -249,7 +258,12 @@ mod tests { .get(&sub_file.to_string_lossy().to_string()) .unwrap() .matches; - assert_eq!(sub_group, &create_matches(&[("hello", "world3")])); + let sub_group: Vec = sub_group.iter().map(|m| { + let mut copy = m.clone(); + copy.id = 0; + copy + }).collect(); + assert_eq!(sub_group, create_matches(&[("hello", "world3")])); }); } @@ -373,6 +387,7 @@ mod tests { .matches .into_iter() .cloned() + .map(|mut m| { m.id = 0; m }) .collect::>(), create_matches(&[ ("hello", "world3"), @@ -387,6 +402,7 @@ mod tests { .global_vars .into_iter() .cloned() + .map(|mut v| { v.id = 0; v }) .collect::>(), create_vars(&["var2", "var1"]) ); @@ -460,6 +476,7 @@ mod tests { .matches .into_iter() .cloned() + .map(|mut m| { m.id = 0; m }) .collect::>(), create_matches(&[ ("hello", "world3"), @@ -474,6 +491,7 @@ mod tests { .global_vars .into_iter() .cloned() + .map(|mut v| { v.id = 0; v}) .collect::>(), create_vars(&["var2", "var1"]) ); @@ -532,7 +550,10 @@ mod tests { ) .unwrap(); - let match_store = DefaultMatchStore::new(&[base_file.to_string_lossy().to_string(), sub_file.to_string_lossy().to_string()]); + let match_store = DefaultMatchStore::new(&[ + base_file.to_string_lossy().to_string(), + sub_file.to_string_lossy().to_string(), + ]); let match_set = match_store.query(&[ base_file.to_string_lossy().to_string(), @@ -544,6 +565,7 @@ mod tests { .matches .into_iter() .cloned() + .map(|mut m| { m.id = 0; m }) .collect::>(), create_matches(&[ ("hello", "world2"), @@ -558,6 +580,7 @@ mod tests { .global_vars .into_iter() .cloned() + .map(|mut v| { v.id = 0; v }) .collect::>(), create_vars(&["var1", "var2"]) ); @@ -631,6 +654,7 @@ mod tests { .matches .into_iter() .cloned() + .map(|mut m| { m.id = 0; m }) .collect::>(), create_matches(&[ ("hello", "world3"), // This appears only once, though it appears 2 times @@ -645,6 +669,7 @@ mod tests { .global_vars .into_iter() .cloned() + .map(|mut v| { v.id = 0; v }) .collect::>(), create_vars(&["var2", "var1"]) );