Skip Back: do not allow skipping forward when skipping back #2041
Conversation
leandroalonso
added
the
[Type] Bug
leandroalonso
requested review from
SergioEstevao
and removed request for
a team
podcasts/PlaybackManager.swift
Outdated
| // Here we handle that to avoid this issue | ||
| // See https://github.com/Automattic/pocket-casts-ios/issues/1950 | ||
| if seekHint == .back { | ||
| if let previousSeekTime, time > previousSeekTime { |
There was a problem hiding this comment.
Why not use the currentTime value that we have bellow? And avoid the use of this variable and the deboucer?
There was a problem hiding this comment.
Because it will never work.
currentTime is the wrong value being given. If we compare it to it, time > currentTime will never succeed. You can try adding and testing it, the bug will be there.
Not sure if the usage of debouncer is clear, if not I can add a comment clarifying it. But we have the debouncer to fix issues with skip forward or other seeks after rapidly tapping skipping back happens. If we don't have it, we might get stuck into a state where seek will never work.
There was a problem hiding this comment.
Just wanted to clarify if the currentTime returned is correct or not.
So our current understanding is that value sometimes is not correct/or not in sync with the previous seeks
Regarding the debouncer my understanding is that we want to clear the previousSeektime, so it's only used when you are doing multiple skips back in a quick interval. Right?
podcasts/PlaybackManager.swift
Outdated
| // skip back results (sometimes) in skipping forward | ||
| // Here we handle that to avoid this issue | ||
| // See https://github.com/Automattic/pocket-casts-ios/issues/1950 | ||
| if seekHint == .back { |
There was a problem hiding this comment.
Nit: Could we refactor this to a separate method something like isValidSeek ?
There was a problem hiding this comment.
I tested this on simulator and real device and it looks that it avoid the bug of moving forward.
The only thing that I notice it that sometime the seekPosition get's "stucks: in a specific position ( I see that is because our fail guard kicks in) but it then starts moving again when the times resync.
All in all I think it's better than our current state so I think we should move forward with this solution.
Fixes #1950
Adds a mechanism to ensure that rapidly tapping Skip Back do not result in skipping forward.
This doesn't actually fixes the issue but prevents it from happening.
To test
I highly recommend testing on a real device. In order to reproduce the issue and test this PR you need:
Then:
Checklist
CHANGELOG.mdif necessary.