clickable PDF links; added webbrowser, query link handling, and click… #135
Conversation
… mapping; cleanups
itsjunetime
left a comment
itsjunetime
left a comment
There was a problem hiding this comment.
I really like this code, and I really appreciate your work on it, but I don't think it's functioning fully right now - I downloaded this example PDF, and when I open it with this PR in tdf, I can only get the links to work if I click them at exactly the right spots - clicking on most of any given link doesn't seem to do anything. Maybe there's some slight adjustments of the location checking on the tui and renderer side that needs to be adjusted?
I'll take a look at this a bit later with this same PDF to see if I can figure out why the links aren't registering; for now, if you could resolve these small issues I've attached, that would be appreciated :)
| })?; | ||
| // await the renderer reply; the renderer now returns | ||
| // Result<Option<LinkTarget>, String> so we can display errors | ||
| match resp_rx.recv_async().await { |
There was a problem hiding this comment.
I think I'd prefer this recv_async call to be moved into the tokio::select so that the ui doesn't freeze while waiting for the response (just in case it takes a second, for whatever reason).
| // Result<Option<LinkTarget>, String> so we can display errors | ||
| match resp_rx.recv_async().await { | ||
| Ok(Ok(Some(LinkTarget::Uri(uri)))) => { | ||
| if let Err(e) = webbrowser::open(&uri) { |
There was a problem hiding this comment.
I don't think I'm convinced on using webbrowser, since it always only opens in a browser (as opposed to just whatever the default handler for the mimetype is). For example, if you received a pdf and an attached spreadsheet, and you clicked on a link to the spreadsheet in the pdf, it would definitely be weird for your browser to open instead of your spreadsheet app.
There was a problem hiding this comment.
If we know for certain all other URIs are going to be websites, I'd be chill with it. But I don't think we can know that for certain? I don't know.
| /// Query which link (if any) is under the provided mupdf device coordinates for the given | ||
| /// page. Returns Ok(Some(LinkTarget)) if a link is found, Ok(None) if no link, or | ||
| /// Err(mupdf::error::Error) on mupdf failures. | ||
| fn query_link_at( |
There was a problem hiding this comment.
really appreciate the doc comments :)
| 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 })); | ||
| } |
There was a problem hiding this comment.
Seems that links can have both a destination and a uri? Very weird, didn't expect that. I'm fine with us preferring URIs over destination, but we may just want to keep this in mind in case other pdf viewers honor both at the same time.
| .unwrap_or_else(|e| { | ||
| panic!("Renderer failed to send link-query response: {e}") | ||
| }); |
There was a problem hiding this comment.
| .unwrap_or_else(|e| { | |
| panic!("Renderer failed to send link-query response: {e}") | |
| }); | |
| .expect("Renderer failed to send link-query response"); |
.expect will print out the error along with your message, so there's no need for unwrap_or_else
| 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)) | ||
| } | ||
| } |
There was a problem hiding this comment.
| 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)) | |
| } | |
| } | |
| let msg = query_link_at(&doc, qpage, mupdf_x_px, mupdf_y_px, area_w, area_h, fit_or_fill) | |
| .map_err(|e| format!("Failed to query links: {e}")); | |
| resp.send(msg) |
| let mut page_infos: Vec<(usize, u16)> = self | ||
| .rendered[self.page..] | ||
| .iter() | ||
| .take(pages_shown) | ||
| .enumerate() | ||
| .filter_map(|(idx, p)| p.img.as_ref().map(|img| (self.page + idx, img.w_h().0))) | ||
| .collect(); | ||
|
|
||
| if self.page_constraints.r_to_l { | ||
| page_infos.reverse(); | ||
| } |
There was a problem hiding this comment.
I think we can avoid this collect (and thus should probably remove it in the layout code as well) with something like the following:
let end_idx = (self.page + pages_shown).min(self.rendered.len());
let mut page_infos = self
.rendered[self.page..end_idx]
.iter()
.enumerate()
.filter_map(|(idx, p)| p.img.as_ref().map(|img| (self.page + idx, img.w_h().0)));
if self.page_constraints.r_to_l {
run_checks_with_iter(page_infos.rev());
} else {
run_checks_With_iter(page_infos);
}where run_checks_with_iter does all the stuff you have below this, checking the location and bounds to see where we clicked.
I'm having a somewhat difficult time wrapping my head around all the reversing and calculations and stuff here, though, so maybe I'm wrong.
2 participants
Previously worked on "Improved internal PDF link handling and navigation" closed because my branch was behind ended up losing my changes but with this. Remade the same commit improved link handing, no matter the screen size whether book view or phone view external link handling works perfectly.