Migrate codebase to Kotlin 1.6 #4120
Assignees
Labels
bug
End user-perceivable behaviors which are not desirable.
Impact: Medium
Moderate perceived user impact (non-blocking bugs and general improvements).
Issue: Needs Clarification
Indicates that an issue needs more detail in order to be able to be acted upon.
Work: High
It's not clear what the solution is.
Z-ibt
Temporary label for Ben to keep track of issues he's triaged.
Comments
Broppia
added
issue_type_bug
Impact: Medium
BenHenning
added
Issue: Needs Clarification
seanlip
added
bug
BenHenning
added a commit
that referenced
this issue
Apr 3, 2023
BenHenning
changed the title
This was referenced May 23, 2024
BenHenning
added a commit
that referenced
this issue
May 28, 2024
## Explanation Fixes #1535 Fixes part of #4120 This PR preps for the codebase-wide migration to Kotlin 1.6 (done in #4937) by first upgrading rules_kotlin from 1.5.0 alpha 2 to 1.5.0 beta 3. This upgrade, while small, produces a few nice benefits: - It removes the need for extra WORKSPACE dependency setup (thus addressing #1535). - It moves a bunch of rules_kotlin macros to new locations which results in changing nearly every ``BUILD.bazel`` file in the codebase. The changes are straightforward to review in isolation, so this PR acts as a means to help reduce the complexity of #4937 by pulling ahead these groups of ``BUILD.bazel`` changes while also moving rules_kotlin the smallest number of versions (ahead of the more major upgrades which will occur downstream). Note that this change is needed because: - We'll need to upgrade to a newer version of rules_kotlin in order to bring in Kotlin 1.6 support. - rules_kotlin moved these macros and using the old ones results in a _lot_ of build warnings that spam the local console when trying to build. Separately, this PR includes some minor trailing space cleanup in wiki/Oppia-Bazel-Setup-Instructions.md that was noticed when editing the file (since my local development environment auto-strips trailing spaces). ## Essential Checklist - [x] The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".) - [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation. - [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide). - [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)). - [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop". - [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)). ## For UI-specific PRs only N/A -- This should only affect build infrastructure, and barely impact resulting binary builds. --------- Co-authored-by: Adhiambo Peres <59600948+adhiamboperes@users.noreply.github.com> Co-authored-by: Sean Lip <sean@seanlip.org>
BenHenning
added a commit
that referenced
this issue
Jun 3, 2024
…5402) ## Explanation Fixes part of #4120 Fixes part of #1051 Similar to #5400, this brings forward changes that would otherwise go in #4937 to simplify the transition to Kotlin 1.6. Part of #4937 is introducing warnings-as-errors for both Kotlin and Java in order to reduce developer error, simplify the codebase, and minimize warnings when building (which can result in developer habits of ignoring warnings that might have real consequences to end users of the app). In order to keep the main migration PR smaller, this PR fixes all existing warnings and any new ones detected with the Kotlin 1.6 compiler that are not tied to Kotlin 1.5/1.6 API changes (those are part of #4937, instead). Fortunately, most of the changes could be brought forward into this PR. Specific things to note: - A few new issues were filed for SDK 33 deprecations caused, but not noted by, #5222): #5404, #5405, and #5406 and corresponding TODOs added. This PR opts for TODOs over actual fixes to minimize the amount of manual verification needed, and to try and keep the PR more focused on non-functional refactor changes (to reduce the risk as reverting this PR may be difficult if an issue is introduced). - A lot of the fixes were removing redundant casts or null checks. - The old mechanism we used for view models is deprecated, and had a lot of problems (partially documented in #1051). This PR moves the codebase over to directly injecting view models instead of using the view model provider (thus getting rid of true Jetpack view models entirely in the codebase). - We never used the Jetpack functionality, and we were leaking a lot of context objects that could theoretically result in memory leaks. - The migration of view models in this way has already been ongoing in the codebase; this PR just finishes moving the rest of them over to remove the deprecated JetPack view model reference. - Note that this doesn't actually change the scope of the view models, and in fact they should largely behave as they always have. - ``ObservableViewModel`` was subsequently updated, and may be something we could remove in the future now that it's no longer a Jetpack view model. - The old view model binding code was removed, along with its test file exemptions. It's no longer used now that the view models have been finished being migrated over to direct injection. - Some of the binding adapters didn't correctly correspond to their namespaced properties. I _think_ that the databinding compiler was still hooking them up correctly, but they produced build warnings that have now been addressed (specifically, 'app' is implied). Some other properties were using unusual namespaces, so these were replaced with 'app' versions for consistency & correctness. - Some cases where SAM interfaces could be converted to lambdas were also addressed (mainly for ``Observer`` callbacks in UI code). - ``DrawerLayout.setDrawerListener`` was replaced with calls to ``DrawerLayout.addDrawerListener`` since the former [is deprecated](https://developer.android.com/reference/androidx/drawerlayout/widget/DrawerLayout#setDrawerListener(androidx.drawerlayout.widget.DrawerLayout.DrawerListener)). This isn't expected to have a functional difference. - Some other minor control flow warnings were addressed (such as dead code paths). - ``when`` cases were updated to be comprehensive (as this is enforced starting in newer versions of Kotlin even for non-result based ``when`` statements). - Some unused variables were removed and/or replaced with ``_`` per Kotlin convention. - Some parameter names needed to be updated to match their override signatures. - One change in ``ExitSurveyConfirmationDialogFragment`` involved removing parsing a profile ID. Technically this is a semantic change since now a crash isn't going to happen if the profile ID is missing or incorrect, but that seems fine since the fragment doesn't even need a profile ID to be passed. - Some of the test activities were updated to bind a ``Runnable`` callback rather than binding to a method (just to avoid passing the unused ``View`` parameter and to keep things a bit simple binding-wise). - Some cases were fixed where variables were being shadowed due to reused names in deeper scopes. - There were some typing issues going on in tests with custom test application components. This has been fixed by explicitly declaring the application component types rather than them being implicit within the generated Dagger code. - ``getDrawable`` calls were updated to pass in a ``Theme`` since the non-theme version is deprecated. - Some Java property references were updated, too (i.e. using property syntax instead of Java getters when referencing Java code in Kotlin). - In some cases, deprecated APIs were suppressed since they're needed for testing purposes. - Mockito's ``verifyZeroInteractions`` has been deprecated in favor of ``verifyNoMoreInteractions``, so updates were made in tests accordingly. - ``ExperimentalCoroutinesApi`` and ``InternalCoroutinesApi`` have been deprecated in favor of a newer ``OptIn`` method (which can actually be done via kotlinc arguments, but not in this PR). Thus, they've been outright removed in cases where not needed, and otherwise migrated to the ``OptIn`` approach where they do need to be declared. - In some cases, Kotlin recommends using a ``toSet()`` conversion for iterable operations when it's more performant, so some of those cases (where noticed) have been addressed. - Some unused parameter cases needed to be suppressed for situations when Robolectric is using reflection to access them. - In some cases Android Studio would recommend transformation chain simplifications; these were adopted where obvious. - There are a few new TODOs added on #3616 as well, to clean up deprecated references that have been suppressed in this PR. - ``BundleExtensions`` was updated to implement its own version of the type-based ``getSerializable`` until such time as ``BundleCompat`` can be used, instead (per #5405). - A **lot** of nullability improvements needed to happen throughout the JSON asset loading process since there was a lot of loose typing happening there. - Some Kotlin & OkHttp deprecated API references were also updated to use their non-deprecated replacements. - ``NetworkLoggingInterceptorTest`` was majorly reworked to ensure that the assertions would actually run (``runBlockingTest`` was being used which is deprecated, and something I try to avoid since it's very difficult to write tests that use it correctly). My investigations showed that the assertions weren't being called, so these tests would never fail. The new versions will always run the assertions or fail before reaching them, and fortunately the code under test passes the assertions correctly. Ditto for ``ConsoleLoggerTest``. - Some parts of ``SurveyProgressController`` were reworked to have better typing management and to reduce the need for nullability management. - Some generic typing trickiness needed to be fixed ahead of the Kotlin version upgrade in ``UrlImageParser``. See file comments & links in those comments for more context. - ``BundleExtensionsTest`` had to be changed since ``getSerializableExtra`` is now deprecated. We also can't update the test to run SDK 33 since that requires upgrading Robolectric, and Robolectric can't be upgraded without upgrading other dependencies that eventually lead to needing to upgrade both Kotlin and Bazel (so it's a non-starter; this is a workaround until we can actually move to a newer version of Robolectric). - There was some minor code-deduplication & cleanup done in ``ClickableAreasImage``. - Some incorrect comments were removed in tests (to the effect of "using not-allowed-listed variables should result in a failure."). These seemed to have been copied from an earlier test, but the later tests weren't actually verifying that behavior so the comment wasn't correct. - An unused method was removed from ``ConceptCardRetriever`` (``createWrittenTranslationFromJson``) and some other small cleanup/consolidation work happened in that class. - Some stylistic changes were done in ``TopicController`` for JSON loading to better manage nullable situations, and to try and make the JSON loading code slightly more Kotlin idiomatic. Note that overall the PR has relied **heavily** on tooling to detect warnings to fix, and automated tests to verify that the changes have no side effects. Note also that this PR does not actually enable warnings-as-errors; that will happen in a downstream PR. ## Essential Checklist - [x] The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".) - [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation. - [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide). - [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)). - [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop". - [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)). ## For UI-specific PRs only N/A -- While this changes UI code, it should change very few UI behaviors and only failure cases for those it does affect. It's largely infrastructural-only and falls mainly under refactoring/cleanup work. --------- Co-authored-by: Adhiambo Peres <59600948+adhiamboperes@users.noreply.github.com> Co-authored-by: Sean Lip <sean@seanlip.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
No description provided.
The text was updated successfully, but these errors were encountered: