GNOME Bugzilla – Bug 630673
[bce] Add Extension Request: RandomByLastfm
Last modified: 2010-10-02 13:40:43 UTC
Created attachment 171155 [details] [review] git patch that adds RandomByLastfm Extension to BCE I wrote a RandomBy Implementation that uses lastfm's similarArtists feature to play tracks similar to the current. This extension is inspired by the "Smart Mode" of guayadeque, but was written from scratch. It features a simple logic to build a list of similar artists to the last manually (by user) played track. To keep the shuffled tracks close to the last manually added track, the maximum count of newly added artists is the 2^n fraction of maximal added tracks (see MAX_ARTISTS_ADD), where n is a number increased each time a new artist is played. Lists are properly reset if user manually selects an artist that is not a similar Artist to his last selection. RandomByLastfm is quite fast, as Lastfm is queried in background and database is only queried for artists that are definitely in CoreArtists. I think this extension is worth being added to BCE as an alternative to Mirage, as it is lighter and faster (especially on large music libraries). Attached is a patch for bce (git-master). See https://bugzilla.gnome.org/show_bug.cgi?id=630669 for a bug in the bce build workflow that affects this Extension
Created attachment 171201 [details] [review] Renamed ShuffleByLastfm.cs to RandomByLastfm.cs I'm sorry, i just noticed a little naming inconsistency in my patch, corrected in attached patch against git-master (ea08bf3e9e101da1dcc7e74ba3e96d419f1828c3)
@David, I have some serious doubts that you meant to mark this as depending on a closed gnome-core bug from 2001... :p @Raimo, this is a very cool functionality; I'm really looking forward to testing this out. Thanks!
There we go, missed a 3 somehow. I've looked at the patch a bit. It would be nice if the update and the original patch could be rolled into one as they seem to be two git patches. Other than that, the commented out code for proxying to main should probably be removed and the comments have minor unimportant spelling mistakes. Nice work Raimo.
Created attachment 171227 [details] [review] RandomByLastfm Patch - merged all commits into one This patch fixes some typos in RandomByLastfm. Further, all commits have been merged into one.
Review of attachment 171227 [details] [review]: General: The core code appears twice? Since the renaming to RandomByLastfm you do still appear to have methods using the Shuffle term, would it perhaps be prudent to rename these instances? I don't know if we have a style convention for logging, but perhaps doing something like "RandomByLastFM: <message>" would make it easier to dig out where especially error messages originate from for users (for bug reporting purposes). Though this might need to be applied to all extensions - thoughts? + RandomByLastfm: yes should be something like + RandomByLastfm: ${enable_RandomByLastfm} You can probably steal the logic for this from one of the existing extensions. Indentation for this seems inconsistent with the style of the existing code + RandomByLastfm \ How did you arrive at these constants? A comment would be nice. + public static readonly int MAX_ARTISTS = 50; + public static readonly int MAX_ARTIST_ADD = 40; + public static readonly int MIN_ARTIST_ADD = 5; + // User changed Track to a not similar artist, clear list to + // User changed Track to an artist not on the similar list, clear list How did you arrive at this? + // Formular: numTake = Max(MIN_ARTIST_ADD, MAX_ARTIST_ADD/(2^similarityDepth)) Outside of that the code looks clean, readable and correct so far as I can tell.
As this extension will not be released with 1.8.0 i decided it would be a good idea to develop some more features around Shufflers and LastFm. I therefore forked bce at http://github.com/rradczewski/banshee-extension-randombylastfm where i will continue work on RandomByLastfm (which should be called RandomByLastfmSimilarArtists). Thanks for all your feedback, i hope RandomByLastfm will find its way to bce soon.
Review of attachment 171227 [details] [review]: Please follow the HACKING style guide and remove trailing whitespace. ::: Extensions.sln @@ +92,3 @@ + {20C763D2-6E41-4C4A-BD15-6B4B72911280}.Debug|x86.Build.0 = Debug|Any CPU + {20C763D2-6E41-4C4A-BD15-6B4B72911280}.Release|x86.ActiveCfg = Release|Any CPU + {20C763D2-6E41-4C4A-BD15-6B4B72911280}.Release|x86.Build.0 = Release|Any CPU the Release config is obsolete actually -- can you instead change your .csproj and these changes to .sln to use the Windows config? ::: src/RandomByLastfm/Banshee.RandomByLastfm/LastfmQueryjob.cs @@ +24,3 @@ +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. +using System; Please put a line break between the copyright header and the first 'using' @@ +27,3 @@ +namespace Banshee.RandomByLastfm +{ + public class LastfmQueryjob : Banshee.Kernel.Job I would prefer you didn't use the Banshee.Kernel.Job class either -- it should really be marked as obsolete. Instead just use ThreadAssist.SpawnFromMain or some other mechanism. @@ +36,3 @@ + protected override void RunJob () + { + RandomByLastfm.QueryLastfm(); Missing a space before parenthesis here. ::: src/RandomByLastfm/Banshee.RandomByLastfm/RandomByLastfm.cs @@ +42,3 @@ +namespace Banshee.RandomByLastfm +{ + get rid of extra newline here @@ +45,3 @@ + public class RandomByLastfm : RandomBy + { + public static List<string> artistsAdded; Do these need to be public? Public members should start with capital letter. Private members should_use_underscores. See HACKING for full code style guide. @@ +48,3 @@ + public static List<string> similarArtists; + + public static readonly int MAX_ARTISTS = 50; Change static readonly to const ::: src/RandomByLastfm/RandomByLastfm.addin.xml @@ +5,3 @@ + compatVersion="0.3" + copyright="Copyright © 2010 Raimo Radczewski Licensed under the MIT X11 license." + name="Lastfm Shuffle Mode" Lastfm => Last.fm here
I just merged your github branch to b-c-e master : http://gitorious.org/banshee-community-extensions/banshee-community-extensions I also addressed the build issue with Lastfm.dll, and Gabriel's comments. Please give me your gitorious user id, so that I can add you to the banshee-community-extensions team, to allow you to push directly to the b-c-e repository. Just send me an e-mail or ping me on IRC. Thanks for creating this extension, I think it's a nice addition to the shuffle modes. And I'm looking forward to future enhancements !