Re: XMMS2 Plugin

[Curious Leo <curiousleo ymail com>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/04/11 17:53, Ulrik Sverdrup wrote:
> 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

Thanks for the extensive feedback! I'll look into the issues you've
pointed out. And yes, the XMMS2 plugin is of course based on the
Rhythmbox plugin -- I'm sorry I didn't mention that.

Leo
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJNmYjSAAoJEMO+Cz6xjiWB9xwH/1ujpRFHCgy0NLjZ35CZfd/Q
UmaXa11BHwlERcdigCOw1Q4c71zg0B/tGE9U5XRqZmshUaNmDK5jMGQYGxtboyCY
Rp4prLcnsXlEjp6iX/VHswpSER+h+uiOB3n6XCz1i6bzVPI0J6q3iL+3x/HNeN1t
lG390BAJCCM4rXChqjlv77ZeTG0Cs3wqGX2ImNpIMt5krPwVWhnha2YZGkezjr7D
JzYgdZ4vvDhrVP6TKn36Zu/01CNFN0+26GqNwxSji+yHkHAy8wVzdKwdQRbpp/b1
9VO1s+mcPetV6J7Z7t4RvGVuloSNfTVRi/GIIFXj7ZddefXLhi67wSUfwXbFPSE=
=11Rr
-----END PGP SIGNATURE-----