GNOME Bugzilla – Bug 587964
Use DOS folder separator in playlists
Last modified: 2012-01-22 15:50:34 UTC
I have a Cowon iAudio S9 which works in Mass Storage mode and support M3U playlists. Banshee writes the M3Us with UNIX style slashes (/ rather than \) and line endings. Given that music player manufacturers tend to target windows users, the devices tend to only understand DOS slashes and line endings. Perhaps it would make sense when writing M3U playlists to use DOS slashes and line endings?
As an owner of a Sansa Fuze that does not recognize UNIX separators, I have been reduced to putting 'folder_depth=0' in my .is_audio_player file to reduce the symptoms of this issue.
Bulk changing the assignee to banshee-maint@gnome.bugs to make it easier for people to get updated on all banshee bugs by following that address. It's usually quite apparent who is working on a given bug by the comments and/or patches attached.
According to the last messages in banshee mailing list, this will be implemented checking on information exposed by media-player-info when https://bugs.freedesktop.org/show_bug.cgi?id=40441 is fixed.
Now that https://bugs.freedesktop.org/show_bug.cgi?id=40441 is resolved, I'm working on this feature.
Created attachment 196458 [details] [review] Add FolderSeparator property to devices Needed to implement reading of media-player-info properties
Created attachment 196459 [details] [review] Add DOS folder separator support to playlists Focused primarily on .m3u playlists.
Comment on attachment 196458 [details] [review] Add FolderSeparator property to devices Looks good! Small details below to fix (wrt the cosmetic ones, next time refer to http://git.gnome.org/browse/banshee/tree/HACKING for the coding style conventions please): > From 20dde9d59d12b1ccbd2b3580d939814d419d334c Mon Sep 17 00:00:00 2001 > From: unamanic <unamanic@legolas.tolkien> I guess this identity is wrong? :) > diff --git a/src/Backends/Banshee.Gio/Banshee.Hardware.Gio/DeviceMediaCapabilities.cs b/src/Backends/Banshee.Gio/Banshee.Hardware.Gio/DeviceMediaCapabilities.cs > index e4c1611..6ce724c 100644 > --- a/src/Backends/Banshee.Gio/Banshee.Hardware.Gio/DeviceMediaCapabilities.cs > +++ b/src/Backends/Banshee.Gio/Banshee.Hardware.Gio/DeviceMediaCapabilities.cs > @@ -122,6 +122,12 @@ namespace Banshee.Hardware.Gio > } > } > > + public string FolderSeparator { Can this be a char instead of a string? Then let GMpiFileInfo deal with the conversion from string to char. > + get { > + return mpi.FolderSeparator; > + } > + } Cosmetic: can you place this property below FolderDepth, to have the Folder* ones next to each other? Also, put the entire get{} accessor in one line. > + private string folder_separator = "/"; > + > + public string FolderSeparator { > + get { return folder_separator; } > + set > + { > + if (value=="DOS" || value == "\\") Cosmetic: please put a space before and after the operator ==. > - if (mpi_file.HasGroup (PlaylistGroup) && mpi_file.HasKey (PlaylistGroup, "Formats")) { > - PlaylistFormats = mpi_file.GetStringList (PlaylistGroup, "Formats") ?? new string [] {}; > + if (mpi_file.HasGroup (PlaylistGroup)) > + { Cosmetic: for if clauses, put braces in the same line. > + if (mpi_file.HasKey (PlaylistGroup, "Formats")) > + { Cosmetic: for if clauses, put braces in the same line. > + PlaylistFormats = mpi_file.GetStringList (PlaylistGroup, "Formats") ?? new string [] {}; > + } > + > + if (mpi_file.HasKey (PlaylistGroup, "FolderSeparator")) > + { Cosmetic: for if clauses, put braces in the same line. > + FolderSeparator = mpi_file.GetString (PlaylistGroup, "FolderSeparator"); > + > + } Cosmetic: unneeded blank line after FolderSeparator assignment. > OutputFormats = new string[] {}; > PlaylistFormats = new string[] {}; > AccessProtocols = new string[] {}; > + FolderSeparator = "/"; Try not to do duplicated things. You're already setting this value above to the field wrapped by the property. Actually, maybe this is a better place to put the default value because the method here is called InitDefaults(). > -#endif > +//#endif Be careful, don't include this change.
Comment on attachment 196459 [details] [review] Add DOS folder separator support to playlists Thanks for this patch as well! I'm marking it as needs-work as well because I'm seeing some few things that could be improved (very small improvements and cosmetic nitpicks, so nothing to worry about!): > From 8ab393798f4a1ead29fe5d070116303522a2d9c8 Mon Sep 17 00:00:00 2001 > From: unamanic <unamanic@legolas.tolkien> Correct identity? like previous patch. > Date: Tue, 13 Sep 2011 20:05:10 -0500 > Subject: [PATCH 3/3] PlaylistFormat: Add DOS folder separator support to playlists As you're only changing M3U playlists here, can you specify it in the first line? But trying to be as short as possible, for instance "Dap.MassStorage: M3U playlist support for DOS folder separator" > > Fixes: BGO#587964 By convention, we add a slightly longer summary of what changed and why here (of course, adding the bug number too as you did). > diff --git a/src/Core/Banshee.Services/Banshee.Playlists.Formats/IPlaylistFormat.cs b/src/Core/Banshee.Services/Banshee.Playlists.Formats/IPlaylistFormat.cs > index a8076e5..0dff831 100644 > --- a/src/Core/Banshee.Services/Banshee.Playlists.Formats/IPlaylistFormat.cs > +++ b/src/Core/Banshee.Services/Banshee.Playlists.Formats/IPlaylistFormat.cs > @@ -46,5 +46,6 @@ namespace Banshee.Playlists.Formats > > Uri BaseUri { get; set; } > string Title { get; set; } > + string FolderSeparator {get; set;} This can be a char rather than a string, no? Cosmetic: Please add a space before '}' and after '{'. > } > } > diff --git a/src/Core/Banshee.Services/Banshee.Playlists.Formats/M3uPlaylistFormat.cs b/src/Core/Banshee.Services/Banshee.Playlists.Formats/M3uPlaylistFormat.cs > index 480d996..dee082a 100644 > --- a/src/Core/Banshee.Services/Banshee.Playlists.Formats/M3uPlaylistFormat.cs > +++ b/src/Core/Banshee.Services/Banshee.Playlists.Formats/M3uPlaylistFormat.cs > @@ -122,7 +122,10 @@ namespace Banshee.Playlists.Formats > } > > writer.WriteLine("#EXTINF:{0},{1} - {2}", duration, track.DisplayArtistName, track.DisplayTrackTitle); > - writer.WriteLine(ExportUri(track.Uri)); > + string trackpath =ExportUri(track.Uri); Please add a space after the assignment operator, and a space before the parenthesis. > + if (FolderSeparator != "/") > + trackpath = trackpath.Replace("/", FolderSeparator); Cosmetic: add braces in every if clause. Behaviour: FolderSeparator doesn't have a default value. I mean, if the object we're dealing with here is not created by MassStorageSource, FolderSeparator could be null and then a dumb replace could happen. How about something like: if (FolderSeparator == DosFolderSeparator) { trackpath = trackpath.Replace (UnixFolderSeparator, FolderSeparator); } And then you make two new const fields called DosFolderSeparator and UnixFolderSeparator. > + writer.WriteLine(trackpath); Put a space after the parenthesis. > + > + public virtual string FolderSeparator { > + get { return folder_separator; } > + set { folder_separator = value; } > + } You can make this one an auto-property, right? > } > } > diff --git a/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage/MassStorageSource.cs b/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage/MassStorageSource.cs > index 880dbf1..e533a70 100644 > --- a/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage/MassStorageSource.cs > +++ b/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage/MassStorageSource.cs > @@ -329,7 +329,8 @@ namespace Banshee.Dap.MassStorage > if (from != null && !String.IsNullOrEmpty (escaped_name)) { > from.Reload (); > if (playlist_format == null) { > - playlist_format = Activator.CreateInstance (PlaylistTypes[0].Type) as PlaylistFormatBase; > + playlist_format = Activator.CreateInstance (PlaylistTypes[0].Type) as PlaylistFormatBase; You're not really changing this line. Although identation is incorrect because it has an extra space, patch cleanliness and VCS history are more important. Thanks again!
> I guess this identity is wrong? :) It appears I lost my .gitconfig in my recent OS reload. I'll fix the issues and resubmit the patches.
Created attachment 196577 [details] [review] Add FolderSeparator property to devices In src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage/MassStorageDevice.cs: + private char folder_separator; + public virtual char FolderSeparator { + get { return folder_separator; } + } I could have made this an auto-property, but left it for consistency throughout the file. Is this the appropriate way to go with these edits?
Created attachment 196578 [details] [review] Add DOS folder separator support to M3U playlists
Created attachment 196579 [details] [review] Cleanup - Fix indentation error Fixes indentation error with src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage/MassStorageSource.cs Is it acceptable to put this here?
Comment on attachment 196579 [details] [review] Cleanup - Fix indentation error Hey William, sorry it took so long to review. This looks fine but fails to compile, I guess you forgot to include a change to IDeviceMediaCapabilities interface? This is the error: ./Banshee.Dap.MassStorage/MassStorageSource.cs(333,77): error CS1061: Type `Banshee.Hardware.IDeviceMediaCapabilities' does not contain a definition for `FolderSeparator' and no extension method `FolderSeparator' of type `Banshee.Hardware.IDeviceMediaCapabilities' could be found (are you missing a using directive or an assembly reference?) /home/andres121/Documents/code/bansheeMASTER/bin/Banshee.Services.dll (Location of the symbol related to previous error) Compilation failed: 1 error(s), 0 warnings make[4]: *** [../../../bin/Banshee.Dap.MassStorage.dll] Error 1 make[3]: *** [all-recursive] Error 1 make[2]: *** [all-recursive] Error 1 make[1]: *** [all-recursive] Error 1 make: *** [all] Error 2 Also, if you could tell me a way to test it with my device, that would be grand. Thanks
Comment on attachment 196579 [details] [review] Cleanup - Fix indentation error Oops! There are actually 3 patches here! And one of them contained the change to the interface... And anyway I marked the wrong patch as needs-work so I'm removing the flag. Will, can you tell me the exact steps to test these 2 patches?
Currently, the patches only do DOS folder separators, not line endings. Any ideas on how to change the newline chars? To test: 1) Put the following .is_audio_player file on a thumb drive: audio_folders=MUSIC/ folder_depth=2 output_formats=audio/x-ms-wma,audio/mpeg playlist_formats=audio/x-mpegurl,audio/m3u,audio/mpeg-url folder_separator=DOS 2) In Banshee, add some music to the device 3) Create a "test" playlist on the device using several tracks 4) Eject the device from banshee (so the playlist is synced) 5) Remount the device and open MUSIC/test.m3u in a text editor paths should use DOS folder separators.
(In reply to comment #15) > Currently, the patches only do DOS folder separators, not line endings. Any > ideas on how to change the newline chars? Does the SanzaFuze not work if it doesn't have DOS line endings? Or was this just a "nice-to-have-just-in-case" thing? (James?) We maybe should open a new bug for that. > To test: > > 1) Put the following .is_audio_player file on a thumb drive: > audio_folders=MUSIC/ > folder_depth=2 > output_formats=audio/x-ms-wma,audio/mpeg > playlist_formats=audio/x-mpegurl,audio/m3u,audio/mpeg-url > folder_separator=DOS > > 2) In Banshee, add some music to the device > > 3) Create a "test" playlist on the device using several tracks > > 4) Eject the device from banshee (so the playlist is synced) > > 5) Remount the device and open MUSIC/test.m3u in a text editor paths should use > DOS folder separators. I just tested and it works, good work. We may get it into the release today, I'm just talking with Bertrand on IRC...
The original Fuze works with Unix line endings. The Fuze+ currently has a bug where it doesn't support folders in playlists at all (hope they fix it soon). Jame's original report was for the Cowon iAudio S9.
(In reply to comment #17) > The original Fuze works with Unix line endings. The Fuze+ currently has a bug > where it doesn't support folders in playlists at all (hope they fix it soon). > Jame's original report was for the Cowon iAudio S9. Ok, Will, thanks so much for the update and for the patches. Given that James' hasn't replied to this comment, I'm going to separate concerns and edit this bug's summary to not mention EOLs, so we can close it. James, or anyone else interested in EOL handling, please open a new bug, with exact information of the device models that require this functionality. I just pushed Will's three patches. Thanks go to Alex as well for giving his sign-off. Will, sorry that it took so long to process this, it will be available in upcoming Banshee 2.3.5. This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.