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

[WIP] Add PlaylistQuery plugin #2380

Closed
wants to merge 3 commits into from
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 70 additions & 0 deletions beetsplug/playlistquery.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
# -*- coding: utf-8 -*-
# This file is part of beets.
#
# 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.

import os
from beets.plugins import BeetsPlugin
from beets.dbcore import FieldQuery, types
from beets.util.confit import NotFoundError

class PlaylistQuery(FieldQuery):
"""Matches files listed by a playlist file.
"""
relative_path = None
playlist_dir = None

def __init__(self, field, pattern, fast=True):
super(PlaylistQuery, self).__init__(field, pattern, fast)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you want fast=False here unless you implement a clause method (i.e., a pure-SQL way of executing the query).


playlist_file = pattern + '.m3u'
playlist_path = os.path.join(self.playlist_dir, playlist_file)

self.paths = []
with open(playlist_path, 'r') as f:
for line in f:
if line[0] == '#':
# ignore comments, and extm3u extension
continue
self.paths.append(os.path.normpath(
os.path.join(self.relative_path, line.decode('utf-8').rstrip())
))

def match(self, item):
return item.path.decode('utf-8') in self.paths


class PlaylistType(types.String):
"""Custom type for playlist query.
"""
query = PlaylistQuery


class PlaylistQueryPlugin(BeetsPlugin):
item_types = {'playlist': PlaylistType()}

def __init__(self):
super(PlaylistQueryPlugin, self).__init__()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll likely want to set defaults for the plugin's configuration here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the documentation could benefit from a small section on how to specify config defaults for plugins, it is currently missing so I had to look at other plugins for examples.

self.register_listener('library_opened', self.library_opened)

PlaylistQuery.playlist_dir = self.config['playlist_dir'].as_filename()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using global state to keep track of this could be a little problematic for testing—the right thing to do here might be to look up the configuration option directly in the query class.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How will this work with access needed to lib? The Query objects don't get any context, so the best place I thought to put it was in the library_opened event on PlaylistQueryPlugin, which in turn requires some sort of global state passed to PlaylistQuery for instances to use. Unless there's an API that I haven't seen?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, yes, that is a problem. We haven't had a case where a query needed a reference to the database before—maybe we need to add that capability?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment a Query object is constructed with the field and pattern, which makes it very clean. I think it is unwise to expand that in the default case, since most of the time it will go unused. However, I can see potential for the Builder pattern here - have a Plugin object instantiate Query objects that it controls.

So currently, construct_query_part() in dbcore.queryparse directly constructs the selected query_class. Instead, it should know which Plugin object corresponds to each query class, then calls on that object to construct the desired class. The default implementation in BeetsPlugin would do what construct_query_part() does at the moment (i.e. check if we have a FieldQuery, and construct in two different ways) but can be overridden in custom plugins to do more complex things, passing in other information if desired.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure; good idea. Another relatively simple way would be to use a closure—that is, the plugin would just set self.item_types to reference a class that can access lib from the outer context.

I suppose one way to judge whether we've done it "right" is to ask whether the plugin would break if beets were ever to support multiple libraries for some reason—if there weren't a single, global Library object but queries would work on whatever library they're querying.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't think of a good way to avoid using global state somewhere, without rewriting how the entire mechanism for registering queries works. Closures wouldn't work easily, since the PlaylistQuery class is defined inside PlaylistType, of which an instance belongs to PlaylistQueryPlugin, so the PlaylistQuery class inside the (now global) PlaylistType instance needs to be changed on library_opened events, taking you right back to mutable global state.

I don't think these two configuration variables as global state is necessarily a problem, though. If you consider PlaylistQueryPlugin and PlaylistQuery to be separate units of code, then each can be unit tested independently, where the global state isn't such a big deal.


def library_opened(self, lib):
try:
relative_to = self.config['relative_to'].as_choice(['base', 'playlist'])
except NotFoundError:
relative_to = 'base'

if relative_to == 'playlist':
PlaylistQuery.relative_path = PlaylistQuery.playlist_dir
else:
PlaylistQuery.relative_path = lib.directory
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a difference between setting playlist_dir to be the same as directory vs. setting relative_to to base?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes: playlist_dir affects where playlists are stored (and referenced from, so a playlist:foobar looks up $playlist_dir/foobar.m3u) while relative_to affects how relative paths inside the M3U file are handled. There is no formal M3U spec (https://en.wikipedia.org/wiki/M3U#File_format) so paths may be relative to the playlist file (relative_to = playlist) or relative to the base music directory (ncmpcpp does this, relative_to = base)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, that makes sense! Thanks for clarifying.