After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 345783 - Support magnatune store.
Support magnatune store.
Status: RESOLVED FIXED
Product: rhythmbox
Classification: Other
Component: Plugins (other)
HEAD
Other Linux
: Normal enhancement
: ---
Assigned To: RhythmBox Maintainers
RhythmBox Maintainers
Depends on:
Blocks:
 
 
Reported: 2006-06-23 23:45 UTC by Alex Lancaster
Modified: 2006-11-11 01:09 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
File transferred from mailing list (14.70 KB, text/plain)
2006-06-23 23:46 UTC, Alex Lancaster
  Details
Updated code (17.89 KB, text/x-python)
2006-06-26 03:49 UTC, Adam Zimmerman
  Details
New code (24.38 KB, text/x-python)
2006-06-27 21:07 UTC, Adam Zimmerman
  Details
glade files (3.31 KB, application/x-gzip)
2006-06-27 21:08 UTC, Adam Zimmerman
  Details
Update (23.63 KB, text/x-python)
2006-06-28 20:15 UTC, Adam Zimmerman
  Details
Updated glade files (3.61 KB, application/x-gzip)
2006-06-28 20:17 UTC, Adam Zimmerman
  Details
updated patch (70.45 KB, patch)
2006-06-30 05:01 UTC, James "Doc" Livingston
none Details | Review
updated patch (70.45 KB, patch)
2006-06-30 05:07 UTC, James "Doc" Livingston
none Details | Review
Updates (6.44 KB, application/x-gzip)
2006-06-30 06:00 UTC, Adam Zimmerman
  Details
improved patch (74.58 KB, patch)
2006-07-06 14:39 UTC, James "Doc" Livingston
none Details | Review
patch for the patch (9.38 KB, patch)
2006-07-07 06:55 UTC, Adam Zimmerman
none Details | Review
update (16.29 KB, text/x-python)
2006-07-25 22:43 UTC, Adam Zimmerman
  Details
backtrace (17.40 KB, text/plain)
2006-07-25 22:47 UTC, Adam Zimmerman
  Details
backtrace (23.78 KB, text/plain)
2006-07-25 22:59 UTC, Adam Zimmerman
  Details
update (16.34 KB, text/x-python)
2006-07-28 14:48 UTC, Adam Zimmerman
  Details
fix in preferences code (5.56 KB, text/x-python)
2006-07-28 17:33 UTC, Adam Zimmerman
  Details
work-in-progress to update to async xfer (19.09 KB, text/x-python)
2006-08-07 15:37 UTC, Adam Zimmerman
  Details
combined patch (82.86 KB, patch)
2006-08-11 07:51 UTC, James "Doc" Livingston
none Details | Review
update (16.91 KB, text/x-python)
2006-08-25 20:22 UTC, Adam Zimmerman
  Details
multiple albums backtrace (10.33 KB, text/plain)
2006-08-25 20:23 UTC, Adam Zimmerman
  Details
updated files (11.87 KB, application/x-bzip)
2006-08-26 19:36 UTC, Adam Zimmerman
  Details
updates (16.54 KB, text/x-python)
2006-08-27 19:16 UTC, Adam Zimmerman
  Details
updates (17.85 KB, text/x-python)
2006-08-30 05:22 UTC, Adam Zimmerman
  Details
updated patch (82.74 KB, patch)
2006-09-02 17:03 UTC, Adam Zimmerman
none Details | Review
Images (2.17 KB, application/x-bzip)
2006-09-03 14:45 UTC, Adam Zimmerman
  Details
updated patch (82.37 KB, patch)
2006-09-03 21:49 UTC, Adam Zimmerman
none Details | Review
updated patch (82.39 KB, patch)
2006-09-04 16:33 UTC, Adam Zimmerman
none Details | Review
updated patch (90.67 KB, patch)
2006-09-05 02:22 UTC, Adam Zimmerman
none Details | Review
updated patch (82.28 KB, patch)
2006-09-09 11:50 UTC, Adam Zimmerman
none Details | Review
yet another updated patch (93.76 KB, patch)
2006-09-11 01:56 UTC, Adam Zimmerman
none Details | Review
updates (97.78 KB, patch)
2006-09-12 05:00 UTC, Adam Zimmerman
committed Details | Review
Images (2.95 KB, application/x-bzip)
2006-09-12 05:03 UTC, Adam Zimmerman
  Details
quick fix (888 bytes, patch)
2006-10-06 18:38 UTC, Adam Zimmerman
none Details | Review
Partial toolbar button goodness (4.96 KB, patch)
2006-10-22 18:22 UTC, Adam Zimmerman
none Details | Review
Closer to proper toolbar goodness (5.06 KB, patch)
2006-11-05 19:35 UTC, Adam Zimmerman
none Details | Review
more work (5.11 KB, patch)
2006-11-06 09:00 UTC, James "Doc" Livingston
none Details | Review
icon fix (5.29 KB, patch)
2006-11-11 00:52 UTC, Adam Zimmerman
committed Details | Review

Description Alex Lancaster 2006-06-23 23:45:28 UTC
Adam Zimmerman has started writing a plugin for Magnatune catalog/purchasing plugin, see:

http://mail.gnome.org/archives/rhythmbox-devel/2006-June/thread.html#2

I am transferring the patch here.
Comment 1 Alex Lancaster 2006-06-23 23:46:27 UTC
Created attachment 67915 [details]
File transferred from mailing list
Comment 2 James "Doc" Livingston 2006-06-24 00:50:23 UTC
Actually adding Adam as a CC.
Comment 3 Alex Lancaster 2006-06-24 03:43:32 UTC
(In reply to comment #2)
> Actually adding Adam as a CC.

Thanks, I wasn't sure that he had a bugzilla account. 

Comment 4 Adam Zimmerman 2006-06-26 03:49:17 UTC
Created attachment 68004 [details]
Updated code

> Take a look at plugins/pythonconsole/pythonconsole.py, it adds a menu
> item to show the console. The only real difference is the UI data, e.g.:
> 
> <ui>
>   <popup name="MagnatuneSourceViewPopup">
>     <menuitem name="AddToQueueLibraryPopup" action="AddToQueue"/>
>     <separator/>
>     <menuitem name="MagnaTuneDownloadPopup" action="MagnaTuneDownload"/>
>     <separator/>
>     <menuitem name="BrowseGenreLibraryPopup"
> action="BrowserSrcChooseGenre"/>
>     <menuitem name="BrowseArtistLibraryPopup"
> action="BrowserSrcChooseArtist"/>
>     <menuitem name="BrowseAlbumLibraryPopup"
> action="BrowserSrcChooseAlbum"/>
>     <separator/>
>     <menuitem name="PropertiesLibraryPopup" action="MusicProperties"/>
>   </popup>
> </ui>
> 
> 
> Then add an action "MagnaTuneDownload".

OK, I've tried that, but run into a problem. I can't seem to choose which popup to show, as source.show_popup() complains that it doesn't take any arguments when I try to pass it the name of the popup. Renaming the popup to "BrowserSourceViewPopup" (as you had it in your other email) makes it work, except then the "Download Album" action appears for every source. Am I doing something wrong?
Comment 5 Adam Zimmerman 2006-06-27 21:07:00 UTC
Created attachment 68084 [details]
New code

Purchasing now fully works, as far as I can tell. The popup issue still needs to be resolved, but that's about it in terms of basic getting-it-to-work stuff.

Is there a directory other than /usr/share/rhythmbox/glade where I can put the glade files? ~/.gnome2/rhythmbox/glade didn't seem to work
Comment 6 Adam Zimmerman 2006-06-27 21:08:55 UTC
Created attachment 68085 [details]
glade files

glade files for the preferences window, and confirmation window.
Comment 7 Adam Zimmerman 2006-06-28 20:15:49 UTC
Created attachment 68132 [details]
Update

Updated code. Everything seems to work. Changes:

- made download progress window a glade file
- slight glade fixes (visibility, position)
- show an info dialog before the UI blocks on purchase
- glade file locations aren't hardcoded anymore

(btw, I'll be away for the first 3 or so weeks of July, likely with little internet access)
Comment 8 Adam Zimmerman 2006-06-28 20:17:07 UTC
Created attachment 68133 [details]
Updated glade files

The glade files need to go in /usr/share/rhythmbox/glade/ (or wherever sys.prefix points, if not /usr)
Comment 9 Florian Zeitz 2006-06-29 21:48:58 UTC
For some reason if song_info.xml is not already available the following exception is thrown:
Traceback (most recent call last):
  • File "/home/florian/devel_bin//lib/rhythmbox/plugins/magnatune.py", line 143 in activate
    if check_info():
  • File "/home/florian/devel_bin//lib/rhythmbox/plugins/magnatune.py", line 451 in check_info
    modified = str(gnomevfs.get_file_info(magnatune_song_info_uri).mtime)
ValueError: ntime field has no valid value

The rest is really working great. Good work.
Comment 10 Adam Zimmerman 2006-06-30 00:00:08 UTC
(In reply to comment #9)
> For some reason if song_info.xml is not already available the following
> exception is thrown:
> Traceback (most recent call last):
>   File "/home/florian/devel_bin//lib/rhythmbox/plugins/magnatune.py", line 143,
> in activate
>     if check_info():
>   File "/home/florian/devel_bin//lib/rhythmbox/plugins/magnatune.py", line 451,
> in check_info
>     modified = str(gnomevfs.get_file_info(magnatune_song_info_uri).mtime)
> ValueError: ntime field has no valid value
> 
> The rest is really working great. Good work.
> 

Hm, I'm somewhat stumped. I can't reproduce this problem. Deleting the local file and reloading the plugin with and without an internet connection doesn't do it (although, without an internet connection it throws a different exception(gnomevfs.HostNotFoundError), which I should probably handle properly).

I can't really think of what would be causing it either, except maybe a once-off weirdness with the magnatune server not including the last-modified time for the file. That line of code only checks the mtime of the remote file, and it gets run every time, regardless of the state of the local song_info.xml file. So you should be seeing it every time you load the plugin.

Is there any more info you can give as to how I can reproduce the bug?
Comment 11 James "Doc" Livingston 2006-06-30 05:01:16 UTC
Created attachment 68196 [details] [review]
updated patch

I've just fixed the popup menu stuff in cvs.

This version is a patch against cvs, which putting all the stuff in the plugins/magnatune directory. It can be run uninstalled if --enable-uninstalled-build was passed to configure, or run installed. It works perfectly for me, aside from the purchase code which I haven't tested.

I've also broken the classes out into their own .py files, although a lot of the stuff that is in the main file (__init__.py) should be pushed down to the source or something.


Finding data files in the per-user plugin directory isn't something that works yet. I'll try to get it done soon, but it will probably be a "find_file" method of the plugin object, which works like rb_file but also looks in ~/.gnome2/rhythmbox/plugins/PLUGIN_NAME/
Comment 12 James "Doc" Livingston 2006-06-30 05:07:20 UTC
Created attachment 68197 [details] [review]
updated patch

As a clarification to the above, bits using glade (purchasing) stuff only works when it's installed, because of the above "find plugin data files" thing.
Comment 13 Adam Zimmerman 2006-06-30 05:30:54 UTC
plugins/magnatune/magnatune/Makefile.am seems to be missing, and ./autogen.sh complains. Copying/modifying the one from the artdisplay plugin seems to work.
Comment 14 Adam Zimmerman 2006-06-30 06:00:28 UTC
Created attachment 68199 [details]
Updates

Changes:

- include Makefile.am for plugins/magnatune/magnatune
- Add "Artist Information" action
Comment 15 Florian Zeitz 2006-06-30 09:01:12 UTC
(In reply to comment #10)
> Hm, I'm somewhat stumped. I can't reproduce this problem. Deleting the local
> file and reloading the plugin with and without an internet connection doesn't
> do it (although, without an internet connection it throws a different
> exception(gnomevfs.HostNotFoundError), which I should probably handle
> properly).
> 
> I can't really think of what would be causing it either, except maybe a
> once-off weirdness with the magnatune server not including the last-modified
> time for the file. That line of code only checks the mtime of the remote file,
> and it gets run every time, regardless of the state of the local song_info.xml
> file. So you should be seeing it every time you load the plugin.
> 
> Is there any more info you can give as to how I can reproduce the bug?
> 

Well, it just happened, when I tried to load the plugin. Right now I think it might also be caused by me being too stupid. Basically I just copyed the .py to lib/rhythmbox/plugins and created a .rb-plugin for it (which I suppose is not the right way to do it). I also hardcoded the place for the .glade files.
What got me really wondering is, that I understood the code as song_info.xml going in ~/rhythmbox/magnatune (and placing it there curred the exception) but later I found this file (and the timestamp) in ~/.gnome2/rhythmbox/magnatune which is probably what the code really says.

I'll see if I can reproduce it again using your newest code and doing less stupid things...
Comment 16 Adam Zimmerman 2006-06-30 14:50:44 UTC
(In reply to comment #15)
> Well, it just happened, when I tried to load the plugin. Right now I think it
> might also be caused by me being too stupid. Basically I just copyed the .py to
> lib/rhythmbox/plugins and created a .rb-plugin for it (which I suppose is not
> the right way to do it).

That's just fine, it doesn't really matter how python plugins are installed, and that's basically all I did when I started writing it.

> I also hardcoded the place for the .glade files.

Again, shouldn't affect anything. Missing glade files don't even cause a problem until you try to load the preferences or buy an album.

> What got me really wondering is, that I understood the code as song_info.xml
> going in ~/rhythmbox/magnatune (and placing it there curred the exception) but
> later I found this file (and the timestamp) in ~/.gnome2/rhythmbox/magnatune
> which is probably what the code really says.

Yeah. The line that threw the exception was the one that gets the last-changed timestamp from the server. It doesn't even affect the local file. If you found the file/timestamp later, it means that sometime after that (likely the next time you reloaded the plugin) it worked, and properly downloaded.

(and yes, the "~.gnome2" part of the path gets added at the beginning, when the user_dir variable is set.)

And downloading the file manually wouldn't have actually helped anyway, since it checks the timestamp from the server against the local timestamp every time before doing anything else. If the timestamp file didn't exist, the plugin would redownload song_info.xml anyway (it would think it had never done it before).

> 
> I'll see if I can reproduce it again using your newest code and doing less
> stupid things...
> 

From what you're saying, it basically sounds like a random onetime glitch instead of something you did. No stupidity involved on your part :).
Comment 17 Florian Zeitz 2006-06-30 21:00:30 UTC
Well, I just tried it again and I can still relyably reproduce this Exception.
The behaviour has chaged a bit though:
I can't cure it by putting something in ~/rhythmbox/magnatune (makes sense).
I can cure it by setting modified to some value smaller than the actual mtime instead of getting it (this wasn't possible when I tried it the last time. The exception was just thrown in the next line).
The Exception looks a bit different (makes sense, too)
Traceback (most recent call last):
  • File "/home/florian/devel_bin/lib/rhythmbox/plugins/magnatune/__init__.py", line 105 in activate
    if check_info():
  • File "/home/florian/devel_bin/lib/rhythmbox/plugins/magnatune/__init__.py", line 444 in check_info
    modified = str(gnomevfs.get_file_info(magnatune_song_info_uri).mtime)
ValueError: ntime field has no valid value

Comment 18 Adam Zimmerman 2006-06-30 21:42:37 UTC
(In reply to comment #17)
> Traceback (most recent call last):
>   File "/home/florian/devel_bin/lib/rhythmbox/plugins/magnatune/__init__.py",
> line 105, in activate
>     if check_info():
>   File "/home/florian/devel_bin/lib/rhythmbox/plugins/magnatune/__init__.py",
> line 444, in check_info
>     modified = str(gnomevfs.get_file_info(magnatune_song_info_uri).mtime)
> ValueError: ntime field has no valid value
> 

Weird. I just noticed that it's complaining about "ntime" and not "mtime" (which is what's gotten). I haven't been able to find anything in the gnomevfs documentation (or google) about a field called ntime, or anything with that message.

> I can cure it by setting modified to some value smaller than the actual mtime
> instead of getting it

It just does a string comparison, so if the mtimes are different, it'll download the file again, and if they're the same, it'll just load the file from disk.

> (this wasn't possible when I tried it the last time. The
> exception was just thrown in the next line).

That's interesting. Was it the same exception (about ntime)? The next line calls a different function on a different (local) URI. So that would suggest that maybe the bug has something to do with gnomevfs in general, instead of just the FileInfo.mtime field.

Try splitting the line up into separate ones, so you can better determine exactly which call is throwing the exception. Something like:

file_info = gnomevfs.get_file_info(magnatune_song_info_uri)
mod_time = file_info.mtime
modified = str(mod_time)
Comment 19 Florian Zeitz 2006-06-30 22:12:20 UTC
(In reply to comment #18)
> Weird. I just noticed that it's complaining about "ntime" and not "mtime"
> (which is what's gotten). I haven't been able to find anything in the gnomevfs
> documentation (or google) about a field called ntime, or anything with that
> message.
> 
Yes I noticed that too. I really wonder where this is comming from...

> That's interesting. Was it the same exception (about ntime)? The next line
> calls a different function on a different (local) URI. So that would suggest
> that maybe the bug has something to do with gnomevfs in general, instead of
> just the FileInfo.mtime field.
> 
Yes it was the same Exception, but as I don't get this behaviour anymore this fact is not really useful...

> Try splitting the line up into separate ones, so you can better determine
> exactly which call is throwing the exception. Something like:
> 
> file_info = gnomevfs.get_file_info(magnatune_song_info_uri)
> mod_time = file_info.mtime
> modified = str(mod_time)
> 

It produces the Exception when doing mod_time = file_info.mtime
BTW the same lines work just fine in a normal python console (havin set magnatune_song_info_uri of course), so there must be some relation to the surrounding code.
Comment 20 Adam Zimmerman 2006-06-30 22:43:28 UTC
(In reply to comment #19)
> It produces the Exception when doing mod_time = file_info.mtime
> BTW the same lines work just fine in a normal python console (havin set
> magnatune_song_info_uri of course), so there must be some relation to the
> surrounding code.

Or maybe some relation to the fact that it's being run by rhythmbox? Have you tried running it in rhythmbox's python console?

I can't think of what's going on here. FileInfo.mtime is just a class variable, and so I would think that it's just sitting there in memory, and that the ntime field is something else entirely. What distro and versions of python/gnomevfs/rhythmbox do you have? It could be a bug in one of those.

Has anyone else watching this bug seen the ntime exception, or have any ideas as to what might be causing it?
Comment 21 Florian Zeitz 2006-06-30 22:57:35 UTC
Tada.
Yes actually the same commands do produce the error in rhytmbox's python interpreter:
[quote]
You can access the main window through the 'shell' variable :
<rb.Shell object (RBShell) at 0xb53d7324>
>>> import gnomevfs
>>> uri = gnomevfs.URI("http://magnatune.com/info/song_info.xml")
>>> file_info = gnomevfs.get_file_info(uri)
>>> mod_time = file_info.mtime
Traceback (most recent call last):
  • File "/home/florian/devel_bin/lib/rhythmbox/plugins/pythonconsole.py", line 340 in __run
    exec command in self.namespace
  • File "<string>", line 1 in ?
ValueError: ntime field has no valid value
[/quote]

I'm using Ubuntu 6.06 LTS,
rhythmbox from CVS,
Python 2.4.3 and
gnomevfs 2.14.2 (at least that is what the ubuntu-package claims)
Comment 22 Adam Zimmerman 2006-06-30 23:22:36 UTC
Hm, I've got the same distro/versions, so I doubt that's the issue. I wonder if it's the fact that you've got rhythmbox installed in your home directoy (I'm kind of grasping at straws here, running out of ideas). I also thought of stuff you could do to get some more info, but you replied before I had the time to submit it. I don't know if it will still be of any use, but here it is just in case:

Another thing you could try doing to test is to print out the FileInfo object
before you read the mtime, and verify that it's actually a proper FileInfo
object. Also, try printing out some of the other attributes[1] of the FileInfo
object and see if they work. For reference, here are the results I got when I
tried printing out a bunch of the attributes.

print info: <gnomevfs.FileInfo 'song_info.xml'>
print info.atime: 1151705317
print info.mime_type: text/xml
print info.mtime: 1151353500
print info.name: song_info.xml
print info.size: 6213687
print info.type: 1
print info.valid_fields: 9793

Interestingly, when I tried to get the "permissions" (among others) attriibute,
I got your exception (ValueError: permissions field has no valid value). Which
makes sense, since a file accessed over http has no permission information.
However, when I tried to print info.ntime, I got "AttributeError:
'gnomevfs.FileInfo' object has no attribute 'ntime'". I'm rather confused at
the moment.

[1] http://www.pygtk.org/pygnomevfs/class-gnomevfs-fileinfo.html
Comment 23 Florian Zeitz 2006-06-30 23:34:34 UTC
>>> info.atime
Traceback (most recent call last):
  • File "/home/florian/devel_bin/lib/rhythmbox/plugins/pythonconsole.py", line 336 in __run
    r = eval(command, self.namespace, self.namespace)
  • File "<string>", line 0 in ?
ValueError: atime field has no valid value
>>> info.valid_fields
8257
>>> info.mtime
Traceback (most recent call last):
  File "/home/florian/devel_bin/lib/rhythmbox/plugins/pythonconsole.py", line 336, in __run
    r = eval(command, self.namespace, self.namespace)
  File "<string>", line 0, in ?
ValueError: ntime field has no valid value

The rest is as expected.
And just imagine what I am if you are already confused ;)
Comment 24 Adam Zimmerman 2006-06-30 23:48:11 UTC
With regards to the valid_fields field:

ME:
print info.valid_fields: 9793

YOU:
>>> info.valid_fields
8257

MAGIC:
>>> gnomevfs.FILE_INFO_FIELDS_MTIME | gnomevfs.FILE_INFO_FIELDS_ATIME
1536
>>> 9793-8257
1536
>>> 

Which almost makes sense, in that sort of completely insane way. So your FileInfo object 'knows' that those fields aren't valid (though in the case of mtime it persists in calling it ntime), but I can't see a reason why you shouldn't be able to get the mtime or atime of a file while I can. And it only happens in rhythmbox, not in "regular" python. I'm going to file a separate bug for this issue, since by now I'm pretty sure it has nothing specifically to do with my plugin.
Comment 25 James "Doc" Livingston 2006-07-06 14:39:03 UTC
Created attachment 68475 [details] [review]
improved patch

This is a fairly big overhaul of some of the code, and a lot has moved around. I haven't finished doing the purchasing code, so it may not work; but I though I'd chuck the patch up in case people wanted it look, I'll finish that bit soon

Notable changes:
* use new plugin stuff from cvs: plugin.find_file() for locating data files
* much code moved down to MagnatuneSource, although the catalogue code and purchase code should probably be split out further
* Artist info works correctly with multiple tracks selected
* catalogue is only loaded when the source is activated, which improves startup time and saved memory for when you don't look at it
* catalogue loading and download code rewritten. Sucks less, fully asynchronous, updates don't overwrite existing xml file until completely downloaded, doesn't have mtime issues, uses mtime of the local copy rather then extra file.
* more UI now in glade files
* status bar shows catalogue update progress
* use gnomevfs's async xfer function for moving bulk data
* gconf stuff nicer
* general code cleanup

Still to do:
* all the purchasing code below the "### FIXME..." comment
* purchases with multiple selections
* put details of gconf format in one place
* many really long lines that should be clean up
* probably standard python idioms I haven't used
* extract purchased in the user's library layout, not just dump them in the library location
* plenty more
Comment 26 Adam Zimmerman 2006-07-07 06:55:21 UTC
Created attachment 68528 [details] [review]
patch for the patch

(this patch is probably not done properly. It's meant to be applied to the plugins/magnatune directory after the previous patch has been applied. If anyone could point me to documentation to help me make proper patches, it would be much appreciated).

Changes:
- fixed some typos
- now works properly when the local song info file doesn't exist
- purchasing code should work

My internet connection is too unreliable at the moment to fully test purchasing, but it seems to work up to the point the connection fails.
Comment 27 Adam Zimmerman 2006-07-25 22:43:17 UTC
Created attachment 69615 [details]
update

This update allows downloading of multiple albums (the UI is kinda sucky still, it just shows a sequence of purchase dialogs), which also happens to fix a bug that would have occurred if you purchased an album while another was still downloading.
Comment 28 Adam Zimmerman 2006-07-25 22:47:53 UTC
Created attachment 69616 [details]
backtrace

After downloading the album and unzipping it, the line (426):
self.__db.add_uri("file://" + urllib.quote(track_uri.dirname))
causes rhythmbox to freeze. It prints:
*** glibc detected *** malloc(): memory corruption: 0xb708d3bf ***
Comment 29 Adam Zimmerman 2006-07-25 22:59:49 UTC
Created attachment 69619 [details]
backtrace

sorry, this is a proper backtrace from when the problem actually occurs.
Comment 30 Adam Zimmerman 2006-07-28 14:48:27 UTC
Created attachment 69832 [details]
update

Fixes status message, now "Loading Magnatune Catalog" is not displayed after it's done loading.
Comment 31 Adam Zimmerman 2006-07-28 17:33:36 UTC
Created attachment 69840 [details]
fix in preferences code

Fix pay combobox in the preferences window so it doesn't change every time you open the preferences.
Comment 32 James "Doc" Livingston 2006-08-07 05:14:11 UTC
Looks good to me. Had you heard back on whether it was possible to have a compressed catalogue xml file?
Comment 33 Adam Zimmerman 2006-08-07 15:37:03 UTC
Created attachment 70399 [details]
work-in-progress to update to async xfer

I haven't heard back yet. I was planning on emailing John after I'd updated the purchasing code to use async_xfer.

I'm now trying to use gtk.gdk.threads_enter/leave for updating the progress window, since the idle calls were making rhythmbox crash[1]. That bit works now, but I'm getting a different problem. I'm using a dictionary called self.__downloads to track the amount of data downloaded in each transfer (I'd rather not have multiple progress windows popping up, so I'm trying to keep everything in one).

It works fine if a single album is downloaded completely. However, if the cancel button is pressed, it complains that the dictionary is empty, and throws an exception (self.purchase_filesize is also 0, as I found out when moving the if block to the end of the function). I suspect it may have something to do with threads, but my knowledge of gnomevfs (and threads/thread safety in general) is not good enough to be sure, or to know how to fix it.

Trying to download multiple albums at the same time crashes rhythmbox, but I'll look at that after this problem's been solved.
Comment 34 James "Doc" Livingston 2006-08-11 07:51:22 UTC
Created attachment 70696 [details] [review]
combined patch

This patch combines all the updates sitting on the bug, and a few improvements to the glade files (access keys, spacing, label access targets, etc).

Some other notes/ideas:

CC-related
* expiry month and year must be zero-padded, having "1" in the month didn't work
* expiring fields should use a gtk.SpinButton, or possible ComboBox for the month
* maybe give warning about CC expiry in the past

logo
* logo as png instead of inline
* logo in purchase dialog
* logo on "info" screen

purchasing
* after clicking "purchase" UI blocks until download-progress window appears
* warn before quitting with purchase running, or when pressing cancel
* stick "in progress" purchase info somewhere, and offer to resume if RB quits/crashes. Maybe in ~/.gnome2/rhythmbox/magnatune?
* allowing to pause/resume the download would also be handy
* display download rate (Kb/s) ?
* extracting zip file doesn't put them in the right (base on user prefs) location. Probably best would be to use RBEncoder to transfer - but this may require extracting a file to somewhere (/tmp/ ?), using RBEncoder to copy, deleting temp, and repeating for the rest.

misc
* use gconf key to store entryview sort order
* make download-progress window a transient of the main one
Comment 35 Adam Zimmerman 2006-08-25 20:22:46 UTC
Created attachment 71616 [details]
update

Cancelling a download now works. Cleaned out the old download code. Downloading more than one album at a time still crashes rhythmbox.
Comment 36 Adam Zimmerman 2006-08-25 20:23:51 UTC
Created attachment 71617 [details]
multiple albums backtrace

backtrace from a crash when trying to download multiple albums at once. Not sure what's causing this.
Comment 37 Adam Zimmerman 2006-08-26 19:36:34 UTC
Created attachment 71674 [details]
updated files

Updates. Fixes a few things:

- cc-month entry in prefs is now a ComboBox
- logo(s) are png now. magnatune_circle_small.png goes in $prefix/share/rhythmbox/art/ (or wherever, the code uses find_file to find it), magnatune_logo_color_small.png goes in the directory with the glade files (don't know how to set it to look anywhere else)
- logos in prefs dialog and loading screen
- "Magnatune", not "MagnaTune" in loading screen
- Added James to "Authors" in .rb-plugin file since you contributed a lot of code (and helped a lot in general)
Comment 38 Adam Zimmerman 2006-08-27 19:16:08 UTC
Created attachment 71717 [details]
updates

Fixes:

- UI no longer blocks after hitting purchase button
- move the temp song info catalogue instead of copying it
- close the progress window properly when done downloading an album
Comment 39 Adam Zimmerman 2006-08-30 05:22:04 UTC
Created attachment 71884 [details]
updates

Updates:

- sort order is remembered in gconf
- if rhythmbox quits while an album is being downloaded, the download is restarted next time the Magnatune source is clicked (not resumed though, I couldn't see how to make async.xfer do that).

I also have a question: I've been looking through the python bindings in the console, and I can't find RBEncoder or anything that looks like it (for putting albums in the proper directory structure). Am I missing it, or is there another module I need to import, or is it not exposed to python plugins yet?
Comment 40 Jonathan Matthew 2006-09-01 10:31:35 UTC
I don't think gnomevfs can resume downloads.  I'd be happy to be proven wrong, of course..

We haven't exposed RBEncoder through the python bindings yet.  It doesn't do anything related to the library structure, though.  That's internal to the library source at the moment.  I'm not sure whether it should be exposed as-is or moved somewhere else first.
Comment 41 Adam Zimmerman 2006-09-01 16:04:22 UTC
OK, I was looking through rb-library-source.c, and as I understand it, using track-transfer at the moment requires copying/pasting from another source. That probably won't work in this case, since the downloaded files are not part of any source.

I don't really feel like re-implementing filepath_parse_pattern() and all the other functions I'd need to move the files myself. It seems like it would be a bit of a waste.

What would probably be cleanest for me is for some generic code to be written that moves files into the proper library path, and that function exposed to python. Then I could just call librarySource.transfer_files(uri) (or wherever the code is put). Is that possible?
Comment 42 Alex Lancaster 2006-09-02 11:47:24 UTC
(In reply to comment #39)
> Created an attachment (id=71884) [edit]
> updates
 
Adam, could you perhaps make your updates as patches and obsolete the old patches?  It's difficult to test because you have to figure out which of the old patches to first apply and then figure out which file from the original file you need to overwrite with the new file.

If you continuously update the patch, then you just need to get the latest patch and apply it.
Comment 43 Alex Lancaster 2006-09-02 11:53:43 UTC
To make a patch against CVS do this (from the top-level rhythmbox source):

cvs diff -u > name-of-patch.patch

if you need to add files (which don't exist in CVS) to the patch, do this for each file:

diff -u /dev/null plugins/foobar.py >> name-of-patch.patch
Comment 44 Adam Zimmerman 2006-09-02 17:03:01 UTC
Created attachment 72086 [details] [review]
updated patch

(In reply to comment #43)
> if you need to add files (which don't exist in CVS) to the patch, do this for
> each file:
> 
> diff -u /dev/null plugins/foobar.py >> name-of-patch.patch
> 

Ah, that's the bit I couldn't figure out before. What about the pngs? diff doesn't want to show them.
Comment 45 Alex Lancaster 2006-09-03 11:34:29 UTC
(In reply to comment #44)

> Ah, that's the bit I couldn't figure out before. What about the pngs? diff
> doesn't want to show them.

Yeah, I'm not sure you can add binary files using diff.  You'll probably just have to attach those separately and note where they should be added in the source. 

Since those don't normally change with a patch revision it's less of a hassle.

Comment 46 Adam Zimmerman 2006-09-03 14:45:26 UTC
Created attachment 72133 [details]
Images

magnatune_circle_small.png - data/art/
magnatune_logo_color_small.png - plugins/magnatune/

I also can't see how to obsolete James' old patch. Is it a developer/regular user thing, or can I only obsolete my own attachments?
Comment 47 Adam Zimmerman 2006-09-03 21:49:00 UTC
Created attachment 72159 [details] [review]
updated patch

The song info is now available as a zip file, so this update uses that.
Comment 48 Alex Lancaster 2006-09-04 08:25:24 UTC
(In reply to comment #46)

> I also can't see how to obsolete James' old patch. Is it a developer/regular
> user thing, or can I only obsolete my own attachments?

Yeah, you probably need to have the appropriate bugzilla permissions to edit all aspects of a bug.  Obsoleting the patch myself.

Comment 49 Alex Lancaster 2006-09-04 08:40:46 UTC
(In reply to comment #47)
> Created an attachment (id=72159) [edit]
> updated patch

OK, just tested patch with the dummy id number you sent me.  I can browse and play tracks just fine, but I got a crash when attempted to purchase music (I chose the FLAC format).  I got an message saying "authorizing purchase" and then rhythmbox crashed with:

$ rhythmbox

sys:1: Warning: g_date_set_julian: assertion `g_date_valid_julian (j)' failed
sys:1: Warning: g_date_get_year: assertion `g_date_valid (d)' failed
sys:1: Warning: g_date_set_dmy: assertion `g_date_valid_dmy (day, m, y)' failed
sys:1: Warning: g_date_get_julian: assertion `g_date_valid (d)' failed
Traceback (most recent call last):
  • File "/usr/local//lib/rhythmbox/plugins/magnatune/__init__.py", line 129 in create_configure_dialog
    gladexml.get_widget("mm_entry").set_active(int(self.client.get_string(self.gconf_keys['ccmonth'])) - 1)
ValueError: invalid literal for int():

(rhythmbox:4730): Rhythmbox-CRITICAL **: rb_plugins_engine_configure_plugin: assertion `conf_dlg != NULL' failed
/home/alex/bin/rhythmbox: line 2:  4730 Segmentation fault      

I will try and to attach a backtrace if needed.
Comment 50 Alex Lancaster 2006-09-04 09:04:48 UTC
OK, so on my second attempt the download started, but this time I chose VBR mp3s.  I also tried a second time with another album with FLAC it worked.  I can't quite duplicate the crash.  Perhaps it was some configuration element that needed to be created once.

In any case, it now seems to work just fine.  Great work!  Some minor comments (which could be done after checking into CVS):

* The magnatune icon in the source list is a little bit too large compared with the other icons.  It also looks somewhat bitmapped and ragged.  Also when it selected the icon goes dark because the white appears to be transparent, so it turns into a dark circle.

* The download message title should perhaps be a little more descriptive: e.g. "Downloading purchased Magnatune music" or somesuch.  Actually it would be better if the progress bar (used for transfers and CD ripping progress) at the bottom was used, rather than a separate dialog.

* It might be nice to have the status bar show the total number of songs like the Library, but place of size (20 GB) indicate the number of artists and genres e.g.:  "6297 songs, 20 days, 13 mins and 5 seconds, 17 genres, 845 artists, 285 albums".  Or something like that.
Comment 51 Adam Zimmerman 2006-09-04 16:33:26 UTC
Created attachment 72210 [details] [review]
updated patch

(In reply to comment #50)
> OK, so on my second attempt the download started, but this time I chose VBR
> mp3s.  I also tried a second time with another album with FLAC it worked.  I
> can't quite duplicate the crash.  Perhaps it was some configuration element
> that needed to be created once.
 
Damn, I thought I created all the gconf keys properly, but I forgot to set the format key. That's fixed, and won't happen anymore. (Incidentally, I've noticed a bug like this in the rb prefs when you select an update interval for podcasts, where it won't check for new podcasts unless you select a different interval and then [if you want] go back to 1 hour)

> In any case, it now seems to work just fine.  Great work!  Some minor comments
> (which could be done after checking into CVS):
> 
> * The magnatune icon in the source list is a little bit too large compared with
> the other icons.  It also looks somewhat bitmapped and ragged.  Also when it
> selected the icon goes dark because the white appears to be transparent, so it
> turns into a dark circle.

Yeah, my art skills are definitely nonexistent. I just used part of a logo from [1] and scaled it to 32x32. If I feel inspired I'll see if I can make a better one.

[1] http://www.magnatune.com/info/press/logos/

> 
> * The download message title should perhaps be a little more descriptive: e.g.
> "Downloading purchased Magnatune music" or somesuch.

Done.

> Actually it would be
> better if the progress bar (used for transfers and CD ripping progress) at the
> bottom was used, rather than a separate dialog.

Todo, once I have a bit of time (I'm about to start school, so I'm slightly disorganised :)

> 
> * It might be nice to have the status bar show the total number of songs like
> the Library, but place of size (20 GB) indicate the number of artists and
> genres e.g.:  "6297 songs, 20 days, 13 mins and 5 seconds, 17 genres, 845
> artists, 285 albums".  Or something like that.
> 

Yeah. At one point I think it did automatically, though I'm not sure what I changed to make that disappear. I suspect it was when the status bar started to be used to let the user know that it was loading. Once it blanked, there was no more "number of artists" text. I'll put that on my todo list as well. I would get the info on total time/genres/artists/albums from rhythmdb somehow I assume?
Comment 52 Jonathan Matthew 2006-09-04 23:25:40 UTC
(In reply to comment #51)
> Created an attachment (id=72210) [edit]
> updated patch
> 
> (In reply to comment #50)
> > OK, so on my second attempt the download started, but this time I chose VBR
> > mp3s.  I also tried a second time with another album with FLAC it worked.  I
> > can't quite duplicate the crash.  Perhaps it was some configuration element
> > that needed to be created once.
> 
> Damn, I thought I created all the gconf keys properly, but I forgot to set the
> format key. That's fixed, and won't happen anymore. 

gconf keys should be created in the .schemas file (data/rhythmbox.schemas) rather than in code.


> > * It might be nice to have the status bar show the total number of songs like
> > the Library, but place of size (20 GB) indicate the number of artists and
> > genres e.g.:  "6297 songs, 20 days, 13 mins and 5 seconds, 17 genres, 845
> > artists, 285 albums".  Or something like that.
> > 
> 
> Yeah. At one point I think it did automatically, though I'm not sure what I
> changed to make that disappear. I suspect it was when the status bar started to
> be used to let the user know that it was loading. Once it blanked, there was no
> more "number of artists" text. I'll put that on my todo list as well. I would
> get the info on total time/genres/artists/albums from rhythmdb somehow I
> assume?

Something like this should work (in MagnatuneSource.do_impl_get_status):
  qm = self.get_property("query-model")
  return (qm.compute_status_normal("song", "songs"), None, 0.0)
 
but it doesn't seem to work properly at the moment ("songs, 18 days, 12 hours and 22 minutes").  I'll look into this more later.

I'm slightly concerned about storing credit card numbers in gconf, but I'm not sure what else to do.  Maybe an option to ask for the card details at every purchase?
Comment 53 Adam Zimmerman 2006-09-05 02:22:18 UTC
Created attachment 72228 [details] [review]
updated patch

(In reply to comment #52)
> gconf keys should be created in the .schemas file (data/rhythmbox.schemas)
> rather than in code.

Done.

> Something like this should work (in MagnatuneSource.do_impl_get_status):
>   qm = self.get_property("query-model")
>   return (qm.compute_status_normal("song", "songs"), None, 0.0)
> 
> but it doesn't seem to work properly at the moment ("songs, 18 days, 12 hours
> and 22 minutes").  I'll look into this more later.
> 

That code's been added to the method, and I get the same results.

> I'm slightly concerned about storing credit card numbers in gconf, but I'm not
> sure what else to do.  Maybe an option to ask for the card details at every
> purchase?

That was an idea I had back when I started doing this, for the same reason. I'll get around to it at some point, but right now it's not a huge priority for me.
Comment 54 Alex Lancaster 2006-09-06 00:57:42 UTC
(In reply to comment #53)
> Created an attachment (id=72228) [edit]
> updated patch

> > Something like this should work (in MagnatuneSource.do_impl_get_status):
> >   qm = self.get_property("query-model")
> >   return (qm.compute_status_normal("song", "songs"), None, 0.0)
> > 
> > but it doesn't seem to work properly at the moment ("songs, 18 days, 12 hours
> > and 22 minutes").  I'll look into this more later.
> > 
> 
> That code's been added to the method, and I get the same results.

Same for me.  At least it shows the timing information.

Other issues with the updated patch:

When I abort rhythmbox during an album purchase/download, it writes a ~/.gnome2/magnatune/downloads_in_progress file with the location of the file to be redownloaded.  All well and good, except:

(1) when I restart rhythmbox and then open the Magnatune source, the UI hangs and rhythmbox has to be killed.  The plugin never offers, nor does it actually start, to redownload the aborted purchase. If I remove or rename the downloads_in_progress file, the plugin starts again fine.

(2) if rhythmbox crashes during a download, the downloads_in_progress file never gets written.  Perhaps it should be written as soon as the download starts, or just before the download starts so that crashes don't lose the info.  (Obviously this will only work if (1) is fixed).

Lastly another patch niggle. ;)  It's standard practice to increment the patch version number in the filename so old patches aren't overwriten and can be retested/reapplied, e.g. magnatune2.patch, magnatune3.patch?
Comment 55 Adam Zimmerman 2006-09-06 05:22:19 UTC
(In reply to comment #54)
> Other issues with the updated patch:
> 
> When I abort rhythmbox during an album purchase/download, it writes a
> ~/.gnome2/magnatune/downloads_in_progress file with the location of the file to
> be redownloaded.  All well and good, except:
> 
> (1) when I restart rhythmbox and then open the Magnatune source, the UI hangs
> and rhythmbox has to be killed.  The plugin never offers, nor does it actually
> start, to redownload the aborted purchase. If I remove or rename the
> downloads_in_progress file, the plugin starts again fine.

Strange, it works fine for me (well, the UI does freeze for about 2 seconds, but after that it just starts the download again). Do you get any messages?

> 
> (2) if rhythmbox crashes during a download, the downloads_in_progress file
> never gets written.  Perhaps it should be written as soon as the download
> starts, or just before the download starts so that crashes don't lose the info.

The thing about that is I either have to keep track of what's in the file, and remove only that when it's finished, or rewrite the entire file every n seconds. I'll probably use the second solution. I'll do that soon.

> Lastly another patch niggle. ;)  It's standard practice to increment the patch
> version number in the filename so old patches aren't overwriten and can be
> retested/reapplied, e.g. magnatune2.patch, magnatune3.patch?

Will do.
Comment 56 Alex Lancaster 2006-09-06 06:57:44 UTC
(In reply to comment #55)

> Strange, it works fine for me (well, the UI does freeze for about 2 seconds,
> but after that it just starts the download again). Do you get any messages?

Odd, now I can't duplicate it.  Like the previous problem, maybe it only occurs on the first attempted restart of a download.

> The thing about that is I either have to keep track of what's in the file, and
> remove only that when it's finished, or rewrite the entire file every n
> seconds. I'll probably use the second solution. I'll do that soon.

You mean because you can have multiple downloads simultaneously?

Comment 57 Alex Lancaster 2006-09-06 06:59:10 UTC
(In reply to comment #56)

> You mean because you can have multiple downloads simultaneously?

Actually, that doesn't work at all.  Just tried a second download and it crashed with:

Xlib: unexpected async reply (sequence 0x3526cb)!
 

Comment 58 Adam Zimmerman 2006-09-06 15:41:25 UTC
(In reply to comment #56)
> (In reply to comment #55)
> 
> > Strange, it works fine for me (well, the UI does freeze for about 2 seconds,
> > but after that it just starts the download again). Do you get any messages?
> 
> Odd, now I can't duplicate it.  Like the previous problem, maybe it only occurs
> on the first attempted restart of a download.

Hm. I tried deleting the .gnome2/rhythmbox/magnatune directory, and it still works fine. I can't think of anything else (other than gconf keys) that get changed the first time the plugin's run.

> 
> > The thing about that is I either have to keep track of what's in the file, and
> > remove only that when it's finished, or rewrite the entire file every n
> > seconds. I'll probably use the second solution. I'll do that soon.
> 
> You mean because you can have multiple downloads simultaneously?
> 

That's the idea, although as you found out, it doesn't work at the moment. See attachment (id=71617) [edit] for a backtrace. Perhaps writing multiple files would do the trick though...
Comment 59 Adam Zimmerman 2006-09-09 11:50:01 UTC
Created attachment 72445 [details] [review]
updated patch

Updates:

- use the status bar to show download progress

There's now no way to cancel a download, but since the user has paid for the album by this point I'm not sure it's a huge deal. Should there be a way to do this? What would the UI for it look like? Presumably we don't want a cancel button in the status bar ;).
Comment 60 Alex Lancaster 2006-09-09 14:46:21 UTC
(In reply to comment #59)

> There's now no way to cancel a download, but since the user has paid for the
> album by this point I'm not sure it's a huge deal. Should there be a way to do
> this? What would the UI for it look like? Presumably we don't want a cancel
> button in the status bar ;).

There should definitely be a way of aborting the downloads.  This is also an issue with the CD ripping and transferring patch because there's currently no way to stop a CD import short of quitting rhythmbox: see bug #353844.  

It should probably be implemented/integrated with the same or similar UI as in that case.  My suggestion would be to create a new toolbar button with a "quit" or "cancel download" button.  Each source or plugin could provide the same kind of cancel button and use the same icon for consistency.

Comment 61 Alex Lancaster 2006-09-09 14:51:36 UTC
(In reply to comment #59)
> Created an attachment (id=72445) [edit]
> updated patch
> 
> Updates:
> 
> - use the status bar to show download progress

Hmm, when purchasing I see the "Downloading Magnatune Album(s)" but the progress bar on the right hand side is empty and isn't incrementing.  It is downloading the actual file, however.

While on the subject of the toolbar, it might be nice to add a "Buy Album" toolbar entry, it would be more "discoverable" than a right click.  Of course, a track from an album would have to be selected for it to be active.

Another thought: perhaps activate the browser by default so that the list of albums was visible immediately?


Comment 62 Alex Lancaster 2006-09-09 14:54:39 UTC
(In reply to comment #61)

> > - use the status bar to show download progress
> 
> Hmm, when purchasing I see the "Downloading Magnatune Album(s)" but the
> progress bar on the right hand side is empty and isn't incrementing.  It is
> downloading the actual file, however.

Also the status bar doesn't revert to the "yy songs, xx days, hh hours..." after the download is finished.
Comment 63 Alex Lancaster 2006-09-09 15:04:42 UTC
(In reply to comment #62)
> (In reply to comment #61)
> 
> > > - use the status bar to show download progress
> > 
> > Hmm, when purchasing I see the "Downloading Magnatune Album(s)" but the
> > progress bar on the right hand side is empty and isn't incrementing.  It is
> > downloading the actual file, however.
> 
> Also the status bar doesn't revert to the "yy songs, xx days, hh hours..."
> after the download is finished.

There seems to be some weird UI updating issues with the status bar.  If I select something in the browser after the purchase then all of a sudden the progress bar appears.  There are some other updates that only happen after I select items. 

Comment 64 Alex Lancaster 2006-09-09 15:07:20 UTC
(In reply to comment #63)

> There seems to be some weird UI updating issues with the status bar.  If I
> select something in the browser after the purchase then all of a sudden the
> progress bar appears.  There are some other updates that only happen after I
> select items. 

Yep, there's a consistent pattern: only when an item in the browser is selected is the status bar updating. 

Comment 65 Adam Zimmerman 2006-09-09 17:03:11 UTC
(In reply to comment #64)
> (In reply to comment #63)
> 
> > There seems to be some weird UI updating issues with the status bar.  If I
> > select something in the browser after the purchase then all of a sudden the
> > progress bar appears.  There are some other updates that only happen after I
> > select items. 
> 
> Yep, there's a consistent pattern: only when an item in the browser is selected
> is the status bar updating. 
> 

I've had that sort of thing happen when downloading the catalogue as well (it's not noticeable anymore though, since it happens so quickly now). If you select another source and go back to the Magnatune one, you'll see the progress bar updated. I have no idea what causes that or how to fix it though.

All the times I tested it today though, the status bar was updating fine, and it did revert back to the "number of songs" view. I'm not sure what you mean by "only when an item in the browser is selected" though. I usually don't even make the browser visible when I test the plugin, I just use the search box. So I never have items selected in the browser, unless I'm misunderstanding something.

How would creating the toolbar buttons work? From looking at the iradio source, it looks like I create an ActionGroup with two new Actions in it. But then how do I specify that they become buttons on the toolbar?
Comment 66 Alex Lancaster 2006-09-09 17:15:37 UTC
(In reply to comment #65)

> All the times I tested it today though, the status bar was updating fine, and
> it did revert back to the "number of songs" view. I'm not sure what you mean by
> "only when an item in the browser is selected" though. I usually don't even
> make the browser visible when I test the plugin, I just use the search box. So
> I never have items selected in the browser, unless I'm misunderstanding
> something.

It also works if you put something in the search box (although) it also won't update unless you change it after you start.  It appears it only updates when the query model changes (which can be done via the search or by selecting an item in the browser).

> How would creating the toolbar buttons work? From looking at the iradio source,
> it looks like I create an ActionGroup with two new Actions in it. But then how
> do I specify that they become buttons on the toolbar?

I don't know exactly.  Have a look at the last.fm patch (bug #313049), it's a plugin (although not in Python) that adds things to the toolbar.  It might not yet be exposed to the Python bindings. 

Comment 67 Adam Zimmerman 2006-09-11 01:56:22 UTC
Created attachment 72521 [details] [review]
yet another updated patch

Updates:

- write in-progress download info at beginning of download in case rb crashes
- make storing credit card info optional

Here's the rest of my TODO list, which is basically stuff that people have requested/suggested, and why they're not done yet:

Waiting for help/rhythmbox updates:
- Use track-transfer to transfer downloaded albums into the library. (it would be nice if there was generic code to do this, instead of reimplementing it for every plugin that wants it. What's the likelihood of this happening? Should I just do this myself?)
- allow cancelling of downloads, purchasing from the toolbar (I'm not really clear from reading the C code how to add buttons to the toolbar)

Laziness: ;)
- remembering the browser state/size (just remembered this one, will do it soon)
- maybe give warning about CC expiry in the past (lazy, I'll get around to it soon)
- logo in purchase dialog (same reason, I'll also probably need to make the logo smaller)

This stuff I'm not sure is possible with async.xfer:
- allowing to pause/resume the download
- display download rate (Kb/s)
- warn before quitting with purchase running, or when pressing cancel (I'm also not sure how to stop rhythmbox from quitting, and how to keep the force-quit dialog from showing up if I do)

I also tried making the source logo look better, but my gimp skills are even worse than my C skills, so I'm gonna delegate that one to somebody else :).

The only other thing I can think of that would be nice is if downloading multiple albums magically started working, although I think that's either a rhythmbox python bindings or gnomevfs bug. All my code has been written with multiple downloads in mind, barring any bugs or me using gnomevfs incorrectly (possible).

Are there any other features/bug fixes/code cleanups people would like to see?
Comment 68 Adam Zimmerman 2006-09-12 05:00:44 UTC
Created attachment 72584 [details] [review]
updates

Updates:

- give warning about CC expiry in the past
- logo in purchase dialog

I can't figure out how to save the browser state. The library source seems to do this by just implementing impl_get_browser_key and impl_get_paned key which return some gconf keys. I tried that, and it doesn't work (also tried changing them to do_impl_, since I'm not sure what the difference in naming means). Do I have to read and write the gconf keys manually? If so, what function do I call to get a callback when the browser gets toggled?
Comment 69 Adam Zimmerman 2006-09-12 05:03:43 UTC
Created attachment 72585 [details]
Images

Same as last time:

magnatune_circle_small.png goes in data/art
magnatune_logo_color_small and _tiny.png go in plugins/magnatune
Comment 70 James "Doc" Livingston 2006-09-17 02:20:17 UTC
The updates sound good, I'll try this again sometime soon.


(In reply to comment #67)
> Waiting for help/rhythmbox updates:
> - Use track-transfer to transfer downloaded albums into the library. (it would
> be nice if there was generic code to do this, instead of reimplementing it for
> every plugin that wants it. What's the likelihood of this happening? Should I
> just do this myself?)

What is probably easiest is creating some temporary db entries pointing to the extracted tracks, and then doing "library_source.paste (list_of_entries)". The only thing is that you currently can't find out when it has finished (to clean up).


> This stuff I'm not sure is possible with async.xfer:
> - allowing to pause/resume the download

It isn't, we've already looked at it for podcast downloading.


> - warn before quitting with purchase running, or when pressing cancel (I'm also
> not sure how to stop rhythmbox from quitting, and how to keep the force-quit
> dialog from showing up if I do)

I had a half-done implementation of this for something else, which I'll try to find. It worked by making the shell emit a "quitting" signal, whose handlers normally return FALSE, but can return true to stop it quitting. I hadn't quite figured out all the issues with displaying an "are you sure" dialog yet.


Comment 71 James "Doc" Livingston 2006-09-18 13:05:00 UTC
The plugin seems to be working well from my quick testing. However there was one issue I noticed, it doesn't handle gconf keys not existing well.

If /apps/rhythmbox/plugins/magnatune/sorting does not exist, it will crash when set_sorting_type is passed NULL:

  • File "/home/doc/programming/sources/rhythmbox/plugins/magnatune/magnatune/MagnatuneSource.py", line 96 in do_impl_activate
    self.get_entry_view().set_sorting_type(self.__client.get_string("/apps/rhythmbox/plugins/magnatune/sorting"))

Comment 72 Adam Zimmerman 2006-09-18 15:43:34 UTC
(In reply to comment #71)
> The plugin seems to be working well from my quick testing. However there was
> one issue I noticed, it doesn't handle gconf keys not existing well.
> 
> If /apps/rhythmbox/plugins/magnatune/sorting does not exist, it will crash when
> set_sorting_type is passed NULL:

The gconf keys are now all defined in data/rhythmbox.schemas, so it should have installed the key if you (re-)ran make install. I'll fix that anyways though, in case someone manages to delete a gconf key.
Comment 73 Adam Zimmerman 2006-09-23 02:15:15 UTC
OK, I'm trying to create temporary db entries and pass them to the library's paste method. I'm creating a "TempMagnatuneType" entry type so that they won't show up in a source, and then adding the tracks and passing the list to paste(). Everything seems to go right, except the tracks never get pasted. Dragging the preview tracks from the Magnatune source works fine though.

The "rhythmbox -d" output shows nothing related to the plugin after adding the (TempMagnatuneType) tracks to the db. No error messages, and the tracks never get copied to the library and never show up in the library source. Here's the rewritten __purchase_download_update_cb function. What am I doing wrong? Do the entries need to be a certain type?


	def __purchase_download_update_cb(self, _reserved, info, data):
		if (info.phase == gnomevfs.XFER_PHASE_COMPLETED):
			to_file_uri = data[0]
			library_location = data[1]
			audio_dl_uri = data[2]
			
			self.purchase_filesize -= gnomevfs.get_file_info(audio_dl_uri).size
			del self.__downloads[str(audio_dl_uri)]
			library_source = self.get_property("shell").get_property("library-source")
			entry_type = self.__db.entry_register_type("TempMagnatuneType")
			tracks = []
			album = zipfile.ZipFile(to_file_uri.path)
			for track in album.namelist():
				track_uri = gnomevfs.URI("/tmp/" + track)
				out = create_if_needed(track_uri, gnomevfs.OPEN_WRITE)
				out.write(album.read(track))
				out.close()
				tracks.append(self.__db.entry_new(entry_type, str(track_uri)))
			album.close()
			
			library_source.paste(tracks)
			gobject.timeout_add(5 * 60 * 1000, self.__delete_temp_tracks, tracks) # delete the db entries after 5 minutes
			gnomevfs.unlink(gnomevfs.URI(magnatune_dir + "in_progress_" + to_file_uri.short_name))
			gnomevfs.unlink(to_file_uri)
			if self.purchase_filesize == 0:
				self.__downloading = False
		return 1
Comment 74 Alex Lancaster 2006-10-05 16:37:25 UTC
I think this is ready to check into CVS since 0.9.6 has now been released.  James?
Comment 75 James "Doc" Livingston 2006-10-06 11:14:47 UTC
I've committed it to cvs, with a few minor fixes (I'm about to send a post to the ML too).

I haven't had a chance to look at the track-transfer issue yet.
Comment 76 Adam Zimmerman 2006-10-06 18:38:00 UTC
Created attachment 74176 [details] [review]
quick fix

quick fix:
- download album zip file into the magnatune directory instead of the library, so it doesn't get picked up by the file monitor as an unknown file.
Comment 77 Alex Lancaster 2006-10-22 14:29:40 UTC
(In reply to comment #76)

> quick fix:
> - download album zip file into the magnatune directory instead of the library,
> so it doesn't get picked up by the file monitor as an unknown file.

Makes sense to me (haven't tested it yet, on dial-up right now, not conducive to downloads ;)) 

Comment 78 Adam Zimmerman 2006-10-22 18:22:19 UTC
Created attachment 75210 [details] [review]
Partial toolbar button goodness

Updates:
- adds buttons to the toolbar to purchase, cancel, and get artist info

The buttons appear in the library source as well, and don't have icons. I'm not sure how to do those things. But they work properly when used in the magnatune source.
Comment 79 James "Doc" Livingston 2006-11-05 08:08:20 UTC
Sorry, I've taken so long to review this. The fixes look good, but there is a better way of adding stuff to the toolbar.

For source-specific toolbar items, you should override the "get_ui_actions" virtual function and return a list of strings, being the names of the GtkUiManager actions. Also, you don't need to make separate actions for the menu items and toolbar, you can just re-use the same action.
Comment 80 Adam Zimmerman 2006-11-05 19:35:50 UTC
Created attachment 76045 [details] [review]
Closer to proper toolbar goodness

This should work, but I think it's a bug in rhythmbox that makes it crash when you click on the Magnatune source.

The relevant part of the backtrace is:

Thread 1 (Thread -1229551424 (LWP 26602))

  • #0 __kernel_vsyscall
  • #1 raise
    from /lib/tls/i686/cmov/libc.so.6
  • #2 abort
    from /lib/tls/i686/cmov/libc.so.6
  • #3 __fsetlocking
    from /lib/tls/i686/cmov/libc.so.6
  • #4 mallopt
    from /lib/tls/i686/cmov/libc.so.6
  • #5 free
    from /lib/tls/i686/cmov/libc.so.6
  • #6 g_free
    from /usr/lib/libglib-2.0.so.0
  • #7 g_list_foreach
    from /usr/lib/libglib-2.0.so.0
  • #8 rb_list_deep_free
    at rb-util.c line 717
  • #9 rb_shell_select_source
    at rb-shell.c line 1966

Basically, what I think might be happening is rhythmbox is trying to free the list of strings, but since they're from a python plugin, the python GC should be handling it, which makes rhythmbox crash. Or something like that.

I'll attach the rest of the backtrace if you want, if I'm wrong.
Comment 81 James "Doc" Livingston 2006-11-06 09:00:25 UTC
Created attachment 76072 [details] [review]
more work

This, in combination with some fixes I just made in cvs, should work.
Comment 82 Adam Zimmerman 2006-11-11 00:52:38 UTC
Created attachment 76363 [details] [review]
icon fix

Everything seems to work in that patch. This update just gives the Cancel button an icon.
Comment 83 James "Doc" Livingston 2006-11-11 01:09:24 UTC
I've committed the patch to cvs, thanks.

I'm marking this bug fixed, since we have support in cvs and the bug is getting huge. Feel free to open separate bugs for any furthur improvements etc.