From 3b8b0e7df7ea5ff25cc1758263ef39e58315b97e Mon Sep 17 00:00:00 2001 From: Wieland Hoffmann Date: Sun, 18 Aug 2013 22:36:54 +0200 Subject: [PATCH 01/23] Use a separate class for dealing with images This new class stores the image data in a tempfile.TemporaryFile so we don't keep the data for all images that are used somewhere in memory anymore. It also deals with saving images onto disk when the corresponding tracks are saved (this was previously done by the File class). This removes the ability to use tagger script in the cover file name (cf. the first line of File._make_image_filename). --- picard/file.py | 49 +--------------- picard/formats/apev2.py | 4 +- picard/formats/asf.py | 13 ++--- picard/formats/id3.py | 15 ++--- picard/formats/mp4.py | 6 +- picard/formats/vorbis.py | 23 +++----- picard/metadata.py | 120 +++++++++++++++++++++++++++++++++------ picard/ui/coverartbox.py | 5 +- picard/ui/infodialog.py | 2 +- test/test_formats.py | 4 +- 10 files changed, 135 insertions(+), 106 deletions(-) diff --git a/picard/file.py b/picard/file.py index fb225b612..6bc55dc04 100644 --- a/picard/file.py +++ b/picard/file.py @@ -314,60 +314,13 @@ class File(QtCore.QObject, Item): shutil.move(encode_filename(old_filename), encode_filename(new_filename)) return new_filename - def _make_image_filename(self, image_filename, dirname, metadata): - image_filename = self._script_to_filename(image_filename, metadata) - if not image_filename: - image_filename = "cover" - if os.path.isabs(image_filename): - filename = image_filename - else: - filename = os.path.join(dirname, image_filename) - if config.setting['windows_compatibility'] or sys.platform == 'win32': - filename = filename.replace('./', '_/').replace('.\\', '_\\') - return encode_filename(filename) - def _save_images(self, dirname, metadata): """Save the cover images to disk.""" if not metadata.images: return - default_filename = self._make_image_filename( - config.setting["cover_image_filename"], dirname, metadata) - overwrite = config.setting["save_images_overwrite"] counters = defaultdict(lambda: 0) for image in metadata.images: - filename = image["filename"] - data = image["data"] - mime = image["mime"] - if filename is None: - filename = default_filename - else: - filename = self._make_image_filename(filename, dirname, metadata) - image_filename = filename - ext = mimetype.get_extension(mime, ".jpg") - if counters[filename] > 0: - image_filename = "%s (%d)" % (filename, counters[filename]) - counters[filename] = counters[filename] + 1 - while os.path.exists(image_filename + ext) and not overwrite: - if os.path.getsize(image_filename + ext) == len(data): - log.debug("Identical file size, not saving %r", image_filename) - break - image_filename = "%s (%d)" % (filename, counters[filename]) - counters[filename] = counters[filename] + 1 - else: - new_filename = image_filename + ext - # Even if overwrite is enabled we don't need to write the same - # image multiple times - if (os.path.exists(new_filename) and - os.path.getsize(new_filename) == len(data)): - log.debug("Identical file size, not saving %r", image_filename) - continue - log.debug("Saving cover images to %r", image_filename) - new_dirname = os.path.dirname(image_filename) - if not os.path.isdir(new_dirname): - os.makedirs(new_dirname) - f = open(image_filename + ext, "wb") - f.write(data) - f.close() + image.save(dirname, metadata, counters) def _move_additional_files(self, old_filename, new_filename): """Move extra files, like playlists...""" diff --git a/picard/formats/apev2.py b/picard/formats/apev2.py index eadcce5de..f36903161 100644 --- a/picard/formats/apev2.py +++ b/picard/formats/apev2.py @@ -150,8 +150,8 @@ class APEv2File(File): if not save_this_image_to_tags(image): continue cover_filename = 'Cover Art (Front)' - cover_filename += mimetype.get_extension(image["mime"], '.jpg') - tags['Cover Art (Front)'] = mutagen.apev2.APEValue(cover_filename + '\0' + image["data"], mutagen.apev2.BINARY) + cover_filename += mimetype.get_extension(image.mimetype, '.jpg') + tags['Cover Art (Front)'] = mutagen.apev2.APEValue(cover_filename + '\0' + image.data, mutagen.apev2.BINARY) break # can't save more than one item with the same name # (mp3tags does this, but it's against the specs) tags.save(encode_filename(filename)) diff --git a/picard/formats/asf.py b/picard/formats/asf.py index f78ff782c..0176b031b 100644 --- a/picard/formats/asf.py +++ b/picard/formats/asf.py @@ -140,11 +140,8 @@ class ASFFile(File): if name == 'WM/Picture': for image in values: (mime, data, type, description) = unpack_image(image.value) - extras = { - 'desc': description, - 'type': image_type_from_id3_num(type) - } - metadata.add_image(mime, data, extras=extras) + metadata.add_image(mime, data, comment=description, + imagetype=image_type_from_id3_num(type)) continue elif name not in self.__RTRANS: continue @@ -169,9 +166,9 @@ class ASFFile(File): for image in metadata.images: if not save_this_image_to_tags(image): continue - tag_data = pack_image(image["mime"], image["data"], - image_type_as_id3_num(image['type']), - image['desc']) + tag_data = pack_image(image.mimetype, image.data, + image_type_as_id3_num(image.imagetype), + image.description) cover.append(ASFByteArrayAttribute(tag_data)) if cover: file.tags['WM/Picture'] = cover diff --git a/picard/formats/id3.py b/picard/formats/id3.py index a1acf327b..ca2c810b0 100644 --- a/picard/formats/id3.py +++ b/picard/formats/id3.py @@ -249,11 +249,8 @@ class ID3File(File): else: metadata['discnumber'] = value[0] elif frameid == 'APIC': - extras = { - 'desc': frame.desc, - 'type': image_type_from_id3_num(frame.type) - } - metadata.add_image(frame.mime, frame.data, extras=extras) + metadata.add_image(frame.mime, frame.data, comment=frame.desc, + imagetype=image_type_from_id3_num(frame.type)) elif frameid == 'POPM': # Rating in ID3 ranges from 0 to 255, normalize this to the range 0 to 5 if frame.email == config.setting['rating_user_email']: @@ -308,7 +305,7 @@ class ID3File(File): # any description. counters = defaultdict(lambda: 0) for image in metadata.images: - desc = desctag = image['desc'] + desc = desctag = image.description if not save_this_image_to_tags(image): continue if counters[desc] > 0: @@ -318,10 +315,10 @@ class ID3File(File): desctag = "(%i)" % counters[desc] counters[desc] += 1 tags.add(id3.APIC(encoding=0, - mime=image["mime"], - type=image_type_as_id3_num(image['type']), + mime=image.mimetype, + type=image_type_as_id3_num(image.imagetype), desc=desctag, - data=image["data"])) + data=image.data)) tmcl = mutagen.id3.TMCL(encoding=encoding, people=[]) tipl = mutagen.id3.TIPL(encoding=encoding, people=[]) diff --git a/picard/formats/mp4.py b/picard/formats/mp4.py index 91df4fed4..f7d6ead2f 100644 --- a/picard/formats/mp4.py +++ b/picard/formats/mp4.py @@ -194,11 +194,11 @@ class MP4File(File): for image in metadata.images: if not save_this_image_to_tags(image): continue - mime = image["mime"] + mime = image.mimetype if mime == "image/jpeg": - covr.append(MP4Cover(image["data"], MP4Cover.FORMAT_JPEG)) + covr.append(MP4Cover(image.data, MP4Cover.FORMAT_JPEG)) elif mime == "image/png": - covr.append(MP4Cover(image["data"], MP4Cover.FORMAT_PNG)) + covr.append(MP4Cover(image.data, MP4Cover.FORMAT_PNG)) if covr: file.tags["covr"] = covr diff --git a/picard/formats/vorbis.py b/picard/formats/vorbis.py index 538005ec3..ce4d722d3 100644 --- a/picard/formats/vorbis.py +++ b/picard/formats/vorbis.py @@ -96,22 +96,17 @@ class VCommentFile(File): name = "totaldiscs" elif name == "metadata_block_picture": image = mutagen.flac.Picture(base64.standard_b64decode(value)) - extras = { - 'desc': image.desc, - 'type': image_type_from_id3_num(image.type) - } - metadata.add_image(image.mime, image.data, extras=extras) + metadata.add_image(image.mime, image.data, + comment=image.desc, + imagetype=image_type_from_id3_num(image.type)) continue elif name in self.__translate: name = self.__translate[name] metadata.add(name, value) if self._File == mutagen.flac.FLAC: for image in file.pictures: - extras = { - 'desc': image.desc, - 'type': image_type_from_id3_num(image.type) - } - metadata.add_image(image.mime, image.data, extras=extras) + metadata.add_image(image.mime, image.data, comment=image.desc, + imagetype=image_type_from_id3_num(image.type)) # Read the unofficial COVERART tags, for backward compatibillity only if not "metadata_block_picture" in file.tags: try: @@ -175,10 +170,10 @@ class VCommentFile(File): if not save_this_image_to_tags(image): continue picture = mutagen.flac.Picture() - picture.data = image["data"] - picture.mime = image["mime"] - picture.desc = image['desc'] - picture.type = image_type_as_id3_num(image['type']) + picture.data = image.data + picture.mime = image.mimetype + picture.desc = image.description + picture.type = image_type_as_id3_num(image.imagetype) if self._File == mutagen.flac.FLAC: file.add_picture(picture) else: diff --git a/picard/metadata.py b/picard/metadata.py index a70b79b49..5e351789b 100644 --- a/picard/metadata.py +++ b/picard/metadata.py @@ -15,13 +15,26 @@ # # You should have received a copy of the GNU General Public License # along with this program; if not, write to the Free Software -# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, +# USA. +import os.path +import shutil +import sys +import tempfile + from PyQt4.QtCore import QObject -from picard import config +from picard import config, log from picard.plugin import ExtensionPoint from picard.similarity import similarity2 -from picard.util import load_release_type_scores +from picard.util import ( + encode_filename, + load_release_type_scores, + mimetype as mime, + replace_non_ascii, + replace_win32_incompat, + unaccent, +) from picard.mbxml import artist_credit_from_node MULTI_VALUED_JOINER = '; ' @@ -44,6 +57,88 @@ def save_this_image_to_tags(image): return False +class Image(object): + """Wrapper around images.""" + + def __init__(self, data, mimetype="image/jpeg", imagetype="front", + comment=""): + self.description = comment + self._datafile = tempfile.TemporaryFile() + self._datafile.write(data) + self.datalength = len(data) + self.imagetype = imagetype + self.is_front_image = imagetype == "front" + self.mimetype = mimetype + self.extension = mime.get_extension(mime, ".jpg") + + def _make_image_filename(self, filename, dirname, metadata): + if config.setting["ascii_filenames"]: + if isinstance(filename, unicode): + filename = unaccent(filename) + filename = replace_non_ascii(filename) + if not filename: + filename = "cover" + if not os.path.isabs(filename): + filename = os.path.join(dirname, filename) + # replace incompatible characters + if config.setting["windows_compatibility"] or sys.platform == "win32": + filename = replace_win32_incompat(filename) + # remove null characters + filename = filename.replace("\x00", "") + return encode_filename(filename) + + def save(self, dirname, metadata, counters): + """Saves this image. + + :dirname: The name of the directory that contains the audio file + :metadata: A metadata object + :counters: A dictionary mapping filenames to the amount of how many + images with that filename were already saved in `dirname`. + """ + self._datafile.seek(0, 0) + + if config.setting["caa_image_type_as_filename"]: + log.debug("Using image type %s", self.imagetype) + filename = self._make_image_filename(self.imagetype, dirname, metadata) + else: + log.debug("Using default file name %s", + config.setting["cover_image_filename"]) + filename = self._make_image_filename( + config.setting["cover_image_filename"], dirname, metadata) + + overwrite = config.setting["save_images_overwrite"] + ext = self.extension + image_filename = filename + if counters[filename] > 0: + image_filename = "%s (%d)" % (filename, counters[filename]) + counters[filename] = counters[filename] + 1 + while os.path.exists(image_filename + ext) and not overwrite: + if os.path.getsize(image_filename + ext) == self.datalength: + log.debug("Identical file size, not saving %r", image_filename) + break + image_filename = "%s (%d)" % (filename, counters[filename]) + counters[filename] = counters[filename] + 1 + else: + new_filename = image_filename + ext + # Even if overwrite is enabled we don't need to write the same + # image multiple times + if (os.path.exists(new_filename) and + os.path.getsize(new_filename) == self.datalength): + log.debug("Identical file size, not saving %r", image_filename) + return + log.debug("Saving cover images to %r", image_filename) + new_dirname = os.path.dirname(image_filename) + if not os.path.isdir(new_dirname): + os.makedirs(new_dirname) + with open(image_filename + ext, "wb") as newfile: + shutil.copyfileobj(self._datafile, newfile) + + @property + def data(self): + self._datafile.seek(0, 0) + return self._datafile.read() + + class Metadata(dict): """List of metadata items with dict-like access.""" @@ -61,26 +156,19 @@ class Metadata(dict): self.images = [] self.length = 0 - def add_image(self, mime, data, filename=None, extras=None): + def add_image(self, mime, data, filename=None, comment="", + imagetype="front"): """Adds the image ``data`` to this Metadata object. Arguments: mime -- The mimetype of the image data -- The image data filename -- The image filename, without an extension - extras -- extra informations about image as dict - 'desc' : image description or comment, default to '' - 'type' : main type as a string, default to 'front' - 'front': if set, CAA front flag is true for this image + comment -- image description or comment, default to '' + imagetype -- main type as a string, default to 'front' """ - imagedict = {'mime': mime, - 'data': data, - 'filename': filename, - 'type': 'front', - 'desc': ''} - if extras is not None: - imagedict.update(extras) - self.images.append(imagedict) + image = Image(data, mime, imagetype, comment) + self.images.append(image) def remove_image(self, index): self.images.pop(index) diff --git a/picard/ui/coverartbox.py b/picard/ui/coverartbox.py index 475fbb905..49fc743b7 100644 --- a/picard/ui/coverartbox.py +++ b/picard/ui/coverartbox.py @@ -23,7 +23,6 @@ from picard import config, log from picard.album import Album from picard.track import Track from picard.file import File -from picard.metadata import is_front_image from picard.util import webbrowser2, encode_filename @@ -104,7 +103,7 @@ class CoverArtBox(QtGui.QGroupBox): if self.data: if pixmap is None: pixmap = QtGui.QPixmap() - pixmap.loadFromData(self.data["data"]) + pixmap.loadFromData(self.data.data) if not pixmap.isNull(): offx, offy, w, h = (1, 1, 121, 121) cover = QtGui.QPixmap(self.shadow) @@ -123,7 +122,7 @@ class CoverArtBox(QtGui.QGroupBox): data = None if metadata and metadata.images: for image in metadata.images: - if is_front_image(image): + if image.is_front_image: data = image break else: diff --git a/picard/ui/infodialog.py b/picard/ui/infodialog.py index b45f98f3c..71d5c14d1 100644 --- a/picard/ui/infodialog.py +++ b/picard/ui/infodialog.py @@ -47,7 +47,7 @@ class InfoDialog(QtGui.QDialog): return for image in images: - data = image["data"] + data = image.data size = len(data) item = QtGui.QListWidgetItem() pixmap = QtGui.QPixmap() diff --git a/test/test_formats.py b/test/test_formats.py index 2c0cfa052..2b24b475b 100644 --- a/test/test_formats.py +++ b/test/test_formats.py @@ -550,7 +550,7 @@ class TestCoverArt(unittest.TestCase): f = picard.formats.open(self.filename) loaded_metadata = f._load(self.filename) image = loaded_metadata.images[0] - self.assertEqual(image["mime"], tests[t]['mime']) - self.assertEqual(image["data"], imgdata) + self.assertEqual(image.mimetype, tests[t]['mime']) + self.assertEqual(image.data, imgdata) finally: self._tear_down() From 01cefae96e122bdcb6a7d74193dc66be816fa00c Mon Sep 17 00:00:00 2001 From: Wieland Hoffmann Date: Wed, 22 Jan 2014 23:27:08 +0100 Subject: [PATCH 02/23] Use the same Image objects for all tracks of an album --- picard/coverart.py | 7 ++++--- picard/formats/apev2.py | 2 +- picard/formats/asf.py | 2 +- picard/formats/id3.py | 2 +- picard/formats/mp4.py | 4 ++-- picard/formats/vorbis.py | 6 +++--- picard/metadata.py | 12 +++++++++--- picard/ui/coverartbox.py | 12 ++++++------ test/test_formats.py | 2 +- 9 files changed, 28 insertions(+), 21 deletions(-) diff --git a/picard/coverart.py b/picard/coverart.py index fbbf45bd2..b5935bc02 100644 --- a/picard/coverart.py +++ b/picard/coverart.py @@ -26,7 +26,7 @@ import re import traceback from functools import partial from picard import config, log -from picard.metadata import is_front_image +from picard.metadata import Image, is_front_image from picard.util import mimetype, parse_amazon_url from picard.const import CAA_HOST, CAA_PORT from PyQt4.QtCore import QUrl, QObject @@ -101,9 +101,10 @@ def _coverart_downloaded(album, metadata, release, try_list, coverinfos, data, h filename = None if not is_front_image(coverinfos) and config.setting["caa_image_type_as_filename"]: filename = coverinfos['type'] - metadata.add_image(mime, data, filename, coverinfos) + image = Image(data, mime, coverinfos['type'], coverinfos['desc']) + metadata.add_image(image) for track in album._new_tracks: - track.metadata.add_image(mime, data, filename, coverinfos) + track.metadata.add_image(image) # If the image already was a front image, there might still be some # other front images in the try_list - remove them. diff --git a/picard/formats/apev2.py b/picard/formats/apev2.py index f36903161..e358e5a1c 100644 --- a/picard/formats/apev2.py +++ b/picard/formats/apev2.py @@ -63,7 +63,7 @@ class APEv2File(File): if '\0' in values.value: descr, data = values.value.split('\0', 1) mime = mimetype.get_from_data(data, descr, 'image/jpeg') - metadata.add_image(mime, data) + metadata.make_and_add_image(mime, data) # skip EXTERNAL and BINARY values if values.kind != mutagen.apev2.TEXT: continue diff --git a/picard/formats/asf.py b/picard/formats/asf.py index 0176b031b..e5357531b 100644 --- a/picard/formats/asf.py +++ b/picard/formats/asf.py @@ -140,7 +140,7 @@ class ASFFile(File): if name == 'WM/Picture': for image in values: (mime, data, type, description) = unpack_image(image.value) - metadata.add_image(mime, data, comment=description, + metadata.make_and_add_image(mime, data, comment=description, imagetype=image_type_from_id3_num(type)) continue elif name not in self.__RTRANS: diff --git a/picard/formats/id3.py b/picard/formats/id3.py index ca2c810b0..3d0158fdb 100644 --- a/picard/formats/id3.py +++ b/picard/formats/id3.py @@ -249,7 +249,7 @@ class ID3File(File): else: metadata['discnumber'] = value[0] elif frameid == 'APIC': - metadata.add_image(frame.mime, frame.data, comment=frame.desc, + metadata.make_and_add_image(frame.mime, frame.data, comment=frame.desc, imagetype=image_type_from_id3_num(frame.type)) elif frameid == 'POPM': # Rating in ID3 ranges from 0 to 255, normalize this to the range 0 to 5 diff --git a/picard/formats/mp4.py b/picard/formats/mp4.py index f7d6ead2f..576f92cf4 100644 --- a/picard/formats/mp4.py +++ b/picard/formats/mp4.py @@ -141,9 +141,9 @@ class MP4File(File): elif name == "covr": for value in values: if value.imageformat == value.FORMAT_JPEG: - metadata.add_image("image/jpeg", value) + metadata.make_and_add_image("image/jpeg", value) elif value.imageformat == value.FORMAT_PNG: - metadata.add_image("image/png", value) + metadata.make_and_add_image("image/png", value) self._info(metadata, file) return metadata diff --git a/picard/formats/vorbis.py b/picard/formats/vorbis.py index ce4d722d3..7635997ec 100644 --- a/picard/formats/vorbis.py +++ b/picard/formats/vorbis.py @@ -96,7 +96,7 @@ class VCommentFile(File): name = "totaldiscs" elif name == "metadata_block_picture": image = mutagen.flac.Picture(base64.standard_b64decode(value)) - metadata.add_image(image.mime, image.data, + metadata.make_and_add_image(image.mime, image.data, comment=image.desc, imagetype=image_type_from_id3_num(image.type)) continue @@ -105,13 +105,13 @@ class VCommentFile(File): metadata.add(name, value) if self._File == mutagen.flac.FLAC: for image in file.pictures: - metadata.add_image(image.mime, image.data, comment=image.desc, + metadata.make_and_add_image(image.mime, image.data, comment=image.desc, imagetype=image_type_from_id3_num(image.type)) # Read the unofficial COVERART tags, for backward compatibillity only if not "metadata_block_picture" in file.tags: try: for index, data in enumerate(file["COVERART"]): - metadata.add_image(file["COVERARTMIME"][index], + metadata.make_and_add_image(file["COVERARTMIME"][index], base64.standard_b64decode(data) ) except KeyError: diff --git a/picard/metadata.py b/picard/metadata.py index 5e351789b..a1f6ef87d 100644 --- a/picard/metadata.py +++ b/picard/metadata.py @@ -156,9 +156,15 @@ class Metadata(dict): self.images = [] self.length = 0 - def add_image(self, mime, data, filename=None, comment="", - imagetype="front"): - """Adds the image ``data`` to this Metadata object. + def add_image(self, image): + """Adds the Image object ``image`` to tis Metadata object. + """ + self.images.append(image) + + def make_and_add_image(self, mime, data, filename=None, comment="", + imagetype="front"): + """Build a new image object from ``data`` and adds it to this Metadata + object. Arguments: mime -- The mimetype of the image diff --git a/picard/ui/coverartbox.py b/picard/ui/coverartbox.py index 49fc743b7..1e2c6cc37 100644 --- a/picard/ui/coverartbox.py +++ b/picard/ui/coverartbox.py @@ -184,16 +184,16 @@ class CoverArtBox(QtGui.QGroupBox): self.__set_data([mime, data], pixmap=pixmap) if isinstance(self.item, Album): album = self.item - album.metadata.add_image(mime, data) + album.metadata.make_and_add_image(mime, data) for track in album.tracks: - track.metadata.add_image(mime, data) + track.metadata.make_and_add_image(mime, data) for file in album.iterfiles(): - file.metadata.add_image(mime, data) + file.metadata.make_and_add_image(mime, data) elif isinstance(self.item, Track): track = self.item - track.metadata.add_image(mime, data) + track.metadata.make_and_add_image(mime, data) for file in track.iterfiles(): - file.metadata.add_image(mime, data) + file.metadata.make_and_add_image(mime, data) elif isinstance(self.item, File): file = self.item - file.metadata.add_image(mime, data) + file.metadata.make_and_add_image(mime, data) diff --git a/test/test_formats.py b/test/test_formats.py index 2b24b475b..866c1e58b 100644 --- a/test/test_formats.py +++ b/test/test_formats.py @@ -544,7 +544,7 @@ class TestCoverArt(unittest.TestCase): f = picard.formats.open(self.filename) metadata = Metadata() imgdata = tests[t]['head'] + dummyload - metadata.add_image(tests[t]['mime'], imgdata) + metadata.make_and_add_image(tests[t]['mime'], imgdata) f._save(self.filename, metadata) f = picard.formats.open(self.filename) From 87153ed7524d9e25670ee9fdd6fd0b9bdd4eaf00 Mon Sep 17 00:00:00 2001 From: Wieland Hoffmann Date: Thu, 23 Jan 2014 00:08:58 +0100 Subject: [PATCH 03/23] Catch IOErrors and OSErrors when creating Images They can happen if the disk is full or the user is not allowed to access the folder where tempfile.TemporaryFile wants to create the file. --- picard/coverart.py | 10 +++++++++- picard/metadata.py | 4 +++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/picard/coverart.py b/picard/coverart.py index b5935bc02..6df09bbd1 100644 --- a/picard/coverart.py +++ b/picard/coverart.py @@ -101,7 +101,15 @@ def _coverart_downloaded(album, metadata, release, try_list, coverinfos, data, h filename = None if not is_front_image(coverinfos) and config.setting["caa_image_type_as_filename"]: filename = coverinfos['type'] - image = Image(data, mime, coverinfos['type'], coverinfos['desc']) + try: + image = Image(data, mime, coverinfos['type'], coverinfos['desc']) + except (IOError, OSError), e: + album.error_append(e.message) + album._finalize_loading(errorTrue + # It doesn't make sense to store/download more images if we can't + # save them in the temporary folder, abort. + return + metadata.add_image(image) for track in album._new_tracks: track.metadata.add_image(image) diff --git a/picard/metadata.py b/picard/metadata.py index a1f6ef87d..b30eeb9b1 100644 --- a/picard/metadata.py +++ b/picard/metadata.py @@ -58,7 +58,9 @@ def save_this_image_to_tags(image): class Image(object): - """Wrapper around images.""" + """Wrapper around images. Instantiating an object of this class can raise + an IOError or OSError due to the usage of tempfiles underneath. + """ def __init__(self, data, mimetype="image/jpeg", imagetype="front", comment=""): From 9412d4b07e29ac9d319517ff37e9769972747747 Mon Sep 17 00:00:00 2001 From: Wieland Hoffmann Date: Thu, 23 Jan 2014 00:49:58 +0100 Subject: [PATCH 04/23] Fix a typo Not sure how that happened. --- picard/coverart.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/picard/coverart.py b/picard/coverart.py index 6df09bbd1..142e379d3 100644 --- a/picard/coverart.py +++ b/picard/coverart.py @@ -105,7 +105,7 @@ def _coverart_downloaded(album, metadata, release, try_list, coverinfos, data, h image = Image(data, mime, coverinfos['type'], coverinfos['desc']) except (IOError, OSError), e: album.error_append(e.message) - album._finalize_loading(errorTrue + album._finalize_loading(error=True) # It doesn't make sense to store/download more images if we can't # save them in the temporary folder, abort. return From 05e844da01ab5caa43addc500b7011af8c8ab0ac Mon Sep 17 00:00:00 2001 From: Wieland Hoffmann Date: Thu, 23 Jan 2014 11:17:49 +0100 Subject: [PATCH 05/23] Fix embedding only front images by using the images is_front_image attribute --- picard/metadata.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/picard/metadata.py b/picard/metadata.py index b30eeb9b1..914ea1dd5 100644 --- a/picard/metadata.py +++ b/picard/metadata.py @@ -52,9 +52,7 @@ def is_front_image(image): def save_this_image_to_tags(image): if not config.setting["save_only_front_images_to_tags"]: return True - if is_front_image(image): - return True - return False + return image.is_front_image class Image(object): From bc8b2c89f12dc9e0c0991ec15486baa2f79b9706 Mon Sep 17 00:00:00 2001 From: Wieland Hoffmann Date: Thu, 23 Jan 2014 11:18:38 +0100 Subject: [PATCH 06/23] tis -> this --- picard/metadata.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/picard/metadata.py b/picard/metadata.py index 914ea1dd5..75cd380f9 100644 --- a/picard/metadata.py +++ b/picard/metadata.py @@ -157,7 +157,7 @@ class Metadata(dict): self.length = 0 def add_image(self, image): - """Adds the Image object ``image`` to tis Metadata object. + """Adds the Image object ``image`` to this Metadata object. """ self.images.append(image) From 111afc4fd9876053fe1b77e1e95b068807342098 Mon Sep 17 00:00:00 2001 From: Wieland Hoffmann Date: Wed, 29 Jan 2014 19:38:02 +0100 Subject: [PATCH 07/23] Use tempfile.mkstemp to get a closable tempfile This makes sure we don't run out of file descriptors if many images get loaded into Picard. --- picard/metadata.py | 33 +++++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-) diff --git a/picard/metadata.py b/picard/metadata.py index 75cd380f9..ad2f8f39c 100644 --- a/picard/metadata.py +++ b/picard/metadata.py @@ -23,6 +23,7 @@ import sys import tempfile +from os import fdopen, close, unlink from PyQt4.QtCore import QObject from picard import config, log from picard.plugin import ExtensionPoint @@ -63,8 +64,9 @@ class Image(object): def __init__(self, data, mimetype="image/jpeg", imagetype="front", comment=""): self.description = comment - self._datafile = tempfile.TemporaryFile() - self._datafile.write(data) + (fd, self._filename) = tempfile.mkstemp(prefix="picard") + with fdopen(fd, "wb") as imagefile: + imagefile.write(data) self.datalength = len(data) self.imagetype = imagetype self.is_front_image = imagetype == "front" @@ -95,8 +97,6 @@ class Image(object): :counters: A dictionary mapping filenames to the amount of how many images with that filename were already saved in `dirname`. """ - self._datafile.seek(0, 0) - if config.setting["caa_image_type_as_filename"]: log.debug("Using image type %s", self.imagetype) filename = self._make_image_filename(self.imagetype, dirname, metadata) @@ -130,13 +130,30 @@ class Image(object): new_dirname = os.path.dirname(image_filename) if not os.path.isdir(new_dirname): os.makedirs(new_dirname) - with open(image_filename + ext, "wb") as newfile: - shutil.copyfileobj(self._datafile, newfile) + shutil.copyfile(self._filename, image_filename) @property def data(self): - self._datafile.seek(0, 0) - return self._datafile.read() + """Reads the data from the temporary file created for this image. May + raise IOErrors or OSErrors. + """ + with open(self._filename, "rb") as imagefile: + return imagefile.read() + + def __del__(self): + """Makes sure that the file created to hold this images data is + deleted. + """ + # http://docs.python.org/2/reference/datamodel.html?highlight=__del__#object.__del__ + # Due to the precarious circumstances under which __del__() methods are + # invoked, exceptions that occur during their execution are ignored, + # and a warning is printed to sys.stderr instead. + # + # This really means that wrapping the following unlink call in + # try-except statements will not let us catch the exception. + unlink(self._filename) + super(Image, self).__del__(self) + class Metadata(dict): From 82d83a95f0f9b488bfc3a56c79933a05883cc970 Mon Sep 17 00:00:00 2001 From: Wieland Hoffmann Date: Thu, 30 Jan 2014 00:54:09 +0100 Subject: [PATCH 08/23] Remove a super(...).__del__ call `object` does not actually have a __del__ method --- picard/metadata.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/picard/metadata.py b/picard/metadata.py index ad2f8f39c..2ac7103f8 100644 --- a/picard/metadata.py +++ b/picard/metadata.py @@ -152,8 +152,6 @@ class Image(object): # This really means that wrapping the following unlink call in # try-except statements will not let us catch the exception. unlink(self._filename) - super(Image, self).__del__(self) - class Metadata(dict): From 4aba28ad60bbb8ca5c76bced9fbdda622d1cc206 Mon Sep 17 00:00:00 2001 From: Wieland Hoffmann Date: Thu, 30 Jan 2014 17:19:24 +0100 Subject: [PATCH 09/23] Use md5 hashes to make sure every images is only saved once make_and_add_image will use already existing images from Tagger.images if the hash of the image is the same as that of an image in there. --- picard/coverart.py | 13 ++++++++----- picard/metadata.py | 22 +++++++++++++--------- picard/tagger.py | 2 ++ picard/ui/infodialog.py | 8 +++++++- 4 files changed, 30 insertions(+), 15 deletions(-) diff --git a/picard/coverart.py b/picard/coverart.py index 142e379d3..fef104838 100644 --- a/picard/coverart.py +++ b/picard/coverart.py @@ -101,8 +101,15 @@ def _coverart_downloaded(album, metadata, release, try_list, coverinfos, data, h filename = None if not is_front_image(coverinfos) and config.setting["caa_image_type_as_filename"]: filename = coverinfos['type'] + try: - image = Image(data, mime, coverinfos['type'], coverinfos['desc']) + metadata.make_and_add_image(mime, data, + imagetype=coverinfos['type'], + comment=coverinfos['desc']) + for track in album._new_tracks: + track.metadata.make_and_add_image(mime, data, + imagetype=coverinfos['type'], + comment=coverinfos['desc']) except (IOError, OSError), e: album.error_append(e.message) album._finalize_loading(error=True) @@ -110,10 +117,6 @@ def _coverart_downloaded(album, metadata, release, try_list, coverinfos, data, h # save them in the temporary folder, abort. return - metadata.add_image(image) - for track in album._new_tracks: - track.metadata.add_image(image) - # If the image already was a front image, there might still be some # other front images in the try_list - remove them. if is_front_image(coverinfos): diff --git a/picard/metadata.py b/picard/metadata.py index 2ac7103f8..2cee4b9c3 100644 --- a/picard/metadata.py +++ b/picard/metadata.py @@ -21,9 +21,11 @@ import os.path import shutil import sys import tempfile +import traceback -from os import fdopen, close, unlink +from hashlib import md5 +from os import fdopen, unlink from PyQt4.QtCore import QObject from picard import config, log from picard.plugin import ExtensionPoint @@ -171,15 +173,11 @@ class Metadata(dict): self.images = [] self.length = 0 - def add_image(self, image): - """Adds the Image object ``image`` to this Metadata object. - """ - self.images.append(image) - def make_and_add_image(self, mime, data, filename=None, comment="", - imagetype="front"): + imagetype="front"): """Build a new image object from ``data`` and adds it to this Metadata - object. + object. If an image with the same MD5 hash has already been added to + any Metadata object, that file will be reused. Arguments: mime -- The mimetype of the image @@ -188,7 +186,13 @@ class Metadata(dict): comment -- image description or comment, default to '' imagetype -- main type as a string, default to 'front' """ - image = Image(data, mime, imagetype, comment) + m = md5() + m.update(data) + datahash = m.hexdigest() + image = QObject.tagger.images[datahash] + if image is None: + image = Image(data, mime, imagetype, comment) + QObject.tagger.images[datahash] = image self.images.append(image) def remove_image(self, index): diff --git a/picard/tagger.py b/picard/tagger.py index 1180ce853..5e23ed95f 100644 --- a/picard/tagger.py +++ b/picard/tagger.py @@ -31,6 +31,7 @@ import re import shutil import signal import sys +from collections import defaultdict from functools import partial from itertools import chain @@ -165,6 +166,7 @@ class Tagger(QtGui.QApplication): self.albums = {} self.release_groups = {} self.mbid_redirects = {} + self.images = defaultdict(lambda: None) self.unmatched_files = UnmatchedFiles() self.nats = None self.window = MainWindow() diff --git a/picard/ui/infodialog.py b/picard/ui/infodialog.py index 71d5c14d1..6e151afe9 100644 --- a/picard/ui/infodialog.py +++ b/picard/ui/infodialog.py @@ -18,7 +18,9 @@ # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. import os.path +import traceback from PyQt4 import QtGui, QtCore +from picard import log from picard.util import format_time, encode_filename, bytes2human from picard.ui.ui_infodialog import Ui_InfoDialog @@ -47,7 +49,11 @@ class InfoDialog(QtGui.QDialog): return for image in images: - data = image.data + try: + data = image.data + except (OSError, IOError), e: + log.error(traceback.format_exc()) + continue size = len(data) item = QtGui.QListWidgetItem() pixmap = QtGui.QPixmap() From 3686fde72a325258920ba46a274f9073adfede93 Mon Sep 17 00:00:00 2001 From: Wieland Hoffmann Date: Thu, 30 Jan 2014 17:22:11 +0100 Subject: [PATCH 10/23] Use a _delete method for deleting temporary image files __del__ was not getting called reliably when the interpreter shut down. --- picard/metadata.py | 18 ++++++------------ picard/tagger.py | 1 + 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/picard/metadata.py b/picard/metadata.py index 2cee4b9c3..1c5293e59 100644 --- a/picard/metadata.py +++ b/picard/metadata.py @@ -142,18 +142,12 @@ class Image(object): with open(self._filename, "rb") as imagefile: return imagefile.read() - def __del__(self): - """Makes sure that the file created to hold this images data is - deleted. - """ - # http://docs.python.org/2/reference/datamodel.html?highlight=__del__#object.__del__ - # Due to the precarious circumstances under which __del__() methods are - # invoked, exceptions that occur during their execution are ignored, - # and a warning is printed to sys.stderr instead. - # - # This really means that wrapping the following unlink call in - # try-except statements will not let us catch the exception. - unlink(self._filename) + def _delete(self): + log.debug("Unlinking %s", self._filename) + try: + unlink(self._filename) + except OSError, e: + log.error(traceback.format_exc()) class Metadata(dict): diff --git a/picard/tagger.py b/picard/tagger.py index 5e23ed95f..a99a91999 100644 --- a/picard/tagger.py +++ b/picard/tagger.py @@ -207,6 +207,7 @@ class Tagger(QtGui.QApplication): self.nats.update() def exit(self): + map(lambda i: i._delete(), self.images.itervalues()) self.stopping = True self._acoustid.done() self.thread_pool.waitForDone() From 58f44f479a3c65233bdd59ef291741d917b9061b Mon Sep 17 00:00:00 2001 From: Wieland Hoffmann Date: Thu, 30 Jan 2014 17:26:49 +0100 Subject: [PATCH 11/23] Delete cover files after running TestCoverArt --- test/test_formats.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/test/test_formats.py b/test/test_formats.py index 866c1e58b..093aecd4b 100644 --- a/test/test_formats.py +++ b/test/test_formats.py @@ -1,11 +1,14 @@ import os.path +import picard.formats import unittest import shutil -from tempfile import mkstemp + + +from PyQt4 import QtCore +from collections import defaultdict from picard import config, log from picard.metadata import Metadata -import picard.formats -from PyQt4 import QtCore +from tempfile import mkstemp settings = { @@ -33,6 +36,7 @@ class FakeTagger(QtCore.QObject): QtCore.QObject.config = config QtCore.QObject.log = log self.tagger_stats_changed.connect(self.emit) + self.images = defaultdict(lambda: None) def emit(self, *args): pass @@ -504,6 +508,7 @@ class TestCoverArt(unittest.TestCase): QtCore.QObject.tagger = FakeTagger() def _tear_down(self): + map(lambda i: i._delete(), QtCore.QObject.tagger.images.itervalues()) os.unlink(self.filename) def test_asf(self): From ccdd1750599ed2cf575466a0dde445f848ff2557 Mon Sep 17 00:00:00 2001 From: Wieland Hoffmann Date: Thu, 6 Mar 2014 22:59:18 +0100 Subject: [PATCH 12/23] Catch signals to perform cleanup tasks This currently does only quit Picard after it has regained focus (see [0] and other answers to that question for the reason why). It also doesn't work on windows right now because I have no way to test that. [0] http://stackoverflow.com/a/4939113/307681 --- picard/tagger.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/picard/tagger.py b/picard/tagger.py index a99a91999..78790e926 100644 --- a/picard/tagger.py +++ b/picard/tagger.py @@ -30,6 +30,7 @@ import os.path import re import shutil import signal +import socket import sys from collections import defaultdict from functools import partial @@ -108,6 +109,18 @@ class Tagger(QtGui.QApplication): self.save_thread_pool = QtCore.QThreadPool(self) self.save_thread_pool.setMaxThreadCount(1) + if not sys.platform == "win32": + # Set up signal handling + self.signalfd = socket.socketpair(socket.AF_UNIX, socket.SOCK_STREAM, 0) + + self.signalnotifier = QtCore.QSocketNotifier(self.signalfd[1].fileno(), + QtCore.QSocketNotifier.Read, self) + self.signalnotifier.activated.connect(self.sighandler) + + signal.signal(signal.SIGHUP, self.signal) + signal.signal(signal.SIGINT, self.signal) + signal.signal(signal.SIGTERM, self.signal) + # Setup logging if debug or "PICARD_DEBUG" in os.environ: log.log_levels = log.log_levels | log.LOG_DEBUG @@ -207,6 +220,7 @@ class Tagger(QtGui.QApplication): self.nats.update() def exit(self): + log.debug("exit") map(lambda i: i._delete(), self.images.itervalues()) self.stopping = True self._acoustid.done() @@ -546,6 +560,16 @@ class Tagger(QtGui.QApplication): def instance(cls): return cls.__instance + def signal(self, signum, frame): + log.debug("signal %i received", signum) + self.signalfd[0].sendall("a") + + def sighandler(self): + self.signalnotifier.setEnabled(False) + self.exit() + self.quit() + self.signalnotifier.setEnabled(True) + def help(): print """Usage: %s [OPTIONS] [FILE] [FILE] ... From 7baad05206a59c0d9236bc144962fb428c679b22 Mon Sep 17 00:00:00 2001 From: Wieland Hoffmann Date: Thu, 13 Mar 2014 11:38:03 +0100 Subject: [PATCH 13/23] image class: rename _filename to _tempfile_filename --- picard/metadata.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/picard/metadata.py b/picard/metadata.py index 1c5293e59..d9fa29fa6 100644 --- a/picard/metadata.py +++ b/picard/metadata.py @@ -66,7 +66,7 @@ class Image(object): def __init__(self, data, mimetype="image/jpeg", imagetype="front", comment=""): self.description = comment - (fd, self._filename) = tempfile.mkstemp(prefix="picard") + (fd, self._tempfile_filename) = tempfile.mkstemp(prefix="picard") with fdopen(fd, "wb") as imagefile: imagefile.write(data) self.datalength = len(data) @@ -132,20 +132,20 @@ class Image(object): new_dirname = os.path.dirname(image_filename) if not os.path.isdir(new_dirname): os.makedirs(new_dirname) - shutil.copyfile(self._filename, image_filename) + shutil.copyfile(self._tempfile_filename, image_filename) @property def data(self): """Reads the data from the temporary file created for this image. May raise IOErrors or OSErrors. """ - with open(self._filename, "rb") as imagefile: + with open(self._tempfile_filename, "rb") as imagefile: return imagefile.read() def _delete(self): - log.debug("Unlinking %s", self._filename) + log.debug("Unlinking %s", self._tempfile_filename) try: - unlink(self._filename) + unlink(self._tempfile_filename) except OSError, e: log.error(traceback.format_exc()) From 78720a4f7cfcbe0df0a75aabce73b52d1db3a78e Mon Sep 17 00:00:00 2001 From: Wieland Hoffmann Date: Thu, 13 Mar 2014 11:56:22 +0100 Subject: [PATCH 14/23] Save images with an extension image_filename is the filename without the extension, new_filename does contain it. --- picard/metadata.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/picard/metadata.py b/picard/metadata.py index d9fa29fa6..0b8352bc6 100644 --- a/picard/metadata.py +++ b/picard/metadata.py @@ -132,7 +132,7 @@ class Image(object): new_dirname = os.path.dirname(image_filename) if not os.path.isdir(new_dirname): os.makedirs(new_dirname) - shutil.copyfile(self._tempfile_filename, image_filename) + shutil.copyfile(self._tempfile_filename, new_filename) @property def data(self): From e3aded26a7c00d6378a98f386f86c33770c1e8c9 Mon Sep 17 00:00:00 2001 From: Wieland Hoffmann Date: Thu, 13 Mar 2014 12:00:12 +0100 Subject: [PATCH 15/23] Readd the option to use a custom file name for images --- picard/metadata.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/picard/metadata.py b/picard/metadata.py index 0b8352bc6..3b4353ad5 100644 --- a/picard/metadata.py +++ b/picard/metadata.py @@ -64,16 +64,17 @@ class Image(object): """ def __init__(self, data, mimetype="image/jpeg", imagetype="front", - comment=""): + comment="", filename=None): self.description = comment (fd, self._tempfile_filename) = tempfile.mkstemp(prefix="picard") with fdopen(fd, "wb") as imagefile: imagefile.write(data) self.datalength = len(data) + self.extension = mime.get_extension(mime, ".jpg") + self.filename = filename self.imagetype = imagetype self.is_front_image = imagetype == "front" self.mimetype = mimetype - self.extension = mime.get_extension(mime, ".jpg") def _make_image_filename(self, filename, dirname, metadata): if config.setting["ascii_filenames"]: @@ -99,14 +100,17 @@ class Image(object): :counters: A dictionary mapping filenames to the amount of how many images with that filename were already saved in `dirname`. """ - if config.setting["caa_image_type_as_filename"]: + if self.filename is not None: + log.debug("Using the custom file name %s", self.filename) + filename = self.filename + elif config.setting["caa_image_type_as_filename"]: log.debug("Using image type %s", self.imagetype) - filename = self._make_image_filename(self.imagetype, dirname, metadata) + filename = self.imagetype else: log.debug("Using default file name %s", config.setting["cover_image_filename"]) - filename = self._make_image_filename( - config.setting["cover_image_filename"], dirname, metadata) + filename = config.setting["cover_image_filename"] + filename = self._make_image_filename(filename, dirname, metadata) overwrite = config.setting["save_images_overwrite"] ext = self.extension @@ -185,7 +189,7 @@ class Metadata(dict): datahash = m.hexdigest() image = QObject.tagger.images[datahash] if image is None: - image = Image(data, mime, imagetype, comment) + image = Image(data, mime, imagetype, comment, filename) QObject.tagger.images[datahash] = image self.images.append(image) From ee6fba73ac5145c728a7954531695f7d96acecc5 Mon Sep 17 00:00:00 2001 From: Wieland Hoffmann Date: Thu, 13 Mar 2014 12:01:30 +0100 Subject: [PATCH 16/23] coverart: Remove a write-only assignment --- picard/coverart.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/picard/coverart.py b/picard/coverart.py index fef104838..39dc6ad44 100644 --- a/picard/coverart.py +++ b/picard/coverart.py @@ -98,9 +98,6 @@ def _coverart_downloaded(album, metadata, release, try_list, coverinfos, data, h QObject.tagger.window.set_statusbar_message(N_("Coverart %s downloaded"), http.url().toString()) mime = mimetype.get_from_data(data, default="image/jpeg") - filename = None - if not is_front_image(coverinfos) and config.setting["caa_image_type_as_filename"]: - filename = coverinfos['type'] try: metadata.make_and_add_image(mime, data, From 5a266b1a25c749d224ee369a90b1ead66bd0e533 Mon Sep 17 00:00:00 2001 From: Laurent Monin Date: Thu, 27 Mar 2014 09:45:10 +0100 Subject: [PATCH 17/23] Fix the need to re-focus window after ctrl+c interrupt for the app to exit --- picard/tagger.py | 1 + 1 file changed, 1 insertion(+) diff --git a/picard/tagger.py b/picard/tagger.py index 78790e926..cd82b07e1 100644 --- a/picard/tagger.py +++ b/picard/tagger.py @@ -604,4 +604,5 @@ def main(localedir=None, autoupdate=True): elif opt in ("-d", "--debug"): kwargs["debug"] = True tagger = Tagger(args, localedir, autoupdate, **kwargs) + tagger.startTimer(200) sys.exit(tagger.run()) From 13d95e309acdc279ff293f3337c9a3fd2427e612 Mon Sep 17 00:00:00 2001 From: Laurent Monin Date: Mon, 31 Mar 2014 21:28:12 +0200 Subject: [PATCH 18/23] Add lockable default dict class --- picard/util/__init__.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/picard/util/__init__.py b/picard/util/__init__.py index 03ea2dc5a..c081e9dbd 100644 --- a/picard/util/__init__.py +++ b/picard/util/__init__.py @@ -27,6 +27,19 @@ from PyQt4 import QtCore from encodings import rot_13 from string import Template from functools import partial +from collections import defaultdict + + +class LockableDefaultDict(defaultdict): + def __init__(self, default): + defaultdict.__init__(self, default) + self.__lock = QtCore.QReadWriteLock() + + def lock(self): + self.__lock.lockForWrite() + + def unlock(self): + self.__lock.unlock() def asciipunct(s): From d4753031d078bfaa867d8ba7384363b1cc2fb860 Mon Sep 17 00:00:00 2001 From: Laurent Monin Date: Mon, 31 Mar 2014 21:29:15 +0200 Subject: [PATCH 19/23] Add a bit of debug log when images are saved to temp files --- picard/metadata.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/picard/metadata.py b/picard/metadata.py index 3b4353ad5..29c793391 100644 --- a/picard/metadata.py +++ b/picard/metadata.py @@ -64,11 +64,13 @@ class Image(object): """ def __init__(self, data, mimetype="image/jpeg", imagetype="front", - comment="", filename=None): + comment="", filename=None, datahash=""): self.description = comment (fd, self._tempfile_filename) = tempfile.mkstemp(prefix="picard") with fdopen(fd, "wb") as imagefile: imagefile.write(data) + log.debug("Saving image (hash=%s) to %r" % (datahash, + self._tempfile_filename)) self.datalength = len(data) self.extension = mime.get_extension(mime, ".jpg") self.filename = filename @@ -189,7 +191,8 @@ class Metadata(dict): datahash = m.hexdigest() image = QObject.tagger.images[datahash] if image is None: - image = Image(data, mime, imagetype, comment, filename) + image = Image(data, mime, imagetype, comment, filename, + datahash=datahash) QObject.tagger.images[datahash] = image self.images.append(image) From 906e38f992054f6ab92e1c754e48b3a3705253b4 Mon Sep 17 00:00:00 2001 From: Laurent Monin Date: Mon, 31 Mar 2014 21:32:21 +0200 Subject: [PATCH 20/23] Lock images dict when writing, it is needed to avoid double saved images --- picard/metadata.py | 4 +++- picard/tagger.py | 3 ++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/picard/metadata.py b/picard/metadata.py index 29c793391..e7af4d78f 100644 --- a/picard/metadata.py +++ b/picard/metadata.py @@ -189,11 +189,13 @@ class Metadata(dict): m = md5() m.update(data) datahash = m.hexdigest() + QObject.tagger.images.lock() image = QObject.tagger.images[datahash] if image is None: image = Image(data, mime, imagetype, comment, filename, datahash=datahash) - QObject.tagger.images[datahash] = image + QObject.tagger.images[datahash] = image + QObject.tagger.images.unlock() self.images.append(image) def remove_image(self, index): diff --git a/picard/tagger.py b/picard/tagger.py index cd82b07e1..02c28f022 100644 --- a/picard/tagger.py +++ b/picard/tagger.py @@ -78,6 +78,7 @@ from picard.util import ( check_io_encoding, uniqify, is_hidden_path, + LockableDefaultDict ) from picard.webservice import XmlWebService @@ -179,7 +180,7 @@ class Tagger(QtGui.QApplication): self.albums = {} self.release_groups = {} self.mbid_redirects = {} - self.images = defaultdict(lambda: None) + self.images = LockableDefaultDict(lambda: None) self.unmatched_files = UnmatchedFiles() self.nats = None self.window = MainWindow() From e97f404fff0530ce59ad0d553abcc2decfa90095 Mon Sep 17 00:00:00 2001 From: Laurent Monin Date: Mon, 31 Mar 2014 21:33:32 +0200 Subject: [PATCH 21/23] Update test_formats: defaultdict -> LockableDefaultDict --- test/test_formats.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/test_formats.py b/test/test_formats.py index 093aecd4b..4df7a1006 100644 --- a/test/test_formats.py +++ b/test/test_formats.py @@ -5,7 +5,7 @@ import shutil from PyQt4 import QtCore -from collections import defaultdict +from picard.util import LockableDefaultDict from picard import config, log from picard.metadata import Metadata from tempfile import mkstemp @@ -36,7 +36,7 @@ class FakeTagger(QtCore.QObject): QtCore.QObject.config = config QtCore.QObject.log = log self.tagger_stats_changed.connect(self.emit) - self.images = defaultdict(lambda: None) + self.images = LockableDefaultDict(lambda: None) def emit(self, *args): pass From 9f850830253ec3d4b7e4cf010272a680475a3ee7 Mon Sep 17 00:00:00 2001 From: Wieland Hoffmann Date: Fri, 4 Apr 2014 12:39:39 +0200 Subject: [PATCH 22/23] Add comments to the signal handling code --- picard/tagger.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/picard/tagger.py b/picard/tagger.py index 02c28f022..cf35e2454 100644 --- a/picard/tagger.py +++ b/picard/tagger.py @@ -112,6 +112,13 @@ class Tagger(QtGui.QApplication): if not sys.platform == "win32": # Set up signal handling + # It's not possible to call all available functions from signal + # handlers, therefore we need to set up a QSocketNotifier to listen + # on a socket. Sending data through a socket can be done in a + # signal handler, so we use the socket to notify the application of + # the signal. + # This code is adopted from + # https://qt-project.org/doc/qt-4.8/unix-signals.html self.signalfd = socket.socketpair(socket.AF_UNIX, socket.SOCK_STREAM, 0) self.signalnotifier = QtCore.QSocketNotifier(self.signalfd[1].fileno(), @@ -563,6 +570,8 @@ class Tagger(QtGui.QApplication): def signal(self, signum, frame): log.debug("signal %i received", signum) + # Send a notification about a received signal from the signal handler + # to Qt. self.signalfd[0].sendall("a") def sighandler(self): From b9a4f7898b975ad405896c545162f57459a09503 Mon Sep 17 00:00:00 2001 From: Wieland Hoffmann Date: Tue, 8 Apr 2014 13:06:27 +0200 Subject: [PATCH 23/23] increase the timer delay --- picard/tagger.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/picard/tagger.py b/picard/tagger.py index cf35e2454..fb568d317 100644 --- a/picard/tagger.py +++ b/picard/tagger.py @@ -614,5 +614,5 @@ def main(localedir=None, autoupdate=True): elif opt in ("-d", "--debug"): kwargs["debug"] = True tagger = Tagger(args, localedir, autoupdate, **kwargs) - tagger.startTimer(200) + tagger.startTimer(1000) sys.exit(tagger.run())