Lyrion not updating currentSong in server prefs on final track

[mikecappella]

I noticed long ago that when restarting Lyrion the selection of the last song is incorrect.

I started investigating this and noticed that Slim::Player::Playlist::modifyPlaylistCallback() is not catching the final track change. It seems from debug logs that the notifications are different. For the second to the last track, the command notification after the song jump is for playlist open. But for the final track 12, the notification after the song jump is, or can be, playlist newsong:

The relevant code I think is in modifyPlaylistCallback():

        my $savePlaylist = $request->isCommand([['playlist'], [keys %validSubCommands]]);

        # Did the playlist or the current song change?
        my $saveCurrentSong =
                $savePlaylist ||
                $request->isCommand([['playlist'], ['open']]) ||
                ($request->isCommand([['playlist'], ['jump', 'index', 'shuffle']]));

        if (!$saveCurrentSong) {
                main::INFOLOG && $log->info("saveCurrentSong not set. Returning.");
                return;
        }

        main::INFOLOG && $log->info("saveCurrentSong is: [$saveCurrentSong]");

If I add newsong to the list of sub-commands in the isCommand() test, the preference gets set. But I'm not sure what other affects this might have.

Furthermore, if I'm understanding the code for isCommand(), which calls __matchingRequest() in Slim/Control/Request.pm, it seems isCommand() is being called twice to check, once to check for playlist > open or if that fails, it is called again to test for playlist > jump | index | shuffle. So can this be consolidated into a single isCommand call checking for playlist > open | jump | index | shuffle | newsong ?

Finally, the parens around the final isCommand() are superfluous. These are a relic of the change made to Playlist.pm in this commit at old line 653 where the test was compound, but replaced by a simple test.

Here's the code I'm running now. This seems to work for my limited testing. Also a few typo corrections. If this seems correct, I can create a pull request.

diff --git a/Slim/Player/Playlist.pm b/Slim/Player/Playlist.pm
index 067648ee7..de63da33f 100644
--- a/Slim/Player/Playlist.pm
+++ b/Slim/Player/Playlist.pm
@@ -365,7 +365,7 @@ sub removeTrack {
        my $stopped = 0;
        my $oldMode = Slim::Player::Source::playmode($client);

-       # Stop playing track, if necessary, before cuting old track(s) out of playlist
+       # Stop playing track, if necessary, before cutting old track(s) out of playlist
        # in case Playlist::track() is called while stopping
        my $playingSongIndex = Slim::Player::Source::playingSongIndex($client);
        if ($playingSongIndex >= $tracknum  && $playingSongIndex < $tracknum + $nTracks) {
@@ -1133,8 +1133,7 @@ sub modifyPlaylistCallback {
        # Did the playlist or the current song change?
        my $saveCurrentSong =
                $savePlaylist ||
-               $request->isCommand([['playlist'], ['open']]) ||
-               ($request->isCommand([['playlist'], ['jump', 'index', 'shuffle']]));
+               $request->isCommand([['playlist'], ['open', 'newsong', 'jump', 'index', 'shuffle']]);

        if (!$saveCurrentSong) {
                main::INFOLOG && $log->info("saveCurrentSong not set. Returning.");
@@ -1182,7 +1181,7 @@ sub modifyPlaylistCallback {
                }
        }

-       # Because this callback is asyncronous, reset the flag here.
+       # Because this callback is asynchronous, reset the flag here.
        # there's only one place that sets it - in Client::startup()
        if ($client->startupPlaylistLoading) {