Fix document reloading with delete-then-write editors and redo BottomMessage updating to work better

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
 

benches/rendering.rs

@@ -94,6 +94,7 @@ async fn render_all_files(path: &'static str) -> Vec<PageInfo> {
 
 	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;

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:?}")
 	}
 }

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<dyn std::error::Error>> {
 	let (render_tx, tui_rx) = flume::unbounded();
 	let watch_to_tui_tx = render_tx.clone();
 
-	let mut watcher =
+	let mut watcher = notify::recommended_watcher(on_notify_ev(
-		notify::recommended_watcher(on_notify_ev(watch_to_tui_tx, watch_to_render_tx))?;
+		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
+	// So we have to watch the parent directory of the file that we are interested in because the
-	// to recurse into
+	// `notify` library works on inodes, and if the file is deleted, that inode is gone as well,
-	watcher.watch(&path, RecursiveMode::NonRecursive)?;
+	// 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<dyn std::error::Error>> {
 			},
 			Some(renderer_msg) = tui_rx.next() => {
 				match renderer_msg {
-					// if an Ok comes through, we know the error has been resolved ('cause it kinda
+					Ok(render_info) => match render_info {
-					// bails whenever we run into an error) so just clear it
+						RenderInfo::NumPages(num) => {
-					Ok(render_info) => {
+							tui.set_n_pages(num);
-						match render_info {
+							to_converter.send(ConverterMsg::NumPages(num))?;
-							RenderInfo::NumPages(num) => {
+						},
-								tui.set_n_pages(num);
+						RenderInfo::Page(info) => {
-								to_converter.send(ConverterMsg::NumPages(num))?;
+							tui.got_num_results_on_page(info.page, info.search_results);
-							},
+							to_converter.send(ConverterMsg::AddImg(info))?;
-							RenderInfo::Page(info) => {
+						},
-								tui.got_num_results_on_page(info.page, info.search_results);
+						RenderInfo::Reloaded => tui.set_msg(MessageSetting::Some(BottomMessage::Reloaded)),
-								to_converter.send(ConverterMsg::AddImg(info))?;
-							},
-						}
-						tui.set_bottom_msg(None);
 					},
 					Err(e) => tui.show_error(e),
 				}
@@ -240,7 +251,8 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
 
 fn on_notify_ev(
 	to_tui_tx: flume::Sender<Result<RenderInfo, RenderError>>,
-	to_render_tx: flume::Sender<RenderNotif>
+	to_render_tx: flume::Sender<RenderNotif>,
+	file_name: OsString
 ) -> impl Fn(notify::Result<Event>) {
 	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 {
+		Ok(ev) => {
-			EventKind::Access(_) => (),
+			// We only watch the parent directory (see the comment above `watcher.watch` in `fn
-			EventKind::Remove(_) =>
+			// main`) so we need to filter out events to only ones that pertain to the single file
-				drop(to_tui_tx.send(Err(RenderError::Render("File was deleted".into())))),
+			// we care about
-			// This shouldn't fail to send unless the receiver gets disconnected. If that's
+			if !ev
-			// happened, then like the main thread has panicked or something, so it doesn't matter
+				.paths
-			// we don't handle the error here.
+				.iter()
-			EventKind::Other | EventKind::Any | EventKind::Create(_) | EventKind::Modify(_) =>
+				.any(|path| path.file_name().is_some_and(|f| f == file_name))
-				drop(to_render_tx.send(RenderNotif::Reload)),
+			{
+				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(),
+			}
 		}
 	}
 }

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)
+				match stored_doc {
-				while let Ok(msg) = receiver.recv() {
+					Some(ref d) => d,
-					// and once that comes, just try to reload again
+					None => {
-					if let RenderNotif::Reload = msg {
+						// then wait for a reload notif (since what probably happened is that the file was
-						continue 'reload;
+						// 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
+				//	  need to at least check if it contains the current term to see if it needs a
-				//    re-render
+				//	  re-render
 				if rendered.successful && rendered.contained_term.is_some() {
 					continue;
 				}

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 {
+								if zero_page < rendered_len {
-									self.set_page(page);
+									self.set_page(zero_page);
-									Some(InputAction::JumpingToPage(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(
+									self.set_msg(MessageSetting::Some(
-										term.clone()
+										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<BottomMessage>) {
+	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 => {
+			MessageSetting::Default => self.set_msg(MessageSetting::Some(BottomMessage::default())),
-				let mut new_bottom = self.prev_msg.take().unwrap_or_default();
+			MessageSetting::Reset => {
-				std::mem::swap(&mut self.bottom_msg, &mut new_bottom);
+				self.prev_msg = None;
-				self.prev_msg = Some(new_bottom);
+				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
+}