Missing self:redrawCurrentPage() again...

[tigran123]

Before I start inserting some more calls to self:redrawCurrentPage() again let me explain the bug:

1. Open a DjVu or PDF file with no jump history (delete history/* files to make sure)
2. Press Back. You will briefly see "Retrieving TOC..." message overlayed by "Already first jump!" message.
3. After "Already first jump!" disappears you will see the "Retrieving TOC..." message which is restored from underneath it. You have to refresh the page manually (e.g. Menu or Shift-R) to make it go away.

This is because the call to self:redrawCurrentPage() has been removed today from UniReader:addJump(). In crereader it is not observable because there the Back command does NOT call :addJump() method, so no "Retrieving TOC..." message is displayed. But for PDF and DjVu it is called (addJump() -> getTocTitleByPage() -> fillToc() code path, ignoring the underscored one) so we end up with "Retriving TOC..." remaining on the screen until next refresh.

So, what should we do about it? The naive solution is just to add a call to unireader:redrawCurrentPage() right after the display of "Already first jump!" message in unireader.lua. It works, I tested it, but is this the right solution? Hmmm, I can't see anything wrong with it, but maybe I am missing something, so I am soliciting your opinions.

[tigran123]

The big problem with my suggestion above is that it will cause an unnecessary redraw on every press of Back whereas we only need it under two conditions:

a) TOC has not yet been retrieved by some other means b) we are at the top of the history list

So, I'll improve my fix to be more specific and optimal.

[tigran123]

Actually, before I start optimizing the suggested fix I wish to understand why Back function must differ between PDF/DjVu and crereader implementations, i.e. why do we need this code in unireader but not in crereader:

            if unireader.jump_history.cur > #unireader.jump_history then              
                -- if cur points to head, put current page in history                 
                unireader:addJump(self.pageno)                                        
                prev_jump_no = unireader.jump_history.cur - 2                         
            else                                                                      
                prev_jump_no = unireader.jump_history.cur - 1                         
            end                                                                       

If this code did not exist then the problem would disappear as there would be no call to addJump() method.

[tigran123]

Ah, I see the reason now. The functionality of unireader and crereader jump history is different: crereader's is more deficient, i.e. one last jump is actually lost, try this:

1. open a fresh fb2 book (no history file), obviously you end up at the beginning
2. now jump to 10% (G and enter 10)
3. now jump to 20% (G and enter 20)
4. now keep going Back to the beginning.
5. press Shift-B to observe the jump history: the 20% location is lost, i.e. attempt to Shift-Back twice will give you "Already at last position". But for PDF/DjVu files it is not lost, i.e. pressing Shift-Back will get you forward first to 10% and then to 20% (well, not in percent for PDF/DjVu it would be page numbers, obviously)

So, what shall we do? Fix the crereader's jumps and introduce a bug with refresh which we will then fix by adding appropriate self:redrawCurrentPage() under the two conditions listed in my comment above?

Or change unireader's jump functionality to match crereader's one and cause the refresh bug to disappear?

I think we should deal with this properly, otherwise we'll keep fixing one side of the app (PDF/DjVu) and introducing bugs in the other (crereader)...

[tigran123]

Closing, the fix is in the pull request #325

[houqp]

Thanks for the detailed analysis :)

Just some more comments:

• Will it be better if we move InfoMessage:show("Retrieving TOC...", 1) call from UniReader:fillToc() to UniReader:showToc() method? Because I think fillToc() is looks more like a data processing function, so it's better not use it to do GUI update.
• I see no reason why key event KEY_BACK should be handled differently in crereader and unireader. crereader should also save the last jump as unireader does. Can anyone double check this?

[tigran123]

@houqp I thought about the first thing you mention, but decided against it --- the whole point of the "Retrieving TOC..." message is to not give the user impression that KPV hang while it retrieves TOC from those files which have a massive number of entries in it, such as my morphologically-tagged editions of the Bible where we need a TOC entry for each individual verse (not chapter, as usual) because each page only contains one or two verses. On such files fillToc() takes too long (really it is the self:getToc() that takes too long, the rest of the stuff in fillToc() is almost instant). So, every caller of fillToc() should be warning the user that the program did not hang --- it is busy retrieving TOC. This is the reason for not shifting the message to showToc().

As for the second thing, yes, I thought about it (as I mentioned in my comment) but the change would imply messing around with page<->xpointer translation and I didn't risk it. Let those more familiar with crereader code deal with it. At the moment I feel more comfortable only with code that deals with PDF and DjVu (because I test it thoroughly, unlike the crereader-specific code --- when one doesn't use a program himself he doesn't know what/where to test).

[ghost]

the whole point of the "Retrieving TOC..." message is to not give the user impression that KPV hang while it retrieves TOC from those files which have a massive number of entries in it

I've basically finished the tools to separate messages on importance level -- so user may decide by himself how the reader should inform him about various events. This evening I'll probably submit the proof of concept and test files (say, crereader.lua or any other that is ATM not under heavy development) to illustrate how it works.

It should help us not to pay too much attention to unimportant points such as desicion to show or not to show messages like "Retrieving TOC...", "Scanning folder", etc

[tigran123]

@NuPogodi The decision where to show the message "Retrieving TOC..." does not depend on the user's ability to change the overall verbosity level. It is a completely separate issue. On each verbosity level that the user will be able to set we will still have to make such decisions and do this carefully, analyzing every possible situation and asking ourselves "why do we need to do it?", as @houqp just illustrated by his comments.

[ghost]

The decision where to show the message "Retrieving TOC..." does not depend on the user's ability to change the overall verbosity level. It is a completely separate issue.

Our task will be restricted by simple estimation of the message importance. The rest (namely, how the events with a certain importance will be informed about -- i) ignoring, ii) showing the popup window with the message text, iii) TTS-voice message or (iv) popup & TTS -- the user will adjust for his own taste.

Concerning the importance... ATM, I've made 3 levels -- a) auxiliary messages, b) important messages & c) errors. That's all. I think that "Scanning folder...", "Retrieving TOC..." or similar messages to show user that viewer does not hang belong to the class a). The messages that explain to user why something does not happen or happens in a wrong way -- say, "Already last jump" or "Document has no TOC" -- fit to class b). The typical errors are "Error unpacking zip", etc.

Further discussion (if any) is more reasonable to continue in the corresponding issue I'm going to open this evening.

[tigran123]

Yes, your design sounds very good and well thought through. But this change will involve changing a lot of code everywhere and (what is more important) deciding where the page redraw is needed and where it is not (i.e. it will then depend on the verbosity level, so this is not a trivial matter at all). Therefore, I suggest that this change is merged only after the release, i.e. into the next development tree, not the current "almost release" one. Let us now focus on fixing bugs only, so that the release is as stable as possible and immediately after the release start experimenting with new cool features like your proposal of verbosity levels (plus, maybe russification of the messages and help screens which was also demanded).

[tigran123]

Actually, concerning the redraw issue, if we assume the worst case (i.e. most verbose level) then there is no issue and so there is no reason why your proposal can't be merged now. And, at some later (after-release) point, we can consider optimizing page redraws depending on the verbosity level, i.e. to not redraw the page if the potential message was not actually displayed.

[ghost]

To say the truth, I made these functions available in the only mode introduced for developers & beta-testers as some of them -- say, voice-related tasks -- may properly work on K3 (tested) and not work on other Kindle devices and emulator (or even crash the reader). For evident reasons I could not test it by myself. So, I'd like to have some feedback from lucky K2- & KDX-owners or, even better, no feedback at all -- if everything runs smoothly.

I have also absolutely no problems to wait with commiting these feature until the next official build is released. I have some other features to develop (not to say about using Kindle as it was originally suggested by Amazon - for reading - not so bad idea)

[tigran123]

Ok, wonderful! In that case, here is how I suggest that you proceed (i.e. to make things convenient both for you and others who will wish to test your changes --- e.g. I will gladly do that, I am very curious how you implemented TTS stuff). Do the same thing that @dpavlin does, namely create a separate branch in your github tree (I called it "msg-levels" below but you choose whatever name you feel appropriate):

$ git checkout -b msg-levels

Then work in that branch and commit it and also push it back to github, so we can see it as well. This way we can set up your tree as remote for tracking and set up our own local clone of it:

$ git remote add nupogodi https://github.com/NuPogodi/kindlepdfviewer.git
$ git checkout -b nupogodi-msg-levels
$ git fetch nupogodi
$ git merge nupogodi/msg-levels

Carefully note the spaces and slashes and hyphens in the above.

What I am saying is that instead of sending a pull request (which implies that the change, as far as you are concerned, is 100% ready and should be merged into the main master NOW), work in a separate branch and we can track it and test your changes and provide the feedback. Btw, I do have a DXG device so I will gladly test it on DXG for you. Ok?

Btw, by "push back to github" I meant pushing back to your origin tree, not to upstream, namely:

$ git push origin

For the first push you may have to say explicitly "push msg-levels" instead of just "push" so that the branch "msg-levels" is created remotely (but for you it is local, not remote) on github.

[tigran123]

Ah, I just saw in your github tree "nupogodi-inputbox" branch which means you already know all of the above. Ok, great --- maybe someone else (who is new to git) who reads these posts will benefit then :)