From 85244ec177fef30f5f64a04c6845fbe69f33535d Mon Sep 17 00:00:00 2001 From: Jesse Weinstein Date: Wed, 24 Jun 2015 22:53:16 -0700 Subject: [PATCH 01/18] Fix singular vs plural noun usage --- beets/ui/commands.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/beets/ui/commands.py b/beets/ui/commands.py index 00006ce51..4b403764f 100644 --- a/beets/ui/commands.py +++ b/beets/ui/commands.py @@ -1107,11 +1107,11 @@ def remove_items(lib, query, album, delete): print_() if delete: fmt = u'$path - $title' - prompt = 'Really DELETE %i files (y/n)?' % len(items) + prompt = 'Really DELETE %i file%s (y/n)?' % (len(items), 's' if len(items)>1 else '') else: fmt = '' - prompt = 'Really remove %i items from the library (y/n)?' % \ - len(items) + prompt = 'Really remove %i item%s from the library (y/n)?' % \ + (len(items), 's' if len(items)>1 else '') # Show all the items. for item in items: From fa7b780b862ae1a84441f506de7dc208b3c474cc Mon Sep 17 00:00:00 2001 From: Jesse Weinstein Date: Wed, 24 Jun 2015 23:08:28 -0700 Subject: [PATCH 02/18] Fix singular/plural issue in move subcommand. --- beets/ui/commands.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beets/ui/commands.py b/beets/ui/commands.py index 4b403764f..ff5a807c9 100644 --- a/beets/ui/commands.py +++ b/beets/ui/commands.py @@ -1354,7 +1354,7 @@ def move_items(lib, dest, query, copy, album, pretend): action = 'Copying' if copy else 'Moving' entity = 'album' if album else 'item' - log.info(u'{0} {1} {2}s.', action, len(objs), entity) + log.info(u'{0} {1} {2}{3}.', action, len(objs), entity, 's' if len(objs)>1 else '') if pretend: if album: show_path_changes([(item.path, item.destination(basedir=dest)) From e360438977eb1e398a0413a4394a8e020a4be5bc Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Thu, 25 Jun 2015 21:27:10 -0700 Subject: [PATCH 03/18] Note about escaping regex arguments in shell A la #1520. --- docs/reference/query.rst | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/docs/reference/query.rst b/docs/reference/query.rst index 29c32a222..e2091bb14 100644 --- a/docs/reference/query.rst +++ b/docs/reference/query.rst @@ -100,7 +100,7 @@ While ordinary keywords perform simple substring matches, beets also supports regular expression matching for more advanced queries. To run a regex query, use an additional ``:`` between the field name and the expression:: - $ beet list 'artist::Ann(a|ie)' + $ beet list "artist::Ann(a|ie)" That query finds songs by Anna Calvi and Annie but not Annuals. Similarly, this query prints the path to any file in my library that's missing a track title:: @@ -110,11 +110,16 @@ query prints the path to any file in my library that's missing a track title:: To search *all* fields using a regular expression, just prefix the expression with a single ``:``, like so:: - $ beet list :Ho[pm]eless + $ beet list ":Ho[pm]eless" Regular expressions are case-sensitive and build on `Python's built-in implementation`_. See Python's documentation for specifics on regex syntax. +Most command-line shells will try to interpret common characters in regular +expressions, such as ``()[]|``. To type those characters, you'll need to +escape them (e.g., with backslashes or quotation marks, depending on your +shell). + .. _Python's built-in implementation: http://docs.python.org/library/re.html From 6df54f35ec632eb7d1a4cd4f3365bfde37b276a5 Mon Sep 17 00:00:00 2001 From: Jesse Weinstein Date: Fri, 26 Jun 2015 00:46:30 -0700 Subject: [PATCH 04/18] Fix flake8 warnings --- beets/ui/commands.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/beets/ui/commands.py b/beets/ui/commands.py index ff5a807c9..76473e678 100644 --- a/beets/ui/commands.py +++ b/beets/ui/commands.py @@ -1107,11 +1107,12 @@ def remove_items(lib, query, album, delete): print_() if delete: fmt = u'$path - $title' - prompt = 'Really DELETE %i file%s (y/n)?' % (len(items), 's' if len(items)>1 else '') + prompt = 'Really DELETE %i file%s (y/n)?' % \ + (len(items), 's' if len(items) > 1 else '') else: fmt = '' prompt = 'Really remove %i item%s from the library (y/n)?' % \ - (len(items), 's' if len(items)>1 else '') + (len(items), 's' if len(items) > 1 else '') # Show all the items. for item in items: @@ -1354,7 +1355,8 @@ def move_items(lib, dest, query, copy, album, pretend): action = 'Copying' if copy else 'Moving' entity = 'album' if album else 'item' - log.info(u'{0} {1} {2}{3}.', action, len(objs), entity, 's' if len(objs)>1 else '') + log.info(u'{0} {1} {2}{3}.', action, len(objs), entity, + 's' if len(objs) > 1 else '') if pretend: if album: show_path_changes([(item.path, item.destination(basedir=dest)) From 1f72184533867cd5640a6bc856f433c0220bb763 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Fri, 26 Jun 2015 16:31:49 -0700 Subject: [PATCH 05/18] Changelog for #1521 --- docs/changelog.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index b085c271c..335b88346 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -63,6 +63,8 @@ Fixes: * :doc:`/plugins/fetchart`: When album art is already present, the message is now printed in the ``text_highlight_minor`` color (light gray). Thanks to :user:`Somasis`. :bug:`1512` +* Some messages in the console UI now use plural nouns correctly. Thanks to + :user:`JesseWeinstein`. :bug:`1521` 1.3.13 (April 24, 2015) From d495d35a97d8e89ff4572cdbf80469b3d63dc8d7 Mon Sep 17 00:00:00 2001 From: Fabrice Laporte Date: Fri, 3 Jul 2015 14:39:40 +0200 Subject: [PATCH 06/18] Update lyrics.rst --- docs/plugins/lyrics.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/plugins/lyrics.rst b/docs/plugins/lyrics.rst index e7ecd1b04..521e1733f 100644 --- a/docs/plugins/lyrics.rst +++ b/docs/plugins/lyrics.rst @@ -52,7 +52,7 @@ configuration file. The available options are: - **sources**: List of sources to search for lyrics. An asterisk `*` expands to all available sources. Default: ``google lyricwiki lyrics.com musixmatch``, i.e., all sources. - *google* source will be automatically deactivated if no `google_engine_ID` is + *google* source will be automatically deactivated if no `google_API_key` is setup. Here's an example of ``config.yaml``:: From de100be923ed1174056b93330b9335a206e4c950 Mon Sep 17 00:00:00 2001 From: Fabrice Laporte Date: Fri, 3 Jul 2015 14:40:38 +0200 Subject: [PATCH 07/18] Update lyrics.rst --- docs/plugins/lyrics.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/plugins/lyrics.rst b/docs/plugins/lyrics.rst index 521e1733f..1d91a00eb 100644 --- a/docs/plugins/lyrics.rst +++ b/docs/plugins/lyrics.rst @@ -49,10 +49,10 @@ configuration file. The available options are: - **google_engine_ID**: The custom search engine to use. Default: The `beets custom search engine`_, which gathers an updated list of sources known to be scrapeable. -- **sources**: List of sources to search for lyrics. An asterisk `*` expands +- **sources**: List of sources to search for lyrics. An asterisk ``*`` expands to all available sources. Default: ``google lyricwiki lyrics.com musixmatch``, i.e., all sources. - *google* source will be automatically deactivated if no `google_API_key` is + *google* source will be automatically deactivated if no ``google_API_key`` is setup. Here's an example of ``config.yaml``:: From b8fdd163b942aa65ca7599a3e3fabc77981c3838 Mon Sep 17 00:00:00 2001 From: multikatt Date: Sun, 5 Jul 2015 14:29:37 -0400 Subject: [PATCH 08/18] Fix capitalization of examples and add a comma Fixes syntax highlighting --- docs/dev/plugins.rst | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/dev/plugins.rst b/docs/dev/plugins.rst index a97a8baa9..b1cf2deca 100644 --- a/docs/dev/plugins.rst +++ b/docs/dev/plugins.rst @@ -361,11 +361,11 @@ method. Here's an example plugin that provides a meaningless new field "foo":: - class fooplugin(beetsplugin): + class FooPlugin(BeetsPlugin): def __init__(self): - field = mediafile.mediafield( - mediafile.mp3descstoragestyle(u'foo') - mediafile.storagestyle(u'foo') + field = mediafile.MediaField( + mediafile.MP3DescStorageStyle(u'foo'), + mediafile.StorageStyle(u'foo') ) self.add_media_field('foo', field) From 18bd4471e47eb1e8d1e70bffbfb046f16e8690de Mon Sep 17 00:00:00 2001 From: Tom Jaspers Date: Sun, 5 Jul 2015 19:56:14 +0100 Subject: [PATCH 09/18] Fix sorting of track numbers when case insensitive `LOWER()` implicitly converted numerical columns to strings, causing the ordering of ['1', '10', '2'] to be correct. The type of the column is now checked in the sql query using `CASE WHEN..` construct. This ensures the column is only `LOWER()`'d when dealing with TEXT or BLOB. - Add a test to check for the track number behavior - Add changelog entry Fix #1511 --- beets/dbcore/query.py | 5 ++++- docs/changelog.rst | 1 + test/test_sort.py | 22 +++++++++++++++++++--- 3 files changed, 24 insertions(+), 4 deletions(-) diff --git a/beets/dbcore/query.py b/beets/dbcore/query.py index fff72d5f0..c04e734b8 100644 --- a/beets/dbcore/query.py +++ b/beets/dbcore/query.py @@ -766,7 +766,10 @@ class FixedFieldSort(FieldSort): def order_clause(self): order = "ASC" if self.ascending else "DESC" if self.case_insensitive: - field = 'LOWER({})'.format(self.field) + field = '(CASE ' \ + 'WHEN TYPEOF({0})="text" THEN LOWER({0}) ' \ + 'WHEN TYPEOF({0})="blob" THEN LOWER({0}) ' \ + 'ELSE {0} END)'.format(self.field) else: field = self.field return "{0} {1}".format(field, order) diff --git a/docs/changelog.rst b/docs/changelog.rst index 335b88346..a8f68cd16 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -65,6 +65,7 @@ Fixes: :user:`Somasis`. :bug:`1512` * Some messages in the console UI now use plural nouns correctly. Thanks to :user:`JesseWeinstein`. :bug:`1521` +* Sorting numerical fields (such as track) now works again. :bug:`1511` 1.3.13 (April 24, 2015) diff --git a/test/test_sort.py b/test/test_sort.py index 519d19c6e..e763b6167 100644 --- a/test/test_sort.py +++ b/test/test_sort.py @@ -34,21 +34,21 @@ class DummyDataTestCase(_common.TestCase): albums = [_common.album() for _ in range(3)] albums[0].album = "Album A" albums[0].genre = "Rock" - albums[0].year = "2001" + albums[0].year = 2001 albums[0].flex1 = "Flex1-1" albums[0].flex2 = "Flex2-A" albums[0].albumartist = "Foo" albums[0].albumartist_sort = None albums[1].album = "Album B" albums[1].genre = "Rock" - albums[1].year = "2001" + albums[1].year = 2001 albums[1].flex1 = "Flex1-2" albums[1].flex2 = "Flex2-A" albums[1].albumartist = "Bar" albums[1].albumartist_sort = None albums[2].album = "Album C" albums[2].genre = "Jazz" - albums[2].year = "2005" + albums[2].year = 2005 albums[2].flex1 = "Flex1-1" albums[2].flex2 = "Flex2-B" albums[2].albumartist = "Baz" @@ -67,6 +67,7 @@ class DummyDataTestCase(_common.TestCase): items[0].album_id = albums[0].id items[0].artist_sort = None items[0].path = "/path0.mp3" + items[0].track = 1 items[1].title = 'Baz qux' items[1].artist = 'Two' items[1].album = 'Baz' @@ -77,6 +78,7 @@ class DummyDataTestCase(_common.TestCase): items[1].album_id = albums[0].id items[1].artist_sort = None items[1].path = "/patH1.mp3" + items[1].track = 2 items[2].title = 'Beets 4 eva' items[2].artist = 'Three' items[2].album = 'Foo' @@ -87,6 +89,7 @@ class DummyDataTestCase(_common.TestCase): items[2].album_id = albums[1].id items[2].artist_sort = None items[2].path = "/paTH2.mp3" + items[2].track = 3 items[3].title = 'Beets 4 eva' items[3].artist = 'Three' items[3].album = 'Foo2' @@ -97,6 +100,7 @@ class DummyDataTestCase(_common.TestCase): items[3].album_id = albums[2].id items[3].artist_sort = None items[3].path = "/PATH3.mp3" + items[3].track = 4 for item in items: self.lib.add(item) @@ -399,6 +403,7 @@ class CaseSensitivityTest(DummyDataTestCase, _common.TestCase): item.flex2 = "flex2-A" item.album_id = album.id item.artist_sort = None + item.track = 10 self.lib.add(item) self.new_album = album @@ -451,6 +456,17 @@ class CaseSensitivityTest(DummyDataTestCase, _common.TestCase): self.assertEqual(results[0].flex1, 'Flex1-0') self.assertEqual(results[-1].flex1, 'flex1') + def test_case_sensitive_only_affects_text(self): + config['sort_case_insensitive'] = True + q = 'track+' + results = list(self.lib.items(q)) + # If the numerical values were sorted as strings, + # then ['1', '10', '2'] would be valid. + print([r.track for r in results]) + self.assertEqual(results[0].track, 1) + self.assertEqual(results[1].track, 2) + self.assertEqual(results[-1].track, 10) + def suite(): return unittest.TestLoader().loadTestsFromName(__name__) From d07c8fde69d1dc3f4c25c2d315745e5e50d6e3fb Mon Sep 17 00:00:00 2001 From: Ben Ockmore Date: Mon, 16 Mar 2015 10:48:45 +0000 Subject: [PATCH 10/18] Added loop to iterate over sanitize/truncate until stable. Enabled test_truncation_does_not_conflict_with_replacement test. Fixes #496. --- beets/library.py | 86 +++++++++++++++++++++++++++++++++----------- test/test_library.py | 1 - 2 files changed, 66 insertions(+), 21 deletions(-) diff --git a/beets/library.py b/beets/library.py index 006630c08..0ba29cd68 100644 --- a/beets/library.py +++ b/beets/library.py @@ -742,6 +742,71 @@ class Item(LibModel): # Templating. + def _legalize_partial(self, path, fragment, replacements): + """ Perform a partial legalization of the path (ie. a single + sanitization and truncation). Outputs unicode if fragment is set, + otherwise bytes. + """ + + # Truncate components and remove forbidden characters. + path = util.sanitize_path(path, replacements) + + # Encode for the filesystem. + if not fragment: + path = bytestring_path(path) + + # Preserve extension. + _, extension = os.path.splitext(self.path) + if fragment: + # Outputting Unicode. + extension = extension.decode('utf8', 'ignore') + path += extension.lower() + + # Truncate too-long components. + maxlen = beets.config['max_filename_length'].get(int) + if not maxlen: + # When zero, try to determine from filesystem. + maxlen = util.max_filename_length(self._db.directory) + path = util.truncate_path(path, maxlen) + + return path + + def _legalize_path(self, path, fragment): + """ Perform several iterations of _legalize_partial, to generate a + stable, optimal output path. This is necessary for cases where + truncation produces unclean paths (eg. trailing space). + """ + + # Create a list of path candidates + path_candidates = [b''] + + replacements = self._db.replacements + + # Perform an initial pass + path = self._legalize_partial(path, fragment, replacements) + while path != path_candidates[-1]: + # This will keep sanitizing the path until it's stable, or an + # infinite loop appears + while path not in path_candidates: + path_candidates.append(path) + # Convert back to Unicode with extension removed + print(util.displayable_path(path)) + path = os.path.splitext(util.displayable_path(path))[0] + + # Run next pass + path = self._legalize_partial(path, fragment, replacements) + + # If an infinite loop occurred, adjust replacements to avoid it + if path != path_candidates[-1]: + replacements = dict((k, u'_') for k, v in replacements) + + # If there's a rule to match a single underscore, set the + # target to a blank string. + if '_' in replacements: + replacements['_'] = '' + + return path + def destination(self, fragment=False, basedir=None, platform=None, path_formats=None): """Returns the path in the library directory designated for the @@ -790,26 +855,7 @@ class Item(LibModel): if beets.config['asciify_paths']: subpath = unidecode(subpath) - # Truncate components and remove forbidden characters. - subpath = util.sanitize_path(subpath, self._db.replacements) - - # Encode for the filesystem. - if not fragment: - subpath = bytestring_path(subpath) - - # Preserve extension. - _, extension = os.path.splitext(self.path) - if fragment: - # Outputting Unicode. - extension = extension.decode('utf8', 'ignore') - subpath += extension.lower() - - # Truncate too-long components. - maxlen = beets.config['max_filename_length'].get(int) - if not maxlen: - # When zero, try to determine from filesystem. - maxlen = util.max_filename_length(self._db.directory) - subpath = util.truncate_path(subpath, maxlen) + subpath = self._legalize_path(subpath, fragment) if fragment: return subpath diff --git a/test/test_library.py b/test/test_library.py index a4b4d08a8..e66ad28f0 100644 --- a/test/test_library.py +++ b/test/test_library.py @@ -452,7 +452,6 @@ class DestinationTest(_common.TestCase): self.assertEqual(self.i.destination(), np('base/one/_.mp3')) - @unittest.skip('unimplemented: #496') def test_truncation_does_not_conflict_with_replacement(self): # Use a replacement that should always replace the last X in any # path component with a Z. From 493fbab1a5da81bb5f30a00bf58d24ae9eb9e80e Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Mon, 6 Jul 2015 16:23:04 -0700 Subject: [PATCH 11/18] replaygain: Fix #1518, GStreamer missing plugins --- beetsplug/replaygain.py | 6 ++++++ docs/changelog.rst | 2 ++ 2 files changed, 8 insertions(+) diff --git a/beetsplug/replaygain.py b/beetsplug/replaygain.py index e19101e3e..3c9c0042a 100644 --- a/beetsplug/replaygain.py +++ b/beetsplug/replaygain.py @@ -388,6 +388,12 @@ class GStreamerBackend(Backend): self._res = self.Gst.ElementFactory.make("audioresample", "res") self._rg = self.Gst.ElementFactory.make("rganalysis", "rg") + if self._src is None or self._decbin is None or self._conv is None \ + or self._res is None or self._rg is None: + raise FatalReplayGainError( + "Failed to load required GStreamer plugins" + ) + # We check which files need gain ourselves, so all files given # to rganalsys should have their gain computed, even if it # already exists. diff --git a/docs/changelog.rst b/docs/changelog.rst index a8f68cd16..a651572c9 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -66,6 +66,8 @@ Fixes: * Some messages in the console UI now use plural nouns correctly. Thanks to :user:`JesseWeinstein`. :bug:`1521` * Sorting numerical fields (such as track) now works again. :bug:`1511` +* :doc:`/plugins/replaygain`: Missing GStreamer plugins now cause a helpful + error message instead of a crash. :bug:`1518` 1.3.13 (April 24, 2015) From de17d000bda4c518bc5191f09e379624d7068cbb Mon Sep 17 00:00:00 2001 From: Ben Ockmore Date: Tue, 7 Jul 2015 01:17:12 +0100 Subject: [PATCH 12/18] Rewrote path legalization code to be two module functions rather than class methods. Also made algorithm more predictable, and added an extra test. --- beets/library.py | 75 +++++------------------------------------- beets/util/__init__.py | 53 +++++++++++++++++++++++++++++ test/test_library.py | 20 +++++++++-- 3 files changed, 79 insertions(+), 69 deletions(-) diff --git a/beets/library.py b/beets/library.py index 0ba29cd68..fc51a6c6a 100644 --- a/beets/library.py +++ b/beets/library.py @@ -30,7 +30,7 @@ from beets import logging from beets.mediafile import MediaFile, MutagenError, UnreadableFileError from beets import plugins from beets import util -from beets.util import bytestring_path, syspath, normpath, samefile +from beets.util import bytestring_path, syspath, normpath, samefile, legalize_path from beets.util.functemplate import Template from beets import dbcore from beets.dbcore import types @@ -742,71 +742,6 @@ class Item(LibModel): # Templating. - def _legalize_partial(self, path, fragment, replacements): - """ Perform a partial legalization of the path (ie. a single - sanitization and truncation). Outputs unicode if fragment is set, - otherwise bytes. - """ - - # Truncate components and remove forbidden characters. - path = util.sanitize_path(path, replacements) - - # Encode for the filesystem. - if not fragment: - path = bytestring_path(path) - - # Preserve extension. - _, extension = os.path.splitext(self.path) - if fragment: - # Outputting Unicode. - extension = extension.decode('utf8', 'ignore') - path += extension.lower() - - # Truncate too-long components. - maxlen = beets.config['max_filename_length'].get(int) - if not maxlen: - # When zero, try to determine from filesystem. - maxlen = util.max_filename_length(self._db.directory) - path = util.truncate_path(path, maxlen) - - return path - - def _legalize_path(self, path, fragment): - """ Perform several iterations of _legalize_partial, to generate a - stable, optimal output path. This is necessary for cases where - truncation produces unclean paths (eg. trailing space). - """ - - # Create a list of path candidates - path_candidates = [b''] - - replacements = self._db.replacements - - # Perform an initial pass - path = self._legalize_partial(path, fragment, replacements) - while path != path_candidates[-1]: - # This will keep sanitizing the path until it's stable, or an - # infinite loop appears - while path not in path_candidates: - path_candidates.append(path) - # Convert back to Unicode with extension removed - print(util.displayable_path(path)) - path = os.path.splitext(util.displayable_path(path))[0] - - # Run next pass - path = self._legalize_partial(path, fragment, replacements) - - # If an infinite loop occurred, adjust replacements to avoid it - if path != path_candidates[-1]: - replacements = dict((k, u'_') for k, v in replacements) - - # If there's a rule to match a single underscore, set the - # target to a blank string. - if '_' in replacements: - replacements['_'] = '' - - return path - def destination(self, fragment=False, basedir=None, platform=None, path_formats=None): """Returns the path in the library directory designated for the @@ -855,7 +790,13 @@ class Item(LibModel): if beets.config['asciify_paths']: subpath = unidecode(subpath) - subpath = self._legalize_path(subpath, fragment) + maxlen = beets.config['max_filename_length'].get(int) + if not maxlen: + # When zero, try to determine from filesystem. + maxlen = util.max_filename_length(self._db.directory) + + subpath = util.legalize_path(subpath, self._db.replacements, maxlen, + os.path.splitext(self.path)[1], fragment) if fragment: return subpath diff --git a/beets/util/__init__.py b/beets/util/__init__.py index 68be740f6..a82bb2c89 100644 --- a/beets/util/__init__.py +++ b/beets/util/__init__.py @@ -545,6 +545,59 @@ def truncate_path(path, length=MAX_FILENAME_LENGTH): return os.path.join(*out) +def _legalize_stage(path, replacements, length, extension, fragment): + """ Perform a signle stage of legalization of the path (ie. a single + sanitization and truncation). Returns the path (unicode if fragment is set, + otherwise bytes), and whether truncation was required. + """ + # Perform an initial sanitization including user replacements. + path = sanitize_path(path, replacements) + + # Encode for the filesystem. + if not fragment: + path = bytestring_path(path) + + # Preserve extension. + path += extension.lower() + + # Truncate too-long components. + pre_truncate_path = path + path = truncate_path(path, length) + + return (path, path != pre_truncate_path) + + +def legalize_path(path, replacements, length, extension, fragment): + """ Perform up to three calls to _legalize_stage, to generate a stable + output path, taking user replacements into consideration if possible. This + additional complexity is necessary for cases where truncation produces + unclean paths (eg. trailing space). + """ + + if fragment: + # Outputting Unicode. + extension = extension.decode('utf8', 'ignore') + + first_stage_path =\ + _legalize_stage(path, replacements, length, extension, fragment)[0] + + # Convert back to Unicode with extension removed. + first_stage_path = os.path.splitext(displayable_path(first_stage_path))[0] + + # Re-sanitize following truncation (including user replacements). + second_stage_path, retruncated = _legalize_stage( + first_stage_path, replacements, length, extension, fragment + ) + + # If the path was once again truncated, discard user replacements. + if retruncated: + second_stage_path = _legalize_stage( + first_stage_path, None, length, extension, fragment + )[0] + + return second_stage_path + + def str2bool(value): """Returns a boolean reflecting a human-entered string.""" return value.lower() in ('yes', '1', 'true', 't', 'y') diff --git a/test/test_library.py b/test/test_library.py index e66ad28f0..30b0d6fc4 100644 --- a/test/test_library.py +++ b/test/test_library.py @@ -452,7 +452,7 @@ class DestinationTest(_common.TestCase): self.assertEqual(self.i.destination(), np('base/one/_.mp3')) - def test_truncation_does_not_conflict_with_replacement(self): + def test_legalize_path_one_for_one_replacement(self): # Use a replacement that should always replace the last X in any # path component with a Z. self.lib.replacements = [ @@ -465,7 +465,23 @@ class DestinationTest(_common.TestCase): # The final path should reflect the replacement. dest = self.i.destination() - self.assertTrue('XZ' in dest) + self.assertEqual(dest[-2:], 'XZ') + + def test_legalize_path_one_for_many_replacement(self): + # Use a replacement that should always replace the last X in any + # path component with four Zs. + self.lib.replacements = [ + (re.compile(r'X$'), u'ZZZZ'), + ] + + # Construct an item whose untruncated path ends with a Y but whose + # truncated version ends with an X. + self.i.title = 'X' * 300 + 'Y' + + # The final path should ignore the user replacement and create a path + # of the correct length, containing Xs. + dest = self.i.destination() + self.assertEqual(dest[-2:], 'XX') class ItemFormattedMappingTest(_common.LibTestCase): From 22b2527eed95ded41d5a650c27eb9d3dd0300e31 Mon Sep 17 00:00:00 2001 From: Ben Ockmore Date: Tue, 7 Jul 2015 01:28:16 +0100 Subject: [PATCH 13/18] Remove unused import. --- beets/library.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beets/library.py b/beets/library.py index fc51a6c6a..9d491a66b 100644 --- a/beets/library.py +++ b/beets/library.py @@ -30,7 +30,7 @@ from beets import logging from beets.mediafile import MediaFile, MutagenError, UnreadableFileError from beets import plugins from beets import util -from beets.util import bytestring_path, syspath, normpath, samefile, legalize_path +from beets.util import bytestring_path, syspath, normpath, samefile from beets.util.functemplate import Template from beets import dbcore from beets.dbcore import types From b479982043d319c1f22b0b428af93631db3d0938 Mon Sep 17 00:00:00 2001 From: Ben Ockmore Date: Tue, 7 Jul 2015 12:02:19 +0100 Subject: [PATCH 14/18] Minor changes suggested suggested in PR comments. --- beets/util/__init__.py | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/beets/util/__init__.py b/beets/util/__init__.py index a82bb2c89..bf0abdfe5 100644 --- a/beets/util/__init__.py +++ b/beets/util/__init__.py @@ -546,9 +546,9 @@ def truncate_path(path, length=MAX_FILENAME_LENGTH): def _legalize_stage(path, replacements, length, extension, fragment): - """ Perform a signle stage of legalization of the path (ie. a single - sanitization and truncation). Returns the path (unicode if fragment is set, - otherwise bytes), and whether truncation was required. + """ Performs sanitization of the path, then encodes for the filesystem, + appends the extension and truncates. Returns the path (unicode if fragment + is set, otherwise bytes), and whether truncation was required. """ # Perform an initial sanitization including user replacements. path = sanitize_path(path, replacements) @@ -564,22 +564,24 @@ def _legalize_stage(path, replacements, length, extension, fragment): pre_truncate_path = path path = truncate_path(path, length) - return (path, path != pre_truncate_path) + return path, path != pre_truncate_path def legalize_path(path, replacements, length, extension, fragment): """ Perform up to three calls to _legalize_stage, to generate a stable - output path, taking user replacements into consideration if possible. This - additional complexity is necessary for cases where truncation produces - unclean paths (eg. trailing space). + output path, taking user replacements into consideration if possible. The + limited number of iterations avoids the possibility of an infinite loop of + sanitization and truncation operations, which could be caused by some user + replacements. """ if fragment: # Outputting Unicode. extension = extension.decode('utf8', 'ignore') - first_stage_path =\ - _legalize_stage(path, replacements, length, extension, fragment)[0] + first_stage_path, _ = _legalize_stage( + path, replacements, length, extension, fragment + ) # Convert back to Unicode with extension removed. first_stage_path = os.path.splitext(displayable_path(first_stage_path))[0] @@ -591,9 +593,9 @@ def legalize_path(path, replacements, length, extension, fragment): # If the path was once again truncated, discard user replacements. if retruncated: - second_stage_path = _legalize_stage( + second_stage_path, _ = _legalize_stage( first_stage_path, None, length, extension, fragment - )[0] + ) return second_stage_path From 1f1e0f7240fc418af13d848b7a37a00a46a1dcf8 Mon Sep 17 00:00:00 2001 From: Ben Ockmore Date: Tue, 7 Jul 2015 17:46:42 +0100 Subject: [PATCH 15/18] Added warning message and paragraph about replacements/max length interaction in documentation. --- beets/library.py | 10 ++++++++-- beets/util/__init__.py | 4 +++- docs/reference/config.rst | 5 +++++ 3 files changed, 16 insertions(+), 3 deletions(-) diff --git a/beets/library.py b/beets/library.py index 9d491a66b..10e54b2a1 100644 --- a/beets/library.py +++ b/beets/library.py @@ -795,8 +795,14 @@ class Item(LibModel): # When zero, try to determine from filesystem. maxlen = util.max_filename_length(self._db.directory) - subpath = util.legalize_path(subpath, self._db.replacements, maxlen, - os.path.splitext(self.path)[1], fragment) + subpath, fellback = util.legalize_path( + subpath, self._db.replacements, maxlen, + os.path.splitext(self.path)[1], fragment + ) + + # Print an error message if legalize fell back to default replacements + if fellback: + log.warning(u'fell back to default replacements when naming file') if fragment: return subpath diff --git a/beets/util/__init__.py b/beets/util/__init__.py index bf0abdfe5..42971fdf6 100644 --- a/beets/util/__init__.py +++ b/beets/util/__init__.py @@ -597,7 +597,9 @@ def legalize_path(path, replacements, length, extension, fragment): first_stage_path, None, length, extension, fragment ) - return second_stage_path + return second_stage_path, True + else: + return second_stage_path, False def str2bool(value): diff --git a/docs/reference/config.rst b/docs/reference/config.rst index d737bba69..9000f070a 100644 --- a/docs/reference/config.rst +++ b/docs/reference/config.rst @@ -119,6 +119,11 @@ compatibility with Windows-influenced network filesystems like Samba). Trailing dots and trailing whitespace, which can cause problems on Windows clients, are also removed. +When replacements other than the defaults are used, it is possible that they +will increase the length of the path. In the scenario where this leads to a +conflict with the maximum filename length, the default replacements will be +used to resolve the conflict and beets will display a warning. + Note that paths might contain special characters such as typographical quotes (``“”``). With the configuration above, those will not be replaced as they don't match the typewriter quote (``"``). To also strip these From 12295376eb0a185a46675cf91d0e74069b1f8070 Mon Sep 17 00:00:00 2001 From: Tom Jaspers Date: Tue, 7 Jul 2015 22:56:02 +0100 Subject: [PATCH 16/18] Create empty user config file if it does not exist Running `beet config -e` with a non-existing configuration file does not always work (e.g., OSX uses `open` by default, which requires the file to exist). This could occur during e.g. initial setup. Resolve #1480 --- beets/ui/commands.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/beets/ui/commands.py b/beets/ui/commands.py index 76473e678..bbdbbcf96 100644 --- a/beets/ui/commands.py +++ b/beets/ui/commands.py @@ -1482,11 +1482,13 @@ def config_func(lib, opts, args): def config_edit(): """Open a program to edit the user configuration. + An empty config file is created if no existing config file exists. """ path = config.user_config_path() - editor = os.environ.get('EDITOR') try: + if not os.path.isfile(path): + open(path, 'w+').close() util.interactive_open(path, editor) except OSError as exc: message = "Could not edit configuration: {0}".format(exc) From 39809a8a440583ad06c85557bc97137da2204c61 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Tue, 7 Jul 2015 18:17:12 -0700 Subject: [PATCH 17/18] Expand a little on the docs for #1361 Fix #496, at long last. --- beets/library.py | 8 +++++--- beets/util/__init__.py | 41 ++++++++++++++++++++++++++++------------- 2 files changed, 33 insertions(+), 16 deletions(-) diff --git a/beets/library.py b/beets/library.py index 10e54b2a1..9d11b166c 100644 --- a/beets/library.py +++ b/beets/library.py @@ -799,10 +799,12 @@ class Item(LibModel): subpath, self._db.replacements, maxlen, os.path.splitext(self.path)[1], fragment ) - - # Print an error message if legalize fell back to default replacements if fellback: - log.warning(u'fell back to default replacements when naming file') + # Print an error message if legalization fell back to + # default replacements because of the maximum length. + log.warning('Fell back to default replacements when naming ' + 'file {}. Configure replacements to avoid lengthening ' + 'the filename.', subpath) if fragment: return subpath diff --git a/beets/util/__init__.py b/beets/util/__init__.py index 42971fdf6..30a1fcba9 100644 --- a/beets/util/__init__.py +++ b/beets/util/__init__.py @@ -546,9 +546,11 @@ def truncate_path(path, length=MAX_FILENAME_LENGTH): def _legalize_stage(path, replacements, length, extension, fragment): - """ Performs sanitization of the path, then encodes for the filesystem, - appends the extension and truncates. Returns the path (unicode if fragment - is set, otherwise bytes), and whether truncation was required. + """Perform a single round of path legalization steps + (sanitation/replacement, encoding from Unicode to bytes, + extension-appending, and truncation). Return the path (Unicode if + `fragment` is set, `bytes` otherwise) and whether truncation was + required. """ # Perform an initial sanitization including user replacements. path = sanitize_path(path, replacements) @@ -568,11 +570,25 @@ def _legalize_stage(path, replacements, length, extension, fragment): def legalize_path(path, replacements, length, extension, fragment): - """ Perform up to three calls to _legalize_stage, to generate a stable - output path, taking user replacements into consideration if possible. The - limited number of iterations avoids the possibility of an infinite loop of - sanitization and truncation operations, which could be caused by some user - replacements. + """Given a path-like Unicode string, produce a legal path. Return + the path and a flag indicating whether some replacements had to be + ignored (see below). + + The legalization process (see `_legalize_stage`) consists of + applying the sanitation rules in `replacements`, encoding the string + to bytes (unless `fragment` is set), truncating components to + `length`, appending the `extension`. + + This function performs up to three calls to `_legalize_stage` in + case truncation conflicts with replacements (as can happen when + truncation creates whitespace at the end of the string, for + example). The limited number of iterations iterations avoids the + possibility of an infinite loop of sanitation and truncation + operations, which could be caused by replacement rules that make the + string longer. The flag returned from this function indicates that + the path has to be truncated twice (indicating that replacements + made the string longer again after it was truncated); the + application should probably log some sort of warning. """ if fragment: @@ -584,22 +600,21 @@ def legalize_path(path, replacements, length, extension, fragment): ) # Convert back to Unicode with extension removed. - first_stage_path = os.path.splitext(displayable_path(first_stage_path))[0] + first_stage_path, _ = os.path.splitext(displayable_path(first_stage_path)) # Re-sanitize following truncation (including user replacements). second_stage_path, retruncated = _legalize_stage( first_stage_path, replacements, length, extension, fragment ) - # If the path was once again truncated, discard user replacements. + # If the path was once again truncated, discard user replacements + # and run through one last legalization stage. if retruncated: second_stage_path, _ = _legalize_stage( first_stage_path, None, length, extension, fragment ) - return second_stage_path, True - else: - return second_stage_path, False + return second_stage_path, retruncated def str2bool(value): From ae68cedeafd1e0cc7e7b7f194fc90aeaecf64e28 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Tue, 7 Jul 2015 18:21:15 -0700 Subject: [PATCH 18/18] Changelog for #496 --- docs/changelog.rst | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index a651572c9..a0e7d70c3 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -68,6 +68,9 @@ Fixes: * Sorting numerical fields (such as track) now works again. :bug:`1511` * :doc:`/plugins/replaygain`: Missing GStreamer plugins now cause a helpful error message instead of a crash. :bug:`1518` +* Fix an edge case when producing sanitized filenames where the maximum path + length conflicted with the :ref:`replace` rules. Thanks to Ben Ockmore. + :bug:`496` :bug:`1361` 1.3.13 (April 24, 2015)