Rockbox

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#9677 - Change the erase dynamic playlist warning to a menu with several options including a quick save

Attached to Project: Rockbox
Opened by Alex Bennee (ajb) - Friday, 19 December 2008, 11:26 GMT+2
Last edited by Nils Wallménius (nls) - Friday, 19 December 2008, 14:35 GMT+2
Task Type Patches
Category Playlists
Status Unconfirmed
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version Release 3.0
Due in Version Undecided
Due Date Undecided
Percent Complete 0%
Private No

Details

As discussed on the mailing list this patch gets rid of the "About to Erase Dynamic Playlist" warning and replaces it with a menu option to save the current playlist under it's existing name.

The attached patch is a first cut for review as I'm just getting used to the menu system. If people think it's the right way to go I'll clean it up properly. Any suggestions for tweaks to the functionality also welcome.
   save_on_new.patch (9.7 KiB) 
 b/apps/filetree.c            |   23 ++++----
 b/apps/lang/english.lang     |   42 ++++++++++++++
 b/apps/menus/playlist_menu.c |  123 ++++++++++++++++++++++++++++++++++++++++++-
 b/apps/misc.c                |    2 
 b/apps/playlist.c            |    3 -
 b/apps/playlist.h            |    1 
 b/apps/tagtree.c             |    8 +-
 7 files changed, 186 insertions(+), 16 deletions(-)

This task depends upon

Comment by Steve Bavin (pondlife) - Friday, 19 December 2008, 11:56 GMT+2
I'd think that the warning/menu should be inside warn_on_pl_erase() (or it's replacement).

Also, this should be called just before each call to playlist_create() that refers to the current playlist - there may currently be paths through the code where the warning doesn't occur, yet the playlist is lost.

(Note the warning code can't be inside playlist._create() itself, as playlist.c shouldn't have any UI code at all.)
Comment by Marc Guay (Marc_Guay) - Friday, 19 December 2008, 14:48 GMT+2
The question "Erase dynamic playlist?" should also be reworded. The playlist is not always necessarily "dynamic" (if you've loaded a playlist file and simply made changes). Something akin to "Save changes to playlist?" could work.
Comment by Jonas Häggqvist (rasher) - Friday, 19 December 2008, 18:54 GMT+2
How about "Save current playlist?"?
Comment by Alex Bennee (ajb) - Tuesday, 23 December 2008, 18:52 GMT+2
Can you clarify what you mean by the menu being inside "warn_on_pl_erase() (or it's replacement). "?

I shall see where else playlist_create is called.
Comment by Alex Bennee (ajb) - Wednesday, 24 December 2008, 10:28 GMT+2
So far there are two functions which call playlist create which I
don't seem to be able to access via the menues. These are
onplay.c:add_to_playlist and playlist.c:create_and_play_dir. Any
pointers as to where I trigger them from?

Currently starting a bookmark will kill the current playlist via
bookmark_play which I shall endeavour to fix once I've decoded the logic.
Comment by Alex Bennee (ajb) - Thursday, 01 January 2009, 21:58 GMT+2
Latest version:

* Changed order of questions and tweaked phrasing
* Common name funging code: Only issue is it would be nicer to calculate the whole path once for the input to the menu and the actual write.
   save_on_new_v2.patch (11.8 KiB) 
 b/apps/filetree.c            |   23 +++---
 b/apps/lang/english.lang     |   82 ++++++++++++++++++++--
 b/apps/menus/playlist_menu.c |  159 +++++++++++++++++++++++++++++++++++++++++--
 b/apps/misc.c                |    2 
 b/apps/playlist.c            |    3 
 b/apps/playlist.h            |    1 
 b/apps/tagtree.c             |    8 +-
 7 files changed, 252 insertions(+), 26 deletions(-)

Comment by Bertrik Sikken (bertrik) - Sunday, 04 January 2009, 16:43 GMT+2
Does this patch add any new functionality, or is it basically a convenience?
What does "funge" mean?
I see you disable some old code with an "#if 0". IMO, if code is really no longer used, it should simply be removed instead of lingering on in an #if 0.
Comment by Alex Bennee (ajb) - Sunday, 04 January 2009, 19:10 GMT+2
Basically it saves you navigation key presses. If you want to save
your current playlist you can do that straight from the warning menu
rather than canceling and going to the save menu.

The option to save as the current playlist name is also better.

I shall find a better name for the funge function and remove the #if 0
code in the next version.
Comment by Alex Bennee (ajb) - Monday, 19 January 2009, 18:48 GMT+2
Changes:

Re-factored to get rid of badly named "funge" function
Cleaned up commentary for menus
Removed unneeded passing of variables
   save_on_new_v3.patch (13.5 KiB) 
 b/apps/filetree.c            |   23 ++---
 b/apps/lang/english.lang     |   82 ++++++++++++++++--
 b/apps/menus/playlist_menu.c |  194 ++++++++++++++++++++++++++++++++++++++++---
 b/apps/misc.c                |   18 ---
 b/apps/playlist.c            |    3 
 b/apps/playlist.h            |    1 
 b/apps/tagtree.c             |    8 -
 7 files changed, 278 insertions(+), 51 deletions(-)

Comment by Alex Bennee (ajb) - Monday, 19 January 2009, 19:52 GMT+2
Changes:

Don't pass playlist * when we don't need to. It gets confusing otherwise.
   save_on_new_v4.patch (13.4 KiB) 
 b/apps/filetree.c            |   23 ++---
 b/apps/lang/english.lang     |   82 ++++++++++++++++--
 b/apps/menus/playlist_menu.c |  194 ++++++++++++++++++++++++++++++++++++++++---
 b/apps/misc.c                |   18 ---
 b/apps/playlist.c            |    3 
 b/apps/playlist.h            |    1 
 b/apps/tagtree.c             |    8 -
 7 files changed, 278 insertions(+), 51 deletions(-)

Comment by Alex Bennee (ajb) - Sunday, 06 December 2009, 21:48 GMT+2
OK this patch has been sitting around for around a year now. If no one is interested in reviewing it I shan't bother pushing for inclusion and just carry it in my tree from now on.

The patch has been re-based and re-tested on the current SVN head.
   0001-v6-of-the-Save-Playlist-on-new-playlist-func... (15.2 KiB) 
 b/apps/filetree.c            |   23 ++--
 b/apps/lang/english.lang     |   82 ++++++++++++++++-
 b/apps/menus/playlist_menu.c |  199 ++++++++++++++++++++++++++++++++++++++++---
 b/apps/misc.c                |   17 ---
 b/apps/misc.h                |    4 
 b/apps/playlist.c            |    3 
 b/apps/playlist.h            |    1 
 b/apps/tagtree.c             |    8 -
 8 files changed, 283 insertions(+), 54 deletions(-)

Comment by Dominik Wenger (Domonoky) - Sunday, 06 December 2009, 22:34 GMT+2
I took a short look at this patch. I think the principle is nice, and should get commited.

a few remarks:
- playlist_maybe_save_current() sounds strange, maybe playlist_try_save_current() would sound better ? (not really important)
- You change some lang entrys (especially a id) instead of depreciating the old one, and adding new ones. I am not sure if its save to change .lang entry, it could break language tools and voicefiles.
- A Cancel action in the new menu would be nice.

Otherwise i am not sure if the code is ok, because i am not really a expert in this area.
Comment by Alex Bennee (ajb) - Sunday, 06 December 2009, 23:55 GMT+2
Updates following review:

* Made all use of LANG entries new (not re-using old IDs)
* Added Cancel option

I left the playlist_maybe_save_current() as I'm not sure playlist_try_save_current() is much better.
   0001-v7-of-the-Save-Playlist-on-new-playlist-func... (16.7 KiB) 
 b/apps/filetree.c            |   23 ++--
 b/apps/lang/english.lang     |  104 ++++++++++++++++++++--
 b/apps/menus/playlist_menu.c |  201 ++++++++++++++++++++++++++++++++++++++++---
 b/apps/misc.c                |   17 ---
 b/apps/misc.h                |    4 
 b/apps/playlist.c            |    3 
 b/apps/playlist.h            |    1 
 b/apps/settings_list.c       |    4 
 b/apps/tagtree.c             |    8 -
 9 files changed, 305 insertions(+), 60 deletions(-)

Comment by Alexander Levin (fml2) - Saturday, 12 December 2009, 00:32 GMT+2
I also like the idea of this patch. I can't tell whether it's cleanly made since I don't know this code area, but if it works (who have tested it and on what players?) it should IMO be committed.
Comment by Alex Bennee (ajb) - Sunday, 13 December 2009, 18:02 GMT+2
As far as I know as the author I'm the only one who has tested it on my iPod 5.5G . However I've had it in my tree for almost a year without issue as well as smoketesting it with Valgrind on the simulator. I would welcome more people testing.

Loading...