Sleep screen menu reworked #11549
Sleep screen menu reworked #11549
Conversation
adds instruction for files labeled "on hold" to sorting function
tidy-up
wording changes
wording changes and rearrangement of the whole menu
Screensaver --> Lock Screen
Screensaver --> Lock Screen
wording
You need to stick a
Haven't looked at the code, but I would assume it puts the additional messages "powered off" / "rebooting" on top of any screensaver (or only some?) when powering off or rebooting :) and maybe only when there is no custom message set (because the setting is named
Please tell us before what you have in mind, so the shock is less harsh when one of us will see you have completely thrown out these little menu bits we had crafted with love years ago. |
That's a slightly hurtful statement. :-) The top menu does have a cohesive vision. Some remote parts like the screensaver menu might fall outside that vision to some extent, perhaps mainly because of sabotage, better known as welcome user contributions.[1] This PR actually brings it in line with how it's supposed to be, although it's also attempting some sabotage in the form of capitalization. ;-) As @poire-z said, we'd definitely prefer to discuss changes first to discuss various rationales and solutions to problems. [1] This is why screenshots are important, and perhaps I should insist on them more even if I already feel like a big nag about it, and your PDF is great! It fulfills the same role: seeing the visual/organizational impact at a glance, rather than obscured by code. That also immediately opens it up for comments by people who can't test the code quite as easily for various reasons. |
|
First and foremost, I'm sorry to everyone as I am really not trying to hurt anyones feelings here.
My idea, was to make a tree like the one in the pdf before actually making any changes. I worked in this Lock Screen menu as it was in desperate need and, to use it as a portfolio to sell you all the idea of a 'koreader utopia'. 😅
I understand that, one person does something today, another one adds something tomorrow and suddenly things can get a bit... tangled; It is the nature of open source, both its power and weakness. Nevertheless, I am grateful to everyone who contributes to make it better.
This right here is true, the reason for not having screenshots is thus: I am running this on a kindle 4 and I could not figure out how to take screenshots on it, the method Amazon uses (press keyboard and menu keys at the same time) does not appear to work when koreader is running, so we were left just with the tree. If one of you wants to run it on your emulator and grab them, it would be highly appreciated. |
|
+1 for screensaver not lock screen |
|
So everyone is on the same page, the tree goes like this:
in this context, lock screen refers to the whole screen whilst the e-reader is… locked. The lock screen includes two items, the screen saver (i.e the image) and a custom message. Both things cannot be called the same thing, as they are not the same thing. |
No no, like I said, your alternative works just as well! :-D I'm just trying to explain it's a major contributor to missing changes that wouldn't slip by otherwise, particularly in hidden corners.
Other menus have resolved that as for example |
You Dare Use My Own Spells Against Me? I jest, point is, remember when I said the menu was like the Wild West? let me give you a few examples:
here's another one I don't understand why everyone is so appalled by the term 'Lock Screen' when that is what that screen is, according to the OED, Lock Screen is defined as: (a) Computing. a process or command which temporarily disables the visual interface of a computer; (b) a display screen which replaces the usual visual interface of a device, limiting its functionality until a password or code is entered. |
|
As there is no password or code needed I see it as a screensaver not a lock screen as in And as the OED puts it: |
|
well the definition of Lock Screen has two parts, @yparitcher you seem to have completely ignored the first one. and regarding part two, just because a door lock is not in its locked position, does not make it, not a door lock. The screen is lock until the 'unlock' button is pressed. you also forgot to add this bit: Screensavers were originally used to prevent damage to the screen, which could be caused by displaying a static image for an extended period of time, which I find kind of ironic because well... we are displaying a static image. But this is more a fun fact, not an argument to sustain my lock screen position. Edit:
I am not making changes to functionality, if you would like to contribute with the code, feel free to paste here and I'll happily add it. |
One is by design, the other is arguably something that slipped by in some manner. As stated, some parts will be slightly inconsistent for some reason or another. I don't think this is quite as inconsistent as you claim, but I would indeed prefer to also name it "Bookmark settings".
But there is no lock. It's just a screensaver. |
Concretely, But:
Is not abiding by the current (post-2017ish) design, but it's not inherently silly. |
Sentence case added
| if G_reader_settings:isTrue("screensaver_stretch_images") and percentage then | ||
| return T(_("Stretch to fit screen (with limit: %1 %)"), percentage) | ||
| end | ||
| return _("Stretch cover to fit screen") |
There was a problem hiding this comment.
(When playing, I was expecting that enabling this one, the 3 others (Black/White/Current) would be disabled. But no, they can still be enabled, and may have some effect because of the stretching % limit.)
| text = _("Background fill"), | ||
| help_text = _([[This option will only become available, if you have selected 'Put screen to sleep in current state' | ||
| as screensaver and have 'Sleep screen message' on.]]), | ||
| enabled_func = function() | ||
| return G_reader_settings:readSetting("screensaver_type") == "disable" and G_reader_settings:isTrue("screensaver_show_message") | ||
| end, |
There was a problem hiding this comment.
Genuine experience with this:
I was testing things and I didn't manage to get this menu enabled. Until I read that help_text.
I was naively expecting that background mentionned here is the background of the InfoMessage (white text on black, or black text on a white background color). But it is the background of the full screen, that we may indeed want it fill instead of "Leave screen as-is". But then, we're not letting "screen as-is" :)
I would somehow expect setting the full screen background to white or black to be inside the Wallpaper>: setting a fully unicolor wallpaper.
But may be it's technically more complicated - so, really not caring, just mentionning my first time experience.
There was a problem hiding this comment.
Indentation issue here too. Better checks elsewhere too.
There was a problem hiding this comment.
I suspect this problem (the logic, not the indentation) was already there?
There was a problem hiding this comment.
May be. But the wording (and may be the absence of any enabled_func) made it less surprising:
| @@ -43,7 +42,6 @@ return { | |||
| enabled_func = function() | |||
| return G_reader_settings:readSetting("screensaver_type") == "cover" | |||
| or G_reader_settings:readSetting("screensaver_type") == "random_image" | |||
| or G_reader_settings:readSetting("screensaver_type") == "image_file" | |||
There was a problem hiding this comment.
You are removing all references to G_reader_settings:readSetting("screensaver_type") possible being "image_file".
What will happen to people who have it set as "image_file"?
Does it need a migration?
And I would rather keep image_file (which I think more people may use) than document_cover.
frontend/ui/screensaver.lua
Outdated
| @@ -463,6 +467,7 @@ function Screensaver:setup(event, event_message) | |||
| self.screensaver_type = "random_image" | |||
| end | |||
| end | |||
| -- NB Kept around to support old settings. | |||
There was a problem hiding this comment.
Oh, ok, this is our way to not migrate it. Fine.
There was a problem hiding this comment.
I could do it the other way if you prefer.
There was a problem hiding this comment.
No real preference.
I guess that if an image is indeed a document, we can set that document as the sleep screen.
While having an EPUB or PDF set as "image_file" would feel more odd.
It's just that we'll mostly use it with images, and setting that image (a document)'s cover feels stranger mentionning a non-image document's cover.
|
I added a migration. It can migrate in reverse if you prefer. |
Again, no real preference. Fine with me as-is. Trusting you on all the logic. |
|
Thanks for all the work @Commodore64user! |
|
You didn’t actually merge that: ‘Delay screen update after wake-up‘, are you serious? you did a lot of work too so thanks @Frenzie |
|
@Frenzie there is still one instance of "screensaver"... and that ridiculous name from hell |
|
Where does it say screensaver? |
…/plural Concretely 1 second, %1 seconds since koreader#11549 added `1 second` all by itself.
|
there |
The problem with this reasoning is that it's like saying he ran up the hill yesterday is grammatically correct when he'll run up the hill tomorrow. The correct analogy is that you might indeed have an eye opening delay after waking up on a Saturday. |
This PR brings a complete make over to the Lock Screen, currently know as 'screensaver' menu.
NO changes have been made to functionality, this is a strictly cosmetic but comprehensive update.
The following image, showcases the new arrangement of the menu: most names have been changed for ease of use but again no changes to back end code (you can see that though).
Book1.pdf
Regarding "Hide reboot/poweroff Message", what the **ck does that do? I have never seen such a message and I don't have that selected. Anyway, I was not be able to place it correctly, so please let me know so it can be moved (if needed) to the appropriate place.
Regarding the newly named "background fill" option, could someone please help me make it so that is greyed out when both conditions ('Lock the screen in current state' and 'Add Lock screen message') are not met...
I made these changes because, honestly, the top menu is like the Wild West, if anyone else is happy and on board, I could work on renaming and rearranging the whole top menu in a much more cohesive way.
Blimey, this took a lot longer than I expected... 🤓
This change is