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

Adding a way to disable items in the queue #76

Merged
merged 13 commits into from
Apr 22, 2018
Merged

Conversation

delannoyk
Copy link
Owner

@delannoyk delannoyk commented Oct 31, 2016

This PR is an implementation of what is discussed in #61. The goal here is to define at runtime - and before an item is played - if an AudioItem should be played or not.

This allows the original request to disable remote items when WiFi connection is down, but also allows other use cases.

Here's the todo list:

  • Added a delegate method to check if an item is playable or not
  • Use that method in AudioItemQueue to return the correct values in nextItem(), hasNextItem, previousItem() and hasPreviousItem.
  • Verify the implementation with new unit tests.

@delannoykbot
Copy link

delannoykbot commented Oct 31, 2016

Danger has errored

[!] Invalid Dangerfile file: undefined local variable or method xcov for #<Danger::Dangerfile:0x00007fbf77c36d18>

 #  from Dangerfile:20
 #  -------------------------------------------
 #  # Asserts the test coverage meets the threshold
 >  xcov.report(scheme: 'AudioPlayer iOS', project: 'AudioPlayer/AudioPlayer.xcodeproj', minimum_coverage_percentage: 50)
 #  -------------------------------------------

Generated by 🚫 Danger

@delannoyk
Copy link
Owner Author

delannoyk commented Nov 1, 2016

Documentation warnings are false positives (see this).

Anyway, there are still some concerns about the implementation I'd like to address:

  • What happens if the queue mode is .repeat? Should it still go to previous/next item in the queue? I think the player should fail with a new type of AudioPlayerError, something like .itemNotConsideredPlayable.
  • What happens when the entire queue is considered unplayable? I also think a new error should be added .noItemsConsiredPlayable.

What do guys think of this?

@codecov-io
Copy link

codecov-io commented Nov 6, 2016

Codecov Report

Merging #76 into master will decrease coverage by 20.03%.
The diff coverage is 15.51%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #76       +/-   ##
===========================================
- Coverage   75.72%   55.68%   -20.04%     
===========================================
  Files          39       25       -14     
  Lines        2797     1406     -1391     
===========================================
- Hits         2118      783     -1335     
+ Misses        679      623       -56
Impacted Files Coverage Δ
...ioPlayer/AudioPlayer/player/AudioPlayerState.swift 100% <ø> (ø) ⬆️
...Player/player/extensions/AudioPlayer+Control.swift 9.09% <0%> (-0.53%) ⬇️
...ioPlayer/player/extensions/AudioPlayer+Queue.swift 0% <0%> (ø) ⬆️
...layer/AudioPlayer/player/AudioPlayerDelegate.swift 0% <0%> (ø) ⬆️
AudioPlayer/AudioPlayer/item/AudioItemQueue.swift 97.14% <75%> (-2.86%) ⬇️
...er/player/extensions/AudioPlayer+PlayerEvent.swift 24.77% <0%> (-11.89%) ⬇️
...yer/AudioPlayer/event/AudioItemEventProducer.swift 86.84% <0%> (-10.53%) ⬇️
...ioPlayer/AudioPlayer/event/SeekEventProducer.swift 86.84% <0%> (-10.53%) ⬇️
...oPlayer/AudioPlayer/event/RetryEventProducer.swift 89.58% <0%> (-8.34%) ⬇️
...r/AudioPlayer/utils/CMTime+TimeIntervalValue.swift 91.66% <0%> (-8.34%) ⬇️
... and 23 more

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 1717403...d4a6549. Read the comment docs.

@delannoyk delannoyk force-pushed the feature/ask_to_play branch from 80e1a3a to 5d1f603 Compare April 11, 2018 02:14
@delannoyk delannoyk force-pushed the feature/ask_to_play branch from 5d1f603 to 0535309 Compare April 14, 2018 15:31
@delannoyk delannoyk changed the title [WiP] Adding a way to disable items in the queue Adding a way to disable items in the queue Apr 14, 2018
@jadsonlourenco
Copy link

"What happens if the queue mode is .repeat?" Should play the first item of playlist, no?

@delannoyk
Copy link
Owner Author

@jadsonlourenco I kind of changed my mind implementing it and went for a repeat mode that doesn't account for the current "availability" of the current item.
So if the queue is in repeat mode, it will keep trying to play the same item over and over again. I think that's a pretty fair behavior. What do you think?

@jadsonlourenco
Copy link

Yes, and .repeatAll should repeat all the current items again, from beginning (but, with shuffle?).

@delannoyk
Copy link
Owner Author

@jadsonlourenco Yep.

Shuffle will just play the shuffled queue and skip "unavailable" items.

@delannoyk delannoyk merged commit 549857f into master Apr 22, 2018
@delannoyk delannoyk deleted the feature/ask_to_play branch April 22, 2018 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants