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

Feature: Continue playback where left off for specific URI schemes #1688

Closed
wants to merge 12 commits into from

Conversation

riher
Copy link

@riher riher commented Jun 23, 2018

This is a feature I find myself dearly missing when using the awesome mopidy-podcast backend: Being able to stop playback in the middle of a podcast and having it resume where left off when coming back.
Any podcast app will offer this functionality.

Since the implementation didn't really work in the mopidy-podcast backend and this feature can potentially be applied to any track, I've implemented the function in the mopidy core. As this is my first time contributing to mopidy I might've misplaced my code and/or structured it badly. Therefore any feedback where it might fit better is appreciated.

Also I'm not happy with the naming of this function. I've settled on "continue playback" but if you want to really cover what this does you'll write a lengthy paragraph. So if anyone has an idea for a snappy name, that'd be great, too.

Included:

  • Working code with a config option (you have to explicitly enable the functionality for URI schemes)
  • 100% test coverage
  • Added documentation for the config option

Not included:

  • Any way for a frontend to see the current playback position (big TODO) or ignore the last playback position

@kingosticks
Copy link
Member

Hi, thanks for the PR and for including tests with it. However, is this not possible to implement this with a Frontend extension? One that listens for our existing pause events and records track positions, and then also listens for playback start events and skips to the recorded position?

@riher
Copy link
Author

riher commented Jul 4, 2018

Hi kingosticks, thanks for your comment.

In my mind this function isn't suited for a frontend extension. I believe that all frontends should offer the same functionality (from the core), but with a different "look and feel" (not sure if this is actually the case, haven't tried many). Otherwise the seperation between frontend and core would get blurred.

The functionality in this PR, however, would be an unpredictable change in (playback) behaviour based on the frontend you choose.

More practical concers:

  • Which frontend will I implement it for? The one I'm using right now? What if I choose another one I like better later? Should I implement it again?
  • Same problem with users that want to use this functionality, they'll be forced to use exactly one frontend.

Finally, I really think this is an integral part of podcast listening period. This core change would provide the basis to make mopidy-podcast more complete. This shouldn't depend on the chosen frontend.

I hope I could sway you towards my perspective. 😄

@kingosticks
Copy link
Member

A Frontend is just a class of Mopidy extension. It starts when Mopidy starts and continues running until you stop Mopidy. Frontend extensions have access to the Core API. Some of them happen to provide user interaction (via the Core API) but that's by no means a requirement and is not the definition of a Frontend; I do acknowledge the name does invite this misconception. Take a look at the Mopidy-Scrobbler extension.

@riher
Copy link
Author

riher commented Jul 4, 2018

I'm not familiar with last.fm. Does scrobble mean that played tracks are tracked by last.fm or can you use a last.fm player to play tracks from mopidy?

That seems to answer my concern that the functionality would be tied to a particular 'frontend' (i.e. interface).

Is there a specific reason (and I could think of some) why you would be against adding this to the core?

@kingosticks
Copy link
Member

kingosticks commented Jul 4, 2018

They are tracked by last.fm. Hopefully you've had a quick look at the code, it's very small and simple and prob sly isn't too dissimilar to what your frontend would be like (listening and responding to events).

There's no need for it to go in the core if it can be done in an extension. You can release that extension now and have people use it. Otherwise you'll have to wait for a new mopidy release. It's also quite a niche bit of functionality so I personally don't think it belongs there either.

@riher
Copy link
Author

riher commented Jul 4, 2018

Alright, I'll look into making this a frontend extension, then. I guess you can close this PR, then.

@kingosticks
Copy link
Member

Please do and when it it's finished you are very welcome to submit a quick PR adding it to the frontend extension list in our documentation.

And thanks again for the high quality PR. Hopefully you can reuse lots of this, the tests in particular.

@kingosticks kingosticks closed this Jul 4, 2018
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.

None yet

2 participants