-
Notifications
You must be signed in to change notification settings - Fork 230
[Push Notifications Revamp] Support deeplink for Staff Picks #3872
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
[Push Notifications Revamp] Support deeplink for Staff Picks #3872
Conversation
📲 You can test the changes from this Pull Request in 📱 Mobile by scanning the QR code below to install the corresponding build.
📲 You can test the changes from this Pull Request in 🚗 Automotive by scanning the QR code below to install the corresponding build.
📲 You can test the changes from this Pull Request in ⌚ Wear by scanning the QR code below to install the corresponding build.
|
...iscover/src/main/java/au/com/shiftyjelly/pocketcasts/discover/viewmodel/DiscoverViewModel.kt
Outdated
Show resolved
Hide resolved
...ures/discover/src/main/java/au/com/shiftyjelly/pocketcasts/discover/view/DiscoverFragment.kt
Outdated
Show resolved
Hide resolved
e2d988b
to
9278126
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.
I found two issues:
- When I navigate using
adb shell am start -a android.intent.action.VIEW -d "pktc://discover/staffpicks"
and then go to the previous screen I can no longer navigate using the same deep link. - When I navigate using a deep link, go back, and then change device's configuration the staff pick screen is added to the back stack and is added twice. See the video below:
screen-20250414-092318.mp4
Version |
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.
Thanks for the update! I noticed there’s still a navigation-related issue:
- Deep link to Staff Picks.
- Change the configuration a couple of times.
- The screen stacks on top of itself multiple times, requiring multiple back presses to exit.
Functionally, this solution doesn’t differ much from the previous one. The navigation destination is still managed as top-level state. The main change is that it’s now decoupled from the UI state of the Discover page.
However, the core approach to navigation still raises some concerns. Since the fragment stack is already handled internally by the FragmentManager
, managing it externally can lead to subtle and hard-to-diagnose issues like the one here.
A more robust solution might be to lift the Discover data (or at least the Staff Picks part of it out of the DiscoverViewModel
. That way, the flow could look like this:
- Receive a deep link to Staff Picks.
- Check if the Staff Picks screen is already visible.
- If not, retrieve data from the Staff Picks data source.
- Navigate to the Staff Picks screen.
This approach eliminates the need for a separate state holder just for navigation, and it promotes a single source of truth for the Staff Picks data, Making it more reusable and easier to reason about.
Even in the current PR, navigating to Staff Picks requires passing through both DiscoverFragment
and DiscoverAdapter
, which adds a fair bit of coupling and makes the logic harder to follow.
That said, I understand the goal here may not be to overhaul the structure. While I still have reservations about the architectural principle being reused from the previous implementation, if you're able to resolve the bugs without adding too much complexity, I’m okay with moving forward as-is.
d67720f
to
68a8c34
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.
Thanks for doing all the changes. I'd be great if we shared the data source between DisoverViewModel
and this feature but it probably isn't necessary at the moment.
Description
pktc://discover/staffpicks
Testing Instructions
Since we don't have the notifications implemented, we can test this in command line:
adb shell am start -a android.intent.action.VIEW -d "pktc://discover/staffpicks"
Checklist
./gradlew spotlessApply
to automatically apply formatting/linting)modules/services/localization/src/main/res/values/strings.xml