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

End of Year: Fix pause and vm issues #581

Merged
merged 7 commits into from
Nov 17, 2022
Merged

Conversation

ashiagr
Copy link
Contributor

@ashiagr ashiagr commented Nov 15, 2022

Description

This PR attempts to fix following two issues

Pause behavior: #567 (review)

For this, I added an AwaitPointerEventScope extension inspired from this post. - 68e8851

StoriesViewModel Scope: #570 (comment)

Due to viewmodels not being scoped to a composable, I changed the way the dialog is shown. - 9c140aa

Testing Instructions

Test.1 Pause behavior

  1. Set END_OF_YEAR_ENABLED feature flag to true in base.gradle
  2. Run the app and make sure you're logged in to an account with a few items on Listening History
  3. When the prompt appears, tap "View My 2022"
  4. Tap on a story
  5. ✅ Story should pause immediately
  6. Release tap event
  7. ✅ Story should resume
  8. Tap on a story again and drag without lifting finger
  9. ✅ Story should remain paused
  10. Release tap event
  11. ✅ Story should resume

Test.2 Store reset on share

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

I have tested any UI changes...

  • with different themes
  • with a landscape orientation
  • with the device set to have a large display and font size
  • for accessibility with TalkBack

@ashiagr ashiagr mentioned this pull request Nov 15, 2022
1 task
@ashiagr ashiagr marked this pull request as ready for review November 15, 2022 13:21
@ashiagr ashiagr requested a review from a team as a code owner November 15, 2022 13:21
@ashiagr ashiagr mentioned this pull request Nov 15, 2022
5 tasks
Copy link
Contributor

@mchowning mchowning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the pause behavior when you hold now. It feels really naturally. I also noticed that it pauses when you scroll as well, which I also like.

1. Flinging scroll cycles the story

If you scroll slowly, the story doesn't change, but if you do a quick fling to scroll, then the story changes. This is only likely to come up with large font/display settings where scrolling is more necessary, but it did throw me because I initially didn't realize my fling is what was triggering the story to change and it seemed like the stories were just rotating faster, so I could imagine this might confuse a user.

Since this does not really come up on normal display sizes,imo this probably isn't the highest priority issue to address.

fling.mp4

2. Story continuing to play during share

If I did the tap-share-then-tap-the-story-really-fast move, then the Story would not stop playing like it normally does when I tap Share. The video below shows this issue.

3. Pause stopping working

I had a few times where the pause behavior stopped working. The only way I was able to recreate this consistently was with the tap-share-then-tap-the-story-really-fast move. I did cause the issue without tapping the share button, but I wasn't able to find any other way to consistently recreate it.

share-pause.mp4

Base automatically changed from task/410-top-categories-design to main November 16, 2022 13:36
Flow is not required as on login, when account is syncing, downloadListeningHistory(...) syncs podcasts and episodes before stories are displayed.
Also convert stories from state flow to a list as results from db are not flow based.
@ashiagr
Copy link
Contributor Author

ashiagr commented Nov 17, 2022

👋 @mchowning

For 1. tried to improve the behavior in 53a18b5 but it's just a workaround. It is tricky to detect tap/ long press/ fling / scroll, I'll need more time to figure it out.

For 2 & 3, I converted db functions to suspend from flow as now the sync listening history endpoint downloads podcasts and episodes on login before we show stories. This fixes stories state changes when episode 's played_up_to is updated in db triggering another start.

Can you give it another try? I think we're very close to fixing it.🤞

@mchowning
Copy link
Contributor

I'm still able to recreate all 3 issues. FWIW, 2 & 3 feel like edge cases to me if the only way we can reproduce them is with the tap-share-and-then-quickly-tap-the-story action. I still haven't found another way to recreate it although I'm sure I caused it without sharing before (I only saw the issue with sharing this time, but I'm not sure if that's because it's better or because I just didn't do whatever is causing it).

For 1. tried to improve the behavior in 53a18b5 but it's just a workaround. It is tricky to detect tap/ long press/ fling / scroll,

Yeah, it does seems like it would be really hard to handle all those actions well.

@mchowning
Copy link
Contributor

Going ahead and approving in case you'd like to merge this and try to do some improvements in separate PRs.

@ashiagr
Copy link
Contributor Author

ashiagr commented Nov 17, 2022

Quite a few PRs are dependent on it, so I'll merge it and come up with a fix if I can reproduce the scenario.
Thanks so much for testing it, @mchowning! I really appreciate it.

@ashiagr ashiagr merged commit 1210fa9 into main Nov 17, 2022
@ashiagr ashiagr deleted the task/410-fix-pause-and-vm-issues branch November 17, 2022 15:09
@ashiagr ashiagr added this to the 7.27 ❄️ milestone Nov 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants