Skip to content

Implement debounce time for automatic reloading #117

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
itsjunetime merged 1 commit into from
Nov 26, 2025
Merged

Implement debounce time for automatic reloading #117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ flexi_logger = "0.31"

# for tracing with tokio-console
console-subscriber = { version = "0.5.0", optional = true }
debounce = "0.2.2"

[profile.production]
inherits = "release"
Expand Down
88 changes: 60 additions & 28 deletions src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@ use std::{
borrow::Cow,
ffi::OsString,
io::{BufReader, Read as _, Stdout, Write as _, stdout},
path::PathBuf
mem,
path::PathBuf,
sync::{Arc, Mutex},
time::Duration
};

use crossterm::{
Expand All @@ -17,6 +20,7 @@ use crossterm::{
enable_raw_mode, window_size
}
};
use debounce::EventDebouncer;
use flexi_logger::FileSpec;
use flume::{Sender, r#async::RecvStream};
use futures_util::{FutureExt as _, stream::StreamExt as _};
Expand Down Expand Up @@ -83,6 +87,8 @@ async fn inner_main() -> Result<(), WrappedErr> {
#[cfg(feature = "tracing")]
console_subscriber::init();

const DEFAULT_DEBOUNCE_DELAY: Duration = Duration::from_millis(50);

let flags = xflags::parse_or_exit! {
/// Display the pdf with the pages starting at the right hand size and moving left and
/// adjust input keys to match
Expand All @@ -91,6 +97,9 @@ async fn inner_main() -> Result<(), WrappedErr> {
optional -m,--max-wide max_wide: NonZeroUsize
/// Fullscreen the pdf (hide document name, page count, etc)
optional -f,--fullscreen
/// The time to wait for the file to stop changing before reloading, in milliseconds.
/// Defaults to 50ms.
optional --reload-delay reload_delay: u64
/// The number of pages to prerender surrounding the currently-shown page; 0 means no
/// limit. By default, there is no limit.
optional -p,--prerender prerender: usize
Expand Down Expand Up @@ -162,7 +171,10 @@ async fn inner_main() -> Result<(), WrappedErr> {
watch_to_render_tx,
path.file_name()
.ok_or_else(|| WrappedErr("Path does not have a last component??".into()))?
.to_owned()
.to_owned(),
flags
.reload_delay
.map_or(DEFAULT_DEBOUNCE_DELAY, Duration::from_millis)
))
.map_err(|e| WrappedErr(format!("Couldn't start watching the provided file: {e}").into()))?;

Expand Down Expand Up @@ -454,38 +466,58 @@ async fn enter_redraw_loop(
fn on_notify_ev(
to_tui_tx: flume::Sender<Result<RenderInfo, RenderError>>,
to_render_tx: flume::Sender<RenderNotif>,
file_name: OsString
file_name: OsString,
debounce_delay: Duration
) -> 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
// up lol.
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) => {
// 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::Converting("File was deleted".into())))
.unwrap(),
let last_event: Mutex<Result<(), RenderError>> = Mutex::new(Ok(()));
let last_event = Arc::new(last_event);

let debouncer = EventDebouncer::new(debounce_delay, {
let last_event = last_event.clone();
move |()| {
let event = mem::replace(&mut *last_event.lock().unwrap(), Ok(()));
match event {
// 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(),
Ok(()) => to_render_tx.send(RenderNotif::Reload).unwrap(),
// If we get an error here, and then an error sending, everything's going wrong. Just give
// up lol.
Err(e) => to_tui_tx.send(Err(e)).unwrap()
}
}
});

move |res| {
let event = match res {
Err(e) => Err(RenderError::Notify(e)),

// 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) => {
// 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(_) => return,
EventKind::Remove(_) => Err(RenderError::Converting("File was deleted".into())),
EventKind::Other
| EventKind::Any
| EventKind::Create(_)
| EventKind::Modify(_) => Ok(())
}
}
};
*last_event.lock().unwrap() = event;
debouncer.put(());
Comment on lines +519 to +520
Copy link
Owner

@itsjunetime itsjunetime Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although (I think) I understand why you used both the mutex and the debouncer, I don't think this is necessary - even though the docs for the debouncer say that it filters out only duplicates, it actually (in practice) filters out all but the last event within the timeframe, even if the other events weren't duplicates. You can tell this if you look at the code, but also if you just look at the type signature for EventDebouncer - it doesn't require T: PartialEq.

So I think we can probably just get rid of last_event altogether and just do debouncer.put(event) and that'll be a lot cleaner and do more what we want.

Also, as a sidenote: I noticed that the notify crate (which we use for watching the filesystem) has its own debouncer crate(s), so those might be cleaner or better suited for what we're doing here? Probably doesn't matter, though, either is probably fine.

Copy link
Collaborator Author

@maxdexh maxdexh Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EventDebouncer::put requires PartialEq and does the deduplication, see https://docs.rs/debounce/latest/src/debounce/buffer.rs.html#78. Debouncing is pretty simple to implement, so there it probably doesn't matter which crate is used for this (or if it's hand-written), but yeah notify-debouncer-mini is probably a better choice here, I didn't know it existed.

Copy link
Owner

@itsjunetime itsjunetime Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait, yeah you're totally right. i was definitely confused as to what part of the code did what.

i'm still not crazy about the debouncer + mutex, but it does the job. we can look into switching crates if it turns out this one isn't fulfilling its purpose for whatever reason

Copy link
Collaborator Author

@maxdexh maxdexh Nov 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, this is kind of weird. The callback expected by by notify-debouncer-mini takes events in this form:

pub type DebounceEventResult = Result<Vec<DebouncedEvent>, Error>;

pub enum DebouncedEventKind {
    /// No precise events
    Any,
    /// Event but debounce timed out (for example continuous writes)
    AnyContinuous,
}

/// A debounced event.
///
/// Does not emit any specific event type on purpose, only distinguishes between an any event and a continuous any event.
pub struct DebouncedEvent {
    pub path: PathBuf,
    pub kind: DebouncedEventKind,
}

This seems kind of useless? Am I not seeing something here?

Copy link
Collaborator Author

@maxdexh maxdexh Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

notify-debouncer-full gives proper events, but to me it seems really overbuilt for this job.

Copy link
Collaborator Author

@maxdexh maxdexh Nov 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there is a good way to write this without a mutex. It definitely needs synchronization, since the timeout might expire the same moment that a new event comes in. The only alternative I can think would be to replace it atomically, but that's not possible without allocating it, since RenderError is 56 bytes.

Also in reality the mutex will barely have any contention, so I think this is fine.

Copy link
Owner

@itsjunetime itsjunetime Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it'll be fine - my comment about the mutex was more so noting that the debouncer already contains a mutex, so we're using 2 for the job of 1, but they're both going to be in very low contention, so that's not really an issue (like you said).

Copy link
Collaborator Author

@maxdexh maxdexh Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EventDebouncer::put requires PartialEq and does the deduplication, see https://docs.rs/debounce/latest/src/debounce/buffer.rs.html#78.

That was also not the right link 🙃

Copy link
Collaborator Author

@maxdexh maxdexh Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extra mutex could be removed by calling put with the event wrapped in a newtype whose PartialEq always returns true, but yuck :D

}
}

Expand Down
Loading