Add readhistory_spec / docsettings_spec

frontend/docsettings.lua

@@ -36,12 +36,11 @@ end
 
 function DocSettings:getNameFromHistory(hist_name)
     if hist_name == nil or hist_name == '' then return nil end
-    hist_name = string.match(hist_name, "%b[]")
-    if hist_name == nil or hist_name == '' then return nil end
+    local s = string.match(hist_name, "%b[]")
+    if s == nil or s == '' then return nil end
     -- at first, search for path length
-    local s = string.len(hist_name)
     -- and return the rest of string without 4 last characters (".lua")
-    return string.sub(hist_name, s+2, -5)
+    return string.sub(hist_name, string.len(s)+2, -5)
 end
 
 function DocSettings:open(docfile)

frontend/readhistory.lua

@@ -1,6 +1,7 @@
 local lfs = require("libs/libkoreader-lfs")
 local DataStorage = require("datastorage")
 local DocSettings = require("docsettings")
+local realpath = require("ffi/util").realpath
 local joinPath = require("ffi/util").joinPath
 local dump = require("dump")
 
@@ -14,7 +15,7 @@ local function buildEntry(input_time, input_file)
     return {
         time = input_time,
         text = input_file:gsub(".*/", ""),
-        file = input_file,
+        file = realpath(input_file),
         callback = function()
             local ReaderUI = require("apps/reader/readerui")
             ReaderUI:showReader(input_file)
@@ -22,6 +23,22 @@ local function buildEntry(input_time, input_file)
     }
 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
@@ -31,22 +48,18 @@ end
 
 function ReadHistory:_sort()
     for i = #self.hist, 1, -1 do
-        if lfs.attributes(self.hist[i].file, "mode") ~= "file" then
+        if self.hist[i].file == nil or lfs.attributes(self.hist[i].file, "mode") ~= "file" then
             table.remove(self.hist, i)
         end
     end
-    table.sort(self.hist, function(l, r) return l.file < r.file 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
-            if self.hist[i].time < self.hist[i - 1].time then
-                table.remove(self.hist, i)
-            else
-                table.remove(self.hist,i - 1)
-            end
+            table.remove(self.hist, i)
         end
     end
-    table.sort(self.hist, function(v1, v2) return v1.time > v2.time end)
+    table.sort(self.hist, timeFirstOrdering)
     self:_indexing(1)
 end
 
@@ -75,7 +88,7 @@ end
 -- Reads history table from file
 function ReadHistory:_read()
     local ok, data = pcall(dofile, history_file)
-    if ok then
+    if ok and data then
         for k, v in pairs(data) do
             table.insert(self.hist, buildEntry(v.time, v.file))
         end
@@ -94,7 +107,7 @@ function ReadHistory:_readLegacyHistory()
                 if file ~= nil and file ~= "" then
                     table.insert(
                         self.hist,
-                        buildEntry(lfs.attributes(path, "modification"),
+                        buildEntry(lfs.attributes(joinPath(history_dir, f), "modification"),
                                    joinPath(path, file)))
                 end
             end

spec/unit/docsettings_spec.lua

@@ -1,11 +1,13 @@
 describe("docsettings module", function()
-    local docsettings, lfs
+    local docsettings, lfs, util
 
     setup(function()
         require("commonrequire")
         docsettings = require("docsettings")
         lfs = require("libs/libkoreader-lfs")
+        util = require("ffi/util")
     end)
+
     it("should generate sidecar directory path", function()
         assert.Equals("../../foo.sdr", docsettings:getSidecarDir("../../foo.pdf"))
         assert.Equals("/foo/bar.sdr", docsettings:getSidecarDir("/foo/bar.pdf"))
@@ -73,4 +75,12 @@ describe("docsettings module", function()
         d:close()
         d:purge()
     end)
+
+    it("should build correct legacy history path", function()
+        local file = "/a/b/c--d/c.txt"
+        local history_path = util.basename(docsettings:getHistoryPath(file))
+        local path_from_history = docsettings:getPathFromHistory(history_path)
+        local name_from_history = docsettings:getNameFromHistory(history_path)
+        assert.is.same(file, path_from_history .. "/" .. name_from_history)
+    end)
 end)

spec/unit/readhistory_spec.lua

@@ -0,0 +1,280 @@
+describe("ReadHistory module", function()
+    local DocSettings
+    local DataStorage
+    local mkdir
+    local realpath
+    local joinPath
+    local usleep
+    local history_file
+
+    local function file(name)
+        return joinPath(DataStorage:getDataDir(), name)
+    end
+
+    local function test_file(name)
+        return joinPath(joinPath(DataStorage:getDataDir(), "testdata"), name)
+    end
+
+    local function legacy_history_file(name)
+        return DocSettings:getHistoryPath(realpath(test_file(name)))
+    end
+
+    local function reload()
+        package.loaded["readhistory"] = nil
+        return require("readhistory")
+    end
+
+    local function rm(file)
+        os.remove(file)
+    end
+
+    local function touch(file)
+        local f = io.open(file, "w")
+        f:close()
+    end
+
+    local function assert_item_is(h, i, name)
+        assert.is.same(h.hist[i].text, name)
+        assert.is.same(h.hist[i].index, i)
+        assert.is.same(h.hist[i].file, realpath(test_file(name)))
+    end
+
+    setup(function()
+        require("commonrequire")
+        DocSettings = require("docsettings")
+        DataStorage = require("datastorage")
+        mkdir = require("libs/libkoreader-lfs").mkdir
+        realpath = require("ffi/util").realpath
+        joinPath = require("ffi/util").joinPath
+        usleep = require("ffi/util").usleep
+
+        mkdir(joinPath(DataStorage:getDataDir(), "testdata"))
+    end)
+
+    it("should read empty history.lua", function()
+        rm(file("history.lua"))
+        local h = reload()
+        assert.is.same(#h.hist, 0)
+        touch(file("history.lua"))
+        h = reload()
+        assert.is.same(#h.hist, 0)
+    end)
+
+    it("should read non-empty history.lua", function()
+        rm(file("history.lua"))
+        local h = reload()
+        touch(test_file("a"))
+        h:addItem(test_file("a"))
+        h = reload()
+        assert.is.same(#h.hist, 1)
+        assert_item_is(h, 1, "a")
+        rm(test_file("a"))
+    end)
+
+    it("should order legacy and history.lua", function()
+        rm(file("history.lua"))
+        touch(test_file("a"))
+        touch(test_file("b"))
+        local h = reload()
+        h:addItem(test_file("a"))
+        usleep(1000000)
+        touch(legacy_history_file("b"))
+        h = reload()
+        assert.is.same(#h.hist, 2)
+        assert_item_is(h, 1, "b")
+        assert_item_is(h, 2, "a")
+        rm(legacy_history_file("b"))
+        rm(test_file("a"))
+        rm(test_file("b"))
+    end)
+
+    it("should read legacy history folder", function()
+        rm(file("history.lua"))
+        touch(test_file("a"))
+        touch(test_file("b"))
+        touch(test_file("c"))
+        touch(test_file("d"))
+        touch(test_file("e"))
+        touch(test_file("f"))
+        local h = reload()
+        h:addItem(test_file("f"))
+        usleep(1000000)
+        touch(legacy_history_file("c"))
+        usleep(1000000)
+        touch(legacy_history_file("b"))
+        usleep(1000000)
+        h:addItem(test_file("d"))
+        usleep(1000000)
+        touch(legacy_history_file("a"))
+        usleep(1000000)
+        h:addItem(test_file("e"))
+        h = reload()
+        assert.is.same(#h.hist, 6)
+        assert_item_is(h, 1, "e")
+        assert_item_is(h, 2, "a")
+        assert_item_is(h, 3, "d")
+        assert_item_is(h, 4, "b")
+        assert_item_is(h, 5, "c")
+        assert_item_is(h, 6, "f")
+
+        rm(legacy_history_file("c"))
+        rm(legacy_history_file("b"))
+        rm(legacy_history_file("a"))
+        rm(test_file("a"))
+        rm(test_file("b"))
+        rm(test_file("c"))
+        rm(test_file("d"))
+        rm(test_file("e"))
+        rm(test_file("f"))
+    end)
+
+    it("should add item", function()
+        rm(file("history.lua"))
+        touch(test_file("a"))
+        local h = reload()
+        h:addItem(test_file("a"))
+        assert.is.same(#h.hist, 1)
+        assert_item_is(h, 1, "a")
+        rm(test_file("a"))
+    end)
+
+    it("should be able to remove the first item", function()
+        rm(file("history.lua"))
+        touch(test_file("a"))
+        touch(test_file("b"))
+        touch(test_file("c"))
+        local h = reload()
+        h:addItem(test_file("a"))
+        h:addItem(test_file("b"))
+        h:addItem(test_file("c"))
+        h:removeItem(h.hist[1])
+        assert_item_is(h, 1, "b")
+        assert_item_is(h, 2, "c")
+        h:removeItem(h.hist[1])
+        assert_item_is(h, 1, "c")
+        rm(test_file("a"))
+        rm(test_file("b"))
+        rm(test_file("c"))
+    end)
+
+    it("should be able to remove an item in the middle", function()
+        rm(file("history.lua"))
+        touch(test_file("a"))
+        touch(test_file("b"))
+        touch(test_file("c"))
+        local h = reload()
+        h:addItem(test_file("a"))
+        h:addItem(test_file("b"))
+        h:addItem(test_file("c"))
+        h:removeItem(h.hist[2])
+        assert_item_is(h, 1, "a")
+        assert_item_is(h, 2, "c")
+        rm(test_file("a"))
+        rm(test_file("b"))
+        rm(test_file("c"))
+    end)
+
+    it("should be able to remove the last item", function()
+        rm(file("history.lua"))
+        touch(test_file("a"))
+        touch(test_file("b"))
+        touch(test_file("c"))
+        local h = reload()
+        h:addItem(test_file("a"))
+        h:addItem(test_file("b"))
+        h:addItem(test_file("c"))
+        h:removeItem(h.hist[3])
+        assert_item_is(h, 1, "a")
+        assert_item_is(h, 2, "b")
+        h:removeItem(h.hist[2])
+        assert_item_is(h, 1, "a")
+        rm(test_file("a"))
+        rm(test_file("b"))
+        rm(test_file("c"))
+    end)
+
+    it("should be able to remove two items", function()
+        rm(file("history.lua"))
+        touch(test_file("a"))
+        touch(test_file("b"))
+        touch(test_file("c"))
+        touch(test_file("d"))
+        local h = reload()
+        h:addItem(test_file("a"))
+        h:addItem(test_file("b"))
+        h:addItem(test_file("c"))
+        h:addItem(test_file("d"))
+        h:removeItem(h.hist[3])  -- remove c
+        h:removeItem(h.hist[2])  -- remove b
+        assert_item_is(h, 1, "a")
+        assert_item_is(h, 2, "d")
+        rm(test_file("a"))
+        rm(test_file("b"))
+        rm(test_file("c"))
+        rm(test_file("d"))
+    end)
+
+    it("should be able to remove three items", function()
+        rm(file("history.lua"))
+        touch(test_file("a"))
+        touch(test_file("b"))
+        touch(test_file("c"))
+        touch(test_file("d"))
+        touch(test_file("e"))
+        local h = reload()
+        h:addItem(test_file("a"))
+        h:addItem(test_file("b"))
+        h:addItem(test_file("c"))
+        h:addItem(test_file("d"))
+        h:addItem(test_file("e"))
+        h:removeItem(h.hist[2])  -- remove b
+        h:removeItem(h.hist[2])  -- remove c
+        h:removeItem(h.hist[3])  -- remove e
+        assert_item_is(h, 1, "a")
+        assert_item_is(h, 2, "d")
+        rm(test_file("a"))
+        rm(test_file("b"))
+        rm(test_file("c"))
+        rm(test_file("d"))
+        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()
+        h:addItem(test_file("b"))
+        h:addItem(test_file("b"))
+        touch(legacy_history_file("a"))
+        h:addItem(test_file("a"))  -- ensure a is before b
+        h = reload()
+        assert.is.same(#h.hist, 2)
+        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))
+        end
+        rm(file("history.lua"))
+        local h = reload()
+        for i = 1000, 1, -1 do
+            touch(to_file(i))
+            h:addItem(to_file(i))
+        end
+
+        for i = 1, 500 do  -- at most 500 items are stored
+            assert_item_is(h, i, string.format("%04d", i))
+        end
+        
+        for i = 1, 1000 do
+            rm(to_file(i))
+        end
+    end)
+end)