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

Implemented playlist save & delete #80

Closed
wants to merge 10 commits into from
Closed

Conversation

lino
Copy link

@lino lino commented Dec 2, 2015

It's pretty shitty but it adds rudimentary playlist saving. (As requested e.g. in #22 )
Needs unit tests.

@jodal
Copy link
Member

jodal commented Dec 2, 2015

If nothing else (haven't looked at the code yet, except it's relative short length), this is a nice kick in the but to have me fix this once and for all. Thanks :-)

@lino lino changed the title Added playlist saving Implemented playlist save & delete Dec 3, 2015
@jelly
Copy link

jelly commented Dec 14, 2015

Just tested this functionality. Adding a new track to a new playlist seems to work in ncmpcpp, you end up with two playlists for the newly created playlist "mopidy" however: (as seen in ncmpcpp)

mopidy
mopidy [2]

Adding another song to the playlist also works. Adding a song from the "playlists browser" in ncmpcpp shows an error, but does add the track to the playlist.

Playlist with scheme "spotify" can't add track with scheme "spotify"

Deleting a song from the playlist also shows an error, but actually succeeds.

Backend with "scheme" spotify failed to save playlist

Renaming also works, but ncmpcpp's playlist view isn't being updated. I am not sure if this is a bug of mopidy or ncmpcpp though. (sonata also does not seem to update playlists)
Renaming a non-existant (playlist renamed in another client) does not crash mopidy

@bartekb81
Copy link

Getting rid of those errors is quite simple: just make sure to return the playlist from the input param at the end of the save method:

        logger.info('Saved %s tracks to playlist %s',
                    len(playlist.tracks), playlist.name)
	return playlist

This is what save method in mopidy.core.playlists requires.

Everything works like a charm after that change.

@bartekb81
Copy link

And a comment on delete() method. It didn't work on my installation, here's an adjusted version with comments:

    def delete(self, uri):
        try:
           logger.info(str(uri))
           container = self._backend._session.playlist_container
           if not container.is_loaded:
               container.load()
           for idx, instance in enumerate(container):
               if isinstance(instance, spotify.playlist.Playlist):  # @bartek1981: it can also be a PlaylistContainerFolder so we need to check
                   if instance.link.uri == uri:                     # @bartek1981: link.uri instead of uri
                       del container[idx]
                       break
        except spotify.Error:
            logger.warning('Could not delete Spotify playlist "%s"', uri)

@bartekb81
Copy link

bartekb81 commented Feb 14, 2018

Another comment on the save method. The check that decides if a track has been added:

            if len(sp_playlist.tracks) < len(playlist.tracks): # item was added

may break if there are some tracks in sp_playlist that are no longer available on Spotify. playlist doesn't include such tracks but sp_playlist does so this comparison will not always be correct.

Solution: iterate through sp_playlist and ignore unavailable tracks when computing the count. Use that count instead:

            available_sp_playlist_tracks = filter(lambda track: track.availability == 1, sp_playlist.tracks)

            if len(available_sp_playlist_tracks) < len(playlist.tracks): # item was added
            .... # replace all sp_playlist.tracks with available_sp_playlist_tracks

            elif len(available_sp_playlist_tracks) > len(playlist.tracks): # item was removed
            .... # replace all sp_playlist.tracks with available_sp_playlist_tracks
            ....

@jodal jodal closed this Nov 18, 2019
@jodal
Copy link
Member

jodal commented Nov 18, 2019

To simplify the development setup I removed the develop branch in preference for just using a single master branch. That had the unintentional consequence of closing all PRs which targeted the develop branch.

With the recent porting of Mopidy to Python 3, the current master branch has changed quite a bit, so this PR would have needed a rebase now anyway. Feel free to open a new PR based on the current master branch if you're still interested in this feature.

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.

4 participants