From 2a77a7d50c9038f6b74349a99a077ba2795dac84 Mon Sep 17 00:00:00 2001 From: kokoISnoTarget <72217393+kokoISnoTarget@users.noreply.github.com> Date: Wed, 3 Apr 2024 02:38:58 +0200 Subject: [PATCH] fix+refactor: link handling (#292) * feat+fix+refactor Now we set the current directory to the directory of the file so that relative link can be opened by open::that, also added an error message when open::that fails. * clippy * fix: Removed edge case where opening an relative file would result in crashing the file watcher. * fmt * test: Use a tempdir for history's test env * test: Use a tempfile where needed in CLI test env * fix: Canonicalize the tempdir as well * chore: Dont check for typos on test data --------- Co-authored-by: Cosmic Horror --- src/history.rs | 26 +++++++-- src/main.rs | 124 ++++++++++++++++------------------------ src/opts/mod.rs | 2 +- src/opts/tests/parse.rs | 65 ++++++++++++++------- typos.toml | 2 + 5 files changed, 115 insertions(+), 104 deletions(-) create mode 100644 typos.toml diff --git a/src/history.rs b/src/history.rs index bc5ec01f..939253b2 100644 --- a/src/history.rs +++ b/src/history.rs @@ -7,9 +7,10 @@ pub struct History { } impl History { - pub fn new(path_buf: PathBuf) -> Self { + pub fn new(path: &Path) -> Self { + let canonicalized = path.canonicalize().unwrap(); Self { - history: vec![path_buf], + history: vec![canonicalized], index: 0, } } @@ -22,6 +23,8 @@ impl History { } pub fn make_next(&mut self, file_path: PathBuf) { + let file_path = file_path.canonicalize().unwrap(); + self.history.truncate(self.index + 1); self.history.push(file_path); self.index += 1; @@ -49,15 +52,26 @@ impl History { #[cfg(test)] mod tests { + use std::fs; + use super::*; #[test] fn sanity() { - let root = PathBuf::from("a"); - let fork1 = PathBuf::from("b"); - let fork2 = PathBuf::from("c"); + let temp_dir = tempfile::Builder::new() + .prefix("inlyne-tests-") + .tempdir() + .unwrap(); + let temp_path = temp_dir.path().canonicalize().unwrap(); + + let root = temp_path.join("a"); + let fork1 = temp_path.join("b"); + let fork2 = temp_path.join("c"); + fs::write(&root, "a").unwrap(); + fs::write(&fork1, "b").unwrap(); + fs::write(&fork2, "c").unwrap(); - let mut hist = History::new(root.clone()); + let mut hist = History::new(&root); assert_eq!(hist.get_path(), root); assert_eq!(hist.previous(), None); diff --git a/src/main.rs b/src/main.rs index 280c8c85..56e55045 100644 --- a/src/main.rs +++ b/src/main.rs @@ -205,6 +205,8 @@ impl Inlyne { let watcher = Watcher::spawn(event_loop.create_proxy(), file_path.clone()); + let _ = file_path.parent().map(std::env::set_current_dir); + Ok(Self { opts, window, @@ -455,89 +457,57 @@ impl Inlyne { screen_size, self.renderer.zoom, ) { - if let Hoverable::Summary(summary) = hoverable { - let mut hidden = summary.hidden.borrow_mut(); - *hidden = !*hidden; - event_loop_proxy - .send_event(InlyneEvent::Reposition) - .unwrap(); - } - - let maybe_link = match hoverable { - Hoverable::Image(Image { is_link, .. }) => is_link, - Hoverable::Text(Text { link, .. }) => link, - Hoverable::Summary(_) => &None, - }; - - if let Some(link) = maybe_link { - let maybe_path = PathBuf::from_str(link).ok(); - let is_local_md = maybe_path.as_ref().map_or(false, |p| { - p.extension().map_or(false, |ext| ext == "md") - && !p.to_str().map_or(false, |s| s.starts_with("http")) - }); - if is_local_md { - // Open markdown files ourselves - let path = maybe_path.expect("not a path"); - // Handle relative paths and make them - // absolute by prepending current - // parent - let path = if path.is_relative() { - // Simply canonicalizing it doesn't suffice and leads to "no such file or directory" - let current_parent = self - .opts - .history - .get_path() - .parent() - .expect("no current parent"); - let mut normalized_link = path.as_path(); - if let Ok(stripped) = normalized_link - .strip_prefix(std::path::Component::CurDir) - { - normalized_link = stripped; - } - let mut link = current_parent.to_path_buf(); - link.push(normalized_link); - link - } else { - path - }; - // Open them in a new window, akin to what a browser does - if modifiers.shift() { - Command::new( - std::env::current_exe() - .unwrap_or_else(|_| "inlyne".into()), - ) - .args(Opts::program_args(&path)) - .spawn() - .expect("Could not spawn new inlyne instance"); - } else { - match read_to_string(&path) { - Ok(contents) => { - self.update_file(&path, contents); - self.opts.history.make_next(path); - } - Err(err) => { - tracing::warn!( + match hoverable { + Hoverable::Image(Image { is_link: Some(link), .. }) | + Hoverable::Text(Text { link: Some(link), .. }) => { + let path = PathBuf::from_str(link).unwrap(); // Can't fail + + if path.extension().map_or(false, |ext| ext == "md") + && !path.to_str().map_or(false, |s| s.starts_with("http")) { + // Open them in a new window, akin to what a browser does + if modifiers.shift() { + Command::new( + std::env::current_exe() + .unwrap_or_else(|_| "inlyne".into()), + ) + .args(Opts::program_args(&path)) + .spawn() + .expect("Could not spawn new inlyne instance"); + } else { + match read_to_string(&path) { + Ok(contents) => { + self.update_file(&path, contents); + self.opts.history.make_next(path); + } + Err(err) => { + tracing::warn!( "Failed loading markdown file at {}\nError: {}", path.display(), err, ); + } } } + } else if let Some(anchor_pos) = + self.renderer.positioner.anchors.get(&link.to_lowercase()) + { + self.renderer.set_scroll_y(*anchor_pos); + self.window.request_redraw(); + self.window.set_cursor_icon(CursorIcon::Default); + } else if let Err(e) = open::that(link) { + tracing::error!("Could not open link: {e} from {:?}", std::env::current_dir()) } - } else if let Some(anchor_pos) = - self.renderer.positioner.anchors.get(&link.to_lowercase()) - { - self.renderer.set_scroll_y(*anchor_pos); - self.window.request_redraw(); - self.window.set_cursor_icon(CursorIcon::Default); - } else { - open::that(link).unwrap(); - } - } else if self.renderer.selection.is_none() { - // Only set selection when not over link - self.renderer.selection = Some((last_loc, last_loc)); - } + }, + Hoverable::Summary(summary) => { + let mut hidden = summary.hidden.borrow_mut(); + *hidden = !*hidden; + event_loop_proxy + .send_event(InlyneEvent::Reposition) + .unwrap(); + self.renderer.selection = Some((last_loc, last_loc)) + }, + _ => self.renderer.selection = Some((last_loc, last_loc)), + }; } else if self.renderer.selection.is_none() { self.renderer.selection = Some((last_loc, last_loc)); } @@ -630,6 +600,8 @@ impl Inlyne { match read_to_string(&file_path) { Ok(contents) => { self.update_file(&file_path, contents); + let parent = file_path.parent().expect("File should have parent directory"); + std::env::set_current_dir(parent).expect("Could not set current directory."); } Err(err) => { tracing::warn!( diff --git a/src/opts/mod.rs b/src/opts/mod.rs index c8e7d7a1..f8ad1184 100644 --- a/src/opts/mod.rs +++ b/src/opts/mod.rs @@ -138,7 +138,7 @@ impl Opts { }; Ok(Self { - history: History::new(file_path), + history: History::new(&file_path), theme, scale, page_width, diff --git a/src/opts/tests/parse.rs b/src/opts/tests/parse.rs index 2dac43a2..feb46cf1 100644 --- a/src/opts/tests/parse.rs +++ b/src/opts/tests/parse.rs @@ -1,8 +1,9 @@ use std::ffi::OsString; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use clap::{CommandFactory, Parser}; use pretty_assertions::assert_eq; +use tempfile::NamedTempFile; use crate::color::{SyntaxTheme, Theme, ThemeDefaults}; use crate::history::History; @@ -17,10 +18,20 @@ fn gen_args(args: Vec<&str>) -> Vec { .collect() } +fn temp_md_file() -> (NamedTempFile, String) { + let temp_file = tempfile::Builder::new() + .prefix("inlyne-tests-") + .suffix(".md") + .tempfile() + .unwrap(); + let path = temp_file.path().to_str().unwrap().to_owned(); + (temp_file, path) +} + impl Opts { - fn mostly_default(file_path: impl Into) -> Self { + fn mostly_default(file_path: impl AsRef) -> Self { Self { - history: History::new(file_path.into()), + history: History::new(file_path.as_ref()), theme: ResolvedTheme::Light.as_theme(), scale: None, page_width: None, @@ -55,9 +66,11 @@ fn debug_assert() { fn defaults() { init_test_log(); + let (_tmp, md_file) = temp_md_file(); + assert_eq!( Opts::parse_and_load_with_system_theme( - Cli::try_parse_from(gen_args(vec!["file.md"])) + Cli::try_parse_from(gen_args(vec![&md_file])) .unwrap() .into_view() .unwrap(), @@ -65,7 +78,7 @@ fn defaults() { None, ) .unwrap(), - Opts::mostly_default("file.md") + Opts::mostly_default(&md_file) ); } @@ -73,6 +86,8 @@ fn defaults() { fn config_overrides_default() { init_test_log(); + let (_tmp, md_file) = temp_md_file(); + // Light system theme with dark in config let config = config::Config { theme: Some(ThemeType::Dark), @@ -80,7 +95,7 @@ fn config_overrides_default() { }; assert_eq!( Opts::parse_and_load_with_system_theme( - Cli::try_parse_from(gen_args(vec!["file.md"])) + Cli::try_parse_from(gen_args(vec![&md_file])) .unwrap() .into_view() .unwrap(), @@ -91,7 +106,7 @@ fn config_overrides_default() { Opts { theme: ResolvedTheme::Dark.as_theme(), color_scheme: Some(ResolvedTheme::Dark), - ..Opts::mostly_default("file.md") + ..Opts::mostly_default(&md_file) } ); @@ -102,7 +117,7 @@ fn config_overrides_default() { }; assert_eq!( Opts::parse_and_load_with_system_theme( - Cli::try_parse_from(gen_args(vec!["file.md"])) + Cli::try_parse_from(gen_args(vec![&md_file])) .unwrap() .into_view() .unwrap(), @@ -113,7 +128,7 @@ fn config_overrides_default() { Opts { theme: ResolvedTheme::Light.as_theme(), color_scheme: Some(ResolvedTheme::Light), - ..Opts::mostly_default("file.md") + ..Opts::mostly_default(&md_file) } ); @@ -123,7 +138,7 @@ fn config_overrides_default() { }; assert_eq!( Opts::parse_and_load_with_system_theme( - Cli::try_parse_from(gen_args(vec!["file.md"])) + Cli::try_parse_from(gen_args(vec![&md_file])) .unwrap() .into_view() .unwrap(), @@ -133,7 +148,7 @@ fn config_overrides_default() { .unwrap(), Opts { scale: Some(1.5), - ..Opts::mostly_default("file.md") + ..Opts::mostly_default(&md_file) } ); } @@ -142,9 +157,11 @@ fn config_overrides_default() { fn from_cli() { init_test_log(); + let (_tmp, md_file) = temp_md_file(); + assert_eq!( Opts::parse_and_load_with_system_theme( - Cli::try_parse_from(gen_args(vec!["--theme", "dark", "file.md"])) + Cli::try_parse_from(gen_args(vec!["--theme", "dark", &md_file])) .unwrap() .into_view() .unwrap(), @@ -155,7 +172,7 @@ fn from_cli() { Opts { theme: ResolvedTheme::Dark.as_theme(), color_scheme: Some(ResolvedTheme::Dark), - ..Opts::mostly_default("file.md") + ..Opts::mostly_default(&md_file) } ); @@ -167,7 +184,7 @@ fn from_cli() { }; assert_eq!( Opts::parse_and_load_with_system_theme( - Cli::try_parse_from(gen_args(vec!["--scale", "1.5", "file.md"])) + Cli::try_parse_from(gen_args(vec!["--scale", "1.5", &md_file])) .unwrap() .into_view() .unwrap(), @@ -179,7 +196,7 @@ fn from_cli() { theme: ResolvedTheme::Dark.as_theme(), scale: Some(1.5), color_scheme: Some(ResolvedTheme::Dark), - ..Opts::mostly_default("file.md") + ..Opts::mostly_default(&md_file) } ); } @@ -188,13 +205,15 @@ fn from_cli() { fn cli_kitchen_sink() { init_test_log(); + let (_tmp, md_file) = temp_md_file(); + #[rustfmt::skip] let args = gen_args(vec![ "--theme", "dark", "--scale", "1.5", "--config", "/path/to/file.toml", "--page-width", "500", - "file.md", + &md_file, ]); assert_eq!( Opts::parse_and_load_with_system_theme( @@ -208,7 +227,7 @@ fn cli_kitchen_sink() { scale: Some(1.5), theme: ResolvedTheme::Dark.as_theme(), color_scheme: Some(ResolvedTheme::Dark), - ..Opts::mostly_default("file.md") + ..Opts::mostly_default(&md_file) } ); } @@ -217,6 +236,8 @@ fn cli_kitchen_sink() { fn builtin_syntax_theme() { init_test_log(); + let (_tmp, md_file) = temp_md_file(); + let mut config = config::Config::default(); config.light_theme = Some(config::OptionalTheme { code_highlighter: Some(SyntaxTheme::Defaults(ThemeDefaults::SolarizedLight)), @@ -224,7 +245,7 @@ fn builtin_syntax_theme() { }); let opts = Opts::parse_and_load_with_system_theme( - Cli::try_parse_from(gen_args(vec!["file.md"])) + Cli::try_parse_from(gen_args(vec![&md_file])) .unwrap() .into_view() .unwrap(), @@ -241,8 +262,6 @@ fn builtin_syntax_theme() { #[test] fn custom_syntax_theme() { - init_test_log(); - fn config_with_theme_at(path: PathBuf) -> config::Config { let mut config = config::Config::default(); config.light_theme = Some(config::OptionalTheme { @@ -252,7 +271,11 @@ fn custom_syntax_theme() { config } - let args = Cli::try_parse_from(gen_args(vec!["file.md"])) + init_test_log(); + + let (_tmp, md_file) = temp_md_file(); + + let args = Cli::try_parse_from(gen_args(vec![&md_file])) .unwrap() .into_view() .unwrap(); diff --git a/typos.toml b/typos.toml new file mode 100644 index 00000000..67b5763a --- /dev/null +++ b/typos.toml @@ -0,0 +1,2 @@ +[files] +extend-exclude = ["assets/manual_test_data"]