ReaderStatistics: Data collection improvements (#6778)

* Update the data collection format & handler to make it much less tortuous
* Update the pagecount & resync the stats on document layout changes
* Update the database schema to allow doing most queries against a SQL view that rescales the collected data to be accurate regardless of document layout (thanks to @marek-g for the SQL magic ;)).
* Add a "reset stats for current book" entry in the list of reset options, one that won't horribly break stats in said book ;).
* Fixed a couple of resource (SQL connection) leaks (in ReaderStatistics:getCurrentBookStats & ReaderStatistics:getCurrentBookStats).
* Flush stats to the DB on periodical metadata saves.
* Minor cosmetic tweaks to the code

base

@@ -1 +1 @@
-Subproject commit 8661294d0ebc9e3cacb8dbbf6175de7a1294d23c
+Subproject commit 32af1b85073a9183a9235fa2ebbe67799714fe46

frontend/apps/reader/modules/readerfooter.lua

@@ -1723,7 +1723,7 @@ function ReaderFooter:getAvgTimePerPage()
 end
 
 function ReaderFooter:getDataFromStatistics(title, pages)
-    local sec = 'na'
+    local sec = "N/A"
     local average_time_per_page = self:getAvgTimePerPage()
     if average_time_per_page then
         if self.settings.duration_format == "classic" then

frontend/apps/reader/modules/readertoc.lua

@@ -62,7 +62,9 @@ end
 
 function ReaderToc:onUpdateToc()
     self:resetToc()
-    return true
+
+    --- @note: Let this propagate, plugins/statistics uses it to react to changes in document pagination
+    --return true
 end
 
 function ReaderToc:onPageUpdate(pageno)

frontend/ui/widget/bookstatuswidget.lua

@@ -21,7 +21,6 @@ local RenderImage = require("ui/renderimage")
 local Size = require("ui/size")
 local TextBoxWidget = require("ui/widget/textboxwidget")
 local TextWidget = require("ui/widget/textwidget")
-local TimeVal = require("ui/timeval")
 local ToggleSwitch = require("ui/widget/toggleswitch")
 local UIManager = require("ui/uimanager")
 local VerticalGroup = require("ui/widget/verticalgroup")
@@ -231,9 +230,8 @@ function BookStatusWidget:genHeader(title)
 end
 
 function BookStatusWidget:onChangeBookStatus(option_name, option_value)
-    local curr_time = TimeVal:now()
     self.summary.status = option_name[option_value]
-    self.summary.modified = os.date("%Y-%m-%d", curr_time.sec)
+    self.summary.modified = os.date("%Y-%m-%d", os.time())
     self:saveSummary()
     return true
 end

plugins/coverbrowser.koplugin/bookinfomanager.lua

@@ -108,10 +108,10 @@ end
 -- Build our most often used SQL queries according to columns
 local BOOKINFO_INSERT_SQL = "INSERT OR REPLACE INTO bookinfo " ..
                             "(" .. table.concat(BOOKINFO_COLS_SET, ",") .. ") " ..
-                            "VALUES (" .. table.concat(bookinfo_values_sql, ",") .. ")"
+                            "VALUES (" .. table.concat(bookinfo_values_sql, ",") .. ");"
 local BOOKINFO_SELECT_SQL = "SELECT " .. table.concat(BOOKINFO_COLS_SET, ",") .. " FROM bookinfo " ..
-                            "WHERE directory=? and filename=? and in_progress=0"
-local BOOKINFO_IN_PROGRESS_SQL = "SELECT in_progress, filename, unsupported FROM bookinfo WHERE directory=? and filename=?"
+                            "WHERE directory=? AND filename=? AND in_progress=0;"
+local BOOKINFO_IN_PROGRESS_SQL = "SELECT in_progress, filename, unsupported FROM bookinfo WHERE directory=? AND filename=?;"
 
 
 local BookInfoManager = {}
@@ -173,7 +173,7 @@ function BookInfoManager:openDbConnection()
         self:createDB()
     end
     self.db_conn = SQ3.open(self.db_location)
-    xutil.sqlite_set_timeout(self.db_conn, 5000) -- 5 seconds
+    self.db_conn:set_busy_timeout(5000) -- 5 seconds
 
     -- Prepare our most often used SQL statements
     self.set_stmt = self.db_conn:prepare(BOOKINFO_INSERT_SQL)
@@ -203,10 +203,10 @@ function BookInfoManager:compactDb()
     -- is bigger than available memory...)
     local prev_size = self:getDbSize()
     self:openDbConnection()
-    self.db_conn:exec("PRAGMA temp_store = 2") -- use memory for temp files
+    self.db_conn:exec("PRAGMA temp_store = 2;") -- use memory for temp files
     -- self.db_conn:exec("VACUUM")
     -- Catch possible "memory or disk is full" error
-    local ok, errmsg = pcall(self.db_conn.exec, self.db_conn, "VACUUM") -- this may take some time
+    local ok, errmsg = pcall(self.db_conn.exec, self.db_conn, "VACUUM;") -- this may take some time
     self:closeDbConnection()
     if not ok then
         return T(_("Failed compacting database: %1"), errmsg)
@@ -224,7 +224,7 @@ function BookInfoManager:loadSettings()
     end
     self.settings = {}
     self:openDbConnection()
-    local res = self.db_conn:exec("SELECT key, value FROM config")
+    local res = self.db_conn:exec("SELECT key, value FROM config;")
     local keys = res[1]
     local values = res[2]
     for i, key in ipairs(keys) do
@@ -247,7 +247,7 @@ function BookInfoManager:saveSetting(key, value)
         end
     end
     self:openDbConnection()
-    local query = "INSERT OR REPLACE INTO config (key, value) VALUES (?, ?)"
+    local query = "INSERT OR REPLACE INTO config (key, value) VALUES (?, ?);"
     local stmt = self.db_conn:prepare(query)
     if value == false then -- convert false to NULL
         value = nil
@@ -486,7 +486,7 @@ function BookInfoManager:setBookInfoProperties(filepath, props)
     self:openDbConnection()
     -- Let's do multiple one-column UPDATE (easier than building
     -- a multiple columns UPDATE)
-    local base_query = "UPDATE bookinfo SET %s=? WHERE directory=? AND filename=?"
+    local base_query = "UPDATE bookinfo SET %s=? WHERE directory=? AND filename=?;"
     for k, v in pairs(props) do
         local this_prop_query = string.format(base_query, k) -- add column name to query
         local stmt = self.db_conn:prepare(this_prop_query)
@@ -502,7 +502,7 @@ end
 function BookInfoManager:deleteBookInfo(filepath)
     local directory, filename = util.splitFilePathName(filepath)
     self:openDbConnection()
-    local query = "DELETE FROM bookinfo WHERE directory=? AND filename=?"
+    local query = "DELETE FROM bookinfo WHERE directory=? AND filename=?;"
     local stmt = self.db_conn:prepare(query)
     stmt:bind(directory, filename)
     stmt:step() -- commited
@@ -511,7 +511,7 @@ end
 
 function BookInfoManager:removeNonExistantEntries()
     self:openDbConnection()
-    local res = self.db_conn:exec("SELECT bcid, directory || filename FROM bookinfo")
+    local res = self.db_conn:exec("SELECT bcid, directory || filename FROM bookinfo;")
     if not res then
         return _("Cache is empty. Nothing to prune.")
     end
@@ -523,7 +523,7 @@ function BookInfoManager:removeNonExistantEntries()
             table.insert(bcids_to_remove, tonumber(bcids[i]))
         end
     end
-    local query = "DELETE FROM bookinfo WHERE bcid=?"
+    local query = "DELETE FROM bookinfo WHERE bcid=?;"
     local stmt = self.db_conn:prepare(query)
     for i=1, #bcids_to_remove do
         stmt:bind(bcids_to_remove[i])

plugins/coverbrowser.koplugin/xutil.lua

@@ -32,17 +32,4 @@ function xutil.zlib_uncompress(zdata, datalen)
     return ffi.string(buf, buflen[0])
 end
 
--- Not provided by base/thirdparty/lua-ljsqlite3/init.lua
--- Add a timeout to a lua-ljsqlite3 connection
--- We need that if we have multiple processes accessing the same
--- SQLite db for reading or writting (read lock and write lock can't be
--- obtained at the same time, so waiting & retry is needed)
--- SQLite will retry getting a lock every 1ms to 100ms for
--- the timeout_ms given here
-local sql = ffi.load("sqlite3")
-function xutil.sqlite_set_timeout(conn, timeout_ms)
-    sql.sqlite3_busy_timeout(conn._ptr, timeout_ms)
-end
--- For reference, SQ3 doc at: http://scilua.org/ljsqlite3.html
-
 return xutil

plugins/statistics.koplugin/readerprogress.lua

@@ -339,7 +339,7 @@ function ReaderProgress:genSummaryDay(width)
         CenterContainer:new{
             dimen = Geom:new{ w = tile_width, h = tile_height },
             TextWidget:new{
-                text = util.secondsToClock(self.current_period, true),
+                text = util.secondsToClock(self.current_duration, true),
                 face = self.medium_font_face,
             },
         },
@@ -353,7 +353,7 @@ function ReaderProgress:genSummaryDay(width)
         CenterContainer:new{
             dimen = Geom:new{ w = tile_width, h = tile_height },
             TextWidget:new{
-                text = util.secondsToClock(self.today_period, true),
+                text = util.secondsToClock(self.today_duration, true),
                 face = self.medium_font_face,
             },
         },

spec/unit/readerfooter_spec.lua

@@ -64,6 +64,11 @@ describe("Readerfooter module", function()
             book_time_to_read = true,
             chapter_time_to_read = true,
         })
+        -- NOTE: Forcefully disable the statistics plugin, as lj-sqlite3 is horribly broken under Busted,
+        --       causing it to erratically fail to load, affecting the results of this test...
+        G_reader_settings:saveSetting("plugins_disabled", {
+            statistics = true,
+        })
         UIManager:run()
     end)
 
@@ -173,8 +178,8 @@ describe("Readerfooter module", function()
         footer:onUpdateFooter()
         local timeinfo = footer.textGeneratorMap.time(footer)
         local page_count = readerui.document:getPageCount()
-        -- stats has not been initialized here, so we get na TB and TC
-        assert.are.same('1 / '..page_count..' | '..timeinfo..' | ⇒ 0 | 0% | ⤠ 0% | ⏳ na | ⤻ na',
+        -- c.f., NOTE above, Statistics are disabled, hence the N/A results
+        assert.are.same('1 / '..page_count..' | '..timeinfo..' | ⇒ 0 | 0% | ⤠ 0% | ⏳ N/A | ⤻ N/A',
                         footer.footer_text.text)
     end)
 
@@ -190,7 +195,7 @@ describe("Readerfooter module", function()
         local footer = readerui.view.footer
         readerui.view.footer:onUpdateFooter()
         local timeinfo = readerui.view.footer.textGeneratorMap.time(footer)
-        assert.are.same('1 / 2 | '..timeinfo..' | ⇒ 1 | 0% | ⤠ 50% | ⏳ na | ⤻ na',
+        assert.are.same('1 / 2 | '..timeinfo..' | ⇒ 1 | 0% | ⤠ 50% | ⏳ N/A | ⤻ N/A',
                         readerui.view.footer.footer_text.text)
     end)
 
@@ -209,7 +214,7 @@ describe("Readerfooter module", function()
         footer:resetLayout()
         footer:onUpdateFooter()
         local timeinfo = footer.textGeneratorMap.time(footer)
-        assert.are.same('1 / 2 | '..timeinfo..' | ⇒ 1 | 0% | ⤠ 50% | ⏳ na | ⤻ na',
+        assert.are.same('1 / 2 | '..timeinfo..' | ⇒ 1 | 0% | ⤠ 50% | ⏳ N/A | ⤻ N/A',
                         footer.footer_text.text)
 
         -- disable show all at once, page progress should be on the first
@@ -234,11 +239,11 @@ describe("Readerfooter module", function()
 
         -- disable percentage, book time to read should follow
         tapFooterMenu(fake_menu, "Progress percentage".." (⤠)")
-        assert.are.same('⏳ na', footer.footer_text.text)
+        assert.are.same('⏳ N/A', footer.footer_text.text)
 
         -- disable book time to read, chapter time to read should follow
         tapFooterMenu(fake_menu, "Book time to read".." (⏳)")
-        assert.are.same('⤻ na', footer.footer_text.text)
+        assert.are.same('⤻ N/A', footer.footer_text.text)
 
         -- disable chapter time to read, text should be empty
         tapFooterMenu(fake_menu, "Chapter time to read".." (⤻)")
@@ -246,7 +251,7 @@ describe("Readerfooter module", function()
 
         -- reenable chapter time to read, text should be chapter time to read
         tapFooterMenu(fake_menu, "Chapter time to read".." (⤻)")
-        assert.are.same('⤻ na', footer.footer_text.text)
+        assert.are.same('⤻ N/A', footer.footer_text.text)
     end)
 
     it("should rotate through different modes", function()
@@ -300,20 +305,20 @@ describe("Readerfooter module", function()
         local footer = readerui.view.footer
         local horizontal_margin = Screen:scaleBySize(10)*2
         footer:onUpdateFooter()
-        assert.is.same(354, footer.text_width)
+        assert.is.same(370, footer.text_width)
         assert.is.same(600, footer.progress_bar.width
                             + footer.text_width
                             + horizontal_margin)
-        assert.is.same(226, footer.progress_bar.width)
+        assert.is.same(210, footer.progress_bar.width)
 
         local old_screen_getwidth = Screen.getWidth
         Screen.getWidth = function() return 900 end
         footer:resetLayout()
-        assert.is.same(354, footer.text_width)
+        assert.is.same(370, footer.text_width)
         assert.is.same(900, footer.progress_bar.width
                             + footer.text_width
                             + horizontal_margin)
-        assert.is.same(526, footer.progress_bar.width)
+        assert.is.same(510, footer.progress_bar.width)
         Screen.getWidth = old_screen_getwidth
     end)
 
@@ -328,12 +333,12 @@ describe("Readerfooter module", function()
         }
         local footer = readerui.view.footer
         footer:onPageUpdate(1)
-        assert.are.same(218, footer.progress_bar.width)
-        assert.are.same(362, footer.text_width)
+        assert.are.same(202, footer.progress_bar.width)
+        assert.are.same(378, footer.text_width)
 
         footer:onPageUpdate(100)
-        assert.are.same(194, footer.progress_bar.width)
-        assert.are.same(386, footer.text_width)
+        assert.are.same(178, footer.progress_bar.width)
+        assert.are.same(402, footer.text_width)
     end)
 
     it("should support chapter markers", function()