GNOME Bugzilla – Bug 533727
Support new Magnatune accounts
Last modified: 2010-02-15 03:55:05 UTC
Please describe the problem: Magnatune has launched new membership types[1], and the plugin should support them. I've implemented the streaming account type so far. [1] http://magnatune.com/info/api Steps to reproduce: Actual results: Expected results: Does this happen every time? Other information:
Created attachment 111106 [details] [review] suport streaming accounts Allow the user to set their username, and set the file URIs appropriately when loading the catalogue. The password is asked the first time they try to play a file. The preferences window is too big for some reason.
Created attachment 111565 [details] [review] support all accounts This patch adds support for both account types. Changes: - ask for the account type, and let gnomevfs deal with the username/password - support download accounts (as much as possible, see issues below) - change "Purchase Album" button to "Download Album" Also: - use an svg logo in all the glade files, in response to http://tinyurl.com/4krqut Issues: - the preferences and purchase dialogs now have lots of extra space, probably due to something I did editing the glade files. Not sure what's causing that. - the download account API doesn't actually work yet, I'll email John about getting that fixed so this can be committed.
Created attachment 111566 [details] svg logo for Magnatune
Created attachment 112345 [details] [review] fix API URL and quoting issues This fixes the download API URL, and no longer quotes the URLs received from Magnatune (since they should now properly quote them themselves). It just needs some testing to make sure it works, and that the new Magnatune behaviour doesn't break old rhythmbox/Amarok plugins.
I've just posted some details on what we need to do for authentication to the mailing list. In short: for streaming or download accounts, we need to prompt for and validate the username/password while updating the catalogue, and then include the details in the playback URI. Everything else looks OK to me. I think I looked at making the glade stuff a bit more consistent with respect to spacing, but we can worry about that later.
Great, thanks. I should have some time to work on it around the middle of December, after I finish my current semester at school.
Created attachment 125367 [details] [review] working patch OK, here's a working patch. It's got one big issue though, which is that for download accounts, it will ask for the user's password twice. Basically, I added fields in the preferences dialog for the username and password, and when the catalogue is loading, it puts those in the playback URL. But for downloading (which still uses gnomevfs), the URL includes a redirect and gnomevfs refuses to use the authentication info stored in the URI object. So it always ended up with an access denied error. Eventually, I just added the line "gnome.ui.authentication_manager_init()" in MagnatuneSource.py, and now it pops up an auth dialog. This isn't ideal, but I couldn't make it work any other way, and the user will most likely select "remember forever" anyway, so it'll only appear once. I suspect the solution will eventually be to move the plugin to the GIO bindings when they're stable.
I'm OK with using the libgnomeui authentication stuff there for now, but I'd prefer it if the authentication_manager_init() call only happened if it was necessary (in the if download_account: block in __buy_album, I guess) It looks like it'll be a fair while before the gio bindings in pygobject provide enough API coverage, so I'm thinking of adding a simple downloader object (more complicated/featureful than the Loader thing, though) implemented in c to help get the python plugins off gnome-vfs.
That sounds fine. I was wondering though, is it OK for authentication_manager_init() to be called more than once? Or should the code be checking to see whether it's already been initialized by other code (and how would I do that)? A downloader object would definitely be appreciated. A lot of the changes I've made to the code in the past while seem to involve me fighting with gnomevfs, and I'll be happy to see that part of the plugin go :).
It looks like it's safe to call authentication_manager_init() repeatedly. Once I start working on the downloader thing (no telling when that will be), I'll post some information (API outlines, etc.) on bug 510392.
Created attachment 130454 [details] [review] updated for recent changes I haven't tested this at all, but I think at least everything is in the right place..
*** Bug 592514 has been marked as a duplicate of this bug. ***
I've made some changes to the plugin, and I think they're almost ready to be merged into master. My changes can be gotten from: git://github.com/AdamZ/rhythmbox-magnatune.git in the 'accounts' branch. For download accounts, there are 2 functions that seem to have been removed from the 'rb' module that the Magnatune plugin used: - rb.sanitize_uri_for_filesystem() - rb.uri_create_parent_dirs() The result of this is that the plugin downloads the files just fine, but throws an exception when it tries to unzip them. How can I add them back into the python bindings? Summary of changes: - enable streaming and (almost) downloading accounts - remove credit card purchases - Magnatune stopped being able to accept credit cards via their API last year[1]. Now, the plugin directs the user to a web page allowing them to purchase the album. - use a better name for in-progress files - redo the preferences window to reflect these changes - get rid of the purchase confirmation window, since it's no longer needed. [1] http://blogs.magnatune.com/buckman/2009/06/visa-no-more.html
I don't think those functions were ever added to the python bindings, which probably means the downloading code has been broken since the GIO port. It should be simple to add them, though.
Oh, and if you'd like to request a GNOME git account (see http://live.gnome.org/NewAccounts) so you can work on the magnatune plugin directly, I'd be happy to vouch for you.
I've added those missing functions to the python bindings in commit 38a0336.
OK, thanks, I've merged the changes in. I've tested it, and it seems to be working fine, so commits up to a1962ab should be ready to merge. There's one more commit in there (the last one) that I'd like a bit of feedback on. There's not much notification of the fact that the downloads have completed, other than the progress bar disappearing. So I added one using pynotify. There are a couple of things I'm not quite sure I did right though: - I call pynotify.init() in the source's __init__ method. Is that the right place to do it? - Does it matter if it gets called more than once (e.g., if another plugin wants to use notifications)? - Is there a rhythmbox-specific function that would be better to use, or is using pynotify directly the right way to go?
You could use rb.shell_notify_custom, which I just added to the python bindings. That has the advantage that it'll respect the user's preferences about if/when notifications should be shown, and the notifications will be attached to the status icon.
OK, done. I removed the previous commit, and replaced it with one that uses shell.notify_custom().
Great. I don't have any major comments about your code, it generally looks OK. It'd be nice to store the password in gnome-keyring rather than as plaintext in gconf. I also noticed the comments (that I probably wrote) in __download_finished() - we now depend on a new enough pygobject that we can use gio directly in python plugins, so I'd like to see that code rewritten to support any URI scheme, not just file://.
Not sure it belongs in that bug, but do provide a way for user to sign up for membership? 11:22 < nhn> and if you add a link to sign up for a membership, you get %10 of that members first payment
The strings passed to shell.notify_custom() need to be marked for translation, but otherwise I think this is all OK to commit to master.
OK, I marked the strings for translation and pushed my changes up to master. I'll keep this bug open while I work on the URI scheme issue you mentioned in comment 20. The keyring issue is part of bug 377354. Christophe, I didn't know that about accounts. I can definitely add a link in the preferences window.
I was going to ask you to include a 'magnatune:' prefix in your commit messages, but I see you've already started doing that. Thanks!
I've done some work on the URI scheme issue, available from my github repo, in the 'gio' branch. It works for my local library, although I don't have any non-local library locations to test it with. I think it's correct though. As part of the change, I moved the db.add_uri() call into the loop, so it gets called for each file, because the way I was doing it before (calling it once on the library path + the first directory of the zipfile entries) seemed hacky. Was there a (performance, maybe) reason we were doing this before? Does the recent commit 7fb07944 change that?
I'm not aware of any reason for doing it that way. Adding each of the tracks explicitly sounds like a better idea to me. The rest of the gio changes look OK too. Another possible improvement: do the unzip process in a separate thread, so it doesn't block the UI.
That actually crossed my mind. Are regular python threads enabled in rhythmbox now? Because I seem to remember they weren't allowed before. Or is there another way to do it, with async gio possibly?
Python threads are enabled now. Most rhythmbox internals only work when called on the main thread, though, so they're a bit limited. db.add_uri() should work from any thread, so it should be OK in this case. You could probably use async gio, with the gvfs acrhive:// backend to read the zip file.. but it'd be easier to just use a thread.
Fixed in commits 30ae537e and d6f27115.