[Feature Request]: Migrate away from onBackPressed() #5404

BenHenning · 2024-05-22T22:18:54Z

Is your feature request related to a problem? Please describe.

API 33 (introduced in #5222) deprecated onBackPressed: https://developer.android.com/reference/android/app/Activity#onBackPressed().

#5402 helped reveal which activities need updating:

Describe the solution you'd like

As mentioned in https://developer.android.com/reference/android/app/Activity#onBackPressed(), the solution here seems to be to:

No longer override onBackPressed directly.
To use AndroidX (via androidx.activity.ComponentActivity#getOnBackPressedDispatcher()) to register a backward-compatible back pressed handler.
Migrate existing onBackPressed calls to the new dispatcher.

Describe alternatives you've considered

The only alternative is not using AndroidX, but this isn't compatible with Oppia Android since the new deprecation started in SDK 33 (and the app's min supported SDK in 21).

Additional context

Folks who work on this issue should ensure that back navigation behavior continues to work on changed activities & fragments, specifically:

Using the back arrow or x button in the toolbar (if present) correctly navigates back to the previous activity.
Using the system back button correctly navigates to the previous activity.
If there's any special handling in the changed activity/fragment for tablets, navigation and split fragment displays should be verified on a tablet version of Android.

NB: The first work on this issue will require updating AndroidX core to a newer version.

The text was updated successfully, but these errors were encountered:

…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 <[email protected]> Co-authored-by: Sean Lip <[email protected]>

Tejas-67 · 2024-06-15T15:05:01Z

hey @BenHenning, I would like to work in this issue. please assign this to me.

adhiamboperes · 2024-06-15T15:08:13Z

Hi @Tejas-67, as you can see from the description, the issue has been broken down into activities. Please select 3-5 activities at a time, and raise a PR for those.

Please indicate which ones you are working on.

uphargaur · 2024-06-22T21:46:48Z

Hi @adhiamboperes i would like to work on this as @Tejas-67 is already working on them , can you please assign below 3 activities to me :-

app/src/main/java/org/oppia/android/app/settings/profile/ProfileEditActivity
app/src/main/java/org/oppia/android/app/settings/profile/ProfileEditActivityPresenter
app/src/main/java/org/oppia/android/app/topic/questionplayer/QuestionPlayerActivityPresenter
app/src/main/java/org/oppia/android/app/walkthrough/end/WalkthroughFinalFragmentPresenter

…ressed() - Replaced the deprecated super.onBackPressed() method with onBackPressedDispatcher.onBackPressed() - Ensures compatibility with newer Android SDK versions - Updated necessary activities and fragments to use the new back pressed dispatcher - Verified that back navigation works correctly with the toolbar back arrow, system back button, and tablet-specific behaviors Activities and fragments corrected: - app/src/main/java/org/oppia/android/app/settings/profile/ProfileEditActivity - app/src/main/java/org/oppia/android/app/settings/profile/ProfileEditActivityPresenter - app/src/main/java/org/oppia/android/app/topic/questionplayer/QuestionPlayerActivityPresenter - app/src/main/java/org/oppia/android/app/walkthrough/end/WalkthroughFinalFragmentPresenter Fixes oppia#5404

uphargaur · 2024-06-24T08:27:39Z

@BenHenning @adhiamboperes can i work complete this task in all the activities

adhiamboperes · 2024-06-24T08:55:46Z

@uphargaur it looks like @Tejas-67 is working on all the activities as per #5427. PTAL at other good first issues here: https://github.com/oppia/oppia-android/issues?q=is%3Aissue+is%3Aopen+label%3A%22good+first+issue%22. A similar issue is #5405

uphargaur · 2024-06-26T14:06:15Z

@adhiamboperes okay sure, i would work on issue #5405

Vishwajith-Shettigar · 2024-08-11T03:58:42Z

Hi @uphargaur, would you like to work on this ?

uphargaur · 2024-08-15T21:21:45Z

@Vishwajith-Shettigar i am working on another issue ,once it get complete i will love to work on it

dattasneha · 2024-08-25T04:45:13Z

Hi @adhiamboperes, I'd like to work on the remaining activities, excluding the 4 being handled in PR #5441. I'm creating a draft PR to share my approach.

/cc @Vishwajith-Shettigar

## Explanation  Fixes part of #5404 This PR migrates deprecated `onBackPressed` usage to `OnBackPressedDispatcher` callback in the following activities and presenters. - AdministratorControlsActivity - AppVersionActivity - ProfileAndDeviceIdActivity - MarkChaptersCompletedActivity - MarkStoriesCompletedActivity - MarkTopicsCompletedActivity - ReadingTextSizeActivityPresenter - ExplorationActivityPresenter - ResumeLessonActivityPresenter ## 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  If your PR includes UI-related changes, then: - Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes - For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see [RTL guide](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines)) - Add a video showing the full UX flow with a screen reader enabled (see [accessibility guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide)) - For PRs introducing new UI elements or color changes, both light and dark mode screenshots must be included - Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing --------- Co-authored-by: Adhiambo Peres <[email protected]>

github-actions · 2024-10-03T10:54:27Z

The issue is reopened because of the following unresolved TODOs:

oppia-android/app/src/main/java/org/oppia/android/app/topic/revisioncard/RevisionCardActivity.kt

Line 118 in 20c4cad

// TODO(#5404): Migrate to a back pressed dispatcher.

oppia-android/app/src/main/java/org/oppia/android/app/topic/revisioncard/RevisionCardActivityPresenter.kt

Line 86 in 20c4cad

@Suppress("DEPRECATION") // TODO(#5404): Migrate to a back pressed dispatcher.

TanishMoral11 · 2024-10-07T22:20:33Z

Hey @BenHenning , I would like to work on this issue. please assign this to me.
-> app/src/main/java/org/oppia/android/app/settings/profile/ProfileEditActivity

adhiamboperes · 2024-10-08T06:50:55Z

Hi @TanishMoral11, unfortunately, all the action items for this issue have been taken up.

TanishMoral11 · 2024-10-08T14:12:45Z

Hi @adhiamboperes ,
No problem at all! If this issue needs to be assigned in the future, please feel free to send me a message. I'd be happy to help.

Thanks!

…5548)  ## Explanation  Fixes #5404 This PR migrates deprecated `onBackPressed `usage to `OnBackPressedDispatcher` callback in the RevisonCardActivity and RevisionCardActivityPresenter. ## 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  If your PR includes UI-related changes, then: - Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes - For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see [RTL guide](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines)) - Add a video showing the full UX flow with a screen reader enabled (see [accessibility guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide)) - For PRs introducing new UI elements or color changes, both light and dark mode screenshots must be included - Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing

…ies (oppia#5526)  ## Explanation  Fixes oppia#5404 This PR migrates deprecated `onBackPressed` usage to `OnBackPressedDispatcher` callback in the following activities and presenters. - ProfileEditActivity - ProfileEditActivityPresenter - QuestionPlayerActivityPresenter - WalkthroughFinalFragmentPresenter ## 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  If your PR includes UI-related changes, then: - Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes - For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see [RTL guide](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines)) - Add a video showing the full UX flow with a screen reader enabled (see [accessibility guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide)) - For PRs introducing new UI elements or color changes, both light and dark mode screenshots must be included - Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing --------- Co-authored-by: Adhiambo Peres <[email protected]> Co-authored-by: Mr. 17 <[email protected]>

…ity (oppia#5548)  ## Explanation  Fixes oppia#5404 This PR migrates deprecated `onBackPressed `usage to `OnBackPressedDispatcher` callback in the RevisonCardActivity and RevisionCardActivityPresenter. ## 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  If your PR includes UI-related changes, then: - Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes - For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see [RTL guide](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines)) - Add a video showing the full UX flow with a screen reader enabled (see [accessibility guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide)) - For PRs introducing new UI elements or color changes, both light and dark mode screenshots must be included - Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing

BenHenning added enhancement End user-perceivable enhancements. triage needed labels May 22, 2024

BenHenning mentioned this issue May 23, 2024

Fixes part of #4120, part of #1051: Fix a lot of build-time warnings #5402

Merged

6 tasks

seanlip removed the triage needed label May 29, 2024

adhiamboperes added the good first issue This item is good for new contributors to make their pull request. label Jun 15, 2024

adhiamboperes mentioned this issue Jun 20, 2024

Fix #5404 : Migrate onBackPressed #5427

Closed

19 tasks

uphargaur mentioned this issue Jun 22, 2024

Fix #5404: Migrate from super.onBackPressed() to onBackPressedDispatcher.onBackPressed() #5441

Closed

6 tasks

adhiamboperes assigned Tejas-67 Jul 4, 2024

Vishwajith-Shettigar unassigned Tejas-67 Aug 16, 2024

dattasneha mentioned this issue Aug 26, 2024

Fix part of #5404: Migrate away from onBackPressed #5521

Merged

6 tasks

dattasneha mentioned this issue Sep 5, 2024

Fix #5404: Migrate away from onBackPressed for remaining activities #5526

Merged

6 tasks

adhiamboperes closed this as completed in #5526 Oct 3, 2024

adhiamboperes closed this as completed in 20c4cad Oct 3, 2024

github-actions bot reopened this Oct 3, 2024

dattasneha mentioned this issue Oct 6, 2024

Fix #5404: Migrate away from onBackPressed for RevisionCardActivity #5548

Merged

6 tasks

adhiamboperes assigned dattasneha Oct 8, 2024

adhiamboperes closed this as completed in #5548 Oct 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature Request]: Migrate away from onBackPressed() #5404

[Feature Request]: Migrate away from onBackPressed() #5404

BenHenning commented May 22, 2024 •

edited by adhiamboperes

Loading

Tejas-67 commented Jun 15, 2024

adhiamboperes commented Jun 15, 2024

uphargaur commented Jun 22, 2024 •

edited

Loading

uphargaur commented Jun 24, 2024

adhiamboperes commented Jun 24, 2024

uphargaur commented Jun 26, 2024

Vishwajith-Shettigar commented Aug 11, 2024

uphargaur commented Aug 15, 2024

dattasneha commented Aug 25, 2024

github-actions bot commented Oct 3, 2024

TanishMoral11 commented Oct 7, 2024

adhiamboperes commented Oct 8, 2024

TanishMoral11 commented Oct 8, 2024

[Feature Request]: Migrate away from onBackPressed() #5404

[Feature Request]: Migrate away from onBackPressed() #5404

Comments

BenHenning commented May 22, 2024 • edited by adhiamboperes Loading

Is your feature request related to a problem? Please describe.

Describe the solution you'd like

Describe alternatives you've considered

Additional context

Tejas-67 commented Jun 15, 2024

adhiamboperes commented Jun 15, 2024

uphargaur commented Jun 22, 2024 • edited Loading

uphargaur commented Jun 24, 2024

adhiamboperes commented Jun 24, 2024

uphargaur commented Jun 26, 2024

Vishwajith-Shettigar commented Aug 11, 2024

uphargaur commented Aug 15, 2024

dattasneha commented Aug 25, 2024

github-actions bot commented Oct 3, 2024

TanishMoral11 commented Oct 7, 2024

adhiamboperes commented Oct 8, 2024

TanishMoral11 commented Oct 8, 2024

BenHenning commented May 22, 2024 •

edited by adhiamboperes

Loading

uphargaur commented Jun 22, 2024 •

edited

Loading