From 1ac5846effded5f014ba0969f821ef5403a9ad6d Mon Sep 17 00:00:00 2001 From: NiLuJe Date: Sun, 27 Sep 2020 22:25:16 +0200 Subject: [PATCH] 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 | 9 +- .../apps/reader/modules/readerrolling.lua | 2 +- frontend/apps/reader/modules/readertoc.lua | 185 ++++++++++-------- frontend/apps/reader/skimtowidget.lua | 42 +--- spec/unit/readertoc_spec.lua | 35 ++-- 5 files changed, 131 insertions(+), 142 deletions(-) diff --git a/frontend/apps/reader/modules/readerfooter.lua b/frontend/apps/reader/modules/readerfooter.lua index 42516a37f..9d4cfeb7c 100644 --- a/frontend/apps/reader/modules/readerfooter.lua +++ b/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( - footer.pageno, footer.toc_level) + local left = footer.ui.toc:getChapterPagesLeft(footer.pageno) 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( - footer.pageno, footer.toc_level) + local left = footer.ui.toc:getChapterPagesLeft(footer.pageno) 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 diff --git a/frontend/apps/reader/modules/readerrolling.lua b/frontend/apps/reader/modules/readerrolling.lua index 2790c358f..b304d9ab9 100644 --- a/frontend/apps/reader/modules/readerrolling.lua +++ b/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 diff --git a/frontend/apps/reader/modules/readertoc.lua b/frontend/apps/reader/modules/readertoc.lua index 7d120879c..00d299e09 100644 --- a/frontend/apps/reader/modules/readertoc.lua +++ b/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 -positive level counts nodes of the depth level (level 1 for depth 1) -negative level counts nodes of reversed depth level (level -1 for max_depth) -zero level counts leaf nodes of the toc tree +The ToC ticks is a list of page numbers in ascending order of ToC nodes at a particular depth level. +A positive level returns nodes at that depth level (top-level is 1, depth always matches level. Higher values mean deeper nesting.) +A negative level does the same, but computes the depth level in reverse (i.e., -1 is the most deeply nested one). +Omitting the level argument returns the full hierarchical table. --]] function ReaderToc:getTocTicks(level) - if self.ticks[level] then return self.ticks[level] end - -- build toc ticks if not found + -- Handle negative levels + if level and level < 0 then + level = self:getMaxDepth() + level + 1 + end + + if level then + if self.ticks and self.ticks[level] then + return self.ticks[level] + end + else + if self.ticks then + return self.ticks + end + end + + -- Build ToC ticks if not found self:fillToc() - local ticks = {} + self.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 - end - else - local depth - if level > 0 then - depth = level - else - depth = self:getMaxDepth() + level + 1 - end - for _, v in ipairs(self.toc) do - if v.depth == depth then - table.insert(ticks, v.page) - end + -- Start by building a simple hierarchical ToC tick table + for _, v in ipairs(self.toc) do + if not self.ticks[v.depth] then + 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 -end -function ReaderToc:getTocTicksForFooter() - local ticks_candidates = {} - local max_level = self:getMaxDepth() - for i = 0, -max_level, -1 do - local ticks = self:getTocTicks(i) - table.insert(ticks_candidates, ticks) + if level then + return self.ticks[level] + else + return self.ticks 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 + +--[[ +Returns a flattened list of ToC ticks, without duplicates +]] +function ReaderToc:getTocTicksFlattened() + if self.ticks_flattened then return self.ticks_flattened end + + -- 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 + end end - return {} + + -- 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) - local ticks = self:getTocTicks(level) +function ReaderToc:getNextChapter(cur_pageno) + local ticks = self:getTocTicksFlattened() local next_chapter = nil - for i = 1, #ticks do - if ticks[i] > cur_pageno then - next_chapter = ticks[i] + for _, page in ipairs(ticks) do + if page > cur_pageno then + next_chapter = page break end end return next_chapter end -function ReaderToc:getPreviousChapter(cur_pageno, level) - local ticks = self:getTocTicks(level) +function ReaderToc:getPreviousChapter(cur_pageno) + local ticks = self:getTocTicksFlattened() local previous_chapter = nil - for i = 1, #ticks do - if ticks[i] >= cur_pageno then + for _, page in ipairs(ticks) do + 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) - local ticks = self:getTocTicks(level) +function ReaderToc:isChapterStart(cur_pageno) + local ticks = self:getTocTicksFlattened() local _start = false - for i = 1, #ticks do - if ticks[i] == cur_pageno then + for _, page in ipairs(ticks) do + 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) - local ticks = self:getTocTicks(level) +function ReaderToc:isChapterSecondPage(cur_pageno) + local ticks = self:getTocTicksFlattened() local _second = false - for i = 1, #ticks do - if ticks[i] + 1 == cur_pageno then + for _, page in ipairs(ticks) do + 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) - local ticks = self:getTocTicks(level) +function ReaderToc:isChapterEnd(cur_pageno) + local ticks = self:getTocTicksFlattened() local _end = false - for i = 1, #ticks do - if ticks[i] - 1 == cur_pageno then + for _, page in ipairs(ticks) do + 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) - --if self:isChapterEnd(pageno, level) then return 0 end - local next_chapter = self:getNextChapter(pageno, level) +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 end return next_chapter end -function ReaderToc:getChapterPagesDone(pageno, level) - if self:isChapterStart(pageno, level) then return 0 end - local previous_chapter = self:getPreviousChapter(pageno, level) +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 end diff --git a/frontend/apps/reader/skimtowidget.lua b/frontend/apps/reader/skimtowidget.lua index 7a926ee35..428b950de 100644 --- a/frontend/apps/reader/skimtowidget.lua +++ b/frontend/apps/reader/skimtowidget.lua @@ -71,19 +71,7 @@ function SkimToWidget:init() curr_page_display = self.ui.pagemap:getCurrentPageLabel(true) end - local ticks_candidates = {} - 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 + self.ticks_flattened = self.ui.toc:getTocTicksFlattened() 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 diff --git a/spec/unit/readertoc_spec.lua b/spec/unit/readertoc_spec.lua index aa839c356..e7b0b7614 100644 --- a/spec/unit/readertoc_spec.lua +++ b/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(100, 0) > 100) - assert.are.same(nil, toc:getNextChapter(290, 0)) + assert.truthy(toc:getNextChapter(10) > 10) + assert.truthy(toc:getNextChapter(100) > 100) + 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(100, 0) < 100) - assert.truthy(toc:getPreviousChapter(200, 0) < 200) + assert.truthy(toc:getPreviousChapter(10) < 10) + assert.truthy(toc:getPreviousChapter(100) < 100) + 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(95, 0) > 10) - assert.are.same(nil, toc:getChapterPagesLeft(290, 0)) + assert.truthy(toc:getChapterPagesLeft(10) > 10) + assert.truthy(toc:getChapterPagesLeft(95) > 10) + 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(88, 0) < 5) - assert.truthy(toc:getChapterPagesDone(290, 0) > 10) + assert.truthy(toc:getChapterPagesDone(11) < 5) + assert.truthy(toc:getChapterPagesDone(88) < 5) + assert.truthy(toc:getChapterPagesDone(290) > 10) end) describe("collasible TOC", function() it("should collapse the secondary toc nodes by default", function()