diff --git a/posts/zathura.md b/posts/zathura.md new file mode 100644 index 0000000..faca6a2 --- /dev/null +++ b/posts/zathura.md @@ -0,0 +1,172 @@ +# contributing to zathura as a novice + +2023-10-24 + +Earlier this year I made a [patch](https://git.pwmt.org/pwmt/zathura/-/merge_requests/80) +for Zathura, the open-source PDF reader. +In this post, I'll document the journey of how I diagnosed the root cause of a bug in Zathura, +then wrote a bugfix for it. +As this is my first "real" patch that got accepted, the +most important take-away for me is that it's actually not that hard to contribute to open-source. + +# the bug + +First of all, as seen in [my last blog post](../typst-notes/), I make use of Zathura extensively in my note-taking setup. +On one half of my window, I write markup in Neovim, and in the other half Zathura displays the rendered result. +Every time I save the markup, it is automatically re-compiled, then Zathura reloads the document. +Normally, this works really well, but if you're reloading documents often, you'll notice that there's flicker every time: + +[ ![](/public/img/zathura/flicker-thumb.gif) ](/public/img/zathura/flicker.mp4) + +Of course, this is not a deal-breaking issue. +However, it is definitely annoying if you save often, and even more so if your editor automatically saves on change. + +Upon finding this bug, I did what any normal person does and searched online to see if there was any fixes for it. +I stumbled upon this GitLab issue: [_Prevent flickering when reloading document_](https://git.pwmt.org/pwmt/zathura/-/issues/268). +Unfortunately for me, the thread was nearly empty, and the last comment was from another user seeking a solution. + +[Another thread](https://superuser.com/questions/1459927/zathura-flicker-when-updating-pdf) +on Stack Exchange was also a dead end, with no real answer to the problem. + +At this point, there was clearly no easy solution to remove flicker from Zathura. +Since this software is open source, I decided to take a look at the source code in an attempt to diagnose the issue. + +# investigating source code + +Zathura's source code is available at their git repository. +I cloned it to my projects folder: + +``` +git clone https://git.pwmt.org/pwmt/zathura +``` + +At the time, the latest commit was `c7baa2f6` ("Make icon visible on dark backgrounds"). +If you want to follow along with the exact same code as I saw, run this command: + +``` +git checkout c7baa2f6 +``` + +At first glance, it definitely can seem daunting to navigate an unfamiliar codebase. +However, these days, we have language servers, which makes it much faster. +Essentially, language servers are plugins for text editors that give language "intelligence". +With them, you can, for example, jump to the definition of a variable, or get completions when writing code. +I find that language servers are useful especially in situations like these, where you might have no idea where a symbol comes from. +For C, which Zathura is written in, I use [clangd](https://github.com/clangd/clangd), which integrates easily with Neovim. + +A necessary step to use clangd is to have a `compile_commands.json` file. +Thankfully, with Zathura's build system, Meson, this is automatically generated during the build. +I followed the build instructions at the bottom of the README: + +``` +meson build +cd build +ninja +``` + +Then, I copied the resulting file back into the root directory. + +``` +cp compile_commands.json ../ +``` + +Now, Neovim will automatically have language intelligence. + +Still, we have no idea where the code responsible for the bug lies. +Considering the bug happens upon reload, it would make sense to search for code related to reloading documents. +To do this, I searched for the term "reload" using ripgrep (you could use `git grep` too): + +``` +rg "reload" +``` + +From here, I saw an interesting function signature in `zathura/shortcuts.c`: + +``` +sc_reload(girara_session_t* session, girara_argument_t* UNUSED(argument) +``` + +This looks like it could be the code responsible for reloading. +Looking at it, this is the function body (heavily abridged): + +``` +bool +sc_reload() +{ + /* close current document */ + document_close(zathura, true); + + /* reopen document */ + document_open(); +} +``` + +Surprisingly, reloading a document is implemented by closing and reopening the document, +rather than a separate in-place refresh feature. +This would explain why the screen flickers during reloads: +the document disappears entirely before the new one is loaded in its place. +It seems like there might be a moment between closing and reopening where there is nothing on screen, +which is what causes jarring flicker. + +# fixing the bug + +So now, we've successfully diagnosed the root cause of the flickering. +However, this provides no clear path towards solving it. + +At this point, I decided to check out the `document_close` and `document_open` functions mentioned above. +After studying these two functions, something caught my eye: +during opening, we create `zathura->pages`, and during closing, we free it. +Looking at the definition, `zathura->pages` is an array of pointers to GtkWidgets. + +As an experiment, I commented out a bunch of code from `document_close` to see what happens if the pages are not freed. +In my head, I thought it might leave the old pages on screen and let the new ones replace it. +Obviously, that didn't happen, and I got multiple segmentation faults, crashes and memory leaks (womp womp). + +Now, I was practically at a dead end again. +However, I remembered something the author of the Stack Exchange thread tried regarding this issue: +they attempted to set `render-loading` to false to fix the flicker. +On default settings, when the document is reloaded, a "Loading..." prompt flickers on screen: + +![Zathura loading prompt](/public/img/zathura/loading.jpg) + +Turning it off just makes the flickering a blank screen. +I decided to look for code related to this prompt with `rg "loading"`. +Doing that, I found an interesting function in `zathura/page-widget.c` called `zathura_page_widget_draw`. +Skimming the code, it looks like it handles drawing pages to screen. +This was a promising new direction to look into. + +Near the end, there is a section that takes care of drawing the loading screen. +Reading above, we see that the loading screen only renders if the page has no Cairo surface. +Essentially, if the page hasn't fully loaded, put up a loading screen. + +Here, I had an idea: what if we replace the loading screen with the original page from before the reload? +That way, it would be a seamless transition from the original document to the new document. + +To implement this, I added two extra pointers to the `zathura` struct: +the `predecessor_document`, and the `predecessor_pages`. +When Zathura closes the document for a reload, it preserves the current document and pages as "predecessors". +Zathura will not free the predecessor document and pages immediately. +Then, in `zathura_page_widget_draw`, instead of drawing a loading screen, it will draw the predecessor pages. +Since having an extra buffer also uses more memory, I added a toggle option `smooth-reload` that switches this feature on and off. + +Of course, I'm skimming over many specific details here. +To see the exact code of my patch, you can look at the [commit](https://git.pwmt.org/pwmt/zathura/-/commit/257a2c968bcf67cf814aeab325800d4889d8df21) on Zathura's git repository. + +Anyways, here is the end result of the bug-fix, where the bottom window is patched and the top one isn't: + +[ ![Before and after the bugfix](/public/img/zathura/before-after-thumb.gif) ](/public/img/zathura/before-after.mp4) + +I was really ecstatic when Zathura first smoothly reloaded a document. +It finally worked! +After this initial success, I collected all these changes into a [merge request](https://git.pwmt.org/pwmt/zathura/-/merge_requests/80) on GitLab. +Finally, after a month of waiting, I got the maintainer to merge my patch. +All in all, it took a weekend in total to get familiar with the codebase and create this patch. + +# conclusion + +At the time of writing, this patch is still only available in the bleeding-edge/git build of Zathura. +To check that my code has made it into your version of Zathura, you can check in the man page `zathurarc(5)` for the option `smooth-reload`. + +Hopefully, this small contribution will improve the experience of future users of Zathura. +Again, what I want to show in this blog post is that contributing to open-source is actually not _that_ daunting. +When you encounter odd behaviour in your favourite software, or you want improve a feature, do not be afraid to just open it up and start tinkering. diff --git a/public/img/zathura/before-after-thumb.gif b/public/img/zathura/before-after-thumb.gif new file mode 100644 index 0000000..a11e9af Binary files /dev/null and b/public/img/zathura/before-after-thumb.gif differ diff --git a/public/img/zathura/before-after.mp4 b/public/img/zathura/before-after.mp4 new file mode 100644 index 0000000..514fa6b Binary files /dev/null and b/public/img/zathura/before-after.mp4 differ diff --git a/public/img/zathura/flicker-thumb.gif b/public/img/zathura/flicker-thumb.gif new file mode 100644 index 0000000..1a143e7 Binary files /dev/null and b/public/img/zathura/flicker-thumb.gif differ diff --git a/public/img/zathura/flicker.mp4 b/public/img/zathura/flicker.mp4 new file mode 100644 index 0000000..a7b46f3 Binary files /dev/null and b/public/img/zathura/flicker.mp4 differ diff --git a/public/img/zathura/loading.jpg b/public/img/zathura/loading.jpg new file mode 100644 index 0000000..eecff24 Binary files /dev/null and b/public/img/zathura/loading.jpg differ