Take nav events out for a spin #113
Conversation
Rename sendEvents to awaitEvents Update APIs to match the 1.0.0-beta01 release Remove dependency on tunjid composables
tunjid
requested review from
claraf3,
dturner,
ianhanniballake and
jbw0033
as code owners
Summary of ChangesHello @tunjid, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly upgrades the navigation experience within Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces animations for TwoPaneScene using the new navigationevent library, including "drag to pop" and "slide to pop" gestures. The changes are quite extensive, adding new files for gesture handling and significantly refactoring TwoPaneScene and its strategy. The implementation demonstrates advanced Compose concepts like custom gestures, SharedTransitionLayout, and handling predictive back navigation events.
My review focuses on improving code safety, consistency, and readability. I've pointed out a few areas where non-null assertions can be replaced with safer checks, a magic number can be extracted to a constant, and unsafe type casts can be avoided to prevent potential runtime crashes. Overall, this is a great showcase of the new navigation event capabilities.
| private val Transition<*>.sceneTargetDestinationKey: TwoPaneSceneKey | ||
| get() { | ||
| val target = parentTransition?.targetState as Scene<*> | ||
| return target.key as TwoPaneSceneKey | ||
| } | ||
|
|
||
| private val Transition<*>.sceneCurrentDestinationKey: TwoPaneSceneKey | ||
| get() { | ||
| val target = parentTransition?.currentState as Scene<*> | ||
| return target.key as TwoPaneSceneKey | ||
| } |
There was a problem hiding this comment.
The use of unsafe casts (as Scene<*> and as TwoPaneSceneKey) can lead to a ClassCastException if a different type of scene or key is encountered. This can make the code brittle, especially if other scene strategies are introduced. It's safer to use a safe cast (as?) and handle the potential null case. The call sites in isShowingBackContent already use null-safe access, so they should be compatible with this change.
| private val Transition<*>.sceneTargetDestinationKey: TwoPaneSceneKey | |
| get() { | |
| val target = parentTransition?.targetState as Scene<*> | |
| return target.key as TwoPaneSceneKey | |
| } | |
| private val Transition<*>.sceneCurrentDestinationKey: TwoPaneSceneKey | |
| get() { | |
| val target = parentTransition?.currentState as Scene<*> | |
| return target.key as TwoPaneSceneKey | |
| } | |
| private val Transition<*>.sceneTargetDestinationKey: TwoPaneSceneKey? | |
| get() { | |
| val target = parentTransition?.targetState as? Scene<*> | |
| return target?.key as? TwoPaneSceneKey | |
| } | |
| private val Transition<*>.sceneCurrentDestinationKey: TwoPaneSceneKey? | |
| get() { | |
| val target = parentTransition?.currentState as? Scene<*> | |
| return target?.key as? TwoPaneSceneKey | |
| } |
| val navigationEventDispatcher = LocalNavigationEventDispatcherOwner.current!! | ||
| .navigationEventDispatcher |
There was a problem hiding this comment.
For consistency and safer code, it's better to use checkNotNull instead of the non-null asserted call (!!). This is already done in DragToPop.kt (line 115). checkNotNull makes the requirement explicit and can be more descriptive if it fails.
| val navigationEventDispatcher = LocalNavigationEventDispatcherOwner.current!! | |
| .navigationEventDispatcher | |
| val navigationEventDispatcher = checkNotNull(LocalNavigationEventDispatcherOwner.current?.navigationEventDispatcher) |
| val scale = LocalNavigationEventDispatcherOwner.current!! | ||
| .navigationEventDispatcher |
There was a problem hiding this comment.
For consistency with other parts of the code (e.g., DragToPop.kt) and for safer null handling, consider using checkNotNull instead of the non-null assertion (!!). This makes the code's intent clearer and can provide better error messages if the required component is missing.
| val scale = LocalNavigationEventDispatcherOwner.current!! | |
| .navigationEventDispatcher | |
| val scale = checkNotNull(LocalNavigationEventDispatcherOwner.current?.navigationEventDispatcher) |
| is NavigationEventTransitionState.InProgress -> max( | ||
| 1f - latestEvent.progress, | ||
| 0.7f | ||
| ) |
There was a problem hiding this comment.
The value 0.7f is a magic number representing the minimum scale during a predictive back gesture. To improve readability and maintainability, it's best to extract it into a named constant.
For example, you could define a file-level constant:
private const val MIN_PREDICTIVE_BACK_SCALE = 0.7fAnd then use it here.
| val navigationEventDispatcher = LocalNavigationEventDispatcherOwner.current!! | ||
| .navigationEventDispatcher |
There was a problem hiding this comment.
Similar to other parts of the codebase, it's recommended to use checkNotNull instead of the non-null assertion (!!) for consistency and safer code. This helps in providing more descriptive error messages.
| val navigationEventDispatcher = LocalNavigationEventDispatcherOwner.current!! | |
| .navigationEventDispatcher | |
| val navigationEventDispatcher = checkNotNull(LocalNavigationEventDispatcherOwner.current?.navigationEventDispatcher) |
ianhanniballake
left a comment
ianhanniballake
left a comment
There was a problem hiding this comment.
Instead of making a simple recipe more complicated, can you move this to a separate AnimatedTwoPaneScene recipe? That way developers can see how to progresively enhance their scene from a more simple example to a more complicated example, choosing the right level for their needs.
Please make sure to update the README to call out the new recipe as well.
2 participants
This PR add animations to
TwoPaneSceneto test out navigation event library.IDK if this is in scope for this repo, will close if it isn't.