Re: XMMS2 Plugin

[Ulrik Sverdrup <ulrik sverdrup gmail com>]

2011/3/31 Curious Leo <curiousleo ymail com>:
> Hi all,
>
> I wrote a plugin for the music daemon XMMS2. It still lacks support for
> XMMS2 collections and playlists, but you can use it to look for an
> artist/an album/a title and play or enqueue it.
>
> I would be glad if you could take a look (and maybe even test it --
> XMMS2, anyone?):

Hi there

First comments. There is no rush since we're doing bugfixes-only this
week before the next release.

It looks nice and it's based right off the Rhythmbox plugin. This is
good. I want to refactor the rhythmbox plugin too a bit. I've also
been concerned about the growing number of translation strings. The
reuse here is good but I wonder if we can parameterize these
descriptions:

_("Start playback in %s") % _("Rhythmbox")

etc, so that we re-use these strings completely across Rhythmbox,
XMMS, audacious and future plugins. I'll ask translators if this is
ok.  Anyway this is a smaller thing that we can do when cross-updating
xmms and rhythmbox.py etc together.

 Common for xmms2 and rhythmbox

• the extra _support.py files are only cluttering up the plugin
directory, no need for these files if they are not shared across
plugins
• Use spawn_async_raise, catch utils.SpawnError and raise
OperationError  which will give the user a nice visible explanation if
spawn_async can't find the program. This is easily done in a helper
function.
• Are the "toplevel_*" settings still needed now that we have global
catalog preferences? We have to investigate that
• Maybe unify the description strings as above

For xmms2 only

• time.sleep is of course useless. You can use a timeout callback
instead  glib.timeout_add(100, callbackname, arg1, arg2)
• For the notify-send comment, instead use kupfer.uiutils, it has a
function for sending notifications.
• Two lines in xmms2.py are too long (over 80 chars) and they are easy to fix
• I would use a fairly short timeout in get_xmms2_songs, like
sqlite3.connect(... timeout=2)
• use kupfer.config.get_config_file("medialib.db", package="xmms2")
instead of hardcoding ~/.config
• the encoding carousel is an unfortunate mess but it's apparently
from their database
• This is not needed because you can pass an URI to gio.File(..)
 fpath = self.object[0]["url"].replace("file://", "")
(also, it is an incorrect transmutation for many urls)

-ulrik