Support for hiding (skipping) non-linear EPUB fragments #6847

Jellby · 2020-11-04T16:57:52Z

This is the frontend implementation of #6809, to be merged after koreader/koreader-base#1228 (see koreader/crengine#391 too)

The core of the implementation is pretty basic: just create Document:getPrevPage and Document:getNextPage, which return the prev/next page number. By default they are page-1 and page+1, but if a document contains non-linear flows, they can be skipped. If already inside a non-linear flow, paging works normally, until the boundary is reached, where it continues on the main (linear) flow, skipping other non-linear flows that may be in-between. The non-linear pages are still accessible through links, goto, skim, bookmarks... it's only page back/forward that are affected (if enabled).

Some conventions (that can be discussed and changed):

In the footer, pages on the linear flow as indicated as x // y (double slash), and x and y refer only to the linear pages (non-linear pages are not counted here). The intent is to succinctly indicate that there are uncounted pages.
When inside a non-linear flow, the footer shows [x / y]z, where x and y refer only to this particular non-linear flow, and z is the number of the flow.
Pages/time to end of chapter/book are counted as if paging through.
In toc and bookmarks, page numbers refer to the linear flow. Pages in non-linear flows are indicated as [x]y where x is the "local" page number and "y" is the number of the flow.
In goto, a plain number leads to the "global" number. The [x]y format is supported, in which case y=0 corresponds to the linear flow.
In the skim bar, non-linear pages are marked with a darker background.
... and maybe something else I'm forgetting.

Setting help

Footer, linear

Footer, non-linear

Go to window

Skim window

Bookmarks

Closes #6809

This change is

Frenzie · 2020-11-04T17:11:26Z

frontend/apps/reader/modules/readerbookmark.lua

+                local flow = self.ui.document:getPageFlow(page)
+                page = self.ui.document:getPageNumberInFlow(page)
+                if flow > 0 then
+                    page = T("[%1]%2", page, flow)


Might need a translator's comment. Also screenshots would be nice. ;-)

I confess I've used T(...) without knowing what it means... I don't think this should be translated though, so I guess I should use ("[%1]%2"):format(page, flow), as in the footer code?

Actually you confused me by needlessly using the template function here when you're just doing string.format.

I don't think this should be translated though

You and I simply don't know (pinging @WaseemAlkurdi since I'm curious about Arabic), but even for a language like Dutch I might opt to localize it as () instead of [].

I copied that from the toc code, which is "translated", but the footer code is not (e.g. you cannot translate "%d / %d" to "%d : %d")

That would be a bug then.

@Frenzie We would flip it normally for RTL, like a page number:

[2 / 14] 3

(page 2 out of 14 in non-linear flow number 3)
which would become:

3 [14 / 2]

That's the self-evident part. But you'd use those symbols? :-)

@Frenzie Ah. The circular brackets would be better, as you stated. Square brackets have no place in user-facing stuff (imagine them being used in a literary text, a novel of some sort?)

NiLuJe · 2020-11-04T17:19:35Z

frontend/apps/reader/modules/readertoc.lua

 function ReaderToc:getChapterPagesLeft(pageno)
-    --if self:isChapterEnd(pageno) then return 0 end
-    local next_chapter = self:getNextChapter(pageno)
-    if next_chapter then
-        next_chapter = next_chapter - pageno - 1
+    local flow = self.ui.document:getPageFlow(pageno)
+    -- Count pages until new chapter
+    local pages_left = 0
+    local test_page = self.ui.document:getNextPage(pageno)
+    while test_page > 0 do
+        pages_left = pages_left + 1
+        if self:isChapterStart(test_page) then
+            return pages_left
+        end
+        test_page = self.ui.document:getNextPage(test_page)
    end
-    return next_chapter
 end

 function ReaderToc:getChapterPagesDone(pageno)
    if self:isChapterStart(pageno) then return 0 end
-    local previous_chapter = self:getPreviousChapter(pageno)
-    if previous_chapter then
-        previous_chapter = pageno - previous_chapter
+    local flow = self.ui.document:getPageFlow(pageno)
+    -- Count pages until chapter start
+    local pages_done = 0
+    local test_page = self.ui.document:getPrevPage(pageno)
+    while test_page > 0 do
+        pages_done = pages_done + 1
+        if self:isChapterStart(test_page) then
+            return pages_done
+        end
+        test_page = self.ui.document:getPrevPage(test_page)
    end
-    return previous_chapter
 end


Is there something wrong with the existing approach if !hasFlows? If not, can we keep it in this case? ;).

Nothing wrong, I just like it better to have a single solution than lots of ifs.

getPreviousChapter & co happen to be faster by using the ticks directly, though, instead of walking page-by-page ;)

frontend/apps/reader/modules/readerrolling.lua

NiLuJe · 2020-11-04T17:23:44Z

frontend/apps/reader/modules/readerfooter.lua

+            for i,j in pairs(self.ui.toc:getTocTicksFlattened()) do
+                if self.view.document:getPageFlow(j) == self.flow then
+                    self.progress_bar.ticks[i] = self.view.document:getPageNumberInFlow(j)
+                else
+                    self.progress_bar.ticks[i] = nil
+                end
+            end


In the same vein, only relevant if hasFlows (well, has and show, I guess, actually) ;).

You should be able to iterate through getTocTicksFlattened() via ipairs. If it turns out the array is actually sparse, then we have a host of other buggy code that assumes it wouldn't be ;).

EDIT: But given the way it's built, it really never should be sparse, so that assumption should hold.

poire-z · 2020-11-04T18:45:56Z

Mhhhh, first impression is not the best.
It feels too integrated: what I knew and could read and understand easily - is now something that looks really different, with the "flow" word everywhere (even if there are places you succeeded in hiding in methods of document - but then the feeling is just deported to credocument :) I'm jumping all around to see what the regular calls do and the new calls do - to in the end happily discovering it's a no-op - but there's too many indirections until I notice that.

I would have expected stuff to be a bit more like:

if self.ui.pagemap and self.ui.pagemap:wantsPageLabels() then
    page = self.ui.pagemap:getXPointerPageLabel(page, true)
else if self.ui.hide_non_linear == true and document.hasNonLinearStuff() then
    -- The new stuff I can skip when discovering the code.
    your new code and new methods called only when necessary
else
    -- the regular usually simpler stuff
    page = self.ui.document:getPageFromXPointer(page)
end

I'll get into your code :) and may be all that tight integration is needed (the diff is not that large) - but after 45mn reading the diff, I'm not yet passed this first impression :/

Letting my dev impressions on the side for now, my user impressions (haven't tried it, just from the screenshots and what I quickly got from the code):

I find the the Skim widget is ugly with it's half grey bars
not fond of x // y and the other stuff - but I haven't yet thought about alternatives
but I and other users should not see the complicated Goto description and all the new stuff when hide_non_linear is not true : everything should look as before when a user has not opt-in for the non-linear new behaviour
(issue with bookmarks which could have some the normal page, some the tweaked non-linear page number, depending on whether they were made in one mode or the other - but that's a more general issue with our messy Bookmark stuff)
I think the new menu should go into the first top menu (with TOC, Bookmarks, Reference pages) : and should not be shown when the document does not have any non-linear stuff. It should appear there only when there are (like Reference pages do)

NiLuJe · 2020-11-04T18:58:40Z

* I find the the Skim widget is ugly with it's half grey bars

Kinda agree ;p.

Ideas:

Make it a background color (i.e., ticks go above it), only slightly darker than the "usual" empty white.
If something non-linear was read, make the fill color slightly lighter (àla additive blend, except just fake it, say, white is 0xFF, your new bg color is 0xDD, the diff makes that 0x22, so make the fill whatever it's currently is - 0x22)).

Jellby · 2020-11-04T19:13:34Z

but I and other users should not see the complicated Goto description and all the new stuff when hide_non_linear is not true : everything should look as before when a user has not opt-in for the non-linear new behaviour

That's the case. Everything (but the new menu item) should look the same as it does now when disabled.

(issue with bookmarks which could have some the normal page, some the tweaked non-linear page number, depending on whether they were made in one mode or the other - but that's a more general issue with our messy Bookmark stuff)

Right, but that also happens now if I make some bookmarks with large font and others with small font.

I think the new menu should go into the first top menu (with TOC, Bookmarks, Reference pages) : and should not be shown when the document does not have any non-linear stuff. It should appear there only when there are (like Reference pages do)

Hmm... But I want to be able to enable it for all books by default, even if the current book does not have it. Also, I thought that this is something that affects the formatting of the book and the reading experience, and it's not a tool for navigation, so it seems to fit better the second menu, doesn't it?

frontend/apps/reader/modules/readerrolling.lua

frontend/document/credocument.lua

poire-z · 2020-11-04T19:59:24Z

Hmm... But I want to be able to enable it for all books by default, even if the current book does not have it.

I think you need a way to show the user that this book has non-linear flows or not. No need for him to toggle it and then try to see if something has changed like total nb of pages to notice/guess that if not, there's no linear flow. So, the presence of this menu is a good indicator the document has them, and that there's a feature that may makes the reading experience better. Seeing that menu always and toggling it and seeing it has no effect won't advertize how nice that feature can be.
(Having it grey/not enabled when the book does not have non-linear stuff can be a bit confusing. Our other menu items are grey when they depend on other features/settings - so a user, seeing it grey, might want to try tweaking other stuff to make it non-grey/enabled.)

As for the hold_callback to enable it on all books - well, you'll do that on a book that has it :) (in cre, you can't enable pdf stuff on all books).

Also, I thought that this is something that affects the formatting of the book and the reading experience, and it's not a tool for navigation, so it seems to fit better the second menu, doesn't it?

It doesn't at all to me :) How it affects formatting is a side effect I'd say. Most of the things this PR changes are related to pages, TOC, Bookmarks, the skimto widget after all :) things that all are in that first menu.

(For these 2 points, I consider your feature not much different than the Reference page stuff I added - So, it'd be nice they show up, work and are advertize quite similarly.)

poire-z

OK, better second impression after another hour reading :)
With better names and a bit more wrapping, it could be fine.

poire-z · 2020-11-04T21:03:45Z

frontend/apps/reader/modules/readerbookmark.lua

+            local flow = self.ui.document:getPageFlow(page)
+            page = self.ui.document:getPageNumberInFlow(page)
+            if flow > 0 then
+                page = T("[%1]%2", page, flow)
+            end


I'd be happier if that kind of things would be written:

local page = self.ui.document:getPageFromXPointer(item.updated_highlight.pos0) local flow = self.ui.document:getPageFlow(page) -- 0 when no (or not hidden) non-linear flows if flow > 0 then page = self.ui.document:getPageNumberInFlow(page) page = T("[%1]%2", page, flow) end

or even:

local page = self.ui.document:getPageFromXPointer(item.updated_highlight.pos0) if self.ui.document:hasHiddenFlows() then local flow = self.ui.document:getPageFlow(page) -- 0 when no (or not hidden) non-linear flows if flow > 0 then page = self.ui.document:getPageNumberInFlow(page) page = T("[%1]%2", page, flow) end end

no matter the additional function call.

poire-z · 2020-11-04T21:06:00Z

frontend/apps/reader/modules/readerfooter.lua

-        local left = footer.ui.toc:getChapterPagesLeft(footer.pageno)
+        local left = footer.ui.toc:getChapterPagesLeft(footer.ui:getCurrentPage())


Why that change for this one - and not for the others (like the one above you modified)?
Can footer.pageno be different from footer.ui:getCurrentPage() ?

Can footer.pageno be different from footer.ui:getCurrentPage() ?

Yes, footer.pageno is the page number shown in the footer, the "local" page in the current flow, while footer.ui:getCurrentPage() is the global page number, counting all flows.

poire-z · 2020-11-04T21:07:20Z

frontend/apps/reader/modules/readerfooter.lua

@@ -438,6 +448,7 @@ function ReaderFooter:init()
        return
    end

+    self.flow = 0


self.current_flow might be a more explicite name?

poire-z · 2020-11-04T21:10:14Z

frontend/apps/reader/modules/readerfooter.lua

@@ -1869,20 +1887,38 @@ function ReaderFooter:_updateFooterText(force_repaint, force_recompute)
    end
 end

+function ReaderFooter:onChangeViewMode()
+    self:onPageUpdate()


May be a comment about why you need to call it.
(I have the feeling it would be called anyway because of other events, but better explicite than implicite if it being called multiple times is not an issue.)

poire-z · 2020-11-04T21:11:36Z

frontend/apps/reader/modules/readerrolling.lua

@@ -55,7 +55,8 @@ local ReaderRolling = InputContainer:new{
    -- With visible_pages=2, in 2-pages mode, ensure the first
    -- page is always odd or even (odd is logical to avoid a
    -- same page when turning first 2-pages set of document)
-    odd_or_even_first_page = 1 -- 1 = odd, 2 = even, nil or others = free
+    odd_or_even_first_page = 1, -- 1 = odd, 2 = even, nil or others = free
+    hide_nonlinear = nil,


hide_nonlinear_flows may be, to make it clearer it's related to "flows" elsewhere.

poire-z · 2020-11-04T21:21:57Z

frontend/document/credocument.lua

@@ -58,6 +58,10 @@ local CreDocument = Document:new{
    default_css = "./data/cr3.css",
    provider = "crengine",
    provider_name = "Cool Reader Engine",
+
+    hide_nonlinear = false,


hide_nonlinear_flows here too, to make it clearer it's related to "flows" elsewhere.

poire-z · 2020-11-04T21:25:23Z

frontend/document/credocument.lua

+    end
+end
+
+function CreDocument:hasFlows()


I think just seeing this one named hasHiddenFlows() and checking for self.hide_nonlinear_flows - and abused a bit more as wrappers - would make me quite happy with this PR.
(And moved above before all the other :*Flow* functions.)

poire-z · 2020-11-04T21:25:56Z

frontend/document/credocument.lua

+function CreDocument:cacheFlows()
+    -- Build the cache tables "flows" and "page_in_flow", if there are


I'd rather see this one called and do its work only when hide_nonlinear_flows == true

poire-z · 2020-11-04T21:28:13Z

frontend/ui/widget/progresswidget.lua

@@ -90,8 +92,8 @@ function ProgressWidget:paintTo(bb, x, y)
        local bar_width = (my_size.w-2*self.margin_h)
        local y_pos = y + self.margin_v + self.bordersize
        local bar_height = my_size.h-2*(self.margin_v+self.bordersize)
-        for i=1, #self.ticks do
-            local tick_x = bar_width*(self.ticks[i]/self.last)
+        for i,tick in pairs(self.ticks) do


Try to not switch from array iterator to pairs(). We had bad experiences with pairs() in these kind of situations, and switch a lot to ipairs or for i=1, #something.

That one is still outstanding ;).

And since we were talking about rounding, I realize that tick_x was already never rounded here, wheee... -_-".

(At a quick glance, bar_width should always be an int, but the MUL against tick/last might change that.

poire-z · 2020-11-04T21:30:59Z

frontend/ui/elements/reader_menu_order.lua

@@ -26,6 +26,7 @@ local order = {
    typeset = {
        "set_render_style",
        "style_tweaks",
+        "hide_nonlinear",


hide_nonlinear_flows too to make that more explicite.

poire-z · 2020-11-06T18:52:22Z

Following up on #6847 (comment) above here (as I don't like this discussion hidden in the hidden comments above).

Having crengine send a signal to ReaderRolling and then ReaderRolling send it "back" to CreDocument looks like an unnecessary roundabout

That's mostly how KOReader works everywhere. Roundabouts is its middle name (think about all the events). So, it's not at all an issue for me.

Isn't there an easy way for CreDocument to react to crengine events

Well, up until you introduced a cache for page flows in it, it was just a thin wrapper, a pure proxy to crengine, with not much state inside of it. And I think until some bigger refactoring is needed, it should stay that way.
ReaderRolling is the "active" higher level wrapper to CreDocument.
But I'm still fine with you having that cache in CreDocument.

There is one little issue with handleEngineCallback: it's done in some kind of out of the main Lua state, and it won't crash the app if you have some coding error in it - so, we don't want to do too much stuff thru it:

koreader/frontend/apps/reader/modules/readerrolling.lua

Lines 236 to 240 in 5ed2400

    
           self.ui.document:setCallback(function(...) 
        
               -- Catch and log any error happening in handleCallback(), 
        
               -- as otherwise it would just silently abort (but beware 
        
               -- having errors, this may flood crash.log) 
        
               local ok, err = xpcall(self.handleEngineCallback, debug.traceback, self, ...)

I see you do self:cacheFlows() inside CreDocument:setViewMode() and when toggling hide_nonlinear - which are two things that do not do any re-rendering (so, they don't change the document height) but needs flows to be updated.
I also realize the thing in updatePos() to check for a document-height change to notice there was a re-rendering is not perfect: there may be ultra rare cases when changes of styles may move chapters around a bit (and so, pages of some xpointers) while not changing 1px in document height. Also, toggling hide_nonlinear will most often not change the document-height.
The handleEngineCallback was added recently - and before it, there was indeed no way to know that a real re-rendering did happen (so, the check for document-height as the best available solution).

Not for you to do in this PR (unless you think that could make your handling clearer) but a more generic solution to handling that could be:
Here in ReaderRolling:handleEngineCallback():
if ev == "OnDocumentReady" then self.document.re_rendered = true end
and in updatePos() check for self.document.re_rendered (and reset it to false once checked) instead of comparing old and current document height.
"updatePos" is a bad name - but I still think everything should be done in this central place for stuff that should react to a re-rendering - including your call to CreDocument:cacheFlows().

(I'd say that even if your cached page flows are internal to CreDocument, updating them should be done from ReaderRolling, in the upper equivalent that calls the lower CreDocument:setViewMode() and CreDocument:setHideNonlinear() - but I understand you'd like them internal, so I won't be rigid on that :)

And another thing. Is there any guaranteed order in which onUpdateToc methods of different classes/instances are processed? If I want to make sure that the footer is updated after ReaderToc:onUpdateToc, can I just add a ReaderFooter:onUpdateToc method or should I create a different event in ReaderToc:onUpdateToc and make ReaderFooter react to that?

No real contract on that I think. But the implementation (WidgetContainer:propagateEvent()) uses ipairs() to iterate thru the sub widgets. So, the Reader* modules would be processeded in the order they are ReaderUI:registerModule()'d in ReaderUI.
Currently, I think the ReaderFooter (not a child of ReaderUI, but of its ReaderView child) would get it before ReaderToc (as self:registerModule("view", ReaderView happens before self:registerModule("toc", ReaderToc).
So, possibly safer and clearer to add another event send by ReaderToc:onUpdateToc : TocUpdated - that's the way we naturally round about :)

Jellby · 2020-11-07T12:33:17Z

frontend/apps/reader/modules/readerfooter.lua

    end,
    chapter_time_to_read = function(footer)
        local symbol_type = footer.settings.item_prefix or "icons"
        local prefix = symbol_prefix[symbol_type].chapter_time_to_read
-        local left = footer.ui.toc:getChapterPagesLeft(footer.pageno)
+        local left = footer.ui.toc:getChapterPagesLeft(footer.ui:getCurrentPage())
        return footer:getDataFromStatistics(
            prefix .. " ", (left and left or footer.pages - footer.pageno))


Not affected by this PR, but is this construction needed? Wouldn't (left or footer.pages - footer.pageno) be exactly the same?

Looks to me that indeed, it's exactly the same (unless I miss something with Lua pecularities :)

AFAICT, If there are no ticks at all (i.e., no chapter breaks), ReaderToc:getNextChapter will return nil, and ReaderToc:getChapterPagesLeft will return it as-is.

Maths with a nil in the mix == kablooey ;).

That explains the need for left or ..., but not for left and left? If left is nil, left or "foo" is "foo", and left and left or "foo" is "foo" as well... at least as far as I can see.

Oh, yeah, I totally skipped over the fact that there was an or after :D.

Jellby · 2020-11-07T12:59:44Z

(issue with bookmarks which could have some the normal page, some the tweaked non-linear page number, depending on whether they were made in one mode or the other - but that's a more general issue with our messy Bookmark stuff)

Right, but that also happens now if I make some bookmarks with large font and others with small font.

Maybe a matter for a different PR, but would it make sense to set the default bookmark title not to the page number, but to some placeholder, like %p or whatever, and substitute that with the "current" page number when displaying them (as with TOC page numbers)?

poire-z · 2020-11-07T13:26:35Z

Maybe a matter for a different PR, but would it make sense to set the default bookmark title not to the page number

Yep, that would make sense. And for a different PR. #5794 has some discussion about better handling of all that bookmark mess. (Fell free to take over that task if you are courageous :)

NiLuJe · 2020-11-07T13:56:36Z

frontend/ui/widget/progresswidget.lua

-        for i=1, #self.ticks do
-            local tick_x = bar_width*(self.ticks[i]/self.last)
+        for i,tick in pairs(self.ticks) do
+            local tick_x = bar_width*((tick-0.5)/self.last)


I'm... not necessarily a fan of that ;).

And, at the very least, it'll probably require rounding to ensure it doesn't randomly decide to jump a pixel or not ;).

Well, either there is a non-existing "page 0" represented in the progress bar, or the last page is missing. If I use the plain page numbers for the non-linear filling, I get an overflow:

which I can solve by using page-1, but then the ticks are off:

So with tick-0.5 the ticks are "midpage", they're still off, but at least they're not at the boundary of any page:

As for rounding, I don't think it's needed. tick should be an integer, and even though tick_x can jump one pixel, it will always be between the values corresponding to tick and tick-1. In the case when each page takes less than one pixel, I don't really care which one it is, there's not enough resolution anyway.

I'd rather keep the current behavior and just fix the overflow, FWIW ;).

(As in, they're meant to be at boundaries: they are the boundaries).

To fix the overflow and keep the ticks at the boundary, everything should be moved -1 page. Then the first page would have no filled progress bar, and the last page would not be 100% filled (which would make sense if the bar means "pages already read"), or the progress bar should be divided in pages-1 chunks, instead of pages.

But ok, I'll leave it as is. I don't really use the skim widget enough.

As for rounding, I don't think it's needed

Not rounding what might be passed to the drawing code has been the cause of positionning & fitting issues (even in upper containers far from what you're dealing with) in the past. So, please do if you can :)

I have no real opinion on #6831 because I don't use it that much to jump - I just expect it to be "clean", so my preference among the 3 above screenshots is the first (without the overflow of course :) The 2 others, no matter how correct they may be for the person who gave it some thoughts, will look like a bug for most of the other users :/

The first is wronger, because the first page is non-linear, and it appears as the second, it's the same -1 offset. If the first page is non-linear, it should go from 0 to 1, and if the last 2 pages (out of 100) are non-linear, the grey should go from 98 to 100 (even though the pages are 99 and 100).

So, my point is the grey blocks in the 2nd and 3rd example are correct. The ticks in 1st and 2nd are correct, if they are interpreted as marking the end of a page where a chapter starts, and the filled bar is correct if it's interpreted as all pages up to (and including) the current one.

My stance on the whole #6831 discussion was that I might be agreeable to modifying the way the bar is filled in, but that the ticks should very much stick to what they're currently doing.

And as the two are somewhat intricately linked, that only leaves room to really fudge the start & end of the bar, and possibly how the current page is filled in (or not) ;).

To fix the overflow and keep the ticks at the boundary, everything should be moved -1 page. Then the first page would have no filled progress bar, and the last page would not be 100% filled (which would make sense if the bar means "pages already read"), or the progress bar should be divided in pages-1 chunks, instead of pages.

FWIW, I'd probably be amenable to either of those approaches, I think ;).

(Never seeing the bar fully filled on the last page kinda makes sense to me).

Jellby · 2020-11-07T15:29:45Z

I think I've addressed most, if not all the comments so far with the new commit.

frontend/ui/widget/progresswidget.lua

frontend/apps/reader/modules/readerrolling.lua

poire-z

Looks fine (readable, and feeling safe with the hasHiddenFlows() wrappers :)
Mostly nitpicks:

frontend/apps/reader/modules/readerfooter.lua

poire-z · 2020-11-07T16:40:11Z

frontend/apps/reader/modules/readergoto.lua

+                    if self.ui.document:getPagenNumberInFlow(i) == number and
+                       self.ui.document:getPageFlow(i) == flow then
+                        self.ui:handleEvent(Event:new("GotoPage", i))


(Indent a bit more line 154, or make it all on line 153. That too small indentation difference makes me read it wrong the first time - and wondered about the following.)
Can't you break before you reach i=getPageCount() if the number in flow provided is > getPagenNumberInFlow() ?

Do you mean if I want to go to page 30 in a flow that has only 5 pages? Sure... also if I want to go to page 0 or -7.

Yep, that's what I mean (but mostly to not just go scan 2900 pages in a 3000 pages books to realize an early flow won't be found in there :)

frontend/apps/reader/modules/readerrolling.lua

poire-z · 2020-11-07T17:10:19Z

frontend/apps/reader/modules/readertoc.lua

    self:resetToc()
+    self.ui:handleEvent(Event:new("TocUpdated"))


resetToc() just clears the TOC data (so it's rebuild later when needed).
Don't you need it updated for when you catch TocUpdated (or so that that "TocUpdated" event is true to its name) ?
(But beware, rebuilding the TOC might be expensive, so that's why we delay it until needed.)

Hmm... Maybe I should just change the name, because as I see it, when the TOC is reset, the footer needs to rebuild it to get the markers. So yes, I need the updated TOC, but I guess this need triggers the rebuild.

Also please have a quick check that on loading a book (with cache or without), the TOC is only built once.
(I remember #3292 and #3261 (comment) - which are quite old - dunno if these thoughts are still relevant :)

Right, I think there was one TOC too many which should be fixed now.

frontend/apps/reader/modules/readertoc.lua

poire-z · 2020-11-07T17:24:21Z

frontend/document/credocument.lua

+        for test_page=page+1, self:getPageCount() do
+            local test_page_flow = self:getPageFlow(test_page)
+            if test_page_flow == flow or test_page_flow == 0 then
+                return test_page
+            end
+        end
+        return 0


No case for breaking that for loop before reaching getPageCount() ? (I guess you can't for flow 0, but just asking).

I don't think so. You have to keep testing until you find flow 0 or the end of the document. Unless you know in advance that a) you have reached the end of the current flow (this we can know as soon as a different flow is found), and b) there are no remaining pages in flow 0 (this is the tricky part).

But nothing that another cache item cannot fix.

frontend/document/credocument.lua

frontend/ui/widget/progresswidget.lua

poire-z · 2020-11-07T20:46:23Z

frontend/ui/widget/progresswidget.lua

@@ -117,7 +118,7 @@ function ProgressWidget:paintTo(bb, x, y)
        local bar_width = (my_size.w-2*self.margin_h)
        local y_pos = y + self.margin_v + self.bordersize
        local bar_height = my_size.h-2*(self.margin_v+self.bordersize)
-        for i,tick in pairs(self.ticks) do
+        for i,tick in ipairs(self.ticks) do


We like a space after a comma, like in English :)

poire-z · 2020-11-09T12:58:57Z

frontend/apps/reader/modules/readerfooter.lua

+function ReaderFooter:onTocReset()
+    self:setTocMarkers(true)
+    self:onUpdateFooter()


I'm not sure how soon after a toc reset the TOC was filled again before this PR (might be that it was delayed after a repaint, dunno). But with this, is will be immediately filled.
(I can't answer that question after a quick look at the code, so, just asking in case someone get an idea :)

Note that ReaderRolling:updatePos() had before:

self.ui:handleEvent(Event:new("UpdateToc")) self.view.footer:setTocMarkers(true) self.view.footer:onUpdateFooter()

so nothing should have changed in that particular chain of events.

poire-z · 2020-11-09T13:02:33Z

frontend/apps/reader/modules/readerrolling.lua

@@ -457,6 +468,31 @@ You can set how many lines are shown.]])
        help_text = _([[When page overlap is enabled, some lines from the previous pages are shown on the next page.]]),
        sub_item_table = page_overlap_menu,
    }
+    if self.view.document:hasNonLinearFlows() then


I know we use self.view.document in a few modules - but not in this one, readerrolling.
The canonical way to access the document object is usually with self.ui.document (ui instead of view).

At least readerfooter uses both self.ui.document and self.view.document. Any reason for this or is it OK if I change them all to self.ui.document too?

poire-z · 2020-11-09T13:06:49Z

frontend/document/credocument.lua

+            if test_page_flow ~= flow and test_page > last_linear_page then
+                break


Can't you have/use a getLastPageInFlow(flow) when flow > 0 (like you have in the next function) ?
As when a flow is done, you can't meet it again - so it's no use checking up to the last linear page. No?

You can't meet it again, but you will meet flow 0 eventually (unless test_page > last_linear_page), and that will be the "next page". I could limit the loop to math.max(self:getLastPageInFlow(flow), self:getLastLinearPage()).

ETA: I think I misunderstood you and forgot there is no getLastPageInFlow currently...

Or I misunderstood this function :)
-- Get the next/prev page number, skipping non-linear flows,
I thought you could use it to navigate pages inside a non-linear flow - and that if you're in flow No7 out of 19, you don't need to test pages until the end of the document.

You skip non-linear flows, but linear pages may be interspersed between them, and we want to stop there.

But you really don't use CreDocument:getNextPage(p) to page forward inside non-linear flows (and expecting to get the next page inside this same non-linear flow)?
OK, I guess you do (so, this comment is lying :) and you'll stop looping when you meet back a page from flow 0.

There could be another performance improvement: when a different non-linear flow is found, instead of trying pages one by one, I could in principle skip all n pages in the flow in one go. But I somehow liked the simplicity of just paging through, it makes fewer assumptions about how the flows behave (for instance, non-linear flows should not be interleaved, but the one-by-one method would work even if that was the case).

poire-z · 2020-11-09T16:35:04Z

frontend/document/credocument.lua

+        elseif page == 0 then
+            return self.getLastLinearPage()
+        end


In onGoToBegenning, you provide page=0 to go to the first page - which sounds like it's not what you do :)

Of course, I got them mixed.

Jellby · 2020-11-10T19:40:44Z

Maybe I should use #6859 instead of forcing no_wrap on selected methods?

poire-z · 2020-11-10T20:27:49Z

Isn't a reset already forced when you call CreDocument:setHideNonlinearFlows() (because of set*)? (#6854 added a setMethod, #6859 removed it - but you need one :)
(Or did you have something else in mind?)

But may be you could have some of them not be no_wrap (no cache) and neither cache_global (long cache), and have some of them cache_by_tag (cache resut by page, with a 10 MRU stack).

Jellby · 2020-11-11T11:11:28Z

I had #6809 (comment) in mind, but I'll try reproducing/investigating it again, and I'll come back if I can't figure it out.

poire-z · 2020-11-11T11:52:41Z

Had a try on my Kobo, looks ok (as far as the things I care about as concerned, not tested bookmarks, some footer items...)
(A small initial feeling that turning pages was slower on books without non-linear stuff - but I can't see any reason in the code, so may be it's a suggestive impression because I was looking for slowness :)

So, I reported previously that 90% of my books don't have any non-linear stuff.
It turns out that among the 10% that do, 90% of them just have the first page with the cover marked as non-linear :/
I managed to find only 2 books (+ your 1001 nights one) that have some non-linear stuff all along the book, mostly footnotes sections. It works fine: you can't reach them with tap to turn page, but you can with the Skim widget +1 / -1. I was looking for these sections, but they were hard to find, so the hiding works well :) (but I'll personnally prefer not hiding anything and skipping them manually).

I think the footer marking is good (43//67 is nice to indicate you're in this mode, [1/4]3) is ok. although the fragment number is a bit useless).

poire-z · 2020-11-18T12:47:08Z

Still a few luacheck warnings to solve.

poire-z · 2020-11-18T16:46:22Z

Now some issues with unit tests that might need some update to the new functions.

[ RUN      ] spec/front/unit/readerrolling_spec.lua @ 51: Readerrolling module test in portrait screen mode should goto previous chapter
frontend/apps/reader/modules/readerrolling.lua:594: attempt to call method 'getPrevChapter' (a nil value)

stack traceback:
	frontend/apps/reader/modules/readerrolling.lua:594: in function 'onGotoPrevChapter'
	spec/front/unit/readerrolling_spec.lua:55: in function <spec/front/unit/readerrolling_spec.lua:51>

[  ERROR   ] spec/front/unit/readerrolling_spec.lua @ 51: Readerrolling module test in portrait screen mode should goto previous chapter (0.35 ms)

[ RUN      ] spec/front/unit/readerrolling_spec.lua @ 139: Readerrolling module test in landscape screen mode should goto previous chapter
frontend/apps/reader/modules/readerrolling.lua:594: attempt to call method 'getPrevChapter' (a nil value)

stack traceback:
	frontend/apps/reader/modules/readerrolling.lua:594: in function 'onGotoPrevChapter'
	spec/front/unit/readerrolling_spec.lua:143: in function <spec/front/unit/readerrolling_spec.lua:139>

[  ERROR   ] spec/front/unit/readerrolling_spec.lua @ 139: Readerrolling module test in landscape screen mode should goto previous chapter (0.38 ms)

Add option to hide (skip) non-linear fragments, only working in 1-page mode. Tweaks mostly to footer, toc and skim code to make it clear(er) which pages belong to linear or non-linear fragments.

As an aside, it should be "ToC," not "Toc." ;-) Cf. <koreader#6847>.

As an aside, it should be "ToC," not "Toc." ;-) Cf. <#6847>. * Also fix newlines in to be translated string

poire-z · 2023-08-29T17:25:29Z

@Jellby : hope you haven't forgotten everything about this PR :)

I'm reusing much of it to implement "handmade hidden flows" (#10193 (comment)), that would allow one to set hidden flows on PDF documents (and also on CRE documents whether they have non-linear fragments or not).
I'm just implementing/plugging/overridding the no-op functions you added to Document, and let all the toc/footer/bookmap/... works just as you made them work with non-linear-fragments hidden flows.

I have a few questions regarding your code:

koreader/frontend/document/document.lua

Lines 235 to 244 in 38d468f

    
           function Document:getNextPage(page) 
        
               local new_page = page + 1 
        
               return (new_page > 0 and new_page < self.info.number_of_pages) and new_page or 0 
        
           end 
        
           function Document:getPrevPage(page) 
        
               if page == 0 then return self.info.number_of_pages end 
        
               local new_page = page - 1 
        
               return (new_page > 0 and new_page < self.info.number_of_pages) and new_page or 0 
        
           end

(I have to make ReaderPaging use these instead of current_page = current_page+1 / -1 - for the many ways they are used, paging, scrolling, zoom mode, reflow.... and it's not really fun :/)

You allow these methods to return 0. I assume it means for the calling code that it has reaches the end or start of the document, and that it's for that calling code to handle that case. Right?

Also, when called with page=0, they return the page number of the other side, so, getPrevPage(0) returns the last page (of the main flow) - looks like it was not implemented for Document:getNextPage(0), but it is for CreDocument:getNextPage(0) which returns 1 or the start of the main flow - so I guess I'd have to fix this, if it is really used?)
Fine with this, just curious about why you needed that behaviour.

Also, in the snippet above, it looks like there's a logical bug (< vs <=) - shouldn't it be changed to this?:

- return (new_page > 0 and new_page < self.info.number_of_pages) and new_page or 0 
+ return (new_page > 0 and new_page <= self.info.number_of_pages) and new_page or 0

Also mentionning a little bug (that is not part of my scope :) that I noticed, and that I can reproduce on an EPUB with (sick) non-linear fragments:

With chapters spanning fragment boundaries like above, the footer "page left in chapter" would show in the above case at the position of the arrow: 13//21 (so, including the pages in yellow of the chapter continuing when the main flow is reached again - but not the same chapter that was active when the main flow stops) instead of 1//21 (if stopping when the main flow stops, or 3//21 (if accounting the pages in that chapter even if part of a hidden flow).

Jellby · 2023-08-29T19:46:07Z

You allow these methods to return 0. I assume it means for the calling code that it has reaches the end or start of the document, and that it's for that calling code to handle that case. Right?

I guess so, or just to return a reproducible and recognizable value whenever something wrong happens.

Also, when called with page=0, they return the page number of the other side, so, getPrevPage(0) returns the last page (of the main flow) - looks like it was not implemented for Document:getNextPage(0), but it is for CreDocument:getNextPage(0) which returns 1 or the start of the main flow - so I guess I'd have to fix this, if it is really used?) Fine with this, just curious about why you needed that behaviour.

I think that was simply to have kind of "overloaded" functions that still do something useful with 0. That way I could just say getNextPage(0) to find the first (linear) page of the document, which makes sense (as it would by default be page 1), and then getPrevPage(0) to find the last one is the obvious partner.. These are already used in a couple of places.

Document:getNextPage(0) already returns 1 without a special treatment, or am I missing something?

Also, in the snippet above, it looks like there's a logical bug (< vs <=) - shouldn't it be changed to this?:
- return (new_page > 0 and new_page < self.info.number_of_pages) and new_page or 0 
+ return (new_page > 0 and new_page <= self.info.number_of_pages) and new_page or 0 

You're probably right.

With chapters spanning fragment boundaries like above, the footer "page left in chapter" would show in the above case at the position of the arrow: 13//21 (so, including the pages in yellow of the chapter continuing when the main flow is reached again - but not the same chapter that was active when the main flow stops) instead of 1//21 (if stopping when the main flow stops, or 3//21 (if accounting the pages in that chapter even if part of a hidden flow).

I believe I consciously assumed that fragments and chapters would be "properly nested", either a fragment contains full chapters or is fully contained in a chapter. I knew that's not necessarily the case, but it certainly matches my use cases, and figured it would also be the most common situation. Given that there is formally no concept of "chapter" in epub 2 (only TOC and hyperlink targets, which may signal the "beginning", but no the end of a chapter), I wouldn't consider the current behaviour wrong, just... "creative" :)

poire-z · 2023-08-29T20:44:40Z

Document:getNextPage(0) already returns 1 without a special treatment, or am I missing something?

No, you're right, it works - as undocumented ;). I missed your comment explaining the implicit behaviour :) (Was expecting some kind of symetry.)

I think that was simply to have kind of "overloaded" functions that still do something useful with 0. That way I could just say getNextPage(0) to find the first (linear) page of the document, which makes sense (as it would by default be page 1), and then getPrevPage(0) to find the last one is the obvious partner

Got it. But when I read them first, it feels very twisted that getNextPage does not return a page in the direction of "next" :)

Frenzie added this to the 2020.12 milestone Nov 4, 2020

Frenzie reviewed Nov 4, 2020

View reviewed changes

NiLuJe reviewed Nov 4, 2020

View reviewed changes

frontend/apps/reader/modules/readerrolling.lua Show resolved Hide resolved

NiLuJe reviewed Nov 4, 2020

View reviewed changes

Galunid reviewed Nov 4, 2020

View reviewed changes

frontend/apps/reader/modules/readerrolling.lua Outdated Show resolved Hide resolved

frontend/document/credocument.lua Outdated Show resolved Hide resolved

poire-z reviewed Nov 4, 2020

View reviewed changes

Jellby commented Nov 7, 2020

View reviewed changes

NiLuJe reviewed Nov 7, 2020

View reviewed changes

frontend/ui/widget/progresswidget.lua Show resolved Hide resolved

poire-z reviewed Nov 7, 2020

View reviewed changes

frontend/apps/reader/modules/readerrolling.lua Outdated Show resolved Hide resolved

poire-z reviewed Nov 7, 2020

View reviewed changes

frontend/ui/widget/progresswidget.lua Outdated Show resolved Hide resolved

poire-z reviewed Nov 7, 2020

View reviewed changes

poire-z reviewed Nov 9, 2020

View reviewed changes

Jellby force-pushed the nonlinear branch from 7072c6c to ce13e27 Compare November 18, 2020 10:56

poire-z approved these changes Nov 18, 2020

View reviewed changes

Jellby force-pushed the nonlinear branch from ce13e27 to 3a46451 Compare November 18, 2020 13:07

Jellby added 2 commits November 18, 2020 17:58

Fix typos

2f45765

Hide non-linear fragments

f97455f

Add option to hide (skip) non-linear fragments, only working in 1-page mode. Tweaks mostly to footer, toc and skim code to make it clear(er) which pages belong to linear or non-linear fragments.

Jellby force-pushed the nonlinear branch from 3a46451 to f97455f Compare November 18, 2020 16:59

poire-z merged commit 5e3c554 into koreader:master Nov 18, 2020

Frenzie added a commit to Frenzie/koreader that referenced this pull request Nov 19, 2020

[lang] Minor touch-up to non-linear info text

d1aa983

As an aside, it should be "ToC," not "Toc." ;-) Cf. <koreader#6847>.

Frenzie mentioned this pull request Nov 19, 2020

[lang] Minor touch-up to non-linear info text #6895

Merged

Frenzie added a commit that referenced this pull request Nov 20, 2020

[lang] Minor touch-up to non-linear info text (#6895)

ded8ae6

As an aside, it should be "ToC," not "Toc." ;-) Cf. <#6847>. * Also fix newlines in to be translated string

poire-z mentioned this pull request Nov 27, 2020

FB2: mark 2nd++ BODYs as non-linear, fix base64 images in EPUB, sync with upstream koreader/crengine#395

Merged

NiLuJe mentioned this pull request Dec 10, 2020

FR: Ability to sync page counter with an actual book's page #6971

Closed

poire-z mentioned this pull request Feb 1, 2021

FR: Customize progress bar to display only h1 chapter markers #7141

Closed

poire-z mentioned this pull request May 22, 2021

FR: Set "end book at" page in place of last page #7729

Closed

Frenzie mentioned this pull request Sep 8, 2021

Progresswidget: invert read chapter marks #8195

Merged

hius07 mentioned this pull request Sep 24, 2021

Progress Tracking in Academic Books #8254

Closed

poire-z mentioned this pull request Sep 26, 2021

Set 'last' page number #8265

Closed

hius07 mentioned this pull request Dec 13, 2021

Add Readwise.io support to highlight exporter plugin #8548

Merged

poire-z mentioned this pull request Jan 6, 2022

EPUB3 <aside> footnotes rendered twice #8623

Closed

poire-z mentioned this pull request Mar 27, 2023

FR: Exclude index, bibliography pages, etc. from reading statistics #10253

Closed

poire-z mentioned this pull request Apr 30, 2023

Current Page symbol changed from / to // #10375

Closed

poire-z mentioned this pull request May 16, 2023

FR: BookMap enhancements #10433

Closed

poire-z mentioned this pull request Sep 8, 2023

FR: Toggle bookmarks within Page Browser & ReaderHandmade stuff (Custom TOC/Hidden Flows) #10193

Closed

poire-z mentioned this pull request Oct 6, 2023

ReaderHandmade: support for custom TOC and hidden flows. BookMap and PageBrowser tweaks #10982

Merged

poire-z mentioned this pull request Feb 20, 2024

User Guide Maintenance #11147

Open

		local left = footer.ui.toc:getChapterPagesLeft(footer.pageno)
		local left = footer.ui.toc:getChapterPagesLeft(footer.ui:getCurrentPage())

		function CreDocument:cacheFlows()
		-- Build the cache tables "flows" and "page_in_flow", if there are

		if test_page_flow ~= flow and test_page > last_linear_page then
		break

Support for hiding (skipping) non-linear EPUB fragments #6847

Support for hiding (skipping) non-linear EPUB fragments #6847

Conversation

Jellby commented Nov 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WaseemAlkurdi Nov 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NiLuJe Nov 4, 2020 • edited Loading

Choose a reason for hiding this comment

poire-z commented Nov 4, 2020

NiLuJe commented Nov 4, 2020 • edited Loading

Jellby commented Nov 4, 2020 • edited Loading

poire-z commented Nov 4, 2020

poire-z left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NiLuJe Nov 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poire-z commented Nov 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NiLuJe Nov 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jellby commented Nov 7, 2020

poire-z commented Nov 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NiLuJe Nov 7, 2020 • edited Loading

Choose a reason for hiding this comment

Jellby Nov 7, 2020 • edited Loading

Choose a reason for hiding this comment

poire-z Nov 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NiLuJe Nov 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jellby commented Nov 7, 2020

poire-z left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jellby Nov 9, 2020 • edited Loading

Jellby commented Nov 4, 2020 •

edited

Loading

WaseemAlkurdi Nov 5, 2020 •

edited

Loading

NiLuJe Nov 4, 2020 •

edited

Loading

NiLuJe commented Nov 4, 2020 •

edited

Loading

Jellby commented Nov 4, 2020 •

edited

Loading

NiLuJe Nov 7, 2020 •

edited

Loading

NiLuJe Nov 7, 2020 •

edited

Loading

NiLuJe Nov 7, 2020 •

edited

Loading

Jellby Nov 7, 2020 •

edited

Loading

poire-z Nov 7, 2020 •

edited

Loading

NiLuJe Nov 7, 2020 •

edited

Loading

Jellby Nov 9, 2020 •

edited

Loading

Jellby commented Nov 11, 2020 •

edited

Loading