Improved internal PDF link handling and navigation #108

ShemShadrack · 2025-11-05T19:39:37Z

Overview
Added click-to-open links inside PDFs — External URIs now open in the system browser.
Addresses #68.

Problem

Users could not interact with link annotations in PDFs.
This reduced usability, especially for large documents with many cross-references.

Solution

Implemented annotation hit-testing in the renderer thread and a query/response flow so the UI can ask “what link (if any) is under this click?”.
The renderer converts device pixel coordinates to PDF coordinates, inspects page link annotations, and returns a LinkTarget describing a URI or an internal GoTo.
The UI then opens URIs with the webbrowser crate.

Changelog

Added LinkTarget enum (renderer) to represent found link actions:
- Uri(String) — external link to open in browser.
- GoTo { page_index: usize, optional x/y } — internal document destination.
Added RenderNotif::QueryLinkAt message and one-shot reply channel so the UI can query the renderer for a link at given device pixel coordinates.
Implemented QueryLinkAt handling in renderer:
- Map device pixels → PDF coordinates (account for scale and PDF bottom-left origin).
- Iterate page link annotations via MuPDF high-level API when present; otherwise use mupdf-sys fallback.
- Resolve destinations to zero-based page indices where possible.
Wired UI click handling:
- Map terminal cell clicks → page + device pixel coords using existing layout math.
- Send QueryLinkAt and await the reply.
- On Uri → open with webbrowser::open(url).
- On GoTo → send RenderNotif::JumpToPage and ConverterMsg::GoToPage for navigation.
Added webbrowser dependency to Cargo.toml to open external links cross-platform.
Ensured all MuPDF document access happens on the renderer thread (preserves non-Send invariants).

Notes / Future Work

Improve GoTo precision by returning destination coordinates (x/y) and adding a JumpToPageWithOffset message so the viewer can center or scroll to the exact anchor.
Add an option to prompt before opening external links or to disable automatic opening for security.
Consider adding a visual cursor/hover highlight for link regions in the UI for better discoverability.

Disclaimer

For users running on Kitty, make sure the terminal window is sized correctly.
On large/laptop-sized windows, pages may render together (e.g., page 1 and page 2 combined).
On smaller/mobile-sized windows, pages render in standalone view, making links clickable as intended
Wide window → spread mode → misaligned link clicks.
Narrow/tall window → single page → links work perfectly.

Demo

tfd.mp4

itsjunetime

Overall, I think this is a fine approach, but there are a few concerns I have before merging it. I also think it's pertinent to point out that link support is also being worked on in #95, but that is a keyboard-oriented approach, so I don't think these two PRs conflict at all. It's totally fine to allow users to click or use their keyboard (though I think tdf should remain keyboard-forward, so to speak).

My biggest concern with this would be the details in your disclaimer. It seems that this doesn't work when multiple pages are on the screen? Is there any technical reason or just that you couldn't get it working? I'm happy to assist in figuring it out if there's no technical reason, as I don't think we could merge this if it doesn't work when multiple pages are displayed.

Feel free to re-request a review once you've responded to these concerns :)

src/main.rs

itsjunetime · 2025-11-06T23:52:11Z

src/main.rs

+								match resp_rx.recv_async().await {
+									Ok(Some(LinkTarget::Uri(uri))) => {
+										if let Err(e) = webbrowser::open(&uri) {
+											log::error!("Failed to open uri {}: {:?}", uri, e);


Since this is a user-facing application, I think log should only be used for debugging and context, and the tui should be used to show errors like this. Probably the Tui::show_error or Tui::set_msg fns.

Replaced the log::error with Tui::set_msg to display the error in the app's bottom message area.

itsjunetime · 2025-11-06T23:52:53Z

src/main.rs

+										to_renderer.send(RenderNotif::JumpToPage(page_index))?;
+										to_converter.send(ConverterMsg::GoToPage(page_index))?;
+									}
+									_ => (),


This error also shouldn't be swallowed; I doubt the channel will fail, but just in case it does, we want to either show that to the user or panic (if it requires that)

I have made an update on this if the channel fails due to a programmer error or receiver disconnnected the error will be surfaced.

itsjunetime · 2025-11-06T23:58:19Z

src/renderer.rs

+							let result = (|| -> Result<Option<LinkTarget>, mupdf::error::Error> {
+								// load the requested page and compute same scale as used when rendering
+								let page = doc.load_page(qpage as i32)?;
+								let bounds = page.bounds()?;
+								let page_dim = (bounds.x1 - bounds.x0, bounds.y1 - bounds.y0);
+								let scaled = scale_img_for_area(page_dim, (area_w, area_h), fit_or_fill);
+								let scale_factor = scaled.scale_factor;
+								let pdf_x = device_x_px / scale_factor;
+								let pdf_y = device_y_px / scale_factor;
+								if let Ok(mut links) = page.links() {
+									while let Some(link) = links.next() {
+										let lb = link.bounds;
+										if pdf_x >= lb.x0 && pdf_x <= lb.x1 && pdf_y >= lb.y0 && pdf_y <= lb.y1 {
+											// prefer URI if present and non-empty
+											if !link.uri.is_empty() {
+												return Ok(Some(LinkTarget::Uri(link.uri)));
+											}
+											if let Some(dest) = link.dest {
+												return Ok(Some(LinkTarget::GoTo { page_index: dest.loc.page_number as usize }));
+											}
+										}
+									}
+								}
+								Ok(None)
+							})();
+							match result {
+								Ok(Some(t)) => { let _ = resp.send(Some(t)); },
+								Ok(None) => { let _ = resp.send(None); },
+								Err(e) => {
+									log::error!("Failed to query links: {e}");
+									let _ = resp.send(None);
+								}
+							}


A few notes about this arm:

Since this is inside a macro, auto-complete and syntax highlighting support isn't great, so this code would probably be best moved to its own function. That would help with this nested indentation as well.

I understand the usefulness of the immediately-executed closure, but I think this would be better served by doing something like the following at the beginning:
let bounds = match doc.load_page(qpage as i32).and_then(Page::bounds) { Ok(b) => b, Err(e) => { resp.send(Err(e)).unwrap(); return; } }

Added a helper function query_link_at (above start_rendering that encapsulates the MuPDF link lookup and scaling math. It returns Result<Option<LinkTarget>, mupdf::error::Error>.

itsjunetime · 2025-11-07T00:00:01Z

src/renderer.rs

+								Ok(Some(t)) => { let _ = resp.send(Some(t)); },
+								Ok(None) => { let _ = resp.send(None); },
+								Err(e) => {
+									log::error!("Failed to query links: {e}");
+									let _ = resp.send(None);


Failure to send on this channel should probably result in a panic, since it means that our main thread has exited. But I think we should also try to report errors through the channel so that they can be reported to the user, thus making the channel a sender of Result<Option<LinkTarget>, String> (or whatever other error type would fit here)

RenderNotif::QueryLinkAt now carries resp: Sender<Result<Option<LinkTarget>, String>>
The link-query handler now sends:
Ok(Some(LinkTarget)) when a link is found,
Ok(None) when none is found,
Err(String) when there was an internal error querying links.

itsjunetime · 2025-11-07T00:00:50Z

src/renderer.rs

+								let page_dim = (bounds.x1 - bounds.x0, bounds.y1 - bounds.y0);
+								let scaled = scale_img_for_area(page_dim, (area_w, area_h), fit_or_fill);
+								let scale_factor = scaled.scale_factor;
+								let pdf_x = device_x_px / scale_factor;
+								let pdf_y = device_y_px / scale_factor;


I'll need to test out these conversions myself - converting between pixels and mupdf units and such has always been a horrible hassle that's given me a lot of trouble. Maybe I should take this opportunity to make it a bit easier and safer to do.

Yeah, I totally get that those conversions are always tricky and easy to get wrong.

itsjunetime · 2025-11-07T00:03:44Z

src/tui.rs

+			.map(|p| p.img.as_ref().map(|img| (img.w_h().0, img)))
+			.filter_map(|o| o)


Suggested change

.map(|p| p.img.as_ref().map(|img| (img.w_h().0, img)))

.filter_map(|o| o)

.filter_map(|p| p.img.as_ref().map(|img| (img.w_h().0, img)))

Or maybe you could do

Suggested change

.map(|p| p.img.as_ref().map(|img| (img.w_h().0, img)))

.filter_map(|o| o)

.filter_map(|p| p.img.as_ref())

.map(|o| (img.w_h().0, img))

But I can't remember if the borrow checker will allow that

I believe its better to leave as is because the two step:
.map(|p| p.img.as_ref().map(|img| (img.w_h().0, img))) .filter_map(|o| o)
is semantically the same as
.filter_map(|p| p.img.as_ref().map(|img| (img.w_h().0, img))) The compiler treats them equivalently

itsjunetime · 2025-11-07T00:10:34Z

src/tui.rs

+			.take_while(|p| p.img.is_some())
+			.map(|p| p.img.as_ref().map(|img| (img.w_h().0, img)))
+			.filter_map(|o| o)
+			.collect::<Vec<_>>();


Wait, now that I'm looking at this, I don't think we need to recalculate this each time - last_render already contains unused_width, so we don't need to calculate it down below, and then we can just iterate through the pages one at a time without needing to collect them into an intermediate vec. So if you'd be able to change that, that would be great.

kept a small intermediate vector I tried to render pages one-at-a-time without collecting, but that produced borrow-check errors because the code needs to hold multiple mutable references to images while constructing the final KittyDisplayreturn value.

itsjunetime · 2025-11-07T00:13:11Z

src/tui.rs

+				let local_row = row - area.y;
+				let device_x = f32::from(local_col) * f32::from(font_size.0);
+				let device_y = f32::from(local_row) * f32::from(font_size.1);
+				return Some((page_num, device_x, device_y));


Maybe it would be clearer to refer to these as 'mupdf' coordinates instead of 'device' coordinates? When I read 'device', I assumed it was referring to the terminal emulator as the 'device', but it seems it's referring to the mupdf rendering machine. Maybe you disagree, though; either's fine, just something to consider.

Made changes ans changed to mupdf

itsjunetime · 2025-11-07T00:14:08Z

src/tui.rs

-	Invert,
-	Fullscreen,
-	SwitchRenderZoom(crate::FitOrFill)
+    Redraw,


Weird that these got reformatted to use spaces... did you cargo fmt this? (if not, you should)

just did in my latest update

ShemShadrack · 2025-11-13T22:26:33Z

Added new updates, please review.

itsjunetime

I think there are a still a few things that need to be cleaned up, but I do think it's an improvement.

Were you able to get this working with multiple pages showing? I think that's still the biggest roadblock if it's not functional yet.

Also, if you'd make sure the lockfile is in sync (just run cargo build, then add, commit, and push the Cargo.lock), that'll get CI going again.

itsjunetime · 2025-11-15T22:03:55Z

src/main.rs

+									Ok(Ok(Some(LinkTarget::Uri(uri)))) => {
+										if let Err(e) = webbrowser::open(&uri) {
+											tui.set_msg(MessageSetting::Some(BottomMessage::Error(
+												format!("Failed to open uri {}: {e}", uri)


Suggested change

format!("Failed to open uri {}: {e}", uri)

format!("Failed to open uri {uri}: {e}")

Did a build, I've looked at it for the past week Its working right now attached a screen recording. let me know the feedback
https://github.com/user-attachments/assets/69c6b3c3-b82d-4f25-936c-14750b2631fa

itsjunetime · 2025-11-15T22:07:39Z

src/main.rs

+										if let Err(e) = to_renderer.send(RenderNotif::JumpToPage(page_index)) {
+											tui.set_msg(MessageSetting::Some(BottomMessage::Error(
+												format!("Failed to send jump-to-page to renderer: {e}")
+											)));
+										}
+
+										if let Err(e) = to_converter.send(ConverterMsg::GoToPage(page_index)) {
+											tui.set_msg(MessageSetting::Some(BottomMessage::Error(
+												format!("Failed to send jump-to-page to converter: {e}")
+											)));
+										}


I may be kinda going back on earlier suggestions, but I think these two sends specifically should short-circuit and cause the app to exit if they fail. We already do this below in the InputAction::JumpingToPage branch, and I think it still makes sense since these can only fail if the receiving thread exited (which would mean that a crucial part of the app is now inoperable and we should just force a restart).

Suggested change

if let Err(e) = to_renderer.send(RenderNotif::JumpToPage(page_index)) {

tui.set_msg(MessageSetting::Some(BottomMessage::Error(

format!("Failed to send jump-to-page to renderer: {e}")

)));

}

if let Err(e) = to_converter.send(ConverterMsg::GoToPage(page_index)) {

tui.set_msg(MessageSetting::Some(BottomMessage::Error(

format!("Failed to send jump-to-page to converter: {e}")

)));

}

to_renderer.send(RenderNotif::JumpToPage(page_index))?;

to_converter.send(ConverterMsg::GoToPage(page_index))?;

Yeah if the renderer or converter thread exits, the app is fundamentally broken and should restart. I'll replace those two error-handling blocks

itsjunetime · 2025-11-15T22:09:33Z

src/renderer.rs

+	let pdf_y = mupdf_y_px / scale_factor;
+
+	if let Ok(mut links) = page.links() {
+		while let Some(link) = links.next() {


Any reason to use while let Some(_) = ... here instead of just for link in links ?

I've done updates on this

itsjunetime · 2025-11-15T22:11:00Z

src/renderer.rs

+				}
+				if let Some(dest) = link.dest {
+					return Ok(Some(LinkTarget::GoTo { page_index: dest.loc.page_number as usize }));
+				}


This now makes me wonder what we should do if we encounter a link that has no destination and an empty URI - do you think we should report that to the user? Maybe that would just be a useless notification, since they probably don't care about links that effectively aren't links... idk, this is probably fine as-is, just something to think about in case you think something should be done there.

It's fine as-is current behavior is intuitive: The click just has no effect, which is what users expect from a non-functional link. No cognitive load. also silent skip is the right behavior,Silently doing nothing is better than showing an error message for something that isn't really an error

itsjunetime · 2025-11-15T22:12:56Z

src/renderer.rs

+							match query_link_at(&doc, qpage, mupdf_x_px, mupdf_y_px, area_w, area_h, fit_or_fill) {
+								Ok(Some(t)) => {
+									resp.send(Ok(Some(t))).unwrap_or_else(|e| {
+										panic!("Renderer failed to send link query response: {e}")
+									});
+								}
+								Ok(None) => {
+									resp.send(Ok(None)).unwrap_or_else(|e| {
+										panic!("Renderer failed to send link query response: {e}")
+									});
+								}
+								Err(e) => {
+									let err_str = format!("Failed to query links: {e}");
+									resp.send(Err(err_str)).unwrap_or_else(|e| {
+										panic!("Renderer failed to send link-query error: {e}")
+									});
+								}


This can be simplified a good bit, I think:

Suggested change

match query_link_at(&doc, qpage, mupdf_x_px, mupdf_y_px, area_w, area_h, fit_or_fill) {

Ok(Some(t)) => {

resp.send(Ok(Some(t))).unwrap_or_else(|e| {

panic!("Renderer failed to send link query response: {e}")

});

}

Ok(None) => {

resp.send(Ok(None)).unwrap_or_else(|e| {

panic!("Renderer failed to send link query response: {e}")

});

}

Err(e) => {

let err_str = format!("Failed to query links: {e}");

resp.send(Err(err_str)).unwrap_or_else(|e| {

panic!("Renderer failed to send link-query error: {e}")

});

}

match query_link_at(&doc, qpage, mupdf_x_px, mupdf_y_px, area_w, area_h, fit_or_fill) {

Ok(t) => resp.send(Ok(t)),

Err(e) => {

let err_str = format!("Failed to query links: {e}");

resp.send(Err(err_str))

}

}.unwrap_or_else(|e| {

panic!("Renderer failed to send link-query error: {e}")

});

You're right

itsjunetime · 2025-11-15T22:17:44Z

src/tui.rs

+			// We do this in two passes so we don't need to collect the images into an
+			// intermediate vector: first pass sums widths to determine centering, second
+			// pass renders pages one-at-a-time. We still avoid keeping references across
+			// the two passes.
+			let mut total_width: u16 = 0;
+			let mut pages_shown: usize = 0;
+
+			for (idx, page) in self.rendered[self.page..].iter_mut().enumerate() {
+				// only consider pages that have an image ready
+				if page.img.is_none() {
+					break;
+				}
+
+				if let Some(max) = self.page_constraints.max_wide {
+					if idx >= max.get() {
+						break;
+					}
+				}
+
+				let width = page.img.as_mut().map(|img| img.w_h().0).unwrap_or(0);
+
+				// stop if this would overflow the available area
+				match total_width.checked_add(width) {
+					Some(new_total) if new_total <= img_area.width => {
+						total_width = new_total;
+						pages_shown += 1;
+					}
+					_ => break
+				}
+			}
+
+			if pages_shown == 0 {
+				// If none are ready to render, just show the loading thing
+				Self::render_loading_in(frame, img_area);
+				return KittyDisplay::ClearImages;
+			}
+
+			execute!(stdout(), BeginSynchronizedUpdate).unwrap();
+
+			self.last_render.pages_shown = pages_shown;
+
+			let unused_width = img_area.width - total_width;
+			self.last_render.unused_width = unused_width;
+			img_area.x += unused_width / 2;
+
+			// Collect mutable references to the images for rendering in a single pass so
+			// we satisfy the borrow checker, then render them below.


Sorry, but I'm confused as to why this whole section is necessary - it seems to just be duplicating the work done below. Is there a reason that we need to do it twice, or is this doing something different?

Eliminated the double-pass in renderer refactored code now collects pages in a single filtering pass

itsjunetime · 2025-11-15T22:20:31Z

src/tui.rs

+		// First, compute how many pages will be shown and their total width using the same
+		// greedy logic as `render` so our positioning matches exactly.
+		let mut total_width: u16 = 0;
+		let mut pages_shown: usize = 0;
+
+		for (idx, page) in self.rendered[self.page..].iter().enumerate() {
+			// only consider pages that have an image ready
+			if page.img.is_none() {
+				break;
+			}
+
+			if let Some(max) = self.page_constraints.max_wide {
+				if idx >= max.get() {
+					break;
+				}
+			}
+
+			let width = page.img.as_ref().map(|img| img.w_h().0).unwrap_or(0);
+
+			match total_width.checked_add(width) {
+				Some(new_total) if new_total <= img_area.width => {
+					total_width = new_total;
+					pages_shown += 1;
+				}
+				_ => break,
+			}
+		}
+
+		if pages_shown == 0 {
+			return None;
+		}


I think we should be able to trust self.last_render to still be correct, so we don't need to recompute all of this - we can just pull the pages_shown and total_width info directly from that field for this.

Done some optimization and now reusing values instead of recomputing them

ShemShadrack · 2025-11-24T09:50:04Z

Made updates and removed redundant code blocks

Shem Torga added 3 commits November 5, 2025 02:50

Add feature: fix internal link handling in PDF viewer

06681dd

Remove .DS_Store from repository

b8c2662

Ignore .DS_Store files

e14c9cc

itsjunetime requested changes Nov 7, 2025

View reviewed changes

fix: align click coordinate calculation with page rendering logic

c8001f2

Merge branch 'main' into main

7644d92

itsjunetime requested changes Nov 15, 2025

View reviewed changes

Shem Torga and others added 3 commits November 24, 2025 00:09

Updates on highlighted issues

582e9c4

Updates

4395263

Merge branch 'main' into main

9602198

		.map(\|p\| p.img.as_ref().map(\|img\| (img.w_h().0, img)))
		.filter_map(\|o\| o)

	.map(\|p\| p.img.as_ref().map(\|img\| (img.w_h().0, img)))
	.filter_map(\|o\| o)
	.filter_map(\|p\| p.img.as_ref().map(\|img\| (img.w_h().0, img)))

	format!("Failed to open uri {}: {e}", uri)
	format!("Failed to open uri {uri}: {e}")

Improved internal PDF link handling and navigation #108

Are you sure you want to change the base?

Improved internal PDF link handling and navigation #108

Uh oh!

Conversation

ShemShadrack commented Nov 5, 2025

Uh oh!

itsjunetime left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShemShadrack commented Nov 13, 2025

Uh oh!

itsjunetime left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShemShadrack Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShemShadrack Nov 24, 2025 •

edited

Loading