GNOME Bugzilla – Bug 679252
Banshee.Unix.IO Directory.GetFiles () returns plain filenames instead of URIs
Last modified: 2012-08-21 20:44:49 UTC
Created attachment 217820 [details] Return URIs instead of plain filenames The Banshee.Unix.IO provider returns plain filenames in the Directory.GetFiles () method as opposed of URIs (with file:// prefix) that are returned by the Banshee.Gio.IO provider on linux. This leads to problems with playlist syncing on OS X. The attached patch return file URIs the same way Banshee.Gio.IO does. The patch should not affect the linux build, since the IO Provider is dynamically loaded as an extension, where Linux uses the Banshee.Gio provider and Mac OS X uses the Banshee.Unix.IO provider.
Created attachment 217821 [details] [review] same patch - just forgot to hit the "patch" button in the bugreport same patch - i just forgot to mark the attachment as patch.
Comment on attachment 217821 [details] [review] same patch - just forgot to hit the "patch" button in the bugreport Looks good, nice catch! This prompted me to look at Banshee.IO.IDirectory interface again, and I finished some refactoring I wanted to do some months ago so this doesn't happen again. Patch coming..
Created attachment 218220 [details] [review] Use strongly-typed return values for GetFiles() and GetDirectories() After your commit, I would apply this refactoring... which highlighted that we had the same bug in the Banshee.SystemIO.Directory class!
Created attachment 218221 [details] [review] Change SafeUri to be a struct (lightweight, memory-wise) If the above refactoring gets a thumbs-up review, I think this change in hyena's SafeUri would help too. (The initialization of values on every constructor are needed, or otherwise the compiler complains with "error CS0171: Field `Hyena.SafeUri.foo' must be fully assigned before control leaves the constructor". And I couldn't wrap all init values in a private parameterless ctor to be called by the other ctors because apparently structs cannot have parameterless ctors!) PS: while working on this, I also committed this nitpick in Hyena: http://git.gnome.org/browse/hyena/commit/?id=8ba7e7415cb939fcd4fe79a5993b774a58e7439b
Review of attachment 217821 [details] [review]: Thanks for the patch, sorry it took so long to review it. With this patch applied, the unit tests now produce an error : Errors and Failures: 1) Test Error : Banshee.IO.Tests.GetChildFiles System.Exception : Expected element /home/lorentz/Projets/banshee/bin/tmp-io test-dir/baz not found in actual array at Banshee.IO.Tests.AssertContainsSameElements (System.String[] expected, System.String[] actual) [0x00000] in <filename unknown>:0 at Banshee.IO.Tests.<GetChildFiles>m__17 () [0x00000] in <filename unknown>:0 at Banshee.IO.Tests.ForEachProvider (System.Action action) [0x00000] in <filename unknown>:0 at Banshee.IO.Tests.GetChildFiles () [0x00000] in <filename unknown>:0 at (wrapper managed-to-native) System.Reflection.MonoMethod:InternalInvoke (System.Reflection.MonoMethod,object,object[],System.Exception&) at System.Reflection.MonoMethod.Invoke (System.Object obj, BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x00000] in <filename unknown>:0
Review of attachment 218220 [details] [review]: Thanks for the patch, sorry it took so long to review it. Good catch on the FIXME! But with this patch, the build fails if unit tests are enabled (./configure --enable-tests), they need to be updated: MCS ../../../bin/Banshee.Core.dll ./Banshee.IO/Tests.cs(339,21): error CS1502: The best overloaded method match for `Banshee.IO.Tests.AssertContainsSameElements(string[], string[])' has some invalid arguments ... And extra points for checking that all in-tree extensions still build, and mega-bonus points for updating the Community Extensions for the API change ;)
(In reply to comment #6) > Review of attachment 218220 [details] [review]: > > Thanks for the patch, sorry it took so long to review it. Thanks for the review! > Good catch on the > FIXME! > But with this patch, the build fails if unit tests are enabled (./configure > --enable-tests), they need to be updated: Ah, good catch! > MCS ../../../bin/Banshee.Core.dll > ./Banshee.IO/Tests.cs(339,21): error CS1502: The best overloaded method match > for `Banshee.IO.Tests.AssertContainsSameElements(string[], string[])' has some > invalid arguments > ... > > And extra points for checking that all in-tree extensions still build, and > mega-bonus points for updating the Community Extensions for the API change ;) Good points, I'll try to have a look at this the next weekend.
Comment on attachment 218221 [details] [review] Change SafeUri to be a struct (lightweight, memory-wise) BTW, I'm marking this other patch I created as rejected because I'm now wary of it: it's not good to have mutable structs. And SafeUri is sadly mutable if you have a closer look.
Review of attachment 218221 [details] [review]: I'm not sure I understand why a struct is better in this case. Do you have numbers to show what improvement it brings ? Looking at the recommendation at http://stackoverflow.com/questions/521298/when-to-use-struct-in-c which are the same than in my trusty "Framework Design Guidelines" book, I'm not convinced SafeUri is a good fit.
Comment on attachment 218221 [details] [review] Change SafeUri to be a struct (lightweight, memory-wise) Uh, I don't think I set the status to "rejected", bugzilla must have misinterpreted my comment ;)
Comment on attachment 218221 [details] [review] Change SafeUri to be a struct (lightweight, memory-wise) Crap, I didn't see the comment from Andrés, back to rejected.
Well, well, everybody can safely ignore comments 9 to 12...
Comment on attachment 218220 [details] [review] Use strongly-typed return values for GetFiles() and GetDirectories() I've now committed the patch, along with the necessary updates to the unit tests. Thanks to you both!
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.