Replace "Reload After Saving" with option to remove queued tracks

The "Reload After Saving" setting was added in
g3347 (4f83e66) to solve FS#13287, by allowing
you to bookmark a modified playlist after saving,
without having to manually reload it first.

Since the rewrite of playlist_save in g5192
(90e3571), a modified playlist doesn't have to be
reloaded anymore in order to be bookmarked after
it's been saved, unless it contains queued tracks.

To cover the remaining use cases of the previously
available option, Rockbox will now offer to remove
any queued tracks from a playlist when saving it.

Change-Id: I2d6f12bcce14d8ff41a4d921ce84d628774103ac

apps/filetree.c

@@ -129,8 +129,7 @@ int ft_build_playlist(struct tree_context* c, int start_index)
  * avoid allocating yet another path buffer on the stack (and save some
  * code; the caller typically needs to create the full pathname anyway)...
  */
-bool ft_play_playlist(char* pathname, char* dirname,
-                      char* filename, bool skip_warn_and_bookmarks)
+bool ft_play_playlist(char* pathname, char* dirname, char* filename)
 {
     if (global_settings.party_mode && audio_status())
     {
@@ -138,12 +137,9 @@ bool ft_play_playlist(char* pathname, char* dirname,
         return false;
     }
 
-    if (!skip_warn_and_bookmarks)
-    {
-        int res =  bookmark_autoload(pathname);
-        if (res == BOOKMARK_CANCEL || res == BOOKMARK_DO_RESUME || !warn_on_pl_erase())
-            return false;
-    }
+    int res =  bookmark_autoload(pathname);
+    if (res == BOOKMARK_CANCEL || res == BOOKMARK_DO_RESUME || !warn_on_pl_erase())
+        return false;
 
     splash(0, ID2P(LANG_WAIT));
 
@@ -547,7 +543,7 @@ int ft_enter(struct tree_context* c)
 
         switch ( file_attr & FILE_ATTR_MASK ) {
             case FILE_ATTR_M3U:
-                play = ft_play_playlist(buf, c->currdir, file->name, false);
+                play = ft_play_playlist(buf, c->currdir, file->name);
 
                 if (play)
                 {

apps/filetree.h

@@ -28,7 +28,6 @@ int ft_exit(struct tree_context* c);
 int ft_assemble_path(char *buf, size_t bufsz,
                       const char* currdir, const char* filename);
 int ft_build_playlist(struct tree_context* c, int start_index);
-bool ft_play_playlist(char* pathname, char* dirname,
-                      char* filename, bool skip_warn_and_bookmarks);
+bool ft_play_playlist(char* pathname, char* dirname, char* filename);
 
 #endif

apps/iap/iap-lingo4.c

@@ -109,8 +109,7 @@ static void seek_to_playlist(unsigned long index)
                                        MAX_PATH);
                     ft_play_playlist(selected_playlist,
                                      global_settings.playlist_catalog_dir,
-                                     strrchr(selected_playlist, '/') + 1,
-                                     false);
+                                     strrchr(selected_playlist, '/') + 1);
 }
 
 static unsigned long nbr_total_playlists(void)

apps/lang/english.lang

@@ -15936,16 +15936,16 @@
 </phrase>
 <phrase>
   id: LANG_PLAYLIST_RELOAD_AFTER_SAVE
-  desc: reload playlist after saving
+  desc: deprecated
   user: core
   <source>
-    *: "Reload After Saving"
+    *: ""
   </source>
   <dest>
-    *: "Reload After Saving"
+    *: ""
   </dest>
   <voice>
-    *: "Reload After Saving"
+    *: ""
   </voice>
 </phrase>
 <phrase>
@@ -16655,3 +16655,17 @@
     *: "Database Directory"
   </voice>
 </phrase>
+<phrase>
+  id: LANG_REMOVE_QUEUED_TRACKS
+  desc: Confirmation dialog
+  user: core
+  <source>
+    *: "Remove Queued Tracks?"
+  </source>
+  <dest>
+    *: "Remove Queued Tracks?"
+  </dest>
+  <voice>
+    *: "Remove Queued Tracks?"
+  </voice>
+</phrase>

apps/menus/playlist_menu.c

@@ -38,7 +38,6 @@
 #include "talk.h"
 #include "playlist_catalog.h"
 #include "splash.h"
-#include "filetree.h"
 #include "general.h"
 
 /* load a screen to save the playlist passed in (or current playlist if NULL is passed) */
@@ -46,11 +45,6 @@ int save_playlist_screen(struct playlist_info* playlist)
 {
 
     char directoryonly[MAX_PATH+3];
-    char *filename;
-
-    int resume_index;
-    uint32_t resume_elapsed;
-    uint32_t resume_offset;
 
     char temp[MAX_PATH+1], *p;
     int len;
@@ -86,55 +80,8 @@ int save_playlist_screen(struct playlist_info* playlist)
 
         /* reload in case playlist was saved to cwd */
         reload_directory();
-
-        /* only reload newly saved playlist if:
-         * playlist is null AND setting is turned on
-         *
-         * if playlist is null, we should be dealing with the current playlist,
-         * and thus we got here from the wps screen. That means we want to reload
-         * the current playlist so the user can make bookmarks. */
-        if ((global_settings.playlist_reload_after_save == true) &&
-            (playlist == NULL))
-        {
-
-            /* at least one slash exists in temp */
-            if (strrchr(temp, '/') != NULL)
-            {
-                filename = strrchr(temp, '/') + 1;
-
-                if (temp[0] == '/') /* most common situation - first char is a slash */
-                {
-                    strcpy(directoryonly, temp);
-                    directoryonly[filename - temp] = '\0';
-                } else /* there is a slash, but not at the beginning of temp - prepend one */
-                {
-                    directoryonly[0] = '/';
-
-                    strcpy(directoryonly+1, temp);
-                    directoryonly[filename - temp + 1] = '\0';
-                }
-            } else /* temp doesn't contain any slashes, uncommon? */
-            {
-                directoryonly[0] = '/';
-                directoryonly[1] = '\0';
-                filename = temp;
-            }
-
-            /* can't trust index from id3 (don't know why), get it from playlist */
-            resume_index = playlist_get_display_index() - 1;
-
-            struct mp3entry* id3 = audio_current_track();
-
-            /* record elapsed and offset so they don't change when we load new playlist */
-            resume_elapsed = id3->elapsed;
-            resume_offset = id3->offset;
-
-            ft_play_playlist(temp, directoryonly, filename, true);
-            playlist_start(resume_index, resume_elapsed, resume_offset);
-        }
-    /* cancelled out of name selection */
     } else {
-        return 1;
+        return 1; /* cancelled out of name selection */
     }
 
     return 0;
@@ -179,14 +126,12 @@ MENUITEM_SETTING(warn_on_erase, &global_settings.warnon_erase_dynplaylist, NULL)
 MENUITEM_SETTING(keep_current_track_on_replace, &global_settings.keep_current_track_on_replace_playlist, NULL);
 MENUITEM_SETTING(show_shuffled_adding_options, &global_settings.show_shuffled_adding_options, NULL);
 MENUITEM_SETTING(show_queue_options, &global_settings.show_queue_options, NULL);
-MENUITEM_SETTING(playlist_reload_after_save, &global_settings.playlist_reload_after_save, NULL);
 MAKE_MENU(currentplaylist_settings_menu, ID2P(LANG_CURRENT_PLAYLIST),
           NULL, Icon_Playlist,
           &warn_on_erase,
           &keep_current_track_on_replace,
           &show_shuffled_adding_options,
-          &show_queue_options,
-          &playlist_reload_after_save);
+          &show_queue_options);
 
 MAKE_MENU(playlist_settings, ID2P(LANG_PLAYLISTS), NULL,
           Icon_Playlist,

apps/misc.c

@@ -1235,6 +1235,13 @@ int confirm_overwrite_yesno(void)
     return gui_syncyesno_run(&message, NULL, NULL);
 }
 
+int confirm_remove_queued_yesno(void)
+{
+    static const char *lines[] = { ID2P(LANG_REMOVE_QUEUED_TRACKS) };
+    static const struct text_message message = { lines, 1 };
+    return gui_syncyesno_run(&message, NULL, NULL);
+}
+
 /*  time_split_units()
     split time values depending on base unit
     unit_idx: UNIT_HOUR, UNIT_MIN, UNIT_SEC, UNIT_MS

apps/misc.h

@@ -160,6 +160,7 @@ int hex_to_rgb(const char* hex, int* color);
 
 int confirm_delete_yesno(const char *name);
 int confirm_overwrite_yesno(void);
+int confirm_remove_queued_yesno(void);
 
 char* strrsplt(char* str, int c);
 char* skip_whitespace(char* const str);

apps/playlist.c

@@ -99,6 +99,7 @@
 #include "filetypes.h"
 #include "icons.h"
 #include "system.h"
+#include "misc.h"
 
 #include "lang.h"
 #include "talk.h"
@@ -3977,6 +3978,7 @@ int playlist_save(struct playlist_info* playlist, char *filename)
     char tmpbuf[MAX_PATH+1];
     ssize_t pathlen;
     int rc = 0;
+    bool reload_tracks = false;
 
     if (!playlist)
         playlist = &current_playlist;
@@ -3996,6 +3998,17 @@ int playlist_save(struct playlist_info* playlist, char *filename)
         goto error;
     }
 
+    /* Ask if queued tracks should be removed, so that
+    playlist can be bookmarked after it's been saved */
+    for (int i = playlist->amount - 1; i >= 0; i--)
+        if (playlist->indices[i] & PLAYLIST_QUEUED)
+        {
+            if (reload_tracks || (reload_tracks = (confirm_remove_queued_yesno() == YESNO_YES)))
+                remove_track_unlocked(playlist, i, false);
+            else
+                break;
+        }
+
     rc = pl_save_playlist(playlist, save_path, tmpbuf, sizeof(tmpbuf));
     if (rc < 0)
     {
@@ -4030,5 +4043,8 @@ error:
     playlist_write_unlock(playlist);
     dc_thread_start(playlist, true);
     cpu_boost(false);
+    if (reload_tracks && playlist->started &&
+        (audio_status() & AUDIO_STATUS_PLAY))
+        audio_flush_and_reload_tracks();
     return rc;
 }

apps/settings.h

@@ -607,7 +607,6 @@ struct user_settings
     bool constrain_next_folder; /* whether next_folder is constrained to
                                    directories within start_directory */
     int  recursive_dir_insert; /* should directories be inserted recursively */
-    bool playlist_reload_after_save; /* reload and resume playlist after saving */
     bool fade_on_stop; /* fade on pause/unpause/stop */
     bool playlist_shuffle;
     bool warnon_erase_dynplaylist; /* warn when erasing dynamic playlist */

apps/settings_list.c

@@ -1412,8 +1412,6 @@ const struct settings_list settings[] = {
     CHOICE_SETTING(0, recursive_dir_insert, LANG_RECURSE_DIRECTORY , RECURSE_ON,
                    "recursive directory insert", off_on_ask, NULL , 3 ,
                    ID2P(LANG_OFF), ID2P(LANG_ON), ID2P(LANG_ASK)),
-    OFFON_SETTING(0, playlist_reload_after_save, LANG_PLAYLIST_RELOAD_AFTER_SAVE,
-                  false, "reload after saving playlist", NULL),
     /* bookmarks */
     CHOICE_SETTING(0, autocreatebookmark, LANG_BOOKMARK_SETTINGS_AUTOCREATE,
                    BOOKMARK_NO, "autocreate bookmarks",

manual/configure_rockbox/bookmarking.tex

@@ -18,7 +18,10 @@
         and does not work for tracks launched via the
         database. In addition, they do not work with dynamic (i.e. modified but not saved)
         playlists. If making a bookmark is not available, saving the playlist
-        is sufficient to allow a bookmark to be made.}
+        is sufficient to allow a bookmark to be made. Queued tracks, which are automatically
+        removed after playback, do not get stored to the playlist file and must
+        not be part of the current playlist. You need to confirm the removal of such tracks
+        when saving, or bookmarking will not be possible.}
 
   \begin{description}
 

manual/configure_rockbox/playlist_options.tex

@@ -47,13 +47,6 @@ related to playlists.
   If set to \setting{In Submenu}, Rockbox will move the options into a
   separate submenu.
 
-  \item[Reload After Saving.]
-  If set to \setting{Yes}, saving the current playlist from the While Playing Screen's
-  Context Menu will cause Rockbox to load the newly saved playlist and resume it to the
-  current position. This is useful for creating bookmarks after modifying or customizing
-  a playlist. Saving playlists outside of the While Playing Screen's Context Menu will
-  not be affected by this setting.
-
   \end{description}
 
 \end{description}

manual/main_menu/main.tex

@@ -178,8 +178,10 @@ will be saved in the Playlist Catalogue directory.
 
 \item[Save Current Playlist:]
   Saves the current dynamic playlist, excluding queued tracks, to the
-specified file. If no path is provided then playlist is saved to the current
-directory.
+specified file. Asks for confirmation whether to remove queued tracks
+from the current playlist, so that bookmarks can be created
+(see \reference{ref:createbookmark}).
+If no path is provided then playlist is saved to the current directory.
 
 \item[Reset Playlist Catalogue Directory:]
   Will reset the default location for playlists to the \fname{/Playlists}

manual/rockbox_interface/wps.tex

@@ -223,8 +223,13 @@ enabled. It allows the assignment of a personal rating value (0 -- 10)
 to a track which can be displayed in the WPS and used in the Database
 browser. The value wraps at 10.
 
-\subsubsection{Bookmarks}
-This allows you to create a bookmark in the currently-playing track.
+\subsubsection{\label{ref:createbookmark}Bookmarks}
+This allows you to create a bookmark in the currently-playing track. Note that bookmarks
+can only be created for directories, or for playlists stored on disk. Unsaved or modified
+lists must be saved first. Queued tracks, which are automatically removed after playback,
+do not get stored to the playlist file. When saving, Rockbox will offer to purge the
+current playlist of such tracks, so that you'll be able to create bookmarks afterwards.
+See \reference{ref:Bookmarkconfigactual} for more information on bookmarks.
 
 \subsubsection{\label{ref:trackinfoviewer}Show Track Info}
 \screenshot{rockbox_interface/images/ss-id3-viewer}{The track info viewer}{}

manual/working_with_playlists/main.tex

@@ -173,7 +173,9 @@ select \setting{Save Current Playlist} or enter the context menu for the
 \setting{Save Current Playlist}.
 Either method will bring you to the \setting{Virtual Keyboard} (see
 \reference{sec:virtual_keyboard}), enter a filename for your playlist and
-accept it and you are done.
+accept it. If the current playlist contains any queued tracks, you will be
+asked whether to remove them, as a prerequisiste for creating bookmarks
+(see \reference{ref:createbookmark}).
 
 \subsection{Loading saved playlists}
 \subsubsection{Through the \setting{File Browser}}