attempt at managing album info table in Python instead of with triggers

I think this is the end of the road for the design that treats albums as a
lightweight hanger-on to items. That is, we attempted to keep the interface
strictly item-focused; album information was created and deleted on the fly in
response to creation and deletion of items. I now believe that this was
ultimately a bad idea and can only lead to unexpected behavior and complex
implementation. It's time to start over.

beets/library.py

@@ -115,6 +115,13 @@ def _ancestry(path):
             out.insert(0, path)
     return out
 
+def _mkdirall(path):
+    """Like mkdir -p, make directories for the entire ancestry of path.
+    """
+    for ancestor in _ancestry(path):
+        if not os.path.isdir(ancestor):
+            os.mkdir(ancestor)
+
 def _components(path):
     """Return a list of the path components in path. For instance:
        >>> _components('/a/b/c')
@@ -273,12 +280,9 @@ class Item(object):
         """
         dest = library.destination(self)
         
-        # Create necessary ancestry for the move. Like os.renames but only
-        # halfway.
-        for ancestor in _ancestry(dest):
-            if not os.path.isdir(ancestor):
-                os.mkdir(ancestor)
-        
+        # Create necessary ancestry for the move.
+        _mkdirall(dest)
+
         if copy:
             shutil.copy(self.path, dest)
         else:
@@ -717,8 +721,6 @@ class Library(BaseLibrary):
         
         self._make_table('items', item_fields)
         self._make_table('albums', album_fields)
-        
-        self._make_triggers()
     
     def _make_table(self, table, fields):
         """Set up the schema of the library file. fields is a list of
@@ -756,31 +758,6 @@ class Library(BaseLibrary):
         self.conn.executescript(setup_sql)
         self.conn.commit()
 
-    def _make_triggers(self):
-        """Setup triggers for the database to keep the tables
-        consistent.
-        """
-        # Set up triggers for dropping album info rows when no longer
-        # needed.
-        trigger_sql = """
-        WHEN
-          ((SELECT id FROM items WHERE album=OLD.album AND artist=OLD.artist)
-          IS NULL)
-        BEGIN
-          DELETE FROM albums WHERE
-          album=OLD.album AND artist=OLD.artist;
-        END;
-        """
-        self.conn.execute("""
-        CREATE TRIGGER IF NOT EXISTS delete_album
-        AFTER DELETE ON items
-        """ + trigger_sql)
-        self.conn.execute("""
-        CREATE TRIGGER IF NOT EXISTS change_album
-        AFTER UPDATE OF album, artist ON items
-        """ + trigger_sql)
-        self.conn.commit()
-
     def destination(self, item):
         """Returns the path in the library directory designated for item
         item (i.e., where the file ought to be).
@@ -879,10 +856,13 @@ class Library(BaseLibrary):
         # build assignments for query
         assignments = ''
         subvars = []
+        album_changed = False
         for key in ITEM_KEYS:
             if (key != 'id') and (item.dirty[key] or store_all):
                 assignments += key + '=?,'
                 subvars.append(getattr(item, key))
+                if key in ('artist', 'album'):
+                    album_changed = True
         
         if not assignments:
             # nothing to store (i.e., nothing was dirty)
@@ -890,6 +870,12 @@ class Library(BaseLibrary):
         
         assignments = assignments[:-1] # knock off last ,
 
+        # Get old artist and album for cleanup.
+        old_artist, old_album = self.conn.execute(
+            'SELECT artist, album FROM items WHERE id=?',
+            (item.id,)
+        ).fetchone()
+
         # finish the query
         query = 'UPDATE items SET ' + assignments + ' WHERE id=?'
         subvars.append(item.id)
@@ -897,9 +883,45 @@ class Library(BaseLibrary):
         self.conn.execute(query, subvars)
         item._clear_dirty()
 
-    def remove(self, item):
-        self.conn.execute('DELETE FROM items WHERE id=?', (item.id,))
+        # Clean up album.
+        album_row = self._cleanup_album(old_artist, old_album)
 
+    def remove(self, item, delete=False):
+        # Get album and artist so we can clean up the album entry.
+        artist, album = self.conn.execute(
+            'SELECT artist, album FROM items WHERE id=?',
+            (item.id,)
+        ).fetchone()
+
+        self.conn.execute('DELETE FROM items WHERE id=?', (item.id,))
+        if delete:
+            os.unlink(item.path)
+
+        # Clean up album.
+        album_row = self._cleanup_album(artist, album)
+        if delete and album_row and album_row['artpath']:
+            # When deleting items, delete their art as well.
+            os.unlink(album_row['artpath'])
+
+    def _cleanup_album(self, artist, album):
+        """If there are no items with the album specified, then removes
+        the corresponding album entry and returns it. Otherwise, returns
+        None.
+        """
+        c = self.conn.execute(
+            'SELECT id FROM items WHERE artist=? AND album=?',
+            (artist, album)
+        )
+        if c.fetchone() is None:
+            album_row = self.conn.execute(
+                'SELECT * FROM albums WHERE artist=? AND album=?',
+                (artist, album)
+            ).fetchone()
+            self.conn.execute(
+                'DELETE FROM albums WHERE artist=? AND album=?',
+                (artist, album)
+            )
+            return album_row
 
     # Browsing.
 
@@ -972,3 +994,4 @@ class Library(BaseLibrary):
     def _album_set(self, album_id, key, value):
         sql = 'UPDATE albums SET %s=? WHERE id=?' % key
         self.conn.execute(sql, (value, album_id))
+

beets/ui/commands.py

@@ -411,9 +411,7 @@ def remove_items(lib, query, album, delete=False):
 
     # Remove and delete.
     for item in items:
-        lib.remove(item)
-        if delete:
-            os.unlink(item.path)
+        lib.remove(item, delete)
     lib.save()
 
 remove_cmd = ui.Subcommand('remove',

test/test_db.py

@@ -19,6 +19,7 @@ import unittest
 import sys
 import os
 import sqlite3
+import shutil
 sys.path.append('..')
 import beets.library
 
@@ -263,6 +264,45 @@ class ArtDestinationTest(unittest.TestCase):
         track = self.i.path
         self.assertEqual(os.path.dirname(art), os.path.dirname(track))
 
+class ArtFileTest(unittest.TestCase):
+    def _touch(self, path):
+        # Create file if it doesn't exist.
+        open(path, 'a').close()
+
+    def setUp(self):
+        # Make library and item.
+        self.lib = beets.library.Library(':memory:')
+        self.libdir = os.path.join('rsrc', 'testlibdir')
+        self.lib.directory = self.libdir
+        self.i = item()
+        self.i.path = self.lib.destination(self.i)
+        # Make a file.
+        beets.library._mkdirall(self.i.path)
+        self._touch(self.i.path)
+        self.lib.add(self.i)
+        # Make an art file too.
+        self.art = self.lib.art_path(self.i, 'something.jpg')
+        self._touch(self.art)
+        self.lib.albuminfo(self.i).artpath = self.art
+    def tearDown(self):
+        if os.path.exists(self.libdir):
+            shutil.rmtree(self.libdir)
+
+    def test_art_deleted_when_items_deleted(self):
+        self.assertTrue(os.path.exists(self.art))
+        self.lib.remove(self.i, True)
+        self.assertFalse(os.path.exists(self.art))
+
+    def test_art_moves_with_last_album(self):
+        self.assertTrue(os.path.exists(self.art))
+        oldpath = self.i.path
+        self.i.artist = 'newArtist'
+        self.i.move(self.lib)
+        self.assertNotEqual(self.i.path, oldpath)
+        self.assertFalse(os.path.exists(self.art))
+        newart = self.lib.art_path(self.i)
+        self.assertTrue(os.path.exists(newart))
+
 class MigrationTest(unittest.TestCase):
     """Tests the ability to change the database schema between
     versions.

test/test_files.py

@@ -101,3 +101,4 @@ def suite():
 
 if __name__ == '__main__':
     unittest.main(defaultTest='suite')
+