GNOME Bugzilla – Bug 363018
[PATCH] Playlist Import/Export functionality
Last modified: 2007-04-12 14:27:32 UTC
Please describe the problem: Banshee should provide the ability to import/export playlists. Steps to reproduce: 1. 2. 3. Actual results: Expected results: Does this happen every time? Other information:
Created attachment 74908 [details] [review] Added playlist import/export functionality. This patch resolves bug #360466. I went ahead and included the confirmation before deleting a playlist code, since export and delete are so close together when you right click a playlist.
Cool, please merge this into a release soon! Could playlist import also happen in response to invoking banshee with a playlist as an argument? (Then clicking on a link to a shoutcast playlist (.pls) in the web browser or in the filemanager will finally work.)
Hi Trey, A few things need to happen with this patch first: a) Need to adhere to the coding style better (see HACKING) - the main things I see right off are that you are using tabs and not spaces (4) and you have methodNamesLikeThis and not MethodNamesLikeThis. I hate to nit pick, but these small things make a big difference when working inside the core codebase. For private members (fields, local variables especially), use_casing_like_this and notCasingLikeThis. b) All the new playlist API needs to be organized into namespaces. We're trying to move away from lumping things in the Banshee.Base namespace. I'm thinking a primary namespace Banshee.Playlists for the top-level API (IPlaylist, etc.) and then Banshee.Playlists.Formats (Banshee.Playlists.Formats.M3U, for example). Directory structure should also reflect this. The directory structure for new files is : src/<assembly>/<namespace>/<class-or-interface>.cs So you'd have: src/Banshee.Base/Banshee.Playlists/IPlaylist.cs src/Banshee.Base/Banshee.Playlists.Formats/M3u.cs This is not reflected in HACKING, and should be. I'll updated it. c) Do not add inline documentation. It makes code harder to read. We use monodoc [1] to generate documentation which can later be annotated. [1] This documentation has not been updated since we switched to gmcs/2.0. We are waiting for a Monodoc release that supports 2.0 before we can easily continue in this space. d) Use generics and generic collections (List<T> instead of ArrayList) e) It's Pls and not PLS in terms of API. It's IO and not Io (two letter acronyms are okay in all caps, three or more is generally bad - this comes straight from the .NET framework guidelines, two which we try to adhere). M3u and not M3U. f) Do not hard code path separators (/). On other platforms they may be different. Use Path.DirectorySeparatorChar instead - it's portable. Again, I'll update HACKING with the thigns I've mentined here but are currently not there. Please don't take this in any way but positive. The patch functionality itself looks pretty good and I'd like to get this in as soon as possible.
HACKING is updated, please read that.
Created attachment 78106 [details] [review] The revised patch with Aaron's requests. I believe I changed everything you requested. Let me know if it needs anything else.
Created attachment 78724 [details] [review] Revised patch Parts of my previous patch were applied last weekend during the work to get playlist support for DAP devices. This is a revised patch with those parts omitted.
Looking at this strictly from a patch perspective (I haven't tested the functionality), this looks pretty good. In my experience, I've found that good code in a nice language like C# (where naming schemes are based around english rather than do_the_underscore_dance_so_it_has_a_unique_name() function names) doesn't require this many //comments all over the place...I have it all over my MtpDap code, because it's rubbish and buggy and a comment to me is a sign of a piece of code that will need maintenance and I plan to fix. Oh and yes, the 4 spaces rule is in effect, except for in Makefile.am, which is always tabbed. Don't ask me why, I don't know. It just is. However I think if we wrote out every single pristine rule in the HACKING file, we'd find that we could never accept patches and regardless, no one would want to write them :) This is a good improvement over the early version of the patch. If the functionality is really solid (and I mean really solid), then I think it's ready to commit. However the last thing I want to see in Banshee 0.11.4 is playlist importing and exporting that only works 90% of the time. We all know that playlist formats are numerous, buggy, inconsistent, and have a tendency to come from the strangest sources. The one thing that I do not like is the complete lack of debugging output...after all, playlist importing could mean that someone accidentally tries to import any text document, and it needs to both present a meaningful error message as well as provide us something to go off on our bugzilla reports that we don't want to get, but have to decipher somehow. I will give this thing a good run-thru hopefully tomorrow or this weekend, and I might even revise a bit...we'll see :) Good work Trey!
Any work being done here? I think having this functionality in is vital for the next big release (0.12) which will probably be the version shipping with the next round of distributions. I've read in forums that people didn't change to basnhee because that would have meant re-creating all their playlists and this is something that most are not willing to do.
This patch is ready to be added to subversion. However, the code that stores the gconf keys needs to be updated. The framework for accessing the gconf keys changed around the end of December and I haven't updated the patch yet. I asked Aaron when he would have time to accept this patch and he told me in two weeks or so. This was about a week ago. I plan to update the patch in another week or two. Possibly sooner if Aaron is ready to accept the patch. The banshee code base changes fast. I've already updated the patch twice. I'd like to only do it once more.
Created attachment 84475 [details] [review] Updated for version 0.12.0 This still has an issue with it. It crashes when trying to change export formats. I'll look into it later.
Actually, the patch applies to the latest svn but not to the 0.12.0 release.
Latest update : I can't get the patched svn to compile : make complains with Unhandled Exception: System.ArgumentException: An element with the same key already exists in the dictionary. at System.Collections.Generic.Dictionary`2[System.String,System.Text.StringBuilder].Add (System.String , System.Text.StringBuilder ) [0x00000] at GConfSchemaExtractor.AddSchemaEntry (System.Object value, System.String namespce, System.String key, System.String short_desc, System.String long_desc) [0x00000] at GConfSchemaExtractor.Main (System.String[] args) [0x00000]
Created attachment 84628 [details] [review] Playlist Import/Export functionality. Updated to work with the SVN HEAD as of 2007.03.14. Please excuse my label from the last patch. This is intended to be applied against SVN HEAD. This is ready for accepting into the code base pending abock's approval. I've done limited testing. Let me know if you find any issues.
Just applied the latest patch to the latest trunk.... - builds fine! - menu items shot up correctly - when pressing cancel in the import playlist dialog I get a popup window "Unable to Import Playlist: Argument cannot be null. Parameter name: path [Ok]" - exporting playlists seems to work, _but_: - Playlist name is not preserved (perhaps write the plalist name to a comment in the exported plalist?) - I end up with entries like ../media/mp3/albums/The_Donnas/.......... instead of /mnt/media/mp3/albums/The_Donnas/.......... the "/mnt/media" folder is also mouted to $HOME/media and I imported from there... having the full path in the playlist would be quite a bit more usefull I suppose? Would be nice to have this in trunk soon, I have personally been waiting for this for a long time!
I'll fix the cancel issue. I'm not sure what you mean by the playlist name not being preserved. I assume that you mean the playlist name doesn't match the name of the file. I use the playlist name as the default with the format extension added to the end when exporting. When you import the playlist, it uses the tracks to intelligently name the playlist, just like it does when you create a playlist within Banshee. Abock requested that it work this way. As for the paths in the playlist, I made them relative so that playlists can be stored on a music share with the music. Then any client can import the playlist and it would play fine. For example, I have a music share that looks like this: /share/ /music/ /a /b /... /playlists/ /radio/ This allows me to build complex playlists and put them in the playlist folder. Then I just mount the share and I can import the playlists on any machine. If the playlist paths were absolute they wouldn't be portable across machines.
Created attachment 84695 [details] [review] Fixed the import cancel issue. Ready for import into subverion.
Created attachment 86141 [details] [review] Updated to fix minor merge issues.
Awesome work Trey, time to get it in.
Trey, unfortunately this patch is still not building for me. I'm getting the same error I was getting before you updated it on April 11th: ./Banshee.Library/Import.cs(68,34): error CS0117: `Banshee.Base.ImportManager' does not contain a definition for `Instance' It seems like this was added: public static bool IsImportInProgress() { return ImportManager.Instance.IsImportingInProgress(); } Did you mean to use the private static value, "instance", or am I missing something?
Works fine for me.
Nevermind, as we established in IRC, I am a moron. I had Ruben's branch merged into my trunk which was conflicting. This is now finally committed! Thanks Trey!