Created attachment 133555 [details] [review]
Make it work, but not well

libsoup can't seem to handle the incomplete packets that audioscrobbler responds to posts with.  This patch will work, but error code 7 (connection ended unexpectedly) is treated as success.

*** Bug 579538 has been marked as a duplicate of this bug. ***

The actual problem here is that we're sending requests that look like HTTP 0.9, as the post URI has a newline at the end.  This is happening because we're parsing the handshake response badly.  Change the second parameter to g_strsplit() in rb_audioscrobbler_parse_response to 0 and everything works properly.

Created attachment 133814 [details] [review]
Audioscrobbler 1.2 support

This works fine.  Libre.fm seems down at the moment, so, I can't confirm it there, but, it works for last.fm, and they're supposed to be the same anyway.

Created attachment 133830 [details] [review]
Actually good patch

Adds nowplaying to the idle callback so that we don't flood the server if we get track-skipping-happy.  Should fix everything, including the missing unref in the last patch.

This looks pretty good, just a few minor things that need to be fixed up:

+	if (1==0) {//rhythmdb_entry_get_entry_type(rb_entry) == rhythmdb_entry_type_get_by_name (db, "lastfm-track")) {
+		as_entry->source = g_strdup ("L");
+	} else {
+		as_entry->source = g_strdup ("P");
+	}
+
+// Not implemented yet
+//		case RHYTHMDB_STREAM_TYPE:
+//			as_entry->source = g_strdup ("R");

Are you intending to implement this?  If not, remove the commented bits and the if (1==0) and add a comment explaining what needs to be done.  Also, just as a matter of consistency, we don't use c++ style '//' comments.

-	encoded->album = soup_uri_encode (entry->album, 
+	encoded->album = soup_uri_encode (entry->title,

This doesn't look right..

-	strftime (encoded->timestamp, 30, SCROBBLER_DATE_FORMAT, 
-		  gmtime (&entry->play_time));
+	//	strftime (encoded->timestamp, 30, SCROBBLER_DATE_FORMAT, gmtime (&entry->play_time));

If you're going to remove some code, just remove it.

+		} else if (g_str_has_prefix (breaks[0], "BADAUTH")) {
+			rb_debug ("Bad authorization"); // User needs to fix credentials
+			audioscrobbler->priv->status = BADAUTH;
+			audioscrobbler->priv->status_msg = g_strdup ("Bad username or password\n");

We don't need to set status_msg here, the status code already describes the error condition.

+		} else if (g_str_has_prefix (breaks[0], "BADTIME")) {
+			rb_debug ("Bad timestamp"); // timestamp not close enough to server
+

This looks like an error case we should report to the user rather than silently failing.

+	} else if  (msg->status_code == 7) {
+		audioscrobbler->priv->status = STATUS_OK; //Deal with libsoup not being able to deal with http w/o headers.

This shouldn't be here any more since we worked out why the server was sending headerless responses.

Good catches!

The 'not implemented yet' bit is something that will be in the protocol, but, the API spec says isn't implemented yet.  I figured I'd have the code there for when it is implemented, but, right now that section pretty much just needs P's.

I'll fix these later today, thanks moch.

Created attachment 133868 [details] [review]
w/ fixes Johnathan suggested

The only thing I didn't change is that there is still the comment about the source parameter which isn't fully implemented on last.fm's side yet.

Created attachment 133879 [details] [review]
fixes based on libre.fm folk and get rid of the old timestamp format

Created attachment 133880 [details] [review]
one debug output fix

A couple more things..

rb_audioscrobbler_entry_save_to_string should use 'i' not 'I' for the timestamp (same format), and rb_audioscrobbler_entry_load_from_string should parse 'i' and 'I' the same way (strtol is more reliable than atoi).

'I' was a format extension we added to fix some parsing/timezone problems (bug 508895), but since the official format uses unix style timestamps now, we don't need to use it any more.  We still need to parse it for backwards compatibility, though.

The play time we submit is actually supposed to be the time the song started playing, not the time we decided to submit it, so we should record a timestamp when the song starts, in rb_audioscrobbler_song_changed_cb rather than setting it in maybe_add_current_song_to_queue.

+		status = _("Computer's time isn't close enough to server's");

I'm not sure how best to say this, but I don't think we need to talk about it being different to the server's time, we should just say something like "clock is not set correctly".

Created attachment 133971 [details] [review]
FIxes as per mailing list

Changed the error message to the one on the mailing list.  Fixed the formatting import.

I fixed up a few more things (a few error messages and implementing the handshake delay as recommended in the protocol spec, mostly), added some notes about determining the playback source, and commited it.  Thanks for working on this.

*** Bug 563137 has been marked as a duplicate of this bug. ***