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