diff --git a/beetsplug/smartplaylist.py b/beetsplug/smartplaylist.py index 8203ce4ef..c0404b7d6 100644 --- a/beetsplug/smartplaylist.py +++ b/beetsplug/smartplaylist.py @@ -19,8 +19,7 @@ from urllib.parse import quote from urllib.request import pathname2url from beets import ui -from beets.dbcore import OrQuery -from beets.dbcore.query import MultipleSort, ParsingError +from beets.dbcore.query import ParsingError from beets.library import Album, Item, parse_query_string from beets.plugins import BeetsPlugin from beets.plugins import send as send_event @@ -190,25 +189,12 @@ class SmartPlaylistPlugin(BeetsPlugin): elif len(qs) == 1: query_and_sort = parse_query_string(qs[0], model_cls) else: - # multiple queries and sorts - queries, sorts = zip( - *(parse_query_string(q, model_cls) for q in qs) + # multiple queries and sorts - preserve order + # Store tuple of (query, sort) tuples for hashability + queries_and_sorts = tuple( + parse_query_string(q, model_cls) for q in qs ) - query = OrQuery(queries) - final_sorts = [] - for s in sorts: - if s: - if isinstance(s, MultipleSort): - final_sorts += s.sorts - else: - final_sorts.append(s) - if not final_sorts: - sort = None - elif len(final_sorts) == 1: - (sort,) = final_sorts - else: - sort = MultipleSort(final_sorts) - query_and_sort = query, sort + query_and_sort = queries_and_sorts, None playlist_data += (query_and_sort,) @@ -221,10 +207,17 @@ class SmartPlaylistPlugin(BeetsPlugin): self._unmatched_playlists.add(playlist_data) def matches(self, model, query, album_query): - if album_query and isinstance(model, Album): + # Handle tuple/list of queries (multiple queries preserving order) + if isinstance(album_query, (list, tuple)) and isinstance(model, Album): + return any(q.match(model) for q, _ in album_query) + elif album_query and isinstance(model, Album): return album_query.match(model) - if query and isinstance(model, Item): + + if isinstance(query, (list, tuple)) and isinstance(model, Item): + return any(q.match(model) for q, _ in query) + elif query and isinstance(model, Item): return query.match(model) + return False def db_change(self, lib, model): @@ -270,9 +263,18 @@ class SmartPlaylistPlugin(BeetsPlugin): self._log.info("Creating playlist {}", name) items = [] - if query: + # Handle tuple/list of queries (preserves order) + if isinstance(query, (list, tuple)): + for q, sort in query: + items.extend(lib.items(q, sort)) + elif query: items.extend(lib.items(query, q_sort)) - if album_query: + + if isinstance(album_query, (list, tuple)): + for q, sort in album_query: + for album in lib.albums(q, sort): + items.extend(album.items()) + elif album_query: for album in lib.albums(album_query, a_q_sort): items.extend(album.items()) diff --git a/docs/changelog.rst b/docs/changelog.rst index 366af9ff0..af9012a23 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -29,6 +29,10 @@ New features: Bug fixes: +- :doc:`/plugins/smartplaylist`: Fixed an issue where multiple queries in a + playlist configuration were not preserving their order, causing items to + appear in database order rather than the order specified in the config. + :bug:`6183` - When hardlinking from a symlink (e.g. importing a symlink with hardlinking enabled), dereference the symlink then hardlink, rather than creating a new (potentially broken) symlink :bug:`5676` diff --git a/test/plugins/test_smartplaylist.py b/test/plugins/test_smartplaylist.py index d3569d836..a16313b28 100644 --- a/test/plugins/test_smartplaylist.py +++ b/test/plugins/test_smartplaylist.py @@ -22,7 +22,6 @@ from unittest.mock import MagicMock, Mock, PropertyMock import pytest from beets import config -from beets.dbcore import OrQuery from beets.dbcore.query import FixedFieldSort, MultipleSort, NullSort from beets.library import Album, Item, parse_query_string from beets.test.helper import BeetsTestCase, PluginTestCase @@ -54,16 +53,15 @@ class SmartPlaylistTest(BeetsTestCase): foo_foo = parse_query_string("FOO foo", Item) baz_baz = parse_query_string("BAZ baz", Item) baz_baz2 = parse_query_string("BAZ baz", Album) - bar_bar = OrQuery( - ( - parse_query_string("BAR bar1", Album)[0], - parse_query_string("BAR bar2", Album)[0], - ) - ) + # Multiple queries are now stored as a tuple of (query, sort) tuples + bar_queries = tuple([ + parse_query_string("BAR bar1", Album), + parse_query_string("BAR bar2", Album), + ]) assert spl._unmatched_playlists == { ("foo", foo_foo, (None, None)), ("baz", baz_baz, baz_baz2), - ("bar", (None, None), (bar_bar, None)), + ("bar", (None, None), (bar_queries, None)), } def test_build_queries_with_sorts(self): @@ -86,19 +84,28 @@ class SmartPlaylistTest(BeetsTestCase): ) spl.build_queries() - sorts = {name: sort for name, (_, sort), _ in spl._unmatched_playlists} + + # Multiple queries now return a tuple of (query, sort) tuples, not combined + sorts = {} + for name, (query_data, sort), _ in spl._unmatched_playlists: + if isinstance(query_data, tuple): + # Tuple of queries - each has its own sort + sorts[name] = [s for _, s in query_data] + else: + sorts[name] = sort sort = FixedFieldSort # short cut since we're only dealing with this assert sorts["no_sort"] == NullSort() assert sorts["one_sort"] == sort("year") - assert sorts["only_empty_sorts"] is None - assert sorts["one_non_empty_sort"] == sort("year") - assert sorts["multiple_sorts"] == MultipleSort( - [sort("year"), sort("genre", False)] - ) - assert sorts["mixed"] == MultipleSort( - [sort("year"), sort("genre"), sort("id", False)] - ) + # Multiple queries store individual sorts in the tuple + assert sorts["only_empty_sorts"] == [NullSort(), NullSort()] + assert sorts["one_non_empty_sort"] == [sort("year"), NullSort()] + assert sorts["multiple_sorts"] == [sort("year"), sort("genre", False)] + assert sorts["mixed"] == [ + sort("year"), + NullSort(), + MultipleSort([sort("genre"), sort("id", False)]), + ] def test_matches(self): spl = SmartPlaylistPlugin() @@ -122,6 +129,15 @@ class SmartPlaylistTest(BeetsTestCase): assert spl.matches(i, query, a_query) assert spl.matches(a, query, a_query) + # Test with list of queries + q1 = Mock() + q1.match.return_value = False + q2 = Mock() + q2.match.side_effect = {i: True}.__getitem__ + queries_list = [(q1, None), (q2, None)] + assert spl.matches(i, queries_list, None) + assert not spl.matches(a, queries_list, None) + def test_db_changes(self): spl = SmartPlaylistPlugin() @@ -327,6 +343,66 @@ class SmartPlaylistTest(BeetsTestCase): assert content == b"http://beets:8337/item/3/file\n" + def test_playlist_update_multiple_queries_preserve_order(self): + """Test that multiple queries preserve their order in the playlist.""" + spl = SmartPlaylistPlugin() + + # Create three mock items + i1 = Mock(path=b"/item1.mp3") + i1.evaluate_template.return_value = "ordered.m3u" + i2 = Mock(path=b"/item2.mp3") + i2.evaluate_template.return_value = "ordered.m3u" + i3 = Mock(path=b"/item3.mp3") + i3.evaluate_template.return_value = "ordered.m3u" + + lib = Mock() + lib.replacements = CHAR_REPLACE + lib.albums.return_value = [] + + # Set up lib.items to return different items for different queries + q1 = Mock() + q2 = Mock() + q3 = Mock() + + def items_side_effect(query, sort): + if query == q1: + return [i1] + elif query == q2: + return [i2] + elif query == q3: + return [i3] + return [] + + lib.items.side_effect = items_side_effect + + # Create playlist with multiple queries (stored as tuple) + queries_and_sorts = ((q1, None), (q2, None), (q3, None)) + pl = "ordered.m3u", (queries_and_sorts, None), (None, None) + spl._matched_playlists = [pl] + + dir = mkdtemp() + config["smartplaylist"]["relative_to"] = False + config["smartplaylist"]["playlist_dir"] = str(dir) + try: + spl.update_playlists(lib) + except Exception: + rmtree(syspath(dir)) + raise + + # Verify that lib.items was called with queries in the correct order + assert lib.items.call_count == 3 + lib.items.assert_any_call(q1, None) + lib.items.assert_any_call(q2, None) + lib.items.assert_any_call(q3, None) + + m3u_filepath = Path(dir, "ordered.m3u") + assert m3u_filepath.exists() + content = m3u_filepath.read_bytes() + rmtree(syspath(dir)) + + # Items should be in order: i1, i2, i3 + assert content == b"/item1.mp3\n/item2.mp3\n/item3.mp3\n" + class SmartPlaylistCLITest(PluginTestCase): plugin = "smartplaylist"