Rockbox

  • Status Unconfirmed
  • Percent Complete
    0%
  • Task Type Patches
  • Category Playlists
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version Release 3.0
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by Alex Bennee - 2008-12-19
Last edited by Nils Wallménius - 2008-12-19

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

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.

Steve Bavin commented on 2008-12-19 10:56

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.)

Marc Guay commented on 2008-12-19 13:48

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.

Jonas Häggqvist commented on 2008-12-19 17:54

How about "Save current playlist?"?

Alex Bennee commented on 2008-12-23 17:52

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.

Alex Bennee commented on 2008-12-24 09:28

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.

Alex Bennee commented on 2009-01-01 20:58

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.

Bertrik Sikken commented on 2009-01-04 15:43

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.

Alex Bennee commented on 2009-01-04 18:10

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.

Alex Bennee commented on 2009-01-19 17:48

Changes:

Re-factored to get rid of badly named "funge" function
Cleaned up commentary for menus
Removed unneeded passing of variables

Alex Bennee commented on 2009-01-19 18:52

Changes:

Don't pass playlist * when we don't need to. It gets confusing otherwise.

Alex Bennee commented on 2009-12-06 20:48

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.

Dominik Wenger commented on 2009-12-06 21:34

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.

Alex Bennee commented on 2009-12-06 22:55

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.

Alexander Levin commented on 2009-12-11 23:32

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.

Alex Bennee commented on 2009-12-13 17:02

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...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing