Implement debounce time for automatic reloading

[maxdexh]

When running tdf on the output of pdflatex, one can observe

• When pdflatex truncates the output file, pages that have not yet been rewritten become blank
• pdflatex actually updates the output file so fast that tdf breaks entirely on non-trivial tex files, causing the program to just show "loading" when switching pages until it loads the file from scratch. I suspect this is coming from mupdf.

While in the case of pdflatex, this can be worked around by using the pdflatex && mv ... approach, other pdf viewers like zathura handle this without issue.

This PR addresses these issues using the same approach as zathura: a debounce time for file reloads. The handler for the file watcher now delays sending RenderNotif::Reload to the renderer until the file has stopped changing for 50ms (implemented using the debounce crate). This causes reloads to be capped at 20 times per second, which completely fixes the second issue.

The time can be configured via a cli option. Choosing a time around 1 second works well for pdflatex, effectively alleviating the first issue by mimicking the behavior of zathura.

[maxdexh]

Converting to draft because this does not entirely fix the issue, since mupdf still gives errors for every file change. This still prevents the document from being displayed, although without breaking as badly as before. Probably needs some debounce as well. Nevermind, I ran the unpatched version on accident and it happened to not break the same way that time 🤦

[maxdexh]

Here is a video demonstrating the current behavior:

broken.mp4

And here is a video demonstrating the new behavior when passing --reload-delay 1000:

fixed.mp4

Note that using the default delay of 50ms will still cause the pages to become white, and mupdf will still complain about the bad format, but will go back to normal shortly after the file has finished writing.

[itsjunetime]

This definitely looks goods, works well, and I really appreciate you adding it, but I think it can be a bit cleaner and better.

[itsjunetime]

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.

[maxdexh]

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.

[itsjunetime]

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

[maxdexh]

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?

[maxdexh]

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

[maxdexh]

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.

[itsjunetime]

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).

[maxdexh]

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 🙃

[maxdexh]

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

[itsjunetime]

All looks good now :) Just gonna wait for CI and then I'll merge it in

[maxdexh]

Welp, in my defense clippy was using up 8GB of ram when I pkilled it 😅

[maxdexh]

Want me to squash the commits while I'm at it?

[itsjunetime]

yeah, feel free to squash - i like having lots of lints on the project since it’s getting more contributors, but maybe i need to pare them down cause clippy should not be taking up 8gb lmao

gonna look into if i need to update the toolchain or smth

[maxdexh]

Also got rid of the merge, did a rebase instead

[maxdexh]

Gonna use tdf to present the slides for my tutorial at uni tomorrow :D

Thank you for making this!

[itsjunetime]

Beautiful, love it. And thanks for using it :)