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

UnicodeError saving m3u playlists with Unicode track titles #1175

Closed
tkem opened this Issue May 10, 2015 · 8 comments

Comments

3 participants
@tkem
Member

tkem commented May 10, 2015

Although care has been taken to encode track titles to latin-1 before writing to an M3U file,
string concatenation with (implicit) Unicode constants in https://github.com/mopidy/mopidy/blob/develop/mopidy/m3u/playlists.py#L96 will cause an error for non-ASCII titles. These constants should be explicitly prefixed with b to generate ASCII strings.

@tkem tkem self-assigned this May 10, 2015

@tkem tkem added the 2 - Working label May 10, 2015

@tkem

This comment has been minimized.

Member

tkem commented May 10, 2015

I'd like to see this fixed in v1.0.5, so I'll provide a PR for the v1.0.x branch ASAP.

@tkem

This comment has been minimized.

Member

tkem commented May 10, 2015

Maybe it would be preferable to use codecs.open() after all, also for reading/parsing. This might also help when m3u8 playlists get implemented. Should also add some non-ASCII title unit tests for m3u.

@tkem

This comment has been minimized.

Member

tkem commented May 10, 2015

codecs.open may also be convenient when porting to Python3. Need to watch for CRLF line endings, though.

@adamcik

This comment has been minimized.

Member

adamcik commented May 10, 2015

https://github.com/kilogram/mopidy/commit/f49ed9a2b421c250e575e9dddf3609daadc1f497 might be relevant for the m3u8 part, noticed @kilogram was working on this from the network view.

@adamcik adamcik added this to the v1.0.5 milestone May 10, 2015

@adamcik adamcik added the C-bug label May 10, 2015

@tkem

This comment has been minimized.

Member

tkem commented May 10, 2015

@adamcik: I'd rather not tackle m3u8 for v1.0.x, since I consider that a new feature (semantic versioning), and there are some open questions regarding the "write part" of the playlists API (what format to use for newly created playlists, do we want to "auto-upgrade" playlists to m3u8 if tracks with non-latin1 titles are added, etc.).
However, refactoring the m3u.translator to take an encoding paramter, use codecs.open to get rid of the encoding mess, and maybe moving the save part into translator would probably be okay for a bugfix release, and will help with adding m3u support in v1.1.
So, that's the plan, at least ;-)

@adamcik

This comment has been minimized.

Member

adamcik commented May 10, 2015

Didn't mean to imply that you tackle that for 1.0.5 release. Your plan for this sounds spot on :-)

@tkem

This comment has been minimized.

Member

tkem commented May 10, 2015

After toying around with this for a while, I think I'll better leave the parsing stuff as-is until later, and just fix the issue at hand and add some basic latin-1/utf-8 unit tests to tests/m3u/test_playlists.py.

@adamcik

This comment has been minimized.

Member

adamcik commented May 10, 2015

Fixed in #1176

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