From 7592d2ca5eab2e684f45b7a7163f1594f3c36c96 Mon Sep 17 00:00:00 2001 From: Christian Soffke Date: Sun, 29 Sep 2024 17:42:03 +0200 Subject: playlist: deprecate PLAYLIST_COMMAND_CLEAR In the following scenario, a garbage file name would be written to the control file for the playing track (resulting in a failure to resume correctly): - "Keep Current Track When Replacing Playlist" option is set - A track is playing that was not inserted but instead comes from a playlist file on disk - User performs "Playing Next..." -> "Play" on some tracks, so that the current playlist is replaced (but leaving the playing track queued) - User saves the playlist while queued track is still playing (the offer to remove queued tracks is declined) The failure occurs because the pl_save_update_control function assumes that the seek offset for queued files always points into the control file. Meanwhile, the remove_all_tracks_unlocked function adds the PLAYLIST_QUEUED flag indiscriminately, even if a track comes from a playlist file instead of having been inserted. Theoretically, this could be addressed by adding the playing track's file name as a parameter to PLAYLIST_COMMAND_CLEAR, which the track is then updated to point to, unless it was already inserted (alternatively, we could seek within the playlist file for such tracks). Unless I'm missing something, it may be preferable, though, to get rid of PLAYLIST_COMMAND_CLEAR completely, and instead have the remove_all_tracks_unlocked function start over with a fresh control file, into which we insert a single ('P' and) 'Q' command. This seems to have several advantages: - When resuming, we eliminate the need to parse and handle all of the outdated entries again that end up being cleared anyway - It is ensured that the control file doesn't rely on the existence of a playlist file anymore after the playlist has been cleared - We can reset the playlist's file name, which should make it less likely for the user to overwrite their previous (now unconnected) playlist that was still displayed in the Save dialog - Unrelated bookmarks for the previous playlist aren't displayed anymore - Improved consistency with existing behavior when the "Keep Current Track When Replacing Playlist" was disabled. Change-Id: I41a89295bbac878807d65db9cf67b8a485daf0e5 --- apps/playlist.c | 53 ++++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 40 insertions(+), 13 deletions(-) (limited to 'apps/playlist.c') diff --git a/apps/playlist.c b/apps/playlist.c index af5e1b5df0..23d3355708 100644 --- a/apps/playlist.c +++ b/apps/playlist.c @@ -138,9 +138,10 @@ * v4 added the (F)lags command. * v5 added index to the (C)lear command. Added PLAYLIST_INSERT_LAST_ROTATED (-8) as * a supported position for (A)dd or (Q)eue commands. + * v6 removed the (C)lear command. */ #define PLAYLIST_CONTROL_FILE_MIN_VERSION 2 -#define PLAYLIST_CONTROL_FILE_VERSION 5 +#define PLAYLIST_CONTROL_FILE_VERSION 6 #define PLAYLIST_COMMAND_SIZE (MAX_PATH+12) @@ -436,9 +437,6 @@ static int update_control_unlocked(struct playlist_info* playlist, case PLAYLIST_COMMAND_RESET: result = write(fd, "R\n", 2); break; - case PLAYLIST_COMMAND_CLEAR: - result = fdprintf(fd, "C:%d\n", i1); - break; case PLAYLIST_COMMAND_FLAGS: result = fdprintf(fd, "F:%u:%u\n", i1, i2); break; @@ -1174,9 +1172,32 @@ static int create_and_play_dir(int direction, bool play_last) */ static int remove_all_tracks_unlocked(struct playlist_info *playlist, bool write) { + char filename[MAX_PATH]; + int seek_pos = -1; + if (playlist->amount <= 0) return 0; + if (write) /* Write control file commands to disk */ + { + if (playlist->control_fd < 0) + return -1; + + if (get_track_filename(playlist, playlist->index, + filename, sizeof(filename)) != 0) + return -1; + + /* Start over with fresh control file for emptied dynamic playlist */ + pl_close_control(playlist); + create_control_unlocked(playlist); + update_control_unlocked(playlist, PLAYLIST_COMMAND_PLAYLIST, + PLAYLIST_CONTROL_FILE_VERSION, -1, + "", "", NULL); + update_control_unlocked(playlist, PLAYLIST_COMMAND_QUEUE, + 0, 0, filename, NULL, &seek_pos); + sync_control_unlocked(playlist); + } + /* Move current track down to position 0 */ playlist->indices[0] = playlist->indices[playlist->index]; #ifdef HAVE_DIRCACHE @@ -1190,22 +1211,25 @@ static int remove_all_tracks_unlocked(struct playlist_info *playlist, bool write /* Update playlist state as if by remove_track_unlocked() */ playlist->first_index = 0; + playlist->index = 0; playlist->amount = 1; playlist->indices[0] |= PLAYLIST_QUEUED; + playlist->flags = 0; /* Reset dirplay and modified flags */ if (playlist->last_insert_pos == 0) playlist->last_insert_pos = -1; else playlist->last_insert_pos = 0; - if (write && playlist->control_fd >= 0) - { - update_control_unlocked(playlist, PLAYLIST_COMMAND_CLEAR, - playlist->index, -1, NULL, NULL, NULL); - sync_control_unlocked(playlist); - } + if (seek_pos == -1) + return 0; - playlist->index = 0; + /* Update seek offset so it points into the new control file. */ + playlist->indices[0] &= ~PLAYLIST_INSERT_TYPE_MASK & ~PLAYLIST_SEEK_MASK; + playlist->indices[0] |= PLAYLIST_INSERT_TYPE_INSERT | seek_pos; + + /* Cut connection to playlist file */ + update_playlist_filename_unlocked(playlist, "", ""); return 0; } @@ -3402,6 +3426,8 @@ int playlist_resume(void) playlist->last_insert_pos = -1; break; } + /* FIXME: Deprecated. + * Adjust PLAYLIST_CONTROL_FILE_MIN_VERSION after removal */ case PLAYLIST_COMMAND_CLEAR: { if (strp[0]) @@ -3960,11 +3986,12 @@ static int pl_save_update_control(struct playlist_info* playlist, playlist->last_insert_pos = rotate_index(playlist, playlist->last_insert_pos); playlist->first_index = 0; } - + for (int index = 0; index < playlist->amount; ++index) { /* We only need to update queued files */ - if (!(playlist->indices[index] & PLAYLIST_QUEUED)) + if (!(playlist->indices[index] & PLAYLIST_INSERT_TYPE_MASK && + playlist->indices[index] & PLAYLIST_QUEUED)) continue; /* Read filename from old control file */ -- cgit v1.2.3