GNOME Bugzilla – Bug 688540
Banshee syncs incorrect file type to MTP with libmtp >= 1.1.0 / libmtp.so.9
Last modified: 2013-01-06 15:26:43 UTC
Created attachment 229241 [details] [review] Minimal fix that assumes you are using libmtp >= 1.1.0 As of March 2011 [1] (subsequently released as libmtp 1.1.0 [2] in June 2011, which appears to be libmtp.so.9) libmtp changed its FileType enum and added a "Folder" type to the top of the list. This causes Banshee's FileType to be off by one when transferring files, so MTP devices that are sensitive to the reported file type (e.g. my Creative Zen X-Fi) think the file is a WAV when really it is MTP. This causes synced files to fail to play correctly. The attached patch will fix the problem (and is currently in my OBS build [3]) but isn't backwards compatible with people still using libmtp < 1.1.0 (libmtp.so.8) [1] http://libmtp.git.sourceforge.net/git/gitweb.cgi?p=libmtp/libmtp;a=commitdiff;h=a8b8889ee3664b2c966a2816c25a9a536bedca64#patch2 [2] http://libmtp.git.sourceforge.net/git/gitweb.cgi?p=libmtp/libmtp;a=commit;h=1aec6fd1f49307ea20ccb6e8099292e3bf260c19 [3] https://build.opensuse.org/package/show?package=banshee&project=home%3AIBBoard%3Adesktop
Thanks for the patch IBBoard! Do you mind making it backwards compatible by using some version detection at configure time? It shouldn't be too hard. I'll be on IRC if you need a hand.
Sorry, didn't get an email about this. I could try some version detection, but I didn't have a clue how it was done (I assumed it was flags rather than versions, and I didn't want to go adding arbitrary flags!). I'll see what I can do this weekend.
Considering how broken mtp support is I would advocate just bumping the requirement. 1.1.0 is nearly 18 months only and we typically only support stuff going back 18 months anyways (though that is typically measured from what is in distros). For Ubuntu that means we can still support things back to Ocelot (2011-08-09 - they ship 1.1.0). For Fedora every release from F16 and onwards has had 1.1.5 pushed as an update (2011-11-08) For OpenSuSE 12.1 shipped with 1.1.0 (2011-11-16) - I don't know if 11.4 had updates pushed for libmtp. Regardless we are on a new development cycle, adding another 6 months to those dates amply puts us beyond the 18 months guideline. Can we please do the sane thing and bump the requirement for the next stable and merge this patch?
Created attachment 229761 [details] [review] Fix with version checking I think this should be the correct fix - I'm just building it on the OBS now. Changes since last time: * Doesn't add "Album" and "Playlist" to FileType (which were added to libmtp earlier, but aren't used in other code and are unrelated to this change) * Add conditional compile-time check in configure script that looks for WAV being defined as 1 (instead of 0) * Add "#if" block around Folder in FileType.cs
Thanks for the patch IBBoard. To be honest I think I'm going to agree with David's approach here: just because it conforms more to the KISS principle. If we just require libmtp 1.1.0, we: - Make sure the user is running a not too old version of mtp, which may mean less bugs too (and 1.1.0 doesn't seem too bleeding edge either). - Don't need any version detection in the already convoluted M4 macros. - Don't need ifdefs in C# files, cleaner. - It would mean including all enum values of file type (Album, Playlist, too): even though they are not used, it maps cleanly to the native side, and it allows contributors to use them in case they want to hack something together (and lack time to figure out why those elements are not in the enum in the first place).
Created attachment 231067 [details] [review] Patch for master Ok, what I propose is that we commit IBBoard's patch to master and the 2-6-0 branch (in case we do a 2.6.1 release), and then bump the requirement. This then would be the patch for master, and as you see it would make everything a bit cleaner. BTW IBBoard, you need to use "git format-patch HEAD^" to publish patches, so they come with the commit message (and with your name and email address, which I don't know, can you tell please?).
typo: s/name and email address/full name/ (because yes I know where to get your email address obviously :) )
Created attachment 232811 [details] [review] Correctly exported patch Correctly formatted patch (delayed because I was still expecting to get notified, but didn't).
Created attachment 232828 [details] [review] Optional patch for logging during configure While having some problems on the OpenSuse Build Service, I decided it might be a good idea to have it output a message about whether it had enabled the FOLDER code or not. I'm not sure that this is the best way, but it seems to work!
Comment on attachment 232811 [details] [review] Correctly exported patch Thanks for the patch ! I've committed a reworked version to git master and the stable-2.6 branch: http://git.gnome.org/browse/banshee/commit/?id=de85d6a2
Comment on attachment 232828 [details] [review] Optional patch for logging during configure I've included logging in the reworked patch I've committed.
Comment on attachment 231067 [details] [review] Patch for master I've bumped the libmpt requirement to 1.1.0 in git master.
This problem has been fixed in our software repository. The fix will go into the next software release. Thank you for your bug report.