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

Hack week: Lock screen scrubber setting #2536

Merged
merged 7 commits into from
Dec 13, 2024

Conversation

bjtitus
Copy link
Contributor

@bjtitus bjtitus commented Dec 11, 2024

Fixes #2200

Adds a "Lock Screen Scrubbing" setting to the general player to disable (or enable) scrubbing from the lock screen.

To test

  • Play back a podcast and verify changing the playback position in the scrubber still works
  • Navigate to Profile > Settings > General and change the "Lock Screen Scrubbing" setting under "Player" from enabled to disabled
  • Lock the screen again and verify changing the playback position in the scrubber no longer works

Checklist

  • I have considered if this change warrants user-facing release notes and have added them to CHANGELOG.md if necessary.
  • I have considered adding unit tests for my changes.
  • I have updated (or requested that someone edit) the spreadsheet to reflect any new or changed analytics.

@bjtitus bjtitus added the [Type] Enhancement Improve an existing feature. label Dec 11, 2024
@bjtitus bjtitus added this to the 7.80 milestone Dec 11, 2024
@rviljoen
Copy link
Contributor

@bjtitus Great news! Thanks for tackling this one.

Have you considered the other implementation approach mentioned here:
#2200 (comment)

By not registering a target for commandCenter.changePlaybackPositionCommand, the scrubber control is completely disabled on the lock screen and dynamic island. I first noticed this implementation in the Audible app, which feels more correct to me. By just ignoring the scrub, a user who enabled the setting (maybe by accident) may be confused as to why the scrub action is ignored. Both approaches are effective though in preventing the accidental seeks.

@bjtitus
Copy link
Contributor Author

bjtitus commented Dec 11, 2024

By not registering a target for commandCenter.changePlaybackPositionCommand, the scrubber control is completely disabled on the lock screen and dynamic island.

Oh! That's good to know.

I just avoided that so that we didn't have to re-register the playback commands when changing this setting but I'll take another look at it. Thanks!

@bjtitus bjtitus marked this pull request as ready for review December 12, 2024 17:35
@bjtitus bjtitus requested a review from a team as a code owner December 12, 2024 17:35
@bjtitus bjtitus requested review from danielebogo and removed request for a team December 12, 2024 17:35
@bjtitus bjtitus force-pushed the bjtitus/hack-week/lock-screen-scrubber-setting branch from d3254ab to 34cfdef Compare December 12, 2024 17:40
Copy link
Contributor

@danielebogo danielebogo left a comment

Choose a reason for hiding this comment

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

Nice @bjtitus ! It's working correctly! I made a test while playing and works :shipit:

if !Settings.isLockScreenScrubbingDisabled { // Only perform the seek if lock screen scrubbing is enabled
commandCenter.changePlaybackPositionCommand.addTarget { [weak self] event -> MPRemoteCommandHandlerStatus in

guard let strongSelf = self, let _ = strongSelf.currentEpisode() else { return .noActionableNowPlayingItem }
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: guard let self should be enough here

@bjtitus bjtitus modified the milestones: 7.80, 7.79 ❄️ Dec 13, 2024
@bjtitus bjtitus changed the base branch from trunk to release/7.79 December 13, 2024 18:24
@bjtitus bjtitus changed the base branch from release/7.79 to trunk December 13, 2024 18:24
@bjtitus bjtitus modified the milestones: 7.79 ❄️, 7.80 Dec 13, 2024
@bjtitus bjtitus merged commit 929dab2 into trunk Dec 13, 2024
4 of 6 checks passed
@bjtitus bjtitus deleted the bjtitus/hack-week/lock-screen-scrubber-setting branch December 13, 2024 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement Improve an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a setting to disable scrubbing on the lock screen
3 participants