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

refactor MainActivity, backport android_native_glue changes from r22 #257

Merged
merged 2 commits into from
Nov 3, 2020

Conversation

pazos
Copy link
Member

@pazos pazos commented Oct 18, 2020

Fixed: timeout logic (system timeout wasn't restored on activity paused)
Fixed: code styling issues
Moved: clipboard, device and timeouts to helper classes
Added: utils to manage permissions
Added: manage battery optimizations (API23+)
Changed: non-mandatory permission dialogs are available from lua
Changed: non e-ink devices don't use a SurfaceView
Changed: theme is now material design on compatible android versions (API21+)
Changed: backported android_native_glue changes from NDK r22
Removed: useless warning in AssetsUtils

cf: #253 (comment)

It doesn't involve massive changes in the code but uses modern "material design" dialogs if possible.

Pinging both @Galunid and @zwim: can you test that everything still works fine on your epd devices?

@zwim: lights still work?, epd updates fires fine?
@Galunid: does it build on docker container?, do you see the new light dialog with material style?, Is it legible?


This change is Reviewable

@pazos pazos requested a review from Frenzie October 18, 2020 13:50
@Galunid
Copy link
Member

Galunid commented Oct 18, 2020

@pazos I'm building it, I'll let you know what happens. Anything I should pay attention to in particular?

EDIT: I build it using docker successfully! :)

@pazos
Copy link
Member Author

pazos commented Oct 18, 2020

Anything I should pay attention to in particular?

A screenshot of the light dialog would be great. Other than that just check that nothing crashes :)

@Galunid
Copy link
Member

Galunid commented Oct 18, 2020

There was some problem with mupdf, it seems I need to clean build (it happened on emulator too), I'll test it in ~1h

@Galunid
Copy link
Member

Galunid commented Oct 18, 2020

Doesn't seem to crash. I'll continue to use it in case something comes up, but all seems good.

Screenshot:

Screenshot_20201018-170949

@zwim
Copy link
Contributor

zwim commented Oct 18, 2020

@pazos: I will check it, but I will need until Friday, because I have more work due to Corona :-(

@pazos
Copy link
Member Author

pazos commented Oct 19, 2020

Last commit on this branch contains changes into the android_native_app_glue that affect the frontend.

We were reacting to APP_CMD_WINDOW_REDRAW_NEEDED even when that callback was never fired. So, to prevent undefined behaviour in KOReader, the following change is needed:

diff --git a/frontend/device/android/device.lua b/frontend/device/android/device.lua
index c5530ffd..e9d93488 100644
--- a/frontend/device/android/device.lua
+++ b/frontend/device/android/device.lua
@@ -135,9 +135,14 @@ function Device:init()
             if ev.code == C.APP_CMD_SAVE_STATE then
                 return "SaveState"
             elseif ev.code == C.APP_CMD_GAINED_FOCUS
-                or ev.code == C.APP_CMD_INIT_WINDOW
-                or ev.code == C.APP_CMD_WINDOW_REDRAW_NEEDED then
+                or ev.code == C.APP_CMD_INIT_WINDOW then
                 this.device.screen:_updateWindow()
+            elseif ev.code == C.APP_CMD_WINDOW_REDRAW_NEEDED then
+                logger.dbg("APP_CMD_WINDOW_REDRAW_NEEDED")
+            elseif ev.code == C.APP_CMD_WINDOW_RESIZED then
+                logger.dbg("APP_CMD_WINDOW_RESIZED")
+            elseif ev.code == C.APP_CMD_CONTENT_RECT_CHANGED then
+                logger.dbg("APP_CMD_CONTENT_RECT_CHANGED")
             elseif ev.code == C.APP_CMD_CONFIG_CHANGED then
                 -- orientation and size changes
                 if android.screen.width ~= android.getScreenWidth()

The rest of changes shouldn't affect KO behaviour, but it is cool to log them anyways.

@zwim
Copy link
Contributor

zwim commented Oct 21, 2020

@pazos: This PR seems to work. I have noticed no problem.

@pazos
Copy link
Member Author

pazos commented Oct 21, 2020

This PR seems to work. I have noticed no problem.

Thank you for testing!. Some questions:

Did you applied #257 (comment)? If not I assume you didn't find any difference on the behaviour (ie: when returning to the activity, when closing the frontlight dialog ...)

Does the frontlight dialog looks any different?. It shouldn't if the firmware is older than Lollipop. But if it looks the same and works the same is a big win (and what's expected based on what I saw on old emulators)

@zwim
Copy link
Contributor

zwim commented Oct 21, 2020

On 4.4.2 (Tolino) I did not notice any difference.
Android dialog looks the same.

…tform/ndk/+/1180321

some APP_CMDs callbacks were never triggered:
APP_CMD_WINDOW_RESIZED / APP_CMD_WINDOW_REDRAW_NEEDED / APP_CMD_CONTENT_RECT_CHANGED

See android/ndk#1139
@pazos pazos force-pushed the android_helpers branch 2 times, most recently from 98f8bee to efe9b8c Compare October 24, 2020 14:59
@pazos pazos changed the title get rid of BaseActivity, refactor some code on helper classes, … refactor MainActivity, backport android_native_glue changes from r22 Oct 24, 2020
@pazos
Copy link
Member Author

pazos commented Oct 31, 2020

@Frenzie: still waiting your review :)

@NiLuJe
Copy link
Member

NiLuJe commented Oct 31, 2020

I think he's still in the "just moved" frenzy (ha!), and may not yet have a stable landline ;).

@pazos
Copy link
Member Author

pazos commented Oct 31, 2020

I think he's still in the "just moved" frenzy (ha!), and may not yet have a stable landline ;).

I can wait. Is just that I have another pile or changes to submit and don't want to push on top of this, yet.

@Frenzie
Copy link
Member

Frenzie commented Nov 1, 2020 via email

Copy link
Member

@Frenzie Frenzie left a comment

Choose a reason for hiding this comment

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

Looks fairly straightforward; didn't have time to check in the Android docs for some of the new (at least to us) calls/constants of which I don't know exactly what they're doing though.

Comment on lines 28 to 30
val hasLargeHeap = (applicationInfo.flags and ApplicationInfo.FLAG_LARGE_HEAP != 0)
val isDebuggable = (applicationInfo.flags and ApplicationInfo.FLAG_DEBUGGABLE != 0)
val isSystemApp = (applicationInfo.flags and ApplicationInfo.FLAG_SYSTEM == 1)
Copy link
Member

Choose a reason for hiding this comment

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

What are these for exactly? :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Build flags. Used for logging mainly. They're all booleans.

hasLargeHeap can be removed, because it was added to log the size of the dalvik VM heap when we disabled largeHeap app property. Nobody reported a crash due to a not enough dalvik heap size.

@pazos
Copy link
Member Author

pazos commented Nov 1, 2020

Looks fairly straightforward; didn't have time to check in the Android docs for some of the new (at least to us) calls/constants of which I don't know exactly what they're doing though.

Probably there's nothing very new. Just a few minor changes:

  • We start activities (dictionary apps, browser, ...) using NO_ANIMATION flag, to prevent the animation during activity transition.
  • Add a couple of api calls to check if the battery is optimized and to change that.
  • Permission code was refactored to be easily upgraded for new api requirements.

The size of the APK grows about 800kb because of the new required library "androidx.appcompat:appcompat". That's probably the last app size grow that's caused by android dependencies.

refactored runtime permissions, almost API30 ready.
simplified MainApp

Added: manage battery optimizations (API23+)
Fixed: code styling issues
Changed: non-mandatory permission dialogs are available from lua
Changed: non e-ink devices don't use a SurfaceView
Changed: theme is now material design on compatible android versions (API21+)
Changed: disabled animations while switching tasks
Removed: useless logs in AssetsUtils, unused manifest properties
@pazos pazos merged commit f2d946b into koreader:master Nov 3, 2020
pazos added a commit to koreader/koreader that referenced this pull request Nov 4, 2020
- Lcd devices won't use the SurfaceView, just the good old native content/window (except AndroidTv and ChromeOS)
- All android dialogs will be presented with Material Design on recent devices.
- Added an option to device settings to manage application battery optimization.
- Permissions that require the user to go to a settings page will be presented with a native android dialog.
- bump android-luajit-launcher

- Changes under the hood: koreader/android-luajit-launcher#257
@pazos pazos deleted the android_helpers branch December 19, 2020 19:06
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.

5 participants