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

playlist: Add option to force forward slashes in paths #3331

Closed
MartyLake opened this issue Jul 19, 2019 · 11 comments · Fixed by #3334
Closed

playlist: Add option to force forward slashes in paths #3331

MartyLake opened this issue Jul 19, 2019 · 11 comments · Fixed by #3334
Labels
feature features we would like to implement

Comments

@MartyLake
Copy link

Use case

Following up on #irc and MusicPlayerDaemon/MPD#607
I'm trying to use beets to export playlists for MPD. Unfortunately, I am using Windows, and beets uses backslashes on windows, and MPD can only recognize forwards.

Example of playlist that works fixed by bash+sed:

#EXTINF:346,Le Knight Club - Coco Girlz
Le Knight Club/Ilictronix/01 Coco Girlz.mp3
#EXTINF:325,Alec Carlsson - OMG
Compilations/Shiny Disco Club Presents_ Millenium Disco (vol.1)/18 OMG.mp3
#EXTINF:190,Calypso
Compilations/Shiny Disco Club Presents_ Millenium Disco (vol.1)/12 Calypso.mp3
#EXTINF:225,Kartell - Ending Season
Compilations/Shiny Disco Club Presents_ Millenium Disco (vol.1)/19 Ending Season.mp3

Example of playlist that does not work, generated by beets.io:

Compilations\Sakifo Musik Festival 2006\05 Avec Le Vent.mp3
Compilations\Disco Hits\00 Ring My Bell (badq).mp3
Anita Ward\Maximum Disco 12''\00 Ring My Bell (Original Disco Version).mp3
BlastRec\Volume Zero\09 Astrid Eve.mp3
Django Unchained\Original Motion Picture Soundtrack\23 Trinity (Titoli).mp3

Solution

add a new option called separator_char that, if set, forces this character to be used as a separator.
or
add a new option called type_of_separator that defaults to auto (current behavior), but can also be force_backslash or force_forward

Alternatives

Have a post script hook where I can call my own bash+sed solution

@jackwilsdon jackwilsdon added the feature features we would like to implement label Jul 19, 2019
@sampsyo sampsyo changed the title Playlist plugin add option to force forward or backward slashes playlist: Add option to force Windows-style backslashes in paths Jul 19, 2019
@sampsyo
Copy link
Member

sampsyo commented Jul 19, 2019

Sounds like a reasonable feature! Perhaps there is a built-in os.path module that normalizes a path on Windows to use backslashes. I think it would be fine to have a binary option—when it's off, it does the current behavior; when it's on, it normalizes the paths in the Windows MPD-compatible style.

@MartyLake MartyLake changed the title playlist: Add option to force Windows-style backslashes in paths playlist: Add option to force forward slashes in paths Jul 21, 2019
@MartyLake
Copy link
Author

MartyLake commented Jul 21, 2019

Just to clarify, Windows MPD expects forward slashes "/" so I think the feature would be the opposite of the text you've written (addendum in bold is my addition):

have a binary option—when it's off, it does the current behavior; when it's on, it normalizes the paths in Windows MPD-compatible style, which is foward slash "/" only.

Do you have any advice on how to get started on this? I am eager to contribute a pull request if I can :)

update1:
It seems that

new_path = os.path.relpath(new_path, base_dir)

and
item_path = os.path.relpath(item.path, relative_to)

are the place where the conversion happens (the code is super readable!!!!)

update2:
I have tried on my win10's python3.6 REPL and this seems to work:

>>> from pathlib import Path
>>> p = Path("Various Artists\Beatport 2017\9492281_Itsa_Trumpet_Thing_Extended_Mix.mp3")
>>> str(p.as_posix())
'Various Artists/Beatport 2017/9492281_Itsa_Trumpet_Thing_Extended_Mix.mp3'
>>>

@sampsyo
Copy link
Member

sampsyo commented Jul 21, 2019

Cool! Looks like you're on your way there.

One nit is that pathlib is only available on Python 3.4+. Could be worth seeing if the implementation of that method is simple enough that we could just call some underlying function…

Anyway, throwing this behind a config option seems good!

@MartyLake
Copy link
Author

MartyLake commented Jul 21, 2019

>>> from pathlib import Path
>>> p = Path("Various Artists\Beatport 2017\9492281_Itsa_Trumpet_Thing_Extended_Mix.mp3")
>>> p.as_posix
<bound method PurePath.as_posix of WindowsPath('Various Artists/Beatport 2017/9492281_Itsa_Trumpet_Thing_Extended_Mix.mp3')>
>>> import inspect
>>> inspect.getsourcelines(p.as_posix)
(['    def as_posix(self):\n', '        """Return the string representation of the path with forward (/)\n', '        slashes."""\n', '        f = self._flavour\n', "        return str(self).replace(f.sep, '/')\n"], 703)

so this means that the method is defined like:

    def as_posix(self):
        """Return the string representation of the path with forward (/)
        slashes."""
        f = self._flavour
        return str(self).replace(f.sep, '/')

I imagine here since beets already uses only os.path, it means that f.sep == os.sep ?

@sampsyo
Copy link
Member

sampsyo commented Jul 21, 2019

Oh wow, that's quite a bit simpler than what I was imagining!

One concern, however: I imagine that the absolute paths that beets stores on Windows start with a drive letter, like C:\foo\bar\.... Will just replacing \ with /, to obtain C:/foo/bar, work with MPD?

@MartyLake
Copy link
Author

Surprisingly yes, I was waiting for something with lots of edge cases !!!
That's the current behavior of as_posix yes

>>> from pathlib import Path
>>> p = Path("C:\Program Files\internet explorer\iexplore.exe")
>>> p.as_posix()
'C:/Program Files/internet explorer/iexplore.exe'

MPD uses relative path, relative to the root of the library (and not the playlist files). MusicPlayerDaemon/MPD#200
And I only want to deal with relative path. Should I name the option force_relative_path_forward_slash then ?

@sampsyo
Copy link
Member

sampsyo commented Jul 21, 2019

Nah, no need for that—we already have a relative_to option. It's better to have smaller, composable, broadly useful options than fewer, monolithic, specialized options.

Let's just add an option called forward_slash and call it a day!

@MartyLake
Copy link
Author

MartyLake commented Jul 22, 2019

https://github.com/MartyLake/beets/tree/martylake_add_forward_slash_option

I think I am quite close to the goal, but encoding always beats me at this game =/ do you have any advice on how to get any further ? why do we use "bytes" instead of string in the playlist code ?

It prints
b'Air//Pocket Symphony//04 Napalm Love.mp3'
instead of
Air/Pocket Symphony/04 Napalm Love.mp3

@sampsyo
Copy link
Member

sampsyo commented Jul 22, 2019

All paths in beets are bytes, not strings, because Unix paths are bytes—they don't necessarily use any particular text encoding, and certainly not consistently anything Unicode. See also: http://beets.io/blog/paths.html

In your code, I think it should be fine to just use bytestring replacement, i.e., path.replace(b'\\', b'/'). For example:

>>> b'foo'.replace(b'o', b'x')
b'fxx'

@MartyLake
Copy link
Author

Oh my, I remember having read thig article a long time ago... I am so glad that you figured things out before e !!!
I've force pushed the solution

@MartyLake
Copy link
Author

For the record, MaxKellermann also fixed it for MPD 2 hours ago. MusicPlayerDaemon/MPD@90ea3bf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature features we would like to implement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants