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

Add predictive back support for iOS #1432

Open
wants to merge 1 commit into
base: jb-main
Choose a base branch
from

Conversation

LukasAnda
Copy link

@LukasAnda LukasAnda commented Jul 5, 2024

Added predictive back support for iOS which in turn allows it to work in navigation

Testing

I tested thoroughly on a sample project on both iOS and Android

This should be tested by QA

Release Notes

Feature - iOS

  • Added support for Predictive back, now swiping back on iOS correctly shows progress to the previous screen

Google CLA

You need to sign the Google Contributor’s License Agreement at https://cla.developers.google.com/.
This is needed since we synchronise most of the code with Google’s AOSP repository. Signing this agreement allows us to synchronise code from your Pull Requests as well.

@kropp kropp requested a review from MatkovIvan July 5, 2024 12:16
Copy link
Member

@MatkovIvan MatkovIvan left a comment

Choose a reason for hiding this comment

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

Thanks for a contribution.

But this approach won't work and cannot be merged. A few issues here:

  • Changes in Android in fork
  • A few new common public APIs
  • Adding APIs in modules that they shouldn't belong to

All of these are blockers - we cannot do such changes in the fork (we do not publish android binaries).

We're in the process with colleagues from Google to make this feature happen, but currently there is no decision about the shape of common public APIs. So once unblocked, I'll use this as a reference (if you're OK with it), but I cannot accept this PR as-is

/**
* Compat around the BackEvent class
*/
class BackEventCompat @VisibleForTesting constructor(
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't introduce a new public classes on Android

import kotlinx.coroutines.flow.onCompletion
import kotlinx.coroutines.launch

val LocalBackEventHandler = staticCompositionLocalOf<BackEventHandler> {
Copy link
Member

Choose a reason for hiding this comment

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

Adding public common API in fork won't work - we do NOT re-compile/publish android artifact but using binary redirection to original Google's library instead

import kotlinx.coroutines.flow.Flow

@Composable
expect fun PredictiveBackHandler(
Copy link
Member

Choose a reason for hiding this comment

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

It shouldn't be placed as part of this module because BackHandler logic is also handled in non-compose navigation modules.

(+ another common API)

@gregriggins36
Copy link

This makes sense @MatkovIvan
Is there any timeline for the proper predictive back implementation?

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