search

beetbox / beets

music library manager and MusicBrainz tagger
http://beets.io/
MIT License
12.8k stars 1.82k forks source link

playlist: sorting of an on-disk playlist is ignored #4553

Open JOJ0 opened 1 year ago

JOJ0 commented 1 year ago

When using the playlist plugin for querying (syntax playlist:/path/to/list.m3u) it seems like the sorting of the list is lost.

Problem

We see that even though the query sorts by bpm and the playlist file (saved by smartplaylist plugin earlier) is sorted by bpm ascending, it gets lost when the playlist:.... syntax is used:


 prod  (beets)    ~/git/beets   all_in ●  beet config | grep include: 
include: [smart_playlists.yaml]
 prod  (beets)    ~/git/beets   all_in ●  cat $BEETSDIR/smart_playlists.yaml
smartplaylist:
    auto: yes
    playlists:
        - name: 110..130_TechnoJungleDub_Hi.m3u
          query: >
              year:1993.. bpm:110..130 "genre::(?i)Jungle|Footwork|Drum & Bass|Drum and Bass|Dubstep|Bass" initial_key+ bpm+ bitrate:200000..

<snip>

        - name: mood_happy_relaxed.m3u
          query: >
             mood_happy:0.8.. mood_relaxed:0.8.. bpm+ initial_key+

        - name: mood_sad_relaxed.m3u
          query: >
             mood_sad:0.9.. mood_relaxed:0.9.. bpm+ initial_key+

        - name: mood_acoustic_relaxed.m3u
          query: >
             mood_acoustic:0.9.. mood_relaxed:0.9.. bpm+ initial_key+

   prod  (beets)    ~/git/beets   all_in ●  beet ls mood_acoustic:0.9.. mood_relaxed:0.9.. bpm+ initial_key+ -f '$bpm, $initial_key, $artist, $title' | head -5
70, Dm, Aphex Twin, Nanou 2
83, , µ-Ziq, Fall Of Antioch
90, F, Wisp, Around Corners
91, A, Portishead, Deep Water
92, G, Clueso, Barfuß

 prod  (beets)    ~/git/beets   all_in ●  beet ls playlist:/remote/data/music-library/0-playlists/mood_acoustic_relaxed.m3u -f '$bpm, $initial_key, $artist, $title' | head -5
148, , Long Beach Dub Allstars, Love Her Madly
166, D, Deep & Wide, Seven Seas
161, Fm, Squarepusher, 213 (Maritime Eposis)
105, Em, Squarepusher, Spymania Theme
108, C, Harold Budd, Balthus Bemused by Colour

   prod  (beets)    ~/git/beets   all_in ●  head -5 /remote/data/music-library/0-playlists/mood_acoustic_relaxed.m3u
/remote/data/music-library/IDM,Electronica/Aphex Twin/2001 - Aphex Twin - Drukqs/30 - Aphex Twin - Drukqs - Nanou 2 (2001).mp3
/remote/data/music-library/IDM,Electronica/u-Ziq/2003 - u-Ziq - Bilious Paths/11 - u-Ziq - Bilious Paths - Fall Of Antioch (2003).mp3
/remote/data/music-library/IDM,Electronica/Wisp/2004 - Wisp - About Things That Never Were/09 - Wisp - About Things That Never Were - Around Corners (2004).mp3
/remote/data/music-library/DowntempoTriphopDubAcidjazz/Portishead/2008 - Portishead - Third/07 - Portishead - Third - Deep Water (2008).m4a
/remote/data/music-library/AlternativeRockPop/Clueso/2008 - Clueso - So sehr dabei/01 - Clueso - So sehr dabei - Barfuss (2008).mp3

Setup

My configuration (output of beet config) is:

See above for the IMHO relevant parts. Can certainly provide more on demand.

JOJ0 commented 1 year ago

@Holzhaus Hi! Since you seem to have written this plugin or worked on it last, a friendly ping. I would love to look into it but wanted to ask first since probably you will find the issue quicker in your own code. Hopefully it is not an oversight on my end/configuration. I doublechecked as far as time permitted. HTH and thanks for a very useful plugin :-)

jackwilsdon commented 1 year ago

This looks to just be how the playlist plugin works at the moment - it's just a filter on the query results set:

https://github.com/beetbox/beets/blob/e201dd4fe57b0aa2e80890dc3939b0a803e3448d/beetsplug/playlist.py#L72

Looking at the beets code, I'm not quite sure how this plugin could be updated to implement sorting (it doesn't seem like we allow implementing custom sort orders in plugins currently).

JOJ0 commented 1 year ago

Hi, thanks for taking a look and trying to find a possible solution already! Appreciated! I came about this line of code and was not sure at what I'm looking at except that it might be an sql syntax I've not used / seen before. Turns out it is. I read the sqlite docs and learned about the IN keyword.

In the meantime I tried to take a closer look what's happening there but I don't seem to find out when col_clause is called. I'm now looking at beets.dbcore.Query which we are inheriting here. No col_clause is defined here for being overwritten but that does not mean it could be implemented.

I put some debug statements into the col_clause method we are talking about but they are never printed when I query the beets db using the playlist:.. syntax. I might not have understood how beets and db queries work yet. New territory but I'll get there. The match method is being called and returns what I had expected. Is it possible thatmatch is doing all the work and col_clause is never called / abandonend? I even tried what happens if I comment out the whole col_clause method: It turns out, it doesn't matter, the playlist: syntax query still returns the same, all good, playlist plugin working as-is.

Anyway, even if this is just "not fixable at the moment" I'd suggest to add this behaviour to the beets docs. Do you agree that a user would except a functionality that is about playlists in its classic sense would expect that querying what is found in a playlist should be returned excatly as it is saved? IMHO that is what m3u playlists are about in general: A sorted list of music files :-)) The docs don't tell about it: https://beets.readthedocs.io/en/latest/plugins/playlist.html

If you'd find the time to file a PR for the docs change I would appreciated it, if time doesn't permit currently, I would put it on my list of things to do and hopefully will get around to doing it soonish :-) That said, still I would like to know what's really going on there and why col_clause is unused. Will do some poking around in old PR's. It might have been working differently before and forgotten in a refactoring PR or something the like....

jackwilsdon commented 1 year ago

In the meantime I tried to take a closer look what's happening there but I don't seem to find out when col_clause is called. I'm now looking at beets.dbcore.Query which we are inheriting here. No col_clause is defined here for being overwritten but that does not mean it could be implemented.

You're right - it seems like beets.dbcore.FieldQuery is what calls col_clause, but the playlist plugin was updated to no longer extend that (and to just extend beets.dbcore.Query) in #3150. It looks like we can remove the col_clause function completely.

Do you agree that a user would except a functionality that is about playlists in its classic sense would expect that querying what is found in a playlist should be returned excatly as it is saved?

I'm not quite sure I agree here. The playlist plugin just provides a filter for your beets database, just as any other non-sorting query does (e.g. title:ABC). I'd find it a bit surprising if a filter suddenly started sorting the results, but at the same time I can understand the desire to be able to sort in playlist order.

Beets does have support for a special sorting syntax in queries (field+/field-), but I'm not quite sure how this could integrate with the playlist plugin in terms of syntax.

JOJ0 commented 1 year ago

Do you agree that a user would except a functionality that is about playlists in its classic sense would expect that querying what is found in a playlist should be returned excatly as it is saved?

I'm not quite sure I agree here. The playlist plugin just provides a filter for your beets database, just as any other non-sorting query does (e.g. title:ABC). I'd find it a bit surprising if a filter suddenly started sorting the results, but at the same time I can understand the desire to be able to sort in playlist order.

The current documentation of the playlist plugin essentially gives 2 sentences of what the thing does:

playlist is a plugin to use playlists in m3u format.

It is possible to query the library based on a playlist by specifying its absolute path:

https://beets.readthedocs.io/en/latest/plugins/playlist.html#playlist-plugin

Only after reading these last sentences of yours and having digested the fact that a beets feature that has playlist in its name ignores the lists sorting, I finally understand it. Syntax playlist: and title: are the same thing. I definitely agree with you that it would be surprising if playlist: would not respect the sorting that is either defined in the beets config item_sort setting or passed via a regular something+/something- field in the search string. But still, in the special case of a playlist it does makes sense to have the possibility of respecting the sorting in there. I could imagine a config option that enables the playlist: syntax to either respect the usual sorting from sort_item but can be set to use_playlist_order or something.

Beets does have support for a special sorting syntax in queries (field+/field-), but I'm not quite sure how this could integrate with the playlist plugin in terms of syntax.

Yep I'm using that a lot, especially whith generating lists with the smartplaylist plugin. And that's where I'm coming from actually. Going off topic now: My initial idea a couple of months ago was simple: I wanted to export smart playlists from beets and play it somewhere else, on another computer. Thus I wrote this feature to use convert to include a playlist: https://github.com/beetbox/beets/pull/4399. This describes my initial question/feature idea: https://github.com/beetbox/beets/discussions/4373

For the reading of the generated on-disk smart-playlists I figured that it would be smart to not reinvent the wheel but use a feature that can handle (read) playlists already: The playlist plugin....well and now here I am realising that I did not test the playlist plugin well enough to find out that it does not do sorting.

Beets usually is a very well documented piece of software, which I do really appreciate! If only all software would be like that! In this case a first step could be to clarify the sorting behavior of the plugin, explain why the current implementation makes sense and how a user could work around it in some situtation. Would you welcome a PR adding this to the docs and willing to review and merge? If yes, I'll submit one when I find a couple of minutes.

Workaround

Long story short, in the current implementation a possible workaround in case the original sorting of the on-disk playlist is known, would be to apply sort ordering with the playlist query manually. Taking an example from above with one of my smartplaylist-generated lists, this could be done:

beet ls playlist:mood_acoustic_relaxed bpm+ initial_key+

In case the original ordering of the playlist is unknown or it is just a manually created list, there currently is no way of respecting the order of the on-disk playlist.

sampsyo commented 1 year ago

I do think it would be cool to try to clarify some issues around this stuff in the docs! The various uses of the concept of a "playlist" in beets can indeed be confusing…

JOJ0 commented 1 year ago

Docs were changed to describe the expected behaviour better in #4563