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

1 files changed, 40 insertions, 13 deletions

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 */