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

Add a playback threshold percentage at which point to mark an episode as played #111

Merged
merged 1 commit into from
Oct 22, 2022

Conversation

dyerc
Copy link
Owner

@dyerc dyerc commented Oct 11, 2022

This PR fixes #106.

@GetToSet would you mind reviewing please. It seemed necessary to have an additional observer event for when the player finishes playback. Do you think we should reset the player visual state at this point? Currently it just stays the same but with 0:00 remaining on the clock (as it always has done). Nothing necessarily short-term, but it was something that occurred to me.

@@ -53,6 +51,18 @@ final class Player: NSObject {

private(set) var pausedAt: TimeInterval? = nil

var playedThreshold: Double {
get {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Swift code generally omits the get keyword for readonly property getters.

SwiftLint has a default rule for this and actually I've planned to enable that in the next version.

get {
let prefPercentage = Preference.double(for: Preference.Key.markAsPlayedAfter)

if prefPercentage <= 100 && prefPercentage >= 50 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's more common to use , instead of && in if statements.

@@ -193,6 +203,7 @@ final class Player: NSObject {

// Register to receive status events
avPlayer.addObserver(self, forKeyPath: "status", options: NSKeyValueObservingOptions(rawValue: 0), context: nil)
NotificationCenter.default.addObserver(self, selector: #selector(playerDidFinishPlaying(notification:)), name: NSNotification.Name.AVPlayerItemDidPlayToEndTime, object: avPlayer)
Copy link
Collaborator

Choose a reason for hiding this comment

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

With some testing it seems that the notification is not firing when playing to the end. According to this SO thread, the object should be player.currentItem instead of the player itself.

Copy link
Collaborator

@GetToSet GetToSet left a comment

Choose a reason for hiding this comment

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

See the comments for details.

@GetToSet
Copy link
Collaborator

This PR fixes #106.

@GetToSet would you mind reviewing please. It seemed necessary to have an additional observer event for when the player finishes playback. Do you think we should reset the player visual state at this point? Currently it just stays the same but with 0:00 remaining on the clock (as it always has done). Nothing necessarily short-term, but it was something that occurred to me.

I think there's no need to cleanup player states when played to the end and we'll revisit the behavior when adding the playlist feature. Further, should we set the default threshold to 90% (to match the old behavior) or keep the default value 100%?

@dyerc dyerc merged commit 5be6942 into master Oct 22, 2022
dyerc added a commit that referenced this pull request Oct 22, 2022
@dyerc
Copy link
Owner Author

dyerc commented Oct 22, 2022

This PR fixes #106.
@GetToSet would you mind reviewing please. It seemed necessary to have an additional observer event for when the player finishes playback. Do you think we should reset the player visual state at this point? Currently it just stays the same but with 0:00 remaining on the clock (as it always has done). Nothing necessarily short-term, but it was something that occurred to me.

I think there's no need to cleanup player states when played to the end and we'll revisit the behavior when adding the playlist feature. Further, should we set the default threshold to 90% (to match the old behavior) or keep the default value 100%?

I think we can set the new value to 100% because it wasn't shown anywhere that the episode is marked as played after 90%, unless the user looked at the source code.

I have left in the playerDidFinishPlaying observer because I like the idea of resetting the play position of the episode to 0. If the user plays it again, starting from the beginning seems logical.

Thanks for your testing @GetToSet 👍

@GetToSet GetToSet added this to the v2.0.1 milestone Oct 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FR/Discussion: Better logic to mark episode as played
2 participants