Bugfix: Interruption ended #115

ddfreiling · 2017-06-09T09:02:06Z

This fixes #66

Receiving audio interruptions from un-resumable sources like Spotify and YouTube would leave the player in a very bad state with the pausedForInterruption flag set indefinitely.
pausedForInterruption is now reset on resume, currentItem set, and on any interruptionEnded callback, whether it has the shouldResume option or not.

@delannoyk this commit I need feedback on, am I correct that the check on pausedForInterruption flag is unnecessary in shouldResumePlaying?

…umable source ended happened when an interruption with no shouldResume option ended. this did not forward the interruption ended event and the pausedForInterruption flag would never be removed. resolves #66

… change

flag is only used while state isPaused anyway

# Conflicts: # AudioPlayer/AudioPlayer/player/AudioPlayer.swift

delannoykbot · 2017-06-09T09:08:06Z

SwiftLint found issues

Warnings

File	Line	Reason
PlayerEventProducer.swift	215	Lines should not have trailing whitespace.
AudioPlayer.swift	104	Lines should not have trailing whitespace.
AudioPlayer.swift	107	Lines should not have trailing whitespace.
AudioPlayer.swift	110	Lines should not have trailing whitespace.
AudioPlayer.swift	117	Lines should not have trailing whitespace.
AudioPlayer.swift	221	Lines should not have trailing whitespace.
AudioPlayer.swift	228	Lines should not have trailing whitespace.
AudioPlayer.swift	232	Lines should not have trailing whitespace.
AudioPlayer.swift	396	Lines should not have trailing whitespace.
AudioPlayer.swift	404	Lines should not have trailing whitespace.
AudioPlayer.swift	437	File should contain 400 lines or less: currently contains 437
AudioPlayer+Control.swift	85	Prefer `!= nil` over `let _ =`
AudioPlayer+Control.swift	89	Prefer `!= nil` over `let _ =`
AudioPlayer+Control.swift	92	Prefer `!= nil` over `let _ =`
AudioPlayer+Control.swift	19	Lines should not have trailing whitespace.
AudioPlayer+Control.swift	44	Lines should not have trailing whitespace.
AudioPlayer+Control.swift	50	Lines should not have trailing whitespace.
AudioPlayer+Control.swift	204	Lines should not have trailing whitespace.
AudioPlayer+Control.swift	206	Function parameters should be aligned vertically if they're in multiple lines in a declaration.
AudioPlayer+Control.swift	207	Function parameters should be aligned vertically if they're in multiple lines in a declaration.
AudioPlayer+Control.swift	208	Function parameters should be aligned vertically if they're in multiple lines in a declaration.
AudioPlayer+PlayerEvent.swift	95	TODOs should be avoided (where to start?).
AudioPlayer+PlayerEvent.swift	101	TODOs should be avoided (there should be a check if sta...).
AudioPlayer+PlayerEvent.swift	15	Function body should span 40 lines or less excluding comments and whitespace: currently spans 77 lines
AudioPlayer+PlayerEvent.swift	52	Lines should not have trailing whitespace.
AudioPlayer+PlayerEvent_Tests.swift	30	Force unwrapping should be avoided.
AudioPlayer+PlayerEvent_Tests.swift	53	Force unwrapping should be avoided.
AudioPlayer+PlayerEvent_Tests.swift	46	Prefer `!= nil` over `let _ =`
AudioPlayer+PlayerEvent_Tests.swift	69	Prefer `!= nil` over `let _ =`

Errors

File	Line	Reason
AudioPlayer.swift	249	Prefer shorthand operators (+=, -=, *=, /=) over doing the operation and assigning.
AudioPlayer.swift	264	Prefer shorthand operators (+=, -=, *=, /=) over doing the operation and assigning.
AudioPlayer+PlayerEvent.swift	15	Function should have complexity 10 or less: currently complexity equals 25
AudioPlayer+PlayerEvent_Tests.swift	13	Type name should only contain alphanumeric characters: 'AudioPlayer_PlayerEvent_Tests'

Current coverage for AudioPlayer.framework is `55%`

Files changed	-	-
AudioPlayer+Control.swift	`8%`	💀
AudioPlayer+PlayerEvent.swift	`23%`	💀
AudioPlayer.swift	`27%`	🚫
PlayerEventProducer.swift	`94%`	✅

Powered by xcov

Generated by 🚫 Danger

codecov-io · 2017-06-09T09:20:10Z

Codecov Report

Merging #115 into master will decrease coverage by 0.14%.
The diff coverage is 54.54%.

@@            Coverage Diff             @@
##           master     #115      +/-   ##
==========================================
- Coverage   72.58%   72.43%   -0.15%     
==========================================
  Files          39       39              
  Lines        2783     2790       +7     
==========================================
+ Hits         2020     2021       +1     
- Misses        763      769       +6

Impacted Files	Coverage Δ
...Player/player/extensions/AudioPlayer+Control.swift	`8.33% <0%> (-0.17%)`	⬇️
...er/player/extensions/AudioPlayer+PlayerEvent.swift	`23.33% <0%> (ø)`	⬆️
AudioPlayer/AudioPlayer/player/AudioPlayer.swift	`26.6% <0%> (-0.35%)`	⬇️
...dioPlayerTests/AudioPlayer+PlayerEvent_Tests.swift	`92.59% <100%> (ø)`	⬆️
...Player/AudioPlayer/event/PlayerEventProducer.swift	`93.95% <90.9%> (+0.04%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 846b390...7dd19da. Read the comment docs.

ddfreiling · 2017-06-14T16:32:14Z

Looks to me that stateBeforeBuffering & stateWhenConnectionLost, as with pausedForInterruption maybe should be reset on currentItem change as well. Thoughts?

dani-mp · 2017-06-29T10:27:39Z

AudioPlayer/AudioPlayer/event/PlayerEventProducer.swift

@@ -70,7 +70,7 @@ class PlayerEventProducer: NSObject, EventProducer {
        case progressed(CMTime)
        case endedPlaying(Error?)
        case interruptionBegan
-        case interruptionEnded
+        case interruptionEnded(Bool)


I'd be great if you add a label for this param, like .interruptionEnded(shouldResume: Bool), so it's clear that the Bool is not about the fact that the interruption ended, but what to do after.

dani-mp · 2017-06-29T10:36:28Z

AudioPlayer/AudioPlayer/player/extensions/AudioPlayer+PlayerEvent.swift

-        case .interruptionEnded where pausedForInterruption:
-            if resumeAfterInterruption {
+        case .interruptionEnded(let shouldResume) where pausedForInterruption:
+            if resumeAfterInterruption && shouldResume {


Instead of checking two properties that more or less means the same, is there a possibility that shouldResume variable affects the value of resumeAfterInterruption before arriving to this point, so all the logic is centralised in one place? Usually, when I start having several boolean flags in a class, I start thinking if I can model things better with just an enum variable that gets changed when different events happen, having all the state changes occurring in one function, avoiding possible inconsistencies. Also, this helps checking for this value later, because you don't have to remember of the flags and merge them, that job is already done. What do you think?

resumeAfterInterruption is a configuration for clients, if they want to enable the resume feature
So in my opinion it is distinct enough to check them both together like this.

Yup, makes sense!

dani-mp · 2017-06-29T10:44:23Z

Yeah, I think I agree. I've added a comment to one piece of this PR regarding this. It seems to me that having all these flags/variables with almost the same meaning but coming from different places is difficult to handle, because you can forget to update or check some of them. I'd rather have a single place with the state of the class, a list of different actions/events that can happen, and only one function that takes those actions and create a new state, centralised. What do you guys think?

ddfreiling · 2017-06-29T11:01:41Z

@danielmartinprieto I agree that state variables and handling is way too fragmented and chaotic right now. But improved state handling should be done in a separate PR than this. Preferably as a new extension to AudioPlayer.

dani-mp · 2017-06-29T11:02:43Z

@ddfreiling Also agree ✨

delannoyk

Sorry for the delay! Looks good thanks for your work.

ddfreiling · 2017-06-30T07:56:17Z

@delannoyk Good to see you back, what was your thoughts on this Kevin? I wanted to add it to this PR if you agreed.

Looks to me that stateBeforeBuffering & stateWhenConnectionLost, as with pausedForInterruption maybe should be reset on currentItem change as well. Thoughts?

delannoyk · 2017-06-30T12:13:47Z

@ddfreiling Oh yea we should, it totally makes sense.
stateBeforeBuffering is a per item state,
stateWhenConnectionLost is a global state but it should be set back accordingly if the new track we're trying to play is a remote item.
👍

ddfreiling · 2017-06-30T13:09:05Z

@delannoyk Cool, I'll add a PR for this when I get time. I actually stumbled on a real edge-case bug with stateWhenConnectionLost not being reset:

Play remote track
Enable flight-mode, dropping connection
Skip forward in playlist to offline track
Manually pause offline track
Disable flight-mode
Paused offline track now resumes playing because it tried recovering the lost connection

Resetting these on currentItem change should fix this, however edge-case it might be 😄

ddfreiling added 4 commits June 9, 2017 10:50

fix: player stayed in interrupted state when interruption from un-res…

d96bb5c

…umable source ended happened when an interruption with no shouldResume option ended. this did not forward the interruption ended event and the pausedForInterruption flag would never be removed. resolves #66

fix: make sure pausedForInterruption flag is reset on resume and item…

04a75da

… change

fix: shouldResumePlaying should not check pausedForInterruption flag

2769b92

flag is only used while state isPaused anyway

Merge branch 'master' into fix/interruption-ended

f9a02bb

# Conflicts: # AudioPlayer/AudioPlayer/player/AudioPlayer.swift

dani-mp reviewed Jun 29, 2017

View reviewed changes

chore: added labels to PlayerEvent parameters

7dd19da

delannoyk approved these changes Jun 29, 2017

View reviewed changes

delannoyk merged commit 231342f into delannoyk:master Jun 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix: Interruption ended #115

Bugfix: Interruption ended #115

ddfreiling commented Jun 9, 2017 •

edited

delannoykbot commented Jun 9, 2017 •

edited

codecov-io commented Jun 9, 2017 •

edited

ddfreiling commented Jun 14, 2017

dani-mp Jun 29, 2017

dani-mp Jun 29, 2017

ddfreiling Jun 29, 2017

dani-mp Jun 29, 2017

dani-mp commented Jun 29, 2017

ddfreiling commented Jun 29, 2017 •

edited

dani-mp commented Jun 29, 2017

delannoyk left a comment

ddfreiling commented Jun 30, 2017 •

edited

delannoyk commented Jun 30, 2017

ddfreiling commented Jun 30, 2017 •

edited

Bugfix: Interruption ended #115

Bugfix: Interruption ended #115

Conversation

ddfreiling commented Jun 9, 2017 • edited

delannoykbot commented Jun 9, 2017 • edited

SwiftLint found issues

Warnings

Errors

Current coverage for AudioPlayer.framework is 55%

codecov-io commented Jun 9, 2017 • edited

Codecov Report

ddfreiling commented Jun 14, 2017

dani-mp Jun 29, 2017

Choose a reason for hiding this comment

dani-mp Jun 29, 2017

Choose a reason for hiding this comment

ddfreiling Jun 29, 2017

Choose a reason for hiding this comment

dani-mp Jun 29, 2017

Choose a reason for hiding this comment

dani-mp commented Jun 29, 2017

ddfreiling commented Jun 29, 2017 • edited

dani-mp commented Jun 29, 2017

delannoyk left a comment

Choose a reason for hiding this comment

ddfreiling commented Jun 30, 2017 • edited

delannoyk commented Jun 30, 2017

ddfreiling commented Jun 30, 2017 • edited

ddfreiling commented Jun 9, 2017 •

edited

delannoykbot commented Jun 9, 2017 •

edited

Current coverage for AudioPlayer.framework is `55%`

codecov-io commented Jun 9, 2017 •

edited

ddfreiling commented Jun 29, 2017 •

edited

ddfreiling commented Jun 30, 2017 •

edited

ddfreiling commented Jun 30, 2017 •

edited