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 501405 - Update the Audioscrobbler Last.fm plugin to the new protocol version
Update the Audioscrobbler Last.fm plugin to the new protocol version
Status: RESOLVED FIXED
Product: banshee
Classification: Other
Component: Other Extensions
git master
Other Linux
: Normal enhancement
: 0.98.1
Assigned To: Banshee Maintainers
Banshee Maintainers
: 520559 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2007-12-04 00:19 UTC by Pepijn van de Geer
Modified: 2008-03-11 10:28 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Update Audioscrobbler to new protocol (27.43 KB, patch)
2007-12-04 00:21 UTC, Pepijn van de Geer
none Details | Review
Updated patch (29.58 KB, patch)
2007-12-05 17:09 UTC, Pepijn van de Geer
committed Details | Review

Description Pepijn van de Geer 2007-12-04 00:19:36 UTC
This (provisional) patch updates the Audioscrobbler plugin to the new protocol version 1.2. (1)

Most notable changes:
*  "Now Playing" notification added.
* New authentication scheme to increase flexibility and prevent session invalidation by malicious users.
* Submission point moved to end of track.
* "No skipping" rule removed (50%/240 seconds must still be played).
* INTERVAL protocol feature removed as it was never used.
* Timestamps changed to UNIX format for easier development and less ambiguity in URL-encoding.

It also adheres to the new failure handling guidelines stated in (1).

It has been running fine for a couple of hours without any trouble now. 
I would appreciate your comments, suggestions and testing. 

In the meanwhile I'll see if i can fix (or fixed) some of the the other bugs relating to the plugin. 

ps. there's still some debug output in there and the playername and version are still set to tst and 1.0 respectively
1) http://www.audioscrobbler.net/development/protocol/
Comment 1 Pepijn van de Geer 2007-12-04 00:21:31 UTC
Created attachment 100156 [details] [review]
Update Audioscrobbler to new protocol

And the patch itself..
Comment 2 Pepijn van de Geer 2007-12-05 17:09:11 UTC
Created attachment 100299 [details] [review]
Updated patch

I cleaned it up a bit and fixed bug #469490 as well.

* src/Plugins/Banshee.Plugins.Audioscrobbler/AudioscrobblerPlugin.cs:
* src/Plugins/Banshee.Plugins.Audioscrobbler/Queue.cs:
* src/Plugins/Banshee.Plugins.Audioscrobbler/Engine.cs: Updated to the 
new audioscrobbler protocol (1.2) and connection failure guidelines. 
(BGO: #404965, #501405)
Add connection time-out. (BGO: #469490)
Comment 3 Andrew Conkling 2008-02-08 11:09:04 UTC
Thanks for your patch. I'll try to get it reviewed soon.
Comment 4 Ruben Vermeersch 2008-02-10 15:00:29 UTC
Looks good, commited. Thanks Pepijn!
Comment 5 Pepijn van de Geer 2008-02-10 15:20:20 UTC
Thank you for accepting my patch! I was afraid it would get lost with the whole banshee-ng thing. 
If I find the time I'll try to port it to the new Banshee, if nobody beats me to it.
Comment 6 Andrew Conkling 2008-02-10 17:47:25 UTC
(In reply to comment #5)
> Thank you for accepting my patch! I was afraid it would get lost with the whole
> banshee-ng thing. 

a) We're getting better at reviewing patches as they come in, but b) feel free to prod on the mailing list if you don't get a reply in a reasonable amount of time.
Comment 7 Andrew Conkling 2008-02-13 06:17:47 UTC
There seems to be a relevant error message attached to bug #469490: http://bugzilla.gnome.org/attachment.cgi?id=105120&action=view
Comment 8 Pepijn van de Geer 2008-02-13 09:09:57 UTC
The windows popping up never happened here. I'll see if I can look in to it this weekend. 
Maybe just remove the error message altogether? The error is fine, it's catched, it's just the reporting. 
Comment 9 Asaf 2008-02-17 08:20:58 UTC
I also agree with Pepijn, it seems that the error is not causing any misbehavior to the end user, it is only the pop up message that is problematic. Maybe it is possible to make the message only appear in the terminal when in debug mode or something like that. This way it will not get in the way of regular use of Banshee
Comment 10 Andrew Conkling 2008-03-05 21:08:57 UTC
Pepijn, is the following screenshot related to your patch?
http://bugs.debian.org/cgi-bin/bugreport.cgi?msg=5;filename=audioscrobbler.png;att=1;bug=469510

This was reported downstream, and is bug #520559 here.
Comment 11 Pepijn van de Geer 2008-03-05 21:11:42 UTC
Yes, I think it is. This is the message you get when the connection times-out.
Only it was never meant to be shown in a dialog. Only in the debug window.
Comment 12 Andrew Conkling 2008-03-05 21:15:42 UTC
*** Bug 520559 has been marked as a duplicate of this bug. ***
Comment 13 Andrew Conkling 2008-03-05 21:17:25 UTC
(In reply to comment #11)
> Yes, I think it is. This is the message you get when the connection times-out.
> Only it was never meant to be shown in a dialog. Only in the debug window.

Thanks for confirming that; I've marked that bug as a duplicate. I'm also updating the milestone here; hopefully we can get this behavior ironed out.
Comment 14 Pepijn van de Geer 2008-03-05 21:27:33 UTC
I think this is they offending line:

LogCore.Instance.PushError ("Audioscrobbler NowPlaying failed", 
  String.Format("Failed to post NowPlaying: {0}", e));

Maybe I should've just a PushWarning? 
PushError is used on another occasion as well. That should probably be fixed too. I'm currently not in the situation to fix it myself, but this seems to be a one-line fix. (or two)

 
Comment 15 Sebastian Dröge (slomo) 2008-03-11 06:13:44 UTC
So this is not an error but only a warning that submission failed, the song(s) were added to the queue and on next successful submission everything will be sent? If so a PushWarning sounds good enough IMHO.
Comment 16 Pepijn van de Geer 2008-03-11 09:01:28 UTC
That's it. It just tries to submit it again a few sec. later.
If PushWarning doesn't display large dialogs then that should be it.
Comment 17 Sebastian Dröge (slomo) 2008-03-11 10:28:05 UTC
Ok, I've changed this one and a few other's where it made sense to a PushWarning... anything else that needs to be done? :)

I'll close this bug for now...