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

Fix effects being lost when paused #3084

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

geekygecko
Copy link
Member

@geekygecko geekygecko commented Oct 24, 2024

Description

When the app is first open or playback is paused, the player doesn't show that the playback effects are enabled. The reason is that a new PlaybackState is created without the effects.

I have tried to group the initialisation logic and put it in the PlaybackState object. I'm not sure if it should just in the PlaybackManager. 🤔

Fixes #3057

Testing Instructions

  1. Go to Profile tab -> Settings -> General
  2. Turn on "Adjust remaining time"
  3. Play an episode
  4. Open the full screen player
  5. Take note of the time remaining
  6. Tap on the effects icon
  7. Increase the playback speed to 2
  8. Close the effect dialog
  9. ✅ Verify the time remaining has decreased
  10. Pause the episode
  11. Move the seekbar to a different position in the episode
  12. ✅ Verify the playback effect icon stays highlight and the remaining time stays decreased
  13. Restart the app
  14. Open the full screen player
  15. ✅ Verify the playback effect icon is highlight and the remaining time is still decreased

Video

Screen.Recording.2024-10-24.at.5.23.39.pm.mov

Checklist

  • If this is a user-facing change, I have added an entry in CHANGELOG.md
  • Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
  • 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 updated (or requested that someone edit) the spreadsheet to reflect any new or changed analytics.

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

@geekygecko geekygecko added [Type] Bug Not functioning as intended. [Area] Player UI Full screen player or mini player UI issue labels Oct 24, 2024
@geekygecko geekygecko added this to the 7.76 milestone Oct 24, 2024
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Oct 24, 2024

📲 You can test the changes from this Pull Request in 📱 Mobile by scanning the QR code below to install the corresponding build.
App Name 📱 Mobile
Build TypedebugProd
Commitf6bb01d
Direct Downloadpocketcasts-app-prototype-build-pr3084-f6bb01d.apk
📲 You can test the changes from this Pull Request in 🚗 Automotive by scanning the QR code below to install the corresponding build.
App Name 🚗 Automotive
Build TypedebugProd
Commitf6bb01d
Direct Downloadpocketcasts-automotive-prototype-build-pr3084-f6bb01d.apk
📲 You can test the changes from this Pull Request in ⌚ Wear by scanning the QR code below to install the corresponding build.
App Name ⌚ Wear
Build TypedebugProd
Commitf6bb01d
Direct Downloadpocketcasts-wear-prototype-build-pr3084-f6bb01d.apk

@geekygecko geekygecko marked this pull request as ready for review October 24, 2024 06:58
@geekygecko geekygecko requested a review from a team as a code owner October 24, 2024 06:58
@geekygecko geekygecko requested review from ashiagr and removed request for a team October 24, 2024 06:58
remainingTimeText.contentDescription = resources.getString(LR.string.player_time_remaining, remaingingTime.removePrefix("-"))
val remainingTime = (-remainingDuration()).toHhMmSs()
remainingTimeText.text = remainingTime
remainingTimeText.contentDescription = resources.getString(LR.string.player_time_remaining, remainingTime.removePrefix("-"))
Copy link
Member Author

Choose a reason for hiding this comment

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

Minor typo

podcast = podcast,
chapters = if (sameEpisode) (previousPlaybackState?.chapters ?: Chapters()) else Chapters(),
lastChangeFrom = LastChangeFrom.OnUpdatePausedPlaybackState.value,
Copy link
Member Author

Choose a reason for hiding this comment

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

The playback effects were not being added here.

@ashiagr
Copy link
Contributor

ashiagr commented Oct 25, 2024

I have tried to group the initialisation logic and put it in the PlaybackState object. I'm not sure if it should just in the PlaybackManager. 🤔

I tried changing local playback effects and those reverted to the original value in a paused state:

time.changed.webm

@mebarbosa mebarbosa modified the milestones: 7.76, 7.77 Oct 28, 2024
Comment on lines +10 to 13
* Bug Fixes
* Fix effects being lost when paused
([#3084](https://github.com/Automattic/pocket-casts-android/pull/3084))
* Updates
Copy link
Contributor

Choose a reason for hiding this comment

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

@geekygecko I've moved this PR to 7.77. Please, make sure you update the changelog of this PR to 7.77 too

@geekygecko
Copy link
Member Author

I tried changing local playback effects and those reverted to the original value in a paused state:

Thanks for finding this, I will work on a fix tomorrow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Area] Player UI Full screen player or mini player UI issue [Type] Bug Not functioning as intended.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remaining time stops being adjusted, when scrubbing while paused
4 participants