pocket-casts-android icon indicating copy to clipboard operation
pocket-casts-android copied to clipboard

Increase limit on playback speed to 5.0

Open zimmerrol opened this issue 1 year ago • 9 comments

Description

This PR increases the maximum playback speed from 3.0 to 5.0. The current maximum speed (3.0) can still be perceived as rather slow for podcasts with very slow speakers. Thus, this PR increases the upper bound from 3.0 to 5.0.

Testing Instructions

  1. Play a podcast
  2. Increase the playback speed to values higher than 3.0 (the previous maximum) and lower than 5.0 (the new maximum).

Screenshot

screenshot

Checklist

  • [x] If this is a user-facing change, I have added an entry in CHANGELOG.md
  • [x] Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
  • [x] I have considered whether it makes sense to add tests for my changes
  • [x] All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml
  • [x] 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

zimmerrol avatar Dec 23 '23 18:12 zimmerrol

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Dec 23 '23 18:12 CLAassistant

This is unlikely to be merged as it leads to too much tapping, can you do something about it , I am also working on a redesigned playback FX player to justify a speed increase to 5.0, if you can implement that then i would say we can go to 5x 😉

https://github.com/Automattic/pocket-casts-android/pull/1150

CookieyedCodes avatar Dec 23 '23 19:12 CookieyedCodes

🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏🙏

MtndaleRedneck avatar Feb 23 '24 18:02 MtndaleRedneck

@MiSikora What are your thoughts on this?

zimmerrol avatar Mar 14 '24 09:03 zimmerrol

If you're asking about my opinion I feel that it is too much but I don't have any problems with having it in the app. I'll ask internally if it is something that we want to support and get back to you.

However, this code on its own isn't enough. It needs changes that will support speed toggling from notification center and Tasker support for these settings.

MiSikora avatar Mar 14 '24 17:03 MiSikora

@zimmerrol We can add it. However, as I mentioned, you need to add support for notifications center and Tasker plugin.

Notifications are handled here

https://github.com/Automattic/pocket-casts-android/blob/2ea07252cb1b4afc99df966159b87023a0d77664/modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/MediaSessionManager.kt#L435-L462

https://github.com/Automattic/pocket-casts-android/blob/2ea07252cb1b4afc99df966159b87023a0d77664/modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/MediaSessionManager.kt#L738-L750

And Tasker is handled here

https://github.com/Automattic/pocket-casts-android/blob/2ea07252cb1b4afc99df966159b87023a0d77664/modules/features/taskerplugin/src/main/java/au/com/shiftyjelly/pocketcasts/taskerplugin/controlplayback/config/ViewModelConfigControlPlayback.kt#L46-L48

MiSikora avatar Mar 14 '24 18:03 MiSikora

Great, thanks for looking into this @MiSikora. I'll implement the necessary changes!

@mebarbosa I noticed that you recently (#1862) updated all playback speed resources to ensure they all have the same font family; since we now need resources for the added speed values, I was wondering whether you could tell me how you created these images (based on what template) so that I can create the missing ones.

zimmerrol avatar Mar 15 '24 12:03 zimmerrol

Great, thanks for looking into this @MiSikora. I'll implement the necessary changes!

@mebarbosa I noticed that you recently (#1862) updated all playback speed resources to ensure they all have the same font family; since we now need resources for the added speed values, I was wondering whether you could tell me how you created these images (based on what template) so that I can create the missing ones.

@zimmerrol thanks for the ping and working on this! ❤️ I think our design @david-gonzalez-a8c could help us with this. David generated all images for me

mebarbosa avatar Mar 15 '24 12:03 mebarbosa

@mebarbosa see my comment above, David actually gave me some feedback from a design I did a few months ago 😅 😉

CookieyedCodes avatar Mar 15 '24 14:03 CookieyedCodes

@david-gonzalez-a8c Did you have a chance for looking at this PR or can you please send me the templates you created/used? So that we can finalize this PR?

zimmerrol avatar May 12 '24 12:05 zimmerrol

Hey there, @zimmerrol

Let me know how these look and if you need a different file structure or naming:

Speed icons Android 5x.zip

Thank you!

david-gonzalez-a8c avatar May 13 '24 16:05 david-gonzalez-a8c

@david-gonzalez-a8c I'm still a tad unsure going up to 5x speed would be good without a redesign on safety grounds & tapping +- is very laborious beyond even 3x speed 😅 just a reminder of our chat from last year https://github.com/Automattic/pocket-casts-android/pull/1150

CookieyedCodes avatar May 13 '24 17:05 CookieyedCodes

@MiSikora I implemented your suggestions above and think this should complete the PR. Is there anything else that needs to be done before this can get merged?

Here is an example of how the new speeds are correctly displayed in the notifications: image

zimmerrol avatar May 30 '24 08:05 zimmerrol

These tests are failing

[2024-06-07T08:43:11Z] au.com.shiftyjelly.pocketcasts.utils.featureflag.DoubleTest > roundedSpeed respects upper boundary FAILED
[2024-06-07T08:43:11Z]     junit.framework.AssertionFailedError at DoubleTest.kt:23
[2024-06-07T08:43:11Z]
[2024-06-07T08:43:11Z] au.com.shiftyjelly.pocketcasts.utils.featureflag.DoubleTest > roundedSpeed adjusts above upper boundary FAILED
[2024-06-07T08:43:11Z]     junit.framework.AssertionFailedError at DoubleTest.kt:38

MiSikora avatar Jun 07 '24 08:06 MiSikora