Fix onBackPressed() deprecation In Card Browser #17594
Conversation
david-allison
added
the
Needs Author Reply
There was a problem hiding this comment.
The goal of the new predictive back navigation is to have distinct back callbacks for any actions you may want on BACK pressed and to enable/disable those callbacks dynamically so the system knows ahead of time if you are managing the back action or the system should handle it(and show animations).
Using OnBackPressedCallback and permanently have it enabled is equivalent with the current onBackPressed() so the changes in this PR are not an improvement. I would rather keep the deprecated onBackPressed callback present because it stands out as needing to be fixed, instead of the callback which is going to make it harder to see there's an issue.
|
Hi David, I will address the review points |
Prince-kushwaha
force-pushed
the
deprecation
branch
from
7487ddf to
08e0bf6
Compare
david-allison
added
Needs Review
and removed
Needs Author Reply
There was a problem hiding this comment.
I don't feel this code matches the logic of the previous implementation. It feels like it should be the following (please disagree, you're closer to the code than I am)
- If the drawer is open, the drawer should take prioirty
- If multiselect is open, it should be closed
If neither happen, there shouldn't be an active back dispatcher, and the activity should close normally, with the provided result set somewhere before the back button was pressed (fail + reload required should be the default).
A test here would also be great, regarding the exit + exit intent.
| val data = Intent() | ||
| // Add reload flag to result intent so that schedule reset when returning to note editor | ||
| data.putExtra(NoteEditor.RELOAD_REQUIRED_EXTRA_KEY, reloadRequired) | ||
| closeCardBrowser(RESULT_OK, data) |
There was a problem hiding this comment.
This seems... unusual. It feels like it shouldn't be in the callback and should be the default of pressing 'back', unless there's documentation to disagree.
There was a problem hiding this comment.
this back callback method responsible for close of CardBrowser.
There was a problem hiding this comment.
As stated twice, this isn't correct
There was a problem hiding this comment.
First of all the drawer back state is handled in the super class. I defined there a back callback that should be exposed to subclasses to add to the back dispatcher. Also note that the order of adding the callbacks is important.
Secondly, there shouldn't be a "default" back callback because this would defeat the entire purpose of the new system(to know ahead of time the status of any back actions).
lukstbit
added
the
Needs Author Reply
Prince-kushwaha
force-pushed
the
deprecation
branch
from
08e0bf6 to
cf348d6
Compare
Prince-kushwaha
force-pushed
the
deprecation
branch
from
cf348d6 to
2df6e67
Compare
|
Hi @david-allison ,i defined the order of callback method ->drawerBackCallback->multiSelectBackCallback->closeCardBrowserBackCallback. and it is matching the logic of the previous implementation (onBackPressed). Below adding the testing video of it WhatsApp.Video.2024-12-27.at.6.38.24.PM.mp4 |
| @@ -284,6 +286,31 @@ open class CardBrowser : | |||
| private var shouldRestoreScroll = false | |||
| private var postAutoScroll = false | |||
|
|
|||
| private val drawerBackCallback = | |||
There was a problem hiding this comment.
Why is this in CardBrowser? This should be a responsibility of the drawer
| val data = Intent() | ||
| // Add reload flag to result intent so that schedule reset when returning to note editor | ||
| data.putExtra(NoteEditor.RELOAD_REQUIRED_EXTRA_KEY, reloadRequired) | ||
| closeCardBrowser(RESULT_OK, data) |
There was a problem hiding this comment.
As stated twice, this isn't correct
| val drawerListener = | ||
| object : ActionBarDrawerToggle( | ||
| this, | ||
| drawerLayout, | ||
| R.string.drawer_open, | ||
| R.string.drawer_close, | ||
| ) { | ||
| override fun onDrawerClosed(drawerView: View) { | ||
| super.onDrawerClosed(drawerView) | ||
| drawerBackCallback.isEnabled = false | ||
| } | ||
|
|
||
| override fun onDrawerOpened(drawerView: View) { | ||
| super.onDrawerOpened(drawerView) | ||
| drawerBackCallback.isEnabled = true | ||
| } | ||
| } | ||
| drawerLayout.addDrawerListener(drawerListener) |
There was a problem hiding this comment.
This shouldn't be the responsibility of the Card Browser
Purpose / Description
Fix onBackPressed() deprecation In Card Browser
Fixes
How Has This Been Tested?
Physical Device
WhatsApp.Video.2024-12-27.at.6.38.24.PM.mp4
Checklist
Please, go through these checks before submitting the PR.