Notice: A non well formed numeric value encountered in /sites/rockbox.org/flyspray/includes/class.flyspray.php on line 96 Notice: A non well formed numeric value encountered in /sites/rockbox.org/flyspray/includes/class.flyspray.php on line 96 Notice: A non well formed numeric value encountered in /sites/rockbox.org/flyspray/includes/class.flyspray.php on line 96 Deprecated: Function create_function() is deprecated in /sites/rockbox.org/flyspray/includes/class.flyspray.php on line 104 Deprecated: The each() function is deprecated. This message will be suppressed on further calls in /sites/rockbox.org/flyspray/adodb/adodb.inc.php on line 845 Deprecated: Function create_function() is deprecated in /sites/rockbox.org/flyspray/includes/class.user.php on line 111 FS#11808 : playlist handling changes

Rockbox

Tasklist

FS#11808 - playlist handling changes

Attached to Project: Rockbox
Opened by Jonathan Gordon (jdgordon) - Monday, 13 December 2010, 11:05 GMT
Last edited by Jonathan Gordon (jdgordon) - Wednesday, 20 July 2011, 14:14 GMT
Task Type Patches
Category Playlists
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Release 3.7.1
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

changes discussed on the maling list:

1) main menu > playlists loads the playlist catalog
2) running a playlist opens it like a browser
3)... other stuff i cant remember
   playlistchanges.0.diff (22.4 KiB)
This task depends upon

Closed by  Jonathan Gordon (jdgordon)
Wednesday, 20 July 2011, 14:14 GMT
Reason for closing:  Accepted
Additional comments about closing:  in r30176 \o/
Comment by Jonathan Gordon (jdgordon) - Tuesday, 14 December 2010, 11:45 GMT
closer to finished, needs testing....

ok so whats changed?
1) main menu > playlists loads the playlist catalog
2) running a playlist opens it like a browser
3) playlist viewer settings menu has moved to "main menu > settings > general > playlists"
4) playlist viewer context menu has changed so it has "playlist, playlist catalog, remove, move" items (the first two are the same as file/db browser, need to change the icons but that isnt so easy)
5) fix a very UI-crap buig where you could endlessly go in a playlist catalog loop.



to be closer to the db/file browsers it should auto-exit to WPS when a new playlist is started, but for now this is good enough (big code changes for that)
   playlistchanges.1.diff (26 KiB)
Comment by Jonathan Gordon (jdgordon) - Tuesday, 14 December 2010, 12:33 GMT
change the default action of m3u files to open the viewer, fix the "view current playlist" menu options
   playlistchanges.2.diff (27.9 KiB)
Comment by William (lee321987) - Tuesday, 14 December 2010, 12:54 GMT
Hotkey on WPS is set to "View Current Playlist".
When Hotkey is pressed on WPS I get "no files" message box.
Comment by William (lee321987) - Tuesday, 14 December 2010, 13:01 GMT
OK, if I put a playlist in /playlists then WPS Hotkey shows me a list of playlist files in that folder. Though Hotkey is still set to "View Current Playlist".
Comment by William (lee321987) - Tuesday, 14 December 2010, 14:00 GMT
On a file or folder:
long press Select > Playlist Catalog > Add To Playlist
Shows the playlists in the the root of the player ("/"), instead of the ones in "/playlists" dir

long press Select > Playlist Catalog > Add To New Playlist
The default filename has way to many forward slashes, e.g. "/playlists/////////////////////".
Comment by Teruaki Kawashima (teru) - Wednesday, 15 December 2010, 13:04 GMT
synched to r28836
   playlistchanges.3.patch (13.1 KiB)
Comment by Teruaki Kawashima (teru) - Thursday, 16 December 2010, 12:29 GMT
fixed following.
* fix typo: onlplay_show_playlist_cat_menu => onplay_show_playlist_cat_menu.
* make onplay_show_playlist_menu() actually work.
* add "save current playlist" item to context menu of playlist viewer.

bugs and comments.
* bookmark isn't resumed when starting a playlist from playlist viewer (when configured to do so of course).
* changing use of GO_TO_PLAYLIST_VIEWER might not be good idea as it is used in %cs (current screen) tag and thus it might break backward compatibility of skin. i don't know if that part in %cs is actually used though.
it causes bug that William reported, WPS Hotkey displays playlists in the playlist catalog directory.
* it should go to WPS after starting playback from playlist viewer.
it looks strange that the list is shuffled in place after starting playback when Shuffle setting is turned on.

iirc, there was an idea to not view playlist in file browser. is it rejected?
It might be nice if there is playlist-related settings in context menu of root menu but i'm not sure.
How about to have setting to set default behaviour, view or start the playlist when it is selected?
   playlistchanges.4.patch (13.6 KiB)
Comment by Jonathan Gordon (jdgordon) - Thursday, 16 December 2010, 12:45 GMT
the context menu in playlist_viewer.c needs to be changed to use a proper MENU() instead of STRINGLIST_MENU() so those onplay_show_playlist_*() functions can go away (was going to do this today but didnt)



* bookmark isn't resumed when starting a playlist from playlist viewer (when configured to do so of course).
how do bookmarks handle .m3u playlists in svn?

* changing use of GO_TO_PLAYLIST_VIEWER might not be good idea as it is used in %cs (current screen) tag and thus it might break backward compatibility of skin. i don't know if that part in %cs is actually used though.
it causes bug that William reported, WPS Hotkey displays playlists in the playlist catalog directory.
Yeah, what does it do in svn? we don't want this to break anything, merely fix gaping holes.

* it should go to WPS after starting playback from playlist viewer.
yes, it should be fixed for this

* it looks strange that the list is shuffled in place after starting playback when Shuffle setting is turned on.
That is what should happen. When you start playback from the viewer it reloads the playlist viewer to show the new playlist which should be shuffled.. this is correct.

* iirc, there was an idea to not view playlist in file browser. is it rejected?
There was a bit of sentiment against this idea but I think it makes sense. This is what started this discussion anyway. m3u files should be considered like a folder in the file browser. This will mean 1 extra button click for a few people but that is worth it.

Please no more settings.

Thanks for syncing and those fixes/.
Comment by Teruaki Kawashima (teru) - Thursday, 16 December 2010, 14:51 GMT
> * bookmark isn't resumed when starting a playlist from playlist viewer (when configured to do so of course).
> how do bookmarks handle .m3u playlists in svn?
if Load Last Bookmark is set to yes, resume the first entry of the bookmark when the playlist is selected.
it should work similar when selected an audio file in the file browser: if there is a bookmark and Load Last Bookmark is set to yes, resume the bookmark and so on.

> * changing use of GO_TO_PLAYLIST_VIEWER might not be good idea as it is used in %cs (current screen) tag and thus it might break backward compatibility of skin. i don't know if that part in %cs is actually used though.
> it causes bug that William reported, WPS Hotkey displays playlists in the playlist catalog directory.
> Yeah, what does it do in svn? we don't want this to break anything, merely fix gaping holes.
it display the plalist currently being played as the name implies. i'm not entirely sure why you changed this to display plalist catalog.

> This will mean 1 extra button click for a few people but that is worth it.
>
> Please no more settings.
I donot understand why you often mention button click, but well, I feel it is more than 1 extra button. it needs to load file first (and doesn't go to WPS immediately after starting playback) i found it slightly annoying.
i against to commit this unless
- bookmark and shuffle setting works correctly.
- go to WPS after starting playback.

# or the action is configurable.
Comment by Jonathan Gordon (jdgordon) - Thursday, 16 December 2010, 22:13 GMT
I didnt intentionally change GO_TO_PLAYLIST_VIEWER, and yes the bookmark and going to wps need to be fixed.

have a read through of the mailing list discussion.the extra button click is the only downside for some people (the file needs to be loaded anyway), but if it opens as the default action then it opens up a heap of other uses which just cant be done right now.
Comment by Teruaki Kawashima (teru) - Friday, 17 December 2010, 11:55 GMT
* no one seems to complain about extra button click to me. reason to against it seems something like "i prefer to start playlist" or "it looks strange to me".
* changing the action to view playlist doesn't add new functionality which can't be done right now.
at most, it saves some clicks for people who wants to view playlist often, from my point of view.
* personally, I think it is better to separate the patch to two. one is adding Playlst and Playlist Catalog to playlist viewer context menu, the other is changing the default action when playlist is selected.
* as for loading, difference is loading happens before the click to start playback. below is what happens when i want to start a playlist.
in svn: select playlist > load playlist(spin up disk) > playback starts
with the change: select playlist > load playlist(spin up disk) > select track > load playlist(disk is already spinning, so there won't be notable delay) > playback starts
it might be nothing to speek of...sorry for an useless comment.
Comment by Jonathan Gordon (jdgordon) - Sunday, 17 July 2011, 13:34 GMT
this is only a sync for svn. I havnt gone through the last few comments to see what is still broken and what needs fixing. I would like to get this finished so any testing would be great.
   playlistchanges.diff (14.2 KiB)
Comment by Jonathan Gordon (jdgordon) - Monday, 18 July 2011, 11:49 GMT
fix the playlist wps hotkey so it goes to the viewer.

testers badly wanted :)

edit: try again if the playlist viewer doesnt work from the main menu... reuploading patch
   playlistchanges.diff (14 KiB)
Comment by Jonathan Gordon (jdgordon) - Monday, 18 July 2011, 13:03 GMT
and this one starts the WPS if you start playback from the playlist viewer.

I think this is close to commitable, just need to work out what the issue with bookmarking is and any other issues we want to fix
   playlistchanges.diff (14.7 KiB)
Comment by Jonathan Gordon (jdgordon) - Monday, 18 July 2011, 13:43 GMT
this fixed bookmakrs, "warn on playlist erase", party mode, %cs in the playlist browser (need to update the wiki/manual for this one)
   playlistchanges.diff (15.9 KiB)
Comment by Jonathan Gordon (jdgordon) - Monday, 18 July 2011, 14:44 GMT
last update for tonight.
save all playlists in the catalog (except dirplay), rename /root.m3u8 to /Playlists/all.m3u8.

not sure how to handle dirplay saves. currently if you dirplay /Music/foo/bar it ends up as /Music/foo/bar.m3u8 with absolute paths in the file so should it be /Playlists/foo - bar.m3u8 or something else? or leave it where it is?
   playlistchanges.diff (18.1 KiB)
Comment by Paul Louden (Llorean) - Monday, 18 July 2011, 15:12 GMT
So if I can no longer "create playlist" and get a file local to where the songs are, what's the replacement for easily creating/saving/using playlists on a memory card? If I create one now, do I have to then find it in the catalog and copy it over?

Would it be possible to make playlist catalog more functional *without* removing existing functionality in the process?
Comment by Jonathan Gordon (jdgordon) - Monday, 18 July 2011, 23:23 GMT
You obviously didnt actually try the patch.
Comment by Paul Louden (Llorean) - Monday, 18 July 2011, 23:39 GMT
So what exactly is the bit about " rename /root.m3u8 to /Playlists/all.m3u8." then?
Comment by Steve Bavin (pondlife) - Tuesday, 19 July 2011, 07:16 GMT
I will test shortly, but from your description this patch encourages (or compels) use of a dedicated /Playlists directory. That's not how my collection operates, hence I've never found a use for the playlist catalogue (or I'm totally misunderstanding it!).

Maybe it would be better to first fix up the Playlist Catalog so it picked up playlists from anywhere first? (Kind of a database browser for playlist files...) Then come back to this?
Comment by Jonathan Gordon (jdgordon) - Tuesday, 19 July 2011, 07:24 GMT
you can change the catalog directory fairly easily (or will be easy when i add the ui for it which should have happend years ago!).
This change will do nothing for people who dont care about the catalog already. you are of course free to put playlists anywhere you like and view them from the filebrowser. What this does do is make it easier for people who want to use the catalog.

I mentioned using the db to index .m3u8 files in IRC and was shot down pretty convincingly but perhaps a lua script could be added to move all playlists into there if you wanted to do that.

edit: Oh, St. says that the above patch may not be what I expected it to be. if the "Playlists" menu item doesnt drop you in the filebrowser in the catalog (or say "no files") then dont worry too much about testing that version
Comment by Steve Bavin (pondlife) - Tuesday, 19 July 2011, 07:49 GMT
OK, started testing..

1) I very much like the "treat playlists as subdirectories" vibe
2) I think it should it go the WPS when I start playback from a playlist, rather than the file browser.
Comment by Steve Bavin (pondlife) - Tuesday, 19 July 2011, 08:11 GMT
Random thoughts (for a followup project):
- It would be nice if saved playlists were fully treated as subdirs / fully integrated into the browser as a list of links - Follow Playlists could work in side playlists then!
- There was a discussion about having both a "go into" (RIGHT) and a "go into and play" (SELECT) option where keymaps allow. Any conclusion on this?
Comment by Jonathan Gordon (jdgordon) - Tuesday, 19 July 2011, 14:37 GMT
by the sound of your comments the above patch isnt the latest... this one should be correct.

1) :)
2) that is fixed (should have been in the above patch
3) not sure how that would work... and this patch is getting big enough already, maybe later
4) no conclusion, maybe later :)


edit: wtf? this patch which is a new diff from origin/master seems to be the same... it defintly works here and was uploaded from the correct place 6bcf3118512f5cb25528e0d3f323d4ad is the md5sum of the patch i tihnk im uploading!
   playlistchanges.diff (18.1 KiB)

Loading...