Re: [Banshee-List] banshee r3335 - in trunk/banshee: .src/Extensions/Banshee.Lastfm/Banshee.Lastfm.Audioscrobblersrc/Libraries/Lastfm/Lastfm

[Alex Hixon <hixon alexander mediati org>]

On Wed, 27 Feb 2008 12:33:01 -0600, "Gabriel Burt" <gabriel burt gmail com>
wrote:
> First, please rename the AudioscrobblerConnection.State enum values
> from FOO_BAR to FooBar.
> 
> On Wed, Feb 27, 2008 at 2:50 AM,  <ahixon svn gnome org> wrote:
>> +++
>
trunk/banshee/src/Extensions/Banshee.Lastfm/Banshee.Lastfm.Audioscrobbler/AudioscrobblerService.cs
>  Wed Feb 27 08:50:05 2008
>>  -            if (song_started && !queued && track.Duration.TotalSeconds
>> 30 &&
>>  +            if (!queued && track.Duration.TotalSeconds > 30 &&
>>                  track.ArtistName != "" && track.TrackTitle != "" &&
> 
> Please do not compare against "".  Use String.Empty, or use the
> String.IsNullOrEmpty method.
> 
>>  +++
> trunk/banshee/src/Libraries/Lastfm/Lastfm/AudioscrobblerConnection.cs    
 
> Wed Feb 27 08:50:05 2008
>>  +            if (artist == "" || title == "") {
> <snip>
>>  +            string str_track_number = "";
> 
> More of the same.
> 

This code is actually the original from when I ported the Audscriobbler
plugin from stable. At the moment, I'd like to at least get the code
working nice and smoothly before I begin to start doing cleanup, just
incase I accidentally introduce any bugs. As far as I know, these problems
also exist in stable.

> Second, you have a private bool connected variable, but it seems to
> hold the network_connected state - not whether this instance itself is
> connected - it is called AudioscrobblerConnection after all.  The
> Connect property is the same thing.  And the real fun starts with the
> Connect () method, which actually affects the started variable, not
> the connected one.  Please fix this confusion.

Yeah, this really should be fixed up by me. Also in a similar boat are the
now_playing_url and now_playing_uri variables - having names that are so
similar will more than likely introduce confusion.

>>  +            // Fill in placeholder text if NowPlaying was called when
>>  +            // we weren't authenticated.
>>  +            string fillin = "%now_playing_uri%?s=%session_id%";
> 
> Is fillin literally equal to the above, or are those two values some
> how interpolated in?
> 
>>  +
>>  +            // We prefer not to use replace with placeholders due to
> security
>>  +            // risks - so we substring to a predetermined length.
>>  +            if (uri.StartsWith (fillin) && session_id != null) {
>>  +                uri = String.Format ("{0}?s={1}&{2}", now_playing_url,
>>  +                                    session_id,
>>  +                                    uri.Substring (fillin.Length +
> 1));
>>  +            }
> 
> The goal here is to append any other parameters to the new uri, right?
>  Can you explain how this code is supposed to work?  Could you just
> look for the IndexOf session_id in the existing uri, and take
> everything after it?  When would the uri not StartWith(fillin)?

Basically, if NowPlaying was executed before we had already connected, we
store the URL to send data to in a private variable, run the Handshake
method, then re-execute the NowPlaying method with the previous URL we
stored. However, when we store the URL, we don't actually end up having the
session_id or the now_playing_url yet (ie we get a URI of
?s=&t=My%20Song..., rather than
http://ws.audioscrobbler.net/blah.php?s=a243ads&t=MySong...). Therefore, we
put in placeholders, and replace the beginning of the string only. 

Otherwise, we assume the URI is complete (that is, having http:// and the
session variables filled in), and don't run the substring formatting code.

The current way of doing it isn't *as* logical as it could be, and I'll fix
this by putting the if case away from just before we create the request to
when we form the URI instead.

Hopefully that made some sense. :)

Cheers,
Alex Hixon