BookMap: add indicator for pinned page#14510
Conversation
|
What's a pinned page? A bookmark by another name? |
|
It's new: #14483. |
|
I'm using the latest development release. Some confusions and question...
|
|
One pinned page only, for quick jumping to it. |
|
This PR screenshot confused me. Ok got it👍
How do we completely remove pinned page? |
|
It cannot be removed, just don't jump to it. |
No, that wasn't my intention when I wrote that FR :). By temporary I just meant that it gets overwritten when the next location is set. Question for @hius07 : Would it be possible to save the exact location for fixed-layout docs? Say my pinned area of interest is some footnote on a PDF that I read in Reflow mode. So that when I call my pinned page, it jumps directly to the bottom of the page. (Same as when using 'Go to previous location' etc.) |
|
I understand it could cause problems if part of the following page is visible at the bottom of the screen -- then I guess it would save the next page's location, which could cause confusion when the pinned page is called with different view settings. (Edit: But this happens anyway, even with full-page bookmarks, which also get set for the page on the bottom edge of the screen.) |
I think yes, as we do in the "Back" action, with the same restrictions about changed view mode. |
|
Do you mean these overlaps? They don't change in this PR. Unfortunately, there isn't enough room to avoid them. 
|
It's unclear if it can be sensibly avoided, but more important I don't believe it's new so it doesn't seem to be related to this PR. |
|
Not ready yet, the pinned page indicator should be added to the Page browser as well. |
|
No problem with me with the overlap, that's what we get with a fixed height for those icons.
I think we should have a way to remove it. No idea yet. |
I didn't do that to avoid overlapping with the pen indicator at least. |
I see. But it feels we would see the pin miscentering more often than a pin + a pencil :) |
| return x | ||
| end | ||
|
|
||
| function BookMapRow:getIndicatorXY(page, shift_y) |
There was a problem hiding this comment.
Fine with factorizing this.
shift_y (which reads like it accepts a number by which it will be shifted) should be named shift_down if a boolean is to be provided.
|
Center pin Show pin in Page browser |
| -- Shift it a bit down to keep bookmark glyph(s) readable | ||
| y = y + math.floor(self.span_height * (1/3)) | ||
| end | ||
| local x, y = self:getIndicatorXY(page, self.bookmarked_pages[page] or page == self.pinned_page) |
There was a problem hiding this comment.
The condition gets cumulative (for previously added other indicators).
Is it ok that it stops with these 2 first ones ? (the next ones: extra, cur_page, won't be shifted if there are any previous locations).
If not (I really don't know if I stopped for some reason, of just forgot to do it when adding more indicators), may be we should in each branch set a should_shift_down=true and use that ?
There was a problem hiding this comment.
I don't understand.
Annotations are not shifted.
The pin is shifted iff there are annotations.
Other indicators are shifted if there are annotations or the pin.
There was a problem hiding this comment.
Other indicators are shifted if there are annotations or the pin.
Ok, but cur_page is not shifted down if there are "previous locations" (circled numbers (1) (2)) or some "extra" symbols.
I'm just asking, may be this is just right, I don't remember :)
There was a problem hiding this comment.
"previous location" indicator is not shown in the current page slot.
There was a problem hiding this comment.
Ok then, so it's ok and already taken care of by my old self :)
This change is