From afc33502576489998b5c4336d08c7bc17e3a89f4 Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Tue, 7 Jan 2025 16:20:01 +0100 Subject: [PATCH 1/5] PICARD-3012: Sort files by disc no. when submitting cluster as release This ensures the disc numbers are in order even if the cluster gets edited after clustering. --- picard/browser/addrelease.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/picard/browser/addrelease.py b/picard/browser/addrelease.py index 690f921e77..f617fa2e24 100644 --- a/picard/browser/addrelease.py +++ b/picard/browser/addrelease.py @@ -2,7 +2,7 @@ # # Picard, the next-generation MusicBrainz tagger # -# Copyright (C) 2021-2023 Philipp Wolfer +# Copyright (C) 2021-2023, 2025 Philipp Wolfer # Copyright (C) 2021-2024 Laurent Monin # Copyright (C) 2022 Bob Swift # Copyright (C) 2022 jesus2099 @@ -23,6 +23,7 @@ from html import escape +from operator import attrgetter from secrets import token_bytes from PyQt6.QtCore import QCoreApplication @@ -214,7 +215,7 @@ def mkey(disc, track, name): disc_counter = 0 track_counter = 0 last_discnumber = None - for f in files: + for f in sorted(files, key=attrgetter('discnumber', 'tracknumber')): m = f.metadata discnumber = extract_discnumber(m) if last_discnumber is not None and discnumber != last_discnumber: From 7e34d9d2251b7d4f37ebe1d388277b831ee1a6b7 Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Tue, 7 Jan 2025 17:12:17 +0100 Subject: [PATCH 2/5] Moved addrelease.extract_discnumber functionality to Item.discnumber --- picard/browser/addrelease.py | 10 +-------- picard/item.py | 6 +++--- test/test_browser_addrelease.py | 36 --------------------------------- test/test_file.py | 6 +++++- 4 files changed, 9 insertions(+), 49 deletions(-) delete mode 100644 test/test_browser_addrelease.py diff --git a/picard/browser/addrelease.py b/picard/browser/addrelease.py index f617fa2e24..4b76badb0a 100644 --- a/picard/browser/addrelease.py +++ b/picard/browser/addrelease.py @@ -113,14 +113,6 @@ def serve_form(token): raise InvalidTokenError -def extract_discnumber(metadata): - try: - discnumber = metadata.get('discnumber', '1').split('/')[0] - return int(discnumber) - except ValueError: - return 1 - - def _open_url_with_token(payload): token = jwt.encode(payload, __key, algorithm=__algorithm) if isinstance(token, bytes): # For compatibility with PyJWT 1.x @@ -217,7 +209,7 @@ def mkey(disc, track, name): last_discnumber = None for f in sorted(files, key=attrgetter('discnumber', 'tracknumber')): m = f.metadata - discnumber = extract_discnumber(m) + discnumber = f.discnumber if last_discnumber is not None and discnumber != last_discnumber: disc_counter += 1 track_counter = 0 diff --git a/picard/item.py b/picard/item.py index 8527af599a..81ffd64043 100644 --- a/picard/item.py +++ b/picard/item.py @@ -3,7 +3,7 @@ # Picard, the next-generation MusicBrainz tagger # # Copyright (C) 2006-2007 Lukáš Lalinský -# Copyright (C) 2010, 2018, 2020-2022, 2024 Philipp Wolfer +# Copyright (C) 2010, 2018, 2020-2022, 2024-2025 Philipp Wolfer # Copyright (C) 2011-2012 Michael Wiencek # Copyright (C) 2012 Chad Wilson # Copyright (C) 2013, 2020-2021, 2023-2024 Laurent Monin @@ -117,8 +117,8 @@ def tracknumber(self): def discnumber(self): """The disc number as an int.""" try: - return int(self.metadata['discnumber']) - except BaseException: + return int(self.metadata.get('discnumber', '0').split('/')[0]) + except ValueError: return 0 @property diff --git a/test/test_browser_addrelease.py b/test/test_browser_addrelease.py deleted file mode 100644 index 1744b0edf3..0000000000 --- a/test/test_browser_addrelease.py +++ /dev/null @@ -1,36 +0,0 @@ -# -*- coding: utf-8 -*- -# -# Picard, the next-generation MusicBrainz tagger -# -# Copyright (C) 2021 Philipp Wolfer -# Copyright (C) 2021-2022 Laurent Monin -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU General Public License -# as published by the Free Software Foundation; either version 2 -# of the License, or (at your option) any later version. -# -# This program is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# 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. - - -from test.picardtestcase import PicardTestCase - -from picard.browser.addrelease import extract_discnumber -from picard.metadata import Metadata - - -class BrowserAddreleaseTest(PicardTestCase): - - def test_extract_discnumber(self): - self.assertEqual(1, extract_discnumber(Metadata())) - self.assertEqual(1, extract_discnumber(Metadata({'discnumber': '1'}))) - self.assertEqual(42, extract_discnumber(Metadata({'discnumber': '42'}))) - self.assertEqual(3, extract_discnumber(Metadata({'discnumber': '3/12'}))) - self.assertEqual(3, extract_discnumber(Metadata({'discnumber': ' 3 / 12 '}))) diff --git a/test/test_file.py b/test/test_file.py index 55186c6c04..db57b37370 100644 --- a/test/test_file.py +++ b/test/test_file.py @@ -2,7 +2,7 @@ # # Picard, the next-generation MusicBrainz tagger # -# Copyright (C) 2018-2024 Philipp Wolfer +# Copyright (C) 2018-2025 Philipp Wolfer # Copyright (C) 2019-2022, 2024 Laurent Monin # Copyright (C) 2021 Bob Swift # Copyright (C) 2021 Sophist-UK @@ -71,6 +71,10 @@ def test_discnumber(self): self.assertEqual(42, self.file.discnumber) self.file.metadata['discnumber'] = 'FOURTYTWO' self.assertEqual(0, self.file.discnumber) + self.file.metadata["discnumber"] = "3/12" + self.assertEqual(3, self.file.discnumber) + self.file.metadata["discnumber"] = "3 / 12" + self.assertEqual(3, self.file.discnumber) def test_set_acoustid_fingerprint(self): fingerprint = 'foo' From 5555476d5eb107c5535bfc88004ec90fab11a6a3 Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Tue, 7 Jan 2025 17:19:54 +0100 Subject: [PATCH 3/5] Use consistent logic for Item.discnumber and Item.tracknumber --- picard/item.py | 2 +- test/test_file.py | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/picard/item.py b/picard/item.py index 81ffd64043..723d6a974a 100644 --- a/picard/item.py +++ b/picard/item.py @@ -109,7 +109,7 @@ def load(self, priority=False, refresh=False): def tracknumber(self): """The track number as an int.""" try: - return int(self.metadata['tracknumber']) + return int(self.metadata.get('tracknumber', '0').split('/')[0]) except BaseException: return 0 diff --git a/test/test_file.py b/test/test_file.py index db57b37370..4779d50cf5 100644 --- a/test/test_file.py +++ b/test/test_file.py @@ -64,6 +64,10 @@ def test_tracknumber(self): self.assertEqual(42, self.file.tracknumber) self.file.metadata['tracknumber'] = 'FOURTYTWO' self.assertEqual(0, self.file.tracknumber) + self.file.metadata['tracknumber'] = '3/12' + self.assertEqual(3, self.file.tracknumber) + self.file.metadata['tracknumber'] = '3 / 12' + self.assertEqual(3, self.file.tracknumber) def test_discnumber(self): self.assertEqual(0, self.file.discnumber) @@ -71,9 +75,9 @@ def test_discnumber(self): self.assertEqual(42, self.file.discnumber) self.file.metadata['discnumber'] = 'FOURTYTWO' self.assertEqual(0, self.file.discnumber) - self.file.metadata["discnumber"] = "3/12" + self.file.metadata["discnumber"] = '3/12' self.assertEqual(3, self.file.discnumber) - self.file.metadata["discnumber"] = "3 / 12" + self.file.metadata["discnumber"] = '3 / 12' self.assertEqual(3, self.file.discnumber) def test_set_acoustid_fingerprint(self): From fb6b30f14157ad9bc250d117e629f9c65705b36f Mon Sep 17 00:00:00 2001 From: Laurent Monin Date: Tue, 7 Jan 2025 20:58:54 +0100 Subject: [PATCH 4/5] Use ValueError exception (missed change) --- picard/item.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/picard/item.py b/picard/item.py index 723d6a974a..ebb24cb6c9 100644 --- a/picard/item.py +++ b/picard/item.py @@ -110,7 +110,7 @@ def tracknumber(self): """The track number as an int.""" try: return int(self.metadata.get('tracknumber', '0').split('/')[0]) - except BaseException: + except ValueError: return 0 @property From 77ef1c404c8bfad8719bd61854570bf57c0c7f4e Mon Sep 17 00:00:00 2001 From: Laurent Monin Date: Tue, 7 Jan 2025 21:10:20 +0100 Subject: [PATCH 5/5] Use a common function for both tracknumber() and discnumber() --- picard/item.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/picard/item.py b/picard/item.py index ebb24cb6c9..970e68fc84 100644 --- a/picard/item.py +++ b/picard/item.py @@ -108,16 +108,17 @@ def load(self, priority=False, refresh=False): @property def tracknumber(self): """The track number as an int.""" - try: - return int(self.metadata.get('tracknumber', '0').split('/')[0]) - except ValueError: - return 0 + return self._track_or_disc_number('tracknumber') @property def discnumber(self): """The disc number as an int.""" + return self._track_or_disc_number('discnumber') + + def _track_or_disc_number(self, field): + """Extract tracknumber or discnumber as int, defaults to 0.""" try: - return int(self.metadata.get('discnumber', '0').split('/')[0]) + return int(self.metadata.get(field, '0').split('/')[0]) except ValueError: return 0