Skip to content
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

Add pager to watch app #908

Merged
merged 10 commits into from
Apr 27, 2023
Merged

Add pager to watch app #908

merged 10 commits into from
Apr 27, 2023

Conversation

mchowning
Copy link
Contributor

@mchowning mchowning commented Apr 25, 2023

Description

This adds a three screen pager to the watch app. The first page will be the current page. The second page will always be the now playing screen. The third page will always be the Up Next screen.

I initially wanted to create a Pager at the top level and have navigation within the first page, but it looks like that isn't really possible currently.

Testing Instructions

Verify that the pager works as expected and is present on the screens you would expect. Transient notification-style screens and the Settings screen are the only ones that should not have the pager. Also check that back navigation works as expected (i.e., swiping back and swiping to exit the app).

Note that you should only be able to swipe back to the previous screen if you are on the 0 page of the pager. I did this to avoid having users tap on the episode title in the Now Playing screen, swipe to the Now Playing screen again from the episode screen, and then having swipe to dismiss take them from one Now Playing screen to their previous (identical) Now Playing screen on the previous pager.

Lastly, this PR includes a somewhat-unrelated change: Tapping on the podcast name on the episode details screen should now take you to the podcast screen. (internal discussion: p1682360221894429/1682357294.725699-slack-C040S9BK7SP)

Screenshots or Screencast

Screen.Recording.2023-04-25.at.8.51.52.AM.mov

Checklist

  • If this is a user-facing change, I have added an entry in CHANGELOG.md
  • I have considered whether it makes sense to add tests for my changes
  • All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml
  • Any jetpack compose components I added or changed are covered by compose previews

@mchowning mchowning added the [Area] Wear OS watch app label Apr 25, 2023
@mchowning mchowning requested a review from a team as a code owner April 25, 2023 12:53
@ashiagr
Copy link
Contributor

ashiagr commented Apr 27, 2023

Changes look great overall!

We could have just kept a single pager at the top level (without the swipe ability at the nested levels) and popped to it let's say when we hit play on the episode screen to show the now playing screen. It's nice that you were able to show the pager even at nested levels.

Sharing a few findings/ questions, none of them are blocking, just raising them to know what you think.

  1. From the episode screen, after we click the play icon, what do you think about directly going to the now playing (player) screen (middle page in the pager) to immediately start seeing progress animation and options to skip time, instead of having to swipe to see them? It will also become the last selected pager screen just after hitting episode play and faster to access from the recent apps list.

    play_pause_episode_screen.mp4
  2. Can we add a header "Up Next" to the third page in the pager? I see it is implemented in the follow-up PR.

  3. After we select an episode from the up next queue (the third page in the pager), the pager scrolls to the first page and displays episode details. Repeating the same steps keeps building (back stack) history.

    player_depth_from_up_next.mp4

    When we play episodes from a podcast episode list, it remembers just the last played episode.

    history_episodes_list.mp4

    I wonder if we need a deep navigation back stack history in the first case. 🤔

  4. Unrelated to changes in this PR, it seems that ordering of the episodes is not the same in the watch and phone app:

    Watch Phone
    watch_ordering

@mchowning
Copy link
Contributor Author

mchowning commented Apr 27, 2023

Thanks for the review!

From the episode screen, after we click the play icon, what do you think about directly going to the now playing (player) screen (middle page in the pager) to immediately start seeing progress animation and options to skip time, instead of having to swipe to see them?

Good question. I've been tempted to make that change a few times now, and the fact that you're bringing it up makes me think it's probably a good idea. FWIW, the phone app does dismiss the episode details screen when the user taps play. What do you think @david-gonzalez-a8c ?

After we select an episode from the up next queue (the third page in the pager), the pager scrolls to the first page and displays episode details. Repeating the same steps keeps building (back stack) history. When we play episodes from a podcast episode list, it remembers just the last played episode.

The reason for that behavior is because in the first scenario the user isn't ever dismissing any of the episode screens, but in the second scenario the user is dismissing each episode screen when they swipe back to the podcast screen. The first scenario (episode screen backstack) can also happen when tapping on the episode title on the Now Playing screen.

My original implementation actually only allowed a single episode screen to be in the backstack at a time, but I was worried that might get confusing as to why, in the first scenario, swiping back would never get you back to the episode you looked at earlier. Do you have any thoughts here @david-gonzalez-a8c ?

it seems that ordering of the episodes is not the same in the watch and phone app

Good catch, I'll look into that and address it in a separate PR along with any of the other changes David suggests.

@mchowning mchowning merged commit bb44115 into main Apr 27, 2023
@mchowning mchowning deleted the add/watch-pager branch April 27, 2023 11:34
@david-gonzalez-a8c
Copy link

Good question. I've been tempted to make that change a few times now, and the fact that you're bringing it up makes me think it's probably a good idea.

I agree! In fact that was my understanding when I first reviewed the designs.

My original implementation actually only allowed a single episode screen to be in the backstack at a time

Not a super strong opinion, but I actually prefer this for the sake of simplicity (and consistency in some way). I think a good approach (as this might be a bit of an edge case) is going for the simplest behaviour, so only a 1-episode backstack and listen to user feedback if in fact becomes an issue.

@mchowning
Copy link
Contributor Author

mchowning commented Apr 27, 2023

it seems that ordering of the episodes is not the same in the watch and phone app

I don't think that is synced between devices (internal ref: pdeCcb-2o9-p2, and also confirmed by testing sync between two phones). That makes the fact that we don't (currently) let you customize the sort order on the watch a bit unpleasant if you don't want the default sort order. 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants