Skip to content
This repository has been archived by the owner on May 22, 2021. It is now read-only.

Cleanup Android project (Minor refactorings, etc.) #1244

Merged
merged 9 commits into from
Apr 4, 2019

Conversation

christxph
Copy link
Contributor

Cleaned up a few files related to the Android project to follow common Android/Kotlin conventions.

@dannycoates dannycoates requested a review from fzzzy March 13, 2019 23:40
@fzzzy
Copy link
Contributor

fzzzy commented Mar 14, 2019

Thank you very much! I'll take a look next week.

Copy link
Contributor

@fzzzy fzzzy left a comment

Choose a reason for hiding this comment

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

These look great, thank you very much for doing them. However if I pull the branch and run it on my S8, nothing in the webview works for some reason -- tapping the plus button brings up the file selection dialog properly, but choosing a file does not cause it to be added. Also tapping the sign in/up button does nothing. Can you confirm if things are working for you in the emulator or on a device? There could just be something wrong with my build for some reason.

@christxph
Copy link
Contributor Author

Thanks for the review, I will look into this later today and update the PR.

Instead of holding a nullable reference to the WebView, we are now
accessing the WebView using the view binding utility of Kotlin's
Android Extensions.

Further reading:
https://kotlinlang.org/docs/tutorials/android-plugin.html
This updates the Kotlin plugin to 1.3.21 and the Gradle plugin to 3.3.2
Layout files should generally have as few nested layers as possible,
because every layer affects the performance.
It is way safer to construct a JSON string using classes that are
meant for doing that, instead of concatenating raw strings.
@christxph
Copy link
Contributor Author

Hi,
unfortunately I am currently experiencing the same behavior (sign-up button doesn't respond and choosing a file does not cause it to be added) on both, the latest master and this branch (android-cleanup).
Can you build the latest master and use the two mentioned features as expected?

Also rebased on the latest master.

@fzzzy
Copy link
Contributor

fzzzy commented Mar 21, 2019

@christxph Yes, I see master being broken also. @dannycoates Did anything happen on master recently that might have broken android?

@christxph
Copy link
Contributor Author

christxph commented Mar 21, 2019

Alright, I will add an instrumentation test to this branch that verifies that the sign up button does anything so that it's easier to test this issue.

Edit: Seems like this is already being done in #1266

@fzzzy
Copy link
Contributor

fzzzy commented Mar 27, 2019

I'm going to take another swing at getting this branch running tomorrow.

@fzzzy
Copy link
Contributor

fzzzy commented Apr 4, 2019

Master is fixed now, and this branch looks good. Thank you very much!

@fzzzy fzzzy merged commit 48b5d85 into mozilla:master Apr 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants