[screensaver] Add option to cycle through folder images#13094
Conversation
| if G_reader_settings:isTrue("screensaver_cycle_images_alphabetically") then | ||
| local files = filemanagerutil.getFiles(dir, match_func) | ||
| if not files then return end | ||
| table.sort(files) -- Sort files alphabetically (numbers go: 1, 10, 11, 2, 20, 21, ...) |
There was a problem hiding this comment.
I suppose a user expects natural sorting.
There was a problem hiding this comment.
fair assessment, will make it so
|
Thank you!! |
|
You might also like #13068 @mergen3107 |
|
Maybe, but I don't like book covers as screensavers in general :D |
|
Well you are obviously wrong ;) |
| index = 1 | ||
| end | ||
| local file = files[index] | ||
| index = (index % #files) + 1 |
There was a problem hiding this comment.
Can be just index = index + 1 after line 103, and saving default 0 in line 103.
I.e. saving the last shown image index instead of the next image index.
|
So, if the new option enabled, it's not a random image anymore. |
| genMenuItem(_("Show custom image or cover on sleep screen"), "screensaver_type", "document_cover"), | ||
| genMenuItem(_("Show random image from folder on sleep screen"), "screensaver_type", "random_image"), | ||
| genMenuItem(G_reader_settings:isTrue("screensaver_cycle_images_alphabetically") and _("Show cycled images from folder on sleep screen") | ||
| or _("Show random image from folder on sleep screen"), "screensaver_type", "random_image"), |
There was a problem hiding this comment.
I think we need a generic wording, not changeable.
There was a problem hiding this comment.
it doesn't work anyway... i am actually okay with original but we'll see what others say
There was a problem hiding this comment.
What doesn't work? Say about what?
There was a problem hiding this comment.
Well that "G-sett and or" didn't work, anyway hius thinks we should change the string because it might not be a random image anymore, i am not bothered by it.
| return filemanagerutil.getRandomFile(dir, match_func) | ||
| -- If the user has set the option to cycle images alphabetically, we sort the files instead of picking a random one. | ||
| if G_reader_settings:isTrue("screensaver_cycle_images_alphabetically") then | ||
| local start_time = os.clock() |
There was a problem hiding this comment.
i disagree this time, i am confident that someone is going to put 3000 images into a folder (with crazy names like dfhkoemd457628.png) and then complain about how slow it is to lock the screen.
edit: i briefly considered the idea of capping the number of files we would cycle through, but who am i to say 100 images (or whatever) is enough?
edit 2: if we capped it at 128 images we could always lie about being some sort of algorithm limitation ;)
There was a problem hiding this comment.
but who am i to say 100 images (or whatever) is enough?
And who are you to say 0.5 seconds is too much ?
A shorter logger.info always showing will be fine for the rare people using this feature.
logger.info("Screensaver: finding and sorting", #files, "files took ", time.to_s(time.since(start_time)), " seconds")
There was a problem hiding this comment.
Estimated Performance on Kindle Devices
sorting 128 files with random and complex names, using our sort function,( according to AI)
Kindle 3 (532 MHz CPU):
Worst-case estimate: 0.5 to 1 second for sorting 128 files
The lower clock speed will significantly impact the performance of complex string operations.
Kindle 4 (800 MHz CPU):
Worst-case estimate: 0.3 to 0.6 seconds for sorting 128 files
The higher clock speed will help, but the complex string operations will still be relatively slow.
Kindle Voyage (Single-core 1 GHz processor):
Estimated worst-case time: 0.3 to 0.5 seconds
Kindle Paperwhite 5 (Quad-core 1 GHz processor):
Estimated worst-case time: 0.1 to 0.2 seconds
These estimates are significantly higher than the previous ones due to:
The complexity of the conversion function
Extensive use of pattern matching and string manipulation
Potential memory constraints affecting cache performance
There was a problem hiding this comment.
I'd tell the AI there's a chance I/O is more significant than CPU (at least on the newer devices).
There was a problem hiding this comment.
I was not talking about the performance, just about you deciding to logger.warn() or not at all at a threashold of 0.5s.
Let the user know with a logger.info about how it takes for how many books, so he can... think about that early.
There was a problem hiding this comment.
lol, i was composing that message before i had even read your 0.5s challenge... my question and the point of asking AI that is, should we in some way intervene in potential terrible decision making by users?
regarding the 0.5 sec, it was taken out of a hat and it was also what i deemed reasonable to wait, alas we can just simply use logger.info indeed.
using I/O all the calculations jump to seconds (as in 1 sec +)
There was a problem hiding this comment.
We do a lot of searching here and there and do not measure.
FileChooser shows folders with hundreds of files, sorts them, and we do not measure and do not warn.
Useless overengineering.
| local elapsed_time = os.clock() - start_time | ||
| if elapsed_time > 0.5 then -- threshold in seconds | ||
| logger.warn("Screensaver: finding and sorting", #files, "files alphabetically took ", elapsed_time, " seconds") | ||
| end |
There was a problem hiding this comment.
We don't use os.clock(), we made every time thingy consistent to use our time.lua module.
So, here, you should do as in:
koreader/frontend/apps/reader/readerui.lua
Lines 317 to 319 in 1750499
There was a problem hiding this comment.
i am crashing with time.now
./luajit: frontend/device/kindle/device.lua:650: loop or previous error loading module 'ui/screensaver'
stack traceback:
[C]: in function 'require'
frontend/device/kindle/device.lua:650: in function 'intoScreenSaver'
frontend/device/kindle/device.lua:766: in function 'handler'
frontend/ui/uimanager.lua:1421: in function 'handleInputEvent'
frontend/ui/uimanager.lua:1523: in function 'handleInput'
frontend/ui/uimanager.lua:1567: in function 'run'
./reader.lua:280: in main chunk
[C]: at 0x00013e81
lipc-wait-event exited normally with status: 0
There was a problem hiding this comment.
nevermind, i had an incorrect path for time
There was a problem hiding this comment.
INFO Screensaver: finding and sorting 23 files took 0.034673 seconds
INFO Screensaver: finding and sorting 23 files took 0.03208 seconds
|
Do we show a checkered sleep screen image under any circumstances? When trying to implement how to limit the number of files, the first thing i thought was to run a loop that would stop at 128 (that didn't work) but what it did do, was show a checkered sleep screen, not the fallback K. And no, i don't have a checkered image on that folder. |
| end, false) | ||
| if #files == 0 then return end | ||
| -- Slippery slope detected! Ensure the number of files does not exceed 128 to prevent performance issues. | ||
| if #files > 128 then -- this seems like a reasonable [arbitrary] limit |
There was a problem hiding this comment.
If you want to bench you should do the two parts separately.
There was a problem hiding this comment.
Do you mean fetching and sorting?
There was a problem hiding this comment.
Yes, if you don't measure you don't know what it is that's actually taking time.
There was a problem hiding this comment.
The sorting, that is what takes 90% of the time. I am not too concerned about the fetching really, is the sorting i am more worried about. Would you really want them separately? I don’t mind having them combined but if you insist...
There was a problem hiding this comment.
Well no, but in the future please try to lead with "I measured it and it only took 10% of the time". ;-)
| files = {table.unpack(files, 1, 128)} | ||
| end | ||
| -- we have files, sort them in natural order, i.e z2 < z11 < z20 | ||
| local sort = require("frontend/sort") |
There was a problem hiding this comment.
| local sort = require("frontend/sort") | |
| local sort = require("sort") |
See bb93c44
| -- Slippery slope detected! Ensure the number of files does not exceed 128 to prevent performance issues. | ||
| if #files > 128 then -- this seems like a reasonable [arbitrary] limit | ||
| logger.warn("Screensaver: found", #files, "files, dropping", #files - 128) | ||
| files = {table.unpack(files, 1, 128)} |
There was a problem hiding this comment.
I'm not sure if this is necessary, but presumably it'd be a lot faster to skip insertion in the first place.
local files = {}
local num_files = 0
util.findFiles(dir, function(file)
if num_files > 512 then return end
table.insert(files, file)
num_files += 1
end, false)There was a problem hiding this comment.
Yeah, I don’t think findFiles likes the returns. i.e you will get the checkered screen of death. i.e v2 it won’t work
There was a problem hiding this comment.
I assume the problem is that += 1 isn't Lua, for which apologies but only half. It's pseudocode to communicate an idea, not something to copy paste and expect it to work out of the box.
There was a problem hiding this comment.
Not what i meant, i tried something like that last night with num = num + 1 and it would not work. Checkered screen of death.
Edit: i am going to look like an absolute "genius" if when i get home, try your suggestion and it works, even though i know it won't
There was a problem hiding this comment.
Well that's what i was trying last night when i got the checkered screen of death, but see edit above.
There was a problem hiding this comment.
congratulation!!! you were right. not sure what it was that I was doing differently but I am happy it works though.
so yeah lets just politely assume that this conversation never took place.
I don't believe so. |
|
for documentation purposes: here are the results on a K4 logsand apparently, these results suddenly contradict the idea that sorting would always take longer than fetching, in this case fetching time was edit: and yet subsequent runs invert the whole matter again, on a second lock we get |
| local file = files[index] | ||
| G_reader_settings:saveSetting("screensaver_cycle_index", index) | ||
| return file |
There was a problem hiding this comment.
Keep things logically together, so saveSeeting(index) once you're done, no need to pick file before it and store it as a local.
G_reader_settings:saveSetting("screensaver_cycle_index", index)
return files[index]
| text = _("Show images in alphabetical order"), | ||
| help_text = _("When enabled, images will be shown in alphabetical order rather than in random order."), |
There was a problem hiding this comment.
We display a single cover image, so it's not really "Show images in alpha order".
Pick image ?
May be you should mention the only benefit of this option: each image will be picked, no duplicate one before we iterate the whole set, something like that.
I was wrong btw, we do: 6ea292e |
Commodore64user
changed the title
Co-authored-by: Frans de Jonge <fransdejonge@gmail.com>
RenderImage does when whatever was asked of it failed to render/decode. |
The checkered grid of death wasn’t too far off my assumption, then. ;) |
|
Yup, that was indeed the intent ;). |
Ever thought of writing books? :D |
6 participants
what's new
frontend/ui/elements/screensaver_menu.lua: Added a new menu item to toggle the display of images in alphabetical order.frontend/ui/screensaver.lua: Modified the_getRandomImagefunction to support the new alphabetical order option by sorting files and cycling through them. Wasn't sure whether to create a brand new one or just adapt this one, alas the latter was chosen.screenshots
This change is