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

Update where we setup BackHandler for Android #470

Merged
merged 3 commits into from
Feb 27, 2023
Merged

Conversation

kierse
Copy link
Collaborator

@kierse kierse commented Feb 27, 2023

Problem

#433 (part of efforts to improve KMP support) renamed BaseNavigableCircuitContent to NavigableCircuitContent, overloading the existing Android-specific method with the same name. The only difference between the two method signatures was an additional param in the android version (related to setting up the BackHandler).

As a result of this change existing uses of NavigableCircuitContent on Android started calling the wrong method, bypassing the logic that configured the system back handler.

Fix

Rather than change the Android overload in some way, or require that Android callers provide the extra parameter (a friction when 99% of uses will want the default), I opted to move the back handler setup logic into the Android-specific rememberCircuitNavigator, and remove the Android overload entirely.

@kierse kierse marked this pull request as ready for review February 27, 2023 18:01
@kierse kierse marked this pull request as draft February 27, 2023 18:02
@kierse kierse marked this pull request as ready for review February 27, 2023 18:18
@kierse kierse requested review from ZacSweers and jpetote February 27, 2023 18:19
Copy link
Collaborator

@ZacSweers ZacSweers left a comment

Choose a reason for hiding this comment

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

Nice solution 👍

Copy link
Collaborator

@ZacSweers ZacSweers left a comment

Choose a reason for hiding this comment

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

Actually, I think rememberSaveableBackstack may also get moved to common eventually so this may still be an issue in the future. I do think this is a better place for this API though 👍. Maybe when that time comes we just make this an expect/actual setup

@kierse kierse added this pull request to the merge queue Feb 27, 2023
Merged via the queue into main with commit 7c9e58f Feb 27, 2023
@kierse kierse deleted the ke/fix-back-handler branch February 27, 2023 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants