-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
convert: New feature "Write m3u playlist to destination folder" #4399
Merged
Merged
Changes from all commits
Commits
Show all changes
46 commits
Select commit
Hold shift + click to select a range
861bc69
convert: Add a quick & dirty m3u playlist feature
JOJ0 d448e0c
convert: Refine and fix playlist feature
JOJ0 fd8fe69
convert: Playlist feature linting fixes
JOJ0 16e25bb
convert: playlist feature: Fix relative paths
JOJ0 c0b1bc9
convert: playlist feature: Better relative path gen
JOJ0 d589e77
convert: playlist: Fix redundant join path
JOJ0 c251ed1
convert: playlist: Generate m3u file in one batch
JOJ0 5dfff50
convert: playlist: Refactor m3u writing to class
JOJ0 9930a5d
convert: playlist: Test playlist existence
JOJ0 e41525a
convert: playlist: Remove clutter from test
JOJ0 55b3863
convert: playlist: Move m3u creation after conversions
JOJ0 ba3740c
convert: playlist: Fix filename attr in load method
JOJ0 0cbf91e
convert: playlist: Add test_m3ufile and fixtures
JOJ0 68240f6
convert: playlist: Add EmptyPlaylistError and test
JOJ0 39e4b90
convert: playlist: Add tests checking extm3u and
JOJ0 01b77f5
convert: playlist: Add changelog entry
JOJ0 7d121c3
convert: playlist: Move M3UFile class to separate
JOJ0 8dc556d
convert: playlist: Use syspath()
JOJ0 cb630c4
convert: playlist: Also use syspath() for contents
JOJ0 c1908d5
convert: playlist: Document the feature
JOJ0 2c1163c
convert: playlist: Linter and import fixes
JOJ0 a1baf9e
convert: playlist: Fix rst linter error in docs
JOJ0 785ef15
convert: playlist: Use syspath() for media files
JOJ0 da01be3
convert: playlist: Enforce utf-8 encoding on load()
JOJ0 5f5be52
convert: playlist: Debug commit: Learn syspath()
JOJ0 bd5335f
convert: playlist: Separate unicode test for Windows
JOJ0 b3d0c1c
Revert "convert: playlist: Debug commit: Learn syspath()"
JOJ0 004d10a
convert: playlist: Put actual Windows paths
JOJ0 31b9e7a
convert: playlist: Construct Windows path programatically
JOJ0 e421371
convert: playlist: Disable prefix in syspath on
JOJ0 54d22be
convert: playlist: Construct winpath before assert
JOJ0 a641fd1
convert: playlist: debug winpath in test
JOJ0 39efd23
convert: playlist: Fix winpath driveletter in test
JOJ0 ff03eca
convert: playlist: Add another Windows test
JOJ0 c28eb95
convert: playlist: Remove debug print winpath
JOJ0 d248063
convert: playlist: Improve --playlist help text
JOJ0 068208f
convert: Fix copyright year in test_m3ufile.py
JOJ0 20a0012
convert: playlist: Use normpath for playlist file
JOJ0 46fb8fe
convert: playlist: Fix typo in m3u module docstring
JOJ0 952aa0b
convert: playlist: Handle playlist path subdirs
JOJ0 0884e67
convert: playlist: Handle errors on read/write
JOJ0 a4d03ef
convert: playlist: M3U write + contents as bytes
JOJ0 9923116
convert: playlist: M3U read as bytes
JOJ0 16e361b
convert: playlist: item_paths relative to playlist
JOJ0 86929eb
convert: playlist: Adapt code comments
JOJ0 94784c2
convert: playlist: Documentation overhaul
JOJ0 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,93 @@ | ||
# This file is part of beets. | ||
# Copyright 2022, J0J0 Todos. | ||
# | ||
# Permission is hereby granted, free of charge, to any person obtaining | ||
# a copy of this software and associated documentation files (the | ||
# "Software"), to deal in the Software without restriction, including | ||
# without limitation the rights to use, copy, modify, merge, publish, | ||
# distribute, sublicense, and/or sell copies of the Software, and to | ||
# permit persons to whom the Software is furnished to do so, subject to | ||
# the following conditions: | ||
# | ||
# The above copyright notice and this permission notice shall be | ||
# included in all copies or substantial portions of the Software. | ||
|
||
"""Provides utilities to read, write and manipulate m3u playlist files.""" | ||
|
||
import traceback | ||
|
||
from beets.util import syspath, normpath, mkdirall, FilesystemError | ||
|
||
|
||
class EmptyPlaylistError(Exception): | ||
"""Raised when a playlist file without media files is saved or loaded.""" | ||
pass | ||
|
||
|
||
class M3UFile(): | ||
"""Reads and writes m3u or m3u8 playlist files.""" | ||
def __init__(self, path): | ||
"""``path`` is the absolute path to the playlist file. | ||
|
||
The playlist file type, m3u or m3u8 is determined by 1) the ending | ||
being m3u8 and 2) the file paths contained in the list being utf-8 | ||
encoded. Since the list is passed from the outside, this is currently | ||
out of control of this class. | ||
""" | ||
self.path = path | ||
self.extm3u = False | ||
self.media_list = [] | ||
|
||
def load(self): | ||
"""Reads the m3u file from disk and sets the object's attributes.""" | ||
pl_normpath = normpath(self.path) | ||
try: | ||
with open(syspath(pl_normpath), "rb") as pl_file: | ||
raw_contents = pl_file.readlines() | ||
except OSError as exc: | ||
raise FilesystemError(exc, 'read', (pl_normpath, ), | ||
traceback.format_exc()) | ||
|
||
self.extm3u = True if raw_contents[0].rstrip() == b"#EXTM3U" else False | ||
for line in raw_contents[1:]: | ||
if line.startswith(b"#"): | ||
# Support for specific EXTM3U comments could be added here. | ||
continue | ||
self.media_list.append(normpath(line.rstrip())) | ||
if not self.media_list: | ||
raise EmptyPlaylistError | ||
|
||
def set_contents(self, media_list, extm3u=True): | ||
"""Sets self.media_list to a list of media file paths. | ||
|
||
Also sets additional flags, changing the final m3u-file's format. | ||
|
||
``media_list`` is a list of paths to media files that should be added | ||
to the playlist (relative or absolute paths, that's the responsibility | ||
of the caller). By default the ``extm3u`` flag is set, to ensure a | ||
save-operation writes an m3u-extended playlist (comment "#EXTM3U" at | ||
the top of the file). | ||
""" | ||
self.media_list = media_list | ||
self.extm3u = extm3u | ||
|
||
def write(self): | ||
"""Writes the m3u file to disk. | ||
|
||
Handles the creation of potential parent directories. | ||
""" | ||
header = [b"#EXTM3U"] if self.extm3u else [] | ||
if not self.media_list: | ||
raise EmptyPlaylistError | ||
contents = header + self.media_list | ||
pl_normpath = normpath(self.path) | ||
mkdirall(pl_normpath) | ||
|
||
try: | ||
with open(syspath(pl_normpath), "wb") as pl_file: | ||
for line in contents: | ||
pl_file.write(line + b'\n') | ||
pl_file.write(b'\n') # Final linefeed to prevent noeol file. | ||
except OSError as exc: | ||
raise FilesystemError(exc, 'create', (pl_normpath, ), | ||
traceback.format_exc()) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
#EXTM3U | ||
/This/is/a/path/to_a_file.mp3 | ||
/This/is/another/path/to_a_file.mp3 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
#EXTM3U | ||
/This/is/å/path/to_a_file.mp3 | ||
/This/is/another/path/tö_a_file.mp3 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
/This/is/a/path/to_a_file.mp3 | ||
/This/is/another/path/to_a_file.mp3 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
#EXTM3U | ||
x:\This\is\å\path\to_a_file.mp3 | ||
x:\This\is\another\path\tö_a_file.mp3 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wisp3rwind may I ask for your advice on a specific thing here: I recently came across about a PR where you cleaned up a little and moved the wrapping through syspath to the very last thing happening before a filesystem operation is done by beets: https://github.com/beetbox/beets/pull/4675/files#diff-1ab0ce830f9dd337e8c57f809d3488b4dfb78c12a61a2a526100038e05a7ca54R29
I think back when I wrote this feature / opened this PR I did not have a good understanding of what and why
syspath()
wrapping is required. Have a look (best via "Files changed" tab) at line 488 above where I'm generatingitems_paths
In words: I'm generating a list of paths to media files that then will be saved into an m3u playlist file. I wrap every one of these paths through
syspath()
I now think that this actually is not the purpose of
syspath()
since I do not actually do a filesystem operation from within beets. I should not usesyspath()
here. Is that correct?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an aside, I'm preparing to go through the codebase and convert everything to
pathlib
which will negate the need for syspath calls.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah pathlib! That is a cool package! I use it all the time in my projects! Great idea!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds right: Currently, beets' code represents every path internally as a
bytes
object. To keep things manageable, paths should be converted to this representation immediately upon ingestion from any source (CLI, input files, library APIs, ...), and converted to other representations as late as possible (usually just upon leaving beets' direct control, i.e. when writing them to a file or the DB, etc).To be honest, I'm also not entirely sure without studying the code again what exactly is the right approach when receiving paths from, say, the commandline. Anyway, yes,
syspath
seems like the wrong function here, at least without further research on whether it does the right thing on each platform. The question to answer here would be how m3u files should normally be encoded in each case, and ensure that beets adheres to that.As a matter of fact, does your code actually run on Linux? At a first glance, I would expect that
M3UFile.write()
would crash when trying to join the paths (bytes
as returned bysyspath
) with newlines.Sorry for the late reply, I missed that you mentioned me here!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @wisp3rwind thanks a lot for the reply, late or not, it's helpful! :-)
Code is tested on macOS Catalina and Manjaro Linux (my former dev machine and my current main dev machine ;-))
One thing before I answer more: Is it possible that you are confusing the
syspath
function withnormpath
or evenbytestring_path
(which is called bynormpath
)Reason being:
syspath
doesn't do anything on Linux/Posix, it just returns as-is. As I understoodsyspath
, it actually is ment for sanitizing Windows paths:beets/beets/util/__init__.py
Lines 395 to 426 in 2e18f84
beets/beets/util/__init__.py
Lines 138 to 144 in 2e18f84
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly! I did my research back then when I started to code this around last summer (See description and todos here: #4399 (comment)). It turns out that there is no real standard of what m3u is. It is rather loose, therefore tools do what the dev's thought it should do. The best we can do is do a compromise to work for the most circumstances. The main points from my research are:
We find a little about this here: https://en.wikipedia.org/wiki/M3U#M3U8
So to summarize what my goal is and what I think makes the most sense for usability (for this feature):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry, now I understand: Since internally we are having
bytes
paths,.syspath
, even when not doing anything on Non-Windows systems, will return exactly that: The internal representation of paths, we still havebytes
So to make my question more concrete now @wisp3rwind : My goal is to make sure that all paths that go in the m3u list are converted to UTF-8. Which function from utils could I use?
displayable_path()
? Sounds odd but actually it translatesbytes
tounicode
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
beets/beets/util/__init__.py
Lines 376 to 392 in 2e18f84