-
Notifications
You must be signed in to change notification settings - Fork 313
Update libraries and fix crashes #1733
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
Conversation
…d context instead of baseContext. Also making sure to call the right styles on manifest
…lled in super() instantiation before the mTokenizer variable is instantiated
Generated by 🚫 Danger |
|
📲 You can test the changes from this Pull Request in Simplenote Android by scanning the QR code below to install the corresponding build.
|
… we should use BiometricPrompt
- upgraded AGP (android gradle plugin) - updated all test dependencies, mockito, androidx.test - updated a test using an old syntax that would make the build fail - bumped compileSdk to 36 - handling onBackPressed overrides calling superclass in NoteEditorActivity and NewCredentialsActivity
…orted as a path in the manifest declaration
| protected InputFilter[] filters = null; | ||
| protected TextView topMessage = null; | ||
|
|
||
| @SuppressLint("RestrictedApi") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this to avoid linter complaining for now, but tracking separately here #1734
- Issue: 11 unit tests were failing on CI (Buildkite) with java.lang.ClassNotFoundException: org.wordpress.passcodelock.AbstractAppLock - Root Cause: CI was running testRelease which enables ProGuard/R8 minification, while local tests run in debug mode without minification - Specific Problem: All failing tests used AnalyticsTracker.track() which triggered instantiation of SimplenoteAppLock → DefaultAppLock → AbstractAppLock, but R8 was stripping the PasscodeLock classes
PasscodeLock/build.gradle
Outdated
| releaseTest { | ||
| initWith release | ||
| minifyEnabled false | ||
| shrinkResources false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
had to create a different task to run tests on Release with minify disabled, since otherwise there was a ClassDefNotFound error on the AbstractAppLock class, and R8 was stripping the PasscodeLock classes. Kudos to Claude Code for helping me out on this one, see 8c792b4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the Simplenote Android app's dependencies and fixes crashes, with the primary goal of supporting Android 15. Key changes include upgrading Gradle to 8.14.3, updating Android target SDK to 36, modernizing dependencies, and fixing various compatibility issues.
- Updates Gradle wrapper and build configurations for Android 15 support
- Modernizes dependencies including AndroidX libraries, testing frameworks, and other third-party libraries
- Fixes UI compatibility issues by replacing deprecated
android:tintwithapp:tint
Reviewed Changes
Copilot reviewed 58 out of 60 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| gradlew.bat, gradlew | Updated Gradle wrapper scripts with improved error handling and latest template |
| gradle/wrapper/gradle-wrapper.properties | Upgraded Gradle to version 8.14.3 |
| Wear/build.gradle | Updated Wear module dependencies and compile SDK |
| Simplenote/build.gradle | Major dependency updates and Java 11 migration |
| PasscodeLock module | Updated dependencies and added compatibility annotations |
| Layout XML files | Replaced deprecated android:tint with app:tint |
| Java/Kotlin source files | Fixed crashes and deprecated API usage |
Comments suppressed due to low confidence (1)
Simplenote/src/test/java/com/automattic/simplenote/viewmodels/CollaboratorsViewModelTest.kt:166
- This test assertion may not properly verify the intended behavior. The test should verify that the UI state remains unchanged when listening is stopped, but the current assertion compares against an initial state that may not represent the expected final state.
assertEquals(initialState, viewModel.uiState.value)
Simplenote/src/main/java/com/automattic/simplenote/authentication/NewCredentialsActivity.kt
Show resolved
Hide resolved
Simplenote/src/main/java/com/automattic/simplenote/NoteEditorActivity.java
Show resolved
Hide resolved
|
👋 @mzorz and just an FYI on my part that I started with reviewing this change today, but I didn't manage to finish with it. I'll continue tomorrow. ⏳ PS: In the meanwhile I am not sure if my review should be enough here, lots of changes and lots of library updates. On the other hand, it might be okay if we move faster (than usual) and only do a final check right at the end, when we're done with all the changes. What we want is to at least know that next time we'll need to release an update, we should be set. Wdyt? 🤔 |
I'm fine either way, the main reason I split the PRs and asked for your review in this one is because this PR updates libraries and fixes crashes on the base PR (the one you wrote). The actual functional changes are on the other PRS (targeting SDK 35 plus changing the cwac:anddown library which is not 16kb-page-size compatible). If you don't spot anything weird on this PR, this one should be good to go (since the PR we merged actually crashes the app anyway, this would be already putting things in a better place). 😅 I'll have someone else review and smoke test the rest of the changes too 👍 |
|
👋 @mzorz and I am on it! 🏃 💨 💨 💨
Got it! 👍
True... 😅
Awesome, thanks! 🙇 |
ParaskP7
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋 @mzorz !
I have reviewed and tested this PR as per the instructions, great job updating all these dependencies! 🌟 x 🌟 ^ 🌟
I have left a couple of warnings (
|
|
||
| private fun handlePasswordReset(uri: Uri) { | ||
| val redirectParam = uri.getQueryParameter("redirect") | ||
| if (redirectParam == "simplenote://launch") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question (❓): The logic within both if/else seems the same to me, what that done on purpose? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oof good catch, fixed in 2589709 - it should just launch the app when capturing a password reset link and the app is installed (this to do the same thing it was doing before, no behavior change)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks for the fix @mzorz , so in the end, no query parameters no nothing needed here, right? 😊
Simplenote/src/main/java/com/automattic/simplenote/NoteEditorActivity.java
Show resolved
Hide resolved
Simplenote/src/test/java/com/automattic/simplenote/viewmodels/CollaboratorsViewModelTest.kt
Show resolved
Hide resolved
|
FYI: I was able to reproduce this exact problem while testing this PR, but only did it once, then I just couldn't any more... 🤷 |
|
Thanks a lot for all this feedback @ParaskP7 ! 🙌 really appreciated.
Thanks for flagging - all of that is being addressed either on the PR changing to Just so we're on the same page: for this PR, I concentrated on:
This PR is already large enough with that aim (upgrading the libraries versions and updating code to match behavior changes only). So, I fixed some of the comments that were related to the changes above, but all further suggestions I think are better to be handled in separate PRs.
Thanks for the pointers here! Also for instrumented tests, I agree it'd be nice to bring them back to life, yet it exceeds the aim of updating the libraries only. I also can imagine integrated tests will change after the changes we're making due to the edge-to-edge UI changes by targeting SDK 35, so let's keep that task isolated for the time being. I think the only really blocking thing in this PR should be this. Let's continue the discussion there 👍 |
|
💯 @mzorz and I agree with all that you said above, thank you so much for applying all and any of my suggestions! 🙇 FYI: I've resolved all comments but did left the ones we chose to differ as unresolved, just to make it more visible going forward, when and if we decide to do something about them. 🤞
Yes, let's do that and then merge this! 🚀 |
… unneeded in gradle as suggested by @ParaskP7
|
Thanks once more for you thorough review @ParaskP7 ! We'll take over the things that were left unresolved in subsequent PRs as agreed 🙌 |
Note: this PR sits on top of #1731
Fix
This app was not being consistently maintained so I had to bring a lot of libraries up to date and do some changes to make it all work as expected (also fixes a couple crashes, one of them listed here from the previous PR)
Test
Smoke test the app, create entires, verify they sync on another platform, check against the production app.
Review
@ParaskP7
Release
Maintenance update - support for Android 15