Skip to content

[screensaver menu] remove gesture based option from NT devices - #12747

Merged
Frenzie merged 9 commits into
koreader:masterfrom
Commodore64user:sleep-screen
Nov 23, 2024
Merged

[screensaver menu] remove gesture based option from NT devices#12747
Frenzie merged 9 commits into
koreader:masterfrom
Commodore64user:sleep-screen

Conversation

@Commodore64user

@Commodore64user Commodore64user commented Nov 17, 2024

Copy link
Copy Markdown
Member

what's new:

  • Added a new local function getUntilTapOrKeyPressText to return appropriate text based on whether the device is a touch device or not.
  • Refactored the return statement to use a local menu_items table.
  • Conditionally added the Until 'exit sleep screen' gesture menu item only to touch devices.

issue was first documented here #12744


This change is Reviewable

@mergen3107

Copy link
Copy Markdown
Contributor

Conditionally added the "Until 'exit sleep screen' gesture" menu item only for touch devices.

@Commodore64user
Can you please also make it so it unchecks it if 1) device is non-touch; and 2) this setting was on in the settings.readerl.lua? This will definitely help, because I scratched my head when it was on - I had to also press a key after power button to let KOReader open it (there is no frontlight on K4NT, so I didn't see any indication that it was waiting for "gesture" after power button :D)

@Commodore64user

Commodore64user commented Nov 17, 2024

Copy link
Copy Markdown
Member Author

according to this comment here

-- These 2 (optional) parameters are to support poweroff and reboot actions on Kobo (c.f., UIManager)
self.prefix = event and event .. "_" or "" -- "", "poweroff_" or "reboot_"
self.event_message = event_message
if G_reader_settings:has(self.prefix .. "screensaver_type") then
self.screensaver_type = G_reader_settings:readSetting(self.prefix .. "screensaver_type")
else
if event and G_reader_settings:isFalse("screensaver_hide_fallback_msg") then
-- Display the provided event_message over the screensaver,
-- so the user can distinguish between suspend (no overlay),
-- and reboot/poweroff (overlaid message).
self.overlay_message = self.event_message
end
end

it appears the "Hide reboot/poweroff message" is kobo specific, should it be removed from non kobo devices?

{
text = _("Hide reboot/poweroff message"),
checked_func = function()
return G_reader_settings:isTrue("screensaver_hide_fallback_msg")
end,
callback = function()
G_reader_settings:toggle("screensaver_hide_fallback_msg")
end,
},

@NiLuJe

NiLuJe commented Nov 17, 2024

Copy link
Copy Markdown
Member

it appears the "Hide reboot/poweroff message" is kobo specific, should it be removed from non kobo devices?

No, that comment is misleading/outdated, it's actually available anywhere we can control poweroff/reboot.

@NiLuJe

NiLuJe commented Nov 17, 2024

Copy link
Copy Markdown
Member

anywhere we can control poweroff/reboot.

i.e., canReboot & canPowerOff Device caps.

}

if Device:isTouchDevice() then
table.insert(menu_items[1].sub_item_table[8].sub_item_table,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly #menu_items[1].sub_item_table instead of 8? (Assuming it's indeed the final element).

Or anything else you might come up with to avoid the magic number, basically ;).

@Commodore64user Commodore64user Nov 17, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iterating through all the elements and matching the name of the sub_item_table ("Postpone screen update after wake-up")? ignore that

(Assuming it's indeed the final element).

it isn't. it the 8th/9

"Show book cover on sleep screen"
"Show custom image or cover on sleep screen"
"Show random image from folder on sleep screen"
"Show reading progress on sleep screen"
"Show book status on sleep screen"
"Leave screen as-is"
"Border fill, rotation, and fit"
8. "Postpone screen update after wake-up" -- this is the one
"Custom images"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you just on line 97:

- -- "Until 'exit sleep screen' gesture" is added later to touch devices
+ Device:isTouchDevice() and genMenuItem(_("Until 'exit sleep screen' gesture"), "screensaver_delay", "gesture") or nil

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that works, yes.

@Commodore64user

Copy link
Copy Markdown
Member Author

anywhere we can control poweroff/reboot.

i.e., canReboot & canPowerOff Device caps.

AND or OR?

@NiLuJe

NiLuJe commented Nov 17, 2024

Copy link
Copy Markdown
Member

AND or OR?

U+0026 ;p.

genMenuItem(_("5 seconds"), "screensaver_delay", "5"),
genMenuItem(_("Until a tap"), "screensaver_delay", "tap"),
genMenuItem(_("Until 'exit sleep screen' gesture"), "screensaver_delay", "gesture"),
genMenuItem(getUntilTapOrKeyPressText(), "screensaver_delay", "tap"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not worth a function 80 lines away, you can inline that:
genMenuItem(Device:isTouchDevice() and _("Until a tap") or _("Until a key press"), "screensaver_delay", "tap"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(as I did, but opposite in #12771)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was done prior to realising that trick worked inside that function

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just learned it today myself :D
_meta.lua:
https://github.com/koreader/koreader/pull/12766/files

Comment on lines 176 to -168
},
{
text = _("Hide reboot/poweroff message"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is the last item in the array, you can use:

Device:canReboot() and Device:canPowerOff() and {
  text...
} or nil

instead of the table.insert() below.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do then

Comment on lines +29 to +31
return {

local menu_items = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can now keep the original return {.

@hius07

hius07 commented Nov 22, 2024

Copy link
Copy Markdown
Member


Looks like a wrong line.

Comment thread frontend/ui/data/onetime_migration.lua Outdated

-- Date at which the last migration snippet was added
local CURRENT_MIGRATION_DATE = 20240928
local CURRENT_MIGRATION_DATE = 20241117

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Update the date to the day this gets a chance to be merged :))

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmm what does that mean? today? I thought the specific date was irrelevant so long as it was a number larger than the previously used one.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it indeed just needs to be larger than the previous one - but best to have it precise so we know when things happened and are not misleaded.
Today is fine if the PR is ready (dunno about @hius07 last comment), even if merged tomorrow.

@Commodore64user

Copy link
Copy Markdown
Member Author

Happy 50th commit-versary to me. 😬🥂

@mergen3107

Copy link
Copy Markdown
Contributor

And your Commodore 64!

@Frenzie Frenzie added this to the 2025.01 milestone Nov 23, 2024
@Frenzie
Frenzie merged commit de7c592 into koreader:master Nov 23, 2024
@Commodore64user
Commodore64user deleted the sleep-screen branch November 23, 2024 20:34
0xstillb pushed a commit to 0xstillb/koreader-thai that referenced this pull request May 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants