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

OnBackpress deprecation handled #13586

Conversation

MudssirAhmed
Copy link

@MudssirAhmed MudssirAhmed commented Apr 9, 2023

Pull Request template

Purpose / Description

deprecation: compileSdk 33 issues, In this PR onBackPress depreciation, is handled.

Fixes

I change onBackPress with onBackPressedDispatcher.
Issue Link

Approach

onBackPress handled with onBackPressedDispatcher.

How Has This Been Tested?

Manual test in my Handset.
I test manually by going to the screen.

Checklist

Please, go through these checks before submitting the PR.

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

@welcome
Copy link

welcome bot commented Apr 9, 2023

First PR! 🚀 We sincerely appreciate that you have taken the time to propose a change to AnkiDroid! Please have patience with us as we are all volunteers - we will get to this as soon as possible.

Copy link
Contributor

@pratyaksh1610 pratyaksh1610 left a comment

Choose a reason for hiding this comment

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

Template ignored .
Please fill the PR template, else the PR will get closed.

@MudssirAhmed
Copy link
Author

Template ignored . Please fill the PR template, else the PR will get closed.

I'm new here, Thanks for the mention!

@mikehardy
Copy link
Member

Really sorry this has sat so long! It looks like the correct direction to go to fix the problem correctly.
It also has some conflicts because we (those that have reviewer power...) did not get to it in a timely way, our fault, apologies.

I'll see if I can fix it up against the current API34 compile target in dependency-updates branch and get this landed as part of the effort to resolve #14558

@mikehardy
Copy link
Member

mikehardy commented Oct 19, 2023

related commit 9ec94ed

@BrayanDSO
Copy link
Member

Marking as Needs a new dev

@BrayanDSO BrayanDSO added the Needs a new dev For PR that were good start but the original dev' left. Any dev can take over and finish it. label Nov 3, 2023
@lukstbit
Copy link
Member

lukstbit commented Nov 3, 2023

From what I understood about predictive back navigation there's a lot more work to be done to properly implement it(check this answer from one of the engineers working on it), it's not as simple as adding onBackPressedDispatcher.addCallback() and deleting onBackPressed().

As an example, I implemented this predictive back navigation in AnkiPackageImporterFragment(at least how I think it should be implemented).

I would recommend that we close this PR + the linked api 33 deprecation issue and we move to track it in #14558 .

@mikehardy
Copy link
Member

I would recommend that we close this PR + the linked api 33 deprecation issue and we move to track it in #14558

Agreed

@mikehardy mikehardy closed this Nov 3, 2023
@BrayanDSO
Copy link
Member

I gave a ride to testing the predictive stuff. I guess that we could use a simple version of it while this thing actually gets out to the general public (it is still restricted to a developer option) and more activities are updated to good Android practices.

@lukstbit
Copy link
Member

lukstbit commented Nov 4, 2023

I still think keeping the deprecated onBackPressed() method along with the warning is better to indicate that work needs to be done vs adding a simple callback, removing the warnings and realizing when the feature becomes available that we need to do work to actually implement it.

After a quick look I think we could resolve like half of the deprecations now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs a new dev For PR that were good start but the original dev' left. Any dev can take over and finish it. New contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants