Revamp ToC ticks handling (#6716)

Replace the level `0` `getTocTicks` heuristic with a simple sorted, deduped flat listing of *every* ToC node (via `getTocTicksFlattened`).

frontend/apps/reader/modules/readerfooter.lua

@@ -196,8 +196,7 @@ local footerTextGeneratorMap = {
     pages_left = function(footer)
         local symbol_type = footer.settings.item_prefix or "icons"
         local prefix = symbol_prefix[symbol_type].pages_left
-        local left = footer.ui.toc:getChapterPagesLeft(
+        local left = footer.ui.toc:getChapterPagesLeft(footer.pageno)
-            footer.pageno, footer.toc_level)
         return prefix .. " " .. (left and left or footer.pages - footer.pageno)
     end,
     percentage = function(footer)
@@ -221,8 +220,7 @@ local footerTextGeneratorMap = {
     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(
+        local left = footer.ui.toc:getChapterPagesLeft(footer.pageno)
-            footer.pageno, footer.toc_level)
         return footer:getDataFromStatistics(
             prefix .. " ", (left and left or footer.pages - footer.pageno))
     end,
@@ -294,7 +292,6 @@ local ReaderFooter = WidgetContainer:extend{
     mode = MODE.page_progress,
     pageno = nil,
     pages = nil,
-    toc_level = 0,
     progress_percentage = 0.0,
     footer_text = nil,
     text_font_face = "ffont",
@@ -1676,7 +1673,7 @@ function ReaderFooter:setTocMarkers(reset)
         end
         self.progress_bar.ticks = {}
         if self.ui.toc then
-            self.progress_bar.ticks = self.ui.toc:getTocTicksForFooter()
+            self.progress_bar.ticks = self.ui.toc:getTocTicksFlattened()
         end
         self.progress_bar.last = self.pages or self.view.document:getPageCount()
     else

frontend/apps/reader/modules/readerrolling.lua

@@ -983,7 +983,7 @@ function ReaderRolling:updateTopStatusBarMarkers()
         return
     end
     local pages = self.ui.document:getPageCount()
-    local ticks = self.ui.toc:getTocTicksForFooter()
+    local ticks = self.ui.toc:getTocTicksFlattened()
     self.ui.document:setHeaderProgressMarks(pages, ticks)
 end
 

frontend/apps/reader/modules/readertoc.lua

@@ -17,7 +17,9 @@ local T = require("ffi/util").template
 
 local ReaderToc = InputContainer:new{
     toc = nil,
-    ticks = {},
+    toc_depth = nil,
+    ticks = nil,
+    ticks_flattened = nil,
     toc_indent = "    ",
     collapsed_toc = {},
     collapse_depth = 2,
@@ -48,8 +50,11 @@ end
 
 function ReaderToc:resetToc()
     self.toc = nil
-    self.ticks = {}
+    self.toc_depth = nil
+    self.ticks = nil
+    self.ticks_flattened = nil
     self.collapsed_toc = {}
+    self.collapse_depth = 2
     self.expanded_nodes = {}
 end
 
@@ -72,11 +77,11 @@ function ReaderToc:onPageUpdate(pageno)
             end
         end
 
-        if paging_backward and self:isChapterEnd(pageno, 0) then
+        if paging_backward and self:isChapterEnd(pageno) then
             UIManager:setDirty(nil, "full")
-        elseif self:isChapterStart(pageno, 0) then
+        elseif self:isChapterStart(pageno) then
             UIManager:setDirty(nil, "full")
-        elseif paging_forward and self:isChapterSecondPage(pageno, 0) then
+        elseif paging_forward and self:isChapterSecondPage(pageno) then
             UIManager:setDirty(nil, "full")
         end
     end
@@ -275,6 +280,9 @@ function ReaderToc:getTocTitleOfCurrentPage()
 end
 
 function ReaderToc:getMaxDepth()
+    if self.toc_depth then return self.toc_depth end
+
+    -- Not cached yet, compute it
     self:fillToc()
     local max_depth = 0
     for _, v in ipairs(self.toc) do
@@ -282,97 +290,116 @@ function ReaderToc:getMaxDepth()
             max_depth = v.depth
         end
     end
-    return max_depth
+    self.toc_depth = max_depth
+    return self.toc_depth
 end
 
 --[[
-TOC ticks is a list of page number in ascending order of TOC nodes at certain level
+The ToC ticks is a list of page numbers in ascending order of ToC nodes at a particular depth level.
-positive level counts nodes of the depth level (level 1 for depth 1)
+A positive level returns nodes at that depth level (top-level is 1, depth always matches level. Higher values mean deeper nesting.)
-negative level counts nodes of reversed depth level (level -1 for max_depth)
+A negative level does the same, but computes the depth level in reverse (i.e., -1 is the most deeply nested one).
-zero level counts leaf nodes of the toc tree
+Omitting the level argument returns the full hierarchical table.
 --]]
 function ReaderToc:getTocTicks(level)
-    if self.ticks[level] then return self.ticks[level] end
+    -- Handle negative levels
-    -- build toc ticks if not found
+    if level and level < 0 then
-    self:fillToc()
+        level = self:getMaxDepth() + level + 1
-    local ticks = {}
-
-    if #self.toc > 0 then
-        if level == 0 then
-            local depth = 0
-            for i = #self.toc, 1, -1 do
-                local v = self.toc[i]
-                if v.depth >= depth then
-                    table.insert(ticks, v.page)
     end
-                depth = v.depth
+
+    if level then
+        if self.ticks and self.ticks[level] then
+            return self.ticks[level]
         end
     else
-            local depth
+        if self.ticks then
-            if level > 0 then
+            return self.ticks
-                depth = level
-            else
-                depth = self:getMaxDepth() + level + 1
         end
+    end
+
+    -- Build ToC ticks if not found
+    self:fillToc()
+    self.ticks = {}
+
+    if #self.toc > 0 then
+        -- Start by building a simple hierarchical ToC tick table
         for _, v in ipairs(self.toc) do
-                if v.depth == depth then
+            if not self.ticks[v.depth] then
-                    table.insert(ticks, v.page)
+                self.ticks[v.depth] = {}
             end
+            table.insert(self.ticks[v.depth], v.page)
         end
+
+        -- Normally the ticks are already sorted, but in rare cases,
+        -- ToC nodes may be not in ascending order
+        for k, _ in ipairs(self.ticks) do
+            table.sort(self.ticks[k])
         end
-        -- normally the ticks are sorted already but in rare cases
-        -- toc nodes may be not in ascending order
-        table.sort(ticks)
-        -- cache ticks only if ticks are available
-        self.ticks[level] = ticks
     end
-    return ticks
+
+    if level then
+        return self.ticks[level]
+    else
+        return self.ticks
+    end
 end
 
-function ReaderToc:getTocTicksForFooter()
+--[[
-    local ticks_candidates = {}
+Returns a flattened list of ToC ticks, without duplicates
-    local max_level = self:getMaxDepth()
+]]
-    for i = 0, -max_level, -1 do
+function ReaderToc:getTocTicksFlattened()
-        local ticks = self:getTocTicks(i)
+    if self.ticks_flattened then return self.ticks_flattened end
-        table.insert(ticks_candidates, ticks)
+
+    -- It hasn't been cached yet, compute it.
+    local ticks = self:getTocTicks()
+    local ticks_flattened = {}
+
+    -- Keep track of what we add to avoid duplicates (c.f., https://stackoverflow.com/a/20067270)
+    local hash = {}
+    for _, v in ipairs(ticks) do
+        for depth, page in ipairs(v) do
+            if not hash[page] then
+                table.insert(ticks_flattened, page)
+                hash[page] = true
             end
-    if #ticks_candidates > 0 then
-        -- Find the finest toc ticks by sorting out the largest one
-        table.sort(ticks_candidates, function(a, b) return #a > #b end)
-        return ticks_candidates[1]
         end
-    return {}
+    end
+
+    -- And finally, sort it again
+    table.sort(ticks_flattened)
+
+    self.ticks_flattened = ticks_flattened
+    return self.ticks_flattened
 end
 
-function ReaderToc:getNextChapter(cur_pageno, level)
+function ReaderToc:getNextChapter(cur_pageno)
-    local ticks = self:getTocTicks(level)
+    local ticks = self:getTocTicksFlattened()
     local next_chapter = nil
-    for i = 1, #ticks do
+    for _, page in ipairs(ticks) do
-        if ticks[i] > cur_pageno then
+        if page > cur_pageno then
-            next_chapter = ticks[i]
+            next_chapter = page
             break
         end
     end
     return next_chapter
 end
 
-function ReaderToc:getPreviousChapter(cur_pageno, level)
+function ReaderToc:getPreviousChapter(cur_pageno)
-    local ticks = self:getTocTicks(level)
+    local ticks = self:getTocTicksFlattened()
     local previous_chapter = nil
-    for i = 1, #ticks do
+    for _, page in ipairs(ticks) do
-        if ticks[i] >= cur_pageno then
+        if page >= cur_pageno then
             break
         end
-        previous_chapter = ticks[i]
+        previous_chapter = page
     end
     return previous_chapter
 end
 
-function ReaderToc:isChapterStart(cur_pageno, level)
+function ReaderToc:isChapterStart(cur_pageno)
-    local ticks = self:getTocTicks(level)
+    local ticks = self:getTocTicksFlattened()
     local _start = false
-    for i = 1, #ticks do
+    for _, page in ipairs(ticks) do
-        if ticks[i] == cur_pageno then
+        if page == cur_pageno then
             _start = true
             break
         end
@@ -380,11 +407,11 @@ function ReaderToc:isChapterStart(cur_pageno, level)
     return _start
 end
 
-function ReaderToc:isChapterSecondPage(cur_pageno, level)
+function ReaderToc:isChapterSecondPage(cur_pageno)
-    local ticks = self:getTocTicks(level)
+    local ticks = self:getTocTicksFlattened()
     local _second = false
-    for i = 1, #ticks do
+    for _, page in ipairs(ticks) do
-        if ticks[i] + 1 == cur_pageno then
+        if page + 1 == cur_pageno then
             _second = true
             break
         end
@@ -392,11 +419,11 @@ function ReaderToc:isChapterSecondPage(cur_pageno, level)
     return _second
 end
 
-function ReaderToc:isChapterEnd(cur_pageno, level)
+function ReaderToc:isChapterEnd(cur_pageno)
-    local ticks = self:getTocTicks(level)
+    local ticks = self:getTocTicksFlattened()
     local _end = false
-    for i = 1, #ticks do
+    for _, page in ipairs(ticks) do
-        if ticks[i] - 1 == cur_pageno then
+        if page - 1 == cur_pageno then
             _end = true
             break
         end
@@ -404,18 +431,18 @@ function ReaderToc:isChapterEnd(cur_pageno, level)
     return _end
 end
 
-function ReaderToc:getChapterPagesLeft(pageno, level)
+function ReaderToc:getChapterPagesLeft(pageno)
-    --if self:isChapterEnd(pageno, level) then return 0 end
+    --if self:isChapterEnd(pageno) then return 0 end
-    local next_chapter = self:getNextChapter(pageno, level)
+    local next_chapter = self:getNextChapter(pageno)
     if next_chapter then
         next_chapter = next_chapter - pageno - 1
     end
     return next_chapter
 end
 
-function ReaderToc:getChapterPagesDone(pageno, level)
+function ReaderToc:getChapterPagesDone(pageno)
-    if self:isChapterStart(pageno, level) then return 0 end
+    if self:isChapterStart(pageno) then return 0 end
-    local previous_chapter = self:getPreviousChapter(pageno, level)
+    local previous_chapter = self:getPreviousChapter(pageno)
     if previous_chapter then
         previous_chapter = pageno - previous_chapter
     end

frontend/apps/reader/skimtowidget.lua

@@ -71,19 +71,7 @@ function SkimToWidget:init()
         curr_page_display = self.ui.pagemap:getCurrentPageLabel(true)
     end
 
-    local ticks_candidates = {}
+    self.ticks_flattened = self.ui.toc:getTocTicksFlattened()
-    if self.ui.toc then
-        local max_level = self.ui.toc:getMaxDepth()
-        for i = 0, -max_level, -1 do
-            local ticks = self.ui.toc:getTocTicks(i)
-            table.insert(ticks_candidates, ticks)
-        end
-        -- find the finest toc ticks by sorting out the largest one
-        table.sort(ticks_candidates, function(a, b) return #a > #b end)
-    end
-    if #ticks_candidates > 0 then
-        self.ticks_candidates = ticks_candidates[1]
-    end
 
     local skimto_title = FrameContainer:new{
         padding = Size.padding.default,
@@ -101,7 +89,7 @@ function SkimToWidget:init()
         width = math.floor(self.screen_width * 0.9),
         height = Size.item.height_big,
         percentage = self.curr_page / self.page_count,
-        ticks = self.ticks_candidates,
+        ticks = self.ticks_flattened,
         tick_width = Size.line.medium,
         last = self.page_count,
     }
@@ -205,7 +193,7 @@ function SkimToWidget:init()
         width = self.button_width,
         show_parent = self,
         callback = function()
-            local page = self:getNextChapter(self.curr_page)
+            local page = self.ui.toc:getNextChapter(self.curr_page)
             if page and page >=1 and page <= self.page_count then
                 self:goToPage(page)
             end
@@ -224,7 +212,7 @@ function SkimToWidget:init()
         width = self.button_width,
         show_parent = self,
         callback = function()
-            local page = self:getPrevChapter(self.curr_page)
+            local page = self.ui.toc:getPreviousChapter(self.curr_page)
             if page and page >=1 and page <= self.page_count then
                 self:goToPage(page)
             end
@@ -357,28 +345,6 @@ function SkimToWidget:addOriginToLocationStack(add_current)
     end
 end
 
-function SkimToWidget:getNextChapter(cur_pageno)
-    local next_chapter = nil
-    for i = 1, #self.ticks_candidates do
-        if self.ticks_candidates[i] > cur_pageno then
-            next_chapter = self.ticks_candidates[i]
-            break
-        end
-    end
-    return next_chapter
-end
-
-function SkimToWidget:getPrevChapter(cur_pageno)
-    local previous_chapter = nil
-    for i = 1, #self.ticks_candidates do
-        if self.ticks_candidates[i] >= cur_pageno then
-            break
-        end
-        previous_chapter = self.ticks_candidates[i]
-    end
-    return previous_chapter
-end
-
 function SkimToWidget:onCloseWidget()
     UIManager:setDirty(nil, function()
         return "ui", self.skimto_frame.dimen

spec/unit/readertoc_spec.lua

@@ -31,12 +31,6 @@ describe("Readertoc module", function()
         assert.is.equal("SCENE I. Friar Laurence's cell.", title)
     end)
     describe("getTocTicks API", function()
-        local ticks_level_0 = nil
-        it("should get ticks of level 0", function()
-            ticks_level_0 = toc:getTocTicks(0)
-            --DEBUG("ticks", ticks_level_0)
-            assert.are.same(28, #ticks_level_0)
-        end)
         local ticks_level_1 = nil
         it("should get ticks of level 1", function()
             ticks_level_1 = toc:getTocTicks(1)
@@ -57,26 +51,31 @@ describe("Readertoc module", function()
                 assert.are.same(ticks_level_2, ticks_level_m1)
             end
         end)
+        local ticks_level_flat = nil
+        it("should get all ticks (flattened)", function()
+            ticks_level_flat = toc:getTocTicksFlattened()
+            assert.are.same(28, #ticks_level_flat)
+        end)
     end)
     it("should get page of next chapter", function()
-        assert.truthy(toc:getNextChapter(10, 0) > 10)
+        assert.truthy(toc:getNextChapter(10) > 10)
-        assert.truthy(toc:getNextChapter(100, 0) > 100)
+        assert.truthy(toc:getNextChapter(100) > 100)
-        assert.are.same(nil, toc:getNextChapter(290, 0))
+        assert.are.same(nil, toc:getNextChapter(290))
     end)
     it("should get page of previous chapter", function()
-        assert.truthy(toc:getPreviousChapter(10, 0) < 10)
+        assert.truthy(toc:getPreviousChapter(10) < 10)
-        assert.truthy(toc:getPreviousChapter(100, 0) < 100)
+        assert.truthy(toc:getPreviousChapter(100) < 100)
-        assert.truthy(toc:getPreviousChapter(200, 0) < 200)
+        assert.truthy(toc:getPreviousChapter(200) < 200)
     end)
     it("should get page left of chapter", function()
-        assert.truthy(toc:getChapterPagesLeft(10, 0) > 10)
+        assert.truthy(toc:getChapterPagesLeft(10) > 10)
-        assert.truthy(toc:getChapterPagesLeft(95, 0) > 10)
+        assert.truthy(toc:getChapterPagesLeft(95) > 10)
-        assert.are.same(nil, toc:getChapterPagesLeft(290, 0))
+        assert.are.same(nil, toc:getChapterPagesLeft(290))
     end)
     it("should get page done of chapter", function()
-        assert.truthy(toc:getChapterPagesDone(11, 0) < 5)
+        assert.truthy(toc:getChapterPagesDone(11) < 5)
-        assert.truthy(toc:getChapterPagesDone(88, 0) < 5)
+        assert.truthy(toc:getChapterPagesDone(88) < 5)
-        assert.truthy(toc:getChapterPagesDone(290, 0) > 10)
+        assert.truthy(toc:getChapterPagesDone(290) > 10)
     end)
     describe("collasible TOC", function()
         it("should collapse the secondary toc nodes by default", function()