feat: use CacheEntry enum to abort previously failed lookups

Cargo.toml

@@ -16,7 +16,7 @@ speculoos = "0.9.0"
 anyhow = "1.0.57"
 linicon = "2.3.0"
 gtk4 = "0.4.7"
-criterion = "0.3"
+criterion = "0.3.5"
 
 [[bench]]
 name = "simple_lookup"

benches/simple_lookup.rs

@@ -1,9 +1,13 @@
-use criterion::{criterion_group, criterion_main, BenchmarkId, Criterion};
+use criterion::{
+    criterion_group, criterion_main, AxisScale, BenchmarkId, Criterion, PlotConfiguration,
+};
 use freedesktop_icons::lookup;
 use gtk4::{IconLookupFlags, IconTheme, TextDirection};
 
 pub fn bench_lookups(c: &mut Criterion) {
     let mut group = c.benchmark_group("ComparisonsLookups");
+    let plot_config = PlotConfiguration::default().summary_scale(AxisScale::Logarithmic);
+    group.plot_config(plot_config);
 
     let args = [
         "user-home", // (Best case) An icon that can be found in the current theme
@@ -49,5 +53,5 @@ pub fn bench_lookups(c: &mut Criterion) {
     group.finish();
 }
 
-criterion_group!(benches, simple_bench, bench_lookups);
+criterion_group!(benches, bench_lookups);
 criterion_main!(benches);

src/cache.rs

@@ -4,40 +4,56 @@ use std::path::{Path, PathBuf};
 use std::sync::Mutex;
 
 pub(crate) static CACHE: Lazy<Cache> = Lazy::new(Cache::default);
-type IconMap = BTreeMap<(String, u16, u16), PathBuf>;
+type IconMap = BTreeMap<(String, u16, u16), CacheEntry>;
 type ThemeMap = BTreeMap<String, IconMap>;
 
 #[derive(Default)]
 pub(crate) struct Cache(Mutex<ThemeMap>);
 
+#[derive(Debug, Clone, PartialEq)]
+pub enum CacheEntry {
+    // We already looked for this and nothing was found, indicates we should not try to perform a lookup.
+    NotFound,
+    // We have this entry.
+    Found(PathBuf),
+    // We don't know this entry yet, indicate we should perform a lookup.
+    Unknown,
+}
+
 impl Cache {
-    pub fn insert(&self, theme: &str, size: u16, scale: u16, icon_name: &str, icon_path: &Path) {
+    pub fn insert<P: AsRef<Path>>(
+        &self,
+        theme: &str,
+        size: u16,
+        scale: u16,
+        icon_name: &str,
+        icon_path: &Option<P>,
+    ) {
         let mut theme_map = self.0.lock().unwrap();
+        let entry = icon_path
+            .as_ref()
+            .map(|path| CacheEntry::Found(path.as_ref().to_path_buf()))
+            .unwrap_or(CacheEntry::NotFound);
 
         match theme_map.get_mut(theme) {
             Some(icon_map) => {
-                icon_map.insert(
+                icon_map.insert((icon_name.to_string(), size, scale), entry);
-                    (icon_name.to_string(), size, scale),
-                    icon_path.to_path_buf(),
-                );
             }
             None => {
                 let mut icon_map = BTreeMap::new();
-                icon_map.insert(
+                icon_map.insert((icon_name.to_string(), size, scale), entry);
-                    (icon_name.to_string(), size, scale),
-                    icon_path.to_path_buf(),
-                );
                 theme_map.insert(theme.to_string(), icon_map);
             }
         }
     }
 
-    pub fn get(&self, theme: &str, size: u16, scale: u16, icon_name: &str) -> Option<PathBuf> {
+    pub fn get(&self, theme: &str, size: u16, scale: u16, icon_name: &str) -> CacheEntry {
         let theme_map = self.0.lock().unwrap();
 
         theme_map
             .get(theme)
             .map(|icon_map| icon_map.get(&(icon_name.to_string(), size, scale)))
             .and_then(|path| path.cloned())
+            .unwrap_or(CacheEntry::Unknown)
     }
 }

src/lib.rs

@@ -50,7 +50,7 @@
 //!     .with_cache()
 //!     .find();
 //! # }
-use crate::cache::CACHE;
+use crate::cache::{CacheEntry, CACHE};
 use crate::theme::{try_build_icon_path, THEMES};
 use std::path::PathBuf;
 
@@ -89,7 +89,7 @@ pub struct LookupBuilder<'a> {
     cache: bool,
     scale: u16,
     size: u16,
-    theme: Option<&'a str>,
+    theme: &'a str,
 }
 
 /// Build an icon lookup for the given icon name.
@@ -149,7 +149,7 @@ impl<'a> LookupBuilder<'a> {
     ///     .find();
     /// # }
     pub fn with_theme<'b: 'a>(mut self, theme: &'b str) -> Self {
-        self.theme = Some(theme);
+        self.theme = theme;
         self
     }
 
@@ -178,18 +178,7 @@ impl<'a> LookupBuilder<'a> {
     /// `/usr/shar/hicolor` theme and then to `/usr/share/pixmaps`.
     pub fn find(self) -> Option<PathBuf> {
         // Lookup for an icon in the given theme and fallback to 'hicolor' default theme
-        let icon = self
+        self.lookup_in_theme()
-            .theme
-            .and_then(|theme| self.lookup_in_theme(theme))
-            .or_else(|| self.lookup_in_theme("hicolor"));
-
-        // Return the icon if found
-        if icon.is_some() {
-            return icon;
-        };
-
-        // Last chance
-        try_build_icon_path(self.name, "/usr/share/pixmaps")
     }
 
     fn new<'b: 'a>(name: &'b str) -> Self {
@@ -198,22 +187,29 @@ impl<'a> LookupBuilder<'a> {
             cache: false,
             scale: 1,
             size: 24,
-            theme: None,
+            theme: "hicolor",
         }
     }
 
     // Recursively lookup for icon in the given theme and its parents
-    fn lookup_in_theme(&self, theme: &str) -> Option<PathBuf> {
+    fn lookup_in_theme(&self) -> Option<PathBuf> {
         // If cache is activated, attempt to get the icon there first
+        // If the icon was previously search but not found, we return
+        // `None` early, otherwise, attempt to perform a lookup
         if self.cache {
-            let cached = self.cache_lookup(theme);
+            match self.cache_lookup(self.theme) {
-            if cached.is_some() {
+                CacheEntry::Found(icon) => {
-                return cached;
+                    return Some(icon);
-            }
+                }
+                CacheEntry::NotFound => {
+                    return None;
+                }
+                CacheEntry::Unknown => {}
+            };
         }
 
         // Then lookup in the given theme
-        THEMES.get(theme).and_then(|icon_theme| {
+        THEMES.get(self.theme).and_then(|icon_theme| {
             let icon = icon_theme
                 .try_get_icon(self.name, self.size, self.scale)
                 .or_else(|| {
@@ -223,10 +219,18 @@ impl<'a> LookupBuilder<'a> {
                             parent.try_get_icon(self.name, self.size, self.scale)
                         })
                     })
-                });
+                })
+                .or_else(|| {
+                    THEMES
+                        .get("hicolor")
+                        // Fallback to 'hicolor'
+                        .and_then(|hicolor| hicolor.try_get_icon(self.name, self.size, self.scale))
+                })
+                // Last chance, try to find the icon in "/usr/share/pixmaps"
+                .or_else(|| try_build_icon_path(self.name, "/usr/share/pixmaps"));
 
             if self.cache {
-                self.store(theme, icon)
+                self.store(self.theme, icon)
             } else {
                 icon
             }
@@ -234,22 +238,22 @@ impl<'a> LookupBuilder<'a> {
     }
 
     #[inline]
-    fn cache_lookup(&self, theme: &str) -> Option<PathBuf> {
+    fn cache_lookup(&self, theme: &str) -> CacheEntry {
         CACHE.get(theme, self.size, self.scale, self.name)
     }
 
     #[inline]
     fn store(&self, theme: &str, icon: Option<PathBuf>) -> Option<PathBuf> {
-        icon.map(|icon| {
+        CACHE.insert(theme, self.size, self.scale, self.name, &icon);
-            CACHE.insert(theme, self.size, self.scale, self.name, &icon);
+        icon
-            icon
-        })
     }
 }
 
+// WARNING: these test are highly dependent on your installed icon-themes.
+// If you want to run them, make sure you have 'Papirus' and 'Arc' icon-themes installed.
 #[cfg(test)]
 mod test {
-    use crate::lookup;
+    use crate::{lookup, CacheEntry, CACHE};
     use speculoos::prelude::*;
     use std::path::PathBuf;
 
@@ -266,7 +270,19 @@ mod test {
     }
 
     #[test]
-    fn should_fallback() {
+    fn theme_lookup() {
+        let firefox = lookup("firefox").with_theme("Papirus").find();
+
+        asserting!("Lookup with no parameters should return an existing icon")
+            .that(&firefox)
+            .is_some()
+            .is_equal_to(PathBuf::from(
+                "/usr/share/icons/Papirus/24x24/apps/firefox.svg",
+            ));
+    }
+
+    #[test]
+    fn should_fallback_to_parent_theme() {
         let icon = lookup("video-single-display-symbolic")
             .with_theme("Arc")
             .find();
@@ -279,6 +295,20 @@ mod test {
             ));
     }
 
+    #[test]
+    fn should_fallback_to_pixmaps_utlimately() {
+        let archlinux_logo = lookup("archlinux-logo")
+            .with_size(16)
+            .with_scale(1)
+            .with_theme("Papirus")
+            .find();
+
+        asserting!("When lookup fail in theme, icon should be found in '/usr/share/pixmaps'")
+            .that(&archlinux_logo)
+            .is_some()
+            .is_equal_to(PathBuf::from("/usr/share/pixmaps/archlinux-logo.png"));
+    }
+
     #[test]
     fn compare_to_linincon_with_theme() {
         let lin_wireshark = linicon::lookup_icon("wireshark")
@@ -300,16 +330,15 @@ mod test {
     }
 
     #[test]
-    fn should_fallback_to_pixmaps_utlimately() {
+    fn should_not_attempt_to_lookup_a_not_found_cached_icon() {
-        let archlinux_logo = lookup("archlinux-logo")
+        let not_found = lookup("not-found").with_cache().find();
-            .with_size(16)
-            .with_scale(1)
-            .with_theme("Papirus")
-            .find();
 
-        asserting!("When lookup fail in theme, icon should be found in '/usr/share/pixmaps'")
+        assert_that!(not_found).is_none();
-            .that(&archlinux_logo)
+
-            .is_some()
+        let expected_cache_result = CACHE.get("hicolor", 24, 1, "not-found");
-            .is_equal_to(PathBuf::from("/usr/share/pixmaps/archlinux-logo.png"));
+
+        asserting!("When lookup fails a first time, subsequent attempts should fail from cache")
+            .that(&expected_cache_result)
+            .is_equal_to(CacheEntry::NotFound);
     }
 }

src/theme/parse.rs

@@ -28,7 +28,13 @@ impl Theme {
     pub fn inherits(&self) -> Vec<&str> {
         self.get_icon_theme_section()
             .and_then(|props| props.get("Inherits"))
-            .map(|parents| parents.split(',').collect())
+            .map(|parents| {
+                parents
+                    .split(',')
+                    // Filtering out 'hicolor' since we are going to fallback there anyway
+                    .filter(|parent| parent != &"hicolor")
+                    .collect()
+            })
             .unwrap_or_default()
     }
 
@@ -84,13 +90,9 @@ mod test {
     fn should_get_theme_parents() {
         let theme = THEMES.get("Arc").unwrap();
         let parents = theme.inherits();
-        assert_that!(parents).is_equal_to(vec![
+
-            "Moka",
+        assert_that!(parents).does_not_contain("hicolor");
-            "Faba",
+
-            "elementary",
+        assert_that!(parents).is_equal_to(vec!["Moka", "Faba", "elementary", "Adwaita", "gnome"]);
-            "Adwaita",
-            "gnome",
-            "hicolor",
-        ]);
     }
 }