Skip to content
This repository has been archived by the owner on Sep 22, 2020. It is now read-only.

bookmark jumping in epub messed up in certain case #856

Open
dracodoc opened this issue Jul 23, 2014 · 11 comments
Open

bookmark jumping in epub messed up in certain case #856

dracodoc opened this issue Jul 23, 2014 · 11 comments

Comments

@dracodoc
Copy link

There is a bookmark jumping feature added by my request, which use alt+k/l to jump between bookmarked pages. It has been working great in pdf for a long time for me, but I just found it have a problem in epub reading.

Suppose you have several bookmarks and you are using alt+L to jump from bookmark 1 to 2, then use page down to flip 1 page normally, then use alt+L to jump to bookmark 3, instead it go to bookmark 1. If you tried alt+K after page down, it didn't go to the previous bookmark, instead nothing happens.

I think it is because once user use page down in a series of alt+L, the self.pos is not updated correctly, it probably is nil or zero, so "previous bookmark" Alt+K will have no effect, and "next bookmark" will jump to the first bookmark.

function CREReader:nextBookMarkedPage()
for k,v in ipairs(self.bookmarks) do
if self.pos < self.doc:getPosFromXPointer(v.page) then
return v
end
end
return nil
end

@dracodoc
Copy link
Author

@houqp , do you have time to have a look at this? This should be a simple problem. There is even comment in code mentioned that self.pos not updated in jumping history.

@hwhw
Copy link
Member

hwhw commented Jul 24, 2014

Are you sure that self.pos is not updated correctly? The comment refers to making use of the (current) value of self.pos for jump history writing, later in the code (https://github.com/koreader/kindlepdfviewer/blob/master/crereader.lua#L288) self.pos is updated.

@dracodoc
Copy link
Author

I knew it looks updated in the code, but I'm not sure if that part get executed in the special case -- using alt+K/L to jump bookmarks, then press page down. It sure can explain the behavior after press page down in bookmark jumping.

I could try to debug or track its value later.

@houqp
Copy link
Member

houqp commented Jul 26, 2014

sorry i got sick while travelling back home couple days ago. i will try take a look at it when i get better :(

@dracodoc
Copy link
Author

No problem, you don't have to hurry at all!
Take care :)

@houqp
Copy link
Member

houqp commented Aug 18, 2014

@dracodoc , sorry for the delay. I checked the code a bit and self.pos seems to be updated properly. Could you verify whether this bug exists in page mode as well?

@dracodoc
Copy link
Author

@houqp , I verified page mode, the same bug exists in page mode. I guess very few people use the keyboard version of kpv now, even less people use the bookmark jumping feature in epub. If you don't have much time and the bug is not obvious (I though it should be relatively obvious), this is not a high priority task. I can avoid the bug with some effort because I know now I should not press page down in bookmark jumping :P

@houqp
Copy link
Member

houqp commented Aug 19, 2014

I will have to use my DXG sometime anyway. We either fix this bug or port koreader to DXG. Last time I tried it, I got stuck at the toolchain. Let's see if I can get it right this time ;)

@dracodoc
Copy link
Author

@houqp , if you started to use your DXG, one of the more critical bug is that sometimes kpv crashes after a while into sleep and all the bookmarks and history for current opened book will be lost. Recently I had several crashes, I'm not sure the cause, but at least we can make sure that kpv write the history and bookmark information each time when user put the reader into sleep.

@houqp
Copy link
Member

houqp commented Aug 22, 2014

@dracodoc I cannot think of an easy way to achieve this in the old codebase. But we can do a dirty workaround: save page settings on every page turn.

Here is how to make it work: find the call back function for KEY_PGBCK and KEY_PGFWD key press in unireader.lua(line 2647) and crereader.lua. At the end of that function add following lines:

self:saveLastPageOrPos()
self.settings:flush()

@NiLuJe
Copy link
Member

NiLuJe commented Aug 22, 2014

@houqp: Feel free to ping me if you hit a snag with TC issues ;).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants