GNOME Bugzilla – Bug 316295
m3u export support
Last modified: 2006-04-20 09:09:03 UTC
Rhythmbox currently export playlist with its own format. I looked into the code and saw that it uses totem-pl-parser to save playlist. totem-pl-parser can save playlist as m3u/pls formats through function totem_pl_parser_write_with_title. Rhythmbox currently don't use this function (totem_pl_parser_write instead). I would request to add another entry to Rhythmbox menu to allow export as m3u/pls or we could use the same save dialog and determine format by extension. This would be easy because all the code is already in totem-pl-parser. All we need is UI :)
Bug 309817 has a patch which makes RB use totem_pl_parser_write_with_title, and use the signals when importing. Now than later versions of totem are out, we can probably apply that patch.
Saving a playlist exports in PLS format. Having an option to export as m3u would be nice, so retitling.
We support exporting as m3u, and this can be done via the dbus interface. All we need to do is make it an option in the UI.
*** Bug 332450 has been marked as a duplicate of this bug. ***
*** Bug 333495 has been marked as a duplicate of this bug. ***
How this would be done: rb_playlist_manager_cmd_save_playlist() [shell/rb-playlist-manager.c] creates the save dialog, and the rb_playlist_source_save_playlist call in the response callback does the saving. Someone would simply need to add a drop-menu to the dialog to choose the playlist format, and modify the parameters of rb_playlist_source_save_playlist accordingly. The final parameter is FALSE->PLS, TRUE->M3U. It would probably be better to make that last parameter an enum, so we could support more than two formats, but that's not required to implement m3u exporting.
Hi, I was reluctant to file a bug on this cause there is a good chance it might be my configuration. I had a go at messing about trying to test this, initially by just modifying the parameter from false to true. On exporting the playlist however, I'm getting a bizarre format that isn't m3u as far as I'm aware. I've uploaded a small sample that exported with 5 tracks here: http://gav.brokentrain.net/upload/air.m3u (over 10 and the file is horrendous). It seems to be a basic, run together mash of files which rhythmbox/totem can't then import and work with. I'm running version 1.2.1-0ubuntu1~breezy1 of libtotem-plparser0 incase it mattered, but I just wondered if anybody was having similar problems upon testing. Apologies again in cluttering up this bug if it's something stupid my end.
It works for me (totem-plparser 1.4) with the caveat that the tracks must be somewhere under the directory that you save the playlist in. It seems that totem-plparse won't write absolute paths into m3u files, only relative ones.
It's very possible that m3u writing is broken in some way, as nobody ever used it. Do file bugs if you find problems like that.
Created attachment 63684 [details] [review] Export-filter functionality. Hello there. Attached is a half-finished patch that I was attempting to implement regarding this bug. It's been lying around untouched for weeks and I figured I'd at least get it up there so hopefully somebody could improve it or at the very least get some ideas from it. Initially when taking this on, I thought about how other applications do the whole file extension business and the first application that sprung to my mind was dia [1]. Even better than this, I managed to locate the exact code that dia uses [2] when handling it's diagram export function which has been extremely handy. If you compare a couple of the functions in the patch to the dia source, you'll find that they are virtually identical to the dia way of doing things aside from some minor code clean-ups, identifier refactoring and HACKING conformance. The patch implements a combobox within the save dialog basically providing a familiar way of saving known playlist formats, or letting it determine automatically by the users filename extension. The user can either specify no file extension if a suitable type is selected, otherwise is required to enter one. Whilst attempting to integrate/follow the dia functionality, I've written a bunch of other functions to try and fill the gap between the different code variations between the applications. However, a few issues have surfaced with it and contain accompanying FIXME's in the code: 1 – I'm essentially inexperienced with C, and I was mainly attempting to get my "feet wet" with it to see how I got on. Coming from a Java background, this whole g_free() business is terrifying to me and I'm sure it's probably the source of a couple of following bugs with the patch. :) 2 – I'm very paranoid as mentioned above about deallocating things, so it's possible that there are some things overlooked. I've been unable to crash it though, which is a good sign I guess. 3 – The control flow of save_playlist_response_cb () seems a bit horrible and probably could use a rethink. (?) 4- Similarly, register_export_filters() seems a bit of a strange way of registering the export filters. I'm not sure if this would be best delegated elsewhere or not. 5 – The rb_playlist_source_save_playlist() function still gets a boolean passed to it. Originally I had it passing the "RBPlaylistExportType" enum contained within, but rb_playlist_source_save_playlist() seems to have further roots into other code which I eventually got lost in, so decided to leave the boolean just now. I saw that totem_pl_parser_write_with_title() only handles the two formats just now anyway, so figured it wasn't a big deal. As the attached screencast illustrates; the patch is pretty much functional, and does "work" as expected aside from the minor annoyances as noted above. Any feedback on this would be great, and don't be afraid to point out any brain dead coding on my part. I'd like to also mention that Doc's above comment regarding on getting started with this was invaluable, and I probably wouldn't have attempted this otherwise. Cheers. [1] - http://www.gnome.org/projects/dia/ [2] - http://cvs.gnome.org/viewcvs/dia/app/filedlg.c?view=markup
Created attachment 63685 [details] Export-filter screencast.
Created attachment 63717 [details] [review] slightly improved patch Looks fairly good to me. I've attached a modified version of the patch which uses glade to create the UI, rather than doing it manually. A couple of comments: strrchr only works correctly for ascii strings; to be able to handle UTF-8 characters (which Rhythmbox uses) you need to use g_utf8_strrchr, passing -1 for the "len" parameter. You don't need to create a GError to display something with rb_error_dialog. GError only needs to be used when you want to pass an error up to the calling function. I've also replaced the dynamic allocation of the entry-filter structure with a simpler static variable. It makes a lot of the code simpler and easier to read These are only things that come from not knowing C and glib/gtk too well, so don't worry too much. Thanks for working on this.
Cheers for the quick response and the helpful comments. I'd recently been wondering how glade fit into all this, and your improved patch has cleared that up, too. Glad to be of some help.
Thanks for working on this, I've committed it to cvs. Note: there is currently an issue with writing m3u playlists which contain tracks that are not under the directory the playlist is in. I've filed this as bug 338974 against totem-plparser. 2006-04-19 James Livingston <doclivingston@gmail.com> patch by: Gavin Stewart <gavin@brokentrain.net> * data/glade/playlist-save.glade: * data/glade/Makefile.am: * po/POTFILES.in: add the playlist saving dialog. * shell/rb-playlist-manager.c: (save_playlist_response_cb), (export_set_extension_cb), (filter_get_export_filter_label), (setup_format_menu), (rb_playlist_manager_cmd_save_playlist): * shell/rb-playlist-manager.h: Allow saving of playlists in alternate formats (PLS and M3U at the moment). Fixes bug 316295.
Created attachment 63924 [details] [review] Small glade issue. Awesome. However; I've noticed a small issue, in that when you resize the "Browse for other folders" item it was causing the group box to grab vertical space. I've attached a couple of screenshots of before and after and a trivial patch to fix the matter. Apologies if I should have filed a new bug but it's pretty unimportant. Cheers again.
Created attachment 63925 [details] Initial screenshot
Created attachment 63926 [details] Fixed screenshot