After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 546962 - Patch to upgrade rhythmbox to libmtp 0.3.0
Patch to upgrade rhythmbox to libmtp 0.3.0
Status: RESOLVED FIXED
Product: rhythmbox
Classification: Other
Component: Plugins (other)
0.11.x
Other All
: Normal enhancement
: ---
Assigned To: RhythmBox Maintainers
RhythmBox Maintainers
: 543645 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2008-08-08 16:18 UTC by Linus Walleij
Modified: 2008-08-09 22:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch to migrate IF to libmtp 0.3.0 and add a few features too (1.90 KB, patch)
2008-08-08 16:19 UTC, Linus Walleij
none Details | Review
Patch to make autoconf detect 0.3.0+ and #define HAVE_LIBMTP_030 if found (655 bytes, patch)
2008-08-08 17:12 UTC, Linus Walleij
none Details | Review
tested configure.ac changes (720 bytes, patch)
2008-08-08 17:42 UTC, Christophe Fergeau
none Details | Review
Patch to migrate IF to libmtp 0.3.0 and add a few features too (1.94 KB, patch)
2008-08-08 20:12 UTC, Linus Walleij
none Details | Review
Cumulative patch with ChangeLog (3.39 KB, patch)
2008-08-08 20:25 UTC, Christophe Fergeau
needs-work Details | Review
Diet patch that only add support for libmtp 0.3.0 (1.25 KB, patch)
2008-08-08 23:36 UTC, Linus Walleij
committed Details | Review
Handle playcount and rating on MTP devices (1.53 KB, patch)
2008-08-09 09:25 UTC, Christophe Fergeau
committed Details | Review

Description Linus Walleij 2008-08-08 16:18:24 UTC
I have written a patch to upgrade Rhythmbox to use libmtp 0.3.0. We need to apply this to the Fedora package to be able to move the entire distribution to libmtp 0.3.0.

It is possible to use autoconf magic to define some preprocessor #define to make it compile with older libmtp as well, I haven't looked into it. One option is to check the library for the LIBMTP_Detect_Raw_Devices() function for example.
Comment 1 Linus Walleij 2008-08-08 16:19:17 UTC
Created attachment 116160 [details] [review]
Patch to migrate IF to libmtp 0.3.0 and add a few features too
Comment 2 Christophe Fergeau 2008-08-08 16:43:52 UTC
*** Bug 543645 has been marked as a duplicate of this bug. ***
Comment 3 Christophe Fergeau 2008-08-08 16:47:24 UTC
libmtp 0.3.0 is really new, a configure check would be really nice especially since there would be only 1 line to #ifdef. 
Ratings on mtp devices have to be divided/multiplied by 20 ? I'm checking because it's exactly the same for ipods, so I want to make sure this is not a copy and paste error ;)
Comment 4 Linus Walleij 2008-08-08 16:53:58 UTC
Ratings on MTP devices are 0..100 and Rhythmbox awards
0..5 stars which means 20 x [0 .. 5] = [0 .. 100] and
I guess the iPod ratings are also 0..100 probably the
MTP rating was plagiarized from the iPod range.
Comment 5 Linus Walleij 2008-08-08 17:12:22 UTC
Created attachment 116164 [details] [review]
Patch to make autoconf detect 0.3.0+ and #define HAVE_LIBMTP_030 if found

I don't have a working GNOME build system on this machine, but can you test if this patch to configure.ac will properly #define HAVE_LIBMTP_030 so that the code can be altered accordingly?
Comment 6 Christophe Fergeau 2008-08-08 17:42:08 UTC
Created attachment 116169 [details] [review]
tested configure.ac changes

I tweaked your patch a little, this one seems to work though I only tested it without libmtp 0.3.0
Comment 7 Linus Walleij 2008-08-08 20:12:46 UTC
Created attachment 116184 [details] [review]
Patch to migrate IF to libmtp 0.3.0 and add a few features too

A newer version that checks for the #define from the other patch.
Comment 8 Linus Walleij 2008-08-08 20:13:56 UTC
If these two work out together on your setup, I suggest they be applied!
Comment 9 Christophe Fergeau 2008-08-08 20:25:16 UTC
Created attachment 116186 [details] [review]
Cumulative patch with ChangeLog

This should be good to commit though it would be nice if someone could confirm things still work when you don't have libmtp/when you have libmtp 0.3.0
Comment 10 Linus Walleij 2008-08-08 23:18:08 UTC
I seem to have problems when compiling from source with this, version (or I just got deps or something wrong) so put it on hold for the time being. (Investigating.)
Comment 11 Linus Walleij 2008-08-08 23:36:38 UTC
Created attachment 116195 [details] [review]
Diet patch that only add support for libmtp 0.3.0

Sorry have to revert out the playcount and rating additions, this just doesn't work for the time being. This also fixes a minor bug that made configure never detect libmtp 0.3.0 and not compile for it.

Tested with source compile and libmtp 0.3.0.
Comment 12 Linus Walleij 2008-08-08 23:39:06 UTC
FYI my code failed with:

(rhythmbox:5683): Rhythmbox-WARNING **: cmd_next: Unhandled error: Problem occurred without error being set. This is a bug in Rhythmbox or GStreamer.
**
** RhythmDB:ERROR:(rhythmdb.c:4977):rhythmdb_entry_get_ulong: code should not be reached

Looks like I'm looking up something I shouldn't :-/
Comment 13 Jonathan Matthew 2008-08-09 00:52:27 UTC
rhythmdb doesn't do type conversion, so you have to access the rating using rhythmdb_entry_get_double rather than rhythmdb_entry_get_ulong.
Comment 14 Jonathan Matthew 2008-08-09 02:19:49 UTC
I've committed a slightly modified version of this - PKG_CHECK_EXISTS is more appropriate for the second check, and avoids some problems I was having that looked like pkg-config results being cached.  I've checked this with libmtp 0.2.6 and 0.3.0, and without libmtp installed.
Comment 15 Christophe Fergeau 2008-08-09 09:25:51 UTC
Created attachment 116216 [details] [review]
Handle playcount and rating on MTP devices

This patch will probably work to update playcount and rating but I don't have a device for testing
Comment 16 Linus Walleij 2008-08-09 18:51:25 UTC
Thanks Jonathan, the commit works like a charm!

Christophe, your latest patch also work swell, can it
be committed? libmtp 0.3.0 and 0.2.x has the same API
here so works fine on both.