Merged
Conversation
github-merge-queue bot
pushed a commit
that referenced
this pull request
Apr 17, 2025
- Little clearer what the sample is for - Avoids a name collision with the api validation and #1669
nesfeder
reviewed
Apr 22, 2025
|
|
||
| // CircuitX Navigation | ||
| val interceptors = | ||
| persistentListOf(AndroidScreenAwareNavigationInterceptor(this), InfoScreenRewriteInterceptor) |
There was a problem hiding this comment.
Okay nice! So the idea here is that AndroidScreenAwareNavigationInterceptor will remove the need to
val backStack = rememberSaveableBackStack(root = root)
val delegate = rememberCircuitNavigator(backStack)
val navigator = rememberAndroidScreenAwareNavigator(delegate, this@MainActivity)
val interceptingNavigator = rememberCircuitInterceptingNavigator(
navigator = navigator,
...
)
if we're already using a AndroidScreenAwareNavigator?
Collaborator
Author
There was a problem hiding this comment.
Correct, using the interceptor provides an alternative to the nested delegation approach.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Apr 29, 2025
Splitting up the pages by artifact instead of adding onto the end of the main circuitx page for #1669 <img src="https://github.com/user-attachments/assets/52bdd251-951f-40c4-af52-a162237940e4" /> <img src="https://github.com/user-attachments/assets/6f6ce26d-8e2c-464a-91b4-bfd4b23b9c62" /> <img src="https://github.com/user-attachments/assets/e6c22d42-23f6-40c6-8d51-b4a8dd53cdfc" /> <img src="https://github.com/user-attachments/assets/2ccd2455-191b-4ab4-b955-6545233779ab" /> <img src="https://github.com/user-attachments/assets/4cf61f52-4f43-4094-a65d-1d8d1515a26a" />
ZacSweers
reviewed
May 1, 2025
Collaborator
ZacSweers
left a comment
ZacSweers
left a comment
There was a problem hiding this comment.
Couple high-level thoughts mostly focused around simplifying the naming. I think some of the "Circuit" in the names probably comes from our internal disambiguation but is a little redundant in first-party circuit APIs?
- Rename
CircuitNavigationInterceptor->NavigationInterceptorrememberCircuitInterceptingNavigator->rememberInterceptingNavigator
- Rename
CircuitNavigationEventListener->NavigationEventListener
| import kotlinx.collections.immutable.ImmutableList | ||
|
|
||
| /** A [CircuitNavigationEventListener] that adds Logcat logging for Circuit navigation. */ | ||
| public object LoggingNavigationEventListener : CircuitNavigationEventListener { |
Collaborator
There was a problem hiding this comment.
Can we hoist the impl into commonMain and then supply a logcat impl option? Similar to how Timber has a LogcatTree
| * A [CircuitInterceptingNavigator.FailureNotifier] that adds Logcat logging for CircuitX navigation | ||
| * interception failures. | ||
| */ | ||
| public object LoggingNavigatorFailureNotifier : CircuitInterceptingNavigator.FailureNotifier { |
Collaborator
There was a problem hiding this comment.
same here re: common impl + logcat helper
| * Notifies of a [InterceptorResult.Failure] from a [CircuitNavigationInterceptor] during a | ||
| * [CircuitNavigationInterceptor.goTo]. | ||
| */ | ||
| public fun goToInterceptorFailure(interceptorResult: InterceptorResult.Failure) |
Collaborator
There was a problem hiding this comment.
nit: goToFailure? Since this is already being used in the context of an interceptor
| } | ||
|
|
||
| /** The result of [CircuitNavigationInterceptor.goTo] being intercepted. */ | ||
| public sealed interface InterceptorGoToResult { |
ZacSweers
approved these changes
May 4, 2025
This was referenced May 4, 2025
github-merge-queue bot
pushed a commit
that referenced
this pull request
May 4, 2025
Follow up docs for #1669 <img width="1373" alt="Screenshot 2025-05-04 at 09 26 22" src="https://github.com/user-attachments/assets/d9f9fde9-3a60-499f-aa8a-92a5fde08292" />
github-merge-queue bot
pushed a commit
that referenced
this pull request
May 4, 2025
Follow up testing for #1669 - Adds tests for the `NavigationEventListener`, `FailureNotifier`, and `NavigationInterceptor` - The `NavigationInterceptorTest` is the bulk of the PR, with tests for single and multiple interceptor uses
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Upstreaming a chunk of our internal circuit navigator which provides apis for intercepting navigation and navigation event listening. This extends the core Circuit navigation system by adding support for navigation interceptors, navigation event listeners, and intercept failure reporting.
CircuitInterceptingNavigator
CircuitInterceptingNavigatoracts as a wrapper around a standard CircuitNavigator.goTo,pop,resetRoot) before they are handled by the underlyingNavigator.rememberCircuitInterceptingNavigatorwas added to create and manage theCircuitInterceptingNavigatorinstance to correctly capture the various navigation events.CircuitNavigationInterceptor
goTo,pop,resetRoot)InterceptorResultwhich is handled by theCircuitInterceptingNavigator, if the result is consumed no other interceptor can intercept the resultInterceptorResult's:Skipped: The interceptor isn't interested in this navigation event.Success: The interceptor has handled the event successfully (optionally consuming it).Failure: The interceptor has failed to handle the event (optionally consuming it).Rewrite: Used only bygoToandresetRoot, allowing the interceptor to replace the navigated screen with another.FailureNotifier
FailureNotifierprovides a way to handleInterceptorResult.Failureevents for any failed intercepted navigationCircuitNavigationEventListener
goTo,poporresetRootmethods are called, and not intercepted, the listeners are notified.PR todos