ReaderLink: allow a forward location stack by yparitcher · Pull Request #10228 · koreader/koreader

yparitcher · 2023-03-20T04:45:34Z

this allows users to jump back and forth to a link

~~When we go back (and are at the end of the stack) if the current page is not the same as the last location add it to the stack. this allows going (back)foward if one by mistake did the back gesture~~

this allows going back and forth from links (think of undo / redo)

when going back and no forward locations and when we are not on the same page as the last saved location, add the current location to the forward stack, helping if one goes back by mistake they can jump back to thier current location

This change is

poire-z · 2023-03-20T08:34:52Z

+        return true
+    elseif show_notification_if_empty then
+        UIManager:show(Notification:new{
+            text = _("Location history is empty."),


"Location forward history is empty." or "No forward location in history." ?

poire-z · 2023-03-20T08:41:45Z

+    self.location_stack_index = self.location_stack_index + 1
+    self.location_stack[self.location_stack_index] = location
+    self.location_stack[self.location_stack_index+1] = nil


Are we supposed somehow somewhere to navigate again to older forward locations, when you got back and follow a new path ?
It's a bit unclear this gymnastic with +1 = nil or ~= nil.

poire-z · 2023-03-20T08:47:39Z

    previous_location = {category="none", event="GoBackLink", arg=true, title=_("Back to previous location"), reader=true},
+    next_location = {category="none", event="GoForwardLink", arg=true, title=_("Forward to net location"), reader=true},


(I guess the naming is for consistency with the existing one, but ok, we go back usually from a link - we're not really going forward from/to a link - or there is another event, follow nearest link.)

poire-z · 2023-03-20T08:48:42Z

        "----------------------------",
        "go_to_previous_location",
+        "go_to_next_location",
    },


More chances to make this menu go above 10 items and become multiple pages :/

poire-z · 2023-03-20T08:54:04Z

It looks like not much, but it was quite hard to review :/ and I'm a bit unsure about how it works.
Can you confirm that, following links and going back, as soon as you follow a link or do anything that addLocationToStack(), all the previous forward locations is unusable - and you won't be able to "go forward" and be back on a totally unrelated/obsolete "forward location chain" ?

I remember there's been requests for that, but it feels totally unpractical to me: as soon as you tap a link/bookmark/ToC/page thumbnail/skim..., you addCurrentLocationToStack and your dear forward history is lost (as it should be).
It might be ok to back/forward from a footnote, but bookmarking that footnote page and using "jump to latest bookmark" feels more robust to me (even if less obvious).

What would be your use cases ? And which gestures would you use for it ?

yparitcher · 2023-03-20T13:00:41Z

Sorry i thought i wrote the explanation comment. Github must have swallowed it.

We are using the table as an array so as long as the next item is nil the array is finished and we dont care about further values. If it is too coplicated for us, i can switch to using ipairs to clear all further values.

When i am at my computer i will fix/answer all the individual points

I don't use links so much, but sometimes i trigger the previous location gesture by mistake and can't get back to my current location.

Frenzie · 2023-03-20T15:23:50Z

Forward location stack was actually on my mental list from the moment I implemented the thing, so I may have mentioned it somewhere without opening an issue. I pretty much forgot about it though and it actually hasn't really bothered me in practice over the past five years.

NiLuJe · 2023-03-20T15:36:46Z

I don't recall if there's a matching GH issue, but it came up very recently on MobileRead ;).

yparitcher · 2023-03-20T16:16:31Z

https://www.mobileread.com/forums/showthread.php?t=352623

yparitcher · 2023-03-23T15:08:02Z

I looked up undo/redo queues, and found a much simpler idea with 2 stacks, we leave the existing stack alone and add a forward stack, when we pop from one (by going back) we insert it onto the other.

Will redo this when i have time.

Thanks @poire-z for reminding me i was overengineering this :)

yparitcher · 2023-03-23T22:53:58Z

ready for review. I mostly copied existing strings. Please suggest better english ;)

poire-z · 2023-03-23T23:25:00Z

+    local saved_location = table.remove(self.forward_location_stack)
+    if saved_location then
+        table.insert(self.location_stack, saved_location)
+        logger.dbg("GoForward: restoring:", saved_location)
+        self.ui:handleEvent(Event:new('RestoreBookLocation', saved_location))


saved_location is the future forward location we are going to.
So, table.insert(self.location_stack, saved_location) seems wrong ? You should insert the current location we are leaving.

I will start with going back (your other comment)
For example we have 3 previous locations on the stack, no forward locations, we pop one. If the current and popped are not the same we add current to forward stack. The popped location is added to the forward stack and we go to it. So now it is forward of us.

When we go forward we pop the last forward location off the stack, push it to previous location stack, and go to it.

we add current to forward stack. The popped location is added to the forward stack and we go to it. So now it is forward of us.

Still confused, sorry :/
When going back, we'll add 2 items to the foward_stack: the footnote page we were in, + the reading page we are going back to ? So the forward stack top elment is the current page ?

When we go forward we pop the last forward location off the stack, push it to previous location stack, and go to it.

You're saying what I said: the previous location stack will be where we are going?
So, we don't add anywhere the current location ?!

Can you give some example with PDF pages (to keep it simpler than xpointer), the state of both stacks, when say:

I am on page 4, I follow a footnote link to page 12

then follow another link to page 17

then "Go back" and end up on page 12

then "Go back" and end up on page 4

then Go forward" and end up on page 12

then "Go back" and end up on page 4

location_stack = {}, forward_location_stack = {}
I am on page 4, I follow a footnote link to page 12
location_stack = { 4 }, forward_location_stack = {}
then follow another link to page 17
location_stack = { 4, 12 }, forward_location_stack = {}
then "Go back" and end up on page 12
location_stack = { 4 }, forward_location_stack = { 17, 12 }
then "Go back" and end up on page 4
location_stack = {}, forward_location_stack = { 17, 12, 4 }
then Go forward" and end up on page 12 4
location_stack = { 4 }, forward_location_stack = { 17, 12 }
then Go forward" and end up on page 12
location_stack = { 4, 12 }, forward_location_stack = { 17 }
then "Go back" and end up on page 4 12
location_stack = { 4 }, forward_location_stack = { 17, 12 }
then "Go back" and end up on page 4
location_stack = {}, forward_location_stack = { 17, 12, 4 }

Since we are always between locations (we store results, not actions) when you switch from forward<-->back, you repeat the first one going the other way. however if you move pages in the middle it make more sense:

location_stack = {}, forward_location_stack = {}
I am on page 4, I follow a footnote link to page 12
location_stack = { 4 }, forward_location_stack = {}
then follow another link to page 17
location_stack = { 4, 12 }, forward_location_stack = {}
then "Go back" and end up on page 12
location_stack = { 4 }, forward_location_stack = { 17, 12 }
then "Go back" and end up on page 4
location_stack = {}, forward_location_stack = { 17, 12, 4 }
then Go forward" and end up on page 12 4
location_stack = { 4 }, forward_location_stack = { 17, 12 }
then Go forward" and end up on page 12
location_stack = { 4, 12 }, forward_location_stack = { 17 }
read to the next page (13) to finish the footnote.
then "Go back" and end up on page 4 12
location_stack = { 4 }, forward_location_stack = { 17, 12 }
then "Go back" and end up on page 4
location_stack = {}, forward_location_stack = { 17, 12, 4 }

If you want we can check if the end of the stack == current_page and #location_stack >= 2 to jump 2 locations, solving the issue in the first case.

I think I understand now your idea behind the obscure "If we are not on the same page as the current item, then add our current location to the forward stack", it's for when we turn pages after following links.
So, I guess that if I am on page 4, then jump to page 12, then turn page to page 13, when "go back to previous location", I'll then be first on page 12, and then on page 4 - and "go forward" will lead me to (x) page 12 then 13, right ?
(with (x) being an intermediate "to page 4" with no visible effect, but this is another issue which is not the one I'm focusing here :)

In my mind, page 12 shouldn't be tracked:
if I am on page 4, then jump to page 12, then turn page to page 13, only 13 should be the forward location. So, going back and getting on page 4, only page 13 should be in the forward location - and when I "go forward", I reach page 13 directly.
So, that's my naive expectation of the behaviour :) and your more complex, may be handling other use cases with more granularity about the locations remembered, feels a bit confusing - with either "no page change" or unwished intermediate page when I hit "go forward" (and even "go back"?)

Did I understand you correctly, and do you get me?

If nobody else has remarks about your behaviour and we go with it, it would be good to have more comments in the code about this behaviour, or at least its tricky parts, with the kind of samples "if I go..." and the stacks states we used above, and "this allows... such use case".

What are we supposed to remark upon precisely? I've neither looked at nor tried the code yet. ^_^

So, ask again after you've done that :)
(I won't put noise/directions into your genuine/naive appreciation of it all - but later, you can read the scenarios described above ("I am on page 4...").

(I won't put noise/directions into your genuine/naive appreciation of it all - but later, you can read the scenarios described above ("I am on page 4...").

Haven't read them yet, but forward and backward don't seem to work I'd expect them to. They're… double?

I am going to add a commit to address the go to same page when switching between back<-->forward.

poire-z · 2023-03-23T23:26:13Z

+            end
+        end
+
+        table.insert(self.forward_location_stack, saved_location)


this allows going back and forth from links (think of undo / redo) when going back and no forward locations and when we are not on the same page as the last saved location, add the current location to the forward stack, helping if one goes back by mistake they can jump back to thier current location

poire-z

OK with the code and the unchanged "go back" behaviour.
(Not interested enough with the "go forward" behaviour to get back into focusing on it :)

mergen3107 · 2023-03-30T11:40:01Z

I tested this quickly in a PDF with internal links. Doesn’t work after first forward location move.

Open a PDF with internal links (Figures, Tables, Sections, references, etc). Top menu first tab has both back and forth buttons grayed out.
Tap a link to go somewhere.
Top menu, now only has go back to prev location ready.
Tap go back
Top menu now has only forward button ready.
Tap go forward.
It goes there OK, top menu has only back button ready.
Tap back - nothing happens in terms of pages.
Back grays out, forward lights up, but it obviously won’t change locations anymore.

Frenzie · 2023-03-30T16:47:42Z

+        callback = function() self:onGoForwardLink() end,
+        hold_callback = function(touchmenu_instance)
+            UIManager:show(ConfirmBox:new{
+                text = _("Clear Forward location history?"),


Missed this, but weird capitalization. But the better solution is to just remove this completely imho. What's the purpose?

Pardon, overlooked the hold_callback part, so it's hidden.

Go Forward to next location has the same problem

yparitcher · 2023-03-30T21:42:35Z

I tested this quickly in a PDF with internal links. Doesn’t work after first forward location move.

1. Open a PDF with internal links (Figures, Tables, Sections, references, etc). Top menu first tab has both back and forth buttons grayed out.

2. Tap a link to go somewhere.

3. Top menu, now only has go back to prev location ready.

4. Tap go back

5. Top menu now has only forward button ready.

6. Tap go forward.

7. It goes there OK, top menu has only back button ready.

8. Tap back - nothing happens in terms of pages.

9. Back grays out, forward lights up, but it obviously won’t change locations anymore.

Works for me, are the link & location on the same page?

mergen3107 · 2023-03-30T22:33:36Z

No, the link is a Figure, about 2-3 pages away

@hius07

KOReader 2023.04 "Solar Panel" ![koreader-2023-04-fs8](https://user-images.githubusercontent.com/202757/234975122-d7739c71-74aa-4cb0-a086-72a4bba70f52.png) It's been another busy month squashing many bugs. Our Mac users will be happy to hear that I told macOS we've supported HiDPI since long before anyone came up with such terminology (koreader#10341), and that the program can now natively build on M1 devices (koreader#10291). Solar panel credit: <https://openclipart.org/detail/294030/solar-energy> by gnokii We'd like to thank all contributors for their efforts. Some highlights since the previous release include: * Readerzooming: fix use of default settings (koreader#10205) @hius07 * ButtonDialog/ButtonDialogTitle: consistent 'width' handling (koreader#10230) @poire-z * MovableContainer: add support for anchoring initial position (koreader#10230) @poire-z * Book map, Page browser: add top left menu (koreader#10230) @poire-z * Book style tweak: add button with CSS suggestions (koreader#10230) @poire-z * crengine: fix parsing of multibytes encodings (koreader#10230) @poire-z * readerstyletweak: update profiles on unregistering in dispatcher (koreader#10247) @hius07 * Filesearcher: add search in book metadata (koreader#10198) @hius07 * Fix: Updated legacy directory, which crashed the program (koreader#10260) @Mochitto * ReaderLink: allow a forward location stack (koreader#10228) @yparitcher * BookInfo: add page information (koreader#10255) @hius07 * Center pdf manual zoom mode (koreader#10246) @nairyosangha * File browser: add Folder Menu (koreader#10275) @hius07 * Calendar view: add options to change start time of days (koreader#10254) @weijiuqiao * filechooser: fix crash on "unreadable content" (koreader#10283) @hius07 * Sync book statistics: add to dispatcher (koreader#10285) @ptrm * [plugin] Exporter: use util.getSafeFilename() to remove illegal characters from output filename (koreader#10282) @Mochitto * readerbookmark: fix writing pdf annotation (koreader#10287) @hius07 * Folder Menu: sign for Home folder (koreader#10288) @hius07 * ListMenu: show mark for books with highlights (koreader#10276) @hius07 * Calendar view's day view: thicker separator at 00:00 (koreader#10289) @poire-z * PageBrowser: tweak scrolling behaviour at book start/end (koreader#10289) @poire-z * Make `kodev check` feature complete (koreader#8682) @yparitcher * macOS: support for M1 building (koreader#10291) @ptrm * Reader: do not apply font size and spacing out of range (koreader#10295, koreader#10307) @hius07 * File browser: show Folder Menu on long-press on Home icon (koreader#10298) @hius07 * SSH.koplugin: fix cant stop SSH server bug when pid file's stale (koreader#10300) @weijiuqiao * PM: Optimize task queue handling around standby (koreader#10203) @zwim * statistic.koplugin: fix today's timeline showing next day when within custom offset (koreader#10299) @weijiuqiao * ReaderThumbnails: update cached page thumbnail on bookmark note change (koreader#10303) @hius07 * SDL: add multitouch support (koreader#10334) @Frenzie * SDL: add HiDPI support (koreader#10341) @Frenzie * BookInfo: fix crash on show cover (koreader#10315) @hius07 * Deal with table.pack corner-cases properly (koreader#10350) @NiLuJe * Android: add Tagus Gea support (<koreader/android-luajit-launcher#412>) @Alfedi [Full changelog](koreader/koreader@v2023.03...v2023.04) — [closed milestone issues](https://github.com/koreader/koreader/milestone/63?closed=1) --- Installation instructions: [Android](https://github.com/koreader/koreader/wiki/Installation-on-Android-devices) • [Cervantes](https://github.com/koreader/koreader/wiki/Installation-on-BQ-devices) • [ChromeOS](https://github.com/koreader/koreader/wiki/Installation-on-Chromebook-devices) • [Kindle](https://github.com/koreader/koreader/wiki/Installation-on-Kindle-devices) • [Kobo](https://github.com/koreader/koreader/wiki/Installation-on-Kobo-devices) • [PocketBook](https://github.com/koreader/koreader/wiki/Installation-on-PocketBook-devices) • [ReMarkable](https://github.com/koreader/koreader/wiki/Installation-on-ReMarkable) • [Desktop Linux](https://github.com/koreader/koreader/wiki/Installation-on-desktop-linux) • [MacOS](https://github.com/koreader/koreader/wiki/Installation-on-MacOS)

this allows going back and forth from links (think of undo / redo) when going back and no forward locations and when we are not on the same page as the last saved location, add the current location to the forward stack, helping if one goes back by mistake they can jump back to their current location when going back and no forward locations and when we are not on the same page as the last saved location, add the current location to the forward stack, helping if one goes back by mistake they can jump back to thier current location

Frenzie added this to the 2023.04 milestone Mar 20, 2023

poire-z reviewed Mar 20, 2023

View reviewed changes

yparitcher marked this pull request as draft March 20, 2023 16:16

yparitcher changed the title ~~ReaderLink: allow a foward location stack~~ ReaderLink: allow a forward location stack Mar 21, 2023

poire-z reviewed Mar 21, 2023

View reviewed changes

Comment thread frontend/apps/reader/modules/readerlink.lua Outdated

poire-z reviewed Mar 21, 2023

View reviewed changes

Comment thread frontend/apps/reader/modules/readerlink.lua Outdated

yparitcher force-pushed the fls branch from 89ab5d8 to 3b0332d Compare March 23, 2023 22:51

yparitcher marked this pull request as ready for review March 23, 2023 22:54

poire-z reviewed Mar 23, 2023

View reviewed changes

poire-z reviewed Mar 26, 2023

View reviewed changes

Comment thread frontend/apps/reader/modules/readerlink.lua Outdated

yparitcher force-pushed the fls branch from f8e10d3 to fde4510 Compare March 28, 2023 02:32

poire-z reviewed Mar 28, 2023

View reviewed changes

Comment thread frontend/apps/reader/modules/readerlink.lua Outdated

review

15ada56

poire-z approved these changes Mar 29, 2023

View reviewed changes

Frenzie merged commit bc5d354 into koreader:master Mar 29, 2023

yparitcher deleted the fls branch March 29, 2023 20:50

Frenzie reviewed Mar 30, 2023

View reviewed changes

poire-z mentioned this pull request Mar 17, 2024

FR: Full text search: Result info notifications; Allow page turns #11203

Closed

yparitcher mentioned this pull request Apr 17, 2025

Fix Location Stack Issues #13607

Merged

poire-z mentioned this pull request Apr 23, 2025

ReaderLink: Back/Forward Location stack logic #13666

Open

		previous_location = {category="none", event="GoBackLink", arg=true, title=_("Back to previous location"), reader=true},
		next_location = {category="none", event="GoForwardLink", arg=true, title=_("Forward to net location"), reader=true},

Uh oh!

Uh oh!

Conversation

yparitcher commented Mar 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

poire-z commented Mar 20, 2023

Uh oh!

yparitcher commented Mar 20, 2023

Uh oh!

Frenzie commented Mar 20, 2023

Uh oh!

NiLuJe commented Mar 20, 2023

Uh oh!

yparitcher commented Mar 20, 2023

Uh oh!

Uh oh!

Uh oh!

yparitcher commented Mar 23, 2023

Uh oh!

yparitcher commented Mar 23, 2023

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!

Uh oh!

Uh oh!

poire-z left a comment

Choose a reason for hiding this comment

Uh oh!

mergen3107 commented Mar 30, 2023

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!

yparitcher commented Mar 30, 2023

Uh oh!

mergen3107 commented Mar 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

yparitcher commented Mar 20, 2023 •

edited

Loading