Links with highlight#153
Conversation
itsjunetime
left a comment
itsjunetime
left a comment
There was a problem hiding this comment.
Generally I really like this; thanks so much for submitting it. I haven't tested it yet, but if you're willing to clean it up a bit and we can resolve the issue with page margins, I'd love to get this merged in.
| // check if it contains the current term to see if it needs a re-render | ||
| if rendered.successful && rendered.num_search_found.is_some() { | ||
| if rendered.successful && rendered.num_search_found.is_some() | ||
| && !(page_num == start_point && link_highlight_changed){ |
There was a problem hiding this comment.
I don't think I follow why we would want to not continue if page_num == start_point && link_highlight_changed - could you add an explanation to the comment?
There was a problem hiding this comment.
My thinking was that if !link_highlight_changed, then there is no reason to re-render it and that link highlighting should be limited to visible pages, thus no need to look at page_num != start_point.
That said I am certain this will need correcting when I fix displaying 2 pages (I'll also have to check if one can display more than 2 pages, I look almost exclusively at 1 page docs).
| fill_default::<PrevRender>(&mut rendered, n_pages.get()); | ||
| let mut start_point = 0; | ||
|
|
||
| // todo there must be a better way to do this |
There was a problem hiding this comment.
Honestly I think it works totally fine for now. It's a simple bool that seems to encapsulate all you care about.
| scale_factor: f32, | ||
| highlighted_link: Option<usize>, | ||
| ) -> Result<Vec<Link>, mupdf::error::Error> { | ||
| let link_groups = page.links()? |
There was a problem hiding this comment.
Could do with a comment on what link_groups represents - I assume it's a Vec with each item being, effectively, one link split across multiple Link items to represent that it exists on multiple pages at once?
| // but something like this is needed in case there are two separate links with the same uri | ||
| // across lines | ||
| let is_same = prev_link.uri == link.uri | ||
| && prev_link.dest == link.dest | ||
| && prev_link.bounds.y1 == link.bounds.y0; | ||
| // && prev_link.bounds.x1 == bounds.x1 | ||
| // && link.bounds.x0 == bounds.x0; |
There was a problem hiding this comment.
Could you give more detail on what exactly the problem is (with page margins; why are they weird right now?) and how we might be able to fix this? I don't think I fully understand how page margins could be getting in the way right now so I'm not quite sure what the problem exactly looks like or what the solution might be.
There was a problem hiding this comment.
A mupdf::Link is a rectangle + some destination. So in a document such as
.... li-\n
nk ...
mupdf creates two Link objects, one for each line.
As a user, my preference would be that if I cycle the highlight to this link, the entire link should be highlighted at once, instead of it being separated into two (or more if for some reason it is long enough to span more than 2 lines). Consequently, we need to detect whether two Link objects actually belong to the same link (which I termed link_group above but it can probably have a better name + clearer comments).
To check if they are the same link, then need to point at the same destination and their bounds (let's call them r0 and r1) should be "contiguous". A naive definition of contiguous here would be that r0.x1 is at the line end, r1 is right below r0 (r1.y0 is equal to r0.y1) and r1.x0 is at the line start (and similar if the link spans >2 lines). Here, line start and end are controlled by page margins etc on a quick search, I wasn't sure how one could determine this.
More generally, this naive definition will not work if a link is spanning columns (or pages when 2 pages are displayed, but that may be more acceptable). So I was hoping to look at mupdf more to see if there is a better way to do this. (Maybe it would be easier in #156?)
Also, this is a problem solely for active highlighting, so if covering all links with a clickable rectangle is enough, this problem would go away. But I imagine a keyboard-compatible solution would be preferable.
| acc | ||
| }); | ||
|
|
||
| Ok(link_groups |
There was a problem hiding this comment.
If we're just gonna re-iterate over link_groups, we can probably avoid constructing all those intermediate vecs, right?
| match bottom_msg { | ||
| BottomMessage::Help => { | ||
| // Create spans with different colors for help message and links info | ||
| let mut spans = vec![Span::styled( |
There was a problem hiding this comment.
Very small nit: I think it would probably be nice to keep the help suggestion in the bottom-right corner so that it doesn't move around a bunch, and just have the link text appear to its left.
| 'c' => highlighted_link.inspect(|&l| { | ||
| let link = ¤t_page_links[l]; | ||
| // Copy to clipboard using copypasta | ||
| use copypasta::{ClipboardContext, ClipboardProvider}; | ||
| if let LinkDestination::URI(ref uri) = link.dest | ||
| && let Ok(mut ctx) = ClipboardContext::new() { | ||
| let _ = ctx.set_contents(uri.clone()); | ||
| } | ||
| }).and(None), |
There was a problem hiding this comment.
I know this feels smart and fun but I think I would still appreciate a simple
'c' => {
if let Some(ref l) = highlighted_link { ... }
None
}:)
| } | ||
| } | ||
| }, | ||
| key @ (KeyCode::Tab | KeyCode::BackTab) => { |
There was a problem hiding this comment.
I don't think I have BackTab on my keyboard - is that maybe synthesized from shift+tab? If not, I feel like it would be nice to add that as a way to go back 1 link.
There was a problem hiding this comment.
I believe one needs to use KeyCode::BackTab over Tab + checking for Shift modifier and in my quick testing it gave the exact behavior you describe.
| (None, _) => Some(InputAction::HighlightLink(Some(0))), // No link is highlighted yet, highlight the first one | ||
| (Some(l), KeyCode::Tab) => Some(InputAction::HighlightLink(Some((l + 1) % n_links))), | ||
| (Some(l), KeyCode::BackTab) => Some(InputAction::HighlightLink(Some((l + n_links - 1) % n_links))), | ||
| _ => unreachable!() // key can't be anything else |
There was a problem hiding this comment.
This is accurate and will work fine. But every unreachable makes me a bit uncomfortable ... I can't think of a clean way to remove this but I just want to mark it to force myself to think about it.
There was a problem hiding this comment.
I can understand that, but don't have an immediate idea how to change it either.
tuncbkose
left a comment
tuncbkose
left a comment
There was a problem hiding this comment.
Thanks a lot for the comments! I've written some explanations but I won't be able to do a second iteration of this MR until next week probably. Hope that is not too annoying.
I will keep an eye on #156 in case that may make handling links easier as well.
| // check if it contains the current term to see if it needs a re-render | ||
| if rendered.successful && rendered.num_search_found.is_some() { | ||
| if rendered.successful && rendered.num_search_found.is_some() | ||
| && !(page_num == start_point && link_highlight_changed){ |
There was a problem hiding this comment.
My thinking was that if !link_highlight_changed, then there is no reason to re-render it and that link highlighting should be limited to visible pages, thus no need to look at page_num != start_point.
That said I am certain this will need correcting when I fix displaying 2 pages (I'll also have to check if one can display more than 2 pages, I look almost exclusively at 1 page docs).
| // but something like this is needed in case there are two separate links with the same uri | ||
| // across lines | ||
| let is_same = prev_link.uri == link.uri | ||
| && prev_link.dest == link.dest | ||
| && prev_link.bounds.y1 == link.bounds.y0; | ||
| // && prev_link.bounds.x1 == bounds.x1 | ||
| // && link.bounds.x0 == bounds.x0; |
There was a problem hiding this comment.
A mupdf::Link is a rectangle + some destination. So in a document such as
.... li-\n
nk ...
mupdf creates two Link objects, one for each line.
As a user, my preference would be that if I cycle the highlight to this link, the entire link should be highlighted at once, instead of it being separated into two (or more if for some reason it is long enough to span more than 2 lines). Consequently, we need to detect whether two Link objects actually belong to the same link (which I termed link_group above but it can probably have a better name + clearer comments).
To check if they are the same link, then need to point at the same destination and their bounds (let's call them r0 and r1) should be "contiguous". A naive definition of contiguous here would be that r0.x1 is at the line end, r1 is right below r0 (r1.y0 is equal to r0.y1) and r1.x0 is at the line start (and similar if the link spans >2 lines). Here, line start and end are controlled by page margins etc on a quick search, I wasn't sure how one could determine this.
More generally, this naive definition will not work if a link is spanning columns (or pages when 2 pages are displayed, but that may be more acceptable). So I was hoping to look at mupdf more to see if there is a better way to do this. (Maybe it would be easier in #156?)
Also, this is a problem solely for active highlighting, so if covering all links with a clickable rectangle is enough, this problem would go away. But I imagine a keyboard-compatible solution would be preferable.
| scale_factor: f32, | ||
| highlighted_link: Option<usize>, | ||
| ) -> Result<Vec<Link>, mupdf::error::Error> { | ||
| let link_groups = page.links()? |
| } | ||
| } | ||
| }, | ||
| key @ (KeyCode::Tab | KeyCode::BackTab) => { |
There was a problem hiding this comment.
I believe one needs to use KeyCode::BackTab over Tab + checking for Shift modifier and in my quick testing it gave the exact behavior you describe.
| (None, _) => Some(InputAction::HighlightLink(Some(0))), // No link is highlighted yet, highlight the first one | ||
| (Some(l), KeyCode::Tab) => Some(InputAction::HighlightLink(Some((l + 1) % n_links))), | ||
| (Some(l), KeyCode::BackTab) => Some(InputAction::HighlightLink(Some((l + n_links - 1) % n_links))), | ||
| _ => unreachable!() // key can't be anything else |
There was a problem hiding this comment.
I can understand that, but don't have an immediate idea how to change it either.
2 participants
Hello. This is yet another attempt at implementing link support (#81).
I was playing around with this independently (and made a bad implementation) when I realized that @perhurt had a nice looking but slightly outdated PR at #95. This PR is a combination of #95 and me implementing highlighting instead of the links menu. I would like to note that I didn't built on top of that branch directly as to avoid having to rebase but if this PR may get merged, @perhurt should get credits as well. I can try to include them in the git history if we get there.
I am aware of at least 3 issues:
renderer.rs::extract_page_links).I am also not very experienced with Rust so any feedback is welcome.
I will try to look into the issues along with adding mouse support like #135, but if someone else built upon this (or implemented this feature independently) I would be very happy as well.