Adds time left for chapter and document to the screensaver message options #7897

IcedEagle · 2021-06-28T08:52:03Z

Adds options for time left for chapter and document to the screensaver message options.

Adds a Duration Format option to the screensaver message option, for Modern (1h23m) or Classic (1:23) styles to be consistent with the status bar options.

Tested on desktop emulator and Kobo Libra H2O device.

Modern:

Classic:

This change is

document for screensaver message

IcedEagle · 2021-06-28T08:57:36Z

As a side note, it's not checked in or part of my change, but I was able to test this on the desktop by changing generic/device.lua to support screensaver, and added onPowerEvent calls to ui/uimanager.lua in the isSDL check within UIManager:Init. However, I wasn't really sure the full ramification of that change, so I left it out. It only displayed the screensaver once per launch so clearly some issues there, but it worked well enough for my local testing of the feature though! :)

If there is another way I'm supposed to be able to test the screensaver functionality on PC, I'd love to know the trick!

Frenzie · 2021-06-28T14:46:09Z

Yes, if you want to play around with it on PC you'd need to add supportsScreensaver (or whatever the exact name is) in device to test it. Since we now distinguish the emulator in the SDL device it can probably be added by default.

Not quite sure what you mean with the powerevent stuff; that sounds neither desired nor necessary. ;-)

Frenzie · 2021-06-28T14:56:32Z

frontend/apps/reader/modules/readerfooter.lua

@@ -1902,6 +1902,7 @@ end

 function ReaderFooter:getDataFromStatistics(title, pages)
    local sec = _("N/A")
+    -- This is implemented by the Statistics plugin


If a comment along these lines is useful, it should be above over on line 1899. That being said, to me the function above strongly implies it's there in order to be overridden by an actual implementation. :-)

I will move it up to 1899. When I first started working through how to get the timing data, I got confused as to how this function was working. Having this comment might save someone else a bit of time finding where the override is, or at least, it would have saved me a bit of time.

NiLuJe · 2021-06-28T17:15:12Z

frontend/ui/elements/screensaver_menu.lua

+                        text = _("Duration format"),
+                        sub_item_table = {
+                            {
+                                text_func = function()
+                                    local util = require("util")
+                                    -- sample text to show 1h23m
+                                    local return_text = util.secondsToHClock(4980, true)
+                                    return T(_("Modern (%1)"), return_text)
+                                end,
+                                checked_func = function()
+                                    return G_reader_settings:readSetting("screensaver_duration_format") == "modern"
+                                end,
+                                callback = function()
+                                    G_reader_settings:saveSetting("screensaver_duration_format", "modern")
+                                end,
+                            },
+                            {
+                                text_func = function()
+                                    local util = require("util")
+                                    -- sample text to show 1:23
+                                    local return_text = util.secondsToClock(4980, true)
+                                    return T(_("Classic (%1)"), return_text)
+                                end,
+                                checked_func = function()
+                                    return G_reader_settings:readSetting("screensaver_duration_format") == "classic"
+                                end,
+                                callback = function()
+                                    G_reader_settings:saveSetting("screensaver_duration_format", "classic")
+                                end,
+                            },
+                        }
+                    }


Vaguely wondering if it wouldn't be worth merging all of these into a single setting in Date/Time, like what was done for 24h clocks?

I had the same thought when I originally put this in. It would be nice to have a single user setting to control this, and a new util function that returns the clock format based on that user preference so plug-ins wouldn't need to do this logic. To do it right, I would change the plug-in's that use util.secondsToClock or util.secondsToHClock to use this new function, and move the relevant user settings to a new one within Device -> "time and date". It would touch readerfooter, screensaver, bookstatuswidget, readerprogress and statistics. I would also add unit tests for the new util function.

Does this sound reasonable? If so, let me know if you'd prefer I close this PR while I do that work, since it's going to be a bit larger of a change than I originally planned.

That sounds perfect, yeah 👍.

I'd be inclined to prefer delaying the PR as a whole, but no strong feelings on that ;).

Latest iteration has this ready for review! Please check it out!

NiLuJe · 2021-06-28T17:24:52Z

frontend/ui/screensaver.lua

@@ -150,6 +182,8 @@ local function expandSpecial(message, fallback)
    ret = string.gsub(ret, "%%T", title)
    ret = string.gsub(ret, "%%A", authors)
    ret = string.gsub(ret, "%%S", series)
+    ret = string.gsub(ret, "%%h", time_left_chapter)
+    ret = string.gsub(ret, "%%H", time_left_document)


Only tangentially related to the PR, but this whole block could be "simplified" with a replacement table.

e.g.,

local rep = { ["%c"] = currentpage, ["%t"] = totalpages, ... } ret = ret:gsub("(%%%a)", rep)

Thank you! Wasn't aware of this. Made this change in latest iteration.

Well, it's kind of my bad for not implementing it this way the first time around, thanks ;).

- Added new utility function to pass to different clock formats - Added unit tests for new utility function - Updated any plugin that called util.secondsToClock to use the new util for different duration format - Add one time migration from footer's duration format - Removed duration format from footer settings Manual tests on desktop and Kobo. Verified all plugins look correct with both modern and classic formats.

IcedEagle · 2021-06-29T08:41:02Z

frontend/ui/uimanager.lua

@@ -377,10 +377,12 @@ function UIManager:init()
    elseif Device:isSDL() then
        self.event_handlers["Suspend"] = function()
            self:_beforeSuspend()
+            Device:onPowerEvent("Suspend")


This is the onPowerEvent I referred to in an earlier comment. This function is what is needed to actually kick off the screensaver. Without this, it suspends, but no screensaver appears.

if you change the above from Device:isSDL() to Device:isEmulator()[1] and it's probably good to go. Needs further testing though (mainly in the regular AppImage target).

[1] That's a fairly recent addition to distinguish the more recent regular desktop AppImage/Debian package from the emulator.

So I've traced this back to the event handler for Resume not firing when I'd expect on either isSDL or isEmulator (thus not resetting the screen_saver_mode variable, and not showing a screensaver after the first attempt).

What I can do to fix this is to add another event handler in uimanager for Device:isEmulator for the default input event, check if it's in screensaver mode, in which case do a device resume, and then send the input event along.

This seems to work:

elseif Device:isEmulator() then self.event_handlers["Suspend"] = function() self:_beforeSuspend() Device:onPowerEvent("Suspend") Device:simulateSuspend() end self.event_handlers["Resume"] = function() Device:onPowerEvent("Resume") Device:simulateResume() self:_afterResume() end **self.event_handlers["__default__"] = function(input_event) if Device.screen_saver_mode then self:resume() end self:sendEvent(input_event)** end

Would you prefer I put this in a separate PR at this point? I'm not as confident in my testing of this, though it seems to work fine in my limited tests. I'd also rip out the "Suspend" and "Resume" UI messages which seem pretty useless if the screensaver triggers when expected...

Yes, that definitely doesn't sound related and it'd just hold up this one. ;-)

IcedEagle · 2021-06-29T10:49:38Z

Forgot to submit these earlier. Wanted to take screenshots of the various places I've looked for the updated formats so you can take a look. Not shown are the screensaver and reader footer, because my earlier screenshots should capture those well.

Now switching back to Classic format. Should be no different to how it looks today.

poire-z · 2021-06-29T11:00:17Z

Guess I'm really classic, because these modern durations looks really ugly to me in all these pannels.
Just wondering if this duration mode for the footer items might mostly be used to reduce the widths of these durations, and let more room for the other items - and if some people might have used this for the footer, but would still prefer durations everywhere else to be "classic".

(But I personally don't mind, I'll stay classic all around :)

Frenzie · 2021-06-29T11:24:32Z

I have to wonder if the modern and classic labels might not be reversed… :-P

IcedEagle · 2021-06-29T12:04:01Z

Hah! I'm also someone who will opt to use Classic. Much easier for me to understand. I have it defaulting to use Classic as well for new users, but for existing users the one time migration will use a user's footer's format as their starting default.

NiLuJe · 2021-06-29T16:09:35Z

frontend/device/generic/device.lua

@@ -410,7 +410,7 @@ function Device:reboot() end
 -- Hardware specific method to initialize network manager module
 function Device:initNetworkManager() end

-function Device:supportsScreensaver() return false end
+function Device:supportsScreensaver() return true end


Unintended leftover from the Emu/SDL shenanigans?

You can pretty much do the same for the emulator with

function Emulator:supportsScreensaver() return true end

in https://github.com/koreader/koreader/blob/master/frontend/device/sdl/device.lua#L338

Yes, I will remove this since it's not necessary for the PR.

NiLuJe · 2021-06-29T16:11:09Z

frontend/ui/data/onetime_migration.lua

+if last_migration_date < 202100629 then
+    logger.info("Performing one-time migration for 202100629")
+
+    local footer = G_reader_settings:child("footer")


Congratulations for being the first actual user of the child method ^^.

NiLuJe

, thanks!

(Short of the Device:Generic question above).

IcedEagle · 2021-06-29T22:31:15Z

, thanks!

(Short of the Device:Generic question above).

Thank you! Made the change to remove the two files that had unrelated changes to the emulator from this PR.

poire-z · 2021-07-21T11:52:25Z

I have it defaulting to use Classic as well for new users, but for existing users the one time migration will use a user's footer's format as their starting default.

Dunno if it's just me, but on my 2 devices (Kobo, Android phone), where I don't remember having ever seen the modern (2"30') anywhere, I got to see it everywhere after some recent update that must have included this PR/migration.
I don't use any "time remaining" footer item, so may be the default for it was "modern" (haven't checked if it's the case - it's possible I did try it at some point and left it be "modern", dunno) - but having that default now propagate to everywhere feels a bit radical :)

NiLuJe · 2021-07-21T12:38:59Z

FWIW, I started from a fresh setup on the Elipsa (@ 2021.06-40), and it defaults to Classic.

poire-z · 2021-07-21T12:51:21Z

It seems the footer had it "modern" by default:

koreader/frontend/apps/reader/modules/readerfooter.lua

Lines 452 to 456 in 96c895d

    
               items_separator = "bar", 
        
               progress_pct_format = "0", 
        
               duration_format = "modern", 
        
               progress_margin = false, 
        
           }

I think you made some migration/cleanup of readerfooter not too long ago when you transfer the app defaults into the user footer settings - and then this picks that user prefered setting (while it was only a default being propagated, not a user preference :)).
Dunno if this is what triggers what I get, and if on a fresh setup, the ordering of this makes it default to "classic" ?

NiLuJe · 2021-07-21T13:20:41Z

I think you made some migration/cleanup of readerfooter not too long ago when you transfer the app defaults into the user footer settings

True, but the new code (post this PR) shouldn't honor that setting remnant anymore, it only checks the "new" consolidated setting:

koreader/frontend/apps/reader/modules/readerfooter.lua

Lines 1869 to 1877 in b31b772

    
           function ReaderFooter:getDataFromStatistics(title, pages) 
        
               local sec = _("N/A") 
        
               local average_time_per_page = self:getAvgTimePerPage() 
        
               local user_duration_format = G_reader_settings:readSetting("duration_format", "classic") 
        
               if average_time_per_page then 
        
                   sec = util.secondsToClockDuration(user_duration_format, pages * average_time_per_page, true) 
        
               end 
        
               return title .. sec 
        
           end

(And, IIRC, that does indeed default to classic now).

IcedEagle · 2021-07-21T20:02:28Z

@poire-z @NiLuJe If modern was the default, and people never changed it or used the time in the footer to notice it, it might have been better to not have handled any migration and just start everyone on Classic to keep consistent throughout more of the UI. Sorry, that didn't occur to me before, but probably would have been a better trade off in migrating.

Ideally we can just point people towards the new setting to flip it back, however if folks wanted, we could do a one-time migration and reset everyone back to classic, which anecdotally seems to be the far more popular option anyway.

koreader/frontend/ui/data/onetime_migration.lua

Lines 266 to 276 in b31b772

    
           -- 20210629, Moves Duration Format to Date Time settings for other plugins to use, https://github.com/koreader/koreader/pull/7897 
        
           if last_migration_date < 20210629 then 
        
               logger.info("Performing one-time migration for 20210629") 
        
               local footer = G_reader_settings:child("footer") 
        
               if footer and footer:has("duration_format") then 
        
                   local user_format = footer:readSetting("duration_format") 
        
                   G_reader_settings:saveSetting("duration_format", user_format) 
        
                   footer:delSetting("duration_format") 
        
               end 
        
           end

NiLuJe · 2021-07-21T20:29:08Z

Oh, right, the upgrade path picked up the "modern" default from the footer, didn't quite think of that ^^.

No strong feelings about it, but if we hear about it, why not?

Frenzie · 2021-07-21T20:33:48Z

I also found this an unpleasant surprise. I thought it was my own fault somehow.

Reset unwanted migration of this setting to "modern" done with #7897.

IcedEagle added 3 commits June 28, 2021 00:25

Adds time left in chapter and time left in

f44216e

document for screensaver message

Add Modern and Classic duration format options

bba0cec

Add a nan check

cdde835

Frenzie added this to the 2021.07 milestone Jun 28, 2021

Frenzie reviewed Jun 28, 2021

View reviewed changes

NiLuJe reviewed Jun 28, 2021

View reviewed changes

IcedEagle added 2 commits June 29, 2021 01:35

fix whitespace in static check

5488802

IcedEagle commented Jun 29, 2021

View reviewed changes

NiLuJe reviewed Jun 29, 2021

View reviewed changes

NiLuJe approved these changes Jun 29, 2021

View reviewed changes

Remove two files with emulator changes from the PR

acda696

NiLuJe approved these changes Jun 29, 2021

View reviewed changes

pazos merged commit ba65dc1 into koreader:master Jun 29, 2021

IcedEagle deleted the read-progress-screensaver branch June 29, 2021 23:51

poire-z mentioned this pull request Jul 23, 2021

Reset everyone's duration time to classic #8008

Merged

poire-z pushed a commit that referenced this pull request Jul 23, 2021

Reset everyone's duration time to classic (#8008)

d9cc3af

Reset unwanted migration of this setting to "modern" done with #7897.

poire-z mentioned this pull request Nov 29, 2021

Menu "<Gear> -> Status Bar -> Settings -> Page2 -> Duration Format" is missing #8493

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds time left for chapter and document to the screensaver message options #7897

Adds time left for chapter and document to the screensaver message options #7897

IcedEagle commented Jun 28, 2021 •

edited

IcedEagle commented Jun 28, 2021 •

edited

Frenzie commented Jun 28, 2021

Frenzie Jun 28, 2021

IcedEagle Jun 28, 2021

NiLuJe Jun 28, 2021

IcedEagle Jun 28, 2021

NiLuJe Jun 29, 2021 •

edited

IcedEagle Jun 29, 2021

NiLuJe Jun 28, 2021 •

edited

IcedEagle Jun 29, 2021

NiLuJe Jun 29, 2021

IcedEagle Jun 29, 2021

Frenzie Jun 29, 2021

IcedEagle Jun 29, 2021 •

edited

Frenzie Jun 29, 2021

IcedEagle commented Jun 29, 2021

poire-z commented Jun 29, 2021

Frenzie commented Jun 29, 2021

IcedEagle commented Jun 29, 2021

NiLuJe Jun 29, 2021

pazos Jun 29, 2021

IcedEagle Jun 29, 2021

NiLuJe Jun 29, 2021 •

edited

NiLuJe left a comment •

edited by Frenzie

IcedEagle commented Jun 29, 2021 •

edited by Frenzie

poire-z commented Jul 21, 2021

NiLuJe commented Jul 21, 2021 •

edited

poire-z commented Jul 21, 2021

NiLuJe commented Jul 21, 2021 •

edited

IcedEagle commented Jul 21, 2021

NiLuJe commented Jul 21, 2021 •

edited

Frenzie commented Jul 21, 2021 •

edited

Adds time left for chapter and document to the screensaver message options #7897

Adds time left for chapter and document to the screensaver message options #7897

Conversation

IcedEagle commented Jun 28, 2021 • edited

IcedEagle commented Jun 28, 2021 • edited

Frenzie commented Jun 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NiLuJe Jun 29, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NiLuJe Jun 28, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IcedEagle Jun 29, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IcedEagle commented Jun 29, 2021

poire-z commented Jun 29, 2021

Frenzie commented Jun 29, 2021

IcedEagle commented Jun 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NiLuJe Jun 29, 2021 • edited

Choose a reason for hiding this comment

NiLuJe left a comment • edited by Frenzie

Choose a reason for hiding this comment

IcedEagle commented Jun 29, 2021 • edited by Frenzie

poire-z commented Jul 21, 2021

NiLuJe commented Jul 21, 2021 • edited

poire-z commented Jul 21, 2021

NiLuJe commented Jul 21, 2021 • edited

IcedEagle commented Jul 21, 2021

NiLuJe commented Jul 21, 2021 • edited

Frenzie commented Jul 21, 2021 • edited

IcedEagle commented Jun 28, 2021 •

edited

IcedEagle commented Jun 28, 2021 •

edited

NiLuJe Jun 29, 2021 •

edited

NiLuJe Jun 28, 2021 •

edited

IcedEagle Jun 29, 2021 •

edited

NiLuJe Jun 29, 2021 •

edited

NiLuJe left a comment •

edited by Frenzie

IcedEagle commented Jun 29, 2021 •

edited by Frenzie

NiLuJe commented Jul 21, 2021 •

edited

NiLuJe commented Jul 21, 2021 •

edited

NiLuJe commented Jul 21, 2021 •

edited

Frenzie commented Jul 21, 2021 •

edited