arcnmx / MPD

MPD fork adding a youtube-dl plugin
https://github.com/MusicPlayerDaemon/MPD/pull/223
GNU General Public License v2.0
7 stars 2 forks source link

Playlists add songs in reverse order #8

Closed arcnmx closed 4 years ago

arcnmx commented 4 years ago

As far as I can tell they do anyway?

Rio6 commented 4 years ago

Seems like they do, didn't notice that before you mentioned it XD

Reversing the list should be alright? Probably don't need to sort tracks by number. I did find a place in YtdlPlaylistPlugin.cxx that reverses the playlist.

    std::forward_list<DetachedSong> songs;
    if (metadata.GetEntries().empty()) {
        std::string url(uri /* metadata.GetWebpageURL() */);
        url.insert(0, "ytdl://");
        songs.emplace_front(url.c_str(), std::move(playlist));
    } else {
        for (auto &entry : metadata.GetEntries()) {
            std::string url = entry.GetWebpageURL().empty()
                ? entry.GetURL() : entry.GetWebpageURL();
            url.insert(0, "ytdl://");
            songs.emplace_front(url.c_str(), entry.GetTagBuilder().Commit());
        }
        songs.reverse();
    }

if its relevant.

arcnmx commented 4 years ago

Hm, convoluted...

  1. each item is parsed into entries.emplace_front(), which ends up with them in reversed order
  2. they are sorted by the playlist_index attribute.
    • This attribute doesn't seem to exist in flat playlists though, which is why this probably only broke when I added support for the RemoteTagScanner.
      • it's sorting them into the correct order, but it's a stable sort so if the index doesn't exist they get left in reverse order
  3. the snippet above iterates through the list in order but uses emplace_front, so reverses it as it adds them
  4. so a final .reverse() fixes it yet again

So um, probably for fixing it:

  1. TagHandler::entries should be a different type than forward_list, something that can efficiently append to the end of the list in O(1)
  2. the snippet above should iterate entries in reverse order so they get added to the final forward_list songs in the right order
    • Then there's no more need for that final songs.reverse();
arcnmx commented 4 years ago

Eh alternatively, just keep it as-is and just remove the songs.reverse(). Same difference, then just swap the polarity of the playlist_index sort.

Rio6 commented 4 years ago

If playlist_index doesn't exist anynore, do we keep the sorting? Maybe replace it with reverse()?

arcnmx commented 4 years ago

Eh well it doesn't need to be replaced with anything if we just erase the songs.reverse() above. It can either be removed or fixed by swapping < and >.

I'm not sure how I feel about removing it - if full mode is ever enabled or youtube-dl decides to include it in the json for certain extracotrs then it should be there... but maybe it won't be, dunno.

Rio6 commented 4 years ago

I think swapping < and > is a good idea.