Re: XMMS2 Plugin

[Curious Leo <curiousleo ymail com>]

On 26/04/11 18:48, Ulrik Sverdrup wrote:
> 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

Cool, how about this:
<https://github.com/curiousleo/kupfer/commit/11ca2ec449c57433796d7b933cba6bbe42691839>?

I got rid of all dirty URL-conversion except for replacing all pluses by
spaces -- otherwise gio.File won't find the file...

Leo