From bab8447c8e5012291b0ff66ed1b8e7319018469d Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Thu, 3 Dec 2020 10:45:30 +0100 Subject: [PATCH 1/4] Add a tagger.iter_files_from_objects method Similar to get_files_from_objects, but uses an iterator instead of creating a list. --- picard/tagger.py | 24 +++++++++++++----------- picard/ui/mainwindow.py | 9 +++++---- picard/ui/playertoolbar.py | 2 +- picard/ui/searchdialog/album.py | 2 +- picard/util/__init__.py | 7 +++++++ test/test_utils.py | 22 +++++++++++++++++++++- 6 files changed, 48 insertions(+), 18 deletions(-) diff --git a/picard/tagger.py b/picard/tagger.py index 77fad8626..3b49ee525 100644 --- a/picard/tagger.py +++ b/picard/tagger.py @@ -111,11 +111,11 @@ from picard.util import ( decode_filename, encode_filename, is_hidden, + iter_unique, mbid_validate, normpath, process_events_iter, thread, - uniqify, versions, webbrowser2, ) @@ -582,7 +582,7 @@ class Tagger(QtWidgets.QApplication): def copy_files(self, objects): mimeData = QtCore.QMimeData() - mimeData.setUrls([QtCore.QUrl.fromLocalFile(f.filename) for f in (self.get_files_from_objects(objects))]) + mimeData.setUrls([QtCore.QUrl.fromLocalFile(f.filename) for f in self.iter_files_from_objects(objects)]) self.clipboard().setMimeData(mimeData) def paste_files(self, target): @@ -646,12 +646,16 @@ class Tagger(QtWidgets.QApplication): def get_files_from_objects(self, objects, save=False): """Return list of files from list of albums, clusters, tracks or files.""" - return uniqify(chain(*[obj.iterfiles(save) for obj in objects])) + return list(self.iter_files_from_objects(objects, save=save)) + + @staticmethod + def iter_files_from_objects(objects, save=False): + """Creates an iterator over all unique files from list of albums, clusters, tracks or files.""" + return iter_unique(chain(*(obj.iterfiles(save) for obj in objects))) def save(self, objects): """Save the specified objects.""" - files = self.get_files_from_objects(objects, save=True) - for file in files: + for file in self.iter_files_from_objects(objects, save=True): file.save() def load_album(self, album_id, discid=None): @@ -707,7 +711,7 @@ class Tagger(QtWidgets.QApplication): if album.id not in self.albums: return album.stop_loading() - self.remove_files(self.get_files_from_objects([album])) + self.remove_files(album.iterfiles()) del self.albums[album.id] if album.release_group: album.release_group.remove_album(album.id) @@ -720,7 +724,7 @@ class Tagger(QtWidgets.QApplication): def remove_nat(self, track): """Remove the specified non-album track.""" log.debug("Removing %r", track) - self.remove_files(self.get_files_from_objects([track])) + self.remove_files(track.iterfiles()) if not self.nats: return self.nats.tracks.remove(track) @@ -801,8 +805,7 @@ class Tagger(QtWidgets.QApplication): """Analyze the file(s).""" if not self.use_acoustid: return - files = self.get_files_from_objects(objs) - for file in files: + for file in self.iter_files_from_objects(objs): if file.can_analyze(): file.set_pending() self._acoustid.analyze(file, partial(file._lookup_finished, File.LOOKUP_ACOUSTID)) @@ -811,12 +814,11 @@ class Tagger(QtWidgets.QApplication): """Generate the fingerprints without matching the files.""" if not self.use_acoustid: return - files = self.get_files_from_objects(objs) def finished(file, result): file.clear_pending() - for file in files: + for file in self.iter_files_from_objects(objs): file.set_pending() self._acoustid.fingerprint(file, partial(finished, file)) diff --git a/picard/ui/mainwindow.py b/picard/ui/mainwindow.py index bb0c4c40c..e7b6fdbd3 100644 --- a/picard/ui/mainwindow.py +++ b/picard/ui/mainwindow.py @@ -73,6 +73,7 @@ from picard.plugin import ExtensionPoint from picard.track import Track from picard.util import ( icontheme, + iter_unique, restore_method, thread, throttle, @@ -1043,16 +1044,16 @@ class MainWindow(QtWidgets.QMainWindow, PreserveGeometry): return QtCore.QUrl.fromLocalFile(url) def play_file(self): - files = self.tagger.get_files_from_objects(self.selected_objects) - for file in files: + for file in self.tagger.iter_files_from_objects(self.selected_objects): QtGui.QDesktopServices.openUrl(self._openUrl(file.filename)) def _on_player_error(self, error, msg): self.set_statusbar_message(msg, echo=log.warning, translate=None) def open_folder(self): - files = self.tagger.get_files_from_objects(self.selected_objects) - folders = set([os.path.dirname(f.filename) for f in files]) + folders = iter_unique( + os.path.dirname(f.filename) for f + in self.tagger.iter_files_from_objects(self.selected_objects)) for folder in folders: QtGui.QDesktopServices.openUrl(self._openUrl(folder)) diff --git a/picard/ui/playertoolbar.py b/picard/ui/playertoolbar.py index 95901a043..79a7ede3a 100644 --- a/picard/ui/playertoolbar.py +++ b/picard/ui/playertoolbar.py @@ -133,7 +133,7 @@ class Player(QtCore.QObject): playlist = QtMultimedia.QMediaPlaylist(self) playlist.setPlaybackMode(QtMultimedia.QMediaPlaylist.Sequential) playlist.addMedia([QtMultimedia.QMediaContent(QtCore.QUrl.fromLocalFile(file.filename)) - for file in self.tagger.get_files_from_objects(self._selected_objects)]) + for file in self.tagger.iter_files_from_objects(self._selected_objects)]) self._player.setPlaylist(playlist) self._player.play() diff --git a/picard/ui/searchdialog/album.py b/picard/ui/searchdialog/album.py index b30eca1e4..060f2e5cc 100644 --- a/picard/ui/searchdialog/album.py +++ b/picard/ui/searchdialog/album.py @@ -367,6 +367,6 @@ class AlbumSearchDialog(SearchDialog): release["musicbrainz_albumid"]) album = self.tagger.load_album(release["musicbrainz_albumid"]) if self.cluster: - files = self.tagger.get_files_from_objects([self.cluster]) + files = self.cluster.iterfiles() self.tagger.move_files_to_album(files, release["musicbrainz_albumid"], album) diff --git a/picard/util/__init__.py b/picard/util/__init__.py index 1fc510579..29508bb90 100644 --- a/picard/util/__init__.py +++ b/picard/util/__init__.py @@ -369,6 +369,13 @@ def uniqify(seq): return [x for x in seq if x not in seen and not add_seen(x)] +def iter_unique(seq): + """Creates an iterator only returning unique values from seq""" + seen = set() + add_seen = seen.add + return (x for x in seq if x not in seen and not add_seen(x)) + + # order is important _tracknum_regexps = ( # search for explicit track number (prefix "track") diff --git a/test/test_utils.py b/test/test_utils.py index fafef4f11..87089d752 100644 --- a/test/test_utils.py +++ b/test/test_utils.py @@ -4,7 +4,7 @@ # # Copyright (C) 2006-2007 Lukáš Lalinský # Copyright (C) 2010 fatih -# Copyright (C) 2010-2011, 2014, 2018-2019 Philipp Wolfer +# Copyright (C) 2010-2011, 2014, 2018-2020 Philipp Wolfer # Copyright (C) 2012, 2014, 2018 Wieland Hoffmann # Copyright (C) 2013 Ionuț Ciocîrlan # Copyright (C) 2013-2014, 2018-2020 Laurent Monin @@ -30,6 +30,7 @@ import builtins from collections import namedtuple +from collections.abc import Iterator import os.path import unittest @@ -42,8 +43,10 @@ from picard.util import ( find_best_match, imageinfo, is_absolute_path, + iter_unique, limited_join, sort_by_similarity, + uniqify, ) @@ -450,3 +453,20 @@ class LimitedJoin(PicardTestCase): expected = '0,1,2,3,…,6,7,8,9' result = limited_join(self.list, len(self.list) - 1, ',') self.assertEqual(result, expected) + + +class IterUniqifyTest(PicardTestCase): + + def test_unique(self): + items = [1, 2, 3, 2, 3, 4] + result = uniqify(items) + self.assertEqual([1, 2, 3, 4], result) + + +class IterUniqueTest(PicardTestCase): + + def test_unique(self): + items = [1, 2, 3, 2, 3, 4] + result = iter_unique(items) + self.assertTrue(isinstance(result, Iterator)) + self.assertEqual([1, 2, 3, 4], list(result)) From 024a809cf2a0f3cea488dbbd8d1763485bd69d95 Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Thu, 3 Dec 2020 11:21:27 +0100 Subject: [PATCH 2/4] Use iter_unique in scriptsmenu --- picard/ui/scriptsmenu.py | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/picard/ui/scriptsmenu.py b/picard/ui/scriptsmenu.py index 87786522a..ea768228f 100644 --- a/picard/ui/scriptsmenu.py +++ b/picard/ui/scriptsmenu.py @@ -3,7 +3,7 @@ # Picard, the next-generation MusicBrainz tagger # # Copyright (C) 2018 Laurent Monin -# Copyright (C) 2018 Philipp Wolfer +# Copyright (C) 2018, 2020 Philipp Wolfer # Copyright (C) 2018 Yvan Rivière # # This program is free software; you can redistribute it and/or @@ -36,7 +36,7 @@ from picard.script import ( ScriptParser, ) from picard.track import Track -from picard.util import uniqify +from picard.util import iter_unique class ScriptsMenu(QtWidgets.QMenu): @@ -53,7 +53,7 @@ class ScriptsMenu(QtWidgets.QMenu): s_text = script[3] parser = ScriptParser() - for obj in self._get_unique_metadata_objects(): + for obj in self._iter_unique_metadata_objects(): try: parser.eval(s_text, obj.metadata) obj.update() @@ -66,20 +66,17 @@ class ScriptsMenu(QtWidgets.QMenu): } self.tagger.window.set_statusbar_message(msg, mparms) - def _get_unique_metadata_objects(self): - objs = self._get_metadata_objects(self.tagger.window.selected_objects) - return uniqify(objs) + def _iter_unique_metadata_objects(self): + return iter_unique(self._iter_metadata_objects(self.tagger.window.selected_objects)) - def _get_metadata_objects(self, objs): + def _iter_metadata_objects(self, objs): for obj in objs: if hasattr(obj, 'metadata'): yield obj - if isinstance(obj, Cluster): - yield from self._get_metadata_objects(obj.files) - if isinstance(obj, ClusterList): - yield from self._get_metadata_objects(obj) - if isinstance(obj, Album): - yield from self._get_metadata_objects(obj.tracks) - yield from self._get_metadata_objects(obj.unmatched_files.iterfiles()) - if isinstance(obj, Track): - yield from self._get_metadata_objects(obj.files) + if isinstance(obj, Cluster) or isinstance(obj, Track): + yield from self._iter_metadata_objects(obj.iterfiles()) + elif isinstance(obj, ClusterList): + yield from self._iter_metadata_objects(obj) + elif isinstance(obj, Album): + yield from self._iter_metadata_objects(obj.tracks) + yield from self._iter_metadata_objects(obj.unmatched_files.iterfiles()) From b805bcccb2e39741a6bada7fae30324ca0201abf Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Thu, 3 Dec 2020 12:05:13 +0100 Subject: [PATCH 3/4] Move iter_files_from_objects to picard.util --- picard/tagger.py | 21 +++++++++------------ picard/ui/mainwindow.py | 5 +++-- picard/ui/playertoolbar.py | 3 ++- picard/util/__init__.py | 6 ++++++ test/test_utils.py | 17 +++++++++++++++++ 5 files changed, 37 insertions(+), 15 deletions(-) diff --git a/picard/tagger.py b/picard/tagger.py index 3b49ee525..78c08d849 100644 --- a/picard/tagger.py +++ b/picard/tagger.py @@ -45,7 +45,6 @@ import argparse from collections import defaultdict from functools import partial -from itertools import chain import logging import os.path import platform @@ -111,7 +110,7 @@ from picard.util import ( decode_filename, encode_filename, is_hidden, - iter_unique, + iter_files_from_objects, mbid_validate, normpath, process_events_iter, @@ -582,7 +581,7 @@ class Tagger(QtWidgets.QApplication): def copy_files(self, objects): mimeData = QtCore.QMimeData() - mimeData.setUrls([QtCore.QUrl.fromLocalFile(f.filename) for f in self.iter_files_from_objects(objects)]) + mimeData.setUrls([QtCore.QUrl.fromLocalFile(f.filename) for f in iter_files_from_objects(objects)]) self.clipboard().setMimeData(mimeData) def paste_files(self, target): @@ -645,17 +644,15 @@ class Tagger(QtWidgets.QApplication): item.filename if isinstance(item, File) else '') def get_files_from_objects(self, objects, save=False): - """Return list of files from list of albums, clusters, tracks or files.""" - return list(self.iter_files_from_objects(objects, save=save)) + """Return list of unique files from list of albums, clusters, tracks or files. - @staticmethod - def iter_files_from_objects(objects, save=False): - """Creates an iterator over all unique files from list of albums, clusters, tracks or files.""" - return iter_unique(chain(*(obj.iterfiles(save) for obj in objects))) + Note: Consider using picard.util.iter_files_from_objects instead, which returns an iterator. + """ + return list(iter_files_from_objects(objects, save=save)) def save(self, objects): """Save the specified objects.""" - for file in self.iter_files_from_objects(objects, save=True): + for file in iter_files_from_objects(objects, save=True): file.save() def load_album(self, album_id, discid=None): @@ -805,7 +802,7 @@ class Tagger(QtWidgets.QApplication): """Analyze the file(s).""" if not self.use_acoustid: return - for file in self.iter_files_from_objects(objs): + for file in iter_files_from_objects(objs): if file.can_analyze(): file.set_pending() self._acoustid.analyze(file, partial(file._lookup_finished, File.LOOKUP_ACOUSTID)) @@ -818,7 +815,7 @@ class Tagger(QtWidgets.QApplication): def finished(file, result): file.clear_pending() - for file in self.iter_files_from_objects(objs): + for file in iter_files_from_objects(objs): file.set_pending() self._acoustid.fingerprint(file, partial(finished, file)) diff --git a/picard/ui/mainwindow.py b/picard/ui/mainwindow.py index e7b6fdbd3..50be1de20 100644 --- a/picard/ui/mainwindow.py +++ b/picard/ui/mainwindow.py @@ -73,6 +73,7 @@ from picard.plugin import ExtensionPoint from picard.track import Track from picard.util import ( icontheme, + iter_files_from_objects, iter_unique, restore_method, thread, @@ -1044,7 +1045,7 @@ class MainWindow(QtWidgets.QMainWindow, PreserveGeometry): return QtCore.QUrl.fromLocalFile(url) def play_file(self): - for file in self.tagger.iter_files_from_objects(self.selected_objects): + for file in iter_files_from_objects(self.selected_objects): QtGui.QDesktopServices.openUrl(self._openUrl(file.filename)) def _on_player_error(self, error, msg): @@ -1053,7 +1054,7 @@ class MainWindow(QtWidgets.QMainWindow, PreserveGeometry): def open_folder(self): folders = iter_unique( os.path.dirname(f.filename) for f - in self.tagger.iter_files_from_objects(self.selected_objects)) + in iter_files_from_objects(self.selected_objects)) for folder in folders: QtGui.QDesktopServices.openUrl(self._openUrl(folder)) diff --git a/picard/ui/playertoolbar.py b/picard/ui/playertoolbar.py index 79a7ede3a..ded898627 100644 --- a/picard/ui/playertoolbar.py +++ b/picard/ui/playertoolbar.py @@ -37,6 +37,7 @@ from picard.const.sys import IS_MACOS from picard.util import ( format_time, icontheme, + iter_files_from_objects, ) from picard.ui.widgets import ( @@ -133,7 +134,7 @@ class Player(QtCore.QObject): playlist = QtMultimedia.QMediaPlaylist(self) playlist.setPlaybackMode(QtMultimedia.QMediaPlaylist.Sequential) playlist.addMedia([QtMultimedia.QMediaContent(QtCore.QUrl.fromLocalFile(file.filename)) - for file in self.tagger.iter_files_from_objects(self._selected_objects)]) + for file in iter_files_from_objects(self._selected_objects)]) self._player.setPlaylist(playlist) self._player.play() diff --git a/picard/util/__init__.py b/picard/util/__init__.py index 29508bb90..e92dfab67 100644 --- a/picard/util/__init__.py +++ b/picard/util/__init__.py @@ -41,6 +41,7 @@ import builtins from collections import namedtuple from collections.abc import Mapping import html +from itertools import chain import json import ntpath from operator import attrgetter @@ -112,6 +113,11 @@ def process_events_iter(iterable, interval=0.1): QtCore.QCoreApplication.processEvents() +def iter_files_from_objects(objects, save=False): + """Creates an iterator over all unique files from list of albums, clusters, tracks or files.""" + return iter_unique(chain(*(obj.iterfiles(save) for obj in objects))) + + _io_encoding = sys.getfilesystemencoding() diff --git a/test/test_utils.py b/test/test_utils.py index 87089d752..ffece9548 100644 --- a/test/test_utils.py +++ b/test/test_utils.py @@ -33,6 +33,7 @@ from collections import namedtuple from collections.abc import Iterator import os.path import unittest +from unittest.mock import Mock from test.picardtestcase import PicardTestCase @@ -43,6 +44,7 @@ from picard.util import ( find_best_match, imageinfo, is_absolute_path, + iter_files_from_objects, iter_unique, limited_join, sort_by_similarity, @@ -455,6 +457,21 @@ class LimitedJoin(PicardTestCase): self.assertEqual(result, expected) +class IterFilesFromObjectsTest(PicardTestCase): + + def test_iterate_only_unique(self): + f1 = Mock() + f2 = Mock() + f3 = Mock() + obj1 = Mock() + obj1.iterfiles = Mock(return_value=[f1, f2]) + obj2 = Mock() + obj2.iterfiles = Mock(return_value=[f2, f3]) + result = iter_files_from_objects([obj1, obj2]) + self.assertTrue(isinstance(result, Iterator)) + self.assertEqual([f1, f2, f3], list(result)) + + class IterUniqifyTest(PicardTestCase): def test_unique(self): From 77bf10c0609a95ce838c9ec1795661b1ba1628d3 Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Thu, 3 Dec 2020 15:47:21 +0100 Subject: [PATCH 4/4] Simplify uniqify and iter_unique --- picard/util/__init__.py | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/picard/util/__init__.py b/picard/util/__init__.py index e92dfab67..67e178b1f 100644 --- a/picard/util/__init__.py +++ b/picard/util/__init__.py @@ -368,18 +368,13 @@ def throttle(interval): def uniqify(seq): """Uniqify a list, preserving order""" - # Courtesy of Dave Kirby - # See http://www.peterbe.com/plog/uniqifiers-benchmark - seen = set() - add_seen = seen.add - return [x for x in seq if x not in seen and not add_seen(x)] + return list(iter_unique(seq)) def iter_unique(seq): """Creates an iterator only returning unique values from seq""" seen = set() - add_seen = seen.add - return (x for x in seq if x not in seen and not add_seen(x)) + return (x for x in seq if x not in seen and not seen.add(x)) # order is important