Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions picard/formats/apev2.py
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,11 @@ def _remove_deleted_tags(self, metadata, tags):
if real_name in tags:
del tags[real_name]

if self.metadata.images.deleted:
for tag in tags:
if tag.lower().startswith('cover art'):
del tags[tag]

def _get_tag_name(self, name):
if name in self.__casemap:
return self.__casemap[name]
Expand Down
2 changes: 2 additions & 0 deletions picard/formats/asf.py
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,8 @@ def _remove_deleted_tags(self, metadata, tags):
real_name = self._get_tag_name(tag)
if real_name and real_name in tags:
del tags[real_name]
if self.metadata.images.deleted:
del tags['WM/Picture']

@classmethod
def supports_tag(cls, name):
Expand Down
5 changes: 5 additions & 0 deletions picard/formats/id3.py
Original file line number Diff line number Diff line change
Expand Up @@ -659,6 +659,11 @@ def _remove_deleted_tags(self, metadata, tags):
except KeyError:
pass

if self.metadata.images.deleted:
for key, frame in tags.items():
if frame.FrameID == 'APIC':
del tags[key]

@classmethod
def supports_tag(cls, name):
return ((name and not name.startswith('~') and name not in UNSUPPORTED_TAGS)
Expand Down
3 changes: 3 additions & 0 deletions picard/formats/mp4.py
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,9 @@ def _remove_deleted_tags(self, metadata, tags):
if tag not in {'totaltracks', 'totaldiscs'}:
del tags[real_name]

if self.metadata.images.deleted:
del tags['covr']

@classmethod
def supports_tag(cls, name):
return (name
Expand Down
11 changes: 10 additions & 1 deletion picard/formats/vorbis.py
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,7 @@ def _save(self, filename, metadata):
base64.b64encode(picture.write()).decode('ascii'))

file.tags.update(tags)

self._clear_cover_art(file)
self._remove_deleted_tags(metadata, file.tags)

kwargs = {}
Expand Down Expand Up @@ -358,6 +358,15 @@ def _remove_deleted_tags(self, metadata, tags):
del tags[tag]
del tags[real_name]

def _clear_cover_art(self, file):
if self.metadata.images.deleted:
if self._File == mutagen.flac.FLAC:
file.clear_pictures()
else:
for tag in file:
if tag.lower().startswith('metadata_block_picture'):
del file.tags[tag]

def _get_tag_name(self, name):
if name == '~rating':
config = get_config()
Expand Down
82 changes: 80 additions & 2 deletions picard/ui/coverartbox.py
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,11 @@ def open_release_page(self):
lookup.album_lookup(self.release)


def image_delete(obj, image):
obj.metadata.images.strip_selected_image(image)
obj.metadata_images_changed.emit()


def set_image_replace(obj, coverartimage):
obj.metadata.images.strip_front_images()
obj.metadata.images.append(coverartimage)
Expand Down Expand Up @@ -457,10 +462,11 @@ def update_metadata(self):
orig_metadata = self.item.orig_metadata

if not metadata or not metadata.images:
self.cover_art.set_metadata(orig_metadata)
self.cover_art.set_metadata(None)
self.orig_cover_art.set_metadata(None)
else:
self.cover_art.set_metadata(metadata)
self.orig_cover_art.set_metadata(orig_metadata)
self.orig_cover_art.set_metadata(orig_metadata)
self.update_display()

def fetch_remote_image(self, url, fallback_data=None):
Expand Down Expand Up @@ -547,6 +553,71 @@ 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:
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

if not self.item.orig_metadata.images:
return

cover_art_list = [image.source or _("Unnamed Cover Art") for image in self.item.metadata.images]

selected_item, ok_pressed = QtWidgets.QInputDialog.getItem(self, _("Delete Cover Art"),
_("Select the cover art image to delete:"),
cover_art_list, 0, False)
if ok_pressed:
selected_image_index = cover_art_list.index(selected_item)
selected_image = self.item.metadata.images[selected_image_index]
try:
self.delete_cover_art_for_item(self.item, selected_image)
except CoverArtImageError as e:
log.error("Can't delete image: %s", e)
return

def delete_cover_art_for_item(self, item, selected_image):
zas marked this conversation as resolved.
Show resolved Hide resolved
metadata = item.metadata
if not metadata or not metadata.images:
return

debug_info = "Deleted %r from %r"

if isinstance(item, Album):
item.enable_update_metadata_images(False)
for track in item.tracks:
track.enable_update_metadata_images(False)
image_delete(track, selected_image)
for file in item.iterfiles():
image_delete(file, selected_image)
file.update(signal=False)
for track in item.tracks:
track.enable_update_metadata_images(True)
item.enable_update_metadata_images(True)
item.update(update_tracks=False)
elif isinstance(item, FileListItem):
parents = set()
item.enable_update_metadata_images(False)
image_delete(item, selected_image)
for file in item.iterfiles():
for parent in iter_file_parents(file):
parent.enable_update_metadata_images(False)
parents.add(parent)
image_delete(file, selected_image)
file.update(signal=False)
for parent in parents:
image_delete(parent, selected_image)
parent.enable_update_metadata_images(True)
if isinstance(parent, Album):
parent.update(update_tracks=False)
else:
parent.update()
item.enable_update_metadata_images(True)
item.update()
elif isinstance(item, File):
image_delete(item, selected_image)
item.update()
else:
debug_info = "Unable to delete %r from %r"

log.debug(debug_info, selected_image, item)

def _try_load_remote_image(self, url, data):
coverartimage = CoverArtImage(
url=url.toString(),
Expand Down Expand Up @@ -636,6 +707,13 @@ def contextMenuEvent(self, event):
show_more_details_action.triggered.connect(self.show_cover_art_info)
menu.addAction(show_more_details_action)

delete_cover_art_action = QtGui.QAction(_("Delete cover art"), self.parent)
if self.item and self.item.can_show_coverart and self.item.metadata.images:
delete_cover_art_action.triggered.connect(self.delete_cover_art)
else:
delete_cover_art_action.setEnabled(False)
menu.addAction(delete_cover_art_action)

if self.orig_cover_art.isVisible():
name = _("Keep original cover art")
use_orig_value_action = QtGui.QAction(name, self.parent)
Expand Down
11 changes: 11 additions & 0 deletions picard/util/imagelist.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ def __init__(self, iterable=()):
self._images = list(iterable)
self._hash_dict = {}
self._changed = True
self._deleted = False
zas marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

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


def __len__(self):
return len(self._images)
Expand All @@ -50,6 +51,7 @@ def __delitem__(self, index):

def insert(self, index, value):
self._changed = True
self._deleted = False
return self._images.insert(index, value)

def __repr__(self):
Expand Down Expand Up @@ -95,12 +97,21 @@ def strip_front_images(self):
self._images = [image for image in self._images if not image.is_front_image()]
self._changed = True

def strip_selected_image(self, image):
if image in self._images:
self._images.remove(image)
self._deleted = not self._images

def hash_dict(self):
if self._changed:
self._hash_dict = {img.datahash.hash(): img for img in self._images}
self._changed = False
return self._hash_dict

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


class ImageListState:
def __init__(self):
Expand Down
Loading