From 0d762cd269b03312de841a02810614dc179cebf3 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Tue, 26 Feb 2013 13:35:26 -0800 Subject: [PATCH 01/32] first half-attempt at a flexible attribute schema This uses two tables, item_attributes and album_attributes, as key/value tables for the respective entities. Plugins register fields, at which point they are magically materialized as properties on Items (Albums are not done). This currently supports adding and modifying these fields but not retrieving them (which will need some sort of join). --- beets/library.py | 67 ++++++++++++++++++++++++++++++++++++------------ beets/plugins.py | 20 +++++++++++++++ 2 files changed, 71 insertions(+), 16 deletions(-) diff --git a/beets/library.py b/beets/library.py index dd2a1ee7f..6b7b087ee 100644 --- a/beets/library.py +++ b/beets/library.py @@ -273,8 +273,8 @@ class Item(object): """If key is an item attribute (i.e., a column in the database), returns the record entry for that key. """ - if key in ITEM_KEYS: - return self.record[key] + if key in ITEM_KEYS or key in plugins.item_fields(): + return self.record.get(key) else: raise AttributeError(key + ' is not a valid item field') @@ -293,7 +293,7 @@ class Item(object): elif isinstance(value, buffer): value = str(value) - if key in ITEM_KEYS: + if key in ITEM_KEYS or key in plugins.item_fields(): # If the value changed, mark the field as dirty. if (key not in self.record) or (self.record[key] != value): self.record[key] = value @@ -1067,6 +1067,8 @@ class Library(BaseLibrary): # Set up database schema. self._make_table('items', item_fields) self._make_table('albums', album_fields) + self._make_attribute_table('item') + self._make_attribute_table('album') def _make_table(self, table, fields): """Set up the schema of the library file. fields is a list of @@ -1111,6 +1113,22 @@ class Library(BaseLibrary): with self.transaction() as tx: tx.script(setup_sql) + def _make_attribute_table(self, entity): + """Create a table and associated index for flexible attributes + for the given entity (if they don't exist). + """ + with self.transaction() as tx: + tx.script(""" + CREATE TABLE IF NOT EXISTS {0}_attributes ( + id INTEGER PRIMARY KEY, + entity_id INTEGER, + key TEXT, + value TEXT, + UNIQUE(entity_id, key) ON CONFLICT REPLACE); + CREATE INDEX IF NOT EXISTS {0}_id_attribute + ON {0}_attributes (entity_id); + """.format(entity)) + def _connection(self): """Get a SQLite connection object to the underlying database. One connection object is created per thread. @@ -1235,9 +1253,22 @@ class Library(BaseLibrary): subvars.append(value) # Issue query. - query = 'INSERT INTO items (' + columns + ') VALUES (' + values + ')' with self.transaction() as tx: - new_id = tx.mutate(query, subvars) + # Main table insertion. + new_id = tx.mutate( + 'INSERT INTO items (' + columns + ') VALUES (' + values + ')', + subvars + ) + + # Flexible attributes. + for key in plugins.item_fields(): + value = getattr(item, key) + if value is not None: + tx.mutate( + 'INSERT INTO item_attributes (entity_id, key, value)' + ' VALUES (?, ?, ?)', + (new_id, key, value) + ) item._clear_dirty() item.id = new_id @@ -1269,21 +1300,25 @@ class Library(BaseLibrary): if key == 'path' and isinstance(value, str): value = buffer(value) subvars.append(value) - - if not assignments: - # nothing to store (i.e., nothing was dirty) - return - assignments = assignments[:-1] # Knock off last , - # Finish the query. - query = 'UPDATE items SET ' + assignments + ' WHERE id=?' - subvars.append(store_id) - with self.transaction() as tx: - tx.mutate(query, subvars) - item._clear_dirty() + # Main table update. + if assignments: + query = 'UPDATE items SET ' + assignments + ' WHERE id=?' + subvars.append(store_id) + tx.mutate(query, subvars) + # Flexible attributes. + for key in plugins.item_fields(): + if item.dirty.get(key) or store_all: + tx.mutate( + 'INSERT INTO item_attributes (entity_id, key, value)' + ' VALUES (?, ?, ?)', + (store_id, key, getattr(item, key)) + ) + + item._clear_dirty() self._memotable = {} def remove(self, item, delete=False, with_album=True): diff --git a/beets/plugins.py b/beets/plugins.py index fbc863227..177fce032 100755 --- a/beets/plugins.py +++ b/beets/plugins.py @@ -48,6 +48,8 @@ class BeetsPlugin(object): self.template_funcs = {} if not self.template_fields: self.template_fields = {} + self.item_fields = [] + self.album_fields = [] def commands(self): """Should return a list of beets.ui.Subcommand objects for @@ -288,6 +290,24 @@ def import_stages(): stages += plugin.import_stages return stages +def item_fields(): + """Get a list of strings indicating registered flexible Item + attributes. + """ + fields = [] + for plugin in find_plugins(): + fields += plugin.item_fields + return fields + +def album_fields(): + """Get a list of strings indicating registered flexible Album + attributes. + """ + fields = [] + for plugin in find_plugins(): + fields += plugin.album_fields + return fields + # Event dispatch. From f5d658c58fd2132876fb1a6ab636ebc606cbeb28 Mon Sep 17 00:00:00 2001 From: steini Date: Mon, 4 Mar 2013 05:55:43 +0000 Subject: [PATCH 02/32] An attempt at flexible attributes with plugin namespaces. Mostly untested. --- beets/library.py | 102 ++++++++++++++++++++++++++++++++++++++--------- beets/plugins.py | 31 +++++++++----- 2 files changed, 104 insertions(+), 29 deletions(-) diff --git a/beets/library.py b/beets/library.py index 6b7b087ee..8a2ea92d4 100644 --- a/beets/library.py +++ b/beets/library.py @@ -229,6 +229,28 @@ def format_for_path(value, key=None, pathmod=None): class InvalidFieldError(Exception): pass +class FixedDict(dict): + """A dict where keys can not be added after creation and keys are marked + as 'dirty' when their values are changed. + Accepts a list of `keys` and an optional `values` which should + be a dict compatible object. + Any key not in `values` will be implicitly added and given a `None` value. + """ + def __init__(self, keys, values=None): + values = values or {} + super(FixedDict, self).__init__(values) + seti = super(FixedDict, self).__setitem__ + for key in keys: + if not key in self: + seti(key, None) + self.dirty = {} + + def __setitem__(self, key, value): + if key not in self: + raise KeyError('{} is not a valid key.'.format(key)) + elif value != self[key]: + super(FixedDict, self).__setitem__(key, value) + self.dirty[key] = True # Library items (songs). @@ -252,12 +274,34 @@ class Item(object): def _fill_record(self, values): self.record = {} + for key in ITEM_KEYS: try: setattr(self, key, values[key]) except KeyError: setattr(self, key, None) + #make the initial, empty flexible attribute dict + flexattrns = plugins.item_fields() + self.flexattrs = FixedDict(flexattrns.keys()) + for key in self.flexattrs.iterkeys(): + self.flexattrs[key] = FixedDict(flexattrns[key]) + + #treat flexattrs values as they would come from the database + if values['flexkeys'] and values['flexvalues'] and values['flexns']: + flexkeys = values['flexkeys'].split(',') + flexvalues = values['flexvalues'].split(',') + flexns = values['flexns'].split(',') + for i, key in enumerate(flexkeys): + namespace = flexns[i] + if namespace not in self.flexattrs: + #not an active plugin, skip + continue + value = flexvalues[i] + if not self.flexattrs.has_key(namespace): + self.flexattrs[namespace] = {} + self.flexattrs[flexns[i]][key] = flexvalues[i] + def _clear_dirty(self): self.dirty = {} for key in ITEM_KEYS: @@ -266,7 +310,6 @@ class Item(object): def __repr__(self): return 'Item(' + repr(self.record) + ')' - # Item field accessors. def __getattr__(self, key): @@ -293,7 +336,7 @@ class Item(object): elif isinstance(value, buffer): value = str(value) - if key in ITEM_KEYS or key in plugins.item_fields(): + if key in ITEM_KEYS: # If the value changed, mark the field as dirty. if (key not in self.record) or (self.record[key] != value): self.record[key] = value @@ -303,6 +346,15 @@ class Item(object): else: super(Item, self).__setattr__(key, value) + def __getitem__(self, key): + """Entry point to flexible attributes. + `key` should be a plugin name or an otherwise + valid flexible attr namespace. + """ + if key in self.flexattrs: + return self.flexattrs[key] + else: + raise KeyError('{} is not a valid attribute namespace'.format(key)) # Interaction with file metadata. @@ -453,7 +505,7 @@ class Query(object): to substitute in for ?s in the query. """ clause, subvals = self.clause() - return ('SELECT ' + columns + ' FROM items WHERE ' + clause, subvals) + return ('SELECT ' + columns + ' FROM unified_items WHERE ' + clause, subvals) def count(self, tx): """Returns `(num, length)` where `num` is the number of items in @@ -461,7 +513,7 @@ class Query(object): length in seconds. """ clause, subvals = self.clause() - statement = 'SELECT COUNT(id), SUM(length) FROM items WHERE ' + clause + statement = 'SELECT COUNT(id), SUM(length) FROM unified_items WHERE ' + clause result = tx.query(statement, subvals)[0] return (result[0], result[1] or 0.0) @@ -1122,11 +1174,22 @@ class Library(BaseLibrary): CREATE TABLE IF NOT EXISTS {0}_attributes ( id INTEGER PRIMARY KEY, entity_id INTEGER, + namespace TEXT, key TEXT, value TEXT, - UNIQUE(entity_id, key) ON CONFLICT REPLACE); + UNIQUE(entity_id, key, namespace) ON CONFLICT REPLACE); CREATE INDEX IF NOT EXISTS {0}_id_attribute ON {0}_attributes (entity_id); + CREATE VIEW IF NOT EXISTS unified_{0}s AS + SELECT + GROUP_CONCAT(ia.key) AS flexkeys, + GROUP_CONCAT(ia.value) AS flexvalues, + GROUP_CONCAT(ia.namespace) AS flexns, + {0}s.* + FROM {0}s + LEFT JOIN {0}_attributes AS ia + ON {0}s.id == ia.entity_id + GROUP BY {0}s.id; """.format(entity)) def _connection(self): @@ -1280,7 +1343,7 @@ class Library(BaseLibrary): load_id = item.id with self.transaction() as tx: - rows = tx.query('SELECT * FROM items WHERE id=?', (load_id,)) + rows = tx.query('SELECT * FROM unified_items WHERE id=?', (load_id,)) item._fill_record(rows[0]) item._clear_dirty() @@ -1309,15 +1372,18 @@ class Library(BaseLibrary): subvars.append(store_id) tx.mutate(query, subvars) - # Flexible attributes. - for key in plugins.item_fields(): - if item.dirty.get(key) or store_all: - tx.mutate( - 'INSERT INTO item_attributes (entity_id, key, value)' - ' VALUES (?, ?, ?)', - (store_id, key, getattr(item, key)) - ) - + #flexible attributes + flexins = '''INSERT INTO item_attributes + (entity_id, key, value, namespace) + VALUES (?,?,?);''' + flexup = '''UPDATE item_attributes SET value = ? WHERE + entity_id = ? AND key = ? AND namespace = ?;''' + for ns,attrs in item.flexattrs.iteritems(): + for key in attrs.dirty: + #TODO: use an upsert method to store attrs + tx.mutate(flexins, (store_id, key, attrs[key], ns)) + attrs.dirty.clear() + item._clear_dirty() self._memotable = {} @@ -1415,7 +1481,7 @@ class Library(BaseLibrary): super_query = AndQuery(queries) where, subvals = super_query.clause() - sql = "SELECT * FROM items " + \ + sql = "SELECT * FROM unified_items " + \ "WHERE " + where + \ " ORDER BY %s, album, disc, track" % \ _orelse("artist_sort", "artist") @@ -1431,7 +1497,7 @@ class Library(BaseLibrary): """Fetch an Item by its ID. Returns None if no match is found. """ with self.transaction() as tx: - rows = tx.query("SELECT * FROM items WHERE id=?", (id,)) + rows = tx.query("SELECT * FROM unified_items WHERE id=?", (id,)) it = ResultIterator(rows) try: return it.next() @@ -1552,7 +1618,7 @@ class Album(BaseAlbum): """ with self._library.transaction() as tx: rows = tx.query( - 'SELECT * FROM items WHERE album_id=?', + 'SELECT * FROM unified_items WHERE album_id=?', (self.id,) ) return ResultIterator(rows) diff --git a/beets/plugins.py b/beets/plugins.py index 177fce032..c8875ec34 100755 --- a/beets/plugins.py +++ b/beets/plugins.py @@ -48,8 +48,6 @@ class BeetsPlugin(object): self.template_funcs = {} if not self.template_fields: self.template_fields = {} - self.item_fields = [] - self.album_fields = [] def commands(self): """Should return a list of beets.ui.Subcommand objects for @@ -95,6 +93,18 @@ class BeetsPlugin(object): """ return {} + def item_attributes(self): + """Returns a list of registered flexible attribute fields + for `Item` entities. + """ + return [] + + def album_attributes(self): + """Returns a list of registered flexible attribute fields + for `Album` entities. + """ + return [] + listeners = None @classmethod @@ -291,24 +301,23 @@ def import_stages(): return stages def item_fields(): - """Get a list of strings indicating registered flexible Item - attributes. + """Get a dict of string lists indicating registered + flexible Item attributes in the form {'pluginname':[fields...]}. """ - fields = [] + fields = {} for plugin in find_plugins(): - fields += plugin.item_fields + fields[plugin.name] = plugin.item_attributes() return fields def album_fields(): - """Get a list of strings indicating registered flexible Album - attributes. + """Get a dict of string lists indicating registered + flexible Album attributes in the form {'pluginname':[fields...]}. """ - fields = [] + fields = {} for plugin in find_plugins(): - fields += plugin.album_fields + fields[plugin.name] = plugin.album_attributes() return fields - # Event dispatch. def event_handlers(): From fbd85ef6baa0542bf9dc91c535e621e5c9397ee7 Mon Sep 17 00:00:00 2001 From: steini Date: Tue, 5 Mar 2013 05:20:03 +0000 Subject: [PATCH 03/32] Started hacking `CollectionQuery` to search flexible attribute fields. --- beets/library.py | 52 ++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 46 insertions(+), 6 deletions(-) diff --git a/beets/library.py b/beets/library.py index 8a2ea92d4..41cde2c37 100644 --- a/beets/library.py +++ b/beets/library.py @@ -521,16 +521,23 @@ class FieldQuery(Query): """An abstract query that searches in a specific field for a pattern. """ - def __init__(self, field, pattern): + def __init__(self, field, pattern, namespace=None, entity='item'): self.field = field self.pattern = pattern + self.namespace = namespace + self.entity = entity class MatchQuery(FieldQuery): """A query that looks for exact matches in an item field.""" def clause(self): + pattern = self.pattern if self.field == 'path' and isinstance(pattern, str): pattern = buffer(pattern) + if self.namespace: + #give a flexible attribute clause + c = 'key = ? AND value = ? AND namespace = ?' + return c, [self.field, pattern, self.namespace] return self.field + " = ?", [pattern] def match(self, item): @@ -551,7 +558,7 @@ class SubstringQuery(FieldQuery): class RegexpQuery(FieldQuery): """A query that matches a regular expression in a specific item field.""" - def __init__(self, field, pattern): + def __init__(self, field, pattern, namespace=None, entity='item'): super(RegexpQuery, self).__init__(field, pattern) self.regexp = re.compile(pattern) @@ -605,14 +612,47 @@ class CollectionQuery(Query): """Returns a clause created by joining together the clauses of all subqueries with the string joiner (padded by spaces). """ + entity = None + flexclause_parts = [] + flexsubvals = [] clause_parts = [] subvals = [] for subq in self.subqueries: - subq_clause, subq_subvals = subq.clause() - clause_parts.append('(' + subq_clause + ')') - subvals += subq_subvals + if subq.namespace: + #it's a flex attr query, initiate -ugly hack- mode + entity = subq.entity + clauses = flexclause_parts + subs = flexsubvals + subq_clause, subq_subvals = subq.clause() + if joiner.lower() == 'and': + #flexible attrs need this nested mess for AND queries + subq_clause = ''' + EXISTS (SELECT 1 FROM {0}_attributes AS ia + WHERE {1} AND ia.entity_id = unified_{0}s.id) + '''.format(entity, '('+subq_clause+')') + else: + clauses = clause_parts + subs = subvals + subq_clause, subq_subvals = subq.clause() + clauses.append('(' + subq_clause + ')') + subs += subq_subvals + clause = (' ' + joiner + ' ').join(clause_parts) - return clause, subvals + + if not flexclause_parts: + return clause, subvals + + fclause = (' ' + joiner + ' ').join(flexclause_parts) + if joiner.lower() == 'and': + pass + else: + flexorclause = ''' + id IN ( + SELECT entity_id FROM {0}_attributes + WHERE {1}) + ''' + fclause = flexorclause.format(entity, fclause) + return clause+' '+joiner+' '+fclause, subvals+flexsubvals # Regular expression for _parse_query_part, below. _pq_regex = re.compile( From 8e342a16a14f3cb61c86e91c81425410b5e1ce3b Mon Sep 17 00:00:00 2001 From: steini Date: Wed, 6 Mar 2013 14:38:17 +0000 Subject: [PATCH 04/32] Fixed up a couple of query classes for flex attr support. --- beets/library.py | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/beets/library.py b/beets/library.py index 41cde2c37..42aa4639a 100644 --- a/beets/library.py +++ b/beets/library.py @@ -548,7 +548,12 @@ class SubstringQuery(FieldQuery): def clause(self): search = '%' + (self.pattern.replace('\\','\\\\').replace('%','\\%') .replace('_','\\_')) + '%' - clause = self.field + " like ? escape '\\'" + if self.namespace: + clause = """key = {0} AND namespace = {1} + AND value like ? escape '\\'""".format( + self.field, self.namespace) + else: + clause = self.field + " like ? escape '\\'" subvals = [search] return clause, subvals @@ -559,11 +564,15 @@ class SubstringQuery(FieldQuery): class RegexpQuery(FieldQuery): """A query that matches a regular expression in a specific item field.""" def __init__(self, field, pattern, namespace=None, entity='item'): - super(RegexpQuery, self).__init__(field, pattern) + super(RegexpQuery, self).__init__(field, pattern, namespace, entity) self.regexp = re.compile(pattern) def clause(self): - clause = self.field + " REGEXP ?" + if self.namespace: + clause = 'key = {0} AND namespace = {1} AND value REGEXP ?'.format( + self.field, self.namespace) + else: + clause = self.field + " REGEXP ?" subvals = [self.pattern] return clause, subvals @@ -1416,11 +1425,8 @@ class Library(BaseLibrary): flexins = '''INSERT INTO item_attributes (entity_id, key, value, namespace) VALUES (?,?,?);''' - flexup = '''UPDATE item_attributes SET value = ? WHERE - entity_id = ? AND key = ? AND namespace = ?;''' for ns,attrs in item.flexattrs.iteritems(): for key in attrs.dirty: - #TODO: use an upsert method to store attrs tx.mutate(flexins, (store_id, key, attrs[key], ns)) attrs.dirty.clear() From 7d1f67881ebaf22b73dd3d17eb4b2a19b668627b Mon Sep 17 00:00:00 2001 From: steini Date: Wed, 6 Mar 2013 18:29:22 +0000 Subject: [PATCH 05/32] ironing out some kinks --- beets/library.py | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/beets/library.py b/beets/library.py index 42aa4639a..dc4925a7f 100644 --- a/beets/library.py +++ b/beets/library.py @@ -288,10 +288,13 @@ class Item(object): self.flexattrs[key] = FixedDict(flexattrns[key]) #treat flexattrs values as they would come from the database - if values['flexkeys'] and values['flexvalues'] and values['flexns']: + k = values.keys() + if 'flexkeys' in k and 'flexvalues' in k and 'flexns' in k: flexkeys = values['flexkeys'].split(',') flexvalues = values['flexvalues'].split(',') flexns = values['flexns'].split(',') + log.debug('flexkeys: %s - flexvals: %s - flexns: %s', + flexkeys, flexvalues, flexns) for i, key in enumerate(flexkeys): namespace = flexns[i] if namespace not in self.flexattrs: @@ -300,7 +303,7 @@ class Item(object): value = flexvalues[i] if not self.flexattrs.has_key(namespace): self.flexattrs[namespace] = {} - self.flexattrs[flexns[i]][key] = flexvalues[i] + self.flexattrs[flexns[i]][key] = flexvalues[i] def _clear_dirty(self): self.dirty = {} @@ -627,7 +630,7 @@ class CollectionQuery(Query): clause_parts = [] subvals = [] for subq in self.subqueries: - if subq.namespace: + if hasattr(subq, 'namespace') and subq.namespace: #it's a flex attr query, initiate -ugly hack- mode entity = subq.entity clauses = flexclause_parts @@ -661,7 +664,13 @@ class CollectionQuery(Query): WHERE {1}) ''' fclause = flexorclause.format(entity, fclause) - return clause+' '+joiner+' '+fclause, subvals+flexsubvals + if clause and fclause: + clause = clause+' '+joiner+' '+fclause + elif clause: + clause = clause + elif fclause: + clause = fclause + return clause, subvals+flexsubvals # Regular expression for _parse_query_part, below. _pq_regex = re.compile( @@ -1424,7 +1433,7 @@ class Library(BaseLibrary): #flexible attributes flexins = '''INSERT INTO item_attributes (entity_id, key, value, namespace) - VALUES (?,?,?);''' + VALUES (?,?,?,?);''' for ns,attrs in item.flexattrs.iteritems(): for key in attrs.dirty: tx.mutate(flexins, (store_id, key, attrs[key], ns)) @@ -1508,7 +1517,7 @@ class Library(BaseLibrary): # "Add" the artist to the query. query = AndQuery((query, MatchQuery('albumartist', artist))) where, subvals = query.clause() - sql = "SELECT * FROM albums " + \ + sql = "SELECT * FROM unified_albums " + \ "WHERE " + where + \ " ORDER BY %s, album" % \ _orelse("albumartist_sort", "albumartist") From 410bdf6dddaa9cddc869baaad96796c5e0700d80 Mon Sep 17 00:00:00 2001 From: steini Date: Wed, 6 Mar 2013 23:25:31 +0000 Subject: [PATCH 06/32] Code for reading attributes from concatenated flex columns into dicts moved to an outside methods to make it usable for both BaseAlbum and Item objects. --- beets/library.py | 67 +++++++++++++++++++++++++++++++----------------- 1 file changed, 44 insertions(+), 23 deletions(-) diff --git a/beets/library.py b/beets/library.py index dc4925a7f..d52820383 100644 --- a/beets/library.py +++ b/beets/library.py @@ -252,6 +252,38 @@ class FixedDict(dict): super(FixedDict, self).__setitem__(key, value) self.dirty[key] = True + +def set_flexattrs(obj, values): + """Create the initial flexible attribute dict for `obj` (e.g. Item/Album) + from given `values` which should be a dict compatible row of values + from the database. + + This sets the `flexattrs` attribute on `obj`. + """ + #make the initial, empty flexible attribute dict + flexattrns = plugins.item_fields() + flexattrs = FixedDict(flexattrns.keys()) + for key in flexattrs.iterkeys(): + flexattrs[key] = FixedDict(flexattrns[key]) + + k = values.keys() + if 'flexkeys' in k and 'flexvalues' in k and 'flexns' in k \ + and values['flexkeys']: + flexkeys = values['flexkeys'].split(',') + flexvalues = values['flexvalues'].split(',') + flexns = values['flexns'].split(',') + log.debug('flexkeys: %s - flexvals: %s - flexns: %s', + flexkeys, flexvalues, flexns) + for i, key in enumerate(flexkeys): + namespace = flexns[i] + if namespace not in flexattrs: + #not an active plugin, skip + continue + if not flexattrs.has_key(namespace): + flexattrs[namespace] = {} + flexattrs[flexns[i]][key] = flexvalues[i] + object.__setattr__(obj, 'flexattrs', flexattrs) + # Library items (songs). class Item(object): @@ -281,29 +313,7 @@ class Item(object): except KeyError: setattr(self, key, None) - #make the initial, empty flexible attribute dict - flexattrns = plugins.item_fields() - self.flexattrs = FixedDict(flexattrns.keys()) - for key in self.flexattrs.iterkeys(): - self.flexattrs[key] = FixedDict(flexattrns[key]) - - #treat flexattrs values as they would come from the database - k = values.keys() - if 'flexkeys' in k and 'flexvalues' in k and 'flexns' in k: - flexkeys = values['flexkeys'].split(',') - flexvalues = values['flexvalues'].split(',') - flexns = values['flexns'].split(',') - log.debug('flexkeys: %s - flexvals: %s - flexns: %s', - flexkeys, flexvalues, flexns) - for i, key in enumerate(flexkeys): - namespace = flexns[i] - if namespace not in self.flexattrs: - #not an active plugin, skip - continue - value = flexvalues[i] - if not self.flexattrs.has_key(namespace): - self.flexattrs[namespace] = {} - self.flexattrs[flexns[i]][key] = flexvalues[i] + set_flexattrs(self, values) def _clear_dirty(self): self.dirty = {} @@ -1048,6 +1058,7 @@ class BaseAlbum(object): def __init__(self, library, record): super(BaseAlbum, self).__setattr__('_library', library) super(BaseAlbum, self).__setattr__('_record', record) + set_flexattrs(self, record) def __getattr__(self, key): """Get the value for an album attribute.""" @@ -1073,6 +1084,16 @@ class BaseAlbum(object): else: super(BaseAlbum, self).__setattr__(key, value) + def __getitem__(self, key): + """Entry point to flexible attributes. + `key` should be a plugin name or an otherwise + valid flexible attr namespace. + """ + if key in self.flexattrs: + return self.flexattrs[key] + else: + raise KeyError('{} is not a valid attribute namespace'.format(key)) + def load(self): """Refresh this album's cached metadata from the library. """ From 4fe91b342b8c2392d9bd63ea4dc46852d7f6ed3d Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Sat, 9 Mar 2013 12:54:55 -0800 Subject: [PATCH 07/32] experiments with "un-registered" flexattrs --- beets/library.py | 258 +++++++++++++---------------------------------- beets/plugins.py | 31 +----- 2 files changed, 70 insertions(+), 219 deletions(-) diff --git a/beets/library.py b/beets/library.py index d52820383..6359ea425 100644 --- a/beets/library.py +++ b/beets/library.py @@ -223,73 +223,28 @@ def format_for_path(value, key=None, pathmod=None): return value +def get_flexattrs(rows): + """Given a list of SQLite result rows, construct a dictionary + hierarchy reflecting the contained flexible attribute values. + """ + out = defaultdict(dict) + for row in rows: + out[row['namespace']][row['key']] = row['value'] + return dict(out) + # Exceptions. class InvalidFieldError(Exception): pass -class FixedDict(dict): - """A dict where keys can not be added after creation and keys are marked - as 'dirty' when their values are changed. - Accepts a list of `keys` and an optional `values` which should - be a dict compatible object. - Any key not in `values` will be implicitly added and given a `None` value. - """ - def __init__(self, keys, values=None): - values = values or {} - super(FixedDict, self).__init__(values) - seti = super(FixedDict, self).__setitem__ - for key in keys: - if not key in self: - seti(key, None) - self.dirty = {} - - def __setitem__(self, key, value): - if key not in self: - raise KeyError('{} is not a valid key.'.format(key)) - elif value != self[key]: - super(FixedDict, self).__setitem__(key, value) - self.dirty[key] = True - - -def set_flexattrs(obj, values): - """Create the initial flexible attribute dict for `obj` (e.g. Item/Album) - from given `values` which should be a dict compatible row of values - from the database. - - This sets the `flexattrs` attribute on `obj`. - """ - #make the initial, empty flexible attribute dict - flexattrns = plugins.item_fields() - flexattrs = FixedDict(flexattrns.keys()) - for key in flexattrs.iterkeys(): - flexattrs[key] = FixedDict(flexattrns[key]) - - k = values.keys() - if 'flexkeys' in k and 'flexvalues' in k and 'flexns' in k \ - and values['flexkeys']: - flexkeys = values['flexkeys'].split(',') - flexvalues = values['flexvalues'].split(',') - flexns = values['flexns'].split(',') - log.debug('flexkeys: %s - flexvals: %s - flexns: %s', - flexkeys, flexvalues, flexns) - for i, key in enumerate(flexkeys): - namespace = flexns[i] - if namespace not in flexattrs: - #not an active plugin, skip - continue - if not flexattrs.has_key(namespace): - flexattrs[namespace] = {} - flexattrs[flexns[i]][key] = flexvalues[i] - object.__setattr__(obj, 'flexattrs', flexattrs) # Library items (songs). class Item(object): - def __init__(self, values): + def __init__(self, values, flexattrs=None): self.dirty = {} - self._fill_record(values) + self._fill_record(values, flexattrs) self._clear_dirty() @classmethod @@ -304,16 +259,14 @@ class Item(object): i.mtime = i.current_mtime() # Initial mtime. return i - def _fill_record(self, values): + def _fill_record(self, values, flexattrs=None): self.record = {} - for key in ITEM_KEYS: try: setattr(self, key, values[key]) except KeyError: setattr(self, key, None) - - set_flexattrs(self, values) + self.flexattrs = flexattrs or {} def _clear_dirty(self): self.dirty = {} @@ -323,13 +276,14 @@ class Item(object): def __repr__(self): return 'Item(' + repr(self.record) + ')' + # Item field accessors. def __getattr__(self, key): """If key is an item attribute (i.e., a column in the database), returns the record entry for that key. """ - if key in ITEM_KEYS or key in plugins.item_fields(): + if key in ITEM_KEYS: return self.record.get(key) else: raise AttributeError(key + ' is not a valid item field') @@ -359,15 +313,6 @@ class Item(object): else: super(Item, self).__setattr__(key, value) - def __getitem__(self, key): - """Entry point to flexible attributes. - `key` should be a plugin name or an otherwise - valid flexible attr namespace. - """ - if key in self.flexattrs: - return self.flexattrs[key] - else: - raise KeyError('{} is not a valid attribute namespace'.format(key)) # Interaction with file metadata. @@ -512,13 +457,13 @@ class Query(object): """ raise NotImplementedError - def statement(self, columns='*'): + def statement(self): """Returns (query, subvals) where clause is a sqlite SELECT statement to enact this query and subvals is a list of values to substitute in for ?s in the query. """ clause, subvals = self.clause() - return ('SELECT ' + columns + ' FROM unified_items WHERE ' + clause, subvals) + return ('SELECT * FROM items WHERE ' + clause, subvals) def count(self, tx): """Returns `(num, length)` where `num` is the number of items in @@ -526,7 +471,7 @@ class Query(object): length in seconds. """ clause, subvals = self.clause() - statement = 'SELECT COUNT(id), SUM(length) FROM unified_items WHERE ' + clause + statement = 'SELECT COUNT(id), SUM(length) FROM items WHERE ' + clause result = tx.query(statement, subvals)[0] return (result[0], result[1] or 0.0) @@ -534,23 +479,16 @@ class FieldQuery(Query): """An abstract query that searches in a specific field for a pattern. """ - def __init__(self, field, pattern, namespace=None, entity='item'): + def __init__(self, field, pattern): self.field = field self.pattern = pattern - self.namespace = namespace - self.entity = entity class MatchQuery(FieldQuery): """A query that looks for exact matches in an item field.""" def clause(self): - pattern = self.pattern if self.field == 'path' and isinstance(pattern, str): pattern = buffer(pattern) - if self.namespace: - #give a flexible attribute clause - c = 'key = ? AND value = ? AND namespace = ?' - return c, [self.field, pattern, self.namespace] return self.field + " = ?", [pattern] def match(self, item): @@ -561,12 +499,7 @@ class SubstringQuery(FieldQuery): def clause(self): search = '%' + (self.pattern.replace('\\','\\\\').replace('%','\\%') .replace('_','\\_')) + '%' - if self.namespace: - clause = """key = {0} AND namespace = {1} - AND value like ? escape '\\'""".format( - self.field, self.namespace) - else: - clause = self.field + " like ? escape '\\'" + clause = self.field + " like ? escape '\\'" subvals = [search] return clause, subvals @@ -576,16 +509,12 @@ class SubstringQuery(FieldQuery): class RegexpQuery(FieldQuery): """A query that matches a regular expression in a specific item field.""" - def __init__(self, field, pattern, namespace=None, entity='item'): - super(RegexpQuery, self).__init__(field, pattern, namespace, entity) + def __init__(self, field, pattern): + super(RegexpQuery, self).__init__(field, pattern) self.regexp = re.compile(pattern) def clause(self): - if self.namespace: - clause = 'key = {0} AND namespace = {1} AND value REGEXP ?'.format( - self.field, self.namespace) - else: - clause = self.field + " REGEXP ?" + clause = self.field + " REGEXP ?" subvals = [self.pattern] return clause, subvals @@ -634,53 +563,14 @@ class CollectionQuery(Query): """Returns a clause created by joining together the clauses of all subqueries with the string joiner (padded by spaces). """ - entity = None - flexclause_parts = [] - flexsubvals = [] clause_parts = [] subvals = [] for subq in self.subqueries: - if hasattr(subq, 'namespace') and subq.namespace: - #it's a flex attr query, initiate -ugly hack- mode - entity = subq.entity - clauses = flexclause_parts - subs = flexsubvals - subq_clause, subq_subvals = subq.clause() - if joiner.lower() == 'and': - #flexible attrs need this nested mess for AND queries - subq_clause = ''' - EXISTS (SELECT 1 FROM {0}_attributes AS ia - WHERE {1} AND ia.entity_id = unified_{0}s.id) - '''.format(entity, '('+subq_clause+')') - else: - clauses = clause_parts - subs = subvals - subq_clause, subq_subvals = subq.clause() - clauses.append('(' + subq_clause + ')') - subs += subq_subvals - + subq_clause, subq_subvals = subq.clause() + clause_parts.append('(' + subq_clause + ')') + subvals += subq_subvals clause = (' ' + joiner + ' ').join(clause_parts) - - if not flexclause_parts: - return clause, subvals - - fclause = (' ' + joiner + ' ').join(flexclause_parts) - if joiner.lower() == 'and': - pass - else: - flexorclause = ''' - id IN ( - SELECT entity_id FROM {0}_attributes - WHERE {1}) - ''' - fclause = flexorclause.format(entity, fclause) - if clause and fclause: - clause = clause+' '+joiner+' '+fclause - elif clause: - clause = clause - elif fclause: - clause = fclause - return clause, subvals+flexsubvals + return clause, subvals # Regular expression for _parse_query_part, below. _pq_regex = re.compile( @@ -905,16 +795,22 @@ class ResultIterator(object): """An iterator into an item query result set. The iterator lazily constructs Item objects that reflect database rows. """ - def __init__(self, rows): + def __init__(self, rows, lib): self.rows = rows self.rowiter = iter(self.rows) + self.lib = lib def __iter__(self): return self def next(self): row = self.rowiter.next() # May raise StopIteration. - return Item(row) + with self.lib.transaction() as tx: + flex_rows = tx.query( + 'SELECT * FROM item_attributes WHERE entity_id=?', + (row['id'],) + ) + return Item(row, get_flexattrs(flex_rows)) # An abstract library. @@ -1058,7 +954,6 @@ class BaseAlbum(object): def __init__(self, library, record): super(BaseAlbum, self).__setattr__('_library', library) super(BaseAlbum, self).__setattr__('_record', record) - set_flexattrs(self, record) def __getattr__(self, key): """Get the value for an album attribute.""" @@ -1084,16 +979,6 @@ class BaseAlbum(object): else: super(BaseAlbum, self).__setattr__(key, value) - def __getitem__(self, key): - """Entry point to flexible attributes. - `key` should be a plugin name or an otherwise - valid flexible attr namespace. - """ - if key in self.flexattrs: - return self.flexattrs[key] - else: - raise KeyError('{} is not a valid attribute namespace'.format(key)) - def load(self): """Refresh this album's cached metadata from the library. """ @@ -1256,19 +1141,9 @@ class Library(BaseLibrary): namespace TEXT, key TEXT, value TEXT, - UNIQUE(entity_id, key, namespace) ON CONFLICT REPLACE); + UNIQUE(entity_id, namespace, key) ON CONFLICT REPLACE); CREATE INDEX IF NOT EXISTS {0}_id_attribute ON {0}_attributes (entity_id); - CREATE VIEW IF NOT EXISTS unified_{0}s AS - SELECT - GROUP_CONCAT(ia.key) AS flexkeys, - GROUP_CONCAT(ia.value) AS flexvalues, - GROUP_CONCAT(ia.namespace) AS flexns, - {0}s.* - FROM {0}s - LEFT JOIN {0}_attributes AS ia - ON {0}s.id == ia.entity_id - GROUP BY {0}s.id; """.format(entity)) def _connection(self): @@ -1403,14 +1278,13 @@ class Library(BaseLibrary): ) # Flexible attributes. - for key in plugins.item_fields(): - value = getattr(item, key) - if value is not None: - tx.mutate( - 'INSERT INTO item_attributes (entity_id, key, value)' - ' VALUES (?, ?, ?)', - (new_id, key, value) - ) + flexins = 'INSERT INTO item_attributes ' \ + ' (entity_id, namespace, key, value)' \ + ' VALUES (?, ?, ?, ?)' + for namespace, attrs in item.flexattrs.items(): + for key, value in attrs.items(): + if value is not None: + tx.mutate(flexins, (new_id, key, value)) item._clear_dirty() item.id = new_id @@ -1422,8 +1296,11 @@ class Library(BaseLibrary): load_id = item.id with self.transaction() as tx: - rows = tx.query('SELECT * FROM unified_items WHERE id=?', (load_id,)) - item._fill_record(rows[0]) + rows = tx.query('SELECT * FROM items WHERE id=?', (load_id,)) + flex_rows = tx.query( + 'SELECT * FROM item_attributes WHERE entity_id=?', (load_id,) + ) + item._fill_record(rows[0], get_flexattrs(flex_rows)) item._clear_dirty() def store(self, item, store_id=None, store_all=False): @@ -1451,15 +1328,14 @@ class Library(BaseLibrary): subvars.append(store_id) tx.mutate(query, subvars) - #flexible attributes - flexins = '''INSERT INTO item_attributes - (entity_id, key, value, namespace) - VALUES (?,?,?,?);''' - for ns,attrs in item.flexattrs.iteritems(): - for key in attrs.dirty: - tx.mutate(flexins, (store_id, key, attrs[key], ns)) - attrs.dirty.clear() - + # Flexible attributes. + flexins = 'INSERT INTO item_attributes ' \ + ' (entity_id, namespace, key, value)' \ + ' VALUES (?, ?, ?, ?)' + for namespace, attrs in item.flexattrs.items(): + for key, value in attrs.items(): + tx.mutate(flexins, (store_id, namespace, key, value)) + item._clear_dirty() self._memotable = {} @@ -1538,7 +1414,7 @@ class Library(BaseLibrary): # "Add" the artist to the query. query = AndQuery((query, MatchQuery('albumartist', artist))) where, subvals = query.clause() - sql = "SELECT * FROM unified_albums " + \ + sql = "SELECT * FROM albums " + \ "WHERE " + where + \ " ORDER BY %s, album" % \ _orelse("albumartist_sort", "albumartist") @@ -1557,14 +1433,14 @@ class Library(BaseLibrary): super_query = AndQuery(queries) where, subvals = super_query.clause() - sql = "SELECT * FROM unified_items " + \ + sql = "SELECT * FROM items " + \ "WHERE " + where + \ " ORDER BY %s, album, disc, track" % \ _orelse("artist_sort", "artist") log.debug('Getting items with SQL: %s' % sql) with self.transaction() as tx: rows = tx.query(sql, subvals) - return ResultIterator(rows) + return ResultIterator(rows, self) # Convenience accessors. @@ -1573,11 +1449,13 @@ class Library(BaseLibrary): """Fetch an Item by its ID. Returns None if no match is found. """ with self.transaction() as tx: - rows = tx.query("SELECT * FROM unified_items WHERE id=?", (id,)) - it = ResultIterator(rows) - try: - return it.next() - except StopIteration: + rows = tx.query("SELECT * FROM items WHERE id=?", (id,)) + flex_rows = tx.query( + 'SELECT * FROM item_attributes WHERE entity_id=?', (id,) + ) + if rows: + return Item(rows, get_flexattrs(flex_rows)) + else: return None def get_album(self, item_or_id): @@ -1694,10 +1572,10 @@ class Album(BaseAlbum): """ with self._library.transaction() as tx: rows = tx.query( - 'SELECT * FROM unified_items WHERE album_id=?', + 'SELECT * FROM items WHERE album_id=?', (self.id,) ) - return ResultIterator(rows) + return ResultIterator(rows, self._library) def remove(self, delete=False, with_items=True): """Removes this album and all its associated items from the diff --git a/beets/plugins.py b/beets/plugins.py index c8875ec34..97f91bcfa 100755 --- a/beets/plugins.py +++ b/beets/plugins.py @@ -48,6 +48,8 @@ class BeetsPlugin(object): self.template_funcs = {} if not self.template_fields: self.template_fields = {} + self.item_fields = [] + self.album_fields = [] def commands(self): """Should return a list of beets.ui.Subcommand objects for @@ -93,18 +95,6 @@ class BeetsPlugin(object): """ return {} - def item_attributes(self): - """Returns a list of registered flexible attribute fields - for `Item` entities. - """ - return [] - - def album_attributes(self): - """Returns a list of registered flexible attribute fields - for `Album` entities. - """ - return [] - listeners = None @classmethod @@ -300,23 +290,6 @@ def import_stages(): stages += plugin.import_stages return stages -def item_fields(): - """Get a dict of string lists indicating registered - flexible Item attributes in the form {'pluginname':[fields...]}. - """ - fields = {} - for plugin in find_plugins(): - fields[plugin.name] = plugin.item_attributes() - return fields - -def album_fields(): - """Get a dict of string lists indicating registered - flexible Album attributes in the form {'pluginname':[fields...]}. - """ - fields = {} - for plugin in find_plugins(): - fields[plugin.name] = plugin.album_attributes() - return fields # Event dispatch. From 3e05d6614c0bae56fc6e05159c9b6ab8437027ed Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Sat, 9 Mar 2013 13:16:24 -0800 Subject: [PATCH 08/32] get and set flexattrs with hyphen keys Here's another little experiment: to make flexattrs a little easier to use for end users, you can now get and set them by using 'namespace-key' as the argument to __getattr__ or __setattr__. For example, try: $ beet mod foo-bar=baz $ beet ls -f '${foo-bar}' baz baz baz ... --- beets/library.py | 22 +++++++++++++++++++++- beets/ui/commands.py | 2 +- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/beets/library.py b/beets/library.py index 6359ea425..441554494 100644 --- a/beets/library.py +++ b/beets/library.py @@ -266,7 +266,10 @@ class Item(object): setattr(self, key, values[key]) except KeyError: setattr(self, key, None) - self.flexattrs = flexattrs or {} + + self.flexattrs = defaultdict(dict) + if flexattrs: + self.flexattrs.update(flexattrs) def _clear_dirty(self): self.dirty = {} @@ -285,6 +288,11 @@ class Item(object): """ if key in ITEM_KEYS: return self.record.get(key) + elif '-' in key: + namespace, key = key.split('-', 1) + if namespace in self.flexattrs: + return self.flexattrs[namespace].get(key) + return None else: raise AttributeError(key + ' is not a valid item field') @@ -310,6 +318,11 @@ class Item(object): self.dirty[key] = True if key in ITEM_KEYS_WRITABLE: self.mtime = 0 # Reset mtime on dirty. + + elif '-' in key: + namespace, key = key.split('-', 1) + self.flexattrs[namespace][key] = value + else: super(Item, self).__setattr__(key, value) @@ -425,6 +438,13 @@ class Item(object): if not mapping['albumartist']: mapping['albumartist'] = mapping['artist'] + # Flexible attributes. + for namespace, attrs in self.flexattrs.items(): + for key, value in attrs.items(): + if sanitize: + value = format_for_path(value, None, pathmod) + mapping['{}-{}'.format(namespace, key)] = value + # Get values from plugins. for key, value in plugins.template_values(self).iteritems(): if sanitize: diff --git a/beets/ui/commands.py b/beets/ui/commands.py index b50341532..a257d5013 100644 --- a/beets/ui/commands.py +++ b/beets/ui/commands.py @@ -980,7 +980,7 @@ def modify_items(lib, mods, query, write, move, album, confirm): fsets = {} for mod in mods: key, value = mod.split('=', 1) - if key not in allowed_keys: + if key not in allowed_keys and '-' not in key: raise ui.UserError('"%s" is not a valid field' % key) fsets[key] = value From 4c85dc8f7269c3e399bea4ae7b2bb9917a963e53 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Sat, 16 Mar 2013 12:50:24 -0700 Subject: [PATCH 09/32] reduce redundancy in item queries Now the SELECT query for items only appears in Library.items(). All other functions that retrieve items go through this method. This should make it easier to change the way flexattrs are picked up. --- beets/library.py | 28 +++++++--------------------- 1 file changed, 7 insertions(+), 21 deletions(-) diff --git a/beets/library.py b/beets/library.py index 441554494..2eb8c6bf1 100644 --- a/beets/library.py +++ b/beets/library.py @@ -1314,13 +1314,8 @@ class Library(BaseLibrary): def load(self, item, load_id=None): if load_id is None: load_id = item.id - - with self.transaction() as tx: - rows = tx.query('SELECT * FROM items WHERE id=?', (load_id,)) - flex_rows = tx.query( - 'SELECT * FROM item_attributes WHERE entity_id=?', (load_id,) - ) - item._fill_record(rows[0], get_flexattrs(flex_rows)) + stored_item = self.get_item(load_id) + item._fill_record(stored_item.record, stored_item.flexattrs) item._clear_dirty() def store(self, item, store_id=None, store_all=False): @@ -1468,14 +1463,10 @@ class Library(BaseLibrary): def get_item(self, id): """Fetch an Item by its ID. Returns None if no match is found. """ - with self.transaction() as tx: - rows = tx.query("SELECT * FROM items WHERE id=?", (id,)) - flex_rows = tx.query( - 'SELECT * FROM item_attributes WHERE entity_id=?', (id,) - ) - if rows: - return Item(rows, get_flexattrs(flex_rows)) - else: + items = self.items(MatchQuery('id', id)) + try: + return items.next() + except StopIteration: return None def get_album(self, item_or_id): @@ -1590,12 +1581,7 @@ class Album(BaseAlbum): """Returns an iterable over the items associated with this album. """ - with self._library.transaction() as tx: - rows = tx.query( - 'SELECT * FROM items WHERE album_id=?', - (self.id,) - ) - return ResultIterator(rows, self._library) + return self._library.items(MatchQuery('album_id', self.id)) def remove(self, delete=False, with_items=True): """Removes this album and all its associated items from the From 1a0d9c507db89cf05192da510bb31dc52d071da1 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Fri, 16 Aug 2013 11:18:05 -0700 Subject: [PATCH 10/32] drop flexattr namespaces Namespaces were a worthy idea, but they added a lot of complexity to both the library code itself and every client of the flexattrs interfaces. Getting rid of them, and having one flat namespace of both traditional fields and flexattrs, has one huge benefit: we can "promote" flexattrs to real attributes (and vice versa) without code changes in every client. This frees us to have a somewhat less efficient implementation of flexattrs because we have a smooth upgrade path for making attributes more efficient via promotion. --- beets/library.py | 126 +++++++++++++++++++++++-------------------- beets/ui/commands.py | 13 ++--- 2 files changed, 71 insertions(+), 68 deletions(-) diff --git a/beets/library.py b/beets/library.py index e29ce8959..468ce063c 100644 --- a/beets/library.py +++ b/beets/library.py @@ -229,12 +229,9 @@ def format_for_path(value, key=None, pathmod=None): def get_flexattrs(rows): """Given a list of SQLite result rows, construct a dictionary - hierarchy reflecting the contained flexible attribute values. + reflecting the contained flexible attribute values. """ - out = defaultdict(dict) - for row in rows: - out[row['namespace']][row['key']] = row['value'] - return dict(out) + return {row['key']: row['value'] for row in rows} # Exceptions. @@ -246,9 +243,15 @@ class InvalidFieldError(Exception): # Library items (songs). class Item(object): - def __init__(self, values, flexattrs=None): + def __init__(self, values=None, flexattrs=None): self.dirty = {} - self._fill_record(values, flexattrs) + self.record = {k: None for k in ITEM_KEYS} + self.flexattrs = {} + + if values: + self.update(values) + if flexattrs: + self.update(flexattrs) self._clear_dirty() @classmethod @@ -263,50 +266,32 @@ class Item(object): i.mtime = i.current_mtime() # Initial mtime. return i - def _fill_record(self, values, flexattrs=None): - self.record = {} - for key in ITEM_KEYS: - try: - setattr(self, key, values[key]) - except KeyError: - setattr(self, key, None) - - self.flexattrs = defaultdict(dict) - if flexattrs: - self.flexattrs.update(flexattrs) - def _clear_dirty(self): self.dirty = {} for key in ITEM_KEYS: self.dirty[key] = False def __repr__(self): - return 'Item(' + repr(self.record) + ')' + return 'Item({0!r}, {1!r})'.format(self.record, self.flexattrs) # Item field accessors. - def __getattr__(self, key): - """If key is an item attribute (i.e., a column in the database), - returns the record entry for that key. + def __getitem__(self, key): + """Get the item's value for a standard field or a flexattr. """ if key in ITEM_KEYS: return self.record.get(key) - elif '-' in key: - namespace, key = key.split('-', 1) - if namespace in self.flexattrs: - return self.flexattrs[namespace].get(key) - return None + elif key in self.flexattrs: + return self.flexattrs[key] else: - raise AttributeError(key + ' is not a valid item field') + raise KeyError('item does not have the key {0}'.format(key)) - def __setattr__(self, key, value): - """If key is an item attribute (i.e., a column in the database), - sets the record entry for that key to value. Note that to change - the attribute in the database or in the file's tags, one must - call store() or write(). + def __contains__(self, key): + return key in ITEM_KEYS or key in self.flexattrs - Otherwise, performs an ordinary setattr. + def __setitem__(self, key, value): + """Set the item's value for a standard field or a flexattr. """ # Encode unicode paths and read buffers. if key == 'path': @@ -323,12 +308,39 @@ class Item(object): if key in ITEM_KEYS_WRITABLE: self.mtime = 0 # Reset mtime on dirty. - elif '-' in key: - namespace, key = key.split('-', 1) - self.flexattrs[namespace][key] = value - else: + self.flexattrs[key] = value + + def update(self, values): + for key, value in values.items(): + self[key] = value + + def get(self, key, default=None): + if key in self: + return self[key] + else: + return default + + def __getattr__(self, key): + """If key is an item attribute (i.e., a column in the database), + returns the record entry for that key. + """ + try: + return super(Item, self).__getattr__(key) + except AttributeError: + if key in self: + return self[key] + else: + raise AttributeError(key + ' is not an item field') + + def __setattr__(self, key, value): + """Set the item's value for a standard field or a flexattr. + """ + # FIXME hack to allow self.x = y in __init__ + if key in ('record', 'flexattrs', 'dirty'): super(Item, self).__setattr__(key, value) + else: + self[key] = value # Interaction with file metadata. @@ -450,11 +462,10 @@ class Item(object): mapping['albumartist'] = mapping['artist'] # Flexible attributes. - for namespace, attrs in self.flexattrs.items(): - for key, value in attrs.items(): - if sanitize: - value = format_for_path(value, None, pathmod) - mapping['{}-{}'.format(namespace, key)] = value + for key, value in self.flexattrs.items(): + if sanitize: + value = format_for_path(value, None, pathmod) + mapping[key] = value # Get values from plugins. for key, value in plugins.template_values(self).items(): @@ -836,7 +847,7 @@ class ResultIterator(object): 'SELECT * FROM item_attributes WHERE entity_id=?', (row['id'],) ) - return Item(row, get_flexattrs(flex_rows)) + return Item(dict(row), get_flexattrs(flex_rows)) # Regular expression for parse_query_part, below. PARSE_QUERY_PART_REGEX = re.compile( @@ -1152,7 +1163,6 @@ class Transaction(object): """Execute an SQL statement with substitution values and return the row ID of the last affected row. """ - print statement, subvals cursor = self.lib._connection().execute(statement, subvals) plugins.send('database_change', lib=self.lib) return cursor.lastrowid @@ -1261,10 +1271,9 @@ class Library(BaseLibrary): CREATE TABLE IF NOT EXISTS {0}_attributes ( id INTEGER PRIMARY KEY, entity_id INTEGER, - namespace TEXT, key TEXT, value TEXT, - UNIQUE(entity_id, namespace, key) ON CONFLICT REPLACE); + UNIQUE(entity_id, key) ON CONFLICT REPLACE); CREATE INDEX IF NOT EXISTS {0}_id_attribute ON {0}_attributes (entity_id); """.format(entity)) @@ -1410,12 +1419,11 @@ class Library(BaseLibrary): # Flexible attributes. flexins = 'INSERT INTO item_attributes ' \ - ' (entity_id, namespace, key, value)' \ + ' (entity_id, key, value)' \ ' VALUES (?, ?, ?, ?)' - for namespace, attrs in item.flexattrs.items(): - for key, value in attrs.items(): - if value is not None: - tx.mutate(flexins, (new_id, key, value)) + for key, value in item.flexattrs.items(): + if value is not None: + tx.mutate(flexins, (new_id, key, value)) item._clear_dirty() item.id = new_id @@ -1426,7 +1434,8 @@ class Library(BaseLibrary): if load_id is None: load_id = item.id stored_item = self.get_item(load_id) - item._fill_record(stored_item.record, stored_item.flexattrs) + item.update(stored_item.record) + item.update(stored_item.flexattrs) item._clear_dirty() def store(self, item, store_id=None, store_all=False): @@ -1456,11 +1465,10 @@ class Library(BaseLibrary): # Flexible attributes. flexins = 'INSERT INTO item_attributes ' \ - ' (entity_id, namespace, key, value)' \ - ' VALUES (?, ?, ?, ?)' - for namespace, attrs in item.flexattrs.items(): - for key, value in attrs.items(): - tx.mutate(flexins, (store_id, namespace, key, value)) + ' (entity_id, key, value)' \ + ' VALUES (?, ?, ?)' + for key, value in item.flexattrs.items(): + tx.mutate(flexins, (store_id, key, value)) item._clear_dirty() self._memotable = {} diff --git a/beets/ui/commands.py b/beets/ui/commands.py index 04a5ebce8..6f17ad5cb 100644 --- a/beets/ui/commands.py +++ b/beets/ui/commands.py @@ -1099,6 +1099,8 @@ def _convert_type(key, value, album=False): `album` indicates whether to use album or item field definitions. """ fields = library.ALBUM_FIELDS if album else library.ITEM_FIELDS + if key not in fields: + return value typ = [f[1] for f in fields if f[0] == key][0] if typ is bool: @@ -1120,15 +1122,9 @@ def _convert_type(key, value, album=False): def modify_items(lib, mods, query, write, move, album, confirm): """Modifies matching items according to key=value assignments.""" # Parse key=value specifications into a dictionary. - if album: - allowed_keys = library.ALBUM_KEYS - else: - allowed_keys = library.ITEM_KEYS_WRITABLE + ['added'] fsets = {} for mod in mods: key, value = mod.split('=', 1) - if key not in allowed_keys and '-' not in key: - raise ui.UserError('"%s" is not a valid field' % key) fsets[key] = _convert_type(key, value, album) # Get the items to modify. @@ -1143,8 +1139,7 @@ def modify_items(lib, mods, query, write, move, album, confirm): # Show each change. for field, value in fsets.iteritems(): - curval = getattr(obj, field) - _showdiff(field, curval, value) + _showdiff(field, obj.get(field), value) # Confirm. if confirm: @@ -1156,7 +1151,7 @@ def modify_items(lib, mods, query, write, move, album, confirm): with lib.transaction(): for obj in objs: for field, value in fsets.iteritems(): - setattr(obj, field, value) + obj[field] = value if move: cur_path = obj.item_dir() if album else obj.path From 7d9f556cbe8de919d585508b57a29320304d0f6f Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Fri, 16 Aug 2013 14:28:46 -0700 Subject: [PATCH 11/32] introducing "slow queries" In preparation for enabling queries over flexattrs, this is a new path that lets queries avoid generating SQLite expressions altogether. Any query that can be completely evaluated in SQLite will be, but when it can't, we now fall back to running the entire query in Python by selecting everything from the database and running the `match` predicate. To begin with, this mechanism replaces RegisteredFieldQueries, which previously used Python callbacks for evaluation. Now they just indicate that they're slow queries and the query system falls back automatically. This has the great upside that it lets use implement arbitrarily complex queries without shoehorning everything into SQLite when that (a) is way too complicated and (b) doesn't buy us much performance anyway. The obvious drawback is that any code dealing with queries now has to handle two cases (slow and fast). In the future, we could optimize this further by combing fast and slow query styles. For example, if you want to match with a substring *and* a regular expression, we can do a first pass in SQLite and apply the regex predicate on the results. Avoided for now because premature optimization, etc., etc. Next step: implement flexattr matches as slow queries. --- beets/library.py | 130 ++++++++++++++++++--------------------- beetsplug/fuzzy.py | 4 +- docs/plugins/writing.rst | 18 +++--- 3 files changed, 72 insertions(+), 80 deletions(-) diff --git a/beets/library.py b/beets/library.py index 468ce063c..59c55303c 100644 --- a/beets/library.py +++ b/beets/library.py @@ -492,11 +492,14 @@ class Query(object): """An abstract class representing a query into the item database. """ def clause(self): - """Returns (clause, subvals) where clause is a valid sqlite + """Generate an SQLite expression implementing the query. + Return a clause string, a sequence of substitution values for + the clause, and a Query object representing the "remainder" + Returns (clause, subvals) where clause is a valid sqlite WHERE clause implementing the query and subvals is a list of items to be substituted for ?s in the clause. """ - raise NotImplementedError + return None, () def match(self, item): """Check whether this query matches a given Item. Can be used to @@ -504,24 +507,6 @@ class Query(object): """ raise NotImplementedError - def statement(self): - """Returns (query, subvals) where clause is a sqlite SELECT - statement to enact this query and subvals is a list of values - to substitute in for ?s in the query. - """ - clause, subvals = self.clause() - return ('SELECT * FROM items WHERE ' + clause, subvals) - - def count(self, tx): - """Returns `(num, length)` where `num` is the number of items in - the library matching this query and `length` is their total - length in seconds. - """ - clause, subvals = self.clause() - statement = 'SELECT COUNT(id), SUM(length) FROM items WHERE ' + clause - result = tx.query(statement, subvals)[0] - return (result[0], result[1] or 0.0) - class FieldQuery(Query): """An abstract query that searches in a specific field for a pattern. Subclasses must provide a `value_match` class method, which @@ -550,25 +535,6 @@ class FieldQuery(Query): def match(self, item): return self._raw_value_match(self.pattern, getattr(item, self.field)) -class RegisteredFieldQuery(FieldQuery): - """A FieldQuery that uses a registered SQLite callback function. - Before it can be used to execute queries, the `register` method must - be called. - """ - def clause(self): - # Invoke the registered SQLite function. - clause = "{name}(?, {field})".format(name=self.__class__.__name__, - field=self.field) - return clause, [self.pattern] - - @classmethod - def register(cls, conn): - """Register this query's matching function with the SQLite - connection. This method should only be invoked when the query - type chooses not to override `clause`. - """ - conn.create_function(cls.__name__, 2, cls._raw_value_match) - class MatchQuery(FieldQuery): """A query that looks for exact matches in an item field.""" def clause(self): @@ -596,7 +562,7 @@ class SubstringQuery(FieldQuery): def value_match(cls, pattern, value): return pattern.lower() in value.lower() -class RegexpQuery(RegisteredFieldQuery): +class RegexpQuery(FieldQuery): """A query that matches a regular expression in a specific item field. """ @@ -721,6 +687,9 @@ class CollectionQuery(Query): subvals = [] for subq in self.subqueries: subq_clause, subq_subvals = subq.clause() + if not subq_clause: + # Fall back to slow query. + return None, () clause_parts.append('(' + subq_clause + ')') subvals += subq_subvals clause = (' ' + joiner + ' ').join(clause_parts) @@ -832,22 +801,27 @@ class ResultIterator(object): """An iterator into an item query result set. The iterator lazily constructs Item objects that reflect database rows. """ - def __init__(self, rows, lib): + def __init__(self, rows, lib, query=None): self.rows = rows self.rowiter = iter(self.rows) self.lib = lib + self.query = query def __iter__(self): return self def next(self): - row = self.rowiter.next() # May raise StopIteration. - with self.lib.transaction() as tx: - flex_rows = tx.query( - 'SELECT * FROM item_attributes WHERE entity_id=?', - (row['id'],) - ) - return Item(dict(row), get_flexattrs(flex_rows)) + for row in self.rowiter: # Iterate until we get a hit. + with self.lib.transaction() as tx: + flex_rows = tx.query( + 'SELECT * FROM item_attributes WHERE entity_id=?', + (row['id'],) + ) + item = Item(dict(row), get_flexattrs(flex_rows)) + if self.query and not self.query.match(item): + continue + return item + raise StopIteration() # Reached the end of the DB rows. # Regular expression for parse_query_part, below. PARSE_QUERY_PART_REGEX = re.compile( @@ -1296,12 +1270,6 @@ class Library(BaseLibrary): # Access SELECT results like dictionaries. conn.row_factory = sqlite3.Row - # Register plugin queries. - RegexpQuery.register(conn) - for prefix, query_class in plugins.queries().items(): - if issubclass(query_class, RegisteredFieldQuery): - query_class.register(conn) - self._connections[thread_id] = conn return conn @@ -1547,14 +1515,30 @@ class Library(BaseLibrary): if artist is not None: # "Add" the artist to the query. query = AndQuery((query, MatchQuery('albumartist', artist))) + where, subvals = query.clause() - sql = "SELECT * FROM albums " + \ - "WHERE " + where + \ - " ORDER BY %s, album" % \ - _orelse("albumartist_sort", "albumartist") with self.transaction() as tx: - rows = tx.query(sql, subvals) - return [Album(self, dict(res)) for res in rows] + rows = tx.query( + "SELECT * FROM albums WHERE {0} " + "ORDER BY {1}, album".format( + where or '1', + _orelse("albumartist_sort", "albumartist"), + ), + subvals, + ) + + if where: + # Fast query. + return [Album(self, dict(res)) for res in rows] + else: + # Slow query. + # FIXME both should be iterators. + out = [] + for row in rows: + album = Album(self, dict(res)) + if query.match(album): + out.append(album) + return out def items(self, query=None, artist=None, album=None, title=None): queries = [get_query(query, False)] @@ -1564,17 +1548,25 @@ class Library(BaseLibrary): queries.append(MatchQuery('album', album)) if title is not None: queries.append(MatchQuery('title', title)) - super_query = AndQuery(queries) - where, subvals = super_query.clause() + query = AndQuery(queries) + where, subvals = query.clause() - sql = "SELECT * FROM items " + \ - "WHERE " + where + \ - " ORDER BY %s, album, disc, track" % \ - _orelse("artist_sort", "artist") - log.debug('Getting items with SQL: %s' % sql) with self.transaction() as tx: - rows = tx.query(sql, subvals) - return ResultIterator(rows, self) + rows = tx.query( + "SELECT * FROM items WHERE {0} " + "ORDER BY {1}, album, disc, track".format( + where or '1', + _orelse("artist_sort", "artist"), + ), + subvals + ) + + if where: + # Fast query. + return ResultIterator(rows, self) + else: + # Slow query. + return ResultIterator(rows, self, query) # Convenience accessors. diff --git a/beetsplug/fuzzy.py b/beetsplug/fuzzy.py index b6ad90d87..21d1c38e2 100644 --- a/beetsplug/fuzzy.py +++ b/beetsplug/fuzzy.py @@ -16,12 +16,12 @@ """ from beets.plugins import BeetsPlugin -from beets.library import RegisteredFieldQuery +from beets.library import FieldQuery import beets import difflib -class FuzzyQuery(RegisteredFieldQuery): +class FuzzyQuery(FieldQuery): @classmethod def value_match(self, pattern, val): # smartcase diff --git a/docs/plugins/writing.rst b/docs/plugins/writing.rst index 27feace65..186a7689b 100644 --- a/docs/plugins/writing.rst +++ b/docs/plugins/writing.rst @@ -357,18 +357,18 @@ To do so, define a subclass of the ``Query`` type from the ``beets.library`` module. Then, in the ``queries`` method of your plugin class, return a dictionary mapping prefix strings to query classes. -One simple kind of query you can extend is the ``RegisteredFieldQuery``, which -implements string comparisons. To use it, create a subclass inheriting from -that class and override the ``value_match`` class method. (Remember the -``@classmethod`` decorator!) The following example plugin declares a query -using the ``@`` prefix to delimit exact string matches. The plugin will be -used if we issue a command like ``beet ls @something`` or ``beet ls -artist:@something``:: +One simple kind of query you can extend is the ``FieldQuery``, which +implements string comparisons on fields. To use it, create a subclass +inheriting from that class and override the ``value_match`` class method. +(Remember the ``@classmethod`` decorator!) The following example plugin +declares a query using the ``@`` prefix to delimit exact string matches. The +plugin will be used if we issue a command like ``beet ls @something`` or +``beet ls artist:@something``:: from beets.plugins import BeetsPlugin - from beets.library import PluginQuery + from beets.library import FieldQuery - class ExactMatchQuery(PluginQuery): + class ExactMatchQuery(FieldQuery): @classmethod def value_match(self, pattern, val): return pattern == val From 206dee34f89801b03111786b4214bfa851d031d2 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Fri, 16 Aug 2013 14:46:46 -0700 Subject: [PATCH 12/32] flexible attributes as slow queries Queries over flexible attributes now Just Work! --- beets/library.py | 43 ++++++++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 17 deletions(-) diff --git a/beets/library.py b/beets/library.py index 59c55303c..e8b7d6069 100644 --- a/beets/library.py +++ b/beets/library.py @@ -511,12 +511,23 @@ class FieldQuery(Query): """An abstract query that searches in a specific field for a pattern. Subclasses must provide a `value_match` class method, which determines whether a certain pattern string matches a certain value - string. Subclasses also need to provide `clause` to implement the + string. Subclasses may also provide `col_clause` to implement the same matching functionality in SQLite. """ - def __init__(self, field, pattern): + def __init__(self, field, pattern, fast=True): self.field = field self.pattern = pattern + self.fast = fast + + def col_clause(self): + return None, () + + def clause(self): + if self.fast: + return self.col_clause() + else: + # Matching a flexattr. This is a slow query. + return None, () @classmethod def value_match(cls, pattern, value): @@ -533,11 +544,11 @@ class FieldQuery(Query): return cls.value_match(pattern, util.as_string(value)) def match(self, item): - return self._raw_value_match(self.pattern, getattr(item, self.field)) + return self._raw_value_match(self.pattern, item.get(self.field)) class MatchQuery(FieldQuery): """A query that looks for exact matches in an item field.""" - def clause(self): + def col_clause(self): pattern = self.pattern if self.field == 'path' and isinstance(pattern, str): pattern = buffer(pattern) @@ -551,7 +562,7 @@ class MatchQuery(FieldQuery): class SubstringQuery(FieldQuery): """A query that matches a substring in a specific item field.""" - def clause(self): + def col_clause(self): search = '%' + (self.pattern.replace('\\','\\\\').replace('%','\\%') .replace('_','\\_')) + '%' clause = self.field + " like ? escape '\\'" @@ -638,7 +649,7 @@ class NumericQuery(FieldQuery): return False return True - def clause(self): + def col_clause(self): if self.point is not None: return self.field + '=?', (self.point,) else: @@ -737,7 +748,7 @@ class AnyFieldQuery(CollectionQuery): subqueries = [] for field in self.fields: - subqueries.append(cls(field, pattern)) + subqueries.append(cls(field, pattern, True)) super(AnyFieldQuery, self).__init__(subqueries) def clause(self): @@ -895,12 +906,14 @@ def construct_query_part(query_part, default_fields, all_keys): # Other query type. return query_class(pattern) + key = key.lower() + # A boolean field. - elif key.lower() == 'comp': - return BooleanQuery(key.lower(), pattern) + if key.lower() == 'comp': + return BooleanQuery(key, pattern) # Path field. - elif key.lower() == 'path' and 'path' in all_keys: + elif key == 'path' and 'path' in all_keys: if query_class is SubstringQuery: # By default, use special path matching logic. return PathQuery(pattern) @@ -908,17 +921,13 @@ def construct_query_part(query_part, default_fields, all_keys): # Specific query type requested. return query_class('path', pattern) - # Other (recognized) field. - elif key.lower() in all_keys: - return query_class(key.lower(), pattern) - # Singleton query (not a real field). - elif key.lower() == 'singleton': + elif key == 'singleton': return SingletonQuery(util.str2bool(pattern)) - # Unrecognized field. + # Other field. else: - log.warn(u'no such field in query: {0}'.format(key)) + return query_class(key.lower(), pattern, key in all_keys) def get_query(val, album=False): """Takes a value which may be None, a query string, a query string From 38f1e6aec2221b25deb5c187057b47bdc127b969 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Fri, 16 Aug 2013 17:22:17 -0700 Subject: [PATCH 13/32] new FlexModel base class I intend to use this for both Item and Album to avoid code duplication and simplify code that uses both entities. --- beets/library.py | 185 ++++++++++++++++++++++++------------------- beetsplug/inline.py | 2 +- beetsplug/missing.py | 68 ++++++++-------- 3 files changed, 139 insertions(+), 116 deletions(-) diff --git a/beets/library.py b/beets/library.py index e8b7d6069..6b65a82bc 100644 --- a/beets/library.py +++ b/beets/library.py @@ -227,12 +227,6 @@ def format_for_path(value, key=None, pathmod=None): return value -def get_flexattrs(rows): - """Given a list of SQLite result rows, construct a dictionary - reflecting the contained flexible attribute values. - """ - return {row['key']: row['value'] for row in rows} - # Exceptions. @@ -242,17 +236,103 @@ class InvalidFieldError(Exception): # Library items (songs). -class Item(object): - def __init__(self, values=None, flexattrs=None): - self.dirty = {} - self.record = {k: None for k in ITEM_KEYS} - self.flexattrs = {} +class FlexModel(object): + """An abstract object that consists of a set of "fast" (fixed) + fields and an arbitrary number of flexible fields. + """ - if values: - self.update(values) - if flexattrs: - self.update(flexattrs) - self._clear_dirty() + _fields = () + """The available "fixed" fields on this type. + """ + + def __init__(self, **values): + """Create a new object with the given field values (which may be + fixed or flex fields). + """ + self._dirty = set() + self._values_fixed = {} + self._values_flex = {} + self.update(values) + self.clear_dirty() + + def __repr__(self): + return '{0}({1})'.format( + type(self).__name__, + ', '.join('{0}={1!r}'.format(k, v) for k, v in dict(self).items()), + ) + + def clear_dirty(self): + self._dirty = set() + + + # Act like a dictionary. + + def __getitem__(self, key): + """Get the value for a field. Fixed fields always return a value + (which may be None); flex fields may raise a KeyError. + """ + if key in self._fields: + return self._values_fixed.get(key) + elif key in self._values_flex: + return self._values_flex[key] + else: + raise KeyError(key) + + def __setitem__(self, key, value): + """Assign the value for a field. + """ + if key in self._fields: + self._values_fixed[key] = value + else: + self._values_flex[key] = value + self._dirty.add(key) + + def update(self, values): + """Assign all values in the given dict. + """ + for key, value in values.items(): + self[key] = value + + def keys(self): + """Get all the keys (both fixed and flex) on this object. + """ + return list(self._fields) + self._values_flex.keys() + + def get(self, key, default=None): + """Get the value for a given key or `default` if it does not + exist. + """ + if key in self: + return self[key] + else: + return default + + def __contains__(self, key): + """Determine whether `key` is a fixed or flex attribute on this + object. + """ + return key in self._fields or key in self._values_flex + + + # Convenient attribute access. + + def __getattr__(self, key): + if key.startswith('_'): + return super(FlexModel, self).__getattr__(key) + else: + try: + return self[key] + except KeyError: + raise AttributeError(key) + + def __setattr__(self, key, value): + if key.startswith('_'): + super(FlexModel, self).__setattr__(key, value) + else: + self[key] = value + +class Item(FlexModel): + _fields = ITEM_FIELDS @classmethod def from_path(cls, path): @@ -266,30 +346,6 @@ class Item(object): i.mtime = i.current_mtime() # Initial mtime. return i - def _clear_dirty(self): - self.dirty = {} - for key in ITEM_KEYS: - self.dirty[key] = False - - def __repr__(self): - return 'Item({0!r}, {1!r})'.format(self.record, self.flexattrs) - - - # Item field accessors. - - def __getitem__(self, key): - """Get the item's value for a standard field or a flexattr. - """ - if key in ITEM_KEYS: - return self.record.get(key) - elif key in self.flexattrs: - return self.flexattrs[key] - else: - raise KeyError('item does not have the key {0}'.format(key)) - - def __contains__(self, key): - return key in ITEM_KEYS or key in self.flexattrs - def __setitem__(self, key, value): """Set the item's value for a standard field or a flexattr. """ @@ -300,47 +356,10 @@ class Item(object): elif isinstance(value, buffer): value = str(value) - if key in ITEM_KEYS: - # If the value changed, mark the field as dirty. - if (key not in self.record) or (self.record[key] != value): - self.record[key] = value - self.dirty[key] = True - if key in ITEM_KEYS_WRITABLE: - self.mtime = 0 # Reset mtime on dirty. + if key in ITEM_KEYS_WRITABLE: + self.mtime = 0 # Reset mtime on dirty. - else: - self.flexattrs[key] = value - - def update(self, values): - for key, value in values.items(): - self[key] = value - - def get(self, key, default=None): - if key in self: - return self[key] - else: - return default - - def __getattr__(self, key): - """If key is an item attribute (i.e., a column in the database), - returns the record entry for that key. - """ - try: - return super(Item, self).__getattr__(key) - except AttributeError: - if key in self: - return self[key] - else: - raise AttributeError(key + ' is not an item field') - - def __setattr__(self, key, value): - """Set the item's value for a standard field or a flexattr. - """ - # FIXME hack to allow self.x = y in __init__ - if key in ('record', 'flexattrs', 'dirty'): - super(Item, self).__setattr__(key, value) - else: - self[key] = value + super(Item, self).__setitem__(key, value) # Interaction with file metadata. @@ -462,7 +481,7 @@ class Item(object): mapping['albumartist'] = mapping['artist'] # Flexible attributes. - for key, value in self.flexattrs.items(): + for key, value in self._values_flex.items(): if sanitize: value = format_for_path(value, None, pathmod) mapping[key] = value @@ -828,7 +847,9 @@ class ResultIterator(object): 'SELECT * FROM item_attributes WHERE entity_id=?', (row['id'],) ) - item = Item(dict(row), get_flexattrs(flex_rows)) + values = dict(row) + values.update({row['key']: row['value'] for row in flex_rows}) + item = Item(**values) if self.query and not self.query.match(item): continue return item diff --git a/beetsplug/inline.py b/beetsplug/inline.py index 9af015f73..40537caca 100644 --- a/beetsplug/inline.py +++ b/beetsplug/inline.py @@ -75,7 +75,7 @@ def compile_inline(python_code, album): out['items'] = list(obj.items()) return out else: - return dict(obj.record) + return dict(obj) if is_expr: # For expressions, just evaluate and return the result. diff --git a/beetsplug/missing.py b/beetsplug/missing.py index 90289cf07..0b12ebde6 100644 --- a/beetsplug/missing.py +++ b/beetsplug/missing.py @@ -61,39 +61,41 @@ def _item(track_info, album_info, album_id): t = track_info a = album_info - return Item({'album_id': album_id, - 'album': a.album, - 'albumartist': a.artist, - 'albumartist_credit': a.artist_credit, - 'albumartist_sort': a.artist_sort, - 'albumdisambig': a.albumdisambig, - 'albumstatus': a.albumstatus, - 'albumtype': a.albumtype, - 'artist': t.artist, - 'artist_credit': t.artist_credit, - 'artist_sort': t.artist_sort, - 'asin': a.asin, - 'catalognum': a.catalognum, - 'comp': a.va, - 'country': a.country, - 'day': a.day, - 'disc': t.medium, - 'disctitle': t.disctitle, - 'disctotal': a.mediums, - 'label': a.label, - 'language': a.language, - 'length': t.length, - 'mb_albumid': a.album_id, - 'mb_artistid': t.artist_id, - 'mb_releasegroupid': a.releasegroup_id, - 'mb_trackid': t.track_id, - 'media': a.media, - 'month': a.month, - 'script': a.script, - 'title': t.title, - 'track': t.index, - 'tracktotal': len(a.tracks), - 'year': a.year}) + return Item( + album_id = album_id, + album = a.album, + albumartist = a.artist, + albumartist_credit = a.artist_credit, + albumartist_sort = a.artist_sort, + albumdisambig = a.albumdisambig, + albumstatus = a.albumstatus, + albumtype = a.albumtype, + artist = t.artist, + artist_credit = t.artist_credit, + artist_sort = t.artist_sort, + asin = a.asin, + catalognum = a.catalognum, + comp = a.va, + country = a.country, + day = a.day, + disc = t.medium, + disctitle = t.disctitle, + disctotal = a.mediums, + label = a.label, + language = a.language, + length = t.length, + mb_albumid = a.album_id, + mb_artistid = t.artist_id, + mb_releasegroupid = a.releasegroup_id, + mb_trackid = t.track_id, + media = a.media, + month = a.month, + script = a.script, + title = t.title, + track = t.index, + tracktotal = len(a.tracks), + year = a.year, + ) class MissingPlugin(BeetsPlugin): From abfad7b2a976f6eb04347a6678e5f290fd329677 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Fri, 16 Aug 2013 17:28:21 -0700 Subject: [PATCH 14/32] kill BaseAlbum I added this back when I thought I would implement a different subclass that represented iPods. That means I wrote this code when I had a lime iPod mini. I am old. --- beets/library.py | 124 ++++++++++------------------------------------- 1 file changed, 26 insertions(+), 98 deletions(-) diff --git a/beets/library.py b/beets/library.py index 6b65a82bc..63915433c 100644 --- a/beets/library.py +++ b/beets/library.py @@ -979,102 +979,6 @@ def get_query(val, album=False): # An abstract library. -class BaseLibrary(object): - """Abstract base class for music libraries, which are loosely - defined as sets of Items. - """ - def __init__(self): - raise NotImplementedError - - - # Basic operations. - - def add(self, item, copy=False): - """Add the item as a new object to the library database. The id - field will be updated; the new id is returned. If copy, then - each item is copied to the destination location before it is - added. - """ - raise NotImplementedError - - def load(self, item, load_id=None): - """Refresh the item's metadata from the library database. If - fetch_id is not specified, use the item's current id. - """ - raise NotImplementedError - - def store(self, item, store_id=None, store_all=False): - """Save the item's metadata into the library database. If - store_id is specified, use it instead of the item's current id. - If store_all is true, save the entire record instead of just - the dirty fields. - """ - raise NotImplementedError - - def remove(self, item): - """Removes the item from the database (leaving the file on - disk). - """ - raise NotImplementedError - - - # Browsing operations. - # Naive implementations are provided, but these methods should be - # overridden if a better implementation exists. - - def _get(self, query=None, default_fields=None): - """Returns a sequence of the items matching query, which may - be None (match the entire library), a Query object, or a query - string. If default_fields is specified, it restricts the fields - that may be matched by unqualified query string terms. - """ - raise NotImplementedError - - def albums(self, artist=None, query=None): - """Returns a sorted list of BaseAlbum objects, possibly filtered - by an artist name or an arbitrary query. Unqualified query - string terms only match fields that apply at an album - granularity: artist, album, and genre. - """ - # Gather the unique album/artist names and associated example - # Items. - specimens = {} - for item in self._get(query, ALBUM_DEFAULT_FIELDS): - if (artist is None or item.artist == artist): - key = (item.artist, item.album) - if key not in specimens: - specimens[key] = item - - # Build album objects. - for k in sorted(specimens.keys()): - item = specimens[k] - record = {} - for key in ALBUM_KEYS_ITEM: - record[key] = getattr(item, key) - yield BaseAlbum(self, record) - - def items(self, artist=None, album=None, title=None, query=None): - """Returns a sequence of the items matching the given artist, - album, title, and query (if present). Sorts in such a way as to - group albums appropriately. Unqualified query string terms only - match intuitively relevant fields: artist, album, genre, title, - and comments. - """ - out = [] - for item in self._get(query, ITEM_DEFAULT_FIELDS): - if (artist is None or item.artist == artist) and \ - (album is None or item.album == album) and \ - (title is None or item.title == title): - out.append(item) - - # Sort by: artist, album, disc, track. - def compare(a, b): - return cmp(a.artist, b.artist) or \ - cmp(a.album, b.album) or \ - cmp(a.disc, b.disc) or \ - cmp(a.track, b.track) - return sorted(out, compare) - class BaseAlbum(object): """Represents an album in the library, which in turn consists of a collection of items in the library. @@ -1121,7 +1025,7 @@ class BaseAlbum(object): self._record[key] = getattr(item, key) -# Concrete DB-backed library. +# The Library: interface to the database. class Transaction(object): """A context manager for safe, concurrent access to the database. @@ -1175,7 +1079,7 @@ class Transaction(object): """Execute a string containing multiple SQL statements.""" self.lib._connection().executescript(statements) -class Library(BaseLibrary): +class Library(object): """A music library using an SQLite database as a metadata store.""" def __init__(self, path='library.blb', directory='~/Music', @@ -1392,6 +1296,11 @@ class Library(BaseLibrary): # Item manipulation. def add(self, item, copy=False): + """Add the item as a new object to the library database. The id + field will be updated; the new id is returned. If copy, then + each item is copied to the destination location before it is + added. + """ item.added = time.time() if copy: self.move(item, copy=True) @@ -1429,6 +1338,9 @@ class Library(BaseLibrary): return new_id def load(self, item, load_id=None): + """Refresh the item's metadata from the library database. If + fetch_id is not specified, use the item's current id. + """ if load_id is None: load_id = item.id stored_item = self.get_item(load_id) @@ -1437,6 +1349,11 @@ class Library(BaseLibrary): item._clear_dirty() def store(self, item, store_id=None, store_all=False): + """Save the item's metadata into the library database. If + store_id is specified, use it instead of the item's current id. + If store_all is true, save the entire record instead of just + the dirty fields. + """ if store_id is None: store_id = item.id @@ -1541,6 +1458,11 @@ class Library(BaseLibrary): # Querying. def albums(self, query=None, artist=None): + """Returns a sorted list of Album objects, possibly filtered + by an artist name or an arbitrary query. Unqualified query + string terms only match fields that apply at an album + granularity: artist, album, and genre. + """ query = get_query(query, True) if artist is not None: # "Add" the artist to the query. @@ -1571,6 +1493,12 @@ class Library(BaseLibrary): return out def items(self, query=None, artist=None, album=None, title=None): + """Returns a sequence of the items matching the given artist, + album, title, and query (if present). Sorts in such a way as to + group albums appropriately. Unqualified query string terms only + match intuitively relevant fields: artist, album, genre, title, + and comments. + """ queries = [get_query(query, False)] if artist is not None: queries.append(MatchQuery('artist', artist)) From 276ce14dd24b768b2ecd2dd54862bd926c6344a4 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Fri, 16 Aug 2013 18:36:30 -0700 Subject: [PATCH 15/32] flexattrs work for Albums A second base class, LibModel, maintains a reference to the Library and should take care of database-related tasks like load and store. This is the beginning of the end of the terrible incongruity between Item and Album objects (only the latter had a library reference). More refactoring to come. One large side effect: Album objects no longer automatically store modifications. You have to call album.store(). Several places in the code assume otherwise; they need cleaning up. ResultIterator is now polymorphic (it takes a type parameter, which must be a subclass of LibModel). --- beets/library.py | 274 +++++++++++++++++++------------------------ beets/ui/commands.py | 5 +- beetsplug/inline.py | 6 +- 3 files changed, 125 insertions(+), 160 deletions(-) diff --git a/beets/library.py b/beets/library.py index 63915433c..053a1f3a2 100644 --- a/beets/library.py +++ b/beets/library.py @@ -318,12 +318,12 @@ class FlexModel(object): def __getattr__(self, key): if key.startswith('_'): - return super(FlexModel, self).__getattr__(key) + raise AttributeError('model has no attribute {0!r}'.format(key)) else: try: return self[key] except KeyError: - raise AttributeError(key) + raise AttributeError('no such field {0!r}'.format(key)) def __setattr__(self, key, value): if key.startswith('_'): @@ -331,17 +331,77 @@ class FlexModel(object): else: self[key] = value -class Item(FlexModel): - _fields = ITEM_FIELDS +class LibModel(FlexModel): + """A model base class that includes a reference to a Library object. + It knows how to load and store itself from the database. + """ + + _table = None + """The main SQLite table name. + """ + + _flex_table = None + """The flex field SQLite table name. + """ + + def __init__(self, lib=None, **values): + self._lib = lib + super(LibModel, self).__init__(**values) + + def store(self, store_id=None, store_all=False): + """Save the object's metadata into the library database. If + store_id is specified, use it instead of the current id. If + store_all is true, save the entire record instead of just the + dirty fields. + """ + if store_id is None: + store_id = self.id + + # Build assignments for query. + assignments = '' + subvars = [] + for key in self._fields: + if (key != 'id') and (key in self._dirty or store_all): + assignments += key + '=?,' + value = getattr(item, key) + # Wrap path strings in buffers so they get stored + # "in the raw". + if key == 'path' and isinstance(value, str): + value = buffer(value) + subvars.append(value) + assignments = assignments[:-1] # Knock off last , + + with self._lib.transaction() as tx: + # Main table update. + if assignments: + query = 'UPDATE {0} SET {1} WHERE id=?'.format( + self._table, assignments + ) + subvars.append(store_id) + tx.mutate(query, subvars) + + # Flexible attributes. + for key, value in self._values_flex.items(): + tx.mutate( + 'INSERT INTO {0} ' + '(entity_id, key, value) ' + 'VALUES (?, ?, ?);'.format(self._flex_table), + (store_id, key, value), + ) + + self.clear_dirty() + +class Item(LibModel): + _fields = ITEM_KEYS + _table = 'items' + _flex_table = 'item_attributes' @classmethod def from_path(cls, path): """Creates a new item from the media file at the specified path. """ # Initiate with values that aren't read from files. - i = cls({ - 'album_id': None, - }) + i = cls(album_id=None) i.read(path) i.mtime = i.current_mtime() # Initial mtime. return i @@ -829,9 +889,10 @@ class PathQuery(Query): class ResultIterator(object): """An iterator into an item query result set. The iterator lazily - constructs Item objects that reflect database rows. + constructs LibModel objects that reflect database rows. """ - def __init__(self, rows, lib, query=None): + def __init__(self, model_class, rows, lib, query=None): + self.model_class = model_class self.rows = rows self.rowiter = iter(self.rows) self.lib = lib @@ -844,15 +905,17 @@ class ResultIterator(object): for row in self.rowiter: # Iterate until we get a hit. with self.lib.transaction() as tx: flex_rows = tx.query( - 'SELECT * FROM item_attributes WHERE entity_id=?', + 'SELECT * FROM {0} WHERE entity_id=?'.format( + self.model_class._flex_table + ), (row['id'],) ) values = dict(row) values.update({row['key']: row['value'] for row in flex_rows}) - item = Item(**values) - if self.query and not self.query.match(item): + obj = self.model_class(self.lib, **values) + if self.query and not self.query.match(obj): continue - return item + return obj raise StopIteration() # Reached the end of the DB rows. # Regular expression for parse_query_part, below. @@ -977,54 +1040,6 @@ def get_query(val, album=False): raise ValueError('query must be None or have type Query or str') -# An abstract library. - -class BaseAlbum(object): - """Represents an album in the library, which in turn consists of a - collection of items in the library. - - This base version just reflects the metadata of the album's items - and therefore isn't particularly useful. The items are referenced - by the record's album and artist fields. Implementations can add - album-level metadata or use distinct backing stores. - """ - def __init__(self, library, record): - super(BaseAlbum, self).__setattr__('_library', library) - super(BaseAlbum, self).__setattr__('_record', record) - - def __getattr__(self, key): - """Get the value for an album attribute.""" - if key in self._record: - return self._record[key] - else: - raise AttributeError('no such field %s' % key) - - def __setattr__(self, key, value): - """Set the value of an album attribute, modifying each of the - album's items. - """ - if key in self._record: - # Reflect change in this object. - self._record[key] = value - # Modify items. - if key in ALBUM_KEYS_ITEM: - items = self._library.items(albumartist=self.albumartist, - album=self.album) - for item in items: - setattr(item, key, value) - self._library.store(item) - else: - super(BaseAlbum, self).__setattr__(key, value) - - def load(self): - """Refresh this album's cached metadata from the library. - """ - items = self._library.items(artist=self.artist, album=self.album) - item = iter(items).next() - for key in ALBUM_KEYS_ITEM: - self._record[key] = getattr(item, key) - - # The Library: interface to the database. class Transaction(object): @@ -1328,11 +1343,11 @@ class Library(object): flexins = 'INSERT INTO item_attributes ' \ ' (entity_id, key, value)' \ ' VALUES (?, ?, ?, ?)' - for key, value in item.flexattrs.items(): + for key, value in item._values_flex.items(): if value is not None: tx.mutate(flexins, (new_id, key, value)) - item._clear_dirty() + item.clear_dirty() item.id = new_id self._memotable = {} return new_id @@ -1344,9 +1359,8 @@ class Library(object): if load_id is None: load_id = item.id stored_item = self.get_item(load_id) - item.update(stored_item.record) - item.update(stored_item.flexattrs) - item._clear_dirty() + item.update(dict(stored_item)) + item.clear_dirty() def store(self, item, store_id=None, store_all=False): """Save the item's metadata into the library database. If @@ -1361,7 +1375,7 @@ class Library(object): assignments = '' subvars = [] for key in ITEM_KEYS: - if (key != 'id') and (item.dirty[key] or store_all): + if (key != 'id') and (key in item._dirty or store_all): assignments += key + '=?,' value = getattr(item, key) # Wrap path strings in buffers so they get stored @@ -1382,10 +1396,10 @@ class Library(object): flexins = 'INSERT INTO item_attributes ' \ ' (entity_id, key, value)' \ ' VALUES (?, ?, ?)' - for key, value in item.flexattrs.items(): + for key, value in item._values_flex.items(): tx.mutate(flexins, (store_id, key, value)) - item._clear_dirty() + item.clear_dirty() self._memotable = {} def remove(self, item, delete=False, with_album=True): @@ -1479,18 +1493,7 @@ class Library(object): subvals, ) - if where: - # Fast query. - return [Album(self, dict(res)) for res in rows] - else: - # Slow query. - # FIXME both should be iterators. - out = [] - for row in rows: - album = Album(self, dict(res)) - if query.match(album): - out.append(album) - return out + return ResultIterator(Album, rows, self, None if where else query) def items(self, query=None, artist=None, album=None, title=None): """Returns a sequence of the items matching the given artist, @@ -1519,12 +1522,7 @@ class Library(object): subvals ) - if where: - # Fast query. - return ResultIterator(rows, self) - else: - # Slow query. - return ResultIterator(rows, self, query) + return ResultIterator(Item, rows, self, None if where else query) # Convenience accessors. @@ -1550,13 +1548,11 @@ class Library(object): if album_id is None: return None - with self.transaction() as tx: - rows = tx.query( - 'SELECT * FROM albums WHERE id=?', - (album_id,) - ) - if rows: - return Album(self, dict(rows[0])) + albums = self.albums(MatchQuery('id', album_id)) + try: + return albums.next() + except StopIteration: + return None def add_album(self, items): """Create a new album in the database with metadata derived @@ -1587,71 +1583,33 @@ class Library(object): self.store(item) # Construct the new Album object. - record = {} - for key in ALBUM_KEYS: - # Unset (non-item) fields default to None. - record[key] = album_values.get(key) - record['id'] = album_id - album = Album(self, record) - + album_values['id'] = album_id + album = Album(self, **album_values) return album -class Album(BaseAlbum): +class Album(LibModel): """Provides access to information about albums stored in a library. Reflects the library's "albums" table, including album art. """ - def __init__(self, lib, record): - # Decode Unicode paths in database. - if 'artpath' in record and isinstance(record['artpath'], unicode): - record['artpath'] = bytestring_path(record['artpath']) - super(Album, self).__init__(lib, record) + _fields = ALBUM_KEYS + _table = 'album' + _flex_table = 'album_attributes' - def __setattr__(self, key, value): + def __setitem__(self, key, value): """Set the value of an album attribute.""" - if key == 'id': - raise AttributeError("can't modify album id") - - elif key in ALBUM_KEYS: - # Make sure paths are bytestrings. - if key == 'artpath' and isinstance(value, unicode): + if key == 'artpath': + if isinstance(value, unicode): value = bytestring_path(value) - - # Reflect change in this object. - self._record[key] = value - - # Store art path as a buffer. - if key == 'artpath' and isinstance(value, str): - value = buffer(value) - - # Change album table. - sql = 'UPDATE albums SET %s=? WHERE id=?' % key - with self._library.transaction() as tx: - tx.mutate(sql, (value, self.id)) - - # Possibly make modification on items as well. - if key in ALBUM_KEYS_ITEM: - for item in self.items(): - setattr(item, key, value) - self._library.store(item) - - else: - object.__setattr__(self, key, value) - - def __getattr__(self, key): - value = super(Album, self).__getattr__(key) - - # Unwrap art path from buffer object. - if key == 'artpath' and isinstance(value, buffer): - value = str(value) - - return value + elif isinstance(value, buffer): + value = bytes(value) + super(Album, self).__setitem__(key, value) def items(self): """Returns an iterable over the items associated with this album. """ - return self._library.items(MatchQuery('album_id', self.id)) + return self._lib.items(MatchQuery('album_id', self.id)) def remove(self, delete=False, with_items=True): """Removes this album and all its associated items from the @@ -1666,11 +1624,11 @@ class Album(BaseAlbum): if artpath: util.remove(artpath) - with self._library.transaction() as tx: + with self._lib.transaction() as tx: if with_items: # Remove items. for item in self.items(): - self._library.remove(item, delete, False) + self._lib.remove(item, delete, False) # Remove album from database. tx.mutate( @@ -1701,22 +1659,22 @@ class Album(BaseAlbum): # Prune old path when moving. if not copy: util.prune_dirs(os.path.dirname(old_art), - self._library.directory) + self._lib.directory) def move(self, copy=False, basedir=None): """Moves (or copies) all items to their destination. Any album art moves along with them. basedir overrides the library base directory for the destination. """ - basedir = basedir or self._library.directory + basedir = basedir or self._lib.directory # Move items. items = list(self.items()) for item in items: - self._library.move(item, copy, basedir=basedir, with_album=False) + self._lib.move(item, copy, basedir=basedir, with_album=False) # Move art. - self.move_art(copy) + self.move_art(lib, copy) def item_dir(self): """Returns the directory containing the album's first item, @@ -1743,7 +1701,7 @@ class Album(BaseAlbum): filename_tmpl = Template(beets.config['art_filename'].get(unicode)) subpath = format_for_path(self.evaluate_template(filename_tmpl)) subpath = util.sanitize_path(subpath, - replacements=self._library.replacements) + replacements=self._lib.replacements) subpath = bytestring_path(subpath) _, ext = os.path.splitext(image) @@ -1783,8 +1741,8 @@ class Album(BaseAlbum): """ # Get template field values. mapping = {} - for key in ALBUM_KEYS: - mapping[key] = format_for_path(getattr(self, key), key) + for key, value in dict(self).items(): + mapping[key] = format_for_path(value, key) mapping['artpath'] = displayable_path(mapping['artpath']) mapping['path'] = displayable_path(self.item_dir()) @@ -1800,6 +1758,14 @@ class Album(BaseAlbum): # Perform substitution. return template.substitute(mapping, funcs) + def load(self): + """Refresh this album's cached metadata from the library. + """ + items = self._lib.items(artist=self.artist, album=self.album) + item = iter(items).next() + for key in ALBUM_KEYS_ITEM: + self[key] = item[key] + # Default path template resources. diff --git a/beets/ui/commands.py b/beets/ui/commands.py index 6f17ad5cb..6fbf0ae2a 100644 --- a/beets/ui/commands.py +++ b/beets/ui/commands.py @@ -1162,8 +1162,9 @@ def modify_items(lib, mods, query, write, move, album, confirm): else: lib.move(obj) - # When modifying items, we have to store them to the database. - if not album: + if album: + obj.store() + else: lib.store(obj) # Apply tags if requested. diff --git a/beetsplug/inline.py b/beetsplug/inline.py index 40537caca..dc42bd29f 100644 --- a/beetsplug/inline.py +++ b/beetsplug/inline.py @@ -70,12 +70,10 @@ def compile_inline(python_code, album): is_expr = True def _dict_for(obj): + out = dict(obj) if album: - out = dict(obj._record) out['items'] = list(obj.items()) - return out - else: - return dict(obj) + return out if is_expr: # For expressions, just evaluate and return the result. From ec10f8c2234ad7b5b3625c2554d8d4c552a93ff2 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Tue, 20 Aug 2013 14:28:28 -0700 Subject: [PATCH 16/32] remove count() calls I removed this method in an earlier commit. --- beetsplug/bpd/__init__.py | 18 ++++++++++-------- test/test_query.py | 19 ------------------- 2 files changed, 10 insertions(+), 27 deletions(-) diff --git a/beetsplug/bpd/__init__.py b/beetsplug/bpd/__init__.py index 6d7940554..ef00c1c31 100644 --- a/beetsplug/bpd/__init__.py +++ b/beetsplug/bpd/__init__.py @@ -29,7 +29,6 @@ import beets from beets.plugins import BeetsPlugin import beets.ui from beets import vfs -from beets import config from beets.util import bluelet from beets.library import ITEM_KEYS_WRITABLE @@ -931,12 +930,12 @@ class Server(BaseServer): def cmd_stats(self, conn): """Sends some statistics about the library.""" with self.lib.transaction() as tx: - songs, totaltime = beets.library.TrueQuery().count(tx) - statement = 'SELECT COUNT(DISTINCT artist), ' \ - 'COUNT(DISTINCT album) FROM items' - result = tx.query(statement)[0] - artists, albums = result[0], result[1] + 'COUNT(DISTINCT album), ' \ + 'COUNT(id), ' \ + 'SUM(length) ' \ + 'FROM items' + artists, albums, songs, totaltime = tx.query(statement)[0] yield (u'artists: ' + unicode(artists), u'albums: ' + unicode(albums), @@ -1046,8 +1045,11 @@ class Server(BaseServer): tag/value query. """ _, key = self._tagtype_lookup(tag) - query = beets.library.MatchQuery(key, value) - songs, playtime = query.count(self.lib) + songs = 0 + playtime = 0.0 + for item in self.lib.items(beets.library.MatchQuery(key, value)): + songs += 1 + playtime += item.length yield u'songs: ' + unicode(songs) yield u'playtime: ' + unicode(int(playtime)) diff --git a/test/test_query.py b/test/test_query.py index be2e88d93..4695d98fb 100644 --- a/test/test_query.py +++ b/test/test_query.py @@ -461,25 +461,6 @@ class BrowseTest(unittest.TestCase, AssertsMixin): items = self.lib.items('2007') self.assert_done(items) -class CountTest(unittest.TestCase): - def setUp(self): - self.lib = beets.library.Library(':memory:') - self.item = some_item - self.lib.add(self.item) - - def test_count_gets_single_item(self): - with self.lib.transaction() as tx: - songs, totaltime = beets.library.TrueQuery().count(tx) - self.assertEqual(songs, 1) - self.assertEqual(totaltime, self.item.length) - - def test_count_works_for_empty_library(self): - self.lib.remove(self.item) - with self.lib.transaction() as tx: - songs, totaltime = beets.library.TrueQuery().count(tx) - self.assertEqual(songs, 0) - self.assertEqual(totaltime, 0.0) - class StringParseTest(unittest.TestCase): def test_single_field_query(self): q = beets.library.AndQuery.from_string(u'albumtype:soundtrack') From 50f724186f3fd75dd6506cb6a976f5303081541e Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Tue, 20 Aug 2013 14:54:24 -0700 Subject: [PATCH 17/32] revamp ResultIterator (now Results) This new design lets us provide better common functionality. The `get()` method is a particularly good example: we can now avoid the try/except dance that was previously necessary to get the only result for a single-result query. This also lets us continue using `len()` on the result of `album.items()`, which previously returned a materialized list. --- beets/library.py | 110 +++++++++++++++++++++++++++++------------------ 1 file changed, 69 insertions(+), 41 deletions(-) diff --git a/beets/library.py b/beets/library.py index 053a1f3a2..dadee063a 100644 --- a/beets/library.py +++ b/beets/library.py @@ -887,22 +887,29 @@ class PathQuery(Query): file_blob = buffer(self.file_path) return '(path = ?) || (path LIKE ?)', (file_blob, dir_pat) -class ResultIterator(object): - """An iterator into an item query result set. The iterator lazily +class Results(object): + """An item query result set. Iterating over the collection lazily constructs LibModel objects that reflect database rows. """ def __init__(self, model_class, rows, lib, query=None): + """Create a result set that will construct objects of type + `model_class`, which should be a subclass of `LibModel`, out of + the query result mapping in `rows`. The new objects are + associated with the library `lib`. If `query` is provided, it is + used as a predicate to filter the results for a "slow query" that + cannot be evaluated by the database directly. + """ self.model_class = model_class self.rows = rows - self.rowiter = iter(self.rows) self.lib = lib self.query = query def __iter__(self): - return self - - def next(self): - for row in self.rowiter: # Iterate until we get a hit. + """Construct Python objects for all rows that pass the query + predicate. + """ + for row in self.rows: + # Get the flexible attributes for the object. with self.lib.transaction() as tx: flex_rows = tx.query( 'SELECT * FROM {0} WHERE entity_id=?'.format( @@ -912,11 +919,53 @@ class ResultIterator(object): ) values = dict(row) values.update({row['key']: row['value'] for row in flex_rows}) + + # Construct the Python object and yield it if it passes the + # predicate. obj = self.model_class(self.lib, **values) - if self.query and not self.query.match(obj): - continue - return obj - raise StopIteration() # Reached the end of the DB rows. + if not self.query or self.query.match(obj): + yield obj + + def __len__(self): + """Get the number of matching objects. + """ + if self.query: + # A slow query. Fall back to testing every object. + count = 0 + for obj in self: + count += 1 + return count + + else: + # A fast query. Just count the rows. + return len(self.rows) + + def __nonzero__(self): + """Does this result contain any objects? + """ + return bool(len(self)) + + def __getitem__(self, n): + """Get the nth item in this result set. This is inefficient: all + items up to n are materialized and thrown away. + """ + it = iter(self) + try: + for i in range(n): + it.next() + return it.next() + except StopIteration: + raise IndexError('result index {0} out of range'.format(n)) + + def get(self): + """Return the first matching object, or None if no objects + match. + """ + it = iter(self) + try: + return it.next() + except StopIteration: + return None # Regular expression for parse_query_part, below. PARSE_QUERY_PART_REGEX = re.compile( @@ -1412,13 +1461,9 @@ class Library(object): with self.transaction() as tx: tx.mutate('DELETE FROM items WHERE id=?', (item.id,)) - if album: - item_iter = album.items() - try: - item_iter.next() - except StopIteration: - # Album is empty. - album.remove(delete, False) + # Remove the album if it is empty. + if album and not album.items(): + album.remove(delete, False) if delete: util.remove(item.path) @@ -1493,7 +1538,7 @@ class Library(object): subvals, ) - return ResultIterator(Album, rows, self, None if where else query) + return Results(Album, rows, self, None if where else query) def items(self, query=None, artist=None, album=None, title=None): """Returns a sequence of the items matching the given artist, @@ -1522,7 +1567,7 @@ class Library(object): subvals ) - return ResultIterator(Item, rows, self, None if where else query) + return Results(Item, rows, self, None if where else query) # Convenience accessors. @@ -1530,11 +1575,7 @@ class Library(object): def get_item(self, id): """Fetch an Item by its ID. Returns None if no match is found. """ - items = self.items(MatchQuery('id', id)) - try: - return items.next() - except StopIteration: - return None + return self.items(MatchQuery('id', id)).get() def get_album(self, item_or_id): """Given an album ID or an item associated with an album, @@ -1548,11 +1589,7 @@ class Library(object): if album_id is None: return None - albums = self.albums(MatchQuery('id', album_id)) - try: - return albums.next() - except StopIteration: - return None + return self.albums(MatchQuery('id', album_id)).get() def add_album(self, items): """Create a new album in the database with metadata derived @@ -1680,9 +1717,8 @@ class Album(LibModel): """Returns the directory containing the album's first item, provided that such an item exists. """ - try: - item = self.items().next() - except StopIteration: + item = self.items().get() + if not item: raise ValueError('empty album') return os.path.dirname(item.path) @@ -1758,14 +1794,6 @@ class Album(LibModel): # Perform substitution. return template.substitute(mapping, funcs) - def load(self): - """Refresh this album's cached metadata from the library. - """ - items = self._lib.items(artist=self.artist, album=self.album) - item = iter(items).next() - for key in ALBUM_KEYS_ITEM: - self[key] = item[key] - # Default path template resources. From 4d20d3b2961dd30be59fde94b8c0ab5a50113267 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Tue, 20 Aug 2013 15:32:27 -0700 Subject: [PATCH 18/32] album.store() now also affects tracks --- beets/library.py | 24 +++++++++++++++-- test/_common.py | 58 ++++++++++++++++++++--------------------- test/rsrc/test.blb | Bin 7168 -> 12288 bytes test/test_autotag.py | 60 +++++++++++++++++++++---------------------- test/test_db.py | 30 +++++++++------------- 5 files changed, 92 insertions(+), 80 deletions(-) diff --git a/beets/library.py b/beets/library.py index dadee063a..d9bd918d7 100644 --- a/beets/library.py +++ b/beets/library.py @@ -363,7 +363,7 @@ class LibModel(FlexModel): for key in self._fields: if (key != 'id') and (key in self._dirty or store_all): assignments += key + '=?,' - value = getattr(item, key) + value = self[key] # Wrap path strings in buffers so they get stored # "in the raw". if key == 'path' and isinstance(value, str): @@ -1407,6 +1407,8 @@ class Library(object): """ if load_id is None: load_id = item.id + if load_id is None: + raise ValueError('cannot load item with no id') stored_item = self.get_item(load_id) item.update(dict(stored_item)) item.clear_dirty() @@ -1630,7 +1632,7 @@ class Album(LibModel): art. """ _fields = ALBUM_KEYS - _table = 'album' + _table = 'albums' _flex_table = 'album_attributes' def __setitem__(self, key, value): @@ -1794,6 +1796,24 @@ class Album(LibModel): # Perform substitution. return template.substitute(mapping, funcs) + def store(self): + """Update the database with the album information. The album's + tracks are also updated. + """ + # Get modified track fields. + track_updates = {} + for key in ALBUM_KEYS_ITEM: + if key in self._dirty: + track_updates[key] = self[key] + + with self._lib.transaction(): + super(Album, self).store() + if track_updates: + for item in self.items(): + for key, value in track_updates.items(): + item[key] = value + item.store() + # Default path template resources. diff --git a/test/_common.py b/test/_common.py index 864813766..f4b4fe37c 100644 --- a/test/_common.py +++ b/test/_common.py @@ -45,35 +45,35 @@ _item_ident = 0 def item(): global _item_ident _item_ident += 1 - return beets.library.Item({ - 'title': u'the title', - 'artist': u'the artist', - 'albumartist': u'the album artist', - 'album': u'the album', - 'genre': u'the genre', - 'composer': u'the composer', - 'grouping': u'the grouping', - 'year': 1, - 'month': 2, - 'day': 3, - 'track': 4, - 'tracktotal': 5, - 'disc': 6, - 'disctotal': 7, - 'lyrics': u'the lyrics', - 'comments': u'the comments', - 'bpm': 8, - 'comp': True, - 'path': 'somepath' + str(_item_ident), - 'length': 60.0, - 'bitrate': 128000, - 'format': 'FLAC', - 'mb_trackid': 'someID-1', - 'mb_albumid': 'someID-2', - 'mb_artistid': 'someID-3', - 'mb_albumartistid': 'someID-4', - 'album_id': None, - }) + return beets.library.Item( + title = u'the title', + artist = u'the artist', + albumartist = u'the album artist', + album = u'the album', + genre = u'the genre', + composer = u'the composer', + grouping = u'the grouping', + year = 1, + month = 2, + day = 3, + track = 4, + tracktotal = 5, + disc = 6, + disctotal = 7, + lyrics = u'the lyrics', + comments = u'the comments', + bpm = 8, + comp = True, + path = 'somepath' + str(_item_ident), + length = 60.0, + bitrate = 128000, + format = 'FLAC', + mb_trackid = 'someID-1', + mb_albumid = 'someID-2', + mb_artistid = 'someID-3', + mb_albumartistid = 'someID-4', + album_id = None, + ) # Dummy import session. def import_session(lib=None, logfile=None, paths=[], query=[], cli=False): diff --git a/test/rsrc/test.blb b/test/rsrc/test.blb index 22c621da507ed223fa32f540116c9d7850968444..a2e441c5f0bc511e0923ccb6760eb71b5a362d54 100644 GIT binary patch delta 1528 zcmZp$Xh@hKE$Ga^z`z5>Z z1$dZ)84D%{vbokbNinmFYilz$XO|=<<)mhoq~^vamXs7_CY6??7Nc?5oP%5)LtGU? z9G!ez70@IVG`JM7fXoyHPrndXch?|=fFMs_$Dl|BZ`Vj2sM>l|9jSRGnI)C+2t6=$ zsLHZaD-}XqBSNstmL=wtrs9zg_45o2b=3gdRS&XEM*(P{rh>npg0sJ$n~$e+h(eHS zfRCfItEMdrn|Po$BSU6hN@_)MVGhvG@rk7+`CxuLS_s4&q6x4A1DvJBi-}!aQj)PF z9~{DoIZ36t#SoI&dGiH91Exs=ESrTS-Z4)M;N#!|#TW|%!)6YNAIwZ#44VZcK7b@d zc$pL!*chxBm{gb@nHw1PGu~&g0>+?PeIpYGySSk)W1}(HQDBpSoFQtIOD&i|{4F2@wAx@wre^5~j&m-GE3I zNJ*(4o|aJ42)&Y0Jv=QDNJ{qXY~sPXxRMfj5X2jzi*SPy24;4xr)z%23}ZhsJ%jEebygsck@+`}WMBnR8%vs)S&Q?FN)$H# jW)|StEX1*bar0(DU#881BJY_eC$LE{GH&LO_`wVS8iN$1 diff --git a/test/test_autotag.py b/test/test_autotag.py index f0b637eb6..20fe20841 100644 --- a/test/test_autotag.py +++ b/test/test_autotag.py @@ -53,30 +53,30 @@ class PluralityTest(_common.TestCase): plurality([]) def test_current_metadata_finds_pluralities(self): - items = [Item({'artist': 'The Beetles', 'album': 'The White Album'}), - Item({'artist': 'The Beatles', 'album': 'The White Album'}), - Item({'artist': 'The Beatles', 'album': 'Teh White Album'})] + items = [Item(artist='The Beetles', album='The White Album'), + Item(artist='The Beatles', album='The White Album'), + Item(artist='The Beatles', album='Teh White Album')] likelies, consensus = match.current_metadata(items) self.assertEqual(likelies['artist'], 'The Beatles') self.assertEqual(likelies['album'], 'The White Album') self.assertFalse(consensus['artist']) def test_current_metadata_artist_consensus(self): - items = [Item({'artist': 'The Beatles', 'album': 'The White Album'}), - Item({'artist': 'The Beatles', 'album': 'The White Album'}), - Item({'artist': 'The Beatles', 'album': 'Teh White Album'})] + items = [Item(artist='The Beatles', album='The White Album'), + Item(artist='The Beatles', album='The White Album'), + Item(artist='The Beatles', album='Teh White Album')] likelies, consensus = match.current_metadata(items) self.assertEqual(likelies['artist'], 'The Beatles') self.assertEqual(likelies['album'], 'The White Album') self.assertTrue(consensus['artist']) def test_albumartist_consensus(self): - items = [Item({'artist': 'tartist1', 'album': 'album', - 'albumartist': 'aartist'}), - Item({'artist': 'tartist2', 'album': 'album', - 'albumartist': 'aartist'}), - Item({'artist': 'tartist3', 'album': 'album', - 'albumartist': 'aartist'})] + items = [Item(artist='tartist1', album='album', + albumartist='aartist'), + Item(artist='tartist2', album='album', + albumartist='aartist'), + Item(artist='tartist3', album='album', + albumartist='aartist')] likelies, consensus = match.current_metadata(items) self.assertEqual(likelies['artist'], 'aartist') self.assertFalse(consensus['artist']) @@ -85,19 +85,17 @@ class PluralityTest(_common.TestCase): fields = ['artist', 'album', 'albumartist', 'year', 'disctotal', 'mb_albumid', 'label', 'catalognum', 'country', 'media', 'albumdisambig'] - items = [Item(dict((f, '%s_%s' % (f, i or 1)) for f in fields)) + items = [Item(**dict((f, '%s_%s' % (f, i or 1)) for f in fields)) for i in range(5)] likelies, _ = match.current_metadata(items) for f in fields: self.assertEqual(likelies[f], '%s_1' % f) def _make_item(title, track, artist=u'some artist'): - return Item({ - 'title': title, 'track': track, - 'artist': artist, 'album': u'some album', - 'length': 1, - 'mb_trackid': '', 'mb_albumid': '', 'mb_artistid': '', - }) + return Item(title=title, track=track, + artist=artist, album=u'some album', + length=1, + mb_trackid='', mb_albumid='', mb_artistid='') def _make_trackinfo(): return [ @@ -560,10 +558,10 @@ class MultiDiscAlbumsInDirTest(_common.TestCase): class AssignmentTest(unittest.TestCase): def item(self, title, track): - return Item({ - 'title': title, 'track': track, - 'mb_trackid': '', 'mb_albumid': '', 'mb_artistid': '', - }) + return Item( + title=title, track=track, + mb_trackid='', mb_albumid='', mb_artistid='', + ) def test_reorder_when_track_numbers_incorrect(self): items = [] @@ -640,14 +638,14 @@ class AssignmentTest(unittest.TestCase): def test_order_works_when_track_names_are_entirely_wrong(self): # A real-world test case contributed by a user. def item(i, length): - return Item({ - 'artist': u'ben harper', - 'album': u'burn to shine', - 'title': u'ben harper - Burn to Shine ' + str(i), - 'track': i, - 'length': length, - 'mb_trackid': '', 'mb_albumid': '', 'mb_artistid': '', - }) + return Item( + artist=u'ben harper', + album=u'burn to shine', + title=u'ben harper - Burn to Shine ' + str(i), + track=i, + length=length, + mb_trackid='', mb_albumid='', mb_artistid='', + ) items = [] items.append(item(1, 241.37243007106997)) items.append(item(2, 342.27781704375036)) diff --git a/test/test_db.py b/test/test_db.py index 285a136be..d01798c42 100644 --- a/test/test_db.py +++ b/test/test_db.py @@ -41,7 +41,7 @@ def remove_lib(): os.unlink(TEMP_LIB) def boracay(l): return beets.library.Item( - l._connection().execute('select * from items where id=3').fetchone() + **l._connection().execute('select * from items where id=3').fetchone() ) np = util.normpath @@ -62,7 +62,7 @@ class LoadTest(unittest.TestCase): def test_load_clears_dirty_flags(self): self.i.artist = 'something' self.lib.load(self.i) - self.assertTrue(not self.i.dirty['artist']) + self.assertTrue('artist' not in self.i._dirty) class StoreTest(unittest.TestCase): def setUp(self): @@ -82,7 +82,7 @@ class StoreTest(unittest.TestCase): def test_store_only_writes_dirty_fields(self): original_genre = self.i.genre - self.i.record['genre'] = 'beatboxing' # change value w/o dirtying + self.i._values_fixed['genre'] = 'beatboxing' # change w/o dirtying self.lib.store(self.i) new_genre = self.lib._connection().execute( 'select genre from items where ' @@ -92,7 +92,7 @@ class StoreTest(unittest.TestCase): def test_store_clears_dirty_flags(self): self.i.composer = 'tvp' self.lib.store(self.i) - self.assertTrue(not self.i.dirty['composer']) + self.assertTrue('composer' not in self.i._dirty) class AddTest(unittest.TestCase): def setUp(self): @@ -139,11 +139,11 @@ class GetSetTest(unittest.TestCase): def test_set_sets_dirty_flag(self): self.i.comp = not self.i.comp - self.assertTrue(self.i.dirty['comp']) + self.assertTrue('comp' in self.i._dirty) def test_set_does_not_dirty_if_value_unchanged(self): self.i.title = self.i.title - self.assertTrue(not self.i.dirty['title']) + self.assertTrue('title' not in self.i._dirty) def test_invalid_field_raises_attributeerror(self): self.assertRaises(AttributeError, getattr, self.i, 'xyzzy') @@ -795,20 +795,23 @@ class AlbumInfoTest(unittest.TestCase): def test_albuminfo_changes_affect_items(self): ai = self.lib.get_album(self.i) ai.album = 'myNewAlbum' - i = self.lib.items().next() + ai.store() + i = self.lib.items()[0] self.assertEqual(i.album, 'myNewAlbum') def test_albuminfo_change_albumartist_changes_items(self): ai = self.lib.get_album(self.i) ai.albumartist = 'myNewArtist' - i = self.lib.items().next() + ai.store() + i = self.lib.items()[0] self.assertEqual(i.albumartist, 'myNewArtist') self.assertNotEqual(i.artist, 'myNewArtist') def test_albuminfo_change_artist_does_not_change_items(self): ai = self.lib.get_album(self.i) ai.artist = 'myNewArtist' - i = self.lib.items().next() + ai.store() + i = self.lib.items()[0] self.assertNotEqual(i.artist, 'myNewArtist') def test_albuminfo_remove_removes_items(self): @@ -824,15 +827,6 @@ class AlbumInfoTest(unittest.TestCase): self.lib.remove(self.i) self.assertEqual(len(self.lib.albums()), 0) -class BaseAlbumTest(_common.TestCase): - def test_field_access(self): - album = beets.library.BaseAlbum(None, {'fld1':'foo'}) - self.assertEqual(album.fld1, 'foo') - - def test_field_access_unset_values(self): - album = beets.library.BaseAlbum(None, {}) - self.assertRaises(AttributeError, getattr, album, 'field') - class ArtDestinationTest(_common.TestCase): def setUp(self): super(ArtDestinationTest, self).setUp() From 238e743b5ebabf9e8f1ddacffe1e1cc797d21c5b Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Tue, 20 Aug 2013 15:47:28 -0700 Subject: [PATCH 19/32] add load() method to LibModel --- beets/library.py | 74 ++++++++++++++++++++++++++++-------------------- 1 file changed, 44 insertions(+), 30 deletions(-) diff --git a/beets/library.py b/beets/library.py index d9bd918d7..91034df90 100644 --- a/beets/library.py +++ b/beets/library.py @@ -348,20 +348,26 @@ class LibModel(FlexModel): self._lib = lib super(LibModel, self).__init__(**values) - def store(self, store_id=None, store_all=False): - """Save the object's metadata into the library database. If - store_id is specified, use it instead of the current id. If - store_all is true, save the entire record instead of just the - dirty fields. + def _check_db(self): + """Ensure that this object is associated with a database row: it + has a reference to a library (`_lib`) and an id. A ValueError + exception is raised otherwise. """ - if store_id is None: - store_id = self.id + if not self._lib: + raise ValueError('{0} has no library'.format(type(self).__name__)) + if not self._id: + raise ValueError('{0} has no id'.format(type(self).__name__)) + + def store(self): + """Save the object's metadata into the library database. + """ + self._check_db() # Build assignments for query. assignments = '' subvars = [] for key in self._fields: - if (key != 'id') and (key in self._dirty or store_all): + if key != 'id' and key in self._dirty: assignments += key + '=?,' value = self[key] # Wrap path strings in buffers so they get stored @@ -377,7 +383,7 @@ class LibModel(FlexModel): query = 'UPDATE {0} SET {1} WHERE id=?'.format( self._table, assignments ) - subvars.append(store_id) + subvars.append(self.id) tx.mutate(query, subvars) # Flexible attributes. @@ -386,11 +392,28 @@ class LibModel(FlexModel): 'INSERT INTO {0} ' '(entity_id, key, value) ' 'VALUES (?, ?, ?);'.format(self._flex_table), - (store_id, key, value), + (self.id, key, value), ) self.clear_dirty() + def load(self): + """Refresh the object's metadata from the library database. + """ + self._check_db() + + # Get a fresh copy of this object from the DB. + with self._lib.transaction() as tx: + rows = tx.query( + 'SELECT * FROM {0} WHERE id=?;'.format(self._table), + (self.id,) + ) + results = Results(type(self), rows, self._lib) + stored_obj = results.get() + + self.update(dict(stored_obj)) + self.clear_dirty() + class Item(LibModel): _fields = ITEM_KEYS _table = 'items' @@ -1401,32 +1424,23 @@ class Library(object): self._memotable = {} return new_id - def load(self, item, load_id=None): - """Refresh the item's metadata from the library database. If - fetch_id is not specified, use the item's current id. + def load(self, item): + """Refresh the item's metadata from the library database. """ - if load_id is None: - load_id = item.id - if load_id is None: - raise ValueError('cannot load item with no id') - stored_item = self.get_item(load_id) + if item.id is None: + raise ValueError('cannot load item with no id') + stored_item = self.get_item(item.id) item.update(dict(stored_item)) item.clear_dirty() - def store(self, item, store_id=None, store_all=False): - """Save the item's metadata into the library database. If - store_id is specified, use it instead of the item's current id. - If store_all is true, save the entire record instead of just - the dirty fields. + def store(self, item): + """Save the item's metadata into the library database. """ - if store_id is None: - store_id = item.id - # Build assignments for query. assignments = '' subvars = [] for key in ITEM_KEYS: - if (key != 'id') and (key in item._dirty or store_all): + if key != 'id' and key in item._dirty: assignments += key + '=?,' value = getattr(item, key) # Wrap path strings in buffers so they get stored @@ -1440,7 +1454,7 @@ class Library(object): # Main table update. if assignments: query = 'UPDATE items SET ' + assignments + ' WHERE id=?' - subvars.append(store_id) + subvars.append(item.id) tx.mutate(query, subvars) # Flexible attributes. @@ -1448,7 +1462,7 @@ class Library(object): ' (entity_id, key, value)' \ ' VALUES (?, ?, ?)' for key, value in item._values_flex.items(): - tx.mutate(flexins, (store_id, key, value)) + tx.mutate(flexins, (item.id, key, value)) item.clear_dirty() self._memotable = {} @@ -1713,7 +1727,7 @@ class Album(LibModel): self._lib.move(item, copy, basedir=basedir, with_album=False) # Move art. - self.move_art(lib, copy) + self.move_art(self._lib, copy) def item_dir(self): """Returns the directory containing the album's first item, From 8bdf2d0efec8c51a19af39698fef2ffa0d1a9225 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Tue, 20 Aug 2013 16:05:45 -0700 Subject: [PATCH 20/32] fix dirtying when unchanged, artpath wrapping Also some naming mistakes. --- beets/library.py | 22 ++++++++++++++-------- test/test_db.py | 12 ++++++++---- 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/beets/library.py b/beets/library.py index 91034df90..1b08cb571 100644 --- a/beets/library.py +++ b/beets/library.py @@ -281,11 +281,12 @@ class FlexModel(object): def __setitem__(self, key, value): """Assign the value for a field. """ - if key in self._fields: - self._values_fixed[key] = value - else: - self._values_flex[key] = value - self._dirty.add(key) + source = self._values_fixed if key in self._fields \ + else self._values_flex + old_value = source.get(key) + source[key] = value + if old_value != value: + self._dirty.add(key) def update(self, values): """Assign all values in the given dict. @@ -344,6 +345,11 @@ class LibModel(FlexModel): """The flex field SQLite table name. """ + _bytes_keys = ('path', 'artpath') + """Keys whose values should be stored as raw bytes blobs rather than + strings. + """ + def __init__(self, lib=None, **values): self._lib = lib super(LibModel, self).__init__(**values) @@ -355,7 +361,7 @@ class LibModel(FlexModel): """ if not self._lib: raise ValueError('{0} has no library'.format(type(self).__name__)) - if not self._id: + if not self.id: raise ValueError('{0} has no id'.format(type(self).__name__)) def store(self): @@ -372,7 +378,7 @@ class LibModel(FlexModel): value = self[key] # Wrap path strings in buffers so they get stored # "in the raw". - if key == 'path' and isinstance(value, str): + if key in self._bytes_keys and isinstance(value, str): value = buffer(value) subvars.append(value) assignments = assignments[:-1] # Knock off last , @@ -1727,7 +1733,7 @@ class Album(LibModel): self._lib.move(item, copy, basedir=basedir, with_album=False) # Move art. - self.move_art(self._lib, copy) + self.move_art(copy) def item_dir(self): """Returns the directory containing the album's first item, diff --git a/test/test_db.py b/test/test_db.py index d01798c42..744933049 100644 --- a/test/test_db.py +++ b/test/test_db.py @@ -533,21 +533,21 @@ class DisambiguationTest(unittest.TestCase, PathFormattingMixin): def test_unique_with_default_arguments_uses_albumtype(self): album2 = self.lib.get_album(self.i1) album2.albumtype = 'bar' - self.lib._connection().commit() + album2.store() self._setf(u'foo%aunique{}/$title') self._assert_dest('/base/foo [bar]/the title', self.i1) def test_unique_expands_to_nothing_for_distinct_albums(self): album2 = self.lib.get_album(self.i2) album2.album = 'different album' - self.lib._connection().commit() + album2.store() self._assert_dest('/base/foo/the title', self.i1) def test_use_fallback_numbers_when_identical(self): album2 = self.lib.get_album(self.i2) album2.year = 2001 - self.lib._connection().commit() + album2.store() self._assert_dest('/base/foo 1/the title', self.i1) self._assert_dest('/base/foo 2/the title', self.i2) @@ -561,6 +561,8 @@ class DisambiguationTest(unittest.TestCase, PathFormattingMixin): album2.year = 2001 album1 = self.lib.get_album(self.i1) album1.albumtype = 'foo/bar' + album2.store() + album1.store() self._setf(u'foo%aunique{albumartist album,albumtype}/$title') self._assert_dest('/base/foo [foo_bar]/the title', self.i1) @@ -757,6 +759,7 @@ class AlbumInfoTest(unittest.TestCase): def test_albuminfo_stores_art(self): ai = self.lib.get_album(self.i) ai.artpath = '/my/great/art' + ai.store() new_ai = self.lib.get_album(self.i) self.assertEqual(new_ai.artpath, '/my/great/art') @@ -906,9 +909,10 @@ class PathStringTest(_common.TestCase): self.assert_(isinstance(dest, str)) def test_artpath_stores_special_chars(self): - path = 'b\xe1r' + path = b'b\xe1r' alb = self.lib.add_album([self.i]) alb.artpath = path + alb.store() alb = self.lib.get_album(self.i) self.assertEqual(path, alb.artpath) From 8bcbe1dea211a7c4e78c7285b7a4846739bc28d7 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Tue, 20 Aug 2013 16:22:24 -0700 Subject: [PATCH 21/32] fix outdated SQL query --- beets/library.py | 2 +- test/test_files.py | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/beets/library.py b/beets/library.py index 1b08cb571..77a7051c3 100644 --- a/beets/library.py +++ b/beets/library.py @@ -1420,7 +1420,7 @@ class Library(object): # Flexible attributes. flexins = 'INSERT INTO item_attributes ' \ ' (entity_id, key, value)' \ - ' VALUES (?, ?, ?, ?)' + ' VALUES (?, ?, ?)' for key, value in item._values_flex.items(): if value is not None: tx.mutate(flexins, (new_id, key, value)) diff --git a/test/test_files.py b/test/test_files.py index a9aad0f95..7abbec94b 100644 --- a/test/test_files.py +++ b/test/test_files.py @@ -170,6 +170,7 @@ class AlbumFileTest(_common.TestCase): def test_albuminfo_move_changes_paths(self): self.ai.album = 'newAlbumName' self.ai.move() + self.ai.store() self.lib.load(self.i) self.assert_('newAlbumName' in self.i.path) @@ -178,6 +179,7 @@ class AlbumFileTest(_common.TestCase): oldpath = self.i.path self.ai.album = 'newAlbumName' self.ai.move() + self.ai.store() self.lib.load(self.i) self.assertFalse(os.path.exists(oldpath)) @@ -187,6 +189,7 @@ class AlbumFileTest(_common.TestCase): oldpath = self.i.path self.ai.album = 'newAlbumName' self.ai.move(True) + self.ai.store() self.lib.load(self.i) self.assertTrue(os.path.exists(oldpath)) @@ -195,6 +198,7 @@ class AlbumFileTest(_common.TestCase): def test_albuminfo_move_to_custom_dir(self): self.ai.move(basedir=self.otherdir) self.lib.load(self.i) + self.ai.store() self.assertTrue('testotherdir' in self.i.path) class ArtFileTest(_common.TestCase): @@ -216,6 +220,7 @@ class ArtFileTest(_common.TestCase): self.art = self.lib.get_album(self.i).art_destination('something.jpg') touch(self.art) self.ai.artpath = self.art + self.ai.store() # Alternate destination dir. self.otherdir = os.path.join(self.temp_dir, 'testotherdir') @@ -239,6 +244,7 @@ class ArtFileTest(_common.TestCase): def test_art_moves_with_album_to_custom_dir(self): # Move the album to another directory. self.ai.move(basedir=self.otherdir) + self.ai.store() self.lib.load(self.i) # Art should be in new directory. From c2acab510d180c7284442d2f88b51f9bc2f54eaf Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Wed, 21 Aug 2013 09:07:20 -0700 Subject: [PATCH 22/32] album.move() now also stores This lets items see any modifications to the album (when the album's fields are dirty). It's also symmetric with the same method on items. --- beets/library.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/beets/library.py b/beets/library.py index 77a7051c3..06d974361 100644 --- a/beets/library.py +++ b/beets/library.py @@ -1721,12 +1721,17 @@ class Album(LibModel): self._lib.directory) def move(self, copy=False, basedir=None): - """Moves (or copies) all items to their destination. Any album + """Moves (or copies) all items to their destination. Any album art moves along with them. basedir overrides the library base - directory for the destination. + directory for the destination. The album is stored to the + database, persisting any modifications to its metadata. """ basedir = basedir or self._lib.directory + # Ensure new metadata is available to items for destination + # computation. + self.store() + # Move items. items = list(self.items()) for item in items: From 9609e41cf88a4d570b714f9c6887db3b077a66d0 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Wed, 21 Aug 2013 09:25:47 -0700 Subject: [PATCH 23/32] store albums after moving art --- beets/library.py | 2 ++ test/test_files.py | 4 +++- test/test_importer.py | 35 ++++++++++++++++++----------------- 3 files changed, 23 insertions(+), 18 deletions(-) diff --git a/beets/library.py b/beets/library.py index 06d974361..89ae12d59 100644 --- a/beets/library.py +++ b/beets/library.py @@ -1530,6 +1530,7 @@ class Library(object): album = self.get_album(item) if album: album.move_art(copy) + album.store() # Prune vacated directory. if not copy: @@ -1739,6 +1740,7 @@ class Album(LibModel): # Move art. self.move_art(copy) + self.store() def item_dir(self): """Returns the directory containing the album's first item, diff --git a/test/test_files.py b/test/test_files.py index 7abbec94b..ff887637c 100644 --- a/test/test_files.py +++ b/test/test_files.py @@ -350,7 +350,8 @@ class ArtFileTest(_common.TestCase): self.assertExists(oldartpath) self.ai.album = 'different_album' - self.lib.move(self.i) + self.ai.store() + self.lib.move(self.ai.items()[0]) artpath = self.lib.albums()[0].artpath self.assertTrue('different_album' in artpath) @@ -427,6 +428,7 @@ class RemoveTest(_common.TestCase): artfile = os.path.join(self.temp_dir, 'testart.jpg') touch(artfile) self.ai.set_art(artfile) + self.ai.store() parent = os.path.dirname(self.i.path) self.lib.remove(self.i, True) diff --git a/test/test_importer.py b/test/test_importer.py index 0b39a3fb5..4d159b504 100644 --- a/test/test_importer.py +++ b/test/test_importer.py @@ -406,7 +406,7 @@ class ApplyExistingItemsTest(_common.TestCase): self._apply_asis([self.i]) # Get the item's path and import it again. - item = self.lib.items().next() + item = self.lib.items().get() new_item = library.Item.from_path(item.path) self._apply_asis([new_item]) @@ -416,7 +416,7 @@ class ApplyExistingItemsTest(_common.TestCase): def test_apply_existing_album_does_not_duplicate_album(self): # As above. self._apply_asis([self.i]) - item = self.lib.items().next() + item = self.lib.items().get() new_item = library.Item.from_path(item.path) self._apply_asis([new_item]) @@ -425,7 +425,7 @@ class ApplyExistingItemsTest(_common.TestCase): def test_apply_existing_singleton_does_not_duplicate_album(self): self._apply_asis([self.i]) - item = self.lib.items().next() + item = self.lib.items().get() new_item = library.Item.from_path(item.path) self._apply_asis([new_item], False) @@ -440,7 +440,7 @@ class ApplyExistingItemsTest(_common.TestCase): self._apply_asis([self.i]) # Import again with new metadata. - item = self.lib.items().next() + item = self.lib.items().get() new_item = library.Item.from_path(item.path) new_item.title = 'differentTitle' self._apply_asis([new_item]) @@ -454,12 +454,12 @@ class ApplyExistingItemsTest(_common.TestCase): config['import']['copy'] = True self._apply_asis([self.i]) - item = self.lib.items().next() + item = self.lib.items().get() new_item = library.Item.from_path(item.path) new_item.title = 'differentTitle' self._apply_asis([new_item]) - item = self.lib.items().next() + item = self.lib.items().get() self.assertTrue('differentTitle' in item.path) self.assertExists(item.path) @@ -468,12 +468,12 @@ class ApplyExistingItemsTest(_common.TestCase): config['import']['copy'] = False self._apply_asis([self.i]) - item = self.lib.items().next() + item = self.lib.items().get() new_item = library.Item.from_path(item.path) new_item.title = 'differentTitle' self._apply_asis([new_item]) - item = self.lib.items().next() + item = self.lib.items().get() self.assertFalse('differentTitle' in item.path) self.assertExists(item.path) @@ -481,13 +481,13 @@ class ApplyExistingItemsTest(_common.TestCase): config['import']['copy'] = True self._apply_asis([self.i]) - item = self.lib.items().next() + item = self.lib.items().get() oldpath = item.path new_item = library.Item.from_path(item.path) new_item.title = 'differentTitle' self._apply_asis([new_item]) - item = self.lib.items().next() + item = self.lib.items().get() self.assertNotExists(oldpath) def test_apply_existing_item_new_metadata_delete_enabled(self): @@ -497,13 +497,13 @@ class ApplyExistingItemsTest(_common.TestCase): config['import']['delete'] = True # ! self._apply_asis([self.i]) - item = self.lib.items().next() + item = self.lib.items().get() oldpath = item.path new_item = library.Item.from_path(item.path) new_item.title = 'differentTitle' self._apply_asis([new_item]) - item = self.lib.items().next() + item = self.lib.items().get() self.assertNotExists(oldpath) self.assertTrue('differentTitle' in item.path) self.assertExists(item.path) @@ -513,13 +513,13 @@ class ApplyExistingItemsTest(_common.TestCase): config['import']['copy'] = True self._apply_asis([self.i]) - item = self.lib.items().next() + item = self.lib.items().get() oldpath = item.path new_item = library.Item.from_path(item.path) self._apply_asis([new_item]) self.assertEqual(len(list(self.lib.items())), 1) - item = self.lib.items().next() + item = self.lib.items().get() self.assertEqual(oldpath, item.path) self.assertExists(oldpath) @@ -528,19 +528,19 @@ class ApplyExistingItemsTest(_common.TestCase): config['import']['delete'] = True # ! self._apply_asis([self.i]) - item = self.lib.items().next() + item = self.lib.items().get() new_item = library.Item.from_path(item.path) self._apply_asis([new_item]) self.assertEqual(len(list(self.lib.items())), 1) - item = self.lib.items().next() + item = self.lib.items().get() self.assertExists(item.path) def test_same_album_does_not_duplicate(self): # With the -L flag, exactly the same item (with the same ID) # is re-imported. This test simulates that situation. self._apply_asis([self.i]) - item = self.lib.items().next() + item = self.lib.items().get() self._apply_asis([item]) # Should not be duplicated. @@ -704,6 +704,7 @@ class DuplicateCheckTest(_common.TestCase): def test_duplicate_va_album(self): self.album.albumartist = 'an album artist' + self.album.store() res = importer._duplicate_check(self.lib, self._album_task(False, 'an album artist')) self.assertTrue(res) From 9e61e4945700311a348df84b3c06fcd5876caa9f Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Wed, 21 Aug 2013 09:42:16 -0700 Subject: [PATCH 24/32] fix NumericQuery constructor --- beets/library.py | 4 +- test/test_query.py | 172 +++++++++++++++++++++------------------------ 2 files changed, 81 insertions(+), 95 deletions(-) diff --git a/beets/library.py b/beets/library.py index 89ae12d59..64241d910 100644 --- a/beets/library.py +++ b/beets/library.py @@ -727,8 +727,8 @@ class NumericQuery(FieldQuery): except ValueError: return None - def __init__(self, field, pattern): - super(NumericQuery, self).__init__(field, pattern) + def __init__(self, field, pattern, fast=True): + super(NumericQuery, self).__init__(field, pattern, fast) self.numtype = self.kinds[field] parts = pattern.split('..', 1) diff --git a/test/test_query.py b/test/test_query.py index 4695d98fb..8fd8b6776 100644 --- a/test/test_query.py +++ b/test/test_query.py @@ -78,30 +78,30 @@ class AnyFieldQueryTest(unittest.TestCase): def test_no_restriction(self): q = beets.library.AnyFieldQuery('title', beets.library.ITEM_KEYS, beets.library.SubstringQuery) - self.assertEqual(self.lib.items(q).next().title, 'the title') + self.assertEqual(self.lib.items(q).get().title, 'the title') def test_restriction_completeness(self): q = beets.library.AnyFieldQuery('title', ['title'], beets.library.SubstringQuery) - self.assertEqual(self.lib.items(q).next().title, 'the title') + self.assertEqual(self.lib.items(q).get().title, 'the title') def test_restriction_soundness(self): q = beets.library.AnyFieldQuery('title', ['artist'], beets.library.SubstringQuery) - self.assertRaises(StopIteration, self.lib.items(q).next) + self.assertEqual(self.lib.items(q).get(), None) # Convenient asserts for matching items. class AssertsMixin(object): - def assert_matched(self, result_iterator, title): - self.assertEqual(result_iterator.next().title, title) - def assert_done(self, result_iterator): - self.assertRaises(StopIteration, result_iterator.next) - def assert_matched_all(self, result_iterator): - self.assert_matched(result_iterator, 'Littlest Things') - self.assert_matched(result_iterator, 'Take Pills') - self.assert_matched(result_iterator, 'Lovers Who Uncover') - self.assert_matched(result_iterator, 'Boracay') - self.assert_done(result_iterator) + def assert_matched(self, results, titles): + self.assertEqual([i.title for i in results], titles) + + def assert_matched_all(self, results): + self.assert_matched(results, [ + 'Littlest Things', + 'Take Pills', + 'Lovers Who Uncover', + 'Boracay', + ]) class GetTest(unittest.TestCase, AssertsMixin): def setUp(self): @@ -122,31 +122,27 @@ class GetTest(unittest.TestCase, AssertsMixin): def test_get_one_keyed_term(self): q = 'artist:Lil' results = self.lib.items(q) - self.assert_matched(results, 'Littlest Things') - self.assert_done(results) + self.assert_matched(results, ['Littlest Things']) def test_get_one_keyed_regexp(self): q = r'artist::L.+y' results = self.lib.items(q) - self.assert_matched(results, 'Littlest Things') - self.assert_done(results) + self.assert_matched(results, ['Littlest Things']) def test_get_one_unkeyed_term(self): q = 'Terry' results = self.lib.items(q) - self.assert_matched(results, 'Boracay') - self.assert_done(results) + self.assert_matched(results, ['Boracay']) def test_get_one_unkeyed_regexp(self): q = r':y$' results = self.lib.items(q) - self.assert_matched(results, 'Boracay') - self.assert_done(results) + self.assert_matched(results, ['Boracay']) def test_get_no_matches(self): q = 'popebear' results = self.lib.items(q) - self.assert_done(results) + self.assert_matched(results, []) def test_invalid_key(self): q = 'pope:bear' @@ -156,93 +152,94 @@ class GetTest(unittest.TestCase, AssertsMixin): def test_term_case_insensitive(self): q = 'UNCoVER' results = self.lib.items(q) - self.assert_matched(results, 'Lovers Who Uncover') - self.assert_done(results) + self.assert_matched(results, ['Lovers Who Uncover']) def test_regexp_case_sensitive(self): q = r':UNCoVER' results = self.lib.items(q) - self.assert_done(results) + self.assert_matched(results, []) q = r':Uncover' results = self.lib.items(q) - self.assert_matched(results, 'Lovers Who Uncover') - self.assert_done(results) + self.assert_matched(results, ['Lovers Who Uncover']) def test_term_case_insensitive_with_key(self): q = 'album:stiLL' results = self.lib.items(q) - self.assert_matched(results, 'Littlest Things') - self.assert_done(results) + self.assert_matched(results, ['Littlest Things']) def test_key_case_insensitive(self): q = 'ArTiST:Allen' results = self.lib.items(q) - self.assert_matched(results, 'Littlest Things') - self.assert_done(results) + self.assert_matched(results, ['Littlest Things']) def test_unkeyed_term_matches_multiple_columns(self): q = 'little' results = self.lib.items(q) - self.assert_matched(results, 'Littlest Things') - self.assert_matched(results, 'Lovers Who Uncover') - self.assert_matched(results, 'Boracay') - self.assert_done(results) + self.assert_matched(results, [ + 'Littlest Things', + 'Lovers Who Uncover', + 'Boracay', + ]) def test_unkeyed_regexp_matches_multiple_columns(self): q = r':^T' results = self.lib.items(q) - self.assert_matched(results, 'Take Pills') - self.assert_matched(results, 'Lovers Who Uncover') - self.assert_matched(results, 'Boracay') - self.assert_done(results) + self.assert_matched(results, [ + 'Take Pills', + 'Lovers Who Uncover', + 'Boracay', + ]) def test_keyed_term_matches_only_one_column(self): q = 'artist:little' results = self.lib.items(q) - self.assert_matched(results, 'Lovers Who Uncover') - self.assert_matched(results, 'Boracay') - self.assert_done(results) + self.assert_matched(results, [ + 'Lovers Who Uncover', + 'Boracay', + ]) def test_keyed_regexp_matches_only_one_column(self): q = r'album::\sS' results = self.lib.items(q) - self.assert_matched(results, 'Littlest Things') - self.assert_matched(results, 'Lovers Who Uncover') - self.assert_done(results) + self.assert_matched(results, [ + 'Littlest Things', + 'Lovers Who Uncover', + ]) def test_multiple_terms_narrow_search(self): q = 'little ones' results = self.lib.items(q) - self.assert_matched(results, 'Lovers Who Uncover') - self.assert_matched(results, 'Boracay') - self.assert_done(results) + self.assert_matched(results, [ + 'Lovers Who Uncover', + 'Boracay', + ]) def test_multiple_regexps_narrow_search(self): q = r':\sS :^T' results = self.lib.items(q) - self.assert_matched(results, 'Lovers Who Uncover') - self.assert_done(results) + self.assert_matched(results, ['Lovers Who Uncover']) def test_mixed_terms_regexps_narrow_search(self): q = r':\sS lily' results = self.lib.items(q) - self.assert_matched(results, 'Littlest Things') - self.assert_done(results) + self.assert_matched(results, ['Littlest Things']) def test_single_year(self): q = 'year:2006' results = self.lib.items(q) - self.assert_matched(results, 'Littlest Things') - self.assert_matched(results, 'Lovers Who Uncover') - self.assert_done(results) + self.assert_matched(results, [ + 'Littlest Things', + 'Lovers Who Uncover', + ]) def test_year_range(self): q = 'year:2000..2010' results = self.lib.items(q) - self.assert_matched(results, 'Littlest Things') - self.assert_matched(results, 'Take Pills') - self.assert_matched(results, 'Lovers Who Uncover') - self.assert_done(results) + self.assert_matched(results, [ + 'Littlest Things', + 'Take Pills', + 'Lovers Who Uncover', + ]) def test_bad_year(self): q = 'year:delete from items' @@ -263,26 +260,22 @@ class MemoryGetTest(unittest.TestCase, AssertsMixin): def test_singleton_true(self): q = 'singleton:true' results = self.lib.items(q) - self.assert_matched(results, 'singleton item') - self.assert_done(results) + self.assert_matched(results, ['singleton item']) def test_singleton_false(self): q = 'singleton:false' results = self.lib.items(q) - self.assert_matched(results, 'album item') - self.assert_done(results) + self.assert_matched(results, ['album item']) def test_compilation_true(self): q = 'comp:true' results = self.lib.items(q) - self.assert_matched(results, 'album item') - self.assert_done(results) + self.assert_matched(results, ['album item']) def test_compilation_false(self): q = 'comp:false' results = self.lib.items(q) - self.assert_matched(results, 'singleton item') - self.assert_done(results) + self.assert_matched(results, ['singleton item']) def test_unknown_field_name_ignored(self): q = 'xyzzy:nonsense' @@ -310,8 +303,7 @@ class MemoryGetTest(unittest.TestCase, AssertsMixin): q = u'title:caf\xe9' results = self.lib.items(q) - self.assert_matched(results, u'caf\xe9') - self.assert_done(results) + self.assert_matched(results, [u'caf\xe9']) class MatchTest(unittest.TestCase): def setUp(self): @@ -369,58 +361,52 @@ class PathQueryTest(unittest.TestCase, AssertsMixin): def test_path_exact_match(self): q = 'path:/a/b/c.mp3' results = self.lib.items(q) - self.assert_matched(results, 'path item') - self.assert_done(results) + self.assert_matched(results, ['path item']) def test_parent_directory_no_slash(self): q = 'path:/a' results = self.lib.items(q) - self.assert_matched(results, 'path item') - self.assert_done(results) + self.assert_matched(results, ['path item']) def test_parent_directory_with_slash(self): q = 'path:/a/' results = self.lib.items(q) - self.assert_matched(results, 'path item') - self.assert_done(results) + self.assert_matched(results, ['path item']) def test_no_match(self): q = 'path:/xyzzy/' results = self.lib.items(q) - self.assert_done(results) + self.assert_matched(results, []) def test_fragment_no_match(self): q = 'path:/b/' results = self.lib.items(q) - self.assert_done(results) + self.assert_matched(results, []) def test_nonnorm_path(self): q = 'path:/x/../a/b' results = self.lib.items(q) - self.assert_matched(results, 'path item') - self.assert_done(results) + self.assert_matched(results, ['path item']) def test_slashed_query_matches_path(self): q = '/a/b' results = self.lib.items(q) - self.assert_matched(results, 'path item') - self.assert_done(results) + self.assert_matched(results, ['path item']) def test_non_slashed_does_not_match_path(self): q = 'c.mp3' results = self.lib.items(q) - self.assert_done(results) + self.assert_matched(results, []) def test_slashes_in_explicit_field_does_not_match_path(self): q = 'title:/a/b' results = self.lib.items(q) - self.assert_done(results) + self.assert_matched(results, []) def test_path_regex(self): q = 'path::\\.mp3$' results = self.lib.items(q) - self.assert_matched(results, 'path item') - self.assert_done(results) + self.assert_matched(results, ['path item']) class BrowseTest(unittest.TestCase, AssertsMixin): def setUp(self): @@ -438,11 +424,12 @@ class BrowseTest(unittest.TestCase, AssertsMixin): def test_item_list(self): items = self.lib.items() - self.assert_matched(items, 'Littlest Things') - self.assert_matched(items, 'Take Pills') - self.assert_matched(items, 'Lovers Who Uncover') - self.assert_matched(items, 'Boracay') - self.assert_done(items) + self.assert_matched(items, [ + 'Littlest Things', + 'Take Pills', + 'Lovers Who Uncover', + 'Boracay', + ]) def test_albums_matches_album(self): albums = list(self.lib.albums('person')) @@ -454,12 +441,11 @@ class BrowseTest(unittest.TestCase, AssertsMixin): def test_items_matches_title(self): items = self.lib.items('boracay') - self.assert_matched(items, 'Boracay') - self.assert_done(items) + self.assert_matched(items, ['Boracay']) def test_items_does_not_match_year(self): items = self.lib.items('2007') - self.assert_done(items) + self.assert_matched(items, []) class StringParseTest(unittest.TestCase): def test_single_field_query(self): From 0176b9742f0139c6b7e609af0eb0561a1b7e1c40 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Wed, 21 Aug 2013 09:46:30 -0700 Subject: [PATCH 25/32] tests: unknown fields silently match nothing Previously, an unknown field would be ignored. Now, they're treated as flexattrs that just haven't been set yet (and thus always hold None). --- test/test_query.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/test/test_query.py b/test/test_query.py index 8fd8b6776..885a4506c 100644 --- a/test/test_query.py +++ b/test/test_query.py @@ -147,7 +147,9 @@ class GetTest(unittest.TestCase, AssertsMixin): def test_invalid_key(self): q = 'pope:bear' results = self.lib.items(q) - self.assert_matched_all(results) + # Matches nothing since the flexattr is not present on the + # objects. + self.assert_matched(results, []) def test_term_case_insensitive(self): q = 'UNCoVER' @@ -277,25 +279,23 @@ class MemoryGetTest(unittest.TestCase, AssertsMixin): results = self.lib.items(q) self.assert_matched(results, ['singleton item']) - def test_unknown_field_name_ignored(self): + def test_unknown_field_name_no_results(self): q = 'xyzzy:nonsense' results = self.lib.items(q) titles = [i.title for i in results] - self.assertTrue('singleton item' in titles) - self.assertTrue('album item' in titles) - self.assertEqual(len(titles), 2) + self.assertEqual(titles, []) - def test_unknown_field_name_ignored_in_album_query(self): + def test_unknown_field_name_no_results_in_album_query(self): q = 'xyzzy:nonsense' results = self.lib.albums(q) names = [a.album for a in results] - self.assertEqual(names, ['the album']) + self.assertEqual(names, []) - def test_item_field_name_ignored_in_album_query(self): + def test_item_field_name_matches_nothing_in_album_query(self): q = 'format:nonsense' results = self.lib.albums(q) names = [a.album for a in results] - self.assertEqual(names, ['the album']) + self.assertEqual(names, []) def test_unicode_query(self): self.single_item.title = u'caf\xe9' From 6677cea0362011271220bd1572e57474b642a8f2 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Wed, 21 Aug 2013 10:57:52 -0700 Subject: [PATCH 26/32] fix uses of item.dirty and item.record --- beets/ui/commands.py | 6 +++--- beetsplug/mbsync.py | 6 +++--- beetsplug/web/__init__.py | 2 +- test/test_ui.py | 29 ++++++++++++++--------------- test/test_zero.py | 18 ++++++++++-------- 5 files changed, 31 insertions(+), 30 deletions(-) diff --git a/beets/ui/commands.py b/beets/ui/commands.py index 6fbf0ae2a..3004fb229 100644 --- a/beets/ui/commands.py +++ b/beets/ui/commands.py @@ -905,7 +905,7 @@ def update_items(lib, query, album, move, pretend): continue # Read new data. - old_data = dict(item.record) + old_data = dict(item) try: item.read() except Exception as exc: @@ -920,12 +920,12 @@ def update_items(lib, query, album, move, pretend): old_data['albumartist'] == old_data['artist'] == \ item.artist: item.albumartist = old_data['albumartist'] - item.dirty['albumartist'] = False + item._dirty.remove('albumartist') # Get and save metadata changes. changes = {} for key in library.ITEM_KEYS_META: - if item.dirty[key]: + if key in item._dirty: changes[key] = old_data[key], getattr(item, key) if changes: # Something changed. diff --git a/beetsplug/mbsync.py b/beetsplug/mbsync.py index 81e802a33..56141d924 100644 --- a/beetsplug/mbsync.py +++ b/beetsplug/mbsync.py @@ -30,7 +30,7 @@ def _print_and_apply_changes(lib, item, move, pretend, write): """ changes = {} for key in library.ITEM_KEYS_META: - if item.dirty[key]: + if 'key' in item._dirty: changes[key] = item.old_data[key], getattr(item, key) if not changes: return False @@ -69,7 +69,7 @@ def mbsync_singletons(lib, query, move, pretend, write): .format(s.title)) continue - s.old_data = dict(s.record) + s.old_data = dict(s) # Get the MusicBrainz recording info. track_info = hooks.track_for_mbid(s.mb_trackid) @@ -94,7 +94,7 @@ def mbsync_albums(lib, query, move, pretend, write): items = list(a.items()) for item in items: - item.old_data = dict(item.record) + item.old_data = dict(item) # Get the MusicBrainz album information. album_info = hooks.album_for_mbid(a.mb_albumid) diff --git a/beetsplug/web/__init__.py b/beetsplug/web/__init__.py index 5fc518f29..ac2372c58 100644 --- a/beetsplug/web/__init__.py +++ b/beetsplug/web/__init__.py @@ -30,7 +30,7 @@ def _rep(obj, expand=False): included. """ if isinstance(obj, beets.library.Item): - out = dict(obj.record) + out = dict(obj) del out['path'] # Get the size (in bytes) of the backing file. This is useful diff --git a/test/test_ui.py b/test/test_ui.py index 6cb09dcf1..bbbcfb7c0 100644 --- a/test/test_ui.py +++ b/test/test_ui.py @@ -17,7 +17,6 @@ import os import shutil import textwrap -import logging import re import yaml @@ -162,7 +161,7 @@ class ModifyTest(_common.TestCase): def test_modify_item_dbdata(self): self._modify(["title=newTitle"]) - item = self.lib.items().next() + item = self.lib.items().get() self.assertEqual(item.title, 'newTitle') def test_modify_album_dbdata(self): @@ -172,47 +171,47 @@ class ModifyTest(_common.TestCase): def test_modify_item_tag_unmodified(self): self._modify(["title=newTitle"], write=False) - item = self.lib.items().next() + item = self.lib.items().get() item.read() self.assertEqual(item.title, 'full') def test_modify_album_tag_unmodified(self): self._modify(["album=newAlbum"], write=False, album=True) - item = self.lib.items().next() + item = self.lib.items().get() item.read() self.assertEqual(item.album, 'the album') def test_modify_item_tag(self): self._modify(["title=newTitle"], write=True) - item = self.lib.items().next() + item = self.lib.items().get() item.read() self.assertEqual(item.title, 'newTitle') def test_modify_album_tag(self): self._modify(["album=newAlbum"], write=True, album=True) - item = self.lib.items().next() + item = self.lib.items().get() item.read() self.assertEqual(item.album, 'newAlbum') def test_item_move(self): self._modify(["title=newTitle"], move=True) - item = self.lib.items().next() + item = self.lib.items().get() self.assertTrue('newTitle' in item.path) def test_album_move(self): self._modify(["album=newAlbum"], move=True, album=True) - item = self.lib.items().next() + item = self.lib.items().get() item.read() self.assertTrue('newAlbum' in item.path) def test_item_not_move(self): self._modify(["title=newTitle"], move=False) - item = self.lib.items().next() + item = self.lib.items().get() self.assertFalse('newTitle' in item.path) def test_album_not_move(self): self._modify(["album=newAlbum"], move=False, album=True) - item = self.lib.items().next() + item = self.lib.items().get() item.read() self.assertFalse('newAlbum' in item.path) @@ -333,7 +332,7 @@ class UpdateTest(_common.TestCase): mf.title = 'differentTitle' mf.save() self._update() - item = self.lib.items().next() + item = self.lib.items().get() self.assertEqual(item.title, 'differentTitle') def test_modified_metadata_moved(self): @@ -341,7 +340,7 @@ class UpdateTest(_common.TestCase): mf.title = 'differentTitle' mf.save() self._update(move=True) - item = self.lib.items().next() + item = self.lib.items().get() self.assertTrue('differentTitle' in item.path) def test_modified_metadata_not_moved(self): @@ -349,7 +348,7 @@ class UpdateTest(_common.TestCase): mf.title = 'differentTitle' mf.save() self._update(move=False) - item = self.lib.items().next() + item = self.lib.items().get() self.assertTrue('differentTitle' not in item.path) def test_modified_album_metadata_moved(self): @@ -357,7 +356,7 @@ class UpdateTest(_common.TestCase): mf.album = 'differentAlbum' mf.save() self._update(move=True) - item = self.lib.items().next() + item = self.lib.items().get() self.assertTrue('differentAlbum' in item.path) def test_modified_album_metadata_art_moved(self): @@ -379,7 +378,7 @@ class UpdateTest(_common.TestCase): self.lib.store(self.i) self._update(reset_mtime=False) - item = self.lib.items().next() + item = self.lib.items().get() self.assertEqual(item.title, 'full') class PrintTest(_common.TestCase): diff --git a/test/test_zero.py b/test/test_zero.py index 2cc9bcf2e..333ca2b51 100644 --- a/test/test_zero.py +++ b/test/test_zero.py @@ -7,11 +7,12 @@ from beetsplug.zero import ZeroPlugin class ZeroPluginTest(unittest.TestCase): def test_no_patterns(self): - v = {'comments' : 'test comment', - 'day' : 13, - 'month' : 3, - 'year' : 2012} - i=Item(v) + i = Item( + comments='test comment', + day=13, + month=3, + year=2012, + ) z = ZeroPlugin() z.debug = False z.fields = ['comments', 'month', 'day'] @@ -25,9 +26,10 @@ class ZeroPluginTest(unittest.TestCase): self.assertEqual(i.year, 2012) def test_patterns(self): - v = {'comments' : 'from lame collection, ripped by eac', - 'year' : 2012} - i=Item(v) + i = Item( + comments='from lame collection, ripped by eac', + year=2012, + ) z = ZeroPlugin() z.debug = False z.fields = ['comments', 'year'] From deef7f9d20decb73f19f0e56225913be53d9ad70 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Wed, 21 Aug 2013 11:23:09 -0700 Subject: [PATCH 27/32] a few more necessary album.store()s --- beets/ui/commands.py | 9 +++++---- beetsplug/fetchart.py | 2 ++ test/test_ui.py | 1 + 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/beets/ui/commands.py b/beets/ui/commands.py index 3004fb229..c835b8bb8 100644 --- a/beets/ui/commands.py +++ b/beets/ui/commands.py @@ -959,17 +959,18 @@ def update_items(lib, query, album, move, pretend): if album_id is None: # Singletons. continue album = lib.get_album(album_id) - if not album: # Empty albums have already been removed. + if not album: # Empty albums have already been removed. log.debug('emptied album %i' % album_id) continue - al_items = list(album.items()) + first_item = album.items().get() # Update album structure to reflect an item in it. for key in library.ALBUM_KEYS_ITEM: - setattr(album, key, getattr(al_items[0], key)) + album[key] = first_item[key] + album.store() # Move album art (and any inconsistent items). - if move and lib.directory in ancestry(al_items[0].path): + if move and lib.directory in ancestry(first_item.path): log.debug('moving album %i' % album_id) album.move() diff --git a/beetsplug/fetchart.py b/beetsplug/fetchart.py index 15986c8d1..61361f169 100644 --- a/beetsplug/fetchart.py +++ b/beetsplug/fetchart.py @@ -216,6 +216,7 @@ def batch_fetch_art(lib, albums, force, maxwidth=None): if path: album.set_art(path, False) + album.store() message = 'found album art' else: message = 'no art found' @@ -274,6 +275,7 @@ class FetchArtPlugin(BeetsPlugin): src_removed = config['import']['delete'].get(bool) or \ config['import']['move'].get(bool) album.set_art(path, not src_removed) + album.store() if src_removed: task.prune(path) diff --git a/test/test_ui.py b/test/test_ui.py index bbbcfb7c0..4f1a6a6e6 100644 --- a/test/test_ui.py +++ b/test/test_ui.py @@ -299,6 +299,7 @@ class UpdateTest(_common.TestCase): artfile = os.path.join(_common.RSRC, 'testart.jpg') _common.touch(artfile) self.album.set_art(artfile) + self.album.store() os.remove(artfile) def _update(self, query=(), album=False, move=False, reset_mtime=True): From 15cf04628535feb6694fdb2d482ef5e83b8cb3b0 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Wed, 21 Aug 2013 11:38:48 -0700 Subject: [PATCH 28/32] don't reset mtime on awakening from DB --- beets/library.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/beets/library.py b/beets/library.py index 64241d910..d254f731c 100644 --- a/beets/library.py +++ b/beets/library.py @@ -446,10 +446,18 @@ class Item(LibModel): value = str(value) if key in ITEM_KEYS_WRITABLE: - self.mtime = 0 # Reset mtime on dirty. + self.mtime = 0 # Reset mtime on dirty. super(Item, self).__setitem__(key, value) + def update(self, values): + """Sett all key/value pairs in the mapping. If mtime is + specified, it is not reset (as it might otherwise be). + """ + super(Item, self).update(values) + if self.mtime == 0 and 'mtime' in values: + self.mtime = values['mtime'] + # Interaction with file metadata. From c7fe017752b1e843d7897dd614c12de579a6e5a9 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Wed, 21 Aug 2013 15:34:45 -0700 Subject: [PATCH 29/32] remove Library.{move,store} methods These methods are now provided by LibModel, which makes dealing with items and albums symmetric. --- beets/importer.py | 4 +-- beets/library.py | 49 +++------------------------------ beets/ui/commands.py | 11 +++----- beetsplug/chroma.py | 14 +++++----- beetsplug/convert.py | 4 +-- beetsplug/echonest_tempo.py | 2 +- beetsplug/lastgenre/__init__.py | 7 +++-- beetsplug/lyrics.py | 2 +- beetsplug/mbsync.py | 2 +- beetsplug/replaygain.py | 3 +- test/test_db.py | 14 +++++----- test/test_files.py | 12 ++++---- test/test_ihate.py | 2 +- test/test_importer.py | 1 + test/test_query.py | 2 +- test/test_ui.py | 18 ++++++------ 16 files changed, 53 insertions(+), 94 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index a5371fabf..398f5fcf6 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -811,7 +811,7 @@ def plugin_stage(session, func): # Stage may modify DB, so re-load cached item data. for item in task.imported_items(): - session.lib.load(item) + item.load() def manipulate_files(session): """A coroutine (pipeline stage) that performs necessary file @@ -858,7 +858,7 @@ def manipulate_files(session): # Save new paths. with session.lib.transaction(): for item in items: - session.lib.store(item) + item.store() # Plugin event. plugins.send('import_task_files', session=session, task=task) diff --git a/beets/library.py b/beets/library.py index d254f731c..ef6f86e85 100644 --- a/beets/library.py +++ b/beets/library.py @@ -1405,6 +1405,8 @@ class Library(object): item.added = time.time() if copy: self.move(item, copy=True) + if not item._lib: + item._lib = self # Build essential parts of query. columns = ','.join([key for key in ITEM_KEYS if key != 'id']) @@ -1438,49 +1440,6 @@ class Library(object): self._memotable = {} return new_id - def load(self, item): - """Refresh the item's metadata from the library database. - """ - if item.id is None: - raise ValueError('cannot load item with no id') - stored_item = self.get_item(item.id) - item.update(dict(stored_item)) - item.clear_dirty() - - def store(self, item): - """Save the item's metadata into the library database. - """ - # Build assignments for query. - assignments = '' - subvars = [] - for key in ITEM_KEYS: - if key != 'id' and key in item._dirty: - assignments += key + '=?,' - value = getattr(item, key) - # Wrap path strings in buffers so they get stored - # "in the raw". - if key == 'path' and isinstance(value, str): - value = buffer(value) - subvars.append(value) - assignments = assignments[:-1] # Knock off last , - - with self.transaction() as tx: - # Main table update. - if assignments: - query = 'UPDATE items SET ' + assignments + ' WHERE id=?' - subvars.append(item.id) - tx.mutate(query, subvars) - - # Flexible attributes. - flexins = 'INSERT INTO item_attributes ' \ - ' (entity_id, key, value)' \ - ' VALUES (?, ?, ?)' - for key, value in item._values_flex.items(): - tx.mutate(flexins, (item.id, key, value)) - - item.clear_dirty() - self._memotable = {} - def remove(self, item, delete=False, with_album=True): """Removes this item. If delete, then the associated file is removed from disk. If with_album, then the item's album (if any) @@ -1531,7 +1490,7 @@ class Library(object): old_path = item.path item.move(dest, copy) if item.id is not None: - self.store(item) + item.store() # If this item is in an album, move its art. if with_album: @@ -1648,7 +1607,7 @@ class Library(object): if item.id is None: self.add(item) else: - self.store(item) + item.store() # Construct the new Album object. album_values['id'] = album_id diff --git a/beets/ui/commands.py b/beets/ui/commands.py index c835b8bb8..70f59c5f4 100644 --- a/beets/ui/commands.py +++ b/beets/ui/commands.py @@ -941,14 +941,14 @@ def update_items(lib, query, album, move, pretend): if move and lib.directory in ancestry(item.path): lib.move(item) - lib.store(item) + item.store() affected_albums.add(item.album_id) elif not pretend: # The file's mtime was different, but there were no changes # to the metadata. Store the new mtime, which is set in the # call to read(), so we don't check this again in the # future. - lib.store(item) + item.store() # Skip album changes while pretending. if pretend: @@ -1163,10 +1163,7 @@ def modify_items(lib, mods, query, write, move, album, confirm): else: lib.move(obj) - if album: - obj.store() - else: - lib.store(obj) + obj.store() # Apply tags if requested. if write: @@ -1223,7 +1220,7 @@ def move_items(lib, dest, query, copy, album): obj.move(copy, basedir=dest) else: lib.move(obj, copy, basedir=dest) - lib.store(obj) + obj.store() move_cmd = ui.Subcommand('move', help='move or copy items', aliases=('mv',)) diff --git a/beetsplug/chroma.py b/beetsplug/chroma.py index 83f67c0a9..084540269 100644 --- a/beetsplug/chroma.py +++ b/beetsplug/chroma.py @@ -160,7 +160,7 @@ class AcoustidPlugin(plugins.BeetsPlugin): help='generate fingerprints for items without them') def fingerprint_cmd_func(lib, opts, args): for item in lib.items(ui.decargs(args)): - fingerprint_item(item, lib=lib, + fingerprint_item(item, write=config['import']['write'].get(bool)) fingerprint_cmd.func = fingerprint_cmd_func @@ -237,12 +237,12 @@ def submit_items(userkey, items, chunksize=64): submit_chunk() -def fingerprint_item(item, lib=None, write=False): +def fingerprint_item(item, write=False): """Get the fingerprint for an Item. If the item already has a fingerprint, it is not regenerated. If fingerprint generation fails, - return None. If `lib` is provided, then new fingerprints are saved - to the database. If `write` is set, then the new fingerprints are - also written to files' metadata. + return None. If the items are associated with a library, they are + saved to the database. If `write` is set, then the new fingerprints + are also written to files' metadata. """ # Get a fingerprint and length for this track. if not item.length: @@ -271,8 +271,8 @@ def fingerprint_item(item, lib=None, write=False): util.displayable_path(item.path) )) item.write() - if lib: - lib.store(item) + if item._lib: + item.store() return item.acoustid_fingerprint except acoustid.FingerprintGenerationError as exc: log.info( diff --git a/beetsplug/convert.py b/beetsplug/convert.py index 1bb81ae4f..f7e922019 100644 --- a/beetsplug/convert.py +++ b/beetsplug/convert.py @@ -120,7 +120,7 @@ def convert_item(lib, dest_dir, keep_new, path_formats): # writing) to get new bitrate, duration, etc. if keep_new: item.read() - lib.store(item) # Store new path and audio data. + item.store() # Store new path and audio data. if config['convert']['embed']: album = lib.get_album(item) @@ -142,7 +142,7 @@ def convert_on_import(lib, item): item.path = dest item.write() item.read() # Load new audio information data. - lib.store(item) + item.store() def convert_func(lib, opts, args): diff --git a/beetsplug/echonest_tempo.py b/beetsplug/echonest_tempo.py index d3a55d213..d5026c021 100644 --- a/beetsplug/echonest_tempo.py +++ b/beetsplug/echonest_tempo.py @@ -54,7 +54,7 @@ def fetch_item_tempo(lib, loglevel, item, write): item.bpm = tempo if write: item.write() - lib.store(item) + item.store() def get_tempo(artist, title): """Get the tempo for a song.""" diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index e26ade1fc..dbfa92224 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -335,7 +335,7 @@ class LastGenrePlugin(plugins.BeetsPlugin): # track on the album. if 'track' in self.sources: item.genre, src = self._get_genre(item) - lib.store(item) + item.store() log.info(u'genre for track {0} - {1} ({2}): {3}'.format( item.artist, item.title, src, item.genre )) @@ -353,17 +353,18 @@ class LastGenrePlugin(plugins.BeetsPlugin): album.genre, src = self._get_genre(album) log.debug(u'added last.fm album genre ({0}): {1}'.format( src, album.genre)) + album.store() if 'track' in self.sources: for item in album.items(): item.genre, src = self._get_genre(item) log.debug(u'added last.fm item genre ({0}): {1}'.format( src, item.genre)) - session.lib.store(item) + item.store() else: item = task.item item.genre, src = self._get_genre(item) log.debug(u'added last.fm item genre ({0}): {1}'.format( src, item.genre)) - session.lib.store(item) + item.store() diff --git a/beetsplug/lyrics.py b/beetsplug/lyrics.py index bc71a5a89..a6d5b7e7a 100644 --- a/beetsplug/lyrics.py +++ b/beetsplug/lyrics.py @@ -444,7 +444,7 @@ class LyricsPlugin(BeetsPlugin): if write: item.write() - lib.store(item) + item.store() def get_lyrics(self, artist, title): """Fetch lyrics, trying each source in turn. Return a string or diff --git a/beetsplug/mbsync.py b/beetsplug/mbsync.py index 56141d924..140cb95c1 100644 --- a/beetsplug/mbsync.py +++ b/beetsplug/mbsync.py @@ -53,7 +53,7 @@ def _print_and_apply_changes(lib, item, move, pretend, write): log.error(u'could not sync {0}: {1}'.format( util.displayable_path(item.path), exc)) return False - lib.store(item) + item.store() return True diff --git a/beetsplug/replaygain.py b/beetsplug/replaygain.py index ba6bae862..6b4c2f407 100644 --- a/beetsplug/replaygain.py +++ b/beetsplug/replaygain.py @@ -225,7 +225,7 @@ class ReplayGainPlugin(BeetsPlugin): for item, info in zip(items, rgain_infos): item.rg_track_gain = info['gain'] item.rg_track_peak = info['peak'] - lib.store(item) + item.store() log.debug(u'replaygain: applied track gain {0}, peak {1}'.format( item.rg_track_gain, @@ -241,3 +241,4 @@ class ReplayGainPlugin(BeetsPlugin): album.rg_album_gain, album.rg_album_peak )) + album.store() diff --git a/test/test_db.py b/test/test_db.py index 744933049..0647a9b8c 100644 --- a/test/test_db.py +++ b/test/test_db.py @@ -40,7 +40,7 @@ def remove_lib(): if os.path.exists(TEMP_LIB): os.unlink(TEMP_LIB) def boracay(l): - return beets.library.Item( + return beets.library.Item(l, **l._connection().execute('select * from items where id=3').fetchone() ) np = util.normpath @@ -56,12 +56,12 @@ class LoadTest(unittest.TestCase): def test_load_restores_data_from_db(self): original_title = self.i.title self.i.title = 'something' - self.lib.load(self.i) + self.i.load() self.assertEqual(original_title, self.i.title) def test_load_clears_dirty_flags(self): self.i.artist = 'something' - self.lib.load(self.i) + self.i.load() self.assertTrue('artist' not in self.i._dirty) class StoreTest(unittest.TestCase): @@ -74,7 +74,7 @@ class StoreTest(unittest.TestCase): def test_store_changes_database_value(self): self.i.year = 1987 - self.lib.store(self.i) + self.i.store() new_year = self.lib._connection().execute( 'select year from items where ' 'title="Boracay"').fetchone()['year'] @@ -83,7 +83,7 @@ class StoreTest(unittest.TestCase): def test_store_only_writes_dirty_fields(self): original_genre = self.i.genre self.i._values_fixed['genre'] = 'beatboxing' # change w/o dirtying - self.lib.store(self.i) + self.i.store() new_genre = self.lib._connection().execute( 'select genre from items where ' 'title="Boracay"').fetchone()['genre'] @@ -91,7 +91,7 @@ class StoreTest(unittest.TestCase): def test_store_clears_dirty_flags(self): self.i.composer = 'tvp' - self.lib.store(self.i) + self.i.store() self.assertTrue('composer' not in self.i._dirty) class AddTest(unittest.TestCase): @@ -884,7 +884,7 @@ class PathStringTest(_common.TestCase): def test_special_chars_preserved_in_database(self): path = 'b\xe1r' self.i.path = path - self.lib.store(self.i) + self.i.store() i = list(self.lib.items())[0] self.assertEqual(i.path, path) diff --git a/test/test_files.py b/test/test_files.py index ff887637c..03588a706 100644 --- a/test/test_files.py +++ b/test/test_files.py @@ -171,7 +171,7 @@ class AlbumFileTest(_common.TestCase): self.ai.album = 'newAlbumName' self.ai.move() self.ai.store() - self.lib.load(self.i) + self.i.load() self.assert_('newAlbumName' in self.i.path) @@ -180,7 +180,7 @@ class AlbumFileTest(_common.TestCase): self.ai.album = 'newAlbumName' self.ai.move() self.ai.store() - self.lib.load(self.i) + self.i.load() self.assertFalse(os.path.exists(oldpath)) self.assertTrue(os.path.exists(self.i.path)) @@ -190,14 +190,14 @@ class AlbumFileTest(_common.TestCase): self.ai.album = 'newAlbumName' self.ai.move(True) self.ai.store() - self.lib.load(self.i) + self.i.load() self.assertTrue(os.path.exists(oldpath)) self.assertTrue(os.path.exists(self.i.path)) def test_albuminfo_move_to_custom_dir(self): self.ai.move(basedir=self.otherdir) - self.lib.load(self.i) + self.i.load() self.ai.store() self.assertTrue('testotherdir' in self.i.path) @@ -234,7 +234,7 @@ class ArtFileTest(_common.TestCase): oldpath = self.i.path self.ai.album = 'newAlbum' self.ai.move() - self.lib.load(self.i) + self.i.load() self.assertNotEqual(self.i.path, oldpath) self.assertFalse(os.path.exists(self.art)) @@ -245,7 +245,7 @@ class ArtFileTest(_common.TestCase): # Move the album to another directory. self.ai.move(basedir=self.otherdir) self.ai.store() - self.lib.load(self.i) + self.i.load() # Art should be in new directory. self.assertNotExists(self.art) diff --git a/test/test_ihate.py b/test/test_ihate.py index 3ff6dcd8d..9c241fc54 100644 --- a/test/test_ihate.py +++ b/test/test_ihate.py @@ -16,7 +16,7 @@ class IHatePluginTest(unittest.TestCase): task = ImportTask() task.cur_artist = u'Test Artist' task.cur_album = u'Test Album' - task.items = [Item({'genre': 'Test Genre'})] + task.items = [Item(genre='Test Genre')] self.assertFalse(IHatePlugin.do_i_hate_this(task, genre_p, artist_p, album_p, white_p)) genre_p = 'some_genre test\sgenre'.split() diff --git a/test/test_importer.py b/test/test_importer.py index 4d159b504..34a9e3f0e 100644 --- a/test/test_importer.py +++ b/test/test_importer.py @@ -193,6 +193,7 @@ class ImportApplyTest(_common.TestCase): shutil.copy(os.path.join(_common.RSRC, 'full.mp3'), self.srcpath) self.i = library.Item.from_path(self.srcpath) self.i.comp = False + self.lib.add(self.i) trackinfo = TrackInfo('one', 'trackid', 'some artist', 'artistid', 1) diff --git a/test/test_query.py b/test/test_query.py index 885a4506c..996e341b4 100644 --- a/test/test_query.py +++ b/test/test_query.py @@ -299,7 +299,7 @@ class MemoryGetTest(unittest.TestCase, AssertsMixin): def test_unicode_query(self): self.single_item.title = u'caf\xe9' - self.lib.store(self.single_item) + self.single_item.store() q = u'title:caf\xe9' results = self.lib.items(q) diff --git a/test/test_ui.py b/test/test_ui.py index 4f1a6a6e6..868c4b6b3 100644 --- a/test/test_ui.py +++ b/test/test_ui.py @@ -54,7 +54,7 @@ class ListTest(_common.TestCase): def test_list_unicode_query(self): self.item.title = u'na\xefve' - self.lib.store(self.item) + self.item.store() self.lib._connection().commit() self._run_list([u'na\xefve']) @@ -241,42 +241,42 @@ class MoveTest(_common.TestCase): def test_move_item(self): self._move() - self.lib.load(self.i) + self.i.load() self.assertTrue('testlibdir' in self.i.path) self.assertExists(self.i.path) self.assertNotExists(self.itempath) def test_copy_item(self): self._move(copy=True) - self.lib.load(self.i) + self.i.load() self.assertTrue('testlibdir' in self.i.path) self.assertExists(self.i.path) self.assertExists(self.itempath) def test_move_album(self): self._move(album=True) - self.lib.load(self.i) + self.i.load() self.assertTrue('testlibdir' in self.i.path) self.assertExists(self.i.path) self.assertNotExists(self.itempath) def test_copy_album(self): self._move(copy=True, album=True) - self.lib.load(self.i) + self.i.load() self.assertTrue('testlibdir' in self.i.path) self.assertExists(self.i.path) self.assertExists(self.itempath) def test_move_item_custom_dir(self): self._move(dest=self.otherdir) - self.lib.load(self.i) + self.i.load() self.assertTrue('testotherdir' in self.i.path) self.assertExists(self.i.path) self.assertNotExists(self.itempath) def test_move_album_custom_dir(self): self._move(dest=self.otherdir, album=True) - self.lib.load(self.i) + self.i.load() self.assertTrue('testotherdir' in self.i.path) self.assertExists(self.i.path) self.assertNotExists(self.itempath) @@ -306,7 +306,7 @@ class UpdateTest(_common.TestCase): self.io.addinput('y') if reset_mtime: self.i.mtime = 0 - self.lib.store(self.i) + self.i.store() commands.update_items(self.lib, query, album, move, False) def test_delete_removes_item(self): @@ -376,7 +376,7 @@ class UpdateTest(_common.TestCase): # Make in-memory mtime match on-disk mtime. self.i.mtime = os.path.getmtime(self.i.path) - self.lib.store(self.i) + self.i.store() self._update(reset_mtime=False) item = self.lib.items().get() From f9f8994d85cf054c78dd972e872e1e3cd2af1099 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Wed, 21 Aug 2013 18:44:14 -0700 Subject: [PATCH 30/32] flexattr description in changelog --- beets/__init__.py | 2 +- docs/changelog.rst | 33 +++++++++++++++++++++++++++++++++ docs/conf.py | 4 ++-- setup.py | 2 +- 4 files changed, 37 insertions(+), 4 deletions(-) diff --git a/beets/__init__.py b/beets/__init__.py index a556c3e44..3e794c068 100644 --- a/beets/__init__.py +++ b/beets/__init__.py @@ -12,7 +12,7 @@ # The above copyright notice and this permission notice shall be # included in all copies or substantial portions of the Software. -__version__ = '1.2.2' +__version__ = '1.3.0' __author__ = 'Adrian Sampson ' import beets.library diff --git a/docs/changelog.rst b/docs/changelog.rst index 08cd8f127..039b803bf 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -1,6 +1,39 @@ Changelog ========= +1.3.0 (in development) +---------------------- + +Albums and items now have **flexible attributes**. This means that, when you +want to store information about your music in the beets database, you're no +longer constrained to the set of fields it supports out of the box (title, +artist, track, etc.). Instead, you can use any field name you can think of and +treat it just like the built-in fields. + +For example, you can use the :ref:`modify-cmd` command to set a new field on a +track:: + + $ beet modify mood=sexy artist:miguel + +and then query your music based on that field:: + + $ beet ls mood:sunny + +or use templates to see the value of the field:: + + $ beet ls -f '$title: $mood' + +While this feature is nifty when used directly with the usual command-line +suspects, it's especially useful for plugin authors and for future beets +features. Stay tuned for great things built on this flexible attribute +infrastructure. + +One side effect of this change: queries that include unknown fields will now +match *nothing* instead of *everything*. So if you type ``beet ls +fieldThatDoesNotExist:foo``, beets will now return no results, whereas +previous versions would spit out a warning and then list your entire library. + + 1.2.2 (in development) ---------------------- diff --git a/docs/conf.py b/docs/conf.py index 4a9e7f2b8..3a646837d 100644 --- a/docs/conf.py +++ b/docs/conf.py @@ -12,8 +12,8 @@ master_doc = 'index' project = u'beets' copyright = u'2012, Adrian Sampson' -version = '1.2' -release = '1.2.2' +version = '1.3' +release = '1.3.0' pygments_style = 'sphinx' diff --git a/setup.py b/setup.py index f5f5e9347..d6201eba1 100755 --- a/setup.py +++ b/setup.py @@ -42,7 +42,7 @@ if 'sdist' in sys.argv: shutil.copytree(os.path.join(docdir, '_build', 'man'), mandir) setup(name='beets', - version='1.2.2', + version='1.3.0', description='music tagger and library organizer', author='Adrian Sampson', author_email='adrian@radbox.org', From 5e09c5e473e03df83ed67883799ed94199687cd0 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Thu, 22 Aug 2013 17:33:56 -0700 Subject: [PATCH 31/32] convert: fix iteration over results --- beetsplug/convert.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beetsplug/convert.py b/beetsplug/convert.py index f7e922019..6beaabbf9 100644 --- a/beetsplug/convert.py +++ b/beetsplug/convert.py @@ -168,7 +168,7 @@ def convert_func(lib, opts, args): if opts.album: items = (i for a in lib.albums(ui.decargs(args)) for i in a.items()) else: - items = lib.items(ui.decargs(args)) + items = iter(lib.items(ui.decargs(args))) convert = [convert_item(lib, dest, keep_new, path_formats) for i in range(threads)] pipe = util.pipeline.Pipeline([items, convert]) pipe.run_parallel() From 343a85d482f0c2ec2d801975b8c61b74c0682980 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Fri, 23 Aug 2013 14:01:31 -0700 Subject: [PATCH 32/32] mbsync: use separate structure for old_data Assigning an attribute on Items doesn't really work here since we try to store that value to the DB as a flexattr. --- beetsplug/mbsync.py | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/beetsplug/mbsync.py b/beetsplug/mbsync.py index 140cb95c1..4ffe9a02e 100644 --- a/beetsplug/mbsync.py +++ b/beetsplug/mbsync.py @@ -24,14 +24,14 @@ from beets import config log = logging.getLogger('beets') -def _print_and_apply_changes(lib, item, move, pretend, write): +def _print_and_apply_changes(lib, item, old_data, move, pretend, write): """Apply changes to an Item and preview them in the console. Return a boolean indicating whether any changes were made. """ changes = {} for key in library.ITEM_KEYS_META: - if 'key' in item._dirty: - changes[key] = item.old_data[key], getattr(item, key) + if key in item._dirty: + changes[key] = old_data[key], getattr(item, key) if not changes: return False @@ -69,7 +69,7 @@ def mbsync_singletons(lib, query, move, pretend, write): .format(s.title)) continue - s.old_data = dict(s) + old_data = dict(s) # Get the MusicBrainz recording info. track_info = hooks.track_for_mbid(s.mb_trackid) @@ -80,7 +80,7 @@ def mbsync_singletons(lib, query, move, pretend, write): # Apply. with lib.transaction(): autotag.apply_item_metadata(s, track_info) - _print_and_apply_changes(lib, s, move, pretend, write) + _print_and_apply_changes(lib, s, old_data, move, pretend, write) def mbsync_albums(lib, query, move, pretend, write): @@ -93,8 +93,7 @@ def mbsync_albums(lib, query, move, pretend, write): continue items = list(a.items()) - for item in items: - item.old_data = dict(item) + old_data = {item: dict(item) for item in items} # Get the MusicBrainz album information. album_info = hooks.album_for_mbid(a.mb_albumid) @@ -116,8 +115,8 @@ def mbsync_albums(lib, query, move, pretend, write): autotag.apply_metadata(album_info, mapping) changed = False for item in items: - changed = _print_and_apply_changes(lib, item, move, pretend, - write) or changed + changed |= _print_and_apply_changes(lib, item, old_data[item], + move, pretend, write) if not changed: # No change to any item. continue