From 0129c498c2fe5770ed436df86937260d12024c89 Mon Sep 17 00:00:00 2001 From: itsjunetime Date: Sun, 1 Dec 2024 13:19:18 -0700 Subject: [PATCH] Fix document reloading with delete-then-write editors and redo BottomMessage updating to work better --- CHANGELOG.md | 2 ++ benches/rendering.rs | 1 + benches/utils.rs | 2 ++ src/main.rs | 86 ++++++++++++++++++++++++++++---------------- src/renderer.rs | 40 ++++++++++++++------- src/tui.rs | 67 +++++++++++++++++++--------------- 6 files changed, 126 insertions(+), 72 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c052e8..df66720 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ - Fixed a bug where jumping to a page out of range could result in weird `esc` key behavior - Added CI ([#31](https://github.com/itsjunetime/tdf/pull/31), thank you [@Kriejstal](https://github.com/Kreijstal)) - Changed global allocator to [`mimalloc`](https://github.com/purpleprotocol/mimalloc_rust) for slightly improved performance +- Fixed issue with document reloading not working when files are intermedially deleted +- Fixed a lot of weirdness with bottom message layering/updating # v0.1.0 diff --git a/benches/rendering.rs b/benches/rendering.rs index 0655dc2..d7e6930 100644 --- a/benches/rendering.rs +++ b/benches/rendering.rs @@ -94,6 +94,7 @@ async fn render_all_files(path: &'static str) -> Vec { while let Some(info) = from_render_rx.next().await { match info.expect("Renderer ran into an error while rendering") { + RenderInfo::Reloaded => (), RenderInfo::NumPages(num) => fill_default(&mut pages, num), RenderInfo::Page(page) => { let num = page.page; diff --git a/benches/utils.rs b/benches/utils.rs index d1dab1c..98474d4 100644 --- a/benches/utils.rs +++ b/benches/utils.rs @@ -21,6 +21,8 @@ pub fn handle_renderer_msg( to_converter_tx.send(ConverterMsg::NumPages(num)).unwrap(); } Ok(RenderInfo::Page(info)) => to_converter_tx.send(ConverterMsg::AddImg(info)).unwrap(), + // We can ignore the `Reloaded` variant 'cause that's only used to send info to the TUI + Ok(RenderInfo::Reloaded) => (), Err(e) => panic!("Got error from renderer: {e:?}") } } diff --git a/src/main.rs b/src/main.rs index 1c430e3..174642a 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,4 +1,5 @@ use std::{ + ffi::OsString, io::{stdout, Read, Write}, num::NonZeroUsize, path::PathBuf @@ -19,7 +20,7 @@ use ratatui_image::picker::Picker; use tdf::{ converter::{run_conversion_loop, ConvertedPage, ConverterMsg}, renderer::{self, RenderError, RenderInfo, RenderNotif}, - tui::{InputAction, Tui} + tui::{BottomMessage, InputAction, MessageSetting, Tui} }; // Dummy struct for easy errors in main @@ -57,12 +58,26 @@ async fn main() -> Result<(), Box> { let (render_tx, tui_rx) = flume::unbounded(); let watch_to_tui_tx = render_tx.clone(); - let mut watcher = - notify::recommended_watcher(on_notify_ev(watch_to_tui_tx, watch_to_render_tx))?; + let mut watcher = notify::recommended_watcher(on_notify_ev( + watch_to_tui_tx, + watch_to_render_tx, + path.file_name() + .ok_or("Path does not have a last component??")? + .to_owned() + ))?; - // We're making this nonrecursive 'cause we're just watching a single file, so there's nothing - // to recurse into - watcher.watch(&path, RecursiveMode::NonRecursive)?; + // So we have to watch the parent directory of the file that we are interested in because the + // `notify` library works on inodes, and if the file is deleted, that inode is gone as well, + // and then the notify library just gives up on trying to watch for the file reappearing. Imo + // they should start watching the parent directory if the file is deleted, and then wait for it + // to reappear and then begin watching it again, but whatever. It seems they've made their + // opinion on this clear + // (https://github.com/notify-rs/notify/issues/113#issuecomment-281836995) so whatever, guess + // we have to do this annoying workaround. + watcher.watch( + path.parent().expect("The root directory is not a PDF"), + RecursiveMode::NonRecursive + )?; // TODO: Handle non-utf8 file names? Maybe by constructing a CString and passing that in to the // poppler stuff instead of a rust string? @@ -187,20 +202,16 @@ async fn main() -> Result<(), Box> { }, Some(renderer_msg) = tui_rx.next() => { match renderer_msg { - // if an Ok comes through, we know the error has been resolved ('cause it kinda - // bails whenever we run into an error) so just clear it - Ok(render_info) => { - match render_info { - RenderInfo::NumPages(num) => { - tui.set_n_pages(num); - to_converter.send(ConverterMsg::NumPages(num))?; - }, - RenderInfo::Page(info) => { - tui.got_num_results_on_page(info.page, info.search_results); - to_converter.send(ConverterMsg::AddImg(info))?; - }, - } - tui.set_bottom_msg(None); + Ok(render_info) => match render_info { + RenderInfo::NumPages(num) => { + tui.set_n_pages(num); + to_converter.send(ConverterMsg::NumPages(num))?; + }, + RenderInfo::Page(info) => { + tui.got_num_results_on_page(info.page, info.search_results); + to_converter.send(ConverterMsg::AddImg(info))?; + }, + RenderInfo::Reloaded => tui.set_msg(MessageSetting::Some(BottomMessage::Reloaded)), }, Err(e) => tui.show_error(e), } @@ -240,7 +251,8 @@ async fn main() -> Result<(), Box> { fn on_notify_ev( to_tui_tx: flume::Sender>, - to_render_tx: flume::Sender + to_render_tx: flume::Sender, + file_name: OsString ) -> impl Fn(notify::Result) { move |res| match res { // If we get an error here, and then an error sending, everything's going wrong. Just give @@ -248,15 +260,29 @@ fn on_notify_ev( Err(e) => to_tui_tx.send(Err(RenderError::Notify(e))).unwrap(), // TODO: Should we match EventKind::Rename and propogate that so that the other parts of the // process know that too? Or should that be - Ok(ev) => match ev.kind { - EventKind::Access(_) => (), - EventKind::Remove(_) => - drop(to_tui_tx.send(Err(RenderError::Render("File was deleted".into())))), - // This shouldn't fail to send unless the receiver gets disconnected. If that's - // happened, then like the main thread has panicked or something, so it doesn't matter - // we don't handle the error here. - EventKind::Other | EventKind::Any | EventKind::Create(_) | EventKind::Modify(_) => - drop(to_render_tx.send(RenderNotif::Reload)), + Ok(ev) => { + // We only watch the parent directory (see the comment above `watcher.watch` in `fn + // main`) so we need to filter out events to only ones that pertain to the single file + // we care about + if !ev + .paths + .iter() + .any(|path| path.file_name().is_some_and(|f| f == file_name)) + { + return; + } + + match ev.kind { + EventKind::Access(_) => (), + EventKind::Remove(_) => to_tui_tx + .send(Err(RenderError::Render("File was deleted".into()))) + .unwrap(), + // This shouldn't fail to send unless the receiver gets disconnected. If that's + // happened, then like the main thread has panicked or something, so it doesn't matter + // we don't handle the error here. + EventKind::Other | EventKind::Any | EventKind::Create(_) | EventKind::Modify(_) => + to_render_tx.send(RenderNotif::Reload).unwrap(), + } } } } diff --git a/src/renderer.rs b/src/renderer.rs index 2a23b91..5c39227 100644 --- a/src/renderer.rs +++ b/src/renderer.rs @@ -25,7 +25,8 @@ pub enum RenderError { pub enum RenderInfo { NumPages(usize), - Page(PageInfo) + Page(PageInfo), + Reloaded } #[derive(Clone)] @@ -91,24 +92,37 @@ pub fn start_rendering( let col_w = size.width / size.columns; let col_h = size.height / size.rows; + let mut stored_doc = None; + 'reload: loop { let doc = match Document::from_file(path, None) { Err(e) => { // if there's an error, tell the main loop sender.send(Err(RenderError::Doc(e)))?; - // then wait for a reload notif (since what probably happened is that the file was - // temporarily removed to facilitate a save or something like that) - while let Ok(msg) = receiver.recv() { - // and once that comes, just try to reload again - if let RenderNotif::Reload = msg { - continue 'reload; + + match stored_doc { + Some(ref d) => d, + None => { + // then wait for a reload notif (since what probably happened is that the file was + // temporarily removed to facilitate a save or something like that) + while let Ok(msg) = receiver.recv() { + // and once that comes, just try to reload again + if let RenderNotif::Reload = msg { + continue 'reload; + } + } + // if that while let Ok ever fails and we exit out of that loop, the main thread is + // done, so we're fine to just return + return Ok(()); } } - // if that while let Ok ever fails and we exit out of that loop, the main thread is - // done, so we're fine to just return - return Ok(()); } - Ok(d) => d + Ok(d) => { + if stored_doc.is_some() { + sender.send(Ok(RenderInfo::Reloaded))?; + } + &*stored_doc.insert(d) + } }; let n_pages = doc.n_pages() as usize; @@ -199,8 +213,8 @@ pub fn start_rendering( // we only want to continue if one of the following is met: // 1. It failed to render last time (we want to retry) // 2. The `contained_term` is set to None (representing 'Unknown'), meaning that we - // need to at least check if it contains the current term to see if it needs a - // re-render + // need to at least check if it contains the current term to see if it needs a + // re-render if rendered.successful && rendered.contained_term.is_some() { continue; } diff --git a/src/tui.rs b/src/tui.rs index a508ab0..b45cd9a 100644 --- a/src/tui.rs +++ b/src/tui.rs @@ -43,7 +43,8 @@ pub enum BottomMessage { Help, SearchResults(String), Error(String), - Input(InputCommand) + Input(InputCommand), + Reloaded } pub enum InputCommand { @@ -186,6 +187,7 @@ impl Tui { Color::Blue ) } + BottomMessage::Reloaded => ("Document was reloaded!".into(), Color::Blue) }; let span = Span::styled(msg_str, Style::new().fg(color)); @@ -394,13 +396,13 @@ impl Tui { 'k' => self.change_page(PageChange::Prev, ChangeAmount::WholeScreen), 'q' => Some(InputAction::QuitApp), 'g' => { - self.set_bottom_msg(Some(BottomMessage::Input( + self.set_msg(MessageSetting::Some(BottomMessage::Input( InputCommand::GoToPage(0) ))); Some(InputAction::Redraw) } '/' => { - self.set_bottom_msg(Some(BottomMessage::Input( + self.set_msg(MessageSetting::Some(BottomMessage::Input( InputCommand::Search(String::new()) ))); Some(InputAction::Redraw) @@ -451,34 +453,33 @@ impl Tui { KeyCode::Esc => match self.bottom_msg { BottomMessage::Help => Some(InputAction::QuitApp), _ => { - self.set_bottom_msg(None); + // When we hit escape, we just want to pop off the current message and + // show the underlying one. + self.set_msg(MessageSetting::Pop); Some(InputAction::Redraw) } }, KeyCode::Enter => { - let BottomMessage::Input(_) = self.bottom_msg else { + let mut default = BottomMessage::default(); + std::mem::swap(&mut self.bottom_msg, &mut default); + let BottomMessage::Input(ref cmd) = default else { + std::mem::swap(&mut self.bottom_msg, &mut default); return None; }; - self.set_bottom_msg(None); - let Some(BottomMessage::Input(ref cmd)) = self.prev_msg else { - // We need to verify it's an input msg currently, and only then take it - // and replace it by a default Help message. Don't exactly know how to - // do this otherwise. - unreachable!(); - }; - match cmd { // Only forward the command if it's within range InputCommand::GoToPage(page) => { - let page = *page; + // We need to subtract 1 b/c they're tracked internally as + // 0-indexed but input and displayed as 1-indexed + let zero_page = page.saturating_sub(1); let rendered_len = self.rendered.len(); - if page < rendered_len { - self.set_page(page); - Some(InputAction::JumpingToPage(page)) + if zero_page < rendered_len { + self.set_page(zero_page); + Some(InputAction::JumpingToPage(zero_page)) } else { - self.set_bottom_msg(Some(BottomMessage::Error( + self.set_msg(MessageSetting::Some(BottomMessage::Error( format!("Cannot jump to page {page}; there are only {rendered_len} pages in the document") ))); Some(InputAction::Redraw) @@ -490,14 +491,14 @@ impl Tui { // We only want to show search results if there would actually be // data to show if !term.is_empty() { - self.set_bottom_msg(Some(BottomMessage::SearchResults( - term.clone() - ))); + self.set_msg(MessageSetting::Some( + BottomMessage::SearchResults(term.clone()) + )); } else { // else, if it's not empty, we just want to reset the bottom // area to show the default data; we don't want it to like show // the data from a previous search - self.set_bottom_msg(Some(BottomMessage::Help)); + self.set_msg(MessageSetting::Reset); } // Reset all the search results @@ -531,7 +532,7 @@ impl Tui { } pub fn show_error(&mut self, err: RenderError) { - self.set_bottom_msg(Some(BottomMessage::Error(match err { + self.set_msg(MessageSetting::Some(BottomMessage::Error(match err { RenderError::Notify(e) => format!("Auto-reload failed: {e}"), RenderError::Doc(e) => format!("Couldn't open document: {e}"), RenderError::Render(e) => format!("Couldn't render page: {e}") @@ -548,17 +549,18 @@ impl Tui { // We have `msg` as optional so that if they reset it to none, it'll replace it with // `prev_msg`, but if they reset it to something else, it'll put the current thing in prev_msg - pub fn set_bottom_msg(&mut self, msg: Option) { + pub fn set_msg(&mut self, msg: MessageSetting) { match msg { - Some(mut msg) => { + MessageSetting::Some(mut msg) => { std::mem::swap(&mut self.bottom_msg, &mut msg); self.prev_msg = Some(msg); } - None => { - let mut new_bottom = self.prev_msg.take().unwrap_or_default(); - std::mem::swap(&mut self.bottom_msg, &mut new_bottom); - self.prev_msg = Some(new_bottom); + MessageSetting::Default => self.set_msg(MessageSetting::Some(BottomMessage::default())), + MessageSetting::Reset => { + self.prev_msg = None; + self.bottom_msg = BottomMessage::default(); } + MessageSetting::Pop => self.bottom_msg = self.prev_msg.take().unwrap_or_default() } } } @@ -581,3 +583,10 @@ enum ChangeAmount { WholeScreen, Single } + +pub enum MessageSetting { + Some(BottomMessage), + Default, + Reset, + Pop +}