Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Screensaver option on Kindle devices #2734

Merged
merged 3 commits into from
Apr 8, 2017
Merged

Conversation

robert00s
Copy link
Contributor

Add screensaver on Kindle devices.
Defaults screensaver is disabled. We can enable it by select Menu -> Screensaver -> Enable cover screensaver
1
2

Tested on KPW3 with special offers and KPW2.

@@ -152,6 +152,20 @@ function FileManagerMenu:setUpdateItemTable()
},
}
}
elseif Device.isKindle() then
Copy link
Member

Choose a reason for hiding this comment

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

Does the folder screensaver functionality not work on Kindle? Maybe I'm missing something, but couldn't you just change if Device.isKobo() then to if Device.isKobo() or Device.isKindle() then and add an extra menu item there? In fact Kobo also needs a way to disable screensaver globally besides just for a specific book.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have not tested folder screensaver functoinality. Bessides there is no option to disable screensaver globally in Kobo.
Maybe in future I will "fix it".

Copy link
Member

@Frenzie Frenzie Apr 7, 2017

Choose a reason for hiding this comment

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

You don't have to add anything Kobo-specific, but my point is that you're currently locking Kindle users out of the regular screensaver functionality. Rather, you'd be adding the Kindle-specific "Enable screensaver" option to the menu so that Kindle users can then also choose to disable a specific cover for screensaver and to use a folder (if applicable). Maybe I'm being a bit unclear as I'm slightly distracted? :-)

Edit: Hm, I just realized there are a lot of weird things going on in the screensaver menu. A good candidate for some refactoring I suppose.

text = _("Screensaver"),
sub_item_table = {
{
text = _("Enable cover screensaver"),
Copy link
Member

Choose a reason for hiding this comment

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

I'd change this to "Enable screensaver".

@Frenzie Frenzie merged commit 59873ae into koreader:master Apr 8, 2017
@houqp
Copy link
Member

houqp commented Apr 10, 2017

I recall there is reason why screen saver was disabled on kindle back then. The MR dev community doesn't want people to use jailbreaking as a mean to circumvent ads in kindles with special offers. @NiLuJe is it still the case today?

@NiLuJe
Copy link
Member

NiLuJe commented Apr 10, 2017 via email

@robert00s
Copy link
Contributor Author

So we should remove screensaver option from Kindle :(

@NiLuJe
Copy link
Member

NiLuJe commented Apr 10, 2017 via email

@houqp
Copy link
Member

houqp commented Apr 11, 2017

OK, I have sent a PR to remove the feature for now. @robert00s I will be more than happy to merge a new PR if this feature is implemented to be only enabled for non SO users.

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.

None yet

4 participants