Thanks for the bug report and the piece of code.

I have some questions for you:

- What do you do with the NokiaM3uPlaylistFormat class, did you modify banshee code to handle it or did you just replace PlaylistFormatBase with the contents of the new class?

- In your ACTUAL paragraph, when you refer to the "file manager", you mean the file manager in Symbian?

Thanks

I took an exact duplicate of the M3uPlaylistFormat, modified it ever so slightly (MagicHandler, PlaylistFormatDescription, both Load() and Save() methods), then added it's PlaylistFormatDescription to the PlaylistParser's static playlist_formats array, and also to the PlaylistFileUtil's static export_formats and PlaylistExtensions arrays.

Then it was just a case of adding one of the mimetypes in it's PlaylistFormatDescription to the .is_audio_player file on the device.

Yeah, in both the ACTUAL and EXPECTED paragraphs I mean the file manager on the device. 

I hadn't tested earlier, but though I've modified the Load() method, the playlist can't be imported into banshee from the device as SafeUri throws this:

Caught an exception - System.ApplicationException: Filename path must be absolute (in `Hyena')
  at Hyena.SafeUri.FilenameToUri

Also, the playlists which are on the device aren't showing up in the source browser the second time the device is plugged in-so I'd have to assume that my Load() method is faulty...

Created attachment 226378 [details]
Nokia M3u Playlist Variant

Updated version, should be more reliable in getting the mount point of the device. This is done via the DriveInfo class.

Can you attach also a sample M3U playlist that your class generates? And a sample of the same playlist when *not* using your class?

Also, is there a way to detect programmatically at runtime if your device is a NokiaN95? I guess it will have its own USB id? Can you extract it with this instructions:

"USB ID:  e.g. 0fce:e112 ; to find this information you can type lsusb in a terminal window (Terminal can usually be accessed under GNOME in Applications/Accessories/Terminal) and find the line corresponding to your device after plugging it. If you’re unsure of the correct line, juste paste the whole command output."

(Extracted from here: http://ernstfamily.ch/jonathan/2010/10/help-me-make-your-music-player-or-phone-recognized-as-such-under-linux/ )

If you help me enough, you may be able to use banshee out of the box in the future without the need of patching it!

Thanks

(In reply to comment #4)
> If you help me enough, you may be able to use banshee out of the box in the
> future without the need of patching it!

And your name will be on the credits ;)

BTW it's also good to post the M3uPlaylist class as a patch (git diff) instead of a standalone file.

Created attachment 226385 [details]
Requested Info

The incentive of getting it through the package manager some day was plenty for me :)

Sorry about the tar, I couldn't see any quick way to add a set of files...

I'm a little unsure of the git diff. I just taught myself to stash so I could try and update to the head version, there's some differences in the git diff which I think are upstream though I can see my own modifications in there too.

I've included pretty much everything that's needed to use it in the tar and some bits of banshee's logs from when the playlists are generated and read from the device, in addition to one of the playlists in both standard m3u (with relative paths) and the mangled nokia version.

The only problem I can think so far with it; is that if you save to the root directory of a *nix, the generated playlist is completely mangled (it replaces every '/' with 'E:') though that's pretty minor with the intent.

IIRC there's specific classes for certain media devices in banshee. I'll take a look at one of those and see if I can integrate this with that approach for the N95, which will allow removal of the class from the playlist loading/saving apparatus, obviously that's not the right place for it to show up...

Created attachment 226412 [details] [review]
Add rudimentary support for Nokia N95

I've still left the NokiaM3uPlaylistFormat in both PlaylistParser and PlaylistFileUtil, though it probably shouldn't appear as an export playlist file type...

Without it specified in both, banshee was unable to recognise the playlist support though I may be missing something.

Please excuse the nexus icon I specified, I was bored of the default mass storage one ;)

Oh, and I know I still need to provide some sane Default* overrides...

Created attachment 226416 [details] [review]
Difference between NokiaM3uPlaylistFormat and M3uPlaylistFormat

(In reply to comment #7)
> Created an attachment (id=226412) [details] [review]
> Add rudimentary support for Nokia N95

Now we're talking! This is much easier to review :)


> I've still left the NokiaM3uPlaylistFormat in both PlaylistParser and
> PlaylistFileUtil, though it probably shouldn't appear as an export playlist
> file type...

Exactly.

And I'm even a bit wary about the whole approach of having a new M3uPlaylistFormat class that is almost the same as the original one but with just a few tweaks. It seems we could do it in a better way, maybe having your new class inherit from M3uPlaylistFormat and only introduce new behaviours via the "override" method? (In case you need to override some method in the base class, you can always mark them as "virtual" in case they are not already marked as such.)

For clarity, I'm attaching here a diff between your new NokiaM3uPlaylistFormat and the original M3uPlaylistFormat, so we can understand better the parts that are different. 


> Without it specified in both, banshee was unable to recognise the playlist
> support though I may be missing something.

Not sure what you mean by this.


> Please excuse the nexus icon I specified, I was bored of the default mass
> storage one ;)

Heh. Well that part of the patch would obviously not be accepted, but you can continue with your forked version of banshee if you like to use that icon (obviously the right thing to do here is design a Nokia N95 icon!).

Comment on attachment 226412 [details] [review]
Add rudimentary support for Nokia N95

I'm going to do now a small review of the general patch now, so you can improve it a bit (don't call me picky pls, these nitpicks need to be fixed for the patch to meet the quality standards that are needed to be consistent with the rest of the codebase):



> +{
> +    public class NokiaM3uPlaylistFormat : PlaylistFormatBase
> +    {
> +        public static readonly PlaylistFormatDescription FormatDescription = new PlaylistFormatDescription(
> +            typeof(NokiaM3uPlaylistFormat), MagicHandler, Catalog.GetString("MPEG Version 3.0 Extended - Nokia Variant (*.m3u)"),
> +            "m3u", new string [] {"audio/x-nokia-mpegurl", "audio/nokia-m3u", "audio/nokia-mpeg-url"});

Did you invent these MIME types or did you find them in any Nokia documentation or reference guide?


> +
> +        public static bool MagicHandler(StreamReader reader)
> +        {
> +            string line = reader.ReadLine();
> +            if(line == null) {
> +                return false;
> +            }
> +			
> +            line = reader.ReadToEnd();
> +            if (line.Contains(@"E:"))

I'm still intrigued by this weird requirement of absolute URIs to start with the "E:" drive letter. Are you sure this is a hard requirement?

I saw that you pointed out that URIs without your patch were being generated as simply relative:

#EXTINF:110,Atari Doll - Queen for a Day
../Music/Atari Doll/Atari Doll/01. Queen for a Day.mp3

And then your version was generating them absolute and with the drive:

#EXTINF:110,Atari Doll - Queen for a Day
E:\Music\Atari Doll\Atari Doll\01. Queen for a Day.mp3

But, have you tested with just absolute URIs without the E: drive letter? Like this?

#EXTINF:110,Atari Doll - Queen for a Day
/Music/Atari Doll/Atari Doll/01. Queen for a Day.mp3

It would be interesting to test that. Maybe if this works the M3U playlists used are more "portable".


> +                return true;
> +            return false;
> +        }
> +
> +        public NokiaM3uPlaylistFormat()
> +        {
> +        }
> +
> +        public override void Load(StreamReader reader, bool validateHeader)
> +        {
> +            string line;
> +            string toReplace = getPathReplacement();

For local variables you should use lowercase and underscores, so "to_replace" vs "toReplace".

For method names you should use Pascal notation, so "GetPathReplacement" instead of "getPathReplacement".


> +
> +        (...)
> +        public override void Save(Stream stream, ITrackModelSource source)
> +        {
> +            using(StreamWriter writer = new StreamWriter(stream)) {
> +                writer.WriteLine("#EXTM3U");
> +
> +                string toReplace = getPathReplacement();

Use always a space befor the starting brace, this way:

GetPathReplacement ();

instead of:

GetPathReplacement();

(For readability)

> diff --git a/src/Core/Banshee.Services/Banshee.Services.csproj b/src/Core/Banshee.Services/Banshee.Services.csproj
> index b52262f..2b800f8 100644
> --- a/src/Core/Banshee.Services/Banshee.Services.csproj
> +++ b/src/Core/Banshee.Services/Banshee.Services.csproj
> @@ -23,7 +23,6 @@
>    <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' ">
>      <DebugSymbols>true</DebugSymbols>
>      <DebugType>full</DebugType>
> -    <WarningLevel>4</WarningLevel>
>      <Optimize>false</Optimize>
>      <OutputPath>..\..\..\bin</OutputPath>
>    </PropertyGroup>
> @@ -31,7 +30,6 @@
>      <DebugSymbols>true</DebugSymbols>
>      <DebugType>full</DebugType>
>      <PlatformTarget>x86</PlatformTarget>
> -    <WarningLevel>4</WarningLevel>
>      <Optimize>false</Optimize>
>      <OutputPath>..\..\..\bin\bin</OutputPath>
>      <DefineConstants>WIN32</DefineConstants>
> @@ -40,31 +38,25 @@
>      <Reference Include="System" />
>      <Reference Include="System.Core" />
>      <Reference Include="glib-sharp">
> -      <SpecificVersion>False</SpecificVersion>
> +      <Package>glib-sharp-2.0</Package>
>      </Reference>
>      <Reference Include="System.Xml" />
>      <Reference Include="dbus-sharp">
> -      <SpecificVersion>False</SpecificVersion>
>        <HintPath>..\..\..\bin\bin\dbus-sharp.dll</HintPath>
>      </Reference>

MonoDevelop is very intrusive when you use it: it modifies files without need. So if you could not include these changes, to only have the addition of <Compile> tags in the project files, it would be better (just edit the file manually if needed).


>      <Reference Include="dbus-sharp-glib">
> -      <SpecificVersion>False</SpecificVersion>
>        <HintPath>..\..\..\bin\bin\dbus-sharp-glib.dll</HintPath>
>      </Reference>
>      <Reference Include="taglib-sharp">
> -      <SpecificVersion>False</SpecificVersion>
>        <HintPath>..\..\..\bin\bin\taglib-sharp.dll</HintPath>
>      </Reference>
>      <Reference Include="ICSharpCode.SharpZipLib">
> -      <SpecificVersion>False</SpecificVersion>
>        <HintPath>..\..\..\bin\bin\ICSharpCode.SharpZipLib.dll</HintPath>
>      </Reference>
>      <Reference Include="Mono.Posix">
> -      <SpecificVersion>False</SpecificVersion>
>        <HintPath>..\..\..\bin\bin\Mono.Posix.dll</HintPath>
>      </Reference>
>      <Reference Include="Mono.Addins">
> -      <SpecificVersion>False</SpecificVersion>
>        <HintPath>..\..\..\bin\bin\Mono.Addins.dll</HintPath>
>      </Reference>
>    </ItemGroup>
> @@ -328,6 +320,7 @@
>      <Compile Include="Banshee.Collection.Database\DatabaseYearListModel.cs" />
>      <Compile Include="Banshee.Collection\InvalidFileException.cs" />
>      <Compile Include="Banshee.Sources\IBatchScrobblerSource.cs" />
> +    <Compile Include="Banshee.Playlists.Formats\NokiaM3uPlaylistFormat.cs" />
>    </ItemGroup>
>    <ItemGroup>
>      <EmbeddedResource Include="Banshee.Services.addin.xml">

So, only this change in the .csproj.


> diff --git a/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage.addin.xml b/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage.addin.xml
> index 251e665..08f2034 100644
> --- a/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage.addin.xml
> +++ b/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage.addin.xml
> @@ -96,6 +96,11 @@
>      <MassStorageDevice class="Banshee.Dap.MassStorage.NookDevice"
>          vendor-name="Barnes and Noble" product-name="Nook Classic"
>          vendor-id="0x2080" product-id="0x001"/>
> +		
> +	<MassStorageDevice class="Banshee.Dap.MassStorage.NokiaN95Device"
> +		vendor-name="Nokia Mobile Phones" product-name="Nokia N95"
> +		vendor-id="0x0421" product-id="0x04ed"/>

Use spaces, not tabs.


>    </Extension>
>  
>  </Addin>
> diff --git a/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage.csproj b/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage.csproj
> index d3126e1..5ca2de0 100644
> --- a/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage.csproj
> +++ b/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage.csproj

Same here about the .csproj changes.


> diff --git a/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage/NokiaN95Device.cs b/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage/NokiaN95Device.cs
> new file mode 100755
> index 0000000..d7cb4df
> --- /dev/null
> +++ b/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage/NokiaN95Device.cs
> @@ -0,0 +1,71 @@
> +// 
> +// NokiaN95Device.cs
> +// 
> +// Author:
> +//   Nicholas Little <arealityfarbetween@googlemail.com>
> +// 
> +// Copyright 2012
> +// 
> +// Permission is hereby granted, free of charge, to any person obtaining a copy
> +// of this software and associated documentation files (the "Software"), to deal
> +// in the Software without restriction, including without limitation the rights
> +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> +// copies of the Software, and to permit persons to whom the Software is
> +// furnished to do so, subject to the following conditions:
> +// 
> +// The above copyright notice and this permission notice shall be included in
> +// all copies or substantial portions of the Software.
> +// 
> +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> +// THE SOFTWARE.
> +using System;
> +
> +using Banshee.IO;
> +using Banshee.Hardware;
> +
> +namespace Banshee.Dap.MassStorage
> +{
> +    public class NokiaN95Device : CustomMassStorageDevice
> +    {
> +        #region Delegate/Supported device structure
> +        /*
> +         * The suggestion is that we use a similar strategy as found in playlist
> +         * parsing and handling to select the device type, based upon this sort 
> +         * of signature...
> +         * 
> +         * But then I spotted the xml file with vendor and product id's ;)
> +         */

It's good to immortalize the ramblings we had when implementing some feature, but instead of leaving them on the source code, it's better to put them in the commit message :)

This way, as it's recommended in the our contribution guidelines ( http://banshee.fm/contribute/write-code/ ), you would generate the patch via `git format-patch HEAD~1`.

Created attachment 226420 [details]
Icon set for Nokia N95

Sorry about that, I had read them a long while ago, but sometimes can't shake those Java roots. 

I'll be sure to keep monodevelop on a tight leash in future. Though, perhaps someone could commit the solution file change so XML documents have tabs converted to spaces too (noticed for C# source it was on already...) :P

For the m3u's, in addition to the default m3u export I had tried:
- switching the unix generated directory separators to windows ones but leaving the relative path
e.g. "..\Music\Artist\Album\Tn. Track.mp3"
- leaving the path separators alone, but prefixing with E:
e.g. "E:/Music/Artist/Album/Tn. Track.mp3"

The n95 has a concept of C: and E: drives which I'm pretty sure all N-series devices have in common with maybe the exception of the N900; while in Mass Storage Mode E: is unmounted from the phone and made available to the PC.

I've tidied most of the patch up according to the style guidelines, I'll get to grips with git format-patch sometime soon... I shall likely pop in #banshee and say hi, with one or two questions ;)

Last, I grabbed a half decent image through google, pretty sure it's public domain. I'm no imagemagick wizard but it looks ok from here :)

Oh, and yeah. I completely invented the mime-types, I just needed something to put in the .is_audio_player file to make it work without doing any further subclassing into the Banshee.Dap namespace.

(In reply to comment #11)
> Sorry about that, I had read them a long while ago, but sometimes can't shake
> those Java roots. 

No worries, just correct it in next version of the patch.

> I'll be sure to keep monodevelop on a tight leash in future. Though, perhaps
> someone could commit the solution file change so XML documents have tabs
> converted to spaces too (noticed for C# source it was on already...) :P

Good catch, you can propose the change as a separate patch.


> For the m3u's, in addition to the default m3u export I had tried:
> - switching the unix generated directory separators to windows ones but leaving
> the relative path
> e.g. "..\Music\Artist\Album\Tn. Track.mp3"
> - leaving the path separators alone, but prefixing with E:
> e.g. "E:/Music/Artist/Album/Tn. Track.mp3"

Cool, can you then try what I suggested? (Keep unix path separators, but use absolute path.)


> The n95 has a concept of C: and E: drives which I'm pretty sure all N-series
> devices have in common with maybe the exception of the N900;

I see.


> while in Mass
> Storage Mode E: is unmounted from the phone and made available to the PC.

But it doesn't matter how Windows mounts the phone, what we're discussing here is how the phone deals with its own paths itself, when disconnected from any computer, right?


> I've tidied most of the patch up according to the style guidelines, I'll get to
> grips with git format-patch sometime soon... I shall likely pop in #banshee and
> say hi, with one or two questions ;)

Sure. Just ask your question in the channel, don't wait for me to show up. You may get answers. Or just ask here.

> Last, I grabbed a half decent image through google, pretty sure it's public
> domain.

Cool, can you please include one of these images (and the reference to it in the code) also in the next version of the patch (use the size that other images have already in the codebase, to be consistent). Also include the URL where you got them from (in the commit message), to have a proof they are public domain.

> I'm no imagemagick wizard but it looks ok from here :)

We're not going to have many N95 users of Banshee so I think we should not be especially picky with this :)

(In reply to comment #12)
> Oh, and yeah. I completely invented the mime-types, I just needed something to
> put in the .is_audio_player file to make it work without doing any further
> subclassing into the Banshee.Dap namespace.

Well, but now that you're including this new class, the .is_audio_player file should not be needed, right? So then you can remove any fictional mime types in the next version of the patch.

Thanks for working on this!

Created attachment 226668 [details] [review]
Add better support for the Nokia N95 (and N-series devices, in future...)

Ok, here goes. The style should adhere to the guidelines in this one and I've removed all the extra monodevelop inspired non-sense from the project file patch, which only contains the reference to the extra file in the Banshee.Dap.MassStorage directory.

I tested your suggestion on the playlist. I hadn't expected it to work and it didn't-I tested with dos directory separators as well as *nix ones. I don't think I was very clear on the C: and E: drive thing. The phone mounts its internal storage at C:, and the card at E:, so without the drive letter prefix it's not an absolute path.

I checked the image I modified, turns out it was from amazon, eek! http://ecx.images-amazon.com/images/I/41XVq7MnW8L._SL500_AA300_.jpg

Ok, one final note:

I had to make some slight changes to the PlaylistFileUtil to hide the 'custom' playlist from users at Load/Save time and added a member variable to the PlaylistParser to store the actual playlist which was built in a stack variable anyway. So, basically:

- ExportFormats becomes a subset of DapExportFormats
- PlaylistParser supports all the playlist types
- PlaylistFileUtil.Load() uses PlaylistParser to load the playlist (remove duplication)
- The Load/Save dialog has its own 'valid' set of supported types.

The things I missed above:

It's not just N95's really-if people put in their product id's it should be all Symbian 3.1 devices. I know that's still a small fraction, however ;)

About removing the fictional mime-type (obviously I could remove the extra two I threw in...), I'm not sure how I'd go about doing that since the MassStorageSource gets it's supported playlist format type by string matching against the mime-types specified in each format's PlaylistFormatDescription. 

I'd thought about writing a new source but that seemed quite a bit more difficult and error prone since I'd have to essentially convert the MassStorageSource into one of the new type and all I really need to do is override the PlaylistTypes property and nest my playlist format out of the way...

Hints would be appreciated! :)

(In reply to comment #14)
> Created an attachment (id=226668) [details] [review]
> Add better support for the Nokia N95 (and N-series devices, in future...)
> 
> Ok, here goes. The style should adhere to the guidelines in this one and I've

Thanks for your work Nicholas, patch is getting better, but here is my next review.


> removed all the extra monodevelop inspired non-sense from the project file
> patch, which only contains the reference to the extra file in the
> Banshee.Dap.MassStorage directory.

Cool.

> I tested your suggestion on the playlist. I hadn't expected it to work and it
> didn't-I tested with dos directory separators as well as *nix ones. I don't
> think I was very clear on the C: and E: drive thing. The phone mounts its
> internal storage at C:, and the card at E:, so without the drive letter prefix
> it's not an absolute path.
> 
> I checked the image I modified, turns out it was from amazon, eek!
> http://ecx.images-amazon.com/images/I/41XVq7MnW8L._SL500_AA300_.jpg

I'm afraid we cannot use it then. Cannot you tweak some other image that we're sure is free to use? For example, from wikipedia: 

http://en.wikipedia.org/wiki/Nokia_N95


> Ok, one final note:
> 
> I had to make some slight changes to the PlaylistFileUtil to hide the 'custom'
> playlist from users at Load/Save time and added a member variable to the
> PlaylistParser to store the actual playlist which was built in a stack variable
> anyway. So, basically:
> 
> - ExportFormats becomes a subset of DapExportFormats
> - PlaylistParser supports all the playlist types
> - PlaylistFileUtil.Load() uses PlaylistParser to load the playlist (remove
> duplication)
> - The Load/Save dialog has its own 'valid' set of supported types.

Mmmm, interesting approach.

I think it still can be improved though, I'll try to explain what I mean below.


(In reply to comment #15)
> The things I missed above:
> 
> It's not just N95's really-if people put in their product id's it should be all
> Symbian 3.1 devices. I know that's still a small fraction, however ;)

That's a good point, and now that I look at the code, making the class be named SymbianDevice instead of NokiaN95Device would be more consistent with the other classes there: MaemoDevice, AndroidDevice, WebOSDevice, etc.
Can you make the change?


> About removing the fictional mime-type (obviously I could remove the extra two
> I threw in...), I'm not sure how I'd go about doing that since the
> MassStorageSource gets it's supported playlist format type by string matching
> against the mime-types specified in each format's PlaylistFormatDescription. 

Good point, but let's see this from a different angle: this M3U playlists that the Nokia device needs are actually just M3U playlists.

The only difference between what we're trying to write here and the playlists that current Banshee 2.6.0 writes now is the format of the URI of the elements.

I don't think this difference is enough to call it a different format. I think it's still an M3U playlist format. Just do this little experiment: make your NokiaM3uPlaylistFormat class inherit from M3uPlaylistFormat instead of from PlaylistFormatBase. When you do that, you can reuse a lot hell more code. Like ParseExtended() method.

And let's take a look at this part of the patch:

-                    string trackpath = ExportUri (track.Uri);
-                    if (FolderSeparator == Folder.DosSeparator) {
-                        trackpath = trackpath.Replace (Folder.UnixSeparator, Folder.DosSeparator);
-                    }
-                    writer.WriteLine( trackpath );
+                    newPath = track.Uri.AbsolutePath.Replace(toReplace, @"E:").Replace(System.IO.Path.DirectorySeparatorChar, '\\');
+                    Hyena.Log.Information(newPath);
+                    writer.WriteLine(newPath);

Do you see that there is a part that you're modifying which you don't need to modify? You just need to replace the mounting point with E:. The part which replaces Unix separators with Dos separators is already
done in the line of code you removed:

trackpath = trackpath.Replace (Folder.UnixSeparator, Folder.DosSeparator);

Of course, that would only work if FolderSeparator is Folder.DosSeparator, so you only need to take care that this happens at runtime. How to do it? Well, let's see who sets the FolderSeparator property: in line 360 of MassStorageSource you have:

  playlist_format = Activator.CreateInstance (PlaylistTypes[0].Type) as PlaylistFormatBase;
  playlist_format.FolderSeparator = MediaCapabilities.FolderSeparator;
  
And MediaCapabilities is a property of the source class! So then you can override that property in our SymbianDevice class, and then M3U writing would work out of the box. And if you think about it, you can use the same approach for the "E:" thing. You could have another property in PlaylistFormatBase that is "bool AbsoluteUrisRequired" and defaults to false, but the SymbianDevice somehow marks it as TRUE in the SyncPlaylists method. Then have another property call "RootPath" which defaults to "/", but which we set to "E:\" in SymbianDevice. Do you see where I'm going?

Maybe you see it more clear when I explain how to hook into the playlist Load process (instead of the Save one): you should not need to modify or create your own M3uPlaylistFormat class for this. What you currently do is:

-                    element["uri"] = ResolveUri(line);
+                    line = line.Replace (@"E:", toReplace)
+                               .Replace (@"\", Convert.ToString(Path.DirectorySeparatorChar));
+                    Hyena.Log.InformationFormat ("Nokia playlist import: {0}", line);
+                    element["uri"] = line;

But, why tweaking this at this level? Why not leave the paths with the "E:" format in the element["uri"] object? If we leave them, what happens is that later Banshee can't play them because it cannot find them. Ok, fair enough, but who should be responsible of interpreting those paths? The M3uPlaylist class should not be the one. The M3uPlaylist class shouldn't know any details about how the URIs of the playlists are "special". The class that should know about this little details is SymbianDevice, which inherits from CustomMassStorageDevice. So, how to make this class convert the paths? Well, let's look at how those element["uri"] objects are loaded: in MassStorageSource class, line 180, we have:

  foreach (Dictionary<string, object> element in loaded_playlist.Elements) {
    string track_path = (element["uri"] as Uri).LocalPath;
    (...insert trackpath into DB...)
                        
So here we have the place where the URI is inserted to the DB (and the DB is queried later by the player
engine to check where is the resource that it needs to play.

So we could create a new method this way:

protected virtual string GetTrackPath (Uri playlistPathElement) {
    return playlistPathElement.LocalPath;
}

Then we modify the foreach to be:

  foreach (Dictionary<string, object> element in loaded_playlist.Elements) {
    string track_path = GetTrackPath(element["uri"] as Uri);
    (...insert trackpath into DB...)
    
And then we override this method in our SymbianDevice class:

protected override string GetTrackPath (Uri playlistPathElement) {
    return playlistPathElement.ToString().Replace ("E:\", GetRootPathOfMountPointForThisDevice());
}

(This way I also renamed "GetPathReplacement" to something a bit more understandable :)


> I'd thought about writing a new source but that seemed quite a bit more
> difficult and error prone since I'd have to essentially convert the
> MassStorageSource into one of the new type and all I really need to do is
> override the PlaylistTypes property and nest my playlist format out of the
> way...
> 
> Hints would be appreciated! :)

I hope those are enough hints for now to get you started on a little nice refactoring. Believe when I say this is a very interesting learning experience and you will be much more satisfied with the result when you realise how much simpler things can end up being :)

BTW, after finishing this review and looking a bit more around the code, I found actually some things that I would like to rectify, but it's easier to do here is an addendum instead of going and fix up what I already wrote: when I suggested the 2 new properties AbsoluteUrisRequired and RootPath, well, I just realised, that there is a property that is already meant for this! It's called "BaseUri" and it is defined in PlaylistFormatBase. Then, the method that takes care of converting a URI of an element in a device (let's say /mnt/mydevice/Music/U2-Lemon.mp3) to a URL to be written in a playlist (E:\Music\U2-Lemon.mp3) is in the ExportUri() method. You just need to be clever to previously set BaseUri to what it should be so ExportUri stops using relative paths and replaces the mount point part of the path with just "E:" (maybe the key is just about setting "BaseUri" property to "E:\Music\").

Let me know what you think!
Thanks again.

Created attachment 226903 [details] [review]
Testing approach...

Hey. Been experimenting... 

First, the mundane: Did the renames, of both NokiaM3uPlaylistFormat and NokiaN95Device, Now: SymbianM3uPlaylistFormat and SymbianDevice. I'm yet to add a switch statement to SymbianDevice for the icon, it's on the todo... The M3u variant is a whole lot leaner now, it deserved an update as it was the first hack I put together to kick this off. However, I have the solution working without it; though I'm not sure what you'll think.

The problems come from the SafeUri class, and the handling of the BaseUri property in PlaylistFormatBase. I've essentially identified some dead code too...

If BaseUri isn't null, then the path is checked if it's a local one, ie. that the schema is "file://", if so-that triggers relative path names to be calculated and resolved, when saving and loading respectively. So I thought, ok; i'll set BaseUri to null and modify the Uri of each track going in so that it doesn't get resolved and you just get the bare string, ie. that it doesn't begin with "file://". Unfortunately BaseUri can't be set that way (hence, the dead code) which kinda forces the relative path resolution.

So, what I've done is this:

Added two delegates to the Banshee.Playlists.Formats namespace

string Exporter(SafeUri uri);
SafeUri Resolver(string uri);

and UriResolver/Exporter properties to IPlaylistFormat, and PlaylistFormatBase. If these are set they are used in ExportUri and ResolveUri to do the right thing (tm). To get at them, I've added two new events and an associated EventArgs class to the MassStorageSource (it should probably live higher up the tree, but I put it here for clarity of the diff):

SyncEventHandler OnSyncStart;
SyncEventHandler OnSyncDone;

The OnSync* events are registered by SymbianDevice in it's SourceInitialise(), I've settled for anonymous methods to keep it simple for now. 

When OnSyncStart is fired SymbianDevice registers it's anonymous Exporter with the M3uPlaylistFormat class, allowing it to completely handle the translation from SafeUri to playlist file name.

In OnSyncDone, we don't have the chance to hook into the playlist because it's already been loaded by that time through PlaylistFileUtil. Just a quick for loop to swap the E: and the folder separator to their real filesystem dependent values.

On a side note: I've been working on the icon from Wikipedia, I vaguely remember looking at it. It's quite difficult to prepare because of the background having quite a few mixed shades but I'm making some headway. I'll post when it's close to decent.

Let me know what you think, it's working for me, for now ;)

A little more information, cos I'm scatterbrained and full of coffee... lol

As I said before, BaseUri can't be null. As you suggested, I tried changing that to "E:\Music" but the path's generated by the M3u class via line 106 in PlaylistFormatBase had very unusual generated path names, this is with a mount point of /media/N95. Similar result with "E:\" for BaseUri:

#EXTM3U
#EXTINF:110,Atari Doll - Queen for a Day
..\..\..\..\..\..\media\N95\Music\Atari Doll\Atari Doll\01 - Queen for a Day.mp3

For it to work like that, I think the SafeUri's in the PlaylistSource would need to be reconstructed with similar format, e.g. replace "file:///media/N95" with "file://E:", but I believe that the construction will fail at the local path checking (unless it's on a windows system, when it may succeed; but only by a fluke if the system decides to choose E: as the drive for the phone's storage card)

I looked at overriding GetTrackPath(TrackInfo, out string); in the device class, but that looked very much like it required a valid filesystem reference for whatever OS the user was currently on. Tbh, I think the event + delegate solution is a neater alternative.

Created attachment 228205 [details] [review]
Testing my approach :)

Hey Nicholas! So sorry for the delay, had a very busy 2 weeks and an exhausting weekend in between, but the last couple of days I've been able to sit down again to look at this.

I'm sorry, but I'm still convinced that we can get the patch much simpler than with all the juggling you did (yes, very clever code about the event and the delegate, but sometimes smaller/simpler code is less clever but better and more concise :) ).

To try to explain my point, I actually implemented what I meant, rather than just trying to make you imagine my castles in the air :D

So, I have a Maemo device (not a Nokia N95) but I cooked a very simple patch which manages to write the "E:\" paths of the NokiaN95 to my devices. You'll see that:
a) It's a much simpler approach.
b) It is consistent to current codebase (which advocates more for virtual/overridable methods instead of null/non-null delegates).
c) Doesn't need to create a subclass of M3uPlaylist, but just uses that with some properties grabbed from the specifics of the device.
d) Doesn't make playlist code be aware of drives :) Decoupling ftw.

Please let me know what you think! If you like it, just borrow my code to put it in your patch, and don't worry, I will still attribute the patch to you, because you've spent more time than me on it than me anyway :)

Hi Andrés, don't worry about a few weeks here and there. I work through the week myself and I've been patching banshee with this for a _long_ time before I thought to make a report and submit the first naive patch :) I haven't had time to actually apply your patch, but I will try it out. However reading it I've a few observations.

First glances, your version is definitely far less invasive. Though my updated SymbianM3u class was just for posterity sake in the last diff...

I'm not entirely sure about b) & d)-as the solution still comes down to a null member check in the PlaylistFormatBase and MassStorageSource class-though it is the *Device that provides a property instead of something live. It was the magic handlers in the playlists that made me think of delegates :P lol. I know what you mean though, it only takes a couple of semi-anonymous methods floating around before things get confusing...

Your solution does take away the need for the device to touch the playlist, letting the source mediate with some extra properties. Far cleaner in the MassStorageSource.

Last, I'm wondering how it works when the device is plugged back in? When I've been testing, without handling the incoming playlist the paths can't be resolved-leaving empty entries for each (though of course the main Music entry works as normal). It's a minor problem, definitely, and I couldn't solve it without the event. Getting that behaviour with your approach would be a bit more work-as would plugging in the UriResolver delegate too.

Anyway, I'll give it a test run this weekend and submit again, this time with images!

Cheers!

(In reply to comment #20)
> Hi Andrés, don't worry about a few weeks here and there. I work through the
> week myself and I've been patching banshee with this for a _long_ time before I
> thought to make a report and submit the first naive patch :) I haven't had time
> to actually apply your patch, but I will try it out. However reading it I've a
> few observations.

Cool :)


> First glances, your version is definitely far less invasive. Though my updated
> SymbianM3u class was just for posterity sake in the last diff...

Not sure what you mean. Your last patch is still useful: it's better than the previous ones, for example, because you inherit from M3uPlaylist instead of PlaylistFormatBase, so it is simpler (less code). But with my approach what I try to demonstrate is that you don't need the class at all, because you can just tweak it to be a bit more aware of different path-type requirements.


> I'm not entirely sure about b) & d)-as the solution still comes down to a null
> member check

Yeah, I studied first a simple bool, but then for the "true" case a base path needs to be supplied, so why not just assuming that null is a "false"?


> in the PlaylistFormatBase and MassStorageSource class-though it is
> the *Device that provides a property instead of something live. It was the
> magic handlers in the playlists that made me think of delegates :P lol.

Yeah, I don't like that "magic handler" either (especially the naming, I guess I'll rename that at some point). But being able to reuse M3uPlaylist without the need of creating a new playlist class will let you be able to not deal with the MagicHandler at all, I think.


> I know what you mean though, it only takes a couple of semi-anonymous methods 
> floating around before things get confusing...

Exactly ;)


> Your solution does take away the need for the device to touch the playlist,
> letting the source mediate with some extra properties. Far cleaner in the
> MassStorageSource.

That is the thing, we push the "ugly details of how a N95 behaves" to the SymbianClass.


> Last, I'm wondering how it works when the device is plugged back in?

Heh, yeah, I didn't say my patch is complete. I just wanted to show you my approach for *writing* the "E:" paths, but my gut feeling is that with this approach, the reading of the paths will be simpler as well (just tweak the Resolve() method this time instead of the Export/Save).

> When I've
> been testing, without handling the incoming playlist the paths can't be
> resolved-leaving empty entries for each (though of course the main Music entry
> works as normal). It's a minor problem, definitely, and I couldn't solve it
> without the event. Getting that behaviour with your approach would be a bit
> more work-as would plugging in the UriResolver delegate too.

Yeah, what I mean is: try to follow my approach for writing too. You should not need any event or delegate.

> Anyway, I'll give it a test run this weekend and submit again, this time with
> images!

Thanks Nicholas!

Typo: "try to follow my approach for *reading* too"

(In reply to comment #21)
> (In reply to comment #20)
> > Hi Andrés, don't worry about a few weeks here and there. I work through the
> > week myself and I've been patching banshee with this for a _long_ time before I
> > thought to make a report and submit the first naive patch :) I haven't had time
> > to actually apply your patch, but I will try it out. However reading it I've a
> > few observations.
> 
> Cool :)
> 
> 
> > First glances, your version is definitely far less invasive. Though my updated
> > SymbianM3u class was just for posterity sake in the last diff...
> 
> Not sure what you mean. Your last patch is still useful: it's better than the
> previous ones, for example, because you inherit from M3uPlaylist instead of
> PlaylistFormatBase, so it is simpler (less code). But with my approach what I
> try to demonstrate is that you don't need the class at all, because you can
> just tweak it to be a bit more aware of different path-type requirements.

Yeah, that's what I meant. I needed a warm up when I was preparing to get down to work so the SymbianM3u was the first place I started. But, it's not actually used in the diff-it's the plain old M3u class that does the heavy lifting. 

I'll try and go step by step to see how our patches are similar, though the distribution of work is slightly different and as you say the style is virtual/overrides in banshee.

Both start off with extra properties in the PlaylistFormatBase:

yours: DefaultBaseUri and RootPath
mine:  UriExporter and UriResolver

Then, when a playlist is synced-they differ slightly:

yours: MassStorageSource checks a property on the Device, the Source then sets the RootPath and DefaultBaseUri properties
mine:  MassStorageSource fires an event, registered by the Device, containing the Playlist about to be synced, then the Device sets the UriExporter

Finally, the distribution is slightly different when exporting in the playlist (and intended in mine when resolving...):

yours: The M3u class checks the DefaultBaseUri for null and performs the RootPath replacement
mine:  The M3u class checks the UriExporter for null, and calls the Device method to handle the string manipulation

<snip />

> Yeah, what I mean is: try to follow my approach for reading too. You should not
> need any event or delegate.

I shall do, no worries ;)

Created attachment 228637 [details] [review]
Completes your approach ;)

Hi Andrés,

Got around to reverting my local repo, minor disaster in the meantime. Thought I'd lost my SymbianDevice, thank bob for StackOverflow-I'm definitely still a git novice.

I applied your patch, minus the MaemoDevice changes and added similar support when importing playlists back. That bit involved an extra conditional to get valid SafeUri's back in my particular case, with the dos folder separator. The Uri.LocalPath was still such as E:\Music\... I was maybe expecting the folder separator would translate to the host platform's separator. To address that, in OnImportFinished() we check the Device.RootPath and replace that, but first we replace the Device.FolderSeparator if it's not null with whatever the current platform's separator happens to be.

The icons were originally the wikipedia image as you suggested. Sizes should be fine, 256x256 and all multiples down. I've done my best to remove the background though it's a little fuzzy around the edges. I tried to make it in keeping with the style of having the phone appear active so I took a screenshot while playing some music on my device and pasted that into the screen area, I don't feel sick when I look at it. hehe

I'm happy if you are :)

That looks like a very pretty patch!! (Only 1 new class added, and the rest are modifications to already existing classes! so clean! :) )

I'll review it over the weekend, maybe I can fix a nitpick or two from "my approach", but it's almost ready.

Thanks Nicholas

Created attachment 228640 [details] [review]
Completes your approach, take 2

Whoops, don't know what I was thinking... The conditional for Device.FolderSeparator wasn't valid, not a string. Checked the initialisation and it's pretty much guaranteed to be set. I thought it had compiled ok, but it hadn't.

Sorry about that!

(In reply to comment #26)
> Created an attachment (id=228640) [details] [review]
> Completes your approach, take 2
> 
> Whoops, don't know what I was thinking... The conditional for
> Device.FolderSeparator wasn't valid, not a string. Checked the initialisation
> and it's pretty much guaranteed to be set. I thought it had compiled ok, but it
> hadn't.
> 
> Sorry about that!

Meh! That leads me to think you didn't test the patch :/ did you? :)

I tested Take 2, then had a lapse in concentration/sanity and started worrying about the FolderSeparator... Made some modifications to cover that, and failed to notice that the Banshee.Dap.MassStorage assembly hadn't actually compiled in monodevelop until I went to 'make install' it... Oops! :-$

I double checked it the second time, and the FolderSeparator is pretty much guaranteed...

Created attachment 228648 [details] [review]
update for format guidelines, strip unnecessary override

Ok, hopefully third time's the charm.

This one has one less method override in SymbianDevice (it wasn't doing anything anyway) and the formatting should be spot on. Otherwise, it's identical to the last one...

(In reply to comment #29)
> Created an attachment (id=228648) [details] [review]
> update for format guidelines, strip unnecessary override
> 
> Ok, hopefully third time's the charm.
> 
> This one has one less method override in SymbianDevice (it wasn't doing
> anything anyway) and the formatting should be spot on. Otherwise, it's
> identical to the last one...

Cool thanks. Sorry I ended up not having much time to code this weekend, and I anyway didn't finish doing this that I said:

> I'll review it over the weekend, maybe I can fix a nitpick or two from "my
> approach", but it's almost ready.

The thing I wanted to fix is stop relying so much on strings for paths (to avoid PrimitiveObsession[1]), so I went ahead and tried to replace as many strings as I could with SafeUri objects. However I hit a roadblock when trying to use it for "E:", and I thought it was a bug in glib, so I reported bug 688074, and it turns out it's the correct behaviour!

The tricky thing boils down to our need of manipulation of Windows paths within a non-Windows operating system (as glib for Windows works as expected), so I'll have to use string, or System.Uri in this case :( I've been also looking at the code in PlaylistFormatBase.cs and anyway it's already using string and Uris a lot anyway (instead of SafeUri, which I thought was the holy grail!), I guess because of similar reasons...

Anyway, I also refactored a bit some things from this class so your patch will need rebasing, sorry about that! I'll take care of it as soon as possible (hopefully during the week, otherwise in the next weekend).

[1] http://c2.com/cgi/wiki?PrimitiveObsession

Created attachment 229950 [details] [review]
Patch v8

Hey Nicholas, sorry again for the delay.

We're almost done, I think I've finally finished doing some improvements to your patch:

> The tricky thing boils down to our need of manipulation of Windows paths within
> a non-Windows operating system (as glib for Windows works as expected), so I'll
> have to use string, or System.Uri in this case :( I've been also looking at the
> code in PlaylistFormatBase.cs and anyway it's already using string and Uris a
> lot anyway (instead of SafeUri, which I thought was the holy grail!), I guess
> because of similar reasons...

Ended up using "Uri" for this. This way we don't need to use the confusing "String.IsNullOrEmpty" (because we were not using empty strings anyway) but just a null check.


> Anyway, I also refactored a bit some things from this class so your patch will
> need rebasing, sorry about that! I'll take care of it as soon as possible
> (hopefully during the week, otherwise in the next weekend).

I also updated the patch to compile with latest Banshee master.

Last things I did:

- Removed the confusing path replacement in the foreach that looped through the playlist.Elements:
+   if (!string.IsNullOrEmpty (ms_device.RootPath)) {
+       track_path = track_path.Replace (ms_device.FolderSeparator, 
+                                        System.IO.Path.DirectorySeparatorChar)
+                              .Replace (ms_device.RootPath, volume.MountPoint);
+   }

Because there is already the method ResolveUri inside PlaylistFormatBase to "rework" the elements found in the playlist before they are put in the Elements collection.

To be honest I'm not entirely happy with this solution because, as I noted in a comment as a "TODO" we should maybe move the Resolve() method outside the playlist-handling logic (or at least outside the Load() operation of it), but that would need a very serious refactoring which I'm not brave to do without writing a bunch of unit tests right now. On the way I also found out that the NormalizeToUnix() sanitization we do is actually wrong, because according to what I discovered in comment#30, a path with ":" and "\" characters is perfectly valid in Unix, and then the NormalizeToUnix call is just assuming that every "\" is just a Windows path separator. Taking advantage by the fact that we need to do some prove in this patch now to find out what is the correct path from the file-system, I've also fixed this, to only call NormalizeToUnix after a check for existance of the file has been proven negative. Adding some LINQ to the mix has actually made the code even a bit neat!

I also removed the DefaultBaseUri property because I realised we could reuse BaseUri for its purpose all the time.

Do you mind testing this patch to make sure I didn't break anything Nicholas? Should be the last thing to do!

Created attachment 229951 [details] [review]
Patch to Hyena (part of v8)

Oh, you also need this patch in Hyena to test the previous one.

Thanks again!

Review of attachment 229950 [details] [review]:

Hi Andrés, I got around to testing out the latest version of the patch along with the hyena upgrade.

I was actually pretty surprised that your modifications in ResolveUri didn't cause a crash. I'll try and illustrate-it's a point that I was reluctant to address in earlier versions as the workaround; performing a path string substition in OnImportFinished() on each element of the loaded playlist, was much simpler:

The playlist is loaded and hence ResolveUri runs on line 174 of the MassStorageSource, independently of any device held properties, e.g. RootPath. This behaviour didn't cause any trouble in the past-the Uri class just loaded the string as best it could, the problem is when a SafeUri is constructed at line 184, in order to match the track on the device with a database entry, an exception is thrown then. So it surprised me that the new ResolveUri which constructs a SafeUri in it's linq predicate didn't cause the same exception to propogate!

Anyway, I see what you did, but it's the same issue. The linq predicate actually returns null for the 'mangled' playlists, so MassStorageSource gets the same uri "file:///E:/Music/..." which is what causes an exception in SafeUri in OnImportFinished(), when trying to match the file with a database track. Tbh, I'm not sure that what you've done will work, even if RootPath could be set on the playlist (perhaps as an argument to PlaylistFileUtil.Load()), because NormaliseToUnix("E:\Music\...", true) will drop the E:, at which point MakePathAbsolute(normalised, RootPath, BaseUri.LocalPath) will fail as RootPath ("E:") is not contained in the path anymore.

So, the quickest solution was a small hack, add another yield value to the enumeration:
yield return new Uri (BaseUri.LocalPath + Paths.NormalizeToUnix(uri, true).Substring(1));

Although I'm bothered I had to include the substring(1) to strip the leading slash before path concatenation as SafeUri says it exists as a file, but DatabaseTrackInfo.GetTrackIdForUri() is returning an index of 0. So without stripping the leading slash, no tracks in the playlists are recognised. Apart from that, this works well, but perhaps it hi-lighted a separate issue...

However-it doesn't work for tracks which don't exist on the device but are in the playlist. I know it's a fringe problem, but I've had problems with certain oggs exported to mp3 by banshee and I have to delete those files before I refresh my music database, which leaves the stale entries in the playlist-the linq statement ends up returning the mangled path since the enumeration can't generate a valid possible path for the track, for the obvious reason that it's not there anymore. Back to the original exception on line 184 of the MassStorageSource. I enclosed that in a 'try { track_id = DatabaseTrackInfo.GetTrackIdForUri(); } catch { track_id = 0; }' for now.

Both the above small fixes are just hacks so I haven't produced a patch with them. I understand what you mean by the primitive obsession, but I'm not sure trying to use SafeUri everywhere is the answer. As you've seen, it's difficult to do because of the restrictions placed upon that class and the distinction i think is quite descriptive-Uri's loaded from external sources may not be safe, whereas the database only deals with the safe variety which must be a valid path in the host OS.

Also, we still haven't dealt with the dead wood, we do quite a few checks for BaseUri being null in the PlaylistFormatBase, when in fact there's no condition when it will be as it's initialised in the default constructor and the get/set only takes a Uri object, trying to set a null value will cause an exception there. It is wholly right that BaseUri should never be null though as it's used in resolving paths relative to the playlist location.

Export is not bad at all, but I am a little bothered by the extra overhead incurred by the linq enumeration on resolution. For each playlist element we construct at least one Uri and one SafeUri-the SafeUri is then thrown away, to be constructed again later in MassStorageSource. Generally that will only run once as almost all playlist entries will be recognised on the first try-so probably not much of an issue, but I'm wondering about the platform invokation that's done when constructing one. The case is worse for mangled playlists, as the enumeration occurs at least 3 times before a valid one is settled upon.

Cheers Andrés!

*but I've had problems with certain
oggs exported to mp3 by banshee and I have to delete those files before I
refresh my music database [on the Symbian device]

*platform invokation
that's done when constructing one [a SafeUri]. The case is worse for mangled playlists, as
the enumeration occurs at least 3 times before a valid one [Uri/SafeUri combination] is settled upon.

Created attachment 230489 [details] [review]
Patch v9

Hey Nicholas, thanks for reviewing! To be honest I'm a little surprised the patch didn't work for you, because I tested it with my device (of course tweaking my MaemoDevice class instead of using the SymbianDevice one) and it worked for me.

Anyway, it's good that this is taking longer because we're getting slowly there, not sacrifying code quality by being maybe too much perfectionist (but at least you don't seem to be so bothered by that, which is a relief!). I say this because this new patch I'm posting, if it works for you, is a good result of my better understanding of the code and a re-think about my Resolve() approach after you raised some concerns in comment#33. Anyway, see inline for my explanations:

(In reply to comment #33)
> Review of attachment 229950 [details] [review]:
> 
> Hi Andrés, I got around to testing out the latest version of the patch along
> with the hyena upgrade.
> 
> I was actually pretty surprised that your modifications in ResolveUri didn't
> cause a crash. I'll try and illustrate-it's a point that I was reluctant to

Not sure why you think they would cause a crash? Take in account there are two elements that guard against that in the code:

1) If none of the candidates return true for the Banshee.IO.File.Exists() check, then candidate is set to null (because I'm using ".FirstOrDefault()" as opposed to just ".First()") instead of throwing, and then the first non-existing candidate is selected.

2) The function that calls ResolveUri() has a try-catch that simply removes the element from the playlist to load if there has been any problem (exception) when trying to resolve the URI. It's in M3uPlaylistFormat.cs:

	try {
		element.Uri = ResolveUri (line);
	} catch {
		Elements.Remove(element);
	}


> address in earlier versions as the workaround; performing a path string
> substition in OnImportFinished() on each element of the loaded playlist, was
> much simpler:

Yes it's simpler code, but I think it's a bit hacky. Here's why: when you're replacing the paths you're effectively rejecting the previous Uri Resolution ( ResolveUri() ) operation that happened earlier, and executing yours in a different level of abstraction. This means that a) the current Uri-Resolution code is not wide enough to cover this use case (Windows URIs in a playlist) therefore I think it's better to improve the Playlist code overall to support that, b) it is at the wrong level of abstraction because in the place of the
code where it happens we don't have access to the device details (the RootPath value we're setting => E: ).

If you think about it, there's no need why the Playlist classes and methods should try to resolve a Uri. It's a need that is required in different areas of the code: when the elements of a playlist need to be played or when they need to be played (i.e. in OnImportFinished(), MassStorageSource class).
So what I would like to see, instead of a replace of the string value of the "uri" key of the Elements collection of that playlist there, is a call to ResolveUri there. Something like:

 foreach (PlaylistElement element in loaded_playlist.Elements) {
-	string track_path = (element["uri"] as Uri).LocalPath;
+	string track_path = Resolve(element["uri"]);
 	int track_id = DatabaseTrackInfo.GetTrackIdForUri (new SafeUri (track_path), DbId);

You see what I mean? That's why I ranted this way in my previous patch:

+   // ResolveUri is called by the Load process, no exception should be thrown even if some
+   // item does not appear to exist. TODO: maybe decouple ResolveUri from playlists/load logic?

This way we would not need to do the Exists() call in every element of the playlist to resolve it, because at that level of abstraction we would have access to the RootPath of the device (E:) and then we would simply replace it, and we're good to go.

So, given that this patch didn't work for you, I started again to expore this kind of refactoring before landing this patch. On the way, I found interesting things, like the fact that there was no need for the playlist elements to be loosely-typed, so I changed them this way:

http://git.gnome.org/browse/banshee/commit/?id=38e90dd7358db06fe2fd0350e93210bbc2fe61c4

Also, the commit above is a bit simpler thanks to this previous change I did which seems insignificant but helped a lot:

http://git.gnome.org/browse/banshee/commit/?id=b3a60c09e2605e03acd301bae3808f8ea3b337c4

This led me to think that I could possibly manage to refactor the whole thing without the need to write hundreds of unit tests to make sure I don't break anything (because the changes so far seemed simple enough and pretty much safe thanks to the compiler). However, I finally discarded the idea after finding how tightly coupled the Resolve operation is to the code that supports playlists to be inside playlists (mms, asx, etc.). To give you a brief idea of what I mean, this is the unfinished patch that I came up with, which obviously I didn't commit because it's too messy and probably breaks stuff.

After this you may think I gave up! Well it turns out that I discovered there were already some existings tests around playlists which could be a good insurance for our refactorings... So, after unbreaking them ( http://git.gnome.org/browse/banshee/commit/?id=4fd016bc3f51fb1265ab4adac32ebd754d4dcfd2 ) I started to think again about the refactoring... And I thought that after creating PlaylistElement class it could be actually a good opportunity to have the Resolve() operation in this class! So in MassStorageSource we would end up with:

 foreach (PlaylistElement element in loaded_playlist.Elements) {
-	string track_path = element.Uri.LocalPath;
+	string track_path = element.ResolveUri();
 	int track_id = DatabaseTrackInfo.GetTrackIdForUri (new SafeUri (track_path), DbId);


And for that, Uri would be replaced by the plain unresolved "location" element that we get in the playlist, this way:

 public class PlaylistElement
 {
 	public string Title { get; internal set; }
-	public Uri Uri { get; internal set; }
+	public string Location { get; internal set; }
 	public TimeSpan Duration { get; internal set; }
 }

However, at this point I had an eureka moment and went back to tinker with the Load() method of PlaylistFileUtil. Well, it turns out it was only used twice in the code! So changing it would actually maybe not be such a disruption. And actually, 2 minutes later I found out that one of the places where it was used was even dead code! So I removed it:

http://git.gnome.org/browse/banshee/commit/?id=8ba10cd3119c6216538e9d9a8b662e705897384d

So, given that after this commit, this method was only used in one place (MassStorageSource) I went ahead and modified it to add the new parameter (RootPath). So all solved! I'm then attaching the result of all this experimentation (tested again with my MassStorage device), let's call it v9.

> 
> The playlist is loaded and hence ResolveUri runs on line 174 of the
> MassStorageSource, independently of any device held properties, e.g. RootPath.
> This behaviour didn't cause any trouble in the past-the Uri class just loaded
> the string as best it could, the problem is when a SafeUri is constructed at
> line 184, in order to match the track on the device with a database entry, an
> exception is thrown then. So it surprised me that the new ResolveUri which
> constructs a SafeUri in it's linq predicate didn't cause the same exception to
> propogate!

The reason is that before constructing the SafeUri object I'm already stripping the "E:\\" part with MakePathAbsolute().


> Anyway, I see what you did, but it's the same issue. The linq predicate
> actually returns null for the 'mangled' playlists, so MassStorageSource gets
> the same uri "file:///E:/Music/..." which is what causes an exception in

That means that the stripping of E: somehow didn't work for you. It's weird because, as I said, I tested it on my end...


> SafeUri in OnImportFinished(), when trying to match the file with a database
> track. Tbh, I'm not sure that what you've done will work, even if RootPath
> could be set on the playlist (perhaps as an argument to
> PlaylistFileUtil.Load()), 

Exactly, a new argument to the Load() method. I was discarding that because at first glance, I thought the Load method was being called by lots of code, and would be a hard refactoring for just a new overload.


> because NormaliseToUnix("E:\Music\...", true) will
> drop the E:, at which point MakePathAbsolute(normalised, RootPath,
> BaseUri.LocalPath) will fail as RootPath ("E:") is not contained in the path
> anymore.

But that wasn't supposed to fail! Because as I couldn't inject the "E:"-RootPath in the Load() method, RootPath has there its default value ("/") and what would happen is simply replace "/Music/SomeSong.mp3" with "/media/mnt1/Music/SomeSong.mp3".

> 
> So, the quickest solution was a small hack, add another yield value to the
> enumeration:
> yield return new Uri (BaseUri.LocalPath + Paths.NormalizeToUnix(uri,
> true).Substring(1));
> 
> Although I'm bothered I had to include the substring(1) to strip the leading

That was my initial approach before using Paths.MakePathAbsolute, because I was bothered with that hack too.


> slash before path concatenation as SafeUri says it exists as a file, but
> DatabaseTrackInfo.GetTrackIdForUri() is returning an index of 0. So without
> stripping the leading slash, no tracks in the playlists are recognised. Apart
> from that, this works well, but perhaps it hi-lighted a separate issue...
> 
> However-it doesn't work for tracks which don't exist on the device but are in
> the playlist. I know it's a fringe problem, but I've had problems with certain
> oggs exported to mp3 by banshee and I have to delete those files before I
> refresh my music database, which leaves the stale entries in the playlist-the
> linq statement ends up returning the mangled path since the enumeration can't
> generate a valid possible path for the track, for the obvious reason that it's

Meh, that's a hard problem, which is also very related to the fact that the Resolve operation is, again, in playlist code instead of Dap code. Anyway, let's deal with that later in a separate bug after we're done with this, ok?


> not there anymore. Back to the original exception on line 184 of the
> MassStorageSource. I enclosed that in a 'try { track_id =
> DatabaseTrackInfo.GetTrackIdForUri(); } catch { track_id = 0; }' for now.
> 
> Both the above small fixes are just hacks so I haven't produced a patch with
> them. I understand what you mean by the primitive obsession, but I'm not sure
> trying to use SafeUri everywhere is the answer. As you've seen, it's difficult

Well, I didn't use SafeUri everywhere either, just were I needed to (because Banshee.IO.File.Exists receives a SafeUri).


> to do because of the restrictions placed upon that class and the distinction i
> think is quite descriptive-Uri's loaded from external sources may not be safe,
> whereas the database only deals with the safe variety which must be a valid
> path in the host OS.
> 
> Also, we still haven't dealt with the dead wood, we do quite a few checks for
> BaseUri being null in the PlaylistFormatBase, when in fact there's no condition
> when it will be as it's initialised in the default constructor and the get/set
> only takes a Uri object, trying to set a null value will cause an exception
> there. It is wholly right that BaseUri should never be null though as it's used
> in resolving paths relative to the playlist location.

I believe it can be null for cases in which playlists are just pointing at non local paths (i.e. http URLs to playlists somewhere on the internet), but anyway with this patch I think we don't have to worry about this any more.


> Export is not bad at all, but I am a little bothered by the extra overhead
> incurred by the linq enumeration on resolution. For each playlist element we

I guess you mean Resolve, not Export, here.


> construct at least one Uri and one SafeUri-the SafeUri is then thrown away, to
> be constructed again later in MassStorageSource. Generally that will only run

I was not that worried about the construction of SafeUri objects to be honest. It's done already in lots of places in the code. Granted, it would be better to have less allocations, but here the benefits of gradually moving our base APIs (like Banshee.IO in this case, which sits on top of all IO backends as an facade to them) by far outweights the drawbacks.


> once as almost all playlist entries will be recognised on the first try-so
> probably not much of an issue, but I'm wondering about the platform invokation
> that's done when constructing one. The case is worse for mangled playlists, as
> the enumeration occurs at least 3 times before a valid one is settled upon.

Yes I see what you mean and I was worried about that too (but somehow I thought that the previous approach we did in a patch some time ago when we introduced the call to NormalizeToUnix was not really right, because paths can exist in Unix with back slashes without meaning directory separators, so I still believe the Resolve() operation should still maybe do some File.Exists checking before leaning to guess what a file path exactly means). Anyway let's deal with that later because with this new patch I'm not using Exists() any more, as I said.

Let me know if it works for you, and what you think! Hopefully we're now much closer to the end!

Created attachment 230490 [details] [review]
hyena patch for v9


Oh and here's the new Hyena patch.

BTW some typos in my previous comment:

- "when the elements of a playlist need to be played or when they need
to be played" -> "when the elements of a playlist need to be played or when they need to be stored in the DB"

- this is the unfinished patch that I came up with -> I was going to give you a pastebin link to the patch, but somehow I lost it (I do git reset --hard too often!)

- "just were I needed to" -> "just where I needed to"

I also had some typos in the mini-diffs I added in-line. They should have been:

 foreach (PlaylistElement element in loaded_playlist.Elements) {
-    string track_path = element.Uri.LocalPath;
+    string track_path = element.ResolveLocation (baseUri, rootPath);
     int track_id = DatabaseTrackInfo.GetTrackIdForUri (new SafeUri
(track_path), DbId);

and

 public class PlaylistElement
 {
     public string Title { get; internal set; }
-    public Uri Uri { get; internal set; }
+    public string Location { get; internal set; }
     public TimeSpan Duration { get; internal set; }
+
+    public Uri ResolveLocation (Uri baseUri, Uri rootPath) {
+    ....
+    ...
 }

Review of attachment 230489 [details] [review]:

Good morning, just got round to testing today. Very short review, i'll keep to the basics cos I've not had breakfast yet...

Export:  
Functioning, as per v8.

Resolve: 
Perfect. The additional argument to PlaylistFileUtil.Load() ensures that resolution only occurs once, in a constant and predictable fashion-as influenced by the device through the RootPath property, mirroring the Export pattern, thus improving readability ;) Now there's no post-processing in the MassStorageSource.OnImportFinished(), and the SafeUri constructor no longer throws "Filename path must be absolute" as every PlaylistElement has been translated to a valid path on the host system-so no need for the try/catch protection.

Summary:
Plugged it in, worked a treat. Makes sense with other patterns. The rename of MakePathAbsolute() to SwitchRoot() is logical and more descriptive.

I see what you mean about TDD from commit 4fd016. I guess my next feature request will have to come with a test case ;)

Created attachment 231036 [details] [review]
MassStorage Universal Playlist Recognition

I have been thinking a little more about the v8 patch though... The problem with it was the extra work per playlist element really.

This approach I just thought of, and it's kind of a Eureka, it seems too simple but I'm having trouble thinking of a flaw!

Our database stores the proper AbsoluteUri to each file scanned on any MassStorageSource we support. So, the shortest possible path match between a file on the device and a playlist path entry is the file name, right?

So: In OnImportFinished(), after we've read in the playlist elements (no matter how they're formatted) we should be able to get a short list of candidates by doing a query on that database.

I built on the v9 patch, with this one. The modifications weren't too many but my device wasn't recognised due to the lack of reference in the addins and the SymbianDevice class when I reset --hard... I disabled the extra resolution code by passing in null for the RootPath argument to the new PlaylistFileUtil.Load() method.

I haven't tested it very much ;) it works for my device, but I think it would be a more general solution to the path resolution issue...

In comment#38 and comment#39 you say "v8", when I guess you actually mean "v9" right?

The flaw I see with this approach is the following:
- You pass null as RootPath, then the elements with a "E:\" path are not resolved, why do you want that?
- If you don't resolve the elements, you would not store in the DB paths accessible by the system that is running Banshee (which is what is needed to be able to play the songs in the device *within* banshee).


> I haven't tested it very much ;) it works for my device, but I think it would
> be a more general solution to the path resolution issue...

To be honest I'm not sure what are you trying to solve here. Anyway, as you seem to be mixing bits and pieces with my previous patch, I propose that we commit version v9, and then you base your next patches on top of banshee master. This way the patch will be very clear on what it tries to achieve.

No, there's no flaw in v9. It's great, that's why I marked it commit_now :D In v8, Export was functioning great. It was just the Resolve that was difficult-you fixed that beautifully in v9.

The last diff I posted is purely for resolution, as an alternative to v8's TryResolve() approach. But, I'd already applied v9 to my source tree and I'm very happy with it. So I just disabled the v9 resolve code by passing the null, so the uri's are resolved as normal.

I was just considering, because banshee already knows all the valid AbsoluteUri's, we can resolve unusual paths in playlists by doing a partial match on the filename in the playlist-using an SQL query against the database.

(In reply to comment #41)
> No, there's no flaw in v9. It's great, that's why I marked it commit_now :D In
> v8, Export was functioning great. It was just the Resolve that was
> difficult-you fixed that beautifully in v9.
> 
> The last diff I posted is purely for resolution, as an alternative to v8's
> TryResolve() approach. But, I'd already applied v9 to my source tree and I'm
> very happy with it. So I just disabled the v9 resolve code by passing the null,
> so the uri's are resolved as normal.

Ok.


> I was just considering, because banshee already knows all the valid
> AbsoluteUri's, we can resolve unusual paths in playlists by doing a partial
> match on the filename in the playlist-using an SQL query against the database.

It's this what I don't understand. You're mixing levels of abstraction here. So, not sure you're understanding the whole concept: if it's the first time that you plug your device, those songs will be added to the DB. The only reason to look for the songs in subsequent syncs (subsequent times you plug your device to your computer when Banshee is running) is not to insert them again. So we need an exact search here, not a fuzzy search. And as far as I understand, the exact search would work in v9, and tracks would not be added twice to the DB.

(In reply to comment #42)
> (In reply to comment #41)
<snip />
> > I was just considering, because banshee already knows all the valid
> > AbsoluteUri's, we can resolve unusual paths in playlists by doing a partial
> > match on the filename in the playlist-using an SQL query against the database.
> 
> It's this what I don't understand. You're mixing levels of abstraction here.
> So, not sure you're understanding the whole concept: if it's the first time
> that you plug your device, those songs will be added to the DB. The only reason
> to look for the songs in subsequent syncs (subsequent times you plug your
> device to your computer when Banshee is running) is not to insert them again.
> So we need an exact search here, not a fuzzy search. And as far as I
> understand, the exact search would work in v9, and tracks would not be added
> twice to the DB.

I didn't modify how many times the tracks are inserted into the respective tables, just how the path name is resolved to a track id in the MassStorageSource via a new function in DatabaseTrackInfo, which does the fuzzy resolution if the regular function doesn't get a valid track id. It still doesn't guarantee a match.

It was just an idea based on what you were doing in v8 with TryResolve()-but the other way on because it takes into account that we know all the possible valid SafeUri objects that are in the MassStorageSource-they've already been added to the database by the time we process the playlist. So we let the playlist give us whatever Uri it cares to, and we fuzzily match that against the tracks we know are on the device when we do the insert into CorePlaylistEntries. The only thing we insert there is a playlist and track id.

(In reply to comment #43)
> I didn't modify how many times the tracks are inserted into the respective

I didn't say you did, I was just putting you in context :)


> tables, just how the path name is resolved to a track id in the
> MassStorageSource via a new function in DatabaseTrackInfo, which does the fuzzy
> resolution if the regular function doesn't get a valid track id. It still
> doesn't guarantee a match.
> 
> It was just an idea based on what you were doing in v8 with TryResolve()-but
> the other way on because it takes into account that we know all the possible
> valid SafeUri objects that are in the MassStorageSource-they've already been
> added to the database by the time we process the playlist. So we let the
> playlist give us whatever Uri it cares to, and we fuzzily match that against
> the tracks we know are on the device when we do the insert into
> CorePlaylistEntries. The only thing we insert there is a playlist and track id.

But take in account the process is like this:

FooUri in playlist -> resolve to Uri understandable and playable by Banshee -> store in the DB

You would still need to resolve the Uri the first time you connect the device, because the song is still not there.

And if you were thinking Resolve() is slow, not sure it is slowER than a fuzzy search in the DB (the DB is in disk as well) ;-)

Yep. I didn't think there was any reason that the process couldn't be:

path in playlist -> resolve to formatted uri -> resolve to track id in DB -> store in playlist.

Speed was an unmentioned assumption, that the DB would be quicker than generating an IEnumerable<T> ;)

anyway, I'll be very happy to see v9 next time I pull. Then I can get on and parse the on device media database to sync some play counts!

Created attachment 231054 [details] [review]
v10

Small corrections, and adds a unit test which v9 didn't have.

Created attachment 231056 [details] [review]
Patch v11

Hopefully the last version.

Managed to get the unit testing up and running eventually. I just needed to ./configure --enable-tests to get at them in monodevelop.

Patch works great, all tests in PlaylistFormatTests report success :)

Comment on attachment 230490 [details] [review]
hyena patch for v9

http://git.gnome.org/browse/hyena/commit/?id=a2f18e52e618bc7aa3f6ad3aa66072acbe89f9e2

Comment on attachment 231056 [details] [review]
Patch v11

http://git.gnome.org/browse/banshee/commit/?id=55272ed4af65d986cadf623fd7cb983cad57ffe4

(In reply to comment #49)
> Managed to get the unit testing up and running eventually. I just needed to
> ./configure --enable-tests to get at them in monodevelop.
> 
> Patch works great, all tests in PlaylistFormatTests report success :)

Great! Thanks for all your work Nicholas, and for your patience!

I just committed the patches to master.

Hope to work with you in more patches :)

(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.)

Great news Andrés, performed my pull this afternoon. Nice!

Looking forward to it ;)