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

Increase limit on playback speed to 5.0 #1645

Merged
merged 16 commits into from
Jun 7, 2024

Conversation

zimmerrol
Copy link
Contributor

@zimmerrol zimmerrol commented Dec 23, 2023

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

  • 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 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 zimmerrol requested a review from a team as a code owner December 23, 2023 18:47
@CLAassistant
Copy link

CLAassistant commented Dec 23, 2023

CLA assistant check
All committers have signed the CLA.

@CookieyedCodes
Copy link

CookieyedCodes commented Dec 23, 2023

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 😉

#1150

@MtndaleRedneck
Copy link

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

@zimmerrol
Copy link
Contributor Author

@MiSikora What are your thoughts on this?

@MiSikora
Copy link
Contributor

MiSikora commented Mar 14, 2024

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
Copy link
Contributor

MiSikora commented Mar 14, 2024

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

Notifications are handled here

val drawableId = when (playbackState.playbackSpeed.roundedSpeed()) {
in 0.0..<0.55 -> IR.drawable.auto_0_5
in 0.55..<0.65 -> IR.drawable.auto_0_6
in 0.65..<0.75 -> IR.drawable.auto_0_7
in 0.75..<0.85 -> IR.drawable.auto_0_8
in 0.85..<0.95 -> IR.drawable.auto_0_9
in 0.95..<1.05 -> IR.drawable.auto_1
in 1.05..<1.15 -> IR.drawable.auto_1_1
in 1.15..<1.25 -> IR.drawable.auto_1_2
in 1.25..<1.35 -> IR.drawable.auto_1_3
in 1.35..<1.45 -> IR.drawable.auto_1_4
in 1.45..<1.55 -> IR.drawable.auto_1_5
in 1.55..<1.65 -> IR.drawable.auto_1_6
in 1.65..<1.75 -> IR.drawable.auto_1_7
in 1.75..<1.85 -> IR.drawable.auto_1_8
in 1.85..<1.95 -> IR.drawable.auto_1_9
in 1.95..<2.05 -> IR.drawable.auto_2
in 2.05..<2.15 -> IR.drawable.auto_2_1
in 2.15..<2.25 -> IR.drawable.auto_2_2
in 2.25..<2.35 -> IR.drawable.auto_2_3
in 2.35..<2.45 -> IR.drawable.auto_2_4
in 2.45..<2.55 -> IR.drawable.auto_2_5
in 2.55..<2.65 -> IR.drawable.auto_2_6
in 2.65..<2.75 -> IR.drawable.auto_2_7
in 2.75..<2.85 -> IR.drawable.auto_2_8
in 2.85..<2.95 -> IR.drawable.auto_2_9
in 2.95..<3.05 -> IR.drawable.auto_3
else -> IR.drawable.auto_1

val newSpeed = when (playbackManager.getPlaybackSpeed()) {
in 0.0..<0.60 -> 0.6
in 0.60..<0.80 -> 0.8
in 0.80..<1.00 -> 1.0
in 1.00..<1.20 -> 1.2
in 1.20..<1.40 -> 1.4
in 1.40..<1.60 -> 1.6
in 1.60..<1.80 -> 1.8
in 1.80..<2.00 -> 2.0
in 2.00..<3.00 -> 3.0
in 3.00..<3.05 -> 0.6
else -> 1.0
}

And Tasker is handled here

object : InputField<Double>(LR.string.playback_speed_between_values, R.drawable.speedometer, InputControlPlayback.PlaybackCommand.SetPlaybackSpeed, { playbackSpeed }, { playbackSpeed = it }) {
override fun getPossibleValues() = MutableStateFlow((5..30).map { it / 10.0 })
},

@zimmerrol
Copy link
Contributor Author

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.

@mebarbosa
Copy link
Contributor

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

@CookieyedCodes
Copy link

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

@zimmerrol
Copy link
Contributor Author

@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?

@david-gonzalez-a8c
Copy link

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!

@CookieyedCodes
Copy link

@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 #1150

@zimmerrol
Copy link
Contributor Author

@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

@MiSikora MiSikora added the polish A minor improvement label Jun 7, 2024
Copy link
Contributor

@MiSikora MiSikora left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

If you wouldn't mind could you sync with main and add this to the changelog? Something like:

*   Updates:
    *   Playback speed can now be changed up to 5x.
        ([#1645](https://github.com/Automattic/pocket-casts-android/pull/1645))

@MiSikora MiSikora added this to the 7.66 milestone Jun 7, 2024
@MiSikora
Copy link
Contributor

MiSikora commented Jun 7, 2024

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 MiSikora merged commit 91c857c into Automattic:main Jun 7, 2024
11 checks passed
@zimmerrol zimmerrol deleted the feature/playback-speed branch June 7, 2024 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
polish A minor improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants