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

fullscreen toggle for Jelly Bean devices #5088

Merged
merged 2 commits into from Jul 6, 2019

Conversation

@pazos
Copy link
Contributor

commented Jun 21, 2019

Work in progress:

Things work but the new viewport is not changed without reloading current app (I mean the reader or the file manager).
Move the old toggleFullscreen for apis 14-16 to device/android/device.lua?

requires koreader/android-luajit-launcher#148
fixes #4794

-- fullscreen toggle on devices with compatible fullscreen methods (apis 14-16)
if Device.firmware_rev <= 16 then
-- fullscreen toggle on devices with compatible fullscreen methods (apis 14-18)
if Device.firmware_rev < 19 then

This comment has been minimized.

Copy link
@Frenzie

Frenzie Jun 21, 2019

Member

Maybe a little info message that a restart is required for changes to take effect?

This comment has been minimized.

Copy link
@pazos

pazos Jul 5, 2019

Author Contributor

done, but only for jelly bean apis.

@pazos pazos added this to the 2019.07 milestone Jul 4, 2019

@pazos pazos force-pushed the pazos:fullscreen_jb branch from 624c0c8 to 573b733 Jul 5, 2019

@pazos

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

Also removed the patch.lua workaround now that is useless

@pazos pazos requested a review from Frenzie Jul 5, 2019

@pazos pazos changed the title wip: fullscreen toggle for Jelly Bean devices fullscreen toggle for Jelly Bean devices Jul 5, 2019

@pazos

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

before -> toggle -> after

Sin nombre

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

Also removed the patch.lua workaround now that is useless

Might be useless in your context of fixing screen geometry, but it could be useful in other contexts :)
I haven't played with it much recently, but on Android, there's nothing much else to be able to play and tweak stuff, eg #3871 (comment).

So, I don't know. Do you really need to get rid of it so current users who had it to tweak viewport don't go mess with your latest changes?
If not, it would be best to keep that available as a possibility for quick tweaks/tests.

@pazos

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

Also removed the patch.lua workaround now that is useless

Might be useless in your context of fixing screen geometry, but it could be useful in other contexts :)
I haven't played with it much recently, but on Android, there's nothing much else to be able to play and tweak stuff, eg #3871 (comment).

So, I don't know. Do you really need to get rid of it so current users who had it to tweak viewport don't go mess with your latest changes?
If not, it would be best to keep that available as a possibility for quick tweaks/tests.

A half approach would be renaming patch.lua to runtime.lua, so old users who added fancy things there can rename their patch file and users who added things to workaround screen.height can forget about it.

What do you think?

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

That's fine with me :)
(patch.lua was a nice and explicite filename, runtime.lua a bit less, but better than nothing at all :)

@pazos

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

The worst thing that can happen if we leave patch.lua as is: bug reports from people saying "the window doesn't fill the screen". I can live with that 👍

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

I don't know how feasible and/or annoying that would be, but perhaps add an unobtrusive visual indicator somewhere if a patch.lua is in effect?

patch.lua is not needed for screen height workarounds, but can be use…
…ful as it is the only file that allow us to change KOReader behaviour on Android without rebuilding the application

@pazos pazos force-pushed the pazos:fullscreen_jb branch from 573b733 to 31d1929 Jul 5, 2019

@pazos

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

I don't know how feasible and/or annoying that would be, but perhaps add an unobtrusive visual indicator somewhere if a patch.lua is in effect?

I don't want to do that unless it can be done with a Text to Speech engine 😄

@Frenzie

Frenzie approved these changes Jul 6, 2019

@pazos pazos merged commit dca24e0 into koreader:master Jul 6, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@zeroheure

This comment has been minimized.

Copy link

commented Jul 10, 2019

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.