- Status Closed
- Percent Complete
- 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
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.
Closed by speachy
2021-05-12 21:19
Reason for closing: Out of Date
Additional comments about closing: Warning: Undefined array key "typography" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 371 Warning: Undefined array key "camelcase" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 407
2021-05-12 21:19
Reason for closing: Out of Date
Additional comments about closing: Warning: Undefined array key "typography" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 371 Warning: Undefined array key "camelcase" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 407
A variation of this has been recently
merged.
Loading...
Available keyboard shortcuts
- Alt + ⇧ Shift + l Login Dialog / Logout
- Alt + ⇧ Shift + a Add new task
- Alt + ⇧ Shift + m My searches
- Alt + ⇧ Shift + t focus taskid search
Tasklist
- o open selected task
- j move cursor down
- k move cursor up
Task Details
- n Next task
- p Previous task
- Alt + ⇧ Shift + e ↵ Enter Edit this task
- Alt + ⇧ Shift + w watch task
- Alt + ⇧ Shift + y Close Task
Task Editing
- Alt + ⇧ Shift + s save task
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.)
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.
How about "Save current playlist?"?
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.
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.
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.
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.
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.
Changes:
Re-factored to get rid of badly named "funge" function
Cleaned up commentary for menus
Removed unneeded passing of variables
Changes:
Don't pass playlist * when we don't need to. It gets confusing otherwise.
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.
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.
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.
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.
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.