Fix minor bookmark problems/Enhance bookmark functions

This fix:

-fixes when the bookmark menu and submenus are displayed and hidden
 in the context menu.
     -'Create Bookmark' should be hidden when tracks are queued in
      the playlist or nothing is currently playing (previously it was
      never hidden)
     -'List Bookmarks' should be hidden if and only if no bookmark
      file exists for the current playlist (previously it was hidden
      if tracks were queued or nothing was playing neither of which
      hinder loading bookmarks)
     -'Bookmarks' main menu should be hidden if both 'Create
      Bookmarks' and 'List Bookmarks' submenus are hidden

-fixes a problem where the 'Bookmark Error' message was not always
 displayed on bookmarking failure

-adds BOOKMARK_USB_CONNECTED return value to the bookmark functions
 to distinguish if the bookmark list was exited due to a USB
 connection.

-fixes other minor logic problems in the bookmarking functions

Change-Id: If6394b2e77f027773a7c94ffdcb52dbb15e2922b
Reviewed-on: http://gerrit.rockbox.org/177
Reviewed-by: Osborne Jacobs <ozziejacks@gmail.com>
Tested-by: Osborne Jacobs <ozziejacks@gmail.com>
Reviewed-by: Jonathan Gordon <rockbox@jdgordon.info>

3 files changed, 107 insertions, 59 deletions

diff --git a/apps/bookmark.c b/apps/bookmark.c
index cce8ef88bf..e0325b38d4 100644
--- a/apps/bookmark.c
+++ b/apps/bookmark.c
@@ -93,8 +93,7 @@ static const char* get_bookmark_info(int list_index,
                                      void* data,
                                      char *buffer,
                                      size_t buffer_len);
-static char* select_bookmark(const char* bookmark_file_name, bool show_dont_resume);
-static bool  is_bookmarkable_state(void);
+static int   select_bookmark(const char* bookmark_file_name, bool show_dont_resume, char** selected_bookmark);
 static bool  write_bookmark(bool create_bookmark_file, const char *bookmark);
 static int   get_bookmark_count(const char* bookmark_file_name);
 
@@ -109,37 +108,38 @@ static char global_bookmark[MAX_BOOKMARK_SIZE];
 static char global_filename[MAX_PATH];
 
 /* ----------------------------------------------------------------------- */
-/* This is the interface function from the main menu.                      */
+/* This is an interface function from the context menu.                    */
+/* Returns true on successful bookmark creation.                           */
 /* ----------------------------------------------------------------------- */
 bool bookmark_create_menu(void)
 {
-    write_bookmark(true, create_bookmark());
-    return false;
+    return write_bookmark(true, create_bookmark());
 }
 
 /* ----------------------------------------------------------------------- */
-/* This function acts as the load interface from the main menu             */
+/* This function acts as the load interface from the context menu.         */
 /* This function determines the bookmark file name and then loads that file*/
-/* for the user.  The user can then select a bookmark to load.             */
-/* If no file/directory is currently playing, the menu item does not work. */
+/* for the user.  The user can then select or delete previous bookmarks.   */
+/* This function returns BOOKMARK_SUCCESS on the selection of a track to   */
+/* resume, BOOKMARK_FAIL if the menu is exited without a selection and     */
+/* BOOKMARK_USB_CONNECTED if the menu is forced to exit due to a USB       */
+/* connection.                                                             */
 /* ----------------------------------------------------------------------- */
-bool bookmark_load_menu(void)
+int bookmark_load_menu(void)
 {
-    bool ret = false;
+    char* bookmark;
+    int ret = BOOKMARK_FAIL;
 
     push_current_activity(ACTIVITY_BOOKMARKSLIST);
-    if (is_bookmarkable_state())
-    {
-        char* name = playlist_get_name(NULL, global_temp_buffer,
+
+    char* name = playlist_get_name(NULL, global_temp_buffer,
                                        sizeof(global_temp_buffer));
-        if (generate_bookmark_file_name(name))
+    if (generate_bookmark_file_name(name))
+    {
+        ret = select_bookmark(global_bookmark_file_name, false, &bookmark);
+        if (bookmark != NULL)
         {
-            char* bookmark = select_bookmark(global_bookmark_file_name, false);
-            
-            if (bookmark != NULL)
-            {
-                ret = play_bookmark(bookmark);
-            }
+            ret = play_bookmark(bookmark) ? BOOKMARK_SUCCESS : BOOKMARK_FAIL;
         }
     }
 
@@ -150,6 +150,7 @@ bool bookmark_load_menu(void)
 /* ----------------------------------------------------------------------- */
 /* Gives the user a list of the Most Recent Bookmarks.  This is an         */
 /* interface function                                                      */
+/* Returns true on the successful selection of a recent bookmark.          */
 /* ----------------------------------------------------------------------- */
 bool bookmark_mrb_load()
 {
@@ -157,7 +158,7 @@ bool bookmark_mrb_load()
     bool ret = false;
 
     push_current_activity(ACTIVITY_BOOKMARKSLIST);
-    bookmark = select_bookmark(RECENT_BOOKMARK_FILE, false);
+    select_bookmark(RECENT_BOOKMARK_FILE, false, &bookmark);
     if (bookmark != NULL)
     {
         ret = play_bookmark(bookmark);
@@ -170,13 +171,14 @@ bool bookmark_mrb_load()
 /* ----------------------------------------------------------------------- */
 /* This function handles an autobookmark creation.  This is an interface   */
 /* function.                                                               */
+/* Returns true on successful bookmark creation.                           */
 /* ----------------------------------------------------------------------- */
 bool bookmark_autobookmark(bool prompt_ok)
 {
     char*  bookmark;
     bool update;
 
-    if (!is_bookmarkable_state())
+    if (!bookmark_is_bookmarkable_state())
         return false;
 
     audio_pause();    /* first pause playback */
@@ -229,36 +231,50 @@ bool bookmark_autobookmark(bool prompt_ok)
 /* This file will contain N number of bookmarks in the following format:   */
 /* resume_index*resume_offset*resume_seed*resume_first_index*              */
 /* resume_file*milliseconds*MP3 Title*                                     */
+/* Returns true on successful bookmark write.                              */
+/* Returns false if any part of the bookmarking process fails.  It is      */
+/* possible that a bookmark is successfully added to the most recent        */
+/* bookmark list but fails to be added to the bookmark file or vice versa. */
 /* ------------------------------------------------------------------------*/
 static bool write_bookmark(bool create_bookmark_file, const char *bookmark)
 {
-    bool   success=false;
-    if (!bookmark)
-       return false; /* something didn't happen correctly, do nothing */
+    bool ret=true;
 
-    if (global_settings.usemrb)
-        success = add_bookmark(RECENT_BOOKMARK_FILE, bookmark, true);
+    if (!bookmark)
+    {
+       ret = false; /* something didn't happen correctly, do nothing */
+    }
+    else
+    {
+        if (global_settings.usemrb)
+            ret = add_bookmark(RECENT_BOOKMARK_FILE, bookmark, true);
 
 
-    /* writing the bookmark */
-    if (create_bookmark_file)
-    {
-        char* name = playlist_get_name(NULL, global_temp_buffer,
-                                       sizeof(global_temp_buffer));
-        if (generate_bookmark_file_name(name))
+        /* writing the bookmark */
+        if (create_bookmark_file)
         {
-            success = add_bookmark(global_bookmark_file_name, bookmark, false);
+            char* name = playlist_get_name(NULL, global_temp_buffer,
+                                       sizeof(global_temp_buffer));
+            if (generate_bookmark_file_name(name))
+            {
+                ret = ret & add_bookmark(global_bookmark_file_name, bookmark, false);
+            }
+            else
+            {
+                ret = false; /* generating bookmark file failed */
+            }
         }
     }
 
-    splash(HZ, success ? ID2P(LANG_BOOKMARK_CREATE_SUCCESS)
+    splash(HZ, ret ? ID2P(LANG_BOOKMARK_CREATE_SUCCESS)
            : ID2P(LANG_BOOKMARK_CREATE_FAILURE));
 
-    return true;
+    return ret;
 }
 
 /* ----------------------------------------------------------------------- */
 /* This function adds a bookmark to a file.                                */
+/* Returns true on successful bookmark add.                                */
 /* ------------------------------------------------------------------------*/
 static bool add_bookmark(const char* bookmark_file_name, const char* bookmark, 
                          bool most_recent)
@@ -330,13 +346,14 @@ static bool add_bookmark(const char* bookmark_file_name, const char* bookmark,
 /* ----------------------------------------------------------------------- */
 /* This function takes the system resume data and formats it into a valid  */
 /* bookmark.                                                               */
+/* Returns not NULL on successful bookmark format.                         */
 /* ----------------------------------------------------------------------- */
 static char* create_bookmark()
 {
     int resume_index = 0;
     char *file;
 
-    if (!is_bookmarkable_state())
+    if (!bookmark_is_bookmarkable_state())
         return NULL; /* something didn't happen correctly, do nothing */
 
     /* grab the currently playing track */
@@ -395,9 +412,12 @@ static char* create_bookmark()
 /* ----------------------------------------------------------------------- */
 /* This function will determine if an autoload is necessary.  This is an   */
 /* interface function.                                                     */
+/* Returns true on bookmark load or bookmark selection.                    */
 /* ------------------------------------------------------------------------*/
 bool bookmark_autoload(const char* file)
 {
+    char* bookmark;
+
     if(global_settings.autoloadbookmark == BOOKMARK_NO)
         return false;
 
@@ -416,7 +436,7 @@ bool bookmark_autoload(const char* file)
     }
     else
     {
-        char* bookmark = select_bookmark(global_bookmark_file_name, true);
+        select_bookmark(global_bookmark_file_name, true, &bookmark);
         
         if (bookmark != NULL)
         {
@@ -439,6 +459,7 @@ bool bookmark_autoload(const char* file)
 /* ----------------------------------------------------------------------- */
 /* This function loads the bookmark information into the resume memory.    */
 /* This is an interface function.                                          */
+/* Returns true on successful bookmark load.                               */
 /* ------------------------------------------------------------------------*/
 bool bookmark_load(const char* file, bool autoload)
 {
@@ -458,7 +479,7 @@ bool bookmark_load(const char* file, bool autoload)
     else
     {
         /* This is not an auto-load, so list the bookmarks */
-        bookmark = select_bookmark(file, false);
+        select_bookmark(file, false, &bookmark);
     }
 
     if (bookmark != NULL)
@@ -672,10 +693,15 @@ static int bookmark_list_voice_cb(int list_index, void* data)
 }
 
 /* ----------------------------------------------------------------------- */
-/* This displays a the bookmarks in a file and allows the user to          */
+/* This displays the bookmarks in a file and allows the user to            */
 /* select one to play.                                                     */
+/* *selected_bookmark contains a non NULL value on successful bookmark     */
+/* selection.                                                              */
+/* Returns BOOKMARK_SUCCESS on successful bookmark selection, BOOKMARK_FAIL*/
+/* if no selection was made and BOOKMARK_USB_CONNECTED if the selection    */
+/* menu is forced to exit due to a USB connection.                         */
 /* ------------------------------------------------------------------------*/
-static char* select_bookmark(const char* bookmark_file_name, bool show_dont_resume)
+static int select_bookmark(const char* bookmark_file_name, bool show_dont_resume, char** selected_bookmark)
 {
     struct bookmark_list* bookmarks;
     struct gui_synclist list;
@@ -684,6 +710,7 @@ static char* select_bookmark(const char* bookmark_file_name, bool show_dont_resu
     size_t size;
     bool exit = false;
     bool refresh = true;
+    int ret = BOOKMARK_FAIL;
 
     bookmarks = plugin_get_buffer(&size);
     bookmarks->buffer_size = size;
@@ -711,7 +738,8 @@ static char* select_bookmark(const char* bookmark_file_name, bool show_dont_resu
                 /* No more bookmarks, delete file and exit */
                 splash(HZ, ID2P(LANG_BOOKMARK_LOAD_EMPTY));
                 remove(bookmark_file_name);
-                return NULL;
+                *selected_bookmark = NULL;
+                return BOOKMARK_FAIL;
             }
 
             if (bookmarks->show_dont_resume)
@@ -771,7 +799,8 @@ static char* select_bookmark(const char* bookmark_file_name, bool show_dont_resu
             if (item >= 0)
             {
                 talk_shutup();
-                return bookmarks->items[item - bookmarks->start];
+                *selected_bookmark = bookmarks->items[item - bookmarks->start];
+                return BOOKMARK_SUCCESS;
             }
             
             /* Else fall through */
@@ -806,6 +835,7 @@ static char* select_bookmark(const char* bookmark_file_name, bool show_dont_resu
         default:
             if (default_event_handler(action) == SYS_USB_CONNECTED)
             {
+                ret = BOOKMARK_USB_CONNECTED;
                 exit = true;
             }
 
@@ -814,12 +844,14 @@ static char* select_bookmark(const char* bookmark_file_name, bool show_dont_resu
     }
 
     talk_shutup();
-    return NULL;
+    *selected_bookmark = NULL;
+    return ret;
 }
 
 /* ----------------------------------------------------------------------- */
 /* This function takes a location in a bookmark file and deletes that      */
 /* bookmark.                                                               */
+/* Returns true on successful bookmark deletion.                           */
 /* ------------------------------------------------------------------------*/
 static bool delete_bookmark(const char* bookmark_file_name, int bookmark_id)
 {
@@ -919,6 +951,7 @@ static void say_bookmark(const char* bookmark,
 
 /* ----------------------------------------------------------------------- */
 /* This function parses a bookmark and then plays it.                      */
+/* Returns true on successful bookmark play.                               */
 /* ------------------------------------------------------------------------*/
 static bool play_bookmark(const char* bookmark)
 {
@@ -976,6 +1009,7 @@ static const char* long_token(const char* s, long* dest)
 /* This function takes a bookmark and parses it.  This function also       */
 /* validates the bookmark.  The parse_filenames flag indicates whether     */
 /* the filename tokens are to be extracted.                                */
+/* Returns true on successful bookmark parse.                              */
 /* ----------------------------------------------------------------------- */
 static bool parse_bookmark(const char *bookmark, const bool parse_filenames)
 {
@@ -1042,6 +1076,7 @@ static bool parse_bookmark(const char *bookmark, const bool parse_filenames)
 /* Changing this function could result in how the bookmarks are stored.    */
 /* it would be here that the centralized/decentralized bookmark code       */
 /* could be placed.                                                        */
+/* Always returns true                                                     */
 /* ----------------------------------------------------------------------- */
 static bool generate_bookmark_file_name(const char *in)
 {
@@ -1074,30 +1109,28 @@ static bool generate_bookmark_file_name(const char *in)
 }
 
 /* ----------------------------------------------------------------------- */
-/* Returns true if a bookmark file exists for the current playlist         */
+/* Returns true if a bookmark file exists for the current playlist.        */
+/* This is an interface function.                                          */
 /* ----------------------------------------------------------------------- */
 bool bookmark_exists(void)
 {
     bool exist=false;
 
-    if(is_bookmarkable_state())
+    char* name = playlist_get_name(NULL, global_temp_buffer,
+                                   sizeof(global_temp_buffer));
+    if (generate_bookmark_file_name(name))
     {
-        char* name = playlist_get_name(NULL, global_temp_buffer,
-                                       sizeof(global_temp_buffer));
-        if (generate_bookmark_file_name(name))
-        {
-            exist = file_exists(global_bookmark_file_name);
-        }
+        exist = file_exists(global_bookmark_file_name);
     }
-
     return exist;
 }
 
 /* ----------------------------------------------------------------------- */
 /* Checks the current state of the system and returns true if it is in a   */
 /* bookmarkable state.                                                     */
+/* This is an interface funtion.                                           */
 /* ----------------------------------------------------------------------- */
-static bool is_bookmarkable_state(void)
+bool bookmark_is_bookmarkable_state(void)
 {
     int resume_index = 0;
 
diff --git a/apps/bookmark.h b/apps/bookmark.h
index 4b61edba9b..ff7b87c1bf 100644
--- a/apps/bookmark.h
+++ b/apps/bookmark.h
@@ -23,13 +23,20 @@
 
 #include <stdbool.h>
 
-bool bookmark_load_menu(void);
+enum {
+    BOOKMARK_FAIL = -1,
+    BOOKMARK_SUCCESS = 0,
+    BOOKMARK_USB_CONNECTED = 1
+};
+
+int  bookmark_load_menu(void);
 bool bookmark_autobookmark(bool prompt_ok);
 bool bookmark_create_menu(void);
 bool bookmark_mrb_load(void);
 bool bookmark_autoload(const char* file);
 bool bookmark_load(const char* file, bool autoload);
 bool bookmark_exists(void);
+bool bookmark_is_bookmarkable_state(void);
 
 #endif /* __BOOKMARK_H__ */
 
diff --git a/apps/onplay.c b/apps/onplay.c
index 143745d366..11fffb9312 100644
--- a/apps/onplay.c
+++ b/apps/onplay.c
@@ -91,7 +91,8 @@ static int bookmark_menu_callback(int action,
                                   const struct menu_item_ex *this_item);
 MENUITEM_FUNCTION(bookmark_create_menu_item, 0,
                   ID2P(LANG_BOOKMARK_MENU_CREATE),
-                  bookmark_create_menu, NULL, NULL, Icon_Bookmark);
+                  bookmark_create_menu, NULL,
+                  bookmark_menu_callback, Icon_Bookmark);
 MENUITEM_FUNCTION(bookmark_load_menu_item, 0,
                   ID2P(LANG_BOOKMARK_MENU_LIST),
                   bookmark_load_menu, NULL,
@@ -105,13 +106,20 @@ static int bookmark_menu_callback(int action,
     switch (action)
     {
         case ACTION_REQUEST_MENUITEM:
-            if (this_item == &bookmark_load_menu_item)
+            /* hide create bookmark option if bookmarking isn't currently possible (no track playing, queued tracks...) */
+            if (this_item == &bookmark_create_menu_item)
+            {
+                if (!bookmark_is_bookmarkable_state())
+                    return ACTION_EXIT_MENUITEM;
+            }
+            /* hide loading bookmarks menu if no bookmarks exist */
+            else if (this_item == &bookmark_load_menu_item)
             {
                 if (!bookmark_exists())
                     return ACTION_EXIT_MENUITEM;
             }
-            /* hide the bookmark menu if there is no playback */
-            else if ((audio_status() & AUDIO_STATUS_PLAY) == 0)
+            /* hide the bookmark menu if bookmarks can't be loaded or created */
+            else if (!bookmark_is_bookmarkable_state() && !bookmark_exists())
                 return ACTION_EXIT_MENUITEM;
             break;
 #ifdef HAVE_LCD_CHARCELLS