PICARD-187: Support for manually removing cover art #2377

ShubhamBhut · 2024-03-24T14:35:17Z

PICARD-187: Support for manually removing cover art

Summary

This is a…
- Bug fix
- Feature addition
- Refactoring
- Minor / simple change (like a typo)
- Other
Describe this change in 1-2 sentences:

Problem

JIRA ticket (optional): PICARD-187

Solution

Add support in all formats to allow to delete cover art tags, and modify ImageList to allow to remove desired image from ImageList

Action

ShubhamBhut · 2024-03-24T14:37:21Z

I wasn't sure whether the extra test functions (for each format) will be required or not, so they are not added. However I have manually tested with all the audio files in test/data/

zas · 2024-03-24T18:33:01Z

picard/formats/id3.py

@@ -659,6 +659,11 @@ def _remove_deleted_tags(self, metadata, tags):
            except KeyError:
                pass

+        if self.metadata.images._deleted:
+            for key, frame in list(tags.items()):


Is list() needed here?

It doesn't seem to be needed. I will remove it.

zas · 2024-03-24T18:34:11Z

picard/formats/vorbis.py

@@ -358,6 +358,17 @@ def _remove_deleted_tags(self, metadata, tags):
                        del tags[tag]
                    del tags[real_name]

+    def _clear_cover_art(self, file):
+        is_flac = self._File == mutagen.flac.FLAC


I don't think is_flac is needed here, this is used only once.

zas · 2024-03-25T09:32:38Z

@ShubhamBhut I saw your messages about this PR on IRC, please discuss issues here instead

ShubhamBhut · 2024-03-25T09:58:12Z

@ShubhamBhut I saw your messages about this PR on IRC, please discuss issues here instead

Ok. I think I rushed a little and missed an important part :
Basically, as I mentioned, the thumbnail is not updating in case of tag removal. I tried clearing pixmap_cache, and setting self.cover_art with a newly created CoverArtThumbnail but they didn't work. I think set_item() should have updated the image change. To be honest, now I am a bit lost on what part of the code is responsible for still displaying the deleted image in the thumbnail.

zas · 2024-03-25T14:55:30Z

To be honest, now I am a bit lost on what part of the code is responsible for still displaying the deleted image in the thumbnail.

Keep in mind the displayed image is just a visual thing, a pixmap, so it has to be updated after the actual image was removed from metadata, but some care is needed because the same image might be used multiple times (in various tracks, or albums).

Have a look at https://github.com/metabrainz/picard/blob/7d97ff12043b80d8e9bad80423e6d8d2effacd71/picard/util/imagelist.py#L273C1-L286

ShubhamBhut · 2024-03-29T07:00:59Z

Apologies for such late follow up, I tried a lot of different implementations. Now deletion seems to be done perfectly. But the thumbnail update still remains. For some reason, self.cover_art.set_metadata(metadata), sets the metadata correctly but it is overwritten by something else and the thumbnail goes back to the (deleted) cover art image.

zas · 2024-03-29T09:38:08Z

picard/ui/coverartbox.py

@@ -545,6 +550,43 @@ def load_remote_image(self, url, data):
            log.warning("Can't load image: %s", e)
            return

+    def delete_cover_art(self):
+        if not self.item or not self.item.metadata.images:


There's an important flaw here:

if an album was just loaded, and no image was downloaded (it has only embedded artwork), actual metadata that contains images is self.item.orig_metadata and self.item.metadata is empty.

This might be a bit tricky to solve I think. If we remove cover art the right thing is to actually update item.metadata. item.orig_metadata should not be touched here, as it is the original data.

Now I think what happens is that once all images have been removed from item.metadata the cover art box assumes there are no new images and falls back to show the images from orig_metadata (this happens in CoverArtBox.update_metadata.

The correct thing then would be to not do this fallback if self.item.metadata.images.delete == True.

picard/ui/coverartbox.py

picard/util/imagelist.py

picard/ui/coverartbox.py

picard/util/imagelist.py

phw

I added a comment on how I think the cover art box display issue needs to be solved. Otherwise this is looking good now.

It's really one of the issues that looks simple first but is rather tricky to get right.

phw · 2024-04-18T10:40:42Z

picard/ui/coverartbox.py

@@ -545,6 +550,43 @@ def load_remote_image(self, url, data):
            log.warning("Can't load image: %s", e)
            return

+    def delete_cover_art(self):
+        if not self.item or not self.item.metadata.images:


This might be a bit tricky to solve I think. If we remove cover art the right thing is to actually update item.metadata. item.orig_metadata should not be touched here, as it is the original data.

Now I think what happens is that once all images have been removed from item.metadata the cover art box assumes there are no new images and falls back to show the images from orig_metadata (this happens in CoverArtBox.update_metadata.

The correct thing then would be to not do this fallback if self.item.metadata.images.delete == True.

phw · 2024-04-18T10:43:29Z

picard/util/imagelist.py

@@ -33,6 +33,7 @@ def __init__(self, iterable=()):
        self._images = list(iterable)
        self._hash_dict = {}
        self._changed = True
+        self._deleted = False


I would add a deleted property to the class to access this internal variable. Then other code does not need to access a variable that is supposed to be internal to the imagelist implementation.

@property def deleted(self): return self._deleted

rdswift · 2024-04-18T14:52:45Z

Added PICARD-2865 to cover updating the documentation.

rdswift · 2024-04-18T14:55:29Z

@ShubhamBhut, you should probably update the PICARD-187 ticket to show it being assigned to you so nobody else accidentally starts working on it.

rdswift · 2024-04-18T15:40:52Z

picard/ui/coverartbox.py

+        if not metadata or not metadata.images:
+            return
+
+        debug_info = "Deleting %r from %r"


Should this be "Deleted %r from %r", since it isn't logged until the deletion is complete?

rdswift · 2024-04-18T15:43:07Z

picard/ui/coverartbox.py

+            image_delete(item, selected_image)
+            item.update()
+        else:
+            debug_info = "Dropping %r to %r is not handled"


For consistency in terminology, would it be more appropriate to change this to something like "Unable to delete %r from %r"?

PICARD-187: Support for manually removing cover art finishing PICARD-187

ShubhamBhut force-pushed the PICARD-187 branch from 0c17901 to 71f21d6 Compare March 24, 2024 14:44

zas reviewed Mar 24, 2024

View reviewed changes

ShubhamBhut force-pushed the PICARD-187 branch 2 times, most recently from 7ebc65d to f16ed33 Compare March 29, 2024 06:51

zas requested changes Mar 29, 2024

View reviewed changes

ShubhamBhut force-pushed the PICARD-187 branch 4 times, most recently from d3df50e to d328ea3 Compare April 5, 2024 03:14

ShubhamBhut force-pushed the PICARD-187 branch from d328ea3 to 7b6749f Compare April 7, 2024 01:49

zas requested review from phw and rdswift April 7, 2024 09:35

phw added the Feature label Apr 12, 2024

phw requested changes Apr 18, 2024

View reviewed changes

rdswift reviewed Apr 18, 2024

View reviewed changes

ShubhamBhut force-pushed the PICARD-187 branch from 7b6749f to 3c46364 Compare April 26, 2024 08:28

PICARD-187: Support for manually removing cover art

0fc0b5f

PICARD-187: Support for manually removing cover art finishing PICARD-187

ShubhamBhut force-pushed the PICARD-187 branch from 3c46364 to 0fc0b5f Compare April 27, 2024 14:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PICARD-187: Support for manually removing cover art #2377

PICARD-187: Support for manually removing cover art #2377

ShubhamBhut commented Mar 24, 2024

ShubhamBhut commented Mar 24, 2024

zas Mar 24, 2024

ShubhamBhut Mar 25, 2024

zas Mar 24, 2024

zas commented Mar 25, 2024

ShubhamBhut commented Mar 25, 2024

zas commented Mar 25, 2024

ShubhamBhut commented Mar 29, 2024

zas Mar 29, 2024

phw Apr 18, 2024

phw left a comment

phw Apr 18, 2024

phw Apr 18, 2024

rdswift commented Apr 18, 2024

rdswift commented Apr 18, 2024

rdswift Apr 18, 2024

rdswift Apr 18, 2024

PICARD-187: Support for manually removing cover art #2377

Are you sure you want to change the base?

PICARD-187: Support for manually removing cover art #2377

Conversation

ShubhamBhut commented Mar 24, 2024

Summary

Problem

Solution

Action

ShubhamBhut commented Mar 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zas commented Mar 25, 2024

ShubhamBhut commented Mar 25, 2024

zas commented Mar 25, 2024

ShubhamBhut commented Mar 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdswift commented Apr 18, 2024

rdswift commented Apr 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment