diff --git a/Cargo.toml b/Cargo.toml index 8c89a34..b12f171 100644 --- a/Cargo.toml +++ b/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" diff --git a/benches/simple_lookup.rs b/benches/simple_lookup.rs index 8374103..809f15b 100644 --- a/benches/simple_lookup.rs +++ b/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); diff --git a/src/cache.rs b/src/cache.rs index 8e38de4..2dbc0df 100644 --- a/src/cache.rs +++ b/src/cache.rs @@ -4,40 +4,56 @@ use std::path::{Path, PathBuf}; use std::sync::Mutex; pub(crate) static CACHE: Lazy = Lazy::new(Cache::default); -type IconMap = BTreeMap<(String, u16, u16), PathBuf>; +type IconMap = BTreeMap<(String, u16, u16), CacheEntry>; type ThemeMap = BTreeMap; #[derive(Default)] pub(crate) struct Cache(Mutex); +#[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>( + &self, + theme: &str, + size: u16, + scale: u16, + icon_name: &str, + icon_path: &Option

, + ) { 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_name.to_string(), size, scale), - icon_path.to_path_buf(), - ); + icon_map.insert((icon_name.to_string(), size, scale), entry); } None => { let mut icon_map = BTreeMap::new(); - icon_map.insert( - (icon_name.to_string(), size, scale), - icon_path.to_path_buf(), - ); + icon_map.insert((icon_name.to_string(), size, scale), entry); theme_map.insert(theme.to_string(), icon_map); } } } - pub fn get(&self, theme: &str, size: u16, scale: u16, icon_name: &str) -> Option { + 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) } } diff --git a/src/lib.rs b/src/lib.rs index 7fa3115..68341b6 100644 --- a/src/lib.rs +++ b/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 { // Lookup for an icon in the given theme and fallback to 'hicolor' default theme - let icon = self - .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") + self.lookup_in_theme() } 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 { + fn lookup_in_theme(&self) -> Option { // 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); - if cached.is_some() { - return cached; - } + match self.cache_lookup(self.theme) { + CacheEntry::Found(icon) => { + 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 { + fn cache_lookup(&self, theme: &str) -> CacheEntry { CACHE.get(theme, self.size, self.scale, self.name) } #[inline] fn store(&self, theme: &str, icon: Option) -> Option { - icon.map(|icon| { - CACHE.insert(theme, self.size, self.scale, self.name, &icon); - icon - }) + CACHE.insert(theme, self.size, self.scale, self.name, &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() { - let archlinux_logo = lookup("archlinux-logo") - .with_size(16) - .with_scale(1) - .with_theme("Papirus") - .find(); + fn should_not_attempt_to_lookup_a_not_found_cached_icon() { + let not_found = lookup("not-found").with_cache().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")); + assert_that!(not_found).is_none(); + + let expected_cache_result = CACHE.get("hicolor", 24, 1, "not-found"); + + asserting!("When lookup fails a first time, subsequent attempts should fail from cache") + .that(&expected_cache_result) + .is_equal_to(CacheEntry::NotFound); } } diff --git a/src/theme/parse.rs b/src/theme/parse.rs index d7ef421..feaa44b 100644 --- a/src/theme/parse.rs +++ b/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", - "Faba", - "elementary", - "Adwaita", - "gnome", - "hicolor", - ]); + + assert_that!(parents).does_not_contain("hicolor"); + + assert_that!(parents).is_equal_to(vec!["Moka", "Faba", "elementary", "Adwaita", "gnome"]); } }