Fix #314: Delete old album art during re-import #1681

sampsyo · 2015-11-02T00:23:09Z

With a few changes to importer.py, the re-import workflow deletes orphaned album art by keeping track of which files aren't reused by fetchart during the import process.

First step towards fixing #314.

sampsyo · 2015-11-02T00:32:51Z

beets/importer.py

+            # to None, making self.prune() a no-op; hence this workaround of
+            # calling util.prune_dirs() directly.
+            util.prune_dirs(os.path.dirname(old_art_path),
+                            clutter=config['clutter'].as_str_seq())


Maybe we should leave any changes to pruning to a separate fix—let's just focus on album art for now.

That is, unless pruning for import -L works fine except for album art and this is just needed to keep the current functionality intact.

sampsyo · 2015-11-02T00:36:07Z

Here, we should probably write a test for two different situations:

No metadata changes, so the album stays in the same location.
The metadata changes so that the album needs to be moved to a new directory, and the art moved along with it.

In both cases, we should verify that we end up with exactly one image file (not two).

Also, while this strategy might work, it does have a drawback: we have to copy the image and then delete the old file. We should keep in mind an alternative, where either the importer or the fetchart plugin knows that this is a re-import and just re-associates the old image file with the new album without actually copying it.

reiv · 2015-11-03T02:05:57Z

I'm fairly certain that #1683 takes care of the first case, but it can't hurt to add more tests. As for the second case, a move could also be triggered by modifying the path templates, though I think we can treat that the same way as a metadata change.

I've taken a gander at the tests for re-imports located in test_importer.py and it looks as though it's meant for situations where files are being re-imported from outside the library, rather than from inside (correct me if I'm wrong). Any pointers on setting up a test case for the latter?

Regarding your second point: I've been thinking about this as well. A better approach might indeed be to let the importer handle it. Specifically, we would need to grab the artpath from a replaced album and assign it to the artpath of the corresponding imported album. Then calling item.move() in manipulate_files() should automatically move the art along with it.

sampsyo · 2015-11-03T06:37:31Z

I've taken a gander at the tests for re-imports located in test_importer.py and it looks as though it's meant for situations where files are being re-imported from outside the library, rather than from inside (correct me if I'm wrong). Any pointers on setting up a test case for the latter?

Hmm; the way I read it, the setUp method adds the music to the library, and then the _setup_import_session call instructs the tests to import the same music again. So think this is indeed importing already-in-the-library files, right?

Regarding your second point: I've been thinking about this as well. A better approach might indeed be to let the importer handle it. Specifically, we would need to grab the artpath from a replaced album and assign it to the artpath of the corresponding imported album. Then calling item.move() in manipulate_files() should automatically move the art along with it.

Yes, exactly! That would be ideal. The remaining decision is whether the importer should be responsible or fetchart should. I lean toward the former: a re-import can just preserve the album art as it was before. Then we would need to ensure that the fetchart plugin doesn't interfere with that logic.

Revert "PEP8 amendments" This reverts commit 413fe6b. Revert "Importer: Delete orphaned album art on..." This reverts commit 629a80a.

Ensure that album art is preserved when an album is re-imported.

Copy the replaced album's artpath attribute to the new album. This causes the image file to be moved along with the music files.

reiv · 2015-11-03T21:57:03Z

Going with this approach simplified things a lot, actually; the fix was a one-liner.

Now we'd have to make fetchart a bit smarter, too. Is there currently a way for plugins to distinguish between re-imports and regular imports?

Make sure that when an album is re-imported and its files are moved, the artwork isn't left behind in the old folder.

sampsyo · 2015-11-04T02:17:37Z

Awesome! I'm so excited the fix could be that simple. Woohoo!

It depends on when the plugins run, but ideally, a plugin should be able to inspect the task's replaced_albums field. If that's non-empty, then it's a re-import. Would that work?

reiv · 2015-11-04T12:03:15Z

Awesome! I'm so excited the fix could be that simple. Woohoo!

Me too! Turns out the mechanism was already there, it was just missing a single cog for it to work.

Anyway, if the fetchart plugin has access to the album associated with the task, then instead of ignoring re-imports altogether, we could check if an album already has its artpath set (again making sure that the image file actually exists), and if so, skip it.

When re-importing an album, we don't want fetchart to interfere with any existing album art.

reiv · 2015-11-04T17:42:44Z

Can we merge this PR?

sampsyo · 2015-11-04T17:45:49Z

Yes, looks 100% ready. Thanks again! ✨

Fix #314: Delete old album art during re-import

reiv added 2 commits November 1, 2015 16:10

Importer: Delete orphaned album art on re-imports

629a80a

First step towards fixing #314.

PEP8 amendments

413fe6b

sampsyo assigned reiv Nov 2, 2015

sampsyo changed the title ~~Fix #314: delete old album art during re-import~~ Fix #314: Delete old album art during re-import Nov 2, 2015

sampsyo reviewed Nov 2, 2015
View reviewed changes

reiv added 3 commits November 3, 2015 11:33

Revert all changes on this branch thus far

33ea9f1

Revert "PEP8 amendments" This reverts commit 413fe6b. Revert "Importer: Delete orphaned album art on..." This reverts commit 629a80a.

Add test for #314

21f926f

Ensure that album art is preserved when an album is re-imported.

Preserve album art on re-import (fixes #314)

32934bd

Copy the replaced album's artpath attribute to the new album. This causes the image file to be moved along with the music files.

Update re-import test to leave no orphaned art

314dd0e

Make sure that when an album is re-imported and its files are moved, the artwork isn't left behind in the old folder.

reiv added 3 commits November 4, 2015 15:08

fetchart: in auto, ignore albums with art

0ebc4c7

When re-importing an album, we don't want fetchart to interfere with any existing album art.

Changelog for #1681

e32dda7

Merge branch 'master' into 314-reimport-art

81a72f7

reiv added a commit that referenced this pull request Nov 4, 2015

Merge pull request #1681 from sampsyo/314-reimport-art

a7952b1

Fix #314: Delete old album art during re-import

reiv merged commit a7952b1 into master Nov 4, 2015

reiv deleted the 314-reimport-art branch November 4, 2015 17:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #314: Delete old album art during re-import #1681

Fix #314: Delete old album art during re-import #1681

sampsyo commented Nov 2, 2015

sampsyo Nov 2, 2015

sampsyo Nov 2, 2015

sampsyo commented Nov 2, 2015

reiv commented Nov 3, 2015

sampsyo commented Nov 3, 2015

reiv commented Nov 3, 2015

sampsyo commented Nov 4, 2015

reiv commented Nov 4, 2015

reiv commented Nov 4, 2015

sampsyo commented Nov 4, 2015

Fix #314: Delete old album art during re-import #1681

Fix #314: Delete old album art during re-import #1681

Conversation

sampsyo commented Nov 2, 2015

sampsyo Nov 2, 2015

Choose a reason for hiding this comment

sampsyo Nov 2, 2015

Choose a reason for hiding this comment

sampsyo commented Nov 2, 2015

reiv commented Nov 3, 2015

sampsyo commented Nov 3, 2015

reiv commented Nov 3, 2015

sampsyo commented Nov 4, 2015

reiv commented Nov 4, 2015

reiv commented Nov 4, 2015

sampsyo commented Nov 4, 2015