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

Add wakelock helper,dont report isCharging when battery is full and fix some warnings. #90

Merged
merged 9 commits into from
Jan 10, 2019

Conversation

pazos
Copy link
Member

@pazos pazos commented Jan 9, 2019

I was fixing warnings when I found koreader/koreader#4430.

This PR adds an automatic wakelock handler during luajit-launcher app lifecycle and provides functions to modify its standard behaviour from lua.

The standard behaviour (acquire wakelock when the application is resumed, and thus focused, and release the wakelock when the application is paused will grant that both cpu, touchscreen and display are ready as long as the application runs on the foreground).

I tried to move some methods outside MainActivity to fix the mess we have here. For both powermanager, wifimanager and batterymanager we just need to get the MainActivity context but don't need to run on Ui thread.

Also added a minimum gradle wrapper for ant, which works fine, and can be extended later.

@pazos
Copy link
Member Author

pazos commented Jan 9, 2019

Pinging @chrox for review.

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 okay in principle; which versions did you test on? In principle we're going for API level 9 and up.

9 might mostly academic, but 14/15 compatibility is a must due to all the Android 4 ereaders.

}
}


Copy link
Member

Choose a reason for hiding this comment

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

/whistles

(Yes, I nitpick.)

import android.content.Context;
import android.net.wifi.WifiManager;

/* helper class to get battery state */
Copy link
Member

Choose a reason for hiding this comment

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

/whistles

}
}


Copy link
Member

Choose a reason for hiding this comment

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

nit

@pazos
Copy link
Member Author

pazos commented Jan 9, 2019

Looks okay in principle; which versions did you test on? In principle we're going for API level 9 and up.
9 might mostly academic, but 14/15 compatibility is a must due to all the Android 4 ereaders.

Except for building with gradle, which is disabled, everything else is using old methods, so it should work like before.

Tested on emulator(4.2), and devices(7.1 and 8.1)

@Frenzie
Copy link
Member

Frenzie commented Jan 9, 2019

Alright, if you could also test Android 2.3 please do, if not let me know and I'll do it tonight. :-)

@pazos
Copy link
Member Author

pazos commented Jan 9, 2019

Alright, if you could also test Android 2.3 please do, if not let me know and I'll do it tonight. :-)

If you have an emulator please do it yourself. I'm fairly low on disk space on my Ubuntu VM.

@Frenzie
Copy link
Member

Frenzie commented Jan 9, 2019

Never mind, we'll have to go without Android 2.3 testing. I forgot that the SD card on the Android 2.3 emulator is broken.

@pazos
Copy link
Member Author

pazos commented Jan 9, 2019

I forgot that the SD card on the Android 2.3 emulator is broken.

I don't understand. SD card shouldn't be needed for testing this PR. You can install the app with adb install koreader.apk

@Frenzie
Copy link
Member

Frenzie commented Jan 9, 2019

Yes, you can test some things, but Android always has an "SD card" in, e.g., /mnt/sdcard. In newer Androids they call it "internal storage" for display purposes, while an optional real SD card will be mounted someplace like /storage/the-uuid-or-something.

tl;dr The Android 2.3 emulator is broken in such a way that many apps won't run properly, including KOReader. Anyway, the PR crashes where it's supposed to:

E/luajit-launcher( 1599): Failed to run script: frontend/cache.lua:46: cannot open /mnt/sdcard/koreader/cache/: Permission denied

Before #87 it would crash without ever getting around to loading the main program. ;-)

Of course theoretically I could still test pretty much everything with some simple code but, well, I don't really care. I'm 90% sure I'm the only person who even notices when things break on Android 2.3.

Some people claim you can fix it by setting the SD card to more than 512 MB in the settings, but I tried various values (including gigabytes) without success.

screenshot_2019-01-09_19-09-13

wakelocks are handled by luajit-launcher application lifecycle (get onResume and release onPause)
android.setKeepScreenOn(bool) function was removed, use android.setWakeLock(bool) instead.
@pazos pazos changed the title Add wakelock helper, refactor wireless/battery helpers and fix some warnings. Add wakelock helper,dont report isCharging when battery is full and fix some warnings. Jan 10, 2019
@pazos
Copy link
Member Author

pazos commented Jan 10, 2019

@Frenzie: updated wakelock logic

It doesn't make sense to allow fine grained control from the frontend (it makes android/device.lua messy). We can just setWakeLock(false) instead and luajit-launcher will avoid wakelocks completely.

Ready for another round of reviews!

@Frenzie
Copy link
Member

Frenzie commented Jan 10, 2019

Btw, since you're at it you could consider updating 7-zip to 18.06 which came out about a week ago. It claims yet another 3-10% speed increase.

Is the only real change in the "add support for wakelocks commit" or should I look at the whole thing again?

@pazos
Copy link
Member Author

pazos commented Jan 10, 2019

Btw, since you're at it you could consider updating 7-zip to 18.06 which came out about a week ago. It claims yet another 3-10% speed increase.

Sure

Is the only real change in the "add support for wakelocks commit" or should I look at the whole thing again?

Yeah, is the only major change. The other little change is a slightly refactor of battery code to be able to report isCharging = false when the battery is full. It should hurt ;)

@Frenzie Frenzie merged commit c63947f into koreader:master Jan 10, 2019
@Frenzie
Copy link
Member

Frenzie commented Jan 10, 2019

Alright, thanks a lot!

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.

The build is now failing and this would be my prime suspect. Unfortunately the log is too long to tell.

https://gitlab.com/koreader/nightly-builds/-/jobs/144368807

@Frenzie
Copy link
Member

Frenzie commented Jan 11, 2019

Glancing through there suggests r15c is required for that? We currently use r12b because r15c balks at API levels lower than 14, although in practice that doesn't seem to be a problem for running the program on Android 2.3.

@Frenzie
Copy link
Member

Frenzie commented Jan 11, 2019

Btw, I did a quick experiment on GitLab with r15c and it's also failing. Unfortunately GitLab is stupid and they don't show you the part of the log with the actual failure, which is rather inconvenient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants