Link support (issue #81) #95

perhurt · 2025-08-21T08:40:38Z

Implements basic link support for tdf

shows the number of links in the footer: Tab: X links
pressing opens a menu with the links on the current page
the menu is traversed using
links in the menu can be opened () or copied ('c').
'q' is used to close the link menu
if the page is changed when the link menu is open, the links for that page is shown

The link support is intended to be very easy to use when using the keyboard. Just yourself to the right link and open or copy it.

Things in need of consideration:

Testing: only tested on macOS. Should work on other platforms as cross-platform crates are used for copying and link-opening
Is this enough? Currently only url links are supported. One could implement support for all kinds of links
Is the approach ok? Personally (of course), I think this solution is preferable to having links highlighted and clickable in the document (as tdf is a terminal-based viewer) 😄

- Extract links from PDF pages - Add interactive links menu with Tab key navigation - Support link actions: Enter (open with system app), c (copy URL) - Display footer status showing link count when present - Implement URL opening and clipboard functionality - Add visual link selection with ">" indicator and truncation for long URLs

- Add copypasta 0.10.2 dependency for cross-platform clipboard support

- Add open 5.3.2 dependency for cross-platform URL opening

- removed remains of an early attempt using annotated links - removed clippy errors/warnings

removed leftover code trying to handle internal links. If that is a desirable feature, it can be added separately.

itsjunetime

Overall, I really do like this. Thank you so much for working on it, it looks really good so far and I really appreciate your time.

There are a few questions and nits I would like resolved before we merge this in. Most of those are included in my linked comments, but for those that aren't:

You mentioned in your comment above that q should quit the link menu, but it seems that only esc quits that menu, while q still quits the whole app. Is that intended?
In my quick test, it didn't show the links for the page that was currently displayed - it showed them for the next page. Afaict, your code to load the links looks correct, so I'd guess this is a weird mishandling of the mupdf C api? I don't really know for sure, so I'll try to look into that soon.

Also, I think it could be nice to maybe make this instead highlight the links on the page (instead of just listing them) and show the whole link at the bottom of the page when you have it highlighted, but that's definitely more complicated, so I think that should be a project for the future. And it'll definitely be much easier to do once we get this infrastructure merged in.

itsjunetime · 2025-08-21T22:48:30Z

src/renderer.rs


+fn extract_page_links(page: &Page) -> Result<Vec<Link>, mupdf::error::Error> {
+	let links = page.links()?;
+	let mut unique_links = Vec::new();


Imo if a page has multiple of the same link, it's still useful to show them.

For example, imagine if a page has 3 links, where the first and third link to the same site, but the document has the hyperlinks hidden behind normal text so people can't see exactly where each link links to, they can just see that it's a link. In this case, it would be better to display all three so that people can easily correlate what link (on the link menu) corresponds to what text (on the displayed page).

If only two links are shown, that could be confusing and they may not be sure which link they actually want to click on, since they can't tell what link corresponds to what bit of text.

When I made my initial "link menu design", the repetition of links didn't seem logical to me. However, if we're going from menu -> highlighting of links in the document, this makes total sense.

itsjunetime · 2025-08-21T22:52:54Z

src/renderer.rs

+
+	for link in links {
+		// Only include HTTP/HTTPS URLs, skip internal links and empty URIs
+		if link.uri.starts_with("http") {


In the future, we probably want to extend this to support jumping to a different part of the doc, but for now I think this is good.

Yes. When we have a good implementation of the basic http-functionality, I'll try to make a pass over internal links

itsjunetime · 2025-08-21T22:56:18Z

src/tui.rs


+		if self.showing_links_menu {
+			self.render_links_menu(frame);
+			return KittyDisplay::ClearImages;


Honestly imo it would look better to not clear the images, but rather just show the link menu on top of the doc, clearing the part of the image that's underneath it. I'm not too opinionated, though.

I guess this will go away if we remove the menu

itsjunetime · 2025-08-21T22:57:42Z

src/tui.rs

+				let mut to_display = Vec::new();
+
+				for (idx, (width, img)) in page_widths.into_iter().enumerate() {
+					let page_rect = Rect { width, ..img_area };
+					let page_num = idx + self.page;
+
+					// Render the page
+					let maybe_img = Self::render_single_page(frame, img, page_rect);
+
+					img_area.x += width;
+					if let Some((img, pos)) = maybe_img {
+						to_display.push(KittyReadyToDisplay {
 							img,
-							page_num: idx + self.page,
+							page_num,
 							pos,
 							display_loc: DisplayLocation::default()
-						})
-					})
-					.collect::<Vec<_>>();
+						});
+					}
+				}


Is there any reason for this change? Or is it just stylistic?

Let me check this more thoroughyl. I think there is a reason, but I have forgotten it. Should've answered this immediately when you wrote the question:P

itsjunetime · 2025-08-21T22:58:15Z

src/tui.rs

+					spans.push(Span::styled(
+						format!("Tab: {} links", rendered[page_num].links.len()),
+						Style::new().fg(Color::Yellow)
+					));


I love how this bottom section looks, great job there.

itsjunetime · 2025-08-21T23:27:52Z

src/tui.rs

+						if let Some(rendered_info) = self.rendered.get(self.page) {
+							if let Some(link) = rendered_info.links.get(self.selected_link_index) {
+								if link.uri.starts_with("http") {
+									let _ = open::that(&link.uri);
+								}
+							}
+						}


Suggested change

if let Some(rendered_info) = self.rendered.get(self.page) {

if let Some(link) = rendered_info.links.get(self.selected_link_index) {

if link.uri.starts_with("http") {

let _ = open::that(&link.uri);

}

}

}

if let Some(rendered_info) = self.rendered.get(self.page) &&

let Some(link) = rendered_info.links.get(self.selected_link_index) &&

link.uri.starts_with("http")

{

let _ = open::that(&link.uri);

}

I also think the error should be bubbled up to the user here if possible, I just can't remember how to do that off the top of my head

itsjunetime · 2025-08-21T23:31:07Z

src/tui.rs

+					KeyCode::Tab => {
+						if self.showing_links_menu {
+							// Cycle through links when menu is open
+							if let Some(rendered_info) = self.rendered.get(self.page) {


I won't add another suggestion since they'll probably need to be fixed by rustfmt anyways, but I think collapsing the ifs here with let-chains would be nice. I know they were stabilized after the current msrv, but I'm more than happy to bump that so we can use let chains.

And also, since this if let and if !rendered_info.links.is_empty is present in both branches, maybe we should pull that outside of the if self.showing_links_menu

itsjunetime · 2025-08-21T23:36:07Z

src/tui.rs

+		frame.render_widget(Clear, frame_area);
+
+		let block = Block::new()
+			.title("Links in Current Page (Tab: cycle, Enter: open, c: copy, Esc: close)")


I'm kinda conflicted on having the help text here in the title. I think it would be better to either:

Move it to the footer (so that it doesn't make the block so wide and doesn't create as much visual noise for someone who already knows the actions), OR

Make another tab on the help view that just shows these keybindings

What do you think?

itsjunetime · 2025-08-21T23:37:43Z

src/tui.rs

+						format!("> {}. ", i + 1) // Selected item gets ">" prefix
+					} else {
+						format!("  {}. ", i + 1) // Normal items get spaces


Could we use the fill/alignment formatting specifiers to left-pad the number to ensure they're all aligned if there's 10+ links?

itsjunetime · 2025-08-21T23:41:31Z

src/tui.rs

+			.map(str::len)
+			.max()
+			.unwrap_or(30)
+			.min(80)


Maybe I'm overengineering this, but I think it would be nice to have this be dynamically computed - e.g.

const MAX_ALLOWED_LINK_LEN: usize = 70; // ... later on // 5 for `...` and `> .` at beginning, `log(10)` for longest number display, 4 for padding .min(MAX_ALLOWED_LINK_LEN + 3 + 2 + links_text.len().log(10) + 4)

I don't know if log(10) is actually exactly the calculation we want, but I think you get the idea.

perhurt · 2025-08-22T05:53:20Z

Let me answer your two main comments before going into the code :)

Overall, I really do like this. Thank you so much for working on it, it looks really good so far and I really appreciate your time.

There are a few questions and nits I would like resolved before we merge this in. Most of those are included in my linked comments, but for those that aren't:

You mentioned in your comment above that q should quit the link menu, but it seems that only esc quits that menu, while q still quits the whole app. Is that intended?

I actually don't remember, but would guess it's an "advanced typo". It would be silly to flip the key binding logic for this specific feature. Let's rewrite the menu text.

In my quick test, it didn't show the links for the page that was currently displayed - it showed them for the next page. Afaict, your code to load the links looks correct, so I'd guess this is a weird mishandling of the mupdf C api? I don't really know for sure, so I'll try to look into that soon.

When I did my tests I didn't notice any such problems. I had the correct page<->links mapping the whole time, even when the reader was wide enough to display 3-4 pages in parallell.

What operating system do you use? I did my tests on macOS. I guess the debugging has to start on this level :)

Also, I think it could be nice to maybe make this instead highlight the links on the page (instead of just listing them) and show the whole link at the bottom of the page when you have it highlighted, but that's definitely more complicated, so I think that should be a project for the future. And it'll definitely be much easier to do once we get this infrastructure merged in.

That seems like a good idea! I fumbled around testing different approaches and chose the current one for some reason. Let's try your approach!

I will address the rest of your comments after making a pass at these high-level issues. Maybe some of them will automagically solve themselves

itsjunetime · 2025-09-06T21:06:42Z

I actually don't remember, but would guess it's an "advanced typo". It would be silly to flip the key binding logic for this specific feature. Let's rewrite the menu text.

Sounds good

When I did my tests I didn't notice any such problems. I had the correct page<->links mapping the whole time, even when the reader was wide enough to display 3-4 pages in parallell.

What operating system do you use? I did my tests on macOS. I guess the debugging has to start on this level :)

Sorry, this one error is on me - I downloaded a test doc that had a confusing mix of intra-doc and internet links and thought it wasn't working correctly. This code seems to do what it was intended to do, so you're all good there.

That seems like a good idea! I fumbled around testing different approaches and chose the current one for some reason. Let's try your approach!

Sounds good, then - if you'd like to refactor this to do that instead, I'm super chill with it. If you'd rather leave that off for later, though, I totally understand. I think the menu you've added in this PR is really good as it is.

perhurt added 6 commits August 21, 2025 10:10

Replace platform-specific clipboard code with copypasta crate

9a30f4b

- Add copypasta 0.10.2 dependency for cross-platform clipboard support

Replace platform-specific link opening with open crate

28aadc4

- Add open 5.3.2 dependency for cross-platform URL opening

missed to commit Cargo.lock

bf4b333

cleanup:

0d1c26e

- removed remains of an early attempt using annotated links - removed clippy errors/warnings

removed handling of internal links

fefa0a8

removed leftover code trying to handle internal links. If that is a desirable feature, it can be added separately.

itsjunetime mentioned this pull request Aug 21, 2025

Customize the help page for the current context #97

Closed

itsjunetime reviewed Aug 21, 2025

View reviewed changes

itsjunetime mentioned this pull request Nov 7, 2025

Improved internal PDF link handling and navigation #108

Open

Link support (issue #81) #95

Are you sure you want to change the base?

Link support (issue #81) #95

Uh oh!

Conversation

perhurt commented Aug 21, 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!

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!

perhurt commented Aug 22, 2025

Uh oh!

itsjunetime commented Sep 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants