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 plugin #3145

Merged
merged 14 commits into from Feb 17, 2019

Conversation

Projects
None yet
2 participants
@Holzhaus
Copy link
Contributor

commented Feb 15, 2019

Adds M3U playlist support as a query to beets and thus partially
resolves issue #123. The implementation is heavily based on #2380 by
Robin McCorkell.

It supports referencing playlists by absolute path:

$ beet ls playlist:/path/to/someplaylist.m3u

It also supports referencing playlists by name. The playlist is then
seached in the playlist_dir and the .m3u extension is appended to the
name:

$ beet ls playlist:anotherplaylist

The configuration for the plugin looks like this:

playlist:
    relative_to: library
    playlist_dir: /path/to/playlists

The relative_to option specifies how relative paths in playlists are
handled. By default, paths are relative to the library directory. It
also possible to make them relative to the playlist or set the option
or set it to a fixed path.

@Holzhaus

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2019

One of the main problems is that the query is rather slow with large libraries. Since key in model_cls._fields evaluates to False, the col_clause() implemention will not be used. Any ideas?

@Holzhaus Holzhaus force-pushed the Holzhaus:playlist-plugin branch from 2ceb8ef to 9887f1e Feb 15, 2019

@sampsyo
Copy link
Member

left a comment

Cool! This is a really neat prototype.

I think the issue you're running into with the query is a bit more fundamental than it might seem at first: field-qualified queries like foo:bar are currently quite intrinsically tied to the existence of a field called foo (either built-in or flexible). You're introducing a type for a "fake" field called playlist here, which works but isn't really what the query system is expecting. Because PlaylistQuery is a FieldQuery, the system really wants to do a playlist-style query of some field (and that field, here, is always called playlist).

I agree that the syntax playlist:foo would be desirable for this plugin. So maybe it's worth digging a little bit into building special support for these "pseudo-fields" for invoking special queries with field-like syntax but without querying actual, underlying fields. For what it's worth, the random plugin (which currently does not define a query) could use this too: a query term like random:5 could use the same mechanism.

@@ -345,7 +345,7 @@ def _sort_candidates(candidates):
return sorted(candidates, key=lambda match: match.distance)


def _add_candidate(items, results, info):
def _add_candidate(items, results, info, force=False):

This comment has been minimized.

Copy link
@sampsyo

sampsyo Feb 16, 2019

Member

This addition of the force parameter seems to be an unrelated change that got included in this PR by mistake?

This comment has been minimized.

Copy link
@Holzhaus

Holzhaus Feb 17, 2019

Author Contributor

Yeah, sorry about that.

config = beets.config['playlist']

# Get the full path to the playlist
if os.path.isabs(beets.util.syspath(pattern)):

This comment has been minimized.

Copy link
@sampsyo

sampsyo Feb 16, 2019

Member

I might recommend using isfile. That way, anything that refers to an actual file on disk will work, and if the file doesn't exist, we'll fall back on searching playlist_dir.

This comment has been minimized.

Copy link
@Holzhaus

Holzhaus Feb 17, 2019

Author Contributor

That would introduce a race condition in case the file is deleted between the isfile and the open call. With proper error handling this wouldn't be too bad, but I restructured the code a bit so that isfile is not needed at all.

This comment has been minimized.

Copy link
@sampsyo

sampsyo Feb 17, 2019

Member

Looks perfect. 👍

relative_to = beets.util.bytestring_path(relative_to)

self.paths = []
with open(beets.util.syspath(playlist_path), 'rb') as f:

This comment has been minimized.

Copy link
@sampsyo

sampsyo Feb 16, 2019

Member

It might be nice to have some error handling here in case the playlist does not exist (either as a plain filename or in the centralized playlist directory).

This comment has been minimized.

Copy link
@Holzhaus

Holzhaus Feb 17, 2019

Author Contributor

Done. It would be nice to print some error message to the console, but I don't know how. I don't really want to assign the self._log attribute of PlaylistPlugin to the PlaylistQuery class, but I don't see another way to do this.

This comment has been minimized.

Copy link
@sampsyo

sampsyo Feb 17, 2019

Member

Good point. Threading through the plugin-specific log object here seems too hard. Maybe we can work something out as part of the more general "pseudo-field" query infrastructure.

# Playlist is empty
return '0', ()
clause = 'BYTELOWER(path) IN ({0})'.format(
', '.join('BYTELOWER(?)' for path in self.paths))

This comment has been minimized.

Copy link
@sampsyo

sampsyo Feb 16, 2019

Member

I note that this query is always case-insensitive. That's probably fine, but it's probably worth noting, at least in the docstring for this class. (See the built-in PathQuery if you're curious—it can either be case-sensitive or case-insensitive, depending on the filesystem.)

This comment has been minimized.

Copy link
@Holzhaus

Holzhaus Feb 17, 2019

Author Contributor

Well, the problem is that Playlists can contain multiple entries from different filesystems. I should probably make this case sensitive - case-insensitivity is more a convenience feature for FAT/NTFS users and not actually needed.

This comment has been minimized.

Copy link
@sampsyo

sampsyo Feb 17, 2019

Member

Also a good point! Case-sensitive sounds fine; we can revisit this if it gets inconvenient.

- **relative_to**: Interpret paths in the playlist files relative to a base
directory. It is also possible to set it to ``playlist`` to use the
playlist's parent directory as base directory.
Default: ``library``

This comment has been minimized.

Copy link
@sampsyo

sampsyo Feb 16, 2019

Member

It's probably a good idea to mention explicitly that there are three options here: playlist, library, or a path.

This comment has been minimized.

Copy link
@Holzhaus

Holzhaus Feb 17, 2019

Author Contributor

Done.

Holzhaus added some commits Jan 10, 2017

playlist: Add playlist plugin
Adds M3U playlist support as a query to beets and thus partially
resolves issue #123. The implementation is heavily based on #2380 by
Robin McCorkell.

It supports referencing playlists by absolute path:

    $ beet ls playlist:/path/to/someplaylist.m3u

It also supports referencing playlists by name. The playlist is then
seached in the playlist_dir and the ".m3u" extension is appended to the
name:

    $ beet ls playlist:anotherplaylist

The configuration for the plugin looks like this:

    playlist:
        relative_to: library
        playlist_dir: /path/to/playlists

The relative_to option specifies how relative paths in playlists are
handled. By default, paths are relative to the "library" directory. It
also possible to make them relative to the "playlist" or set the option
or set it to a fixed path.
playlist: Improve speed in PlaylistQuery class
Implement the col_clause method for faster, sqlite-based querying. This
will only make a difference if the "fast" kwarg is set to True.

@Holzhaus Holzhaus force-pushed the Holzhaus:playlist-plugin branch from 83ef4dc to d52dcdd Feb 17, 2019

@Holzhaus

This comment has been minimized.

Copy link
Contributor Author

commented Feb 17, 2019

For some reason some testcase on Windows test keep throwing errors:
https://ci.appveyor.com/project/beetbox/beets/builds/22435461/job/prrm5wikfjp8xvm2#L122

Might be related to spaces in the path, but I'm not sure and I can't fix this since I don't have Windows. The other tests should work fine now.

@Holzhaus

This comment has been minimized.

Copy link
Contributor Author

commented Feb 17, 2019

Looks like spaces in the path were indeed the problem. Should work now.

The performance issue could be fixed via #3149. If that PR is merged, we only need to override the check_fast method in PlaylistQuery with this one:

@classmethod
def check_fast(self, key, model_cls):
    return issubclass(model_cls, beets.library.Item)
@Holzhaus

This comment has been minimized.

Copy link
Contributor Author

commented Feb 17, 2019

Is anything else missing? Should I squash the commits?

@sampsyo

This comment has been minimized.

Copy link
Member

commented Feb 17, 2019

Awesome! This looks perfect. Thank you for tracking down the Windows problem even without access to a Windows machine—that was heroic.

No need for a squash from my perspective. I'll merge this now. 🚀

@sampsyo sampsyo merged commit 4f1a468 into beetbox:master Feb 17, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

sampsyo added a commit that referenced this pull request Feb 17, 2019

sampsyo added a commit that referenced this pull request Feb 17, 2019

sampsyo added a commit that referenced this pull request Feb 17, 2019

Prototype support for named (pseudo-field) queries
As discussed here:
#3145 (review)

This would replace the need for #3149.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.