-
Notifications
You must be signed in to change notification settings - Fork 89
Implement advanced deep link recipe #167
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
Conversation
Second app to show case deeplinking with new stack and up button
Contains common code shared between app and appdeeplink for deeplink recipes
Summary of ChangesHello @claraf3, 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 delivers a comprehensive advanced deep linking recipe, showcasing how to handle inter-app navigation, manage Android task stacks, and construct synthetic backstacks for consistent Back and Up button functionality. The changes also involve significant architectural improvements by extracting shared UI and data logic into a new common module, making the codebase more modular and maintainable. 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request successfully implements an advanced deep linking recipe, effectively demonstrating how to manage task stacks and create a synthetic back stack. The separation of concerns into app, appdeeplink, and common modules is a great approach that mimics a real-world scenario. The code is well-structured and the new README.md files provide excellent documentation for this complex topic. I've identified a few areas for improvement, including a potential crash due to unsafe URI parsing, some leftover debug code, and minor code refinements. My detailed comments and suggestions aim to enhance the robustness and clarity of the implementation.
appdeeplink/src/main/java/com/example/nav3recipes/deeplink/app/util/BackStackUtil.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/example/nav3recipes/deeplink/advanced/AdvancedCreateDeepLinkActivity.kt
Show resolved
Hide resolved
app/src/main/java/com/example/nav3recipes/deeplink/advanced/CreateAdvancedDeepLinkActivity.kt
Outdated
Show resolved
Hide resolved
appdeeplink/src/main/java/com/example/nav3recipes/deeplink/app/Nav3RecipesDeeplinkApp.kt
Outdated
Show resolved
Hide resolved
appdeeplink/src/main/java/com/example/nav3recipes/deeplink/app/util/Navigator.kt
Outdated
Show resolved
Hide resolved
9f9e755 to
88c51d1
Compare
The recipe spans across two modules in order to simulate a real-world scenario where `App A` deeplinks into `App B`. The goal of this recipe is to show how to: 1. Create a synthetic backStack A synthetic backStack is created when the deeplinked app is started as the root Activity of a new Task. This backStack is required in order to support proper Back & Up buttons. 2. Support Back / Up buttons Back button brings user back to the prevoius screen, while the Up button brings user to the current screen's hierarhical parent. This requires synthetic backStack and Task stack management. See README for more info.
88c51d1 to
eb5de52
Compare
jbw0033
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make this not touch the basic sample code at all? It is okay to duplicate things here if needed.
I think the most important thing for these samples is that it is clear what code is required for advanced deep linking separate from helper functions for just navigating and what code is required for advanced deep linking vs the basic sample.
We can worry about clean up later.
If anything, extracting the Screens out into a separate With that, the remaining code in
Is any part confusing / too blended? |
Guide that covers basic principles of deep linking and demonstrates how to apply them in Navigation 3
8f2eaea to
e5cfc90
Compare
|
Added the deeplink-guide.md into this PR |
|
Spoke with Jeremy offline. Rename some classes and packages to clarify that the second app is for advanced deep link recipe. |
app/src/main/java/com/example/nav3recipes/deeplink/basic/README.md
Outdated
Show resolved
Hide resolved
Rename second app to advanceddeeplinkapp and reuse the same deeplink.advanced package
35f3b4b to
b2ef58c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If at all possible, I would like to avoid creating separate modules in the recipes repo. Currently each recipe has its own activity - is it not possible to demonstrate the deep linking concepts by having separate activities for each deep link app?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've explored how to represent different Task stacks without a second app module. Separate activities is not enough - we need to show the difference between an app's Activity in another App's Task, and the an app's Activity in its own Task. We do need a second app for this :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you deep link to another Activity within the same module, the Activity would still be started in the original Task even if you flagged it with Intent.FLAG_ACTIVITY_NEW_TASK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK fair enough, thanks for clarifying.
| @@ -0,0 +1,175 @@ | |||
| package com.example.nav3recipes.deeplink.advanced.util | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use of util anywhere is usually a code smell - everything and anything can be a utility and it usually becomes a dumping ground for miscellaneous things.
I would prefer splitting this file to BackStackHelpers.kt, UriHelpers.kt etc. Then moving these files into the package above and removing util package.
| * Up button (see [navigateUp] for more on this). | ||
| */ | ||
|
|
||
| internal interface NavRecipeKey: NavKey { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be better named RecipeNavKey as it's a recipe-specific NavKey.
| val screenTitle: String | ||
| } | ||
|
|
||
| internal interface NavDeepLinkRecipeKey: NavRecipeKey { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better as DeepLinkRecipeNavKey
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming there are no specific resources for the advanceddeeplinkapp, all these resources can be obtained from the app module (although better if we don't have separate modules at all).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a different launcher for second app on purpose so that you can tell which app the Activity is started on,
i.e. on old Task you'd see App A's launcher icon, on new Task you'd see App B's launcher icon.
| import com.example.nav3recipes.deeplink.basic.ui.STRING_LITERAL_USERS | ||
| import kotlinx.serialization.Serializable | ||
|
|
||
| internal interface NavRecipeKey: NavKey { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RecipeNavKey
|
|
||
| @Composable | ||
| internal fun EntryScreen(text: String, content: @Composable () -> Unit = { }) { | ||
| public fun EntryScreen(text: String, content: @Composable () -> Unit = { }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: functions are public by default in kotlin so public is redundant unless you've enabled explicit API mode (which we haven't)
|
|
||
| @Composable | ||
| internal fun EntryScreen(text: String, content: @Composable () -> Unit = { }) { | ||
| public fun EntryScreen(text: String, content: @Composable () -> Unit = { }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a composable that does almost exactly this already in the app's content/Content.kt file called ContentBase. See the other recipes for usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should move the content package into the :common module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. Might as well since :common is there now.
| implementation(libs.androidx.adaptive.layout) | ||
| implementation(platform(libs.androidx.compose.bom)) | ||
|
|
||
| implementation(libs.kotlinx.serialization.core) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are all these dependencies needed for the common module?
| internal fun FriendsList(users: List<User>) { | ||
| public fun FriendsList( | ||
| users: List<User>, | ||
| onClick: ((user: User) -> Unit)? = null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| onClick: ((user: User) -> Unit)? = null | |
| onUserClick: ((user: User) -> Unit)? = null |
The recipe spans across two modules in order to simulate a real-world scenario where
App Adeeplinks intoApp B.App A -
com.example.nav3recipes.deeplink.advancedmoduleApp B -
advanceddeeplinkappmoduleThe goal of this recipe is to show how to:
Create a synthetic backStack
A synthetic backStack is created when the deep linked app is started as the root Activity of a new Task. This backStack is required in order to support proper Back & Up buttons.
Support Back / Up buttons
Back button brings user back to the previous screen, while the Up button brings user to the current screen's hierarchical parent. This requires synthetic backStack and Task stack management. See README for more info.
This PR contains three commits
advanceddeeplinkappmodule which is the app to deep link into:commonmodule to be shared bybasicandadvanceddeep link recipescom.example.nav3recipes.deeplink.advancedmodule