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

Migration, WebClient & Lint Fixes #16

Merged
merged 22 commits into from
May 16, 2022

Conversation

nodinosaur
Copy link
Contributor

@nodinosaur nodinosaur commented May 3, 2022

  1. Migration to ViewBinding from Kotlin Synthetic (deprecated)
  2. Adding a dedicated WebClient to help clean up BrowserActivityNative
  3. Migrate package names to reflect the new App Id (and something more OSS)
  4. Update the About info screen
  5. Bump Crashlytics dependencies (my fork has a google-services.json for dev)
  6. A good few Lint error fixes

Happy to huddle if it helps

(please test the app by run the app for a good length of time)

Copy link
Owner

@TheTimeWalker TheTimeWalker left a comment

Choose a reason for hiding this comment

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

This looks pretty good! For now I've only looked through the code, mentioning some nitpicks and specifically the OKHTTP bump which would break KitKat devices. I wlil be trying on real devices soon just to be sure everything is stable

@TheTimeWalker
Copy link
Owner

While testing out I came across a crash loop on one of my Oreo devices where after trying to start up the camera but failing, it then gives a NullPointerException at CrashlyticsDebugTree. Haven't gotten into debugging this yet.

Fatal Exception: java.lang.RuntimeException: Unable to create service xyz.wallpanel.app.network.WallPanelService: java.lang.NullPointerException
       at android.app.ActivityThread.handleCreateService(ActivityThread.java:3376)
       at android.app.ActivityThread.-wrap4()
       at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1701)
       at android.os.Handler.dispatchMessage(Handler.java:106)
       at android.os.Looper.loop(Looper.java:164)
       at android.app.ActivityThread.main(ActivityThread.java:6523)
       at java.lang.reflect.Method.invoke(Method.java)
       at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:438)
       at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:857)
Caused by java.lang.NullPointerException
       at xyz.wallpanel.app.utils.CrashlyticsDebugTree.log(CrashlyticsDebugTree.kt:17)
       at timber.log.Timber$Tree.prepareLog(Timber.java:532)
       at timber.log.Timber$Tree.e(Timber.java:450)
       at timber.log.Timber$1.e(Timber.java:306)
       at timber.log.Timber.e(Timber.java:83)
       at xyz.wallpanel.app.modules.CameraReader.startCamera(CameraReader.kt:132)
       at xyz.wallpanel.app.network.WallPanelService.configureCamera(WallPanelService.kt:381)
       at xyz.wallpanel.app.network.WallPanelService.onCreate(WallPanelService.kt:177)
       at android.app.ActivityThread.handleCreateService(ActivityThread.java:3366)
       at android.app.ActivityThread.-wrap4()
       at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1701)
       at android.os.Handler.dispatchMessage(Handler.java:106)
       at android.os.Looper.loop(Looper.java:164)
       at android.app.ActivityThread.main(ActivityThread.java:6523)
       at java.lang.reflect.Method.invoke(Method.java)
       at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:438)
       at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:857)

@nodinosaur
Copy link
Contributor Author

Fixed a potential NPE

@nodinosaur
Copy link
Contributor Author

I also see camera errors on the Nexus 9 with SlimRom unfortunately, these JNI bugs are difficult to trace

@TheTimeWalker
Copy link
Owner

Ah, sorry about that. I didn't mean the camera issue specifically. I just meant the CrashlyticsDebugTree.kt as I assumed with the new utils file in this PR, it could potentially also crash with NPE on other exceptions.

@nodinosaur
Copy link
Contributor Author

Yep, that has been addressed with c0a2f09

@TheTimeWalker
Copy link
Owner

This looks very good! The only issue left that I have found is that on KitKat, it seems like the WebView does not get bound on the layout properly. There's only a white page unfortunately regardless on what URL I have set. By debugging, I see the pages do load in the background. I assume this has something to do with the new ViewBinding that replaced the deprecated synthetic way. I personally haven't found a solution yet, maybe you have an idea how to solve this 😄

If that could be solved then this would LGTM

@nodinosaur
Copy link
Contributor Author

I just checked the Kitkat scenario against the TheTimeWalker/wallpanel-android master branch and see exactly the same result. Emulator: Nexus 7 KitKat
kitkat_nexus7

@TheTimeWalker
Copy link
Owner

Oops! Sorry, it seems like I've been confused all along! I've been mentioning the wrong Android version where I found the issue, sorry about that!

The white screen issue mentioned is actually on Lollipop 5.1 and not Kitkat 4.4. Maybe you could also quickly check out if it's running fine on Android 5.1 emulation for you? Because that's a version we definitely will be keeping support for a while now, so it would be good that it doesn't regress there. That's what I see on Lollipop at least with every URL:
image

Just to mention Kitkat, what you see is pretty much how it should be. Because KitKat runs with Chrome v30, it's unfortunately unsupported by Home Assistant. WallPanel can still be used for other dashboards however! :)

@nodinosaur
Copy link
Contributor Author

Lollipop works fine:
Emulator of Nexus 9 Lollipop
Emulator_and_Details

@TheTimeWalker
Copy link
Owner

API 25 is Android 7.1.1 Nougat if the device name is correct 😄

@nodinosaur
Copy link
Contributor Author

Doh! Yes, You are right.
Attached Lollipop, not sure about WebView version, but there is some rendering
Nexus9_API_22

and
Nexus9_API_22_Lollipop5-1

But I tested it with Wallpanel.xyz:

Nexus9_API_22_wallpanel

@TheTimeWalker
Copy link
Owner

Thanks, I guess I'll chalk it up on my setup doing something weird in that regard.
At least locally on devices it seems to work pretty fine as it has before. I haven't noticed anything severe, so this looks good to me!

Thank you very much for this PR and for the work. This definitely helped me quite a lot since quite a lot of things were something I planned to do, but didn't come around yet. I really appreciate the whole cleanup and porting done to modernize the code stack a bit further.

@TheTimeWalker TheTimeWalker merged commit d75d65f into TheTimeWalker:master May 16, 2022
@starkillerOG
Copy link

@TheTimeWalker could you release a new version?
I would like to test these changes and see if they improve the crashes I experiance.

@nodinosaur
Copy link
Contributor Author

This is more of a code package refactor and (in my case) some tweaks to crash logging rather than a series of fixes.
But please check out the code, drop your google-services.json into the WallPanelApp directory, compile, deploy it to your test device and see how you get on.

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.

3 participants