-
Notifications
You must be signed in to change notification settings - Fork 69
**DO NOT MERGE** Add deeplink recipe for single module #97
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
base: main
Are you sure you want to change the base?
Conversation
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 adds a new recipe to the application, focusing on the robust parsing of Android deeplinks within a single-module setup. The recipe illustrates a structured approach to defining, matching, and interpreting deeplink URLs, converting them into serializable navigation keys. It highlights the use of kotlinx.serialization for argument deserialization and includes an interactive UI to facilitate testing of different deeplink patterns. 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
|
|
Ready for initial review. |
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 introduces a new recipe for parsing deeplinks. The implementation is well-structured, demonstrating the key steps from parsing a URL to resolving it into a backstack key. However, I've found several critical issues, particularly in AndroidManifest.xml and DeepLinkUtil.kt, that will cause deeplinks to fail or the app to crash. There are also opportunities to improve code robustness and readability. Please review the detailed comments for suggestions.
app/src/main/java/com/example/nav3recipes/deeplink/parseintent/singleModule/DeepLinkUtil.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/example/nav3recipes/deeplink/parseintent/singleModule/DeepLinkUtil.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/example/nav3recipes/deeplink/parseintent/singleModule/CommonScreens.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/example/nav3recipes/deeplink/basic/CreateDeepLinkActivity.kt
Show resolved
Hide resolved
app/src/main/java/com/example/nav3recipes/deeplink/parseintent/singleModule/CommonScreens.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/example/nav3recipes/deeplink/parseintent/singleModule/CommonResources.kt
Outdated
Show resolved
Hide resolved
...c/main/java/com/example/nav3recipes/deeplink/parseintent/singleModule/ParseIntentActivity.kt
Show resolved
Hide resolved
1bae1b4 to
d22b204
Compare
A recipe that shows how to parse a requested deeplink into a backStack key. It demonstrates a few crucial steps: STEP 1.Parse supported deeplinks (URLs that can be deeplinked into) into a readily readable format (see [DeepLinkCandidate]) STEP 2. Parse the requested deeplink into a readily readable, format (see [DeepLinkTarget]) **note** the parsed requested deeplink and parsed supported deeplinks should be cohesive with each other to facilitate comparison and finding a match STEP 3. Compare the requested deeplink target with supported deeplinks in order to find a match (see [DeepLinkMatchResult]). The match result's format should enable conversion from result to backstack key, regardless of what the conversion method may be. STEP 4. Associate the match results with the correct backstack key This recipes provides an example for each of the above steps by way of kotlinx.serialization. Note that this recipe is designed to focus on parsing an intent into a key, and therefore the following deeplink considerations are not included in this scope: - Create synthetic backStack - Multi-modular setup - DI - Managing TaskStack - Up button ves Back Button
d22b204 to
5c6e910
Compare
app/src/main/java/com/example/nav3recipes/deeplink/parseintent/singleModule/CommonScreens.kt
Outdated
Show resolved
Hide resolved
| import androidx.navigation3.runtime.NavKey | ||
| 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.
I think we can be more specific here by calling this a NavDeepLinkKey.
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.
Could that confuse people into thinking all those keys support deep link?
But you're right, the naming could be better. What about
DeepLinkRecipeKey
BasicDeepLinkRecipeKey
zzz...
Although I have refactored upperbound from NavRecipeKey to NavKey so the name NavRecipeKey shows up lot less now.
app/src/main/java/com/example/nav3recipes/deeplink/parseintent/singleModule/NavRecipeKey.kt
Outdated
Show resolved
Hide resolved
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.
With the current organization it is not immediately clear to me which part developers should be copying to implement their own deep links vs what is specific to this App.
Some ideas I think that might help this for me:
- Splitting the
DeepLinkUtilfile up into separate files for each component. MaybeDeepLinkRequestandDeepLinkMatchResultin one.DeepLinkin another. ThenKeyDecoderin its own as well. - Having
NavRecipeKeyin its own file separate from the specific implementations.
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.
Great to see this coming together. In case it's useful I created a branch with a few of the changes I suggested here: abb9300#diff-b49bfd99d5a52d31e22d0e0fc0d6b6ec029f048a1b665000bfb967efef6ded02
| import com.example.nav3recipes.basicsaveable.BasicSaveableActivity | ||
| import com.example.nav3recipes.commonui.CommonUiActivity | ||
| import com.example.nav3recipes.conditional.ConditionalActivity | ||
| import com.example.nav3recipes.deeplink.parseintent.singleModule.ParseIntentLandingActivity |
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.
The package name could just be com.example.nav3recipes.deeplinks and all the library code could be placed in a subpackage say common or library. We could make it clear that to add deeplinks to your app you can just copy the library package into your own app. WDYT?
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 think it is really important for each recipe to stand on its own - the simple example should include only simple code.
Trying to make just a single shared library means that to understand even what the simple recipe is doing you have to understand a library that handles way more than what is needed for that recipe.
I do think we should separate the recipe into the activity code (e.g., the example of how to use it) from the library code (e.g., what we expect developers to use in their own app if they want to 'adopt' that specific recipe).
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.
Updated with
1. Simplified the base package from "deeplink.parseIntent.singleModule" to "deeplink.basic".
2. Added `ui` and `deeplinkutil` packages to separate sample ui code from parsing/matching helpers
WDYT?
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.
You can simplify deeplinkutil to just util since it's a subpackage of deeplink.basic
...java/com/example/nav3recipes/deeplink/parseintent/singleModule/ParseIntentLandingActivity.kt
Outdated
Show resolved
Hide resolved
...java/com/example/nav3recipes/deeplink/parseintent/singleModule/ParseIntentLandingActivity.kt
Outdated
Show resolved
Hide resolved
...c/main/java/com/example/nav3recipes/deeplink/parseintent/singleModule/ParseIntentActivity.kt
Show resolved
Hide resolved
...c/main/java/com/example/nav3recipes/deeplink/parseintent/singleModule/ParseIntentActivity.kt
Show resolved
Hide resolved
app/src/main/java/com/example/nav3recipes/deeplink/parseintent/singleModule/NavRecipeKey.kt
Outdated
Show resolved
Hide resolved
| /** | ||
| * String resources | ||
| */ | ||
| internal const val STRING_LITERAL_FILTER = "filter" |
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.
It looks like a lot of this data is associated with the navigation keys. Would it be possible to move it into those keys rather than having global constants file?
For the test data it's fine, but for everything associated with specifying the deeplinks it'd be good to see these declared in line with the key itself.
| import androidx.navigation3.runtime.NavKey | ||
| 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.
I'm not sure that inheritance is the right approach here. Not all keys will have deeplinks. It might be better to have routes/keys implement an optional DeepLink interface through their companion object. For example:
internal interface DeepLink {
val name: String
}
@Serializable
internal data class UsersKey(
val filter: String,
): NavKey {
companion object : DeepLink {
const val FILTER_KEY = STRING_LITERAL_FILTER
const val FILTER_OPTION_RECENTLY_ADDED = "recentlyAdded"
const val FILTER_OPTION_ALL = "all"
override val name: String = STRING_LITERAL_USERS
}
}
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.
It inherits NavKey only to get free save/restore from rememberNavBackKey().
This recipe intentionally keeps the inheritance simple - you can either inherit NavKey for free save/restore, or even inherit nothing at all.
But you're right that more advanced use cases could benefit from inheritance, i.e. for building synthetic backStack, for more efficient parsing and matching. That will come with the follow up recipes.
| */ | ||
| class ParseIntentActivity : ComponentActivity() { | ||
| /** STEP 1. Parse supported deeplinks */ | ||
| private val deepLinkCandidates: List<DeepLink<out NavRecipeKey>> = listOf( |
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 feels like it could be improved. As a user of the deeplink API, ideally I don't want to do anything aside from mark my keys as having a deeplink by implementing an interface. I'm guessing it's not possible to obtain a list of objects that implement an interface (although maybe with a plugin? and/or with an annotation?). In which case, I would like to just specify the list of keys that have deep links.
Is it possible to do this work inside the library just by specifying a list of keys?
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.
That would be part of the more advanced recipes i.e. with mulitple modules. The current idea for that would be for each module to inject something into the common module that indicates:
- That it supports a deep link
- The type of deeplink it supports
- How to match and map to its key
Annotations could be one way. But i'll likely go with DI as it's still simpler than annotations (IMO). We could consider expanding recipes in the future with more alternatives.
| // retrieve the target Uri | ||
| val uri: Uri? = intent.data | ||
| // associate the target with the correct backstack key | ||
| val key: NavKey = uri?.let { |
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.
You could move this into a Uri extension function that takes the list of candidates
app/src/main/java/com/example/nav3recipes/deeplink/parseintent/singleModule/DeepLinkUtil.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/example/nav3recipes/deeplink/parseintent/singleModule/DeepLinkUtil.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/example/nav3recipes/deeplink/parseintent/singleModule/DeepLinkUtil.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/example/nav3recipes/deeplink/parseintent/singleModule/DeepLinkUtil.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/example/nav3recipes/deeplink/parseintent/singleModule/DeepLinkUtil.kt
Outdated
Show resolved
Hide resolved
099671a to
03bc869
Compare
Rename activites, classes, and parameters.
03bc869 to
68c060d
Compare
Done 1. But moved the keys into the Screens file per your other comment. |
1. Simplified the base package from "deeplink.parseIntent.singleModule" to "deeplink.basic".
2. Added `ui` and `deeplinkutil` packages to separate sample ui code from parsing/matching helpers
3. Separate classes within DeepLinkUtil into their own separate files
4. Separate DeepLinkRequest into two separate classes
- DeepLinkRequest: parse uri and store parse result
- DeepLinkMatcher: takes a request + pattern and matches the two
5. Change type <T> upper bound to NavKey to make the helpers more general
Other misc non-structural changes:
1. Added kdocs in CreateDeepLinkActivity to explain how this recipe works
2. inlined String parsers into #getTypeParser
a9dc248 to
48ec2ad
Compare
| // retrieve the target Uri | ||
| val uri: Uri? = intent.data | ||
| // associate the target with the correct backstack key | ||
| val key: NavKey = uri?.let { |
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.
May I ask what is planned for supporting passing deep links from onNewIntent to this mechanism? Meaning, what would be the equivalent of this?
| import kotlinx.serialization.modules.SerializersModule | ||
|
|
||
| /** | ||
| * Decodes the list of arguments into a a backstack key |
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.
| * Decodes the list of arguments into a a backstack key | |
| * Decodes the list of arguments into a back stack key |
A recipe that shows how to parse a requested deeplink into a backStack key. It demonstrates a few crucial steps:
STEP 1.Parse supported deeplinks (URLs that can be deeplinked into) into a readily readable format (see [DeepLinkCandidate])
STEP 2. Parse the requested deeplink into a readily readable format (see [DeepLinkTarget])
note the parsed requested deeplink and parsed supported deeplinks should be cohesive with each other to facilitate comparison and finding a match
STEP 3. Compare the requested deeplink target with supported deeplinks in order to find a match (see [DeepLinkMatchResult]). The match result's format should enable conversion from result to backstack key, regardless of what the conversion method may be.
STEP 4. Associate the match results with the correct backstack key
This recipes provides an example for each of the above steps by way of kotlinx.serialization.
Note that this recipe is designed to focus on parsing an intent into a key, and therefore the following deeplink considerations are not included in this scope:
Those topics will be covered in follow up PRs.