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

Playback history #803

Closed
wants to merge 57 commits into
base: develop
from

Conversation

7 participants
@arjunrn
Contributor

arjunrn commented Jul 27, 2014

Added a playback history object to core which maintains a list of played tracks with the last played first.

@@ -2,6 +2,7 @@

# flake8: noqa
from .actor import Core
from .history import TrackkHistory

This comment has been minimized.

@ZenithDK

ZenithDK Jul 28, 2014

Contributor

Track - one 'k' too much

Otherwise - great work, looking forward to seeing this in the clients!

@txomon

This comment has been minimized.

Member

txomon commented Jul 28, 2014

Missing tests?

logger = logging.getLogger(__name__)


class TrackkHistory():

This comment has been minimized.

@adamcik

adamcik Jul 28, 2014

Member

Rename to just history, and likewise add instead of add_track?

:param track: track to change to
:type track: :class:`mopidy.models.Track`
"""
if type(track) is not Track:

This comment has been minimized.

@adamcik

adamcik Jul 28, 2014

Member

We should probably store the the track Ref instances instead of tracks. Having the fat models everywhere is something we are trying to move away from and new APIs should avoid it.

return

# Reorder the track history if the track is already present.
if track in self.track_list:

This comment has been minimized.

@adamcik

adamcik Jul 28, 2014

Member

Do we want to throw away the fact that a song has been played multiple times?

# Reorder the track history if the track is already present.
if track in self.track_list:
self.track_list.remove(track)
self.track_list.insert(0, track)

This comment has been minimized.

@adamcik

adamcik Jul 28, 2014

Member

Should we store the time the song was inserted?

This comment has been minimized.

@adamcik

adamcik Jul 28, 2014

Member

Oh, and the time should probably be milliseconds since epoch as an int. Just to make it match how time is treated elsewhere in mopidy.

self.track_list.remove(track)
self.track_list.insert(0, track)

def get_history_size(self):

This comment has been minimized.

@adamcik

adamcik Jul 28, 2014

Member

Do we have a need for getting the size like this?

:returns: The history as a list of `mopidy.models.Track`
:rtype: L{`mopidy.models.Track`}
"""
return self.track_list

This comment has been minimized.

@adamcik

adamcik Jul 28, 2014

Member

This should probably be a copy so callers of this API can't modify the history by accident

This comment has been minimized.

@arjunrn

arjunrn Jul 28, 2014

Contributor

Agreed. Will fix this in the next commit.

@arjunrn

This comment has been minimized.

Contributor

arjunrn commented Jul 28, 2014

The history with multiple plays of the same song makes sense if the timestamp is also recorded. So I can add that and store all the unique plays of the same song. About using Refs instead of the fat Track model, do I have to create the Ref instance from the track or is there a way to obtain it from the Track instance.

@adamcik

This comment has been minimized.

Member

adamcik commented Jul 28, 2014

Simple way out would just to make your API take the ref and make it the callers problem. Which would be a bit more generic, but I can't think of any reason to have other ref types than tracks in there. Though, eventually this will be the correct API anyway as we switch to making the calls to core take URIs or possibly Refs.

But for now, creating the ref you normally do Ref.track(uri=track.uri, name=something) - the tricky bit is that tracks are generic models, and we can't know how to correctly construct a name for a track from a given backend. So I would try something like parts = [track.name]; if track.artist: parts.append(', '.join(a.name for a in track.artists)); name = ' - '.join(parts) just to have something reasonable enough until the other bits get fixed and we can shift this to the caller.

Other thing that came to mind is asking how much data we want to store. These thoughts are not meant as something to add to this PR, more thinking aloud so we are all on the same page for how far to possibly take this. My question is basically if we should store play stats along the lines of what we report when scrobbling. So do we store how much of the song was played, do we only stored songs that have played at least x seconds etc... Personally I don't have any need for a lot of that extra info, I could probably also do just fine without the timestamp to be honest as long as each play gets recorded.

@adamcik

This comment has been minimized.

Member

adamcik commented Jul 28, 2014

Oh and there is also a flake error to fixup :-)

adamcik and others added some commits Jul 28, 2014

adamcik and others added some commits Aug 2, 2014

audio: Improve GStreamer logging
This adds an extra mopidy.audio.gst logger and moves the GStreamer logging to
it. Additionally this adds more logging so we can likely get by with just
mopidy logs in more cases.
audio: Split out ouput handling from audio actor
This also lays some basic ground work for handling multiple outputs.
audio: Unify source handlers
notify::source and setup-source are the same, just that setup-source is a
convenience wrapper.
audio: Refactor softwaremixer and audio interactions
This rips the mixer bits and pieces that have been hiding in the audio actor to
it's own class. The software mixer now only knows about this and nothing else
from audio.
audio: Group playbin teardown/setup
This makes it possible to see that we setup and teardown the same things. Also
fixes disconnect on a signal we no longer listen to.
Merge pull request #814 from adamcik/feature/audio-actor-refactor
mopidy.audio.actor refactoring and cleanups
@arjunrn

This comment has been minimized.

Contributor

arjunrn commented Aug 12, 2014

I have implemented all the changes as suggested, tests also included. The tests I wrote don't mock the core object. They just test the rules of the history object interface. Is this sufficient?

name_parts.append(
', '.join([artist.name for artist in track.artists])
)
ref_name = ' - '.join(name_parts)

This comment has been minimized.

@trygveaa

trygveaa Aug 12, 2014

Member

In my head, it would be more logical to name it "<artist> - <track>", instead of the other way around. This is also the way ncmpcpp shows it. Though, I see that adamcik had it the same way as you.

This comment has been minimized.

@jodal

jodal Aug 13, 2014

Member

Agree with @trygveaa, not @adamcik. I prefer artist-track, not track-artist.

@@ -23,6 +24,10 @@ class Core(
"""The library controller. An instance of
:class:`mopidy.core.LibraryController`."""

history = None
"""The playback history. An instance of
:class:`mopidy.core.TrackHistory`"""

This comment has been minimized.

@trygveaa

trygveaa Aug 12, 2014

Member

s/Track//

:type track: :class:`mopidy.models.Track`
"""
if type(track) is not Track:
logger.warning('Cannot add non-Track type object to TrackHistory')

This comment has been minimized.

@trygveaa

trygveaa Aug 12, 2014

Member

s/Track//

self.assertTrue(track.name in track_ref.name)
if track.artists:
for artist in track.artists:
self.assertTrue(artist.name in track_ref.name)

This comment has been minimized.

@jodal

jodal Aug 13, 2014

Member

Use self.assertIn(artist.name, track_ref.name), and you'll get much better error messages if the assertion doesn't hold. Same four lines up.

@jodal jodal added this to the v0.20 - Audio cleanup milestone Aug 13, 2014

@jodal jodal added the Core label Aug 13, 2014

@jodal

This comment has been minimized.

Member

jodal commented Sep 22, 2014

I'd like to have a look at getting this merged one of the next few days.

There's quite a lot of unrelated changes here ("Showing 18 changed files with 998 additions and 240 deletions.") Can you rebase your changes on top of the develop branch?

git remote add mopidy https://github.com/mopidy/mopidy.git
git fetch mopidy
git checkout playback-history
git rebase mopidy/develop
git push -f origin playback-history

@jodal jodal self-assigned this Sep 23, 2014

@jodal

This comment has been minimized.

Member

jodal commented Sep 23, 2014

@arjunrn I rebased your commits on top of the develop branch and polished it a bit. The result is in pull request #864, and will be merged after a quick review.

Thank you for the PR! :-)

@jodal jodal closed this Sep 23, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment