Finished books: freeze history timestamp and statistics #10968

hius07 · 2023-10-04T09:53:16Z

Separate settings for History and Statistics.
If enabled in History settings: after opening the finished book will not pop up in History, will not be saved as Last document, will not pop up in the file browser with Sort by last read date.
Doesn't depend on the way of opening (from FM, History, Collections, random book etc.).
Closes #10962.
EDIT: closes #10984.

This change is

poire-z · 2023-10-04T11:37:15Z

Dunno if it is just me and my bad English, but "Preserve" sounds ambiguous to me.
I would understand it as "keep", and at first sight: "keep doing" - and not "prevent doing/updating/accounting".
I don't have non-too-verbose suggestions :/

Also, "Preserve (or whatever) finished book timestamp" may feel too technical and a bit vague about what this does. If we don't find a better wording, it needs some help_text =.

mergen3107 · 2023-10-04T11:39:52Z

How about “Freeze timestamp” and “Stop statistics”?

Frenzie · 2023-10-04T11:54:49Z

Freeze sounds good to me.

poire-z · 2023-10-04T11:59:07Z

plugins/statistics.koplugin/main.lua

+function ReaderStatistics:isDocless(preserve_finished_books)
    return self.ui == nil or self.ui.document == nil or self.ui.document.is_pic == true
+        or (preserve_finished_books and self.settings.preserve_finished_books
+            and self.ui.doc_settings:readSetting("summary").status == "complete")


Feels a bit like a quick hack :)
(and ugly in the last chunk of this PR:

if self:isDocless() or not self.settings.is_enabled then return end if not self:isDocless(true) then...

)
May be the isDocLess() stuff should be remade so it's just a flag, set in :init() or onReadSettings(), and not this whole bunch of checks each time we onPageUpdate.

Book status can be changed at any moment.

OK, so, either:

you don't care about a change of that status while the book is opened, and keep using its state when it was opened (I'm totally fine with that)

you send a new Event("BookStatusChanged") and an handler in statistics (is it really worth the work? :)

And what is the profit?
readSetting costs nothing.
All isDocless(true) calls will remain in place.

costs nothing, but still, a few checks that could be avoided.
return self.ui == nil or self.ui.document == nil or self.ui.document.is_pic == true ended up like that as we added these checks, and were lazy.
Now it gets added: or (preserve_finished_books and self.settings.preserve_finished_books and self.ui.doc_settings:readSetting("summary").status == "complete")
And It's not any longer true to its name: isDocLess() :)

So, either we keep adding unmeaningful stuff to sometihng related to "Document Less usage", or we rename it to isReallyEnabled() or shouldDoAccounting(), and if we do that, we can as well make it cleaner.

I wanted to avoid reading all 3 thousands lines, but it is funny from the beginning

koreader/plugins/statistics.koplugin/main.lua

Lines 104 to 108 in 5b5b4d9

function ReaderStatistics:init()

-- Disable in PIC documents (but not the FM, as we want to be registered to the FM's menu).

if self.ui and self.ui.document and self.ui.document.is_pic then

return

end

Yeah, not necessarily a massive fan of the approach, but agree that the current situation isn't great either.

I was thinking of something like what KOSync (or... something, a few things do that) does, where the handler methods are not enabled by default (i.e., they're declared as Module:_onStuffHappening), and we have a pair of register/unregisterEventHandlers methods that either set self.onStuffHappening to nil (unregister) or self._onStuffHappening (register).

This would allow dropping a bit of cruft from that crappy check, and avoid running the handlers at all when disabled.

hius07 · 2023-10-04T15:08:03Z

BTW, about "preserve"

koreader/plugins/statistics.koplugin/main.lua

Lines 262 to 263 in 5b5b4d9

    
           function ReaderStatistics:onPreserveCurrentSession() 
        
               -- Can be called before ReaderUI:reloadDocument() to not reset the current session

I'm okay with "Freeze".

NiLuJe · 2023-10-04T15:12:24Z

Have no issue with preserve myself, but freeze works for me, too ;).

poire-z · 2023-10-04T15:22:15Z

BTW, about "preserve"

koreader/plugins/statistics.koplugin/main.lua

Lines 262 to 263 in 5b5b4d9

function ReaderStatistics:onPreserveCurrentSession()

-- Can be called before ReaderUI:reloadDocument() to not reset the current session

It's me who did name that function that way, and I meant it my naive way: "save and keep reusing it" (and not prevent/freeze). Anyway, it's some internal name, not exposed to users.

I'm okay with "Freeze".

With that "Freeze" too.
Work for statistics, but dunno how we can use "Freeze" for the history/timestamp part.

mergen3107 · 2023-10-04T15:56:17Z

Fixate timestamp?

NiLuJe · 2023-10-04T16:49:57Z

Fixate timestamp?

For context on the reaction, that sounds extremely clunky ;).

mergen3107 · 2023-10-04T17:10:06Z

Understood 😅

hius07 · 2023-10-05T08:21:29Z

Changing status of the currently opened book will not freeze the statistics.

Do we need a marker in the Current statistics table indicating the book is frozen?

poire-z · 2023-10-05T10:56:54Z

Do we need a marker in the Current statistics table indicating the book is frozen?

Dunno, doesn't feel really needed.
But I see there is:
Estimated finish date (i) (in 4 days) Mon Oct 09 2023
which doesn't make a lot of sense if the book is finished - so may be it can be replaced with:
Book marked as finished (statistics no longer accounted) ?

hius07 · 2023-10-05T11:29:39Z

poire-z

Thanks, looks better to me this way.

poire-z · 2023-10-05T11:27:46Z

plugins/statistics.koplugin/main.lua

+                            self.is_doc_not_freezed = self.is_doc
+                                and (self.is_doc_not_finished or self.settings.freeze_finished_books)
+                            self.settings.freeze_finished_books = not self.settings.freeze_finished_books


Would read easier if you swap the order of these 2 lines (currently, the first line use the previous value of self.settings.freeze_finished_books - I was going to review and say "... or not self.settings.freeze_finished_books")

That was the first attempt, then swapped to save one not.

Clarity/readability/obviousness is better than convoluted optimisation :)

poire-z · 2023-10-05T11:35:33Z

Viewing the above screenshot, I would expect to see the date that the book was marked as finished (which, if frozen, might be date of the last inserted statistics) on the right of "Book marked as finished" :/

Frenzie · 2023-10-05T11:48:22Z

Just "statistics frozen" or "statistics are frozen" btw (not is!) ^_^

hius07 · 2023-10-05T12:18:41Z

The actual date of changing status is doc_settings.summary.modified.
Should be converted to seconds (our datetime library doesn't provide conversion from string to epoch).

NiLuJe · 2023-10-05T14:52:01Z

plugins/statistics.koplugin/main.lua

+    return self.settings.is_enabled and self.is_doc
+end
+
+function ReaderStatistics:isEnabledNotFreezed()


Nit: Frozen ;). (ditto for the variable)

(I'd possibly add an And in the middle there, too).

NiLuJe · 2023-10-05T14:54:15Z

plugins/statistics.koplugin/main.lua

@@ -1123,6 +1126,15 @@ The max value ensures a page you stay on for a long time (because you fell aslee
                            UIManager:show(durations_widget)
                        end,
                        keep_menu_open = true,
+                    },
+                    {
+                        text = _("Freeze finished books statistics"),


Freeze statistics of finished books sounds ever so slightly better to me, RFC?

NiLuJe · 2023-10-05T14:55:19Z

plugins/statistics.koplugin/main.lua

 end

 function ReaderStatistics:onReadSettings(config)
-    self.data = config:readSetting("stats", {})
+    self.data = config:readSetting("stats", { performance_in_pages= {} })


nit: weird spacing around the = there ;)

poire-z · 2023-10-05T15:08:23Z

frontend/datetime.lua

+        datetime_string = datetime_string .. " 00:00:00"
+    end
+    local year, month, day, hour, min, sec = datetime_string:match("(%d+)-(%d+)-(%d+) (%d+):(%d+):(%d+)")
+    local dateTimeTable = {


Our coding style advises thie kind of casing for methods/functions.
So, best to use underscore for these kind of variable name, ie. datetime_table or datetime_t or d or t (it is used just after, no real need to have an explicite name - you could even inline it inside os.time(...).

poire-z · 2023-10-05T15:20:29Z

plugins/statistics.koplugin/main.lua

+        estimated_time_left = { _("Book marked as finished"), _("statistics frozen") }
+        local mark_date = self.ui.doc_settings:readSetting("summary").modified
+        estimated_finish_date = { _("Book marked as finished"), datetime.secondsToDate(datetime.stringToSeconds(mark_date), true) }


I'd like to have a different wording for the first one that replaces "Estimated reading time left" and indicates that statistics are frozen.
(Having twice the same key, and additionally each having different values, feels quite odd.)
Not sure about what to write. May be Reading finished statistics frozen (or Reading marked as finished statistics frozen but the short one feels more different and better) .

I have a mild preference over the ever so slightly more explicit Book marked as finished he went with, Reading finished doesn't necessarily imply that the book was actually marked as finished, it could be construed as "oh, that happened on its own because I reached the last page" or something ;).

(I mean, you can have books auto-flag as finished on EoB, but that's non-default ;p).

So, what about the other one I suggested Reading marked as finished statistics frozen ?
(My wish is just to not have twice the same key, and additionally each having different values.)

Oh, I'd... possibly approach this another way: don't change the key, just the value? The whole thing is opt-in, so I don't think we need to re-explain why statistics are frozen?

Wait, I may be confused; lemme check the full context, as I don't rightly recall how this thing even looks like to begin with ;).

Okay, yeah, I would keep estimated_time_left's description as-is (i.e., Estimated reading time left), and just change the "value" to mention something like N/A and/or stats are frozen.

I do like the change from Estimated finish date to Book marked as finished date, but this one is technically semantically different for both its description and the value, so it makes sense ;).

hius07 · 2023-10-06T06:36:28Z

"Freeze statistics" not checked.

"Freeze statistics" checked.

poire-z

Screenshots, wording and code looks good to me.

poire-z · 2023-10-07T07:30:15Z

plugins/statistics.koplugin/main.lua

-    Dispatcher:registerAction("book_statistics", {category="none", event="ShowBookStats", title=_("Book statistics"), reader=true, separator=false})
-    Dispatcher:registerAction("stats_sync", {category="none", event="SyncBookStats", title=_("Synchronize book statistics"), reader=true, separator=true})
+    Dispatcher:registerAction("stats_calendar_view", {category="none", event="ShowCalendarView", title=_("Statistics calendar view"), general=true})
+    Dispatcher:registerAction("stats_calendar_day_view", {category="none", event="ShowCalendarDayView", title=_("Statistics today's timeline"), general=true})
+    Dispatcher:registerAction("stats_sync", {category="none", event="SyncBookStats", title=_("Synchronize book statistics"), general=true, separator=true})
+    Dispatcher:registerAction("book_statistics", {category="none", event="ShowBookStats", title=_("Book statistics"), reader=true})


Does the order of registerAction matter ?
Does it impact the order in which they appear in gestures action?
Feels like I'd want all "view stats" item first, and "sync stats" last.

"View stats" is in the Reader section, others - in General.

Too late to avoid the whitespace diff churn, unfortunately ;).

hius07 added 3 commits October 4, 2023 12:44

readhistory: preserve finished books

c66076a

filemanagermenu: preserve finished books

8476412

statistics: preserve finished books

7b35eb6

poire-z reviewed Oct 4, 2023

View reviewed changes

Statistics: update flags

56646c2

Statistics: optimize

ea62184

poire-z reviewed Oct 5, 2023

View reviewed changes

hius07 added 2 commits October 5, 2023 17:32

datetime: add stringToSeconds()

9f7b7ae

statistics: indicate frozen books

10fa648

NiLuJe reviewed Oct 5, 2023

View reviewed changes

poire-z reviewed Oct 5, 2023

View reviewed changes

filemanagermenu: "freeze"

a77aff7

hius07 added 3 commits October 6, 2023 09:32

readhistory: "freeze"

e368612

datetime: stringToSeconds

f4cf8f2

statistics: fixes, cleanup, wordings

d96c059

poire-z approved these changes Oct 6, 2023

View reviewed changes

Statistics: fix Dispatcher/QM appearance

28f069e

poire-z reviewed Oct 7, 2023

View reviewed changes

Statistics: ButtonDialogTitle -> ButtonDialog

f397047

hius07 changed the title ~~Finished books: preserve history timestamp and statistics~~ Finished books: freeze history timestamp and statistics Oct 9, 2023

hius07 merged commit 517731d into koreader:master Oct 9, 2023

hius07 deleted the preserve-finished-books branch October 9, 2023 04:45

hius07 added this to the 2023.10 milestone Oct 9, 2023

NiLuJe added a commit to NiLuJe/koreader that referenced this pull request Oct 9, 2023

Statistics: Minor style-nit followup to koreader#10968

8883459

Too late to avoid the whitespace diff churn, unfortunately ;).

NiLuJe added a commit that referenced this pull request Oct 9, 2023

Statistics: Minor style-nit followup to #10968

d039aa5

Too late to avoid the whitespace diff churn, unfortunately ;).

	function ReaderStatistics:init()
	-- Disable in PIC documents (but not the FM, as we want to be registered to the FM's menu).
	if self.ui and self.ui.document and self.ui.document.is_pic then
	return
	end

Uh oh!

Finished books: freeze history timestamp and statistics #10968

Finished books: freeze history timestamp and statistics #10968

Uh oh!

Conversation

hius07 commented Oct 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poire-z commented Oct 4, 2023

Uh oh!

mergen3107 commented Oct 4, 2023

Uh oh!

Frenzie commented Oct 4, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NiLuJe Oct 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hius07 commented Oct 4, 2023

Uh oh!

NiLuJe commented Oct 4, 2023

Uh oh!

poire-z commented Oct 4, 2023

Uh oh!

mergen3107 commented Oct 4, 2023

Uh oh!

NiLuJe commented Oct 4, 2023

Uh oh!

mergen3107 commented Oct 4, 2023

Uh oh!

hius07 commented Oct 5, 2023

Uh oh!

poire-z commented Oct 5, 2023

Uh oh!

hius07 commented Oct 5, 2023

Uh oh!

poire-z left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poire-z commented Oct 5, 2023

Uh oh!

Frenzie commented Oct 5, 2023

Uh oh!

hius07 commented Oct 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

hius07 commented Oct 4, 2023 •

edited

Loading

NiLuJe Oct 4, 2023 •

edited

Loading

NiLuJe Oct 5, 2023 •

edited

Loading