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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: java inheritance and fix some warnings/lints #161

Merged
merged 1 commit into from Aug 13, 2019

Conversation

@pazos
Copy link
Contributor

commented Jul 26, 2019

A friend of mine lent me "Thinking in Java", after a couple of hours reading it I became a hardcore inheritance monk. Things went crazy and now I have an all-in-one PR. Sorry 馃う鈥嶁檪

Native

  • To improve compatibility with Android Studio I moved each lib in their own subfolder with their own Android.mk file. Moved git submodules too.

  • Added a local copy of android_native_app_glue, so we can share logger name and apply the same logger behaviour.

  • Changed the name "main thread" to "NativeThread". The main thread is running on the VM and handled normally via android framework and the native thread is running outside the VM using app_glue(pthreads).

Java

  • Improves logging: we extend android.app.Application in order to obtain the application name. Logger is now easier because it will require only a message, as we log everything with the app name as the logger tag (as we do in C and lua).

  • Improves inheritance and hopefully is now easier to read.

    • Java methods invoked from lua are declared in ILuaJNI.

    • NativeSurfaceView extends from SurfaceView. Implements a placeholder for our native window inside a view hierarchy and adds a few methods to update the view on e-ink devices.

    • Base implementation of lua/JNI methods is done in BaseActivity.

    • MainActivity extends from BaseActivity, takes ownership of the native window using a new NativeSurfaceView and implements support for runtime permissions. It also overrides some BaseActivity methods with implementations that are view-aware.

  • Fixes the underlying cause behind most warnings: most of them were NullPointerException and were fixed by checking if the returned value was not null.

  • Suppress the rest of warnings reported by firing up android studio -> Analyze -> Inspect Code: Trivial warnings are disabled in gradle scripts and the rest are suppressed using annotations because they affect a few classes, methods or fields.

  • Modifies the Makefile:

    • Enable all gradle warnings
    • More informative messages
    • add make test to build all the tests (currently there is only one: einkTest).
    • make release and make debug are used to build the NativeActivity.apk.
  • Minor gradle plugin bump

  • renamed device package from org.koreader.device to org.koreader.launcher.device

  • created a separate package for helpers.

@pazos pazos force-pushed the pazos:inheritance branch 2 times, most recently from 53c9e20 to ba02f17 Jul 26, 2019

@pazos pazos force-pushed the pazos:inheritance branch 2 times, most recently from 2b8943a to 31a07c6 Aug 4, 2019

@pazos pazos changed the title [wip] refactor: java inheritance and fix some warnings/lints refactor: java inheritance and fix some warnings/lints Aug 4, 2019

@pazos pazos force-pushed the pazos:inheritance branch from 31a07c6 to c031f80 Aug 4, 2019

@pazos pazos added the enhancement label Aug 4, 2019

@pazos pazos force-pushed the pazos:inheritance branch 3 times, most recently from bb3e45e to 8523964 Aug 4, 2019

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

@Frenzie
Frenzie approved these changes Aug 5, 2019
Copy link
Member

left a comment

I trust you on the @SuppressWarnings stuff, besides that it's probably slightly easier to understand this way.

.append("\ndata: ")
.append(intent.getDataString())
.append("\nextras: ")
;

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 5, 2019

Member

Huh, interesting. Makes sense (similar to trailing comma) but I don't think I've ever seen it.

@Frenzie

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

Oh btw did you want to get this in the upcoming stable (tomorrow at the earliest; probably a few more days due to translations) or wait for the next one?

@pazos

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

Oh btw did you want to get this in the upcoming stable (tomorrow at the earliest; probably a few more days due to translations) or wait for the next one?

I would say wait for the next release. I profiled the application using Android Studio and things are almost the same as they were before applying this PR. Just need time to (re)review the new classes and see if everything makes sense.

@pazos

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

I trust you on the @SuppressWarnings stuff, besides that it's probably slightly easier to understand this way.

Some of the (unused/unused return values) are here to stay, as they are reported for the launcher project but not for the einkTest project. The same happens on epd controller abstract classes, which are firing weaker access warnings in the context of a project but they need to be public as they are copied and used outside that project. Also all methods of the mainActivity called using JNI are marked as unused because the inspection tools don't really understand that they are used from lua.

@pazos pazos force-pushed the pazos:inheritance branch from 70ea27e to 5d18511 Aug 6, 2019

@pazos

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

@Frenzie: can we disable https://pmd.github.io/latest/pmd_rules_java_codestyle.html#defaultpackage in codacy?.

Encapsulation is cool, but to get rid of these warnings we're declaring methods/fields as public and suppressing WeakerAccess warnings.

@Frenzie

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

We'll have to make a custom ruleset.xml. Pity that the Codacy docs are rather vague even when existent.

@pazos pazos force-pushed the pazos:inheritance branch 2 times, most recently from 61ec425 to fc048f8 Aug 8, 2019

@pazos

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

We'll have to make a custom ruleset.xml. Pity that the Codacy docs are rather vague even when existent.

I just disabled it from Codacy webpage (I logged in with github).

@pazos pazos force-pushed the pazos:inheritance branch 2 times, most recently from 58baeee to fc374c4 Aug 8, 2019

@Frenzie

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

That specific rule you mean?

The Codacy web GUI is alright but then it's not very useful for local checks.

@pazos

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

The Codacy web GUI is alright but then it's not very useful for local checks.

Sure. We want our own ruleset in the long run. I started to make one bindly (before logging in codacy):
ruleset.xml.zip - remove extension.

Codacy does not provide ready available rulesets.xml, so we need to do it manually (and there are more than 200 rules for PMD/Java)

@pazos pazos force-pushed the pazos:inheritance branch from fc374c4 to 7a11199 Aug 8, 2019

@pazos

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2019

I added a local copy of android_native_app_glue to our source tree. It makes it easier for us to customize it (ie: for logging with "tag: [subtag]: message" scheme).

IMO it makes a bit easier to understand what is happening because the (verbose) comments in the sources.

So, instead of requiring adb logcat KOReader:* threaded_app:* *:S to log the interaction between our program and the native glue we just type adb logcat KOReader:* *:S. All the logs that app_glue shows will be ignored on release builds.

@Frenzie

This comment has been minimized.

Copy link
Member

commented Aug 10, 2019

I hope it won't be annoying to update with NDK/SDK updates. :-)

@pazos

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2019

Nah, it never changed since it was introduced (in api 9, gingerbread)

@pazos

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

@Frenzie: please give it a quick review when you have some time (and re-read the first post). I think I'm not going to do more big changes, just little fixes.

@Frenzie
Copy link
Member

left a comment

I have a feeling that it looks clearer still than the last time I looked at it. 馃憤

Makefile Outdated Show resolved Hide resolved
app/src/org/koreader/launcher/device/EPDFactory.java Outdated Show resolved Hide resolved

@pazos pazos force-pushed the pazos:inheritance branch 2 times, most recently from 8ba564a to cca2276 Aug 12, 2019

@pazos

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

Now luajit-launcher logs are better:

We log application info before starting any activity.
We log activities, helpers and native_glue with a subtag.
We log available height and width from NativeSurfaceView

That doesn't affect the standard cross-platform KOReader logs in any way.

Here is what happens on early init (just before running lua code):

release (info, warn, error)

adb logcat KOReader:I *:S

I/KOReader(10884): Application started

release (verbose, debug, info, warn, error)

adb logcat KOReader:V *:S

I/KOReader(10884): Application started
V/KOReader(10884): Application info {
V/KOReader(10884):   Flags: user, debuggable
V/KOReader(10884):   Paths {
V/KOReader(10884):     Assets: /data/data/org.koreader.launcher/files
V/KOReader(10884):     Library: /data/app-lib/org.koreader.launcher-1
V/KOReader(10884):     Storage: /mnt/sdcard
V/KOReader(10884):   }
V/KOReader(10884): }
V/KOReader(10884): [NativeThread]: waiting for activity
V/KOReader(10884): [NativeSurfaceView] surface created
V/KOReader(10884): [NativeThread]: activity window ready.
V/KOReader(10884): [NativeSurfaceView] surface changed {
V/KOReader(10884):   width:  1080
V/KOReader(10884):   height: 1776
V/KOReader(10884): }
V/KOReader(10884): [NativeThread]: activity gained focus.
V/KOReader(10884): [NativeThread]: launching LuaJIT assets

debug (verbose, debug, info, warn, error)

adb logcat KOReader:V *:S

I/KOReader(10884): Application started
V/KOReader(10884): Application info {
V/KOReader(10884):   Flags: user, debuggable
V/KOReader(10884):   Paths {
V/KOReader(10884):     Assets: /data/data/org.koreader.launcher/files
V/KOReader(10884):     Library: /data/app-lib/org.koreader.launcher-1
V/KOReader(10884):     Storage: /mnt/sdcard
V/KOReader(10884):   }
V/KOReader(10884): }
D/KOReader(10884): [MainActivity] onCreate()
D/KOReader(10884): [BaseActivity] onCreate()
D/KOReader(10884): [NativeGlue]: Creating: 0xb8f44dc0
D/KOReader(10884): [NativeThread]: starting
V/KOReader(10884): [NativeThread]: waiting for activity
D/KOReader(10884): [ClipboardHelper] Starting
D/KOReader(10884): [NetworkHelper] Starting
D/KOReader(10884): [PowerHelper] Starting
D/KOReader(10884): [ScreenHelper] Starting
D/KOReader(10884): [NativeSurfaceView] Starting
D/KOReader(10884): [NativeGlue]: Start: 0xb8f44dc0
D/KOReader(10884): [NativeGlue]: activityState=10
D/KOReader(10884): [MainActivity] onResume()
D/KOReader(10884): [NativeGlue]: Resume: 0xb8f44dc0
D/KOReader(10884): [NativeGlue]: activityState=11
D/KOReader(10884): [NativeGlue]: InputQueueCreated: 0xb8f44dc0 -- 0xb8f7df30
D/KOReader(10884): [NativeGlue]: APP_CMD_INPUT_CHANGED
D/KOReader(10884): [NativeGlue]: Attaching input queue to looper
D/KOReader(10884): [BaseActivity] onAttachedToWindow()
V/KOReader(10884): [NativeSurfaceView] surface created
D/KOReader(10884): [NativeGlue]: NativeWindowCreated: 0xb8f44dc0 -- 0xb8fc08e8
D/KOReader(10884): [NativeGlue]: APP_CMD_INIT_WINDOW
V/KOReader(10884): [NativeThread]: activity window ready.
V/KOReader(10884): [NativeSurfaceView] surface changed {
V/KOReader(10884):   width:  1080
V/KOReader(10884):   height: 1776
V/KOReader(10884): }
D/KOReader(10884): [NativeGlue]: WindowFocusChanged: 0xb8f44dc0 -- 1
V/KOReader(10884): [NativeThread]: activity gained focus.
V/KOReader(10884): [NativeThread]: launching LuaJIT assets

@pazos pazos force-pushed the pazos:inheritance branch from cca2276 to 7c9de0b Aug 13, 2019

@pazos pazos force-pushed the pazos:inheritance branch from 7c9de0b to bd29e86 Aug 13, 2019

@Frenzie Frenzie merged commit 2c8a125 into koreader:master Aug 13, 2019

1 of 2 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Frenzie added a commit to Frenzie/koreader that referenced this pull request Aug 13, 2019
Frenzie added a commit to koreader/koreader that referenced this pull request Aug 13, 2019
@pazos pazos referenced this pull request Aug 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can鈥檛 perform that action at this time.