ReadHistory: refactoring with binary search (#9603)

Get rid of indexing and sorting, reduce flushing.

frontend/readhistory.lua

@@ -22,14 +22,12 @@ end
 
 local function buildEntry(input_time, input_file)
     local file_path = realpath(input_file) or input_file -- keep orig file path of deleted files
-    local file_exists = lfs.attributes(file_path, "mode") == "file"
     return {
         time = input_time,
-        text = input_file:gsub(".*/", ""),
         file = file_path,
-        dim = not file_exists, -- "dim", as expected by Menu
-        -- mandatory = file_exists and util.getFriendlySize(lfs.attributes(input_file, "size") or 0),
-        mandatory_func = function() -- Show the last read time (rather than file size)
+        text = input_file:gsub(".*/", ""),
+        dim = lfs.attributes(file_path, "mode") ~= "file", -- "dim", as expected by Menu
+        mandatory_func = function() -- Show the last read time
             local readerui_instance = require("apps/reader/readerui"):_getRunningInstance()
             local currently_opened_file = readerui_instance and readerui_instance.document and readerui_instance.document.file
             local last_read_ts
@@ -40,7 +38,7 @@ local function buildEntry(input_time, input_file)
             else
                 -- For past documents, the last save time of the settings is better
                 -- as last read time than input_time (its last opening time, that
-                -- we fallback to it no sidecar file)
+                -- we fallback to if no sidecar file)
                 last_read_ts = DocSettings:getLastSaveTime(file_path) or input_time
             end
             return util.secondsToDate(last_read_ts, G_reader_settings:isTrue("twelve_hour_clock"))
@@ -50,59 +48,61 @@ local function buildEntry(input_time, input_file)
         end,
         callback = function()
             selectCallback(input_file)
-        end
+        end,
     }
 end
 
-local function fileFirstOrdering(l, r)
-    if l.file == r.file then
-        return l.time > r.time
-    else
-        return l.file < r.file
-    end
-end
-
-local function timeFirstOrdering(l, r)
-    if l.time == r.time then
-        return l.file < r.file
-    else
-        return l.time > r.time
-    end
-end
-
-function ReadHistory:_indexing(start)
-    --- @todo (Hzj_jie): Use binary search to find an item when deleting it.
-    for i = start, #self.hist, 1 do
-        self.hist[i].index = i
-    end
-end
-
-function ReadHistory:_sort()
-    local autoremove_deleted_items_from_history =
-        not G_reader_settings:nilOrFalse("autoremove_deleted_items_from_history")
-    if autoremove_deleted_items_from_history then
-        self:clearMissing()
-    end
-    table.sort(self.hist, fileFirstOrdering)
-    --- @todo (zijiehe): Use binary insert instead of a loop to deduplicate.
-    for i = #self.hist, 2, -1 do
-        if self.hist[i].file == self.hist[i - 1].file then
-            table.remove(self.hist, i)
+function ReadHistory:getIndexByFile(item_file)
+    for i, v in ipairs(self.hist) do
+        if item_file == v.file then
+            return i
         end
     end
-    table.sort(self.hist, timeFirstOrdering)
-    self:_indexing(1)
 end
 
--- Reduces total count in hist list to a reasonable number by removing last
--- several items.
+--- Returns leftmost index of the entry with item_time using binary search
+-- (items in history are sorted by time in reverse order).
+-- If several entries have equal time, search within them by item_file in alphabetical order.
+-- If there are no entries with item_time, returns insertion index.
+function ReadHistory:getIndexByTime(item_time, item_file)
+    local hist_nb = #self.hist
+    if hist_nb == 0 then
+        return 1
+    end
+    if item_time > self.hist[1].time then
+        return 1
+    elseif item_time < self.hist[hist_nb].time then
+        return hist_nb + 1
+    end
+    local s, e, m, d = 1, hist_nb
+    while s <= e do
+        m = bit.rshift(s + e, 1)
+        if item_time < self.hist[m].time then
+            s, d = m + 1, 1
+        else
+            e, d = m - 1, 0
+        end
+    end
+    local index = m + d
+    if item_file then
+        while index <= #self.hist
+                and self.hist[index].time == item_time
+                and self.hist[index].file:gsub(".*/", "") < item_file do
+            index = index + 1
+        end
+    end
+    return index
+end
+
+--- Reduces number of history items to the required limit by removing old items.
 function ReadHistory:_reduce()
-    while #self.hist > 500 do
-        table.remove(self.hist, #self.hist)
+    local history_size = G_reader_settings:readSetting("history_size") or 500
+    while #self.hist > history_size do
+        table.remove(self.hist)
     end
 end
 
--- Flushes current history table into file.
+--- Saves history table to a file.
 function ReadHistory:_flush()
     local content = {}
     for _, v in ipairs(self.hist) do
@@ -123,23 +123,25 @@ end
 -- @treturn boolean true if the history_file has been updated and reloaded.
 function ReadHistory:_read(force_read)
     local history_file_modification_time = lfs.attributes(history_file, "modification")
-    if history_file_modification_time == nil
-            or (not force_read and (history_file_modification_time <= self.last_read_time)) then
-        return false
+    if history_file_modification_time == nil then -- no history_file, proceed legacy only
+        return true
     end
-    self.last_read_time = history_file_modification_time
-    local ok, data = pcall(dofile, history_file)
-    if ok and data then
-        self.hist = {}
-        for _, v in ipairs(data) do
-            table.insert(self.hist, buildEntry(v.time, v.file))
+    if force_read or (history_file_modification_time > self.last_read_time) then
+        self.last_read_time = history_file_modification_time
+        local ok, data = pcall(dofile, history_file)
+        if ok and data then
+            self.hist = {}
+            for _, v in ipairs(data) do
+                table.insert(self.hist, buildEntry(v.time, v.file))
+            end
         end
+        return true
     end
-    return true
 end
 
--- Reads history from legacy history folder
+--- Reads history from legacy history folder.
 function ReadHistory:_readLegacyHistory()
+    local history_updated
     local history_dir = DataStorage:getHistoryDir()
     for f in lfs.dir(history_dir) do
         local path = joinPath(history_dir, f)
@@ -148,14 +150,20 @@ function ReadHistory:_readLegacyHistory()
             if path ~= nil and path ~= "" then
                 local file = DocSettings:getNameFromHistory(f)
                 if file ~= nil and file ~= "" then
-                    table.insert(
-                        self.hist,
-                        buildEntry(lfs.attributes(joinPath(history_dir, f), "modification"),
-                                   joinPath(path, file)))
+                    local item_path = joinPath(path, file)
+                    local item_time = lfs.attributes(joinPath(history_dir, f), "modification")
+                    if self:addItem(item_path, item_time, true) then
+                        history_updated = true
+                    end
                 end
             end
         end
     end
+    if history_updated then
+        self:_reduce()
+        self:_flush()
+        self:ensureLastFile()
+    end
 end
 
 function ReadHistory:_init()
@@ -163,10 +171,10 @@ function ReadHistory:_init()
 end
 
 function ReadHistory:ensureLastFile()
-    local last_existing_file = nil
-    for i=1, #self.hist do
-        if lfs.attributes(self.hist[i].file, "mode") == "file" then
-            last_existing_file = self.hist[i].file
+    local last_existing_file
+    for _, v in ipairs(self.hist) do
+        if lfs.attributes(v.file, "mode") == "file" then
+            last_existing_file = v.file
             break
         end
     end
@@ -178,85 +186,42 @@ function ReadHistory:getLastFile()
     return G_reader_settings:readSetting("lastfile")
 end
 
+--- Get last or previous file in history that is not current_file
+-- (self.ui.document.file, provided as current_file, might have
+-- been removed from history).
 function ReadHistory:getPreviousFile(current_file)
-    -- Get last or previous file in history that is not current_file
-    -- (self.ui.document.file, probided as current_file, might have
-    -- been removed from history)
     if not current_file then
         current_file = G_reader_settings:readSetting("lastfile")
     end
-    for i=1, #self.hist do
+    for _, v in ipairs(self.hist) do
         -- skip current document and deleted items kept in history
-        local file = self.hist[i].file
-        if file ~= current_file and lfs.attributes(file, "mode") == "file" then
-            return file
+        if v.file ~= current_file and lfs.attributes(v.file, "mode") == "file" then
+            return v.file
         end
     end
 end
 
+--- Used in the BookShortcuts plugin.
 function ReadHistory:getFileByDirectory(directory, recursive)
     local real_path = realpath(directory)
-    for i=1, #self.hist do
-        local ipath = realpath(ffiutil.dirname(self.hist[i].file))
+    for _, v in ipairs(self.hist) do
+        local ipath = realpath(ffiutil.dirname(v.file))
         if ipath == real_path or (recursive and util.stringStartsWith(ipath, real_path)) then
-             return self.hist[i].file
+             return v.file
         end
     end
 end
 
-function ReadHistory:fileDeleted(path)
-    if G_reader_settings:isTrue("autoremove_deleted_items_from_history") then
-        self:removeItemByPath(path)
-    else
-        -- Make it dimed
-        for i=1, #self.hist do
-            if self.hist[i].file == path then
-                self.hist[i].dim = true
-                break
-            end
-        end
-        self:ensureLastFile()
-    end
-end
-
-function ReadHistory:fileSettingsPurged(path)
-    if G_reader_settings:isTrue("autoremove_deleted_items_from_history") then
-        -- Also remove it from history on purge when that setting is enabled
-        self:removeItemByPath(path)
-    end
-end
-
-function ReadHistory:clearMissing()
-    for i = #self.hist, 1, -1 do
-        if self.hist[i].file == nil or lfs.attributes(self.hist[i].file, "mode") ~= "file" then
-            self:removeItem(self.hist[i], i)
-        end
-    end
-    self:ensureLastFile()
-end
-
-function ReadHistory:removeItemByPath(path)
-    for i = #self.hist, 1, -1 do
-        if self.hist[i].file == path then
-            self:removeItem(self.hist[i])
-            break
-        end
-    end
-    self:ensureLastFile()
-end
-
 function ReadHistory:updateItemByPath(old_path, new_path)
-    for i = #self.hist, 1, -1 do
-        if self.hist[i].file == old_path then
-            self.hist[i].file = new_path
-            self.hist[i].text = new_path:gsub(".*/", "")
-            self:_flush()
-            self:reload(true)
-            self.hist[i].callback = function()
-                selectCallback(new_path)
-            end
-            break
+    local index = self:getIndexByFile(old_path)
+    if index then
+        self.hist[index].file = new_path
+        self.hist[index].text = new_path:gsub(".*/", "")
+        self.hist[index].callback = function()
+            selectCallback(new_path)
         end
+        self:_flush()
+        self:reload(true)
     end
     if G_reader_settings:readSetting("lastfile") == old_path then
         G_reader_settings:saveSetting("lastfile", new_path)
@@ -264,43 +229,97 @@ function ReadHistory:updateItemByPath(old_path, new_path)
     self:ensureLastFile()
 end
 
-function ReadHistory:removeItem(item, idx)
-    table.remove(self.hist, item.index or idx)
-    os.remove(DocSettings:getHistoryPath(item.file))
-    self:_indexing(item.index or idx)
-    self:_flush()
-    self:ensureLastFile()
-end
-
-function ReadHistory:addItem(file, ts)
-    if file ~= nil and lfs.attributes(file, "mode") == "file" then
-        local now = ts or os.time()
-        table.insert(self.hist, 1, buildEntry(now, file))
-        --- @todo (zijiehe): We do not need to sort if we can use binary insert and
-        -- binary search.
-        -- util.execute("/bin/touch", "-a", file)
-        -- This emulates `touch -a` in LuaFileSystem's API, since it may be absent (Android)
-        -- or provided by busybox, which doesn't support the `-a` flag.
-        local mtime = lfs.attributes(file, "modification")
-        lfs.touch(file, now, mtime)
-        self:_sort()
-        self:_reduce()
-        self:_flush()
-        G_reader_settings:saveSetting("lastfile", file)
+--- Updates the history list after deleting a file.
+function ReadHistory:fileDeleted(path)
+    local index = self:getIndexByFile(path)
+    if index then
+        if G_reader_settings:isTrue("autoremove_deleted_items_from_history") then
+            self:removeItem(self.hist[index], index)
+        else
+            self.hist[index].dim = true
+            self:ensureLastFile()
+        end
     end
 end
 
---- Reloads history from history_file.
--- @treturn boolean true if history_file has been updated and reload happened.
+--- Removes the history item if the document settings has been reset.
+function ReadHistory:fileSettingsPurged(path)
+    if G_reader_settings:isTrue("autoremove_deleted_items_from_history") then
+        -- Also remove it from history on purge when that setting is enabled
+        self:removeItemByPath(path)
+    end
+end
+
+--- Checks the history list for deleted files and removes history items respectively.
+function ReadHistory:clearMissing()
+    local history_updated
+    for i, v in ipairs(self.hist) do
+        if v.file == nil or lfs.attributes(v.file, "mode") ~= "file" then
+            self:removeItem(v, i, true) -- no flush
+            history_updated = true
+        end
+    end
+    if history_updated then
+        self:_flush()
+        self:ensureLastFile()
+    end
+end
+
+function ReadHistory:removeItemByPath(path)
+    local index = self:getIndexByFile(path)
+    if index then
+        self:removeItem(self.hist[index], index)
+    end
+end
+
+function ReadHistory:removeItem(item, idx, no_flush)
+    local index = idx or self:getIndexByTime(item.time, item.file:gsub(".*/", ""))
+    table.remove(self.hist, index)
+    os.remove(DocSettings:getHistoryPath(item.file))
+    if not no_flush then
+        self:_flush()
+        self:ensureLastFile()
+    end
+end
+
+--- Adds new item (last opened document) to the top of the history list.
+-- If item time (ts) is passed, add item to the history list at this time position.
+function ReadHistory:addItem(file, ts, no_flash)
+    if file ~= nil and lfs.attributes(file, "mode") == "file" then
+        local index = self:getIndexByFile(realpath(file))
+        if ts and index and self.hist[index].time == ts then
+            return -- this legacy item is in the history already
+        end
+        local now = ts or os.time()
+        local mtime = lfs.attributes(file, "modification")
+        lfs.touch(file, now, mtime)
+        if index == 1 and not ts then -- last book
+            self.hist[1].time = now
+        else -- old or new book
+            if index then -- old book
+                table.remove(self.hist, index)
+            end
+            index = ts and self:getIndexByTime(ts, file:gsub(".*/", "")) or 1
+            table.insert(self.hist, index, buildEntry(now, file))
+        end
+        if not no_flash then
+            self:_reduce()
+            self:_flush()
+            self:ensureLastFile()
+        end
+        return true -- used while adding legacy items
+    end
+end
+
+--- Reloads history from history_file and legacy history folder.
 function ReadHistory:reload(force_read)
     if self:_read(force_read) then
         self:_readLegacyHistory()
-        self:_sort()
+        if G_reader_settings:isTrue("autoremove_deleted_items_from_history") then
+            self:clearMissing()
+        end
         self:_reduce()
-        return true
     end
-
-    return false
 end
 
 ReadHistory:_init()

spec/unit/readhistory_spec.lua

@@ -43,7 +43,6 @@ describe("ReadHistory module", function()
 
     local function assert_item_is(h, i, name, fileRemoved)
         assert.is.same(name, h.hist[i].text)
-        assert.is.same(i, h.hist[i].index)
         assert.is.same(joinPath(realpath(test_data_dir()), name), h.hist[i].file)
         if fileRemoved then
             assert.is_nil(realpath(test_file(name)))
@@ -260,28 +259,6 @@ describe("ReadHistory module", function()
         rm(test_file("e"))
     end)
 
-    it("should remove duplicate entry", function()
-        rm(file("history.lua"))
-        touch(test_file("a"))
-        touch(test_file("b"))
-        local h = reload()
-        now = now + 61
-        h:addItem(test_file("b"), now)
-        now = now + 61
-        h:addItem(test_file("b"), now)
-        touch(legacy_history_file("a"))
-        now = now + 61
-        h:addItem(test_file("a"), now)  -- ensure a is before b
-        h = reload()
-        assert.is.same(2, #h.hist)
-        assert_item_is(h, 1, "a")
-        assert_item_is(h, 2, "b")
-
-        rm(legacy_history_file("a"))
-        rm(test_file("a"))
-        rm(test_file("b"))
-    end)
-
     it("should reduce the total count", function()
         local function to_file(i)
             return test_file(string.format("%04d", i))