From 72b3cb28012d44338c549de709d106ec796b599c Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 17 Aug 2024 08:44:48 +0200 Subject: [PATCH 1/3] Use xcursor-rs for cursor theme parsing Instead of reinventing the wheel, let's use an existing wheel for the same job: Parsing cursor theme files. This finished #934. A first step towards that goal was previously done in commit d0ed9db7540bb. Fixes: https://github.com/psychon/x11rb/issues/934 Signed-off-by: Uli Schlachter --- x11rb/src/cursor/find_cursor.rs | 299 ++------------------------------ x11rb/src/cursor/mod.rs | 18 +- 2 files changed, 21 insertions(+), 296 deletions(-) diff --git a/x11rb/src/cursor/find_cursor.rs b/x11rb/src/cursor/find_cursor.rs index 3074777d..e8c6dd78 100644 --- a/x11rb/src/cursor/find_cursor.rs +++ b/x11rb/src/cursor/find_cursor.rs @@ -7,11 +7,8 @@ // // and is licensed under MIT/X Consortium License -use std::env::{var, var_os}; -use std::ffi::OsStr; use std::fs::File; -use std::io::{BufRead, BufReader, Error as IOError}; -use std::path::{Path, PathBuf}; +use xcursor::CursorTheme; static CORE_CURSORS: &[(&str, u16)] = &[ ("X_cursor", 0), @@ -104,301 +101,37 @@ fn cursor_shape_to_id(name: &str) -> Option { .next() } -/// An error that occurred while searching -#[derive(Debug)] -pub(crate) enum Error { - /// `$HOME` is not set - NoHomeDir, +#[cfg(test)] +mod test_cursor_shape_to_id { + use super::cursor_shape_to_id; - /// No cursor file could be found - NothingFound, + #[test] + fn test_cursor_shape_to_id() { + assert_eq!(cursor_shape_to_id("heart"), Some(31)); + } } /// The result of finding a cursor #[derive(Debug)] -pub(crate) enum Cursor { +pub(crate) enum Cursor { /// The cursor is a core cursor that can be created with xproto's `CreateGlyphCursor` CoreChar(u16), /// A cursor file was opened - File(F), -} - -// Get the 'Inherits' entry from an index.theme file -fn parse_inherits(filename: &Path) -> Result, IOError> { - let file = File::open(filename)?; - parse_inherits_impl(&mut BufReader::new(file)) -} - -// Get the 'Inherits' entry from an index.theme file -fn parse_inherits_impl(input: &mut impl BufRead) -> Result, IOError> { - let mut buffer = Vec::new(); - loop { - buffer.clear(); - - // Read a line - if 0 == input.read_until(b'\n', &mut buffer)? { - // End of file, return an empty result - return Ok(Default::default()); - } - - // Remove end of line marker - if buffer.last() == Some(&b'\n') { - let _ = buffer.pop(); - } - - let begin = b"Inherits"; - if buffer.starts_with(begin) { - let mut result = Vec::new(); - - let mut to_parse = &buffer[begin.len()..]; - - fn skip_while(mut slice: &[u8], f: impl Fn(u8) -> bool) -> &[u8] { - while !slice.is_empty() && f(slice[0]) { - slice = &slice[1..] - } - slice - } - - // Skip all spaces - to_parse = skip_while(to_parse, |c| c == b' '); - - // Now we need an equal sign - if to_parse.first() == Some(&b'=') { - to_parse = &to_parse[1..]; - - fn should_skip(c: u8) -> bool { - matches!(c, b' ' | b'\t' | b'\n' | b';' | b',') - } - - // Iterate over the pieces - for mut part in to_parse.split(|&x| x == b':') { - // Skip all leading whitespace - part = skip_while(part, should_skip); - - // Skip all trailing whitespace - while let Some((&last, rest)) = part.split_last() { - if !should_skip(last) { - break; - } - part = rest; - } - if !part.is_empty() { - if let Ok(part) = std::str::from_utf8(part) { - result.push(part.to_string()); - } - } - } - } - return Ok(result); - } - } -} - -#[cfg(test)] -mod test_parse_inherits { - use super::parse_inherits_impl; - use std::io::Cursor; - - #[test] - fn parse_inherits_successful() { - let input = - b"Hi\nInherits = \t ; whatever ;,::; stuff : i s ,: \tthis \t \nInherits=ignored\n"; - let mut input = Cursor::new(&input[..]); - let result = parse_inherits_impl(&mut input).unwrap(); - assert_eq!(result, vec!["whatever", "stuff", "i s", "this"]); - } + File(File), } /// Find a cursor file based on the name of a cursor theme and the name of the cursor. -pub(crate) fn find_cursor(theme: &str, name: &str) -> Result, Error> { - let home = match var_os("HOME") { - Some(home) => home, - None => return Err(Error::NoHomeDir), - }; - let cursor_path = var("XCURSOR_PATH").unwrap_or_else(|_| { - "~/.local/share/icons:~/.icons:/usr/share/icons:/usr/share/pixmaps".into() - }); - let open_cursor = |file: &Path| File::open(file); - find_cursor_impl( - &home, - &cursor_path, - theme, - name, - open_cursor, - parse_inherits, - ) -} - -fn find_cursor_impl( - home: &OsStr, - cursor_path: &str, - theme: &str, - name: &str, - mut open_cursor: G, - mut parse_inherits: H, -) -> Result, Error> -where - G: FnMut(&Path) -> Result, - H: FnMut(&Path) -> Result, IOError>, -{ +pub(crate) fn find_cursor(theme: &str, name: &str) -> Option { if theme == "core" { if let Some(id) = cursor_shape_to_id(name) { - return Ok(Cursor::CoreChar(id)); + return Some(Cursor::CoreChar(id)); } } - - let cursor_path = cursor_path.split(':').collect::>(); - - let mut next_inherits = Vec::new(); - let mut last_inherits = vec![theme.into()]; - while !last_inherits.is_empty() { - for theme in last_inherits { - for path in &cursor_path { - // Calculate the path to the theme's directory - let mut theme_dir = PathBuf::new(); - // Does the path begin with '~'? - if let Some(mut path) = path.strip_prefix('~') { - theme_dir.push(home); - // Skip a path separator if there is one - if path.chars().next().map(std::path::is_separator) == Some(true) { - path = &path[1..]; - } - theme_dir.push(path); - } else { - theme_dir.push(path); - } - theme_dir.push(&theme); - - // Find the cursor in the theme - let mut cursor_file = theme_dir.clone(); - cursor_file.push("cursors"); - cursor_file.push(name); - if let Ok(file) = open_cursor(&cursor_file) { - return Ok(Cursor::File(file)); - } - - // Get the theme's index.theme file and parse its 'Inherits' line - let mut index = theme_dir; - index.push("index.theme"); - if let Ok(res) = parse_inherits(&index) { - next_inherits.extend(res); - } - } - } - - last_inherits = next_inherits; - next_inherits = Vec::new(); - } - - Err(Error::NothingFound) -} - -// FIXME: Make these tests pass on Windows; problem is "/" vs "\\" in paths -#[cfg(all(test, unix))] -mod test_find_cursor { - use super::{find_cursor_impl, Cursor, Error}; - use crate::errors::ConnectionError; - use std::io::{Error as IOError, ErrorKind}; - use std::path::Path; - - #[test] - fn core_cursor() { - let cb1 = |_: &Path| -> Result<(), _> { unimplemented!() }; - let cb2 = |_: &Path| unimplemented!(); - match find_cursor_impl("unused".as_ref(), "unused", "core", "heart", cb1, cb2).unwrap() { - Cursor::CoreChar(31) => {} - e => panic!("Unexpected result {:?}", e), - } - } - - #[test] - fn nothing_found() { - let mut opened = Vec::new(); - let mut inherit_parsed = Vec::new(); - let cb1 = |path: &Path| -> Result<(), _> { - opened.push(path.to_str().unwrap().to_owned()); - Err(IOError::new( - ErrorKind::Other, - ConnectionError::UnknownError, - )) - }; - let cb2 = |path: &Path| { - inherit_parsed.push(path.to_str().unwrap().to_owned()); - Ok(Vec::new()) - }; - match find_cursor_impl( - "home".as_ref(), - "path:~/some/:/entries", - "theme", - "theCursor", - cb1, - cb2, - ) { - Err(Error::NothingFound) => {} - e => panic!("Unexpected result {:?}", e), - } - assert_eq!( - opened, - &[ - "path/theme/cursors/theCursor", - "home/some/theme/cursors/theCursor", - "/entries/theme/cursors/theCursor", - ] - ); - assert_eq!( - inherit_parsed, - &[ - "path/theme/index.theme", - "home/some/theme/index.theme", - "/entries/theme/index.theme", - ] - ); - } - - #[test] - fn inherit() { - let mut opened = Vec::new(); - let cb1 = |path: &Path| -> Result<(), _> { - opened.push(path.to_str().unwrap().to_owned()); - Err(IOError::new( - ErrorKind::Other, - ConnectionError::UnknownError, - )) - }; - let cb2 = |path: &Path| { - if path.starts_with("base/theTheme") { - Ok(vec!["inherited".into()]) - } else if path.starts_with("path/inherited") { - Ok(vec!["theEnd".into()]) - } else { - Ok(vec![]) - } - }; - match find_cursor_impl( - "home".as_ref(), - "path:base:tail", - "theTheme", - "theCursor", - cb1, - cb2, - ) { - Err(Error::NothingFound) => {} - e => panic!("Unexpected result {:?}", e), + if let Some(path) = CursorTheme::load(theme).load_icon(name) { + if let Ok(file) = File::open(path) { + return Some(Cursor::File(file)); } - assert_eq!( - opened, - &[ - "path/theTheme/cursors/theCursor", - "base/theTheme/cursors/theCursor", - "tail/theTheme/cursors/theCursor", - "path/inherited/cursors/theCursor", - "base/inherited/cursors/theCursor", - "tail/inherited/cursors/theCursor", - "path/theEnd/cursors/theCursor", - "base/theEnd/cursors/theCursor", - "tail/theEnd/cursors/theCursor", - ] - ); } + None } diff --git a/x11rb/src/cursor/mod.rs b/x11rb/src/cursor/mod.rs index 09c95668..0d126393 100644 --- a/x11rb/src/cursor/mod.rs +++ b/x11rb/src/cursor/mod.rs @@ -11,8 +11,6 @@ use crate::resource_manager::Database; use crate::NONE; use xcursor::parser::Image; -use std::fs::File; - mod find_cursor; mod parse_cursor; @@ -177,17 +175,11 @@ impl Handle { } } -fn open_cursor(theme: &Option, name: &str) -> Option> { - if let Some(theme) = theme { - if let Ok(cursor) = find_cursor::find_cursor(theme, name) { - return Some(cursor); - } - } - if let Ok(cursor) = find_cursor::find_cursor("default", name) { - Some(cursor) - } else { - None - } +fn open_cursor(theme: &Option, name: &str) -> Option { + theme + .as_ref() + .and_then(|theme| find_cursor::find_cursor(theme, name)) + .or_else(|| find_cursor::find_cursor("default", name)) } fn create_core_cursor( From 2695cd92c235e954955a2d7d721d394ea0149ca2 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 17 Aug 2024 08:45:02 +0200 Subject: [PATCH 2/3] Simplify .filter().next() into .find() I would have thought clippy already complains about this...? Or does the intermediate .map() confuse it, perhaps? Signed-off-by: Uli Schlachter --- x11rb/src/cursor/find_cursor.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/x11rb/src/cursor/find_cursor.rs b/x11rb/src/cursor/find_cursor.rs index e8c6dd78..2b0d4170 100644 --- a/x11rb/src/cursor/find_cursor.rs +++ b/x11rb/src/cursor/find_cursor.rs @@ -96,9 +96,8 @@ static CORE_CURSORS: &[(&str, u16)] = &[ fn cursor_shape_to_id(name: &str) -> Option { CORE_CURSORS .iter() - .filter(|&(name2, _)| name == *name2) + .find(|&(name2, _)| name == *name2) .map(|&(_, id)| id) - .next() } #[cfg(test)] From 9ee898ee63a33a55d1c2e1a61d28503264586eaf Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 17 Aug 2024 08:51:05 +0200 Subject: [PATCH 3/3] Try to simplify some pattern matching I'm not sure if the new version really is simpler, but at least I got rid of an unwrap(). Signed-off-by: Uli Schlachter --- x11rb/src/cursor/mod.rs | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/x11rb/src/cursor/mod.rs b/x11rb/src/cursor/mod.rs index 0d126393..35b8d428 100644 --- a/x11rb/src/cursor/mod.rs +++ b/x11rb/src/cursor/mod.rs @@ -223,21 +223,22 @@ fn create_render_cursor( let (width, height) = (to_u16(image.width), to_u16(image.height)); // Get a pixmap of the right size and a gc for it - let (pixmap, gc) = if storage.map(|(_, _, w, h)| (w, h)) == Some((width, height)) { - storage.map(|(pixmap, gc, _, _)| (pixmap, gc)).unwrap() - } else { - let (pixmap, gc) = if let Some((pixmap, gc, _, _)) = storage { - let _ = xproto::free_gc(conn, *gc)?; - let _ = xproto::free_pixmap(conn, *pixmap)?; - (*pixmap, *gc) - } else { - (conn.generate_id()?, conn.generate_id()?) - }; - let _ = xproto::create_pixmap(conn, 32, pixmap, handle.root, width, height)?; - let _ = xproto::create_gc(conn, gc, pixmap, &Default::default())?; + let (pixmap, gc) = match *storage { + Some((pixmap, gc, w, h)) if (w, h) == (width, height) => (pixmap, gc), + _ => { + let (pixmap, gc) = if let Some((pixmap, gc, _, _)) = storage { + let _ = xproto::free_gc(conn, *gc)?; + let _ = xproto::free_pixmap(conn, *pixmap)?; + (*pixmap, *gc) + } else { + (conn.generate_id()?, conn.generate_id()?) + }; + let _ = xproto::create_pixmap(conn, 32, pixmap, handle.root, width, height)?; + let _ = xproto::create_gc(conn, gc, pixmap, &Default::default())?; - *storage = Some((pixmap, gc, width, height)); - (pixmap, gc) + *storage = Some((pixmap, gc, width, height)); + (pixmap, gc) + } }; let _ = xproto::put_image(