greshake / i3status-rust

Very resourcefriendly and feature-rich replacement for i3status, written in pure Rust
GNU General Public License v3.0
2.82k stars 472 forks source link

config: allow widget=<regex> in click handlers #1961

Closed MaxVerevkin closed 5 months ago

MaxVerevkin commented 8 months ago

Before:

[[block.click]]
button = "middle"
action = "toggle_format"
[[block.click]]
button = "middle"
widget = "play_pause_btn"
action = "toggle_format"

After:

[[block.click]]
button = "middle"
widget = "*"
action = "toggle_format"
MaxVerevkin commented 8 months ago

I think allowing a list of strings may be useful too.

bim9262 commented 8 months ago

@MaxVerevkin, what about allowing a regex, so that wildcards and lists (a|b), can be easily expressed?

MaxVerevkin commented 8 months ago

Both may work. Regex is less effort on our side even. I guess widget = "(a|b|c)" is not worse than widget = ["a", "b", "c"].

MaxVerevkin commented 8 months ago

what about allowing a regex

This would require us to assign some name to the default "no widget". Maybe "none" or an empty string.

bim9262 commented 8 months ago

How about block?

MaxVerevkin commented 8 months ago

Hm... like widget = "(block|play_pause_btn)"?

bim9262 commented 8 months ago

Yes, I think that would be easier to understand compared to (none|some_btn)

bim9262 commented 8 months ago

And just add a note that "block" is the default

MaxVerevkin commented 8 months ago

what about allowing a regex

One thing I'm not sure about is whether the regex should be anchored. Currently widget = "lo" matches "block".

bim9262 commented 5 months ago

@MaxVerevkin want to wrap this up to include it in v0.33.0?

BTW, this was what I was suggesting in my previous comment:

diff --git a/src/blocks/sound.rs b/src/blocks/sound.rs
index 5a2a238a..45b21d9e 100644
--- a/src/blocks/sound.rs
+++ b/src/blocks/sound.rs
@@ -95,17 +95,10 @@ mod alsa;
 mod pulseaudio;

 use super::prelude::*;
-use regex::Regex;
-use serde_with::{serde_as, serde_conv, Map};
+use crate::wrappers::RegexAsString;

-// A conversion adapter that is used to convert Regex to String and vice versa
-// when serializing or deserializing using serde.
-serde_conv!(
-    RegexAsString,
-    Regex,
-    |regex: &Regex| regex.as_str().to_string(),
-    |value: String| Regex::new(&value)
-);
+use regex::Regex;
+use serde_with::{serde_as, Map};

 #[serde_as]
 #[derive(Deserialize, Debug, SmartDefault)]
diff --git a/src/click.rs b/src/click.rs
index 175d6c3c..b4aa890a 100644
--- a/src/click.rs
+++ b/src/click.rs
@@ -1,12 +1,14 @@
 use std::fmt;

+use regex::Regex;
 use serde::de::{self, Deserializer, Visitor};
 use serde::Deserialize;
+use serde_with::serde_as;

 use crate::errors::{ErrorContext, Result};
 use crate::protocol::i3bar_event::I3BarEvent;
 use crate::subprocess::{spawn_shell, spawn_shell_sync};
-use crate::wrappers::SerdeRegex;
+use crate::wrappers::RegexAsString;

 /// Can be one of `left`, `middle`, `right`, `up`, `down`, `forward`, `back` or `double_left`.
 ///
@@ -42,7 +44,7 @@ impl ClickHandler {
             .filter(|e| e.button == event.button)
             .find(|e| match &e.widget {
                 None => event.instance.is_none(),
-                Some(re) => re.0.is_match(event.instance.as_deref().unwrap_or("block")),
+                Some(re) => re.is_match(event.instance.as_deref().unwrap_or("block")),
             })
         else {
             return Ok(None);
@@ -64,6 +66,7 @@ impl ClickHandler {
     }
 }

+#[serde_as]
 #[derive(Deserialize, Debug, Clone)]
 #[serde(deny_unknown_fields)]
 pub struct ClickConfigEntry {
@@ -71,7 +74,8 @@ pub struct ClickConfigEntry {
     button: MouseButton,
     /// To which part of the block this entry applies
     #[serde(default)]
-    widget: Option<SerdeRegex>,
+    #[serde_as(as = "Option<RegexAsString>")]
+    widget: Option<Regex>,
     /// Which command to run
     #[serde(default)]
     cmd: Option<String>,
diff --git a/src/wrappers.rs b/src/wrappers.rs
index 1c055166..9a6b2afc 100644
--- a/src/wrappers.rs
+++ b/src/wrappers.rs
@@ -1,6 +1,8 @@
 use crate::errors::*;

+use regex::Regex;
 use serde::de::{self, Deserialize, Deserializer};
+use serde_with::serde_conv;
 use std::borrow::Cow;
 use std::fmt::{self, Display};
 use std::marker::PhantomData;
@@ -192,34 +194,14 @@ where
     }
 }

-#[derive(Debug, Clone)]
-pub struct SerdeRegex(pub regex::Regex);
-
-impl<'de> Deserialize<'de> for SerdeRegex {
-    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
-    where
-        D: Deserializer<'de>,
-    {
-        struct Visitor;
-
-        impl<'de> de::Visitor<'de> for Visitor {
-            type Value = SerdeRegex;
-
-            fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
-                formatter.write_str("a regex")
-            }
-
-            fn visit_str<E>(self, v: &str) -> Result<Self::Value, E>
-            where
-                E: de::Error,
-            {
-                regex::Regex::new(v).map(SerdeRegex).map_err(E::custom)
-            }
-        }
-
-        deserializer.deserialize_any(Visitor)
-    }
-}
+// A conversion adapter that is used to convert Regex to String and vice versa
+// when serializing or deserializing using serde.
+serde_conv!(
+    pub RegexAsString,
+    Regex,
+    |regex: &Regex| regex.as_str().to_string(),
+    |value: String| Regex::new(&value)
+);

 /// Display a slice. Similar to Debug impl for slice, but uses Display impl for elements.
 pub struct DisplaySlice<'a, T>(pub &'a [T]);
MaxVerevkin commented 5 months ago

Regarding anchoring, I guess it's OK to leave it unanchored, at least for consistency with all the other options.


I am just not that familiar with serde_with while SerdeRegex is just a small newtype with a deserialization impl.

bim9262 commented 5 months ago

Yes, I think leaving it unanchored is ok. Might want to do a seperate review of regex uses for example in the docs

interface_name_exclude = [".*kdeconnect.*", "mpd"]

is the same as

interface_name_exclude = ["kdeconnect", "mpd"]

The example is confusing as it would make one think that the regex is anchored.


We could go the other way and remove serde_with and save about 7000 bytes (this could happen in another PR after this one is merged)

diff --git a/Cargo.lock b/Cargo.lock
#Omitted for brevity
diff --git a/Cargo.toml b/Cargo.toml
index 7e863bec..4a15b53c 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -63,7 +63,6 @@ reqwest = { version = "0.11", features = ["json"] }
 sensors = "0.2.2"
 serde = { version = "1.0", features = ["derive", "rc"] }
 serde_json = "1.0"
-serde_with = "3.0"
 shellexpand = "3.0"
 signal-hook = "0.3"
 signal-hook-tokio = { version = "0.3", features = ["futures-v0_3"] }
diff --git a/src/blocks/sound.rs b/src/blocks/sound.rs
index f78128d0..c8e56dd8 100644
--- a/src/blocks/sound.rs
+++ b/src/blocks/sound.rs
@@ -95,19 +95,9 @@ mod alsa;
 mod pulseaudio;

 use super::prelude::*;
+use crate::wrappers::SerdeRegex;
 use regex::Regex;
-use serde_with::{serde_as, serde_conv, Map};

-// A conversion adapter that is used to convert Regex to String and vice versa
-// when serializing or deserializing using serde.
-serde_conv!(
-    RegexAsString,
-    Regex,
-    |regex: &Regex| regex.as_str().to_string(),
-    |value: String| Regex::new(&value)
-);
-
-#[serde_as]
 #[derive(Deserialize, Debug, SmartDefault)]
 #[serde(deny_unknown_fields, default)]
 pub struct Config {
@@ -121,13 +111,11 @@ pub struct Config {
     pub format: FormatConfig,
     pub headphones_indicator: bool,
     pub show_volume_when_muted: bool,
-    #[serde_as(as = "Option<Map<_, _>>")]
     pub mappings: Option<Vec<(String, String)>>,
     #[default(true)]
     pub mappings_use_regex: bool,
     pub max_vol: Option<u32>,
-    #[serde_as(as = "Map<RegexAsString, _>")]
-    pub active_port_mappings: Vec<(Regex, String)>,
+    pub active_port_mappings: Vec<(SerdeRegex, String)>,
 }

 enum Mappings<'a> {
@@ -261,9 +249,9 @@ pub async fn run(config: &Config, api: &CommonApi) -> Result<()> {
             if let Some((regex, mapped)) = config
                 .active_port_mappings
                 .iter()
-                .find(|(regex, _)| regex.is_match(ap))
+                .find(|(regex, _)| regex.0.is_match(ap))
             {
-                let mapped = regex.replace(ap, mapped);
+                let mapped = regex.0.replace(ap, mapped);
                 if mapped.is_empty() {
                     active_port = None;
                 } else {