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

["Gabriel Burt" <gabriel burt gmail com>]

Hi Alex,

Some comments on this commit, and on the Audioscrobbler code in general.

First, please rename the AudioscrobblerConnection.State enum values
from FOO_BAR to FooBar.

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.

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.

>  +            // 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)?

Thanks for your work Alex!

Gabriel