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

Local playlist refactoring #995

Closed
wants to merge 10 commits into
base: develop
from

Conversation

2 participants
@tkem
Member

tkem commented Feb 23, 2015

Fixes #937; I decided to do a little refactoring of the local playlist code, even at the risk of putting "too much" in a single PR:

  • Local playlist URIs now look similar to local track URIs; added local_playlist_uri_to_path() and path_to_local_playlist_uri() to mopidy.local.translator.
  • Dropped formatting.slugify() for playlist names. Since my early MPD days, I have this M3U playlist named Radio Österreich.m3u for local radio streams, and I really hate if that gets messed up... Instead, M3U playlist names are sanitized with respect to the file system encoding, and only the os.path.basename of a given name is used. This means that a LocalPlaylistsProvider.create('A/B') will result in a file named B.m3u. This is less than ideal and subject to discussion, but the best I could come up with for now. Seems to work great with modifying existing M3U files, though...
  • Emit a playlists_loaded event after successfully deleting a playlist. With create and save, playlist_changed events are all already emitted by the PlaylistController, but AFAICS no events are sent when a playlist is deleted, which might also be of interest to clients. I didn't want to use playlist_changed for playlists no longer accessible since clients may not handle this well. I think this needs more discussion and should really be handled by the PlaylistController for consistency.
  • The current docs [https://docs.mopidy.com/en/latest/api/core/#mopidy.core.PlaylistsController.save] state that for save(), "you should not set the uri atribute yourself, but use playlist objects returned by create() or retrieved from playlists". IMHO, this SHOULD might as well be changed to MUST, making things a little easier on the backend side. However, for this PR only a warning is logged if save() is called with a "new" playlist URI.
  • Creating or saving a playlist with a name/URI of another existing playlist will overwrite the other playlist without further notice. Now, I don't think it is specified in the docs what should happen in this case. I left it as I think was originally intended, but I guess this case needs to be documented, at least.
  • In addition to adapting and extending the existing unit tests, I also implemented playlist deletion and adding stream URIs to existing playlist in a feature branch for Mopidy-Mobile, which was my original use case for #937. That's how I also noticed the missing event notification for delete. Therefore I'd really like to see this in a not-too-distant v0.19.x bugfix release ;-)
@tkem

This comment has been minimized.

Member

tkem commented Feb 23, 2015

Now, this is starting to get silly ;-)

Imports statements are in the wrong order. from .translator should be before from .translator
@adamcik

This comment has been minimized.

adamcik commented on mopidy/local/playlists.py in cb2b7e0 Feb 23, 2015

This name should probably be decoded with sys.getfilesystemencoding()

@adamcik

This comment has been minimized.

adamcik commented on mopidy/local/playlists.py in 05f26c1 Feb 23, 2015

Probably makes sense, though I would double check if we would end up with double events in cases like spotify where the playlist change could be initiated externally.

This comment has been minimized.

Owner

tkem replied Feb 24, 2015

After some thinking, I decided this doesn't belong here but warrants its own issue; see mopidy#996.
So I'll remove the event sending from this PR. Clients can initiate a refresh as a workaround until mopidy#996 gets resolved.

@adamcik

This comment has been minimized.

adamcik commented on mopidy/local/playlists.py in cb2b7e0 Feb 23, 2015

Not sure if this should be a warning, we tend to end up with a lot of red herrings if we aren't careful with use of warnings FYI.

This comment has been minimized.

Owner

tkem replied Feb 23, 2015

Sure, but the docs explicitly state that you should not do this.

This comment has been minimized.

adamcik replied Feb 23, 2015

Mhm, as such it's probably OK like this until the TODO just above gets done.

@adamcik

This comment has been minimized.

adamcik commented on mopidy/local/playlists.py in cb2b7e0 Feb 23, 2015

This needs to decode the error with mopidy.utils.encoding.locale_decode or else we risk failing with an UnicodeError with certain foreign locales.

This comment has been minimized.

Owner

tkem replied Feb 23, 2015

But shouldn't the OSError message already be properly localized via perror()? I'm puzzled, never really thought about this... Can you provide more information/links regarding this issue?

This comment has been minimized.

adamcik replied Feb 23, 2015

We get a localized bytestring, which then gets combined with the unicode format string due to unicode_literals and things go kaboom :(

This comment has been minimized.

Owner

tkem replied Feb 23, 2015

Aaargh, I think I see what you mean. Bytestring, sure.

@adamcik

This comment has been minimized.

adamcik commented on mopidy/local/playlists.py in cb2b7e0 Feb 23, 2015

Same as for line 94.

This comment has been minimized.

Owner

tkem replied Feb 24, 2015

Maybe I shouldn't catch the OSError at all, but let the client handle it? I see two major use cases for this:

  1. The file somehow got deleted from disk. In this case, the client can perform a refresh, and everything is back to normal.
  2. The file cannot be removed because of access rights. In this case, the playlist shouldn't be removed either.

Any thoughts on this?

@adamcik

This comment has been minimized.

Member

adamcik commented Feb 23, 2015

Yeah the import ordering check can be a bit annoying, personally I would love if it also told me how to reorder things like I'm used to from the linter at work.

As for storing the playlist name I was looking for a standard #EXTM3U "field" for this, but sadly there doesn't seem to be one. And at least for this PR I don't think inventing our own or switching formats is a good idea.

On that note might want to consider using m3u8 as we are likely not following the "spec"/convention as things are now. But that doesn't need to be squeezed into this PR.

@tkem

This comment has been minimized.

Member

tkem commented Feb 23, 2015

I was considering .m3u8 support myself as a next step, after this gets accepted. As for following specs, I can't see any real deviations from existing practice so far (in all its ugliness), but I may have missed something...

@adamcik

This comment has been minimized.

Member

adamcik commented Feb 23, 2015

For the specs I was thinking of

If an m3u file is edited with a text editor, it must be saved in the Windows-1252 format, which is a superset of ASCII. "m3u" files properly use the Latin-1 character set.[citation needed]

from http://en.wikipedia.org/wiki/M3U

@tkem

This comment has been minimized.

Member

tkem commented Feb 23, 2015

Yes, but that's why the file contents (track titles at least) are encoded as latin-1 (technically not the same as win1252, but close enough for practical purposes). I also couldn't make sense of that second sentence ;-)

@adamcik

This comment has been minimized.

Member

adamcik commented Feb 23, 2015

Ah, forgot the latin-1 stuff was there already. Should have double checked before pestering you with that.

@tkem

This comment has been minimized.

Member

tkem commented Feb 23, 2015

No problem, will provide an update based on your other comments later this week. And hopefully get my imports in order ;-)

tkem added some commits Feb 24, 2015

@tkem tkem added the 2 - Working label Feb 24, 2015

@tkem

This comment has been minimized.

Member

tkem commented Feb 24, 2015

Looks like I missed tests/local/test_events.py; since the playlist_loaded event is already sent here or here (it was sent twice in the original code), and test_events.py only contains that single test, I think we might as well dump test_events.py as a whole...

@tkem tkem added 3 - Done and removed 2 - Working labels Feb 25, 2015

@tkem

This comment has been minimized.

Member

tkem commented Feb 25, 2015

I think I addressed all of @adamcik's comments now (and removed some stuff to their own issues). However, this PR now has somewhat of a history, and I'm feeling a little uncertain about it myself.
So, should we close this and I rebase on develop and re-post, so you can review it in its entirety?

@adamcik

This comment has been minimized.

Member

adamcik commented Feb 25, 2015

Sounds good to me.

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