This is in response to bug 516407, comment 1

I've implemented a Conduit Data Source using the Rhythmbox DBus interface provided by the above Rhythmbox plugin.
It is based on the static playlists Rhythmbox Data source. The UI looks the same, and the only modifications were to use the DBus interface instead of parsing the Rhythmbox DB. 
This data source does not overwrite the older Rhythmbox source, it creates a new, called "Rhythmbox Music (DBus)". The name is mostly for debugging purposes.

Any feedback is welcomed.

Created attachment 107257 [details] [review]
Rhythmbox DBus data source in svn diff form

Comment on attachment 107257 [details] [review]
Rhythmbox DBus data source in svn diff form

Alexandre, this is totally awesome. I really like the implementation, you have followed the coding style, and implemented things just as would have liked.

Top work. I have highlighted any small issues below

>+
>+class RhythmboxDBusFactory(DataProvider.DataProviderFactory):
>+    """

1) 
In the long run, I think it is better, more discoverable for the user, if the RB dbus dataprovider is always available in the dataproviders list, and simply fails refresh() if Rhythmbox (and your plugin) is not loaded. The factory is fine in the meantime.

>+
>+    def _dbus_connect(self):
>+        """
>+        Connect to the Rhythmbox DBus object. Only availiable when Rhythmbox
>+        is running with the DBus plugin activated.
>+        """
>+        try:
>+            rb_obj = self.bus.get_object(DBusPath, "/")
>+            return dbus.Interface(rb_obj, DBusPath)
>+        except dbus.DBusException:
>+            raise Exceptions.SyncronizeFatalError()

2)
How about caching the interface

try:
    if self.rb == None:
        rb_obj = self.bus.get_object(DBusPath, "/")
        self.rb = dbus.Interface(rb_obj, DBusPath)
except dbus.DBusException:
    raise Exceptions.SyncronizeFatalError()
return self.rb

>+
>+    def refresh(self):
>+        DataProvider.DataSource.refresh(self)
>+        self.allPlaylists = self._get_playlists()

3)
As in 1), if this was a dp and not a factory it would go like
try:
    self._dbus_connect()
except Exceptions.SyncronizeFatalError:
    raise Exceptions.RefreashError("Could not connect to RB")

>+
>+CoversPath = os.path.expanduser("~/.gnome2/rhythmbox/covers/")
>+
>+class RhythmboxAudio(Audio.Audio):
>+
>+    def __init__(self, URI, **kwargs):

4)
I like
class RhythmboxAudio(Audio.Audio):
    COVERS_PATH = os.path.expanduser("~/.gnome2/rhythmbox/covers/")
    def __init__(self, URI, **kwargs):

Anyway, this is all really great - I am keen to get this in, and even ship the conduit RB plugin with conduit, if it makes for a more pleasant user experience.

Are you also working on putting the music onto the ipod using python gpod?

Hi,

I'm glad you liked it.

About 1, 2 and 3, I'll implement that. However, I have to take into account when the user closes Rhythmbox with an active connection through DBus and then reopens Rhythmbox, and the DBus connection becomes invalid (as my tests have shown), and so the cache must be recreated. I'll try to fix it with the code that currently detects when the Rhythmbox DBus interface is availiable.
And 4 is good too, I'll do that.

Shipping with Conduit would be great, but I think this DBus extension to Rhythmbox should be inside Rhythmbox. I don't know GObject very well, but I'm trying to implement that. In the short term this plugin could be a good solution.

About the iPod support, I'm using libgpod, in fact, I only extended the current iPod support in Conduit. However the libgpod 0.6 Python bindings have a few bugs I'm trying to fix (one is already fixed in libgpod svn). It is already working for me, but with a few glitches.

(In reply to comment #4)
> Hi,
> 
> I'm glad you liked it.
> 
> About 1, 2 and 3, I'll implement that. However, I have to take into account
> when the user closes Rhythmbox with an active connection through DBus and then
> reopens Rhythmbox, and the DBus connection becomes invalid (as my tests have
> shown), 

Ooh, I did not know that. Thanks for the info

and so the cache must be recreated. I'll try to fix it with the code
> that currently detects when the Rhythmbox DBus interface is availiable.
> And 4 is good too, I'll do that.
> 
> Shipping with Conduit would be great, but I think this DBus extension to
> Rhythmbox should be inside Rhythmbox. I don't know GObject very well, but I'm
> trying to implement that. In the short term this plugin could be a good
> solution.

I agree. It probbably comes down to the folks who package conduit as much as anything.

> 
> About the iPod support, I'm using libgpod, in fact, I only extended the current
> iPod support in Conduit. However the libgpod 0.6 Python bindings have a few
> bugs I'm trying to fix (one is already fixed in libgpod svn). It is already
> working for me, but with a few glitches.

Cool. I also use gpod from SVN for photo support. Its disappointing they dont release more frequently.

Created attachment 107360 [details] [review]
Rhythmbox DBus data source with changes

This is a new patch implementing the changes discussed above.

The DBus interface is being cached and automatically recreated when it is made unavailiable.
The data source is made always availiable, failing to Refresh if Rhythmbox is closed. However, it also fails to configure, without any warning or message error. Any suggestions how this could be improved?

(In reply to comment #6)
> Created an attachment (id=107360) [edit]
> Rhythmbox DBus data source with changes
> 
> This is a new patch implementing the changes discussed above.

That all looks good. I like the magic DbusConnection wrapper, that is something that well be useful enough to be pulled out into conduit.Utils or conduit.Dbus

> 
> The DBus interface is being cached and automatically recreated when it is made
> unavailiable.
> The data source is made always availiable, failing to Refresh if Rhythmbox is
> closed. However, it also fails to configure, without any warning or message
> error. Any suggestions how this could be improved?

Unfortunately not. I guess its a compromise, its now more discoverable, but at the price of this bit of uglyness. I think fixing this might fall under the category of passing more useful errors back to the user, which is a feature that I have been meaning to add to conduit for a while - shown the Exception message in a small status bar for example

Would you mind posting your ipod specific changes when you get a chance, I would quite like to have a play

Thanks

John

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.

I committed all this in r1391. Discussion regarding the iPod music support is on bug #510279

How is all this going? Can I be of any help?

I've worked in improvements for the Rhythmbox module. I've unified both the File and the DBus backends, and I've included the Rhythmbox DBus plugin with Conduit, so it's easily availiable to anyone that wants it.
This work is availiable in https://code.launchpad.net/~airmind/conduit/rhythmbox

Created attachment 112013 [details] [review]
Changes described above in diff form