Re: XMMS2 Plugin

[Ulrik Sverdrup <ulrik sverdrup gmail com>]

2011/4/4 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

Merge time is finally here, after our stable releases.  I've looked
(quickly) at your repository and I think the remaining issue is the
URL handling. On my second thought cycle about this I concluded that
this is the wrong thing to do. Something like unicode URLs that are
created in xmms2.py does not exist, it's simply the URL encoding. I
think you should use the URLs as they are and convert them to paths
using gio.

--ulrik