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 536389 - DAP Scrobbling Support
DAP Scrobbling Support
Status: RESOLVED FIXED
Product: banshee
Classification: Other
Component: Last.fm
git master
Other Linux
: Normal enhancement
: ---
Assigned To: Banshee Maintainers
: 539806 562049 563701 566500 573042 603140 609784 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2008-06-03 09:34 UTC by Peter de Kraker
Modified: 2012-05-01 13:01 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Enables DAP scrobbling support (40.51 KB, patch)
2012-01-31 05:44 UTC, Phil Trimble
reviewed Details | Review
Refactoring of AudioscrobblerService (4.64 KB, patch)
2012-02-12 01:17 UTC, Phil Trimble
reviewed Details | Review
Refactoring of AudioscrobblerService (4.87 KB, patch)
2012-02-17 06:45 UTC, Phil Trimble
none Details | Review
Refactoring of AudioScrobbleService v2 (4.93 KB, patch)
2012-02-19 16:30 UTC, Andrés G. Aragoneses (IRC: knocte)
committed Details | Review
Enables DAP scrobbling support (1/2) (12.45 KB, patch)
2012-03-29 04:56 UTC, Phil Trimble
none Details | Review
Enables DAP scrobbling support (2/2) (14.86 KB, patch)
2012-03-29 05:10 UTC, Phil Trimble
none Details | Review
Enables device scrobbling support (25.00 KB, patch)
2012-04-21 05:18 UTC, Phil Trimble
none Details | Review
Enables device scrobbling support v4 (21.45 KB, patch)
2012-04-21 14:14 UTC, Andrés G. Aragoneses (IRC: knocte)
committed Details | Review

Description Peter de Kraker 2008-06-03 09:34:54 UTC
Got my Ipod working with banshee svn, so I would like to have my tracks that I play on my ipod, to be scrobbled by banshee when I connect it.

This functionality does not exist yet (or I am blind), so this is a request for it.

I hope to fulfill this request myself in the coming weeks.

===
I looked briefly on the internet for Ipod Scrobbling support, but I couldn't find any implementation for offline mediadevice (or non-chronological) scrobbling by unofficial clients/libraries. Probably because this is not covered in the api documentation and the resulting spam protection that prevents you from just submitting the tracks, explained on the lastPod website:

"Do not listen to any tracks using your audio player until you have synced your iPod. If you do, your iPod played tracks will not go through due to the AudioScrobbler spam protection. The spam protection disallows any tracks to be submitted that have a timestamp before the most recently played track. "

Amarok does support scrobbling ipod, but I couldn't find out about the non-chronological support (which is the uber awesome stuff ofcourse ;) )

Fortunately the sourcecode of the official Lastfm Client (that does support this feature) reveals what needs to be added to the submission url to do make it work. Together with Ipod-Sharp this should be possible to implement, so that you don't have to worry about not listening on your pc before you connect your ipod, etc.

Would be very cool to have this stuff. Especially since it would be extensible to cover other mediadevices that keep some sort of songdatabase, making banshee one step more rocking.
Comment 1 Martin Raißle 2008-06-18 14:06:00 UTC
Actually you can't read the timestamps of all tracks played on the ipod, but only the last time a certain song was played, so if you play 10 different tracks you get 10 timestamps, if you play one track 10 times, you get one timestamp. 

As far as I know, Amarok uses the play count since last sync, which is saved on the ipod too and than fakes timestamps. Amarok also gets the last time when a song was scrobbled, to not trigger the spam protection, but as mentioned above, this is not required.

I didn't look at the source of the official client wether a timestamp is required for offline plays too, but if so, you need to fake the timestamps.
Comment 2 Gabriel Burt 2008-07-28 01:01:26 UTC
This should really be generic and work for any DAP.
Comment 3 Gabriel Burt 2008-07-28 01:02:09 UTC
*** Bug 539806 has been marked as a duplicate of this bug. ***
Comment 4 Peter de Kraker 2008-09-07 00:09:09 UTC
This functionality will not be implemented nor worked upon by me. Due to reallife issues I have decided to stop contributing.
Comment 5 Bertrand Lorentz 2008-11-23 20:57:53 UTC
*** Bug 562049 has been marked as a duplicate of this bug. ***
Comment 6 Bertrand Lorentz 2008-12-08 22:02:45 UTC
*** Bug 563701 has been marked as a duplicate of this bug. ***
Comment 7 skerit 2008-12-12 22:15:26 UTC
I'm eagerly awaiting any kind programming soul to code this little feature.
Comment 8 Bertrand Lorentz 2009-01-04 13:10:02 UTC
*** Bug 566500 has been marked as a duplicate of this bug. ***
Comment 9 Bertrand Lorentz 2009-01-04 13:12:06 UTC
Changing component to make it clearer that this bug is about all kinds of DAP devices.
Comment 10 Bertrand Lorentz 2009-02-25 18:14:30 UTC
*** Bug 573042 has been marked as a duplicate of this bug. ***
Comment 11 Gabriel Burt 2009-10-27 20:16:54 UTC
Bulk changing the assignee to banshee-maint@gnome.bugs to make it easier for people to get updated on all banshee bugs by following that address.  It's usually quite apparent who is working on a given bug by the comments and/or patches attached.
Comment 12 Bertrand Lorentz 2009-11-28 12:47:45 UTC
*** Bug 603140 has been marked as a duplicate of this bug. ***
Comment 13 Bertrand Lorentz 2010-02-13 15:58:46 UTC
*** Bug 609784 has been marked as a duplicate of this bug. ***
Comment 14 Andreas Böttger 2010-02-13 16:56:23 UTC
this is a known bug since 2008. We have 2010! I can't understand that.
Comment 15 Gabriel Burt 2010-02-13 20:56:00 UTC
While we do track feature requests here in bugzilla, keep in mind that there are infinite possible feature requests, and if you want yours to have a higher liklihood of making it into Banshee, you should submit a patch or somehow induce somebody else to.  A hint on inducing the core Banshee devs (well, at least me): don't come off sounding entitled or demanding, but be positive, encouraging, and help make it clear why your issue is so important.
Comment 16 skerit 2010-08-29 16:40:33 UTC
I believe this bug is very important in order to improve overal user experience.
It's a genuine pain to scrobble your iPod played tracks at the moment.

It used to be a feature in both Rhythmbox and Amarok 1.4.

Rhythmbox hasn't really been keeping up with latest changes in the audioplayer field, in my opinion, and is really slow when working with large collections.

Amarok 1.4 is now over 2 years old, with the feature not being present in Amarok 2 (and amarok 2 not being as pleasant to use as Amarok 1.4)
Comment 17 Mieszko Ślusarczyk 2010-08-29 21:17:14 UTC
For iPod/players compatible with MTP I suggest you using qtscrobbler, but if you use iPod touch I recommend you using scrobbl, as I do;)
Comment 18 oingman 2011-05-11 21:09:29 UTC
Hi! Is there a chance to get this feature? I moved to Banshee just before a week and this is the only thing i miss.
Comment 19 Phil Trimble 2011-11-20 20:53:17 UTC
Hi all, I've decided to try to take a crack at this. When I switched to Ubuntu I searched across all of the ipod scrobbling projects for something that worked. Nothing worked perfectly for me. Even the native Last.FM linux client flat-out breaks for me and I gave up in frustration. 

Then I found this: http://t-a-w.blogspot.com/2006/06/ipod-lastfm-bridge.html. It's written in Ruby and scrapes the iTunes Database and Play Count file to build a list of songs to send to Last.FM. It works great (and taught me a lot) but it requires either A) manually running two scripts, piping the output of one into the other or B) writing a shell script of some kind to do it for you. I took the second route. 

It works but obviously having this done natively by Banshee would be ideal. I also know that it is possible from within Banshee since it already supports scrobbling songs as you play. So with that idea I started digging into the Banshee code. I believe I see where we can do this. Here is my plan of action:

1) As Gabriel says above, implement this at the DAP level. However, I will limit my scope to Apple devices only. The other devices will implement the method structure inherited from the overall DAP classes but not return anything (or return a message saying that it is not supported, whatever is appropriate, I haven't spent much time looking into this aspect yet).

This is for two reasons: first, I believe that the libgpod library used for Apple devices already has the recent play count info available and so implementing something shouldn't be a huge effort. Second, I have vague memories from my Creative MP3 player days that most other players did not have good playcount info stored on the device. This has probably changed but I will concentrate on the player I have first and then think about moving on based on requests. I might already be biting off more than I can chew, I'll just start here. :)

For this change I am planning on only having the functionality for the DAP device to return playcount info. It won't interact with Last.FM directly.

Lastly, as Martin mentions in the second comment it is not possible to accurately report the exact playtime of each play since your last sync. This is due to the ipod itself...as far as I can see it only retains the most recent playtime. We are forced to fudge things a little for Apple devices, at least. I'll probably be stealing the logic from the Ruby script I mentioned above. We'll see how it goes.

2) Add a new Last.FM preference so a user can select 'Report my recent plays to Last.FM upon syncing' or something along those lines. 

3) Modify the Last.FM code so that it gets the playcount info from the DAP device and then display some kind of window to show what you are about to submit. This is similar to how the official Last.FM plugin works on Windows, based on my memory. You can then watch the progress as each song is submitted. I would like to use the existing scrobbling code as much as possible.

I'm a little fuzzy on two areas: first, I have no GTK experience and so learning it while also learning how to correctly integrate into the existing Banshee infrastructure is tough. We'll see how it goes. 

Second, I'm not sure how to correctly design the interaction between the Last.FM code and the DAP/AppleDevice code. The only thing I can think to do at this point is have the DAP/AppleDevice code, upon initializing, check the Last.FM preference to see if it should submit play info since last sync to last.fm and then call the appropriate Last.FM code to do the rest? The only other thing I can think of is to somehow have the Last.FM code register its interest with the DAP device so that, upon a sync, it executes its own events. But I have no experience with this and I'm already worried that this task is too big for a new developer to Banshee to accomplish.

I don't know, like I said I'm new to all of this. I figured I would just go with the first approach I listed, which is most of the heavy lifting, and then rely on core developers to help me polish things off if things get to that point.

We'll see how it goes! I have no ETA since I'm brand-new to almost all aspects of this effort. I'm taking this as a learning opportunity and at the same time I'm adding functionality that will directly benefit me. Hopefully this will be the motivation I need. I do have a full-time development job and so I have to do this in my spare time. Progress will probably be very slow. 

If anyone is already making progress on this then please let me know.
Comment 20 Phil Trimble 2012-01-31 05:44:51 UTC
Created attachment 206494 [details] [review]
Enables DAP scrobbling support

This patch adds in DAP-level scrobbling support for Last.fm. This new functionality is disabled by default. 

It is applied at the DAP level but currently only supports Apple devices since that is all I own. As I mention in comments above as far as I am aware only Apple devices have this recent play count information available. If anyone else has a different device and would like to collaborate please let me know, I would be happy to discuss it.

I have also updated the help/C/lastfm.page user guide page to highlight the new feature addition. 

After enabling this functionality a user will, upon device sync, have a dialog window pop up that shows all tracks played since the last sync. The user can then either submit to Last.fm or cancel. 

Apple devices have their recent playcount information cleared upon sync so a user has until Banshee is closed to submit. Banshee will keep track of whether a connected device has been synced so a user can cancel and re-sync and still have an option to submit. After a sync and Banshee is closed the recent playcount information is lost from the device. This functionality can be modified to be more flexible if other types of devices act differently. We could also potentially save device scrobble information like we do the Last.fm queue but I decided to leave that as a future enhancement since this was already getting a little large.

I have tested this extensively with my own iPod Classic 7G. In theory this should work for all Apple devices supported by libgpod.
Comment 21 Andrés G. Aragoneses (IRC: knocte) 2012-02-05 01:18:50 UTC
Comment on attachment 206494 [details] [review]
Enables DAP scrobbling support


Awesome patch Phil!

I'm going to do a first review. Please don't get scared with it. Most of it will be nitpicks that are easy to fix and questions you can follow-up with.

I'll do it inline:

> diff --git a/help/C/lastfm.page b/help/C/lastfm.page
> index 16db221..4dc76e1 100644
> --- a/help/C/lastfm.page
> +++ b/help/C/lastfm.page
> @@ -12,6 +12,7 @@
>        <name>Paul Cutler</name>
>        <email>pcutler@gnome.org</email>
>      </credit>    
> +
>  <!--
>      <copyright>
>        <year>2010</year>

Please drop this kind of superflous changes from the patch.


> @@ -37,8 +38,8 @@
>    Visit <link href="http://www.last.fm/join">http://www.last.fm/join</link>
>     to create an account or choose 
>    <guiseq><gui>Edit</gui><gui>Preferences</gui></guiseq> from the Banshee
> -  menu.  Then press the <gui>Source Specific</gui> tab and press the
> -  <gui>Source</gui> drop down menu and choose <gui>Last.fm</gui> and select the
> +  menu. Once in the preferences select the <gui>Source Specific</gui> tab, press the
> +  <gui>Source</gui> drop down menu, choose <gui>Last.fm</gui> and finally select the
>    <em>Sign up for Last.fm</em> link.
>    </p>

This change is not related to this bug, right? Please put separate things in separate patches :)


> @@ -56,18 +57,49 @@
>    
>    <section id="songreporting">
>    <title>Enable Last.fm Song Reporting</title> 
> -  <p>After you have successfully linked Banshee to your Last.fm profile, to 
> -  enable Banshee to report the songs to your Last.fm profile, in the 
> -  <gui>Source Specific</gui> tab in Banshee's preferences, press the 
> -  <gui>Enable Song Reporting</gui> checkbox.  If you have an active internet
> -  connection, Banshee will now send Last.fm information regarding the songs
> -  you play.  To view your play history, visit your profile on the Last.fm 
> +  <p>After you have successfully linked Banshee to your Last.fm profile you must 
> +  ensure that you have enabled Banshee to report your songs.  To enable Banshee to 
> +  report the songs to your Last.fm profile go to Banshee's preferences, select the
> +  <gui>Source Specific</gui> tab, select <gui>Last.fm</gui> from the dropdown, and 
> +  press the <gui>Enable Song Reporting</gui> checkbox.
> +  If you have an active internet connection Banshee will now send Last.fm information
> +  regarding the songs you play. To view your play history visit your profile on the Last.fm
>    website.  Last.fm will automatically update your music metadata if any of your
> -  artist, song title or album information is incorrect.
> +  artist, song title, or album information is incorrect.

These seem like good improvements but not related to DAP Audioscrobbling, right?


>    </p>
>    
>    </section>
> -  
> +
> +  <section id="devicesongreporting">
> +  <title>Enable Last.fm Song Reporting From Your Device</title>
> +  <p>After succesfully linking Banshee to your Last.fm profile and enabling Banshee to 
> +  report songs to Last.fm you can also enable scrobbling from a connected device. Banshee 
> +  will, upon sync of your device, attempt to scrobble the songs you have played since the 
> +  device was last connected and submit them to Last.fm.
> +  </p>
> +
> +  <p>To enable scrobbling of a connected device go to Banshee's preferences, select the 
> +  <gui>Source Specific</gui> tab, select <gui>Last.fm</gui> from the dropdown, and press 
> +  the <gui>Enable Reporting Of Device Plays Since Last Sync</gui> checkbox.  If you have an 

Is "Device Plays" a good name? I ask because I'm not a native English speaker, are you?


> +  active internet connection and the <gui>Enable Song Reporting</gui> checkbox selected 
> +  Banshee will now attempt to gather information regarding the songs that you have played 
> +  on your device since your last sync and display them in a dialog.  You can then select 

Do we really want a dialog every time the sync happens? If people are interested in this feature, they could just enable it, and DAP Audioscrobbling will happen always. If they don't like it they just re-disable it again. Why confirm the action in an every-sync basis? What is the exact use case of this granularity?


> +  whether to submit or cancel.  This process will only take place upon a sync in Banshee.
> +  </p>
> +
> +  <p>As with regular Banshee scrobbling submissions, Last.fm will automatically update 
> +  your music metadata if any of your artist, title, or album information is incorrect.
> +  </p>

I would add something here like "(although we recommend you use the Metadata Fixer extension
to correct your files)".


> +
> +  <p>Please note that currently Banshee only supports this feature with Apple devices. 

"With the Apple devices that the AppleDevice extension currently supports".


> +  Any Apple device supported by Banshee for syncing will be scrobbled. Also note that, due 
> +  to the design of Apple devices, once you sync and close Banshee your 'recently played' 
> +  information will be lost. You must submit your songs to Last.fm at the time of your sync
> +  and before you close Banshee.

Cannot we consider this a bug? I mean, shouldn't we show the user a warning dialog if they want to close Banshee and the scrobbling didn't finish yet? (In the same way we already present a warning if sync didn't finish and a user wants to exit Banshee)



> diff --git a/src/Core/Banshee.Core/Banshee.Collection/TrackInfo.cs b/src/Core/Banshee.Core/Banshee.Collection/TrackInfo.cs
> index 8ecefc7..4da103b 100644
> --- a/src/Core/Banshee.Core/Banshee.Collection/TrackInfo.cs
> +++ b/src/Core/Banshee.Core/Banshee.Collection/TrackInfo.cs
> @@ -319,6 +319,9 @@ namespace Banshee.Collection
>          public virtual int PlayCount { get; set; }
>  
>          [Exportable]
> +        public virtual int RecentPlayCount {get; set; }

Do we really need this property in this class? I've seen that you use it in AppleDeviceSource but you only read it, then you store it in AppleDeviceTrackInfo but I think that doesn't get read from that class anymore.


> diff --git a/src/Dap/Banshee.Dap.AppleDevice/Banshee.Dap.AppleDevice/AppleDeviceSource.cs b/src/Dap/Banshee.Dap.AppleDevice/Banshee.Dap.AppleDevice/AppleDeviceSource.cs
> index bbe3773..553e834 100644
> --- a/src/Dap/Banshee.Dap.AppleDevice/Banshee.Dap.AppleDevice/AppleDeviceSource.cs
> +++ b/src/Dap/Banshee.Dap.AppleDevice/Banshee.Dap.AppleDevice/AppleDeviceSource.cs
> @@ -42,6 +42,7 @@ using Banshee.Hardware;
> +                        // Since the ipod doesn't keep track of individual plays we need to calculate
> +                        // a new fudged last play datetime for each play if we have more than one play.
> +                        // Note that the final play timestamp is entirely fudged. We don't care if we
> +                        // are accurate in terms of datetime, only whether our overall playcount is correct.

Could you add this timestamp-fudging logic into its own method (so the method name also explains the intent and then the comment you need to write is smaller?)?

> +                        if (i == 0) {
> +                            played_track.LastPlayed = most_recent_playtime;
> +                        } else {
> +                            played_track.LastPlayed = most_recent_playtime - played_track.Duration;
> +                            most_recent_playtime = played_track.LastPlayed;
> +                        }
> +
> +                        recently_played_tracks_list.Add (played_track);
> +                    }
> +                }
> +            }
> +
> +            // Sorts the played tracks from oldest to newest
> +            recently_played_tracks_list.Sort (delegate (TrackInfo t1, TrackInfo t2) { return t1.LastPlayed.CompareTo (t2.LastPlayed); });

Why is this sorting done? (Please try to put comments explaining the why, not the what.)



> diff --git a/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage/MassStorageSource.cs b/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage/MassStorageSource.cs
> index 3e23b30..102abd4 100644
> --- a/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage/MassStorageSource.cs
> +++ b/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage/MassStorageSource.cs
> @@ -720,5 +720,18 @@ namespace Banshee.Dap.MassStorage
>          public override bool HasEditableTrackProperties {
>              get { return true; }
>          }
> +
> +        /*
> +         * This method is not supported at this point. This is because, according to my potentially
> +         * out of date information, all devices outside of Apple devices do not contain 'last played' info
> +         * and therefore cannot implement any kind of 'what has played since last sync' functionality.
> +         * If this is not true then this logic can be filled in and the calling methods can process it
> +         * accordingly. - Phil Trimble, 2012/01/19, philtrimble@gmail.com
> +         */

Why not, instead of having an abstract method and have this empty method and huge comment in every non-AppleDeviceSource derived class, have instead a *virtual* empty method that only AppleDeviceSource overrides?


> diff --git a/src/Dap/Banshee.Dap/Banshee.Dap/DapService.cs b/src/Dap/Banshee.Dap/Banshee.Dap/DapService.cs
> index 93e6a63..b5904be 100644
> --- a/src/Dap/Banshee.Dap/Banshee.Dap/DapService.cs
> +++ b/src/Dap/Banshee.Dap/Banshee.Dap/DapService.cs
> @@ -54,6 +54,8 @@ namespace Banshee.Dap
>          private bool initialized;
>          private object sync = new object ();
>  
> +        public event SourceEventHandler DapSourceAdded;

Why not use SourceAdded event which is already there, and check if the source that has been added is a DapSource.

  
> diff --git a/src/Dap/Banshee.Dap/Banshee.Dap/DapSource.cs b/src/Dap/Banshee.Dap/Banshee.Dap/DapSource.cs
> index e5e23e7..3680f4c 100644
> --- a/src/Dap/Banshee.Dap/Banshee.Dap/DapSource.cs
> +++ b/src/Dap/Banshee.Dap/Banshee.Dap/DapSource.cs
> @@ -59,6 +59,8 @@ namespace Banshee.Dap
>          private Page page;
>          // private DapPropertiesDisplay dap_properties_display;
>  
> +        public event EventHandler DapSourceDisposed;
> +

Same suggestion but with generic SourceRemoved (or SourceDisposed, haven't looked at the code to check the real name).



> diff --git a/src/Dap/Banshee.Dap/Banshee.Dap/DapSync.cs b/src/Dap/Banshee.Dap/Banshee.Dap/DapSync.cs
> index ceeb233..e7269a4 100644
> --- a/src/Dap/Banshee.Dap/Banshee.Dap/DapSync.cs
> +++ b/src/Dap/Banshee.Dap/Banshee.Dap/DapSync.cs
> @@ -65,6 +65,7 @@ namespace Banshee.Dap
>          public event Action<DapSync> Updated;
>          public event Action<DapLibrarySync> LibraryAdded;
>          public event Action<DapLibrarySync> LibraryRemoved;
> +        public event Action<DapSync> SyncStarted;

I think DapSync is a class used only for when AutoSync mode is enabled in the device, have you tested your patch when using Manual mode?
If your patch doesn't work in Manual mode, you may want to hook in a different place a SyncStarted event, not in this class (but now that I think about it, do we really want to hook it up when Sync starts or only when a device is detected for the first time? I guess that this is the reason of why you need the submitted_devices variable in the next parts of the diff... so maybe if you can find a place where the detection of a device happens, you would only hook to that event and you wouldn't need the submitted_devices variable).


> diff --git a/src/Extensions/Banshee.Lastfm/Banshee.Lastfm.Audioscrobbler/AudioscrobblerService.cs b/src/Extensions/Banshee.Lastfm/Banshee.Lastfm.Audioscrobbler/AudioscrobblerService.cs
> index 5e0758f..3fa78f5 100644
> --- a/src/Extensions/Banshee.Lastfm/Banshee.Lastfm.Audioscrobbler/AudioscrobblerService.cs
> +++ b/src/Extensions/Banshee.Lastfm/Banshee.Lastfm.Audioscrobbler/AudioscrobblerService.cs
> @@ -67,6 +74,9 @@ namespace Banshee.Lastfm.Audioscrobbler
>          private bool now_playing_sent = false; /* self-explanatory :) */
>          private int iterate_countdown = 4 * 4; /* number of times to wait for iterate event before sending now playing */
>  
> +        /* contains the device uniqueIds that have been scrobbled already so we don't attempt to repeatedly submit the same songs */
> +        private List<String> submitted_devices = new List<String> ();
> +

Not sure but I think we use "string" instead of "String" in banshee codebase in general.


> @@ -136,6 +149,13 @@ namespace Banshee.Lastfm.Audioscrobbler
>                      Catalog.GetString ("Enable song reporting"), OnToggleEnabled, Enabled)
>              });
>  
> +            actions.Add (new ToggleActionEntry [] {
> +                new ToggleActionEntry ("AudioscrobblerEnableReportSinceLastSyncAction", null,
> +                    Catalog.GetString ("_Enable Reporting Of Device Plays Since Last Sync"), null,
> +                    Catalog.GetString ("Enable Reporting Of Device Plays Since Last Sync"), OnToggleDeviceScrobbleEnabled,
> +                                       DeviceScrobbleEnabled)
> +            });
> +

If the previous preference is Enable song reporting, we may still want to say "song" in the next reporting preference. So I would still say "Enable Song Reporting" and I would tweak the last part of the sentence, how about "Enable Song Reporting from Device usage at sync"?
To make the first already existing preference, you could also rename it from "Enable song reporting" to "Enable song reporting for Banshee usage" or something like that.


>              action_service.UIManager.InsertActionGroup (actions, 0);
>              ui_manager_id = action_service.UIManager.AddUiFromResource ("AudioscrobblerMenu.xml");
>  
> @@ -215,6 +235,19 @@ namespace Banshee.Lastfm.Audioscrobbler
>  
>          SongTimer st = new SongTimer ();
>  
> +        private bool isValidTrackInfoForSubmit (TrackInfo track)

The file HACKING explicits the coding guidelines we use, please refer to them (in this case, methods should have Pascal notation, that is, they cannot start with lowercase letter).


> +        {
> +            if (track.Duration.TotalSeconds > 30 &&

Please avoid magic numbers, 30 should be a private const named something like "MINIMUM_TRACK_DURATION_FOR_AUDIOSCROBBLING".


> +                (track.MediaAttributes & TrackMediaAttributes.Music) != 0 &&
> +                !String.IsNullOrEmpty (track.ArtistName) &&
> +                !String.IsNullOrEmpty (track.TrackTitle)) {
> +
> +                return true;
> +            } else {
> +                return false;
> +            }
> +        }
> +
>          private void Queue (TrackInfo track) {
>              if (track == null || st.PlayTime == 0 ||
>                  !(actions["AudioscrobblerEnableAction"] as ToggleAction).Active) {
> @@ -225,16 +258,15 @@ namespace Banshee.Lastfm.Audioscrobbler
>              Log.DebugFormat ("Track {3} had playtime of {0} msec ({4}sec), duration {1} msec, queued: {2}",
>                  st.PlayTime, track.Duration.TotalMilliseconds, queued, track, st.PlayTime / 1000);
>  
> -            if (!queued && track.Duration.TotalSeconds > 30 &&
> -                (track.MediaAttributes & TrackMediaAttributes.Music) != 0 &&
> -                !String.IsNullOrEmpty (track.ArtistName) && !String.IsNullOrEmpty (track.TrackTitle) &&
> +            if (!queued && isValidTrackInfoForSubmit(track) &&
>                  (st.PlayTime > track.Duration.TotalMilliseconds / 2 || st.PlayTime > 240 * 1000)) {
> -                    if (!connection.Started) {
> -                        connection.Start ();
> -                    }
>  
> -                    queue.Add (track, song_start_time);
> -                    queued = true;
> +                if (!connection.Started) {
> +                    connection.Start ();
> +                }
> +
> +                queue.Add (track, song_start_time);
> +                queued = true;

Oh! So this code was already there, you're just moving it around. Can you do a pre-patch for improving this? (So the removal of the magic numbers go on the refactoring pre-patch too.)


> +        private void OnToggleDeviceScrobbleEnabled (object o, EventArgs args)
> +        {
> +            DeviceScrobbleEnabled = (o as ToggleAction).Active;
> +        }

If you're not going to check for null, use static cast instead of dynamic cast please ('as' keyword). This way we would get an InvalidCastException instead of NullReferenceException if anything fails.


>  
> +        internal bool DeviceScrobbleEnabled {
> +            get { return DeviceScrobbleEnabledSchema.Get (); }
> +            set {
> +                DeviceScrobbleEnabledSchema.Set (value);
> +                (actions["AudioscrobblerEnableReportSinceLastSyncAction"] as ToggleAction).Active = value;


Same about the use of "as" keyword.


> +        // When a device is disconnected we want to remove it from the list of already-submitted devices so
> +        // that in the event that it someone disconnects their device, listens to more songs, and reconnects without
> +        // restarting the app we correctly know we need to scrobble the device again

Given this comment, I guess a better name for the variable "submitted_devices" would be "audioscrobbled_devices" maybe?

> +                if ((actions["AudioscrobblerEnableAction"] as ToggleAction).Active &&
> +                    (actions["AudioscrobblerEnableReportSinceLastSyncAction"] as ToggleAction).Active) {

Same about the use of "as" keyword.

Also, I guess a user may want to scrobble only songs from the device but in Banshee, why not? If you think this is not a use case, then you should make the AudioscrobblerDeviceEnable option be grayed when the first one is disabled.


> +
> +                        raw_device_playcount_info = null; // no reason to keep this around anymore

Don't worry about this, GC will get rid of it at some point, no need to force it.



> +                            } else {
> +                                Log.DebugFormat ("User cancelled submission, no songs submitted to Last.FM for device {0}",
> +                                                 dap_source.Name);
> +                            }
> +
> +                        } else {
> +                            Log.InformationFormat ("Found no songs to submit to Last.FM on device {0}", dap_source.Name);
> +                        }
> +
> +                    } else {
> +                        Log.DebugFormat ("Device {0} has already been successfully scrobbled since it was connected, skipping and" +
> +                                            " taking no Last.FM action", dap_source.Name);
> +                    }
> +
> +                } else {
> +                    Log.InformationFormat ("Skipping Last.FM submission from device {0} because it is disabled in preferences",
> +                                     dap_source.Name);
> +                }
> +
> +            } else {
> +                Log.Warning ("The received DAP object was null for some reason, skipping Last.FM submission.");
> +            }

Do you see all these levels of identation? It can be avoided, for example, instead of:

if (audioScrobblingEnable) {
  DoStuff ();
} else {
  Log.Information ("Scrobbling disabled");
}

You can do:

if (!audioScrobblingEnable) {
  Log.Information ("Scrobbling disabled");
  return;
}
DoStuff ();

This way the code is much more readable and there's no need to scroll horizontally so much. Please do it for all the if-else clauses here above.
Use also this trick for the ReturnPlaycountSinceLastSync method please, since that has some levels of identation as well.


>          public static readonly SchemaEntry<string> LastUserSchema = new SchemaEntry<string> (
>              "plugins.lastfm", "username", "", "Last.fm user", "Last.fm username"
>          );
> @@ -325,6 +480,13 @@ namespace Banshee.Lastfm.Audioscrobbler
>              "Audioscrobbler reporting engine enabled"
>          );
>  
> +        public static readonly SchemaEntry<bool> DeviceScrobbleEnabledSchema = new SchemaEntry<bool> (
> +            "plugins.audioscrobbler", "device_scrobble_enabled",
> +            false,
> +            "Device scrobble enabled",
> +            "Device Audioscrobbler reporting enabled"
> +        );
> +
>          string IService.ServiceName {
>              get { return "AudioscrobblerService"; }
>          }
> diff --git a/src/Extensions/Banshee.Lastfm/Banshee.Lastfm.Gui/LastFmSubmitDialog.cs b/src/Extensions/Banshee.Lastfm/Banshee.Lastfm.Gui/LastFmSubmitDialog.cs
> new file mode 100644
> index 0000000..91929e8
> --- /dev/null
> +++ b/src/Extensions/Banshee.Lastfm/Banshee.Lastfm.Gui/LastFmSubmitDialog.cs
> @@ -0,0 +1,113 @@
> +//
> +// LastFmSubmitDialog.cs
> +// 
> +// Author:
> +//   Phil Trimble <philtrimble@gmail.com>
> +// 
> +// Copyright (C) 2012 Novell, Inc

Do you work for Novell? If not, replace it with your name (or your company ;) ).


> +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> +// THE SOFTWARE.
> +using System;

Add a blank line between the license and the usings-block please.
> diff --git a/src/Extensions/Banshee.Lastfm/Banshee.Lastfm.csproj b/src/Extensions/Banshee.Lastfm/Banshee.Lastfm.csproj
> index 6d1f522..694fb01 100644
> --- a/src/Extensions/Banshee.Lastfm/Banshee.Lastfm.csproj
> +++ b/src/Extensions/Banshee.Lastfm/Banshee.Lastfm.csproj
> @@ -37,18 +37,23 @@
>      <Reference Include="System.Xml" />
>      <Reference Include="atk-sharp">
>        <SpecificVersion>False</SpecificVersion>
> +      <Package>gtk-sharp-2.0</Package>
>      </Reference>
>      <Reference Include="gdk-sharp">
>        <SpecificVersion>False</SpecificVersion>
> +      <Package>gtk-sharp-2.0</Package>
>      </Reference>
>      <Reference Include="pango-sharp">
>        <SpecificVersion>False</SpecificVersion>
> +      <Package>gtk-sharp-2.0</Package>
>      </Reference>
>      <Reference Include="glib-sharp">
>        <SpecificVersion>False</SpecificVersion>
> +      <Package>glib-sharp-2.0</Package>
>      </Reference>
>      <Reference Include="gtk-sharp">
>        <SpecificVersion>False</SpecificVersion>
> +      <Package>gtk-sharp-2.0</Package>
>      </Reference>
>      <Reference Include="Mono.Posix">
>        <SpecificVersion>False</SpecificVersion>
> @@ -61,6 +66,7 @@
>      <Reference Include="Mono.Addins">
>        <SpecificVersion>False</SpecificVersion>
>        <HintPath>..\..\..\bin\bin\Mono.Addins.dll</HintPath>
> +      <Package>mono-addins</Package>
>      </Reference>
>    </ItemGroup>
>    <ItemGroup>

Sometimes MonoDevelop does funky things with the projects without the user noticing. Sometimes these are valid changes, but as they're not related to the patch, they should be separated.


> @@ -106,6 +112,22 @@
>        <Project>{C856EFD8-E812-4E61-8B76-E3583D94C233}</Project>
>        <Name>Hyena.Gui</Name>
>      </ProjectReference>
> +    <ProjectReference Include="..\..\Dap\Banshee.Dap\Banshee.Dap.csproj">
> +      <Project>{BC2E94DF-7A82-461E-BE7C-60E41ADC3562}</Project>
> +      <Name>Banshee.Dap</Name>
> +    </ProjectReference>
> +    <ProjectReference Include="..\..\Dap\Banshee.Dap.AppleDevice\Banshee.Dap.AppleDevice.csproj">
> +      <Project>{DEADBEEF-CAFE-BABE-FACE-000C0FFEE000}</Project>
> +      <Name>Banshee.Dap.AppleDevice</Name>
> +    </ProjectReference>
> +    <ProjectReference Include="..\..\Dap\Banshee.Dap.MassStorage\Banshee.Dap.MassStorage.csproj">
> +      <Project>{6B73E278-23FB-4A59-9B44-AB7F0212B936}</Project>
> +      <Name>Banshee.Dap.MassStorage</Name>
> +    </ProjectReference>
> +    <ProjectReference Include="..\..\Dap\Banshee.Dap.Mtp\Banshee.Dap.Mtp.csproj">
> +      <Project>{3935AE8A-E283-4C0D-9094-7435A937DC90}</Project>
> +      <Name>Banshee.Dap.Mtp</Name>
> +    </ProjectReference>

Are you sure do you need all these references there? I think you only need the Dap one...

>    </ItemGroup>
>    <ItemGroup>
>      <Compile Include="Banshee.Lastfm.Audioscrobbler\AudioscrobblerService.cs" />
> diff --git a/src/Extensions/Banshee.Lastfm/Banshee.Lastfm/LastfmPreferences.cs b/src/Extensions/Banshee.Lastfm/Banshee.Lastfm/LastfmPreferences.cs
> index 0a1261d..a5593b4 100644
> --- a/src/Extensions/Banshee.Lastfm/Banshee.Lastfm/LastfmPreferences.cs
> +++ b/src/Extensions/Banshee.Lastfm/Banshee.Lastfm/LastfmPreferences.cs
> @@ -49,6 +49,7 @@ namespace Banshee.Lastfm
>          private Section prefs_section;
>          private SchemaPreference<string> username_preference;
>          private Preference<bool> reporting_preference;
> +        private Preference<bool> reporting_device_plays_since_last_sync_preference;

woah! Let's try to come up with a shorter name :) How about reporting_device_preference (as reporting_preference doesn't mention what it reports anyway...).


> diff --git a/src/Extensions/Banshee.Lastfm/Makefile.am b/src/Extensions/Banshee.Lastfm/Makefile.am
> index 1e79fda..94a2803 100644
> --- a/src/Extensions/Banshee.Lastfm/Makefile.am
> +++ b/src/Extensions/Banshee.Lastfm/Makefile.am
> @@ -1,11 +1,12 @@
>  ASSEMBLY = Banshee.Lastfm
>  TARGET = library
> -LINK = $(REF_EXTENSION_LASTFM) -r:Mono.Security
> +LINK = $(REF_EXTENSION_LASTFM) -r:Mono.Security $(LINK_DAP)
>  INSTALL_DIR = $(EXTENSIONS_INSTALL_DIR)
>  
>  SOURCES =  \
>  	Banshee.Lastfm.Audioscrobbler/AudioscrobblerService.cs \
>  	Banshee.Lastfm.Audioscrobbler/Queue.cs \
> +	Banshee.Lastfm.Gui/LastFmSubmitDialog.cs \
>  	Banshee.Lastfm.Recommendations/ContextPage.cs \
>  	Banshee.Lastfm.Recommendations/RecommendationPane.cs \
>  	Banshee.Lastfm.Recommendations/SimilarArtistTile.cs \
> diff --git a/src/Extensions/Banshee.Lastfm/Resources/AudioscrobblerMenu.xml b/src/Extensions/Banshee.Lastfm/Resources/AudioscrobblerMenu.xml
> index bde075c..0525b8a 100644
> --- a/src/Extensions/Banshee.Lastfm/Resources/AudioscrobblerMenu.xml
> +++ b/src/Extensions/Banshee.Lastfm/Resources/AudioscrobblerMenu.xml
> @@ -3,6 +3,7 @@
>          <menu name="ToolsMenu" action="ToolsMenuAction">
>              <menu name="Audioscrobbler" action="AudioscrobblerAction">
>                      <menuitem name="AudioscrobblerEnable" action="AudioscrobblerEnableAction" />
> +                    <menuitem name="AudioscrobblerEnableReportSinceLastSync" action="AudioscrobblerEnableReportSinceLastSyncAction" />

Let's try shortening again, how about AudioscrobblerDeviceEnable? (Since AudioscrobblerEnable, the previous one, is not mentioning "Report" anyway, and there's no need to be so specific about "since when" here I think.
Comment 22 Andrés G. Aragoneses (IRC: knocte) 2012-02-05 01:18:50 UTC
Comment on attachment 206494 [details] [review]
Enables DAP scrobbling support


Awesome patch Phil!

I'm going to do a first review. Please don't get scared with it. Most of it will be nitpicks that are easy to fix and questions you can follow-up with.

I'll do it inline:

> diff --git a/help/C/lastfm.page b/help/C/lastfm.page
> index 16db221..4dc76e1 100644
> --- a/help/C/lastfm.page
> +++ b/help/C/lastfm.page
> @@ -12,6 +12,7 @@
>        <name>Paul Cutler</name>
>        <email>pcutler@gnome.org</email>
>      </credit>    
> +
>  <!--
>      <copyright>
>        <year>2010</year>

Please drop this kind of superflous changes from the patch.


> @@ -37,8 +38,8 @@
>    Visit <link href="http://www.last.fm/join">http://www.last.fm/join</link>
>     to create an account or choose 
>    <guiseq><gui>Edit</gui><gui>Preferences</gui></guiseq> from the Banshee
> -  menu.  Then press the <gui>Source Specific</gui> tab and press the
> -  <gui>Source</gui> drop down menu and choose <gui>Last.fm</gui> and select the
> +  menu. Once in the preferences select the <gui>Source Specific</gui> tab, press the
> +  <gui>Source</gui> drop down menu, choose <gui>Last.fm</gui> and finally select the
>    <em>Sign up for Last.fm</em> link.
>    </p>

This change is not related to this bug, right? Please put separate things in separate patches :)


> @@ -56,18 +57,49 @@
>    
>    <section id="songreporting">
>    <title>Enable Last.fm Song Reporting</title> 
> -  <p>After you have successfully linked Banshee to your Last.fm profile, to 
> -  enable Banshee to report the songs to your Last.fm profile, in the 
> -  <gui>Source Specific</gui> tab in Banshee's preferences, press the 
> -  <gui>Enable Song Reporting</gui> checkbox.  If you have an active internet
> -  connection, Banshee will now send Last.fm information regarding the songs
> -  you play.  To view your play history, visit your profile on the Last.fm 
> +  <p>After you have successfully linked Banshee to your Last.fm profile you must 
> +  ensure that you have enabled Banshee to report your songs.  To enable Banshee to 
> +  report the songs to your Last.fm profile go to Banshee's preferences, select the
> +  <gui>Source Specific</gui> tab, select <gui>Last.fm</gui> from the dropdown, and 
> +  press the <gui>Enable Song Reporting</gui> checkbox.
> +  If you have an active internet connection Banshee will now send Last.fm information
> +  regarding the songs you play. To view your play history visit your profile on the Last.fm
>    website.  Last.fm will automatically update your music metadata if any of your
> -  artist, song title or album information is incorrect.
> +  artist, song title, or album information is incorrect.

These seem like good improvements but not related to DAP Audioscrobbling, right?


>    </p>
>    
>    </section>
> -  
> +
> +  <section id="devicesongreporting">
> +  <title>Enable Last.fm Song Reporting From Your Device</title>
> +  <p>After succesfully linking Banshee to your Last.fm profile and enabling Banshee to 
> +  report songs to Last.fm you can also enable scrobbling from a connected device. Banshee 
> +  will, upon sync of your device, attempt to scrobble the songs you have played since the 
> +  device was last connected and submit them to Last.fm.
> +  </p>
> +
> +  <p>To enable scrobbling of a connected device go to Banshee's preferences, select the 
> +  <gui>Source Specific</gui> tab, select <gui>Last.fm</gui> from the dropdown, and press 
> +  the <gui>Enable Reporting Of Device Plays Since Last Sync</gui> checkbox.  If you have an 

Is "Device Plays" a good name? I ask because I'm not a native English speaker, are you?


> +  active internet connection and the <gui>Enable Song Reporting</gui> checkbox selected 
> +  Banshee will now attempt to gather information regarding the songs that you have played 
> +  on your device since your last sync and display them in a dialog.  You can then select 

Do we really want a dialog every time the sync happens? If people are interested in this feature, they could just enable it, and DAP Audioscrobbling will happen always. If they don't like it they just re-disable it again. Why confirm the action in an every-sync basis? What is the exact use case of this granularity?


> +  whether to submit or cancel.  This process will only take place upon a sync in Banshee.
> +  </p>
> +
> +  <p>As with regular Banshee scrobbling submissions, Last.fm will automatically update 
> +  your music metadata if any of your artist, title, or album information is incorrect.
> +  </p>

I would add something here like "(although we recommend you use the Metadata Fixer extension
to correct your files)".


> +
> +  <p>Please note that currently Banshee only supports this feature with Apple devices. 

"With the Apple devices that the AppleDevice extension currently supports".


> +  Any Apple device supported by Banshee for syncing will be scrobbled. Also note that, due 
> +  to the design of Apple devices, once you sync and close Banshee your 'recently played' 
> +  information will be lost. You must submit your songs to Last.fm at the time of your sync
> +  and before you close Banshee.

Cannot we consider this a bug? I mean, shouldn't we show the user a warning dialog if they want to close Banshee and the scrobbling didn't finish yet? (In the same way we already present a warning if sync didn't finish and a user wants to exit Banshee)



> diff --git a/src/Core/Banshee.Core/Banshee.Collection/TrackInfo.cs b/src/Core/Banshee.Core/Banshee.Collection/TrackInfo.cs
> index 8ecefc7..4da103b 100644
> --- a/src/Core/Banshee.Core/Banshee.Collection/TrackInfo.cs
> +++ b/src/Core/Banshee.Core/Banshee.Collection/TrackInfo.cs
> @@ -319,6 +319,9 @@ namespace Banshee.Collection
>          public virtual int PlayCount { get; set; }
>  
>          [Exportable]
> +        public virtual int RecentPlayCount {get; set; }

Do we really need this property in this class? I've seen that you use it in AppleDeviceSource but you only read it, then you store it in AppleDeviceTrackInfo but I think that doesn't get read from that class anymore.


> diff --git a/src/Dap/Banshee.Dap.AppleDevice/Banshee.Dap.AppleDevice/AppleDeviceSource.cs b/src/Dap/Banshee.Dap.AppleDevice/Banshee.Dap.AppleDevice/AppleDeviceSource.cs
> index bbe3773..553e834 100644
> --- a/src/Dap/Banshee.Dap.AppleDevice/Banshee.Dap.AppleDevice/AppleDeviceSource.cs
> +++ b/src/Dap/Banshee.Dap.AppleDevice/Banshee.Dap.AppleDevice/AppleDeviceSource.cs
> @@ -42,6 +42,7 @@ using Banshee.Hardware;
> +                        // Since the ipod doesn't keep track of individual plays we need to calculate
> +                        // a new fudged last play datetime for each play if we have more than one play.
> +                        // Note that the final play timestamp is entirely fudged. We don't care if we
> +                        // are accurate in terms of datetime, only whether our overall playcount is correct.

Could you add this timestamp-fudging logic into its own method (so the method name also explains the intent and then the comment you need to write is smaller?)?

> +                        if (i == 0) {
> +                            played_track.LastPlayed = most_recent_playtime;
> +                        } else {
> +                            played_track.LastPlayed = most_recent_playtime - played_track.Duration;
> +                            most_recent_playtime = played_track.LastPlayed;
> +                        }
> +
> +                        recently_played_tracks_list.Add (played_track);
> +                    }
> +                }
> +            }
> +
> +            // Sorts the played tracks from oldest to newest
> +            recently_played_tracks_list.Sort (delegate (TrackInfo t1, TrackInfo t2) { return t1.LastPlayed.CompareTo (t2.LastPlayed); });

Why is this sorting done? (Please try to put comments explaining the why, not the what.)



> diff --git a/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage/MassStorageSource.cs b/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage/MassStorageSource.cs
> index 3e23b30..102abd4 100644
> --- a/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage/MassStorageSource.cs
> +++ b/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage/MassStorageSource.cs
> @@ -720,5 +720,18 @@ namespace Banshee.Dap.MassStorage
>          public override bool HasEditableTrackProperties {
>              get { return true; }
>          }
> +
> +        /*
> +         * This method is not supported at this point. This is because, according to my potentially
> +         * out of date information, all devices outside of Apple devices do not contain 'last played' info
> +         * and therefore cannot implement any kind of 'what has played since last sync' functionality.
> +         * If this is not true then this logic can be filled in and the calling methods can process it
> +         * accordingly. - Phil Trimble, 2012/01/19, philtrimble@gmail.com
> +         */

Why not, instead of having an abstract method and have this empty method and huge comment in every non-AppleDeviceSource derived class, have instead a *virtual* empty method that only AppleDeviceSource overrides?


> diff --git a/src/Dap/Banshee.Dap/Banshee.Dap/DapService.cs b/src/Dap/Banshee.Dap/Banshee.Dap/DapService.cs
> index 93e6a63..b5904be 100644
> --- a/src/Dap/Banshee.Dap/Banshee.Dap/DapService.cs
> +++ b/src/Dap/Banshee.Dap/Banshee.Dap/DapService.cs
> @@ -54,6 +54,8 @@ namespace Banshee.Dap
>          private bool initialized;
>          private object sync = new object ();
>  
> +        public event SourceEventHandler DapSourceAdded;

Why not use SourceAdded event which is already there, and check if the source that has been added is a DapSource.

  
> diff --git a/src/Dap/Banshee.Dap/Banshee.Dap/DapSource.cs b/src/Dap/Banshee.Dap/Banshee.Dap/DapSource.cs
> index e5e23e7..3680f4c 100644
> --- a/src/Dap/Banshee.Dap/Banshee.Dap/DapSource.cs
> +++ b/src/Dap/Banshee.Dap/Banshee.Dap/DapSource.cs
> @@ -59,6 +59,8 @@ namespace Banshee.Dap
>          private Page page;
>          // private DapPropertiesDisplay dap_properties_display;
>  
> +        public event EventHandler DapSourceDisposed;
> +

Same suggestion but with generic SourceRemoved (or SourceDisposed, haven't looked at the code to check the real name).



> diff --git a/src/Dap/Banshee.Dap/Banshee.Dap/DapSync.cs b/src/Dap/Banshee.Dap/Banshee.Dap/DapSync.cs
> index ceeb233..e7269a4 100644
> --- a/src/Dap/Banshee.Dap/Banshee.Dap/DapSync.cs
> +++ b/src/Dap/Banshee.Dap/Banshee.Dap/DapSync.cs
> @@ -65,6 +65,7 @@ namespace Banshee.Dap
>          public event Action<DapSync> Updated;
>          public event Action<DapLibrarySync> LibraryAdded;
>          public event Action<DapLibrarySync> LibraryRemoved;
> +        public event Action<DapSync> SyncStarted;

I think DapSync is a class used only for when AutoSync mode is enabled in the device, have you tested your patch when using Manual mode?
If your patch doesn't work in Manual mode, you may want to hook in a different place a SyncStarted event, not in this class (but now that I think about it, do we really want to hook it up when Sync starts or only when a device is detected for the first time? I guess that this is the reason of why you need the submitted_devices variable in the next parts of the diff... so maybe if you can find a place where the detection of a device happens, you would only hook to that event and you wouldn't need the submitted_devices variable).


> diff --git a/src/Extensions/Banshee.Lastfm/Banshee.Lastfm.Audioscrobbler/AudioscrobblerService.cs b/src/Extensions/Banshee.Lastfm/Banshee.Lastfm.Audioscrobbler/AudioscrobblerService.cs
> index 5e0758f..3fa78f5 100644
> --- a/src/Extensions/Banshee.Lastfm/Banshee.Lastfm.Audioscrobbler/AudioscrobblerService.cs
> +++ b/src/Extensions/Banshee.Lastfm/Banshee.Lastfm.Audioscrobbler/AudioscrobblerService.cs
> @@ -67,6 +74,9 @@ namespace Banshee.Lastfm.Audioscrobbler
>          private bool now_playing_sent = false; /* self-explanatory :) */
>          private int iterate_countdown = 4 * 4; /* number of times to wait for iterate event before sending now playing */
>  
> +        /* contains the device uniqueIds that have been scrobbled already so we don't attempt to repeatedly submit the same songs */
> +        private List<String> submitted_devices = new List<String> ();
> +

Not sure but I think we use "string" instead of "String" in banshee codebase in general.


> @@ -136,6 +149,13 @@ namespace Banshee.Lastfm.Audioscrobbler
>                      Catalog.GetString ("Enable song reporting"), OnToggleEnabled, Enabled)
>              });
>  
> +            actions.Add (new ToggleActionEntry [] {
> +                new ToggleActionEntry ("AudioscrobblerEnableReportSinceLastSyncAction", null,
> +                    Catalog.GetString ("_Enable Reporting Of Device Plays Since Last Sync"), null,
> +                    Catalog.GetString ("Enable Reporting Of Device Plays Since Last Sync"), OnToggleDeviceScrobbleEnabled,
> +                                       DeviceScrobbleEnabled)
> +            });
> +

If the previous preference is Enable song reporting, we may still want to say "song" in the next reporting preference. So I would still say "Enable Song Reporting" and I would tweak the last part of the sentence, how about "Enable Song Reporting from Device usage at sync"?
To make the first already existing preference, you could also rename it from "Enable song reporting" to "Enable song reporting for Banshee usage" or something like that.


>              action_service.UIManager.InsertActionGroup (actions, 0);
>              ui_manager_id = action_service.UIManager.AddUiFromResource ("AudioscrobblerMenu.xml");
>  
> @@ -215,6 +235,19 @@ namespace Banshee.Lastfm.Audioscrobbler
>  
>          SongTimer st = new SongTimer ();
>  
> +        private bool isValidTrackInfoForSubmit (TrackInfo track)

The file HACKING explicits the coding guidelines we use, please refer to them (in this case, methods should have Pascal notation, that is, they cannot start with lowercase letter).


> +        {
> +            if (track.Duration.TotalSeconds > 30 &&

Please avoid magic numbers, 30 should be a private const named something like "MINIMUM_TRACK_DURATION_FOR_AUDIOSCROBBLING".


> +                (track.MediaAttributes & TrackMediaAttributes.Music) != 0 &&
> +                !String.IsNullOrEmpty (track.ArtistName) &&
> +                !String.IsNullOrEmpty (track.TrackTitle)) {
> +
> +                return true;
> +            } else {
> +                return false;
> +            }
> +        }
> +
>          private void Queue (TrackInfo track) {
>              if (track == null || st.PlayTime == 0 ||
>                  !(actions["AudioscrobblerEnableAction"] as ToggleAction).Active) {
> @@ -225,16 +258,15 @@ namespace Banshee.Lastfm.Audioscrobbler
>              Log.DebugFormat ("Track {3} had playtime of {0} msec ({4}sec), duration {1} msec, queued: {2}",
>                  st.PlayTime, track.Duration.TotalMilliseconds, queued, track, st.PlayTime / 1000);
>  
> -            if (!queued && track.Duration.TotalSeconds > 30 &&
> -                (track.MediaAttributes & TrackMediaAttributes.Music) != 0 &&
> -                !String.IsNullOrEmpty (track.ArtistName) && !String.IsNullOrEmpty (track.TrackTitle) &&
> +            if (!queued && isValidTrackInfoForSubmit(track) &&
>                  (st.PlayTime > track.Duration.TotalMilliseconds / 2 || st.PlayTime > 240 * 1000)) {
> -                    if (!connection.Started) {
> -                        connection.Start ();
> -                    }
>  
> -                    queue.Add (track, song_start_time);
> -                    queued = true;
> +                if (!connection.Started) {
> +                    connection.Start ();
> +                }
> +
> +                queue.Add (track, song_start_time);
> +                queued = true;

Oh! So this code was already there, you're just moving it around. Can you do a pre-patch for improving this? (So the removal of the magic numbers go on the refactoring pre-patch too.)


> +        private void OnToggleDeviceScrobbleEnabled (object o, EventArgs args)
> +        {
> +            DeviceScrobbleEnabled = (o as ToggleAction).Active;
> +        }

If you're not going to check for null, use static cast instead of dynamic cast please ('as' keyword). This way we would get an InvalidCastException instead of NullReferenceException if anything fails.


>  
> +        internal bool DeviceScrobbleEnabled {
> +            get { return DeviceScrobbleEnabledSchema.Get (); }
> +            set {
> +                DeviceScrobbleEnabledSchema.Set (value);
> +                (actions["AudioscrobblerEnableReportSinceLastSyncAction"] as ToggleAction).Active = value;


Same about the use of "as" keyword.


> +        // When a device is disconnected we want to remove it from the list of already-submitted devices so
> +        // that in the event that it someone disconnects their device, listens to more songs, and reconnects without
> +        // restarting the app we correctly know we need to scrobble the device again

Given this comment, I guess a better name for the variable "submitted_devices" would be "audioscrobbled_devices" maybe?

> +                if ((actions["AudioscrobblerEnableAction"] as ToggleAction).Active &&
> +                    (actions["AudioscrobblerEnableReportSinceLastSyncAction"] as ToggleAction).Active) {

Same about the use of "as" keyword.

Also, I guess a user may want to scrobble only songs from the device but in Banshee, why not? If you think this is not a use case, then you should make the AudioscrobblerDeviceEnable option be grayed when the first one is disabled.


> +
> +                        raw_device_playcount_info = null; // no reason to keep this around anymore

Don't worry about this, GC will get rid of it at some point, no need to force it.



> +                            } else {
> +                                Log.DebugFormat ("User cancelled submission, no songs submitted to Last.FM for device {0}",
> +                                                 dap_source.Name);
> +                            }
> +
> +                        } else {
> +                            Log.InformationFormat ("Found no songs to submit to Last.FM on device {0}", dap_source.Name);
> +                        }
> +
> +                    } else {
> +                        Log.DebugFormat ("Device {0} has already been successfully scrobbled since it was connected, skipping and" +
> +                                            " taking no Last.FM action", dap_source.Name);
> +                    }
> +
> +                } else {
> +                    Log.InformationFormat ("Skipping Last.FM submission from device {0} because it is disabled in preferences",
> +                                     dap_source.Name);
> +                }
> +
> +            } else {
> +                Log.Warning ("The received DAP object was null for some reason, skipping Last.FM submission.");
> +            }

Do you see all these levels of identation? It can be avoided, for example, instead of:

if (audioScrobblingEnable) {
  DoStuff ();
} else {
  Log.Information ("Scrobbling disabled");
}

You can do:

if (!audioScrobblingEnable) {
  Log.Information ("Scrobbling disabled");
  return;
}
DoStuff ();

This way the code is much more readable and there's no need to scroll horizontally so much. Please do it for all the if-else clauses here above.
Use also this trick for the ReturnPlaycountSinceLastSync method please, since that has some levels of identation as well.


>          public static readonly SchemaEntry<string> LastUserSchema = new SchemaEntry<string> (
>              "plugins.lastfm", "username", "", "Last.fm user", "Last.fm username"
>          );
> @@ -325,6 +480,13 @@ namespace Banshee.Lastfm.Audioscrobbler
>              "Audioscrobbler reporting engine enabled"
>          );
>  
> +        public static readonly SchemaEntry<bool> DeviceScrobbleEnabledSchema = new SchemaEntry<bool> (
> +            "plugins.audioscrobbler", "device_scrobble_enabled",
> +            false,
> +            "Device scrobble enabled",
> +            "Device Audioscrobbler reporting enabled"
> +        );
> +
>          string IService.ServiceName {
>              get { return "AudioscrobblerService"; }
>          }
> diff --git a/src/Extensions/Banshee.Lastfm/Banshee.Lastfm.Gui/LastFmSubmitDialog.cs b/src/Extensions/Banshee.Lastfm/Banshee.Lastfm.Gui/LastFmSubmitDialog.cs
> new file mode 100644
> index 0000000..91929e8
> --- /dev/null
> +++ b/src/Extensions/Banshee.Lastfm/Banshee.Lastfm.Gui/LastFmSubmitDialog.cs
> @@ -0,0 +1,113 @@
> +//
> +// LastFmSubmitDialog.cs
> +// 
> +// Author:
> +//   Phil Trimble <philtrimble@gmail.com>
> +// 
> +// Copyright (C) 2012 Novell, Inc

Do you work for Novell? If not, replace it with your name (or your company ;) ).


> +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> +// THE SOFTWARE.
> +using System;

Add a blank line between the license and the usings-block please.
> diff --git a/src/Extensions/Banshee.Lastfm/Banshee.Lastfm.csproj b/src/Extensions/Banshee.Lastfm/Banshee.Lastfm.csproj
> index 6d1f522..694fb01 100644
> --- a/src/Extensions/Banshee.Lastfm/Banshee.Lastfm.csproj
> +++ b/src/Extensions/Banshee.Lastfm/Banshee.Lastfm.csproj
> @@ -37,18 +37,23 @@
>      <Reference Include="System.Xml" />
>      <Reference Include="atk-sharp">
>        <SpecificVersion>False</SpecificVersion>
> +      <Package>gtk-sharp-2.0</Package>
>      </Reference>
>      <Reference Include="gdk-sharp">
>        <SpecificVersion>False</SpecificVersion>
> +      <Package>gtk-sharp-2.0</Package>
>      </Reference>
>      <Reference Include="pango-sharp">
>        <SpecificVersion>False</SpecificVersion>
> +      <Package>gtk-sharp-2.0</Package>
>      </Reference>
>      <Reference Include="glib-sharp">
>        <SpecificVersion>False</SpecificVersion>
> +      <Package>glib-sharp-2.0</Package>
>      </Reference>
>      <Reference Include="gtk-sharp">
>        <SpecificVersion>False</SpecificVersion>
> +      <Package>gtk-sharp-2.0</Package>
>      </Reference>
>      <Reference Include="Mono.Posix">
>        <SpecificVersion>False</SpecificVersion>
> @@ -61,6 +66,7 @@
>      <Reference Include="Mono.Addins">
>        <SpecificVersion>False</SpecificVersion>
>        <HintPath>..\..\..\bin\bin\Mono.Addins.dll</HintPath>
> +      <Package>mono-addins</Package>
>      </Reference>
>    </ItemGroup>
>    <ItemGroup>

Sometimes MonoDevelop does funky things with the projects without the user noticing. Sometimes these are valid changes, but as they're not related to the patch, they should be separated.


> @@ -106,6 +112,22 @@
>        <Project>{C856EFD8-E812-4E61-8B76-E3583D94C233}</Project>
>        <Name>Hyena.Gui</Name>
>      </ProjectReference>
> +    <ProjectReference Include="..\..\Dap\Banshee.Dap\Banshee.Dap.csproj">
> +      <Project>{BC2E94DF-7A82-461E-BE7C-60E41ADC3562}</Project>
> +      <Name>Banshee.Dap</Name>
> +    </ProjectReference>
> +    <ProjectReference Include="..\..\Dap\Banshee.Dap.AppleDevice\Banshee.Dap.AppleDevice.csproj">
> +      <Project>{DEADBEEF-CAFE-BABE-FACE-000C0FFEE000}</Project>
> +      <Name>Banshee.Dap.AppleDevice</Name>
> +    </ProjectReference>
> +    <ProjectReference Include="..\..\Dap\Banshee.Dap.MassStorage\Banshee.Dap.MassStorage.csproj">
> +      <Project>{6B73E278-23FB-4A59-9B44-AB7F0212B936}</Project>
> +      <Name>Banshee.Dap.MassStorage</Name>
> +    </ProjectReference>
> +    <ProjectReference Include="..\..\Dap\Banshee.Dap.Mtp\Banshee.Dap.Mtp.csproj">
> +      <Project>{3935AE8A-E283-4C0D-9094-7435A937DC90}</Project>
> +      <Name>Banshee.Dap.Mtp</Name>
> +    </ProjectReference>

Are you sure do you need all these references there? I think you only need the Dap one...

>    </ItemGroup>
>    <ItemGroup>
>      <Compile Include="Banshee.Lastfm.Audioscrobbler\AudioscrobblerService.cs" />
> diff --git a/src/Extensions/Banshee.Lastfm/Banshee.Lastfm/LastfmPreferences.cs b/src/Extensions/Banshee.Lastfm/Banshee.Lastfm/LastfmPreferences.cs
> index 0a1261d..a5593b4 100644
> --- a/src/Extensions/Banshee.Lastfm/Banshee.Lastfm/LastfmPreferences.cs
> +++ b/src/Extensions/Banshee.Lastfm/Banshee.Lastfm/LastfmPreferences.cs
> @@ -49,6 +49,7 @@ namespace Banshee.Lastfm
>          private Section prefs_section;
>          private SchemaPreference<string> username_preference;
>          private Preference<bool> reporting_preference;
> +        private Preference<bool> reporting_device_plays_since_last_sync_preference;

woah! Let's try to come up with a shorter name :) How about reporting_device_preference (as reporting_preference doesn't mention what it reports anyway...).


> diff --git a/src/Extensions/Banshee.Lastfm/Makefile.am b/src/Extensions/Banshee.Lastfm/Makefile.am
> index 1e79fda..94a2803 100644
> --- a/src/Extensions/Banshee.Lastfm/Makefile.am
> +++ b/src/Extensions/Banshee.Lastfm/Makefile.am
> @@ -1,11 +1,12 @@
>  ASSEMBLY = Banshee.Lastfm
>  TARGET = library
> -LINK = $(REF_EXTENSION_LASTFM) -r:Mono.Security
> +LINK = $(REF_EXTENSION_LASTFM) -r:Mono.Security $(LINK_DAP)
>  INSTALL_DIR = $(EXTENSIONS_INSTALL_DIR)
>  
>  SOURCES =  \
>  	Banshee.Lastfm.Audioscrobbler/AudioscrobblerService.cs \
>  	Banshee.Lastfm.Audioscrobbler/Queue.cs \
> +	Banshee.Lastfm.Gui/LastFmSubmitDialog.cs \
>  	Banshee.Lastfm.Recommendations/ContextPage.cs \
>  	Banshee.Lastfm.Recommendations/RecommendationPane.cs \
>  	Banshee.Lastfm.Recommendations/SimilarArtistTile.cs \
> diff --git a/src/Extensions/Banshee.Lastfm/Resources/AudioscrobblerMenu.xml b/src/Extensions/Banshee.Lastfm/Resources/AudioscrobblerMenu.xml
> index bde075c..0525b8a 100644
> --- a/src/Extensions/Banshee.Lastfm/Resources/AudioscrobblerMenu.xml
> +++ b/src/Extensions/Banshee.Lastfm/Resources/AudioscrobblerMenu.xml
> @@ -3,6 +3,7 @@
>          <menu name="ToolsMenu" action="ToolsMenuAction">
>              <menu name="Audioscrobbler" action="AudioscrobblerAction">
>                      <menuitem name="AudioscrobblerEnable" action="AudioscrobblerEnableAction" />
> +                    <menuitem name="AudioscrobblerEnableReportSinceLastSync" action="AudioscrobblerEnableReportSinceLastSyncAction" />

Let's try shortening again, how about AudioscrobblerDeviceEnable? (Since AudioscrobblerEnable, the previous one, is not mentioning "Report" anyway, and there's no need to be so specific about "since when" here I think.
Comment 23 Andrés G. Aragoneses (IRC: knocte) 2012-02-05 01:27:55 UTC
Ooops, sorry about the doble comment (bugzilla, you should disable buttons at submission to prevent accidental double-clicks...). Also I made a typo: when I said "To make the first already existing preference, you could also rename it from..." I meant "To make the first already existing preference *clearer*, you could also rename it from..."

Cheers, looking forward for the next version of the patch.
Comment 24 Phil Trimble 2012-02-05 02:03:32 UTC
Hi Andres,

Thanks for the great comments! I appreciate you taking the time. I will sift through them and make changes as necessary. I do have four bigger questions/comments, though, before I start on it:

1) I did get a little carried away with the help/C/lastfm.page changes. I can easily split up the changes. So would it be the correct procedure for me to create a new enhancement for updates to the unrelated Last.fm section and attach a patch there and then include the updates for the device scrobbling here?

2) About the dialog window: my thought behind it was to try to mimic the behavior of the last.fm client in Windows with iTunes. It pops up a dialog to show you what it is about to submit. Also, in the official client you can actually de-select tracks for submission. I didn't go that far but the only purpose was to mimic how the official client works. 

Personally I like the idea of the popup and knowing what I am submitting. If you (or others) think that it is an unnecessary idea then just say the word and I can take it out and let the scrobbling happen behind the scenes.

3) I was afraid someone would say something about the play information being removed upon sync. I could see how we could consider it a bug. If we decide to take away the dialog then the user won't even have the option of 'cancelling' a submission so this would be less of an issue. I can think of ways to handle it, though, so information is not lost. 

Chalk this up to me being scared to tackle it because I thought it was too complicated. ;)

4) About DapSync...I have only tested with 'Sync Entire Library' enabled. I didn't think about someone who was managing everything manually. In that case I think it would be better if I did this on device load instead of sync.

Thanks again for your help!
Comment 25 Andrés G. Aragoneses (IRC: knocte) 2012-02-05 03:28:29 UTC
(In reply to comment #24)
> 1) I did get a little carried away with the help/C/lastfm.page changes. I can
> easily split up the changes. So would it be the correct procedure for me to
> create a new enhancement for updates to the unrelated Last.fm section and
> attach a patch there and then include the updates for the device scrobbling
> here?

You can do that. Or given that the patch for this would be small and straightforward to review, you could just pastebin it and show it to some maintainer on IRC to have a quick review (and then you can commit it or he can commit on your behalf).


> 2) About the dialog window: my thought behind it was to try to mimic the
> behavior of the last.fm client in Windows with iTunes. It pops up a dialog to
> show you what it is about to submit. Also, in the official client you can
> actually de-select tracks for submission. I didn't go that far but the only
> purpose was to mimic how the official client works. 
> 
> Personally I like the idea of the popup and knowing what I am submitting. If
> you (or others) think that it is an unnecessary idea then just say the word and
> I can take it out and let the scrobbling happen behind the scenes.

Interesting, didn't know LastFM client showed the warning everytime. We can explore the idea but my recommendation is that it gets splitted in a 2nd patch, so we discuss it later and so the 1st patch is easier to review as it adds less "stuff" :)

> 3) I was afraid someone would say something about the play information being
> removed upon sync. I could see how we could consider it a bug. If we decide to
> take away the dialog then the user won't even have the option of 'cancelling' a
> submission so this would be less of an issue. I can think of ways to handle it,
> though, so information is not lost. 

Not sure I follow. Do you mean "upon Banshee exit while sync is happening"? If yes, then I still think this would be an issue even if you present the confirmation-summary-dialog of the changes to be scrobbled, because he still may exit Banshee after he presses "Ok", without the sync being finished.


> Chalk this up to me being scared to tackle it because I thought it was too
> complicated. ;)

I wouldn't worry so much. Just take a look at how it is done for a normal sync (try to interrumpt a local sync yourself by quitting banshee and you'll see a dialog, then from the text of that dialog you can find the piece of code that handles it in the codebase).


> 4) About DapSync...I have only tested with 'Sync Entire Library' enabled. I
> didn't think about someone who was managing everything manually. In that case I
> think it would be better if I did this on device load instead of sync.

Yes please the more use cases we cover the better.
Thanks for your effort!
Comment 26 Phil Trimble 2012-02-12 01:17:14 UTC
Created attachment 207375 [details] [review]
Refactoring of AudioscrobblerService

Finally had time to work on this! Here is the pre-patch based on suggestions from Andrés. It moves magic numbers into constants, moves the track validation logic into a separate method for later general use, and changes all dynamic casts to static casts.

It does not implement any of the DAP scrobbling functionality from the other patch. Those changes are moving forward and should be done soon.
Comment 27 Andrés G. Aragoneses (IRC: knocte) 2012-02-13 15:35:00 UTC
Review of attachment 207375 [details] [review]:

Hey Phil, thanks! Looks good, just some minor nitpicks:

::: src/Extensions/Banshee.Lastfm/Banshee.Lastfm.Audioscrobbler/AudioscrobblerService.cs
@@ +73,3 @@
 
+        private const int MINIMUM_TRACK_DURATION_FOR_AUDIOSCROBBLING = 30;
+        private const long MINIMUM_TRACK_PLAYTIME_FOR_AUDIOSCROBBLING = 240000;

As you have the TotalSeconds property as well, you could just use it so you don't need to write 240000 but 240.

@@ +220,3 @@
         SongTimer st = new SongTimer ();
 
+        private bool IsValidTrackInfoForSubmit (TrackInfo track)

The last two words of this method name look weird, how about "ForSubmission" or "ToSubmit"?

@@ +230,3 @@
+            } else {
+                return false;
+                !String.IsNullOrEmpty (track.ArtistName) &&

Returning true in an if() and false in the else{} normally tells you that you don't need the if-else at all :) Just simply do:

return track.Duration.TotalSeconds > ... && etc
Comment 28 Andrés G. Aragoneses (IRC: knocte) 2012-02-13 15:35:07 UTC
Review of attachment 207375 [details] [review]:

Hey Phil, thanks! Looks good, just some minor nitpicks:

::: src/Extensions/Banshee.Lastfm/Banshee.Lastfm.Audioscrobbler/AudioscrobblerService.cs
@@ +73,3 @@
 
+        private const int MINIMUM_TRACK_DURATION_FOR_AUDIOSCROBBLING = 30;
+        private const long MINIMUM_TRACK_PLAYTIME_FOR_AUDIOSCROBBLING = 240000;

As you have the TotalSeconds property as well, you could just use it so you don't need to write 240000 but 240.

@@ +220,3 @@
         SongTimer st = new SongTimer ();
 
+        private bool IsValidTrackInfoForSubmit (TrackInfo track)

The last two words of this method name look weird, how about "ForSubmission" or "ToSubmit"?

@@ +230,3 @@
+            } else {
+                return false;
+                !String.IsNullOrEmpty (track.ArtistName) &&

Returning true in an if() and false in the else{} normally tells you that you don't need the if-else at all :) Just simply do:

return track.Duration.TotalSeconds > ... && etc
Comment 29 Andrés G. Aragoneses (IRC: knocte) 2012-02-13 15:36:03 UTC
Sorry again for the double comment/review, I think there's a bug in the Splinter extension...
Comment 30 Phil Trimble 2012-02-17 06:45:17 UTC
Created attachment 207835 [details] [review]
Refactoring of AudioscrobblerService

New version based on suggestions from Andrés. I also noticed a check in OnPlayerEvent that essentially performs the same verifications as my new method so I switched to use it.

One question, Andrés. I was confused by what was meant by this:

>>As you have the TotalSeconds property as well, you could just use it so you
don't need to write 240000 but 240.

The SongTimer internal class tracks the playtime in milliseconds, which was why I originally made the constant a long. In order to work with the constant in seconds I used the TimeSpan class to convert the SongTimer value. Another approach would have been to change the SongTimer internal class to do things in seconds or give it multiple getters for the different formats but to me it seems like six of this, half a dozen of another. I don't have enough experience with C# to know if one approach is 'better' so I will defer to your judgment, Andrés.

I've said it before but I'll say it again: thanks for taking the time to help me out! I really, really appreciate it. I'm learning a lot and having a blast going through this process.
Comment 31 Andrés G. Aragoneses (IRC: knocte) 2012-02-19 16:30:14 UTC
Created attachment 207992 [details] [review]
Refactoring of AudioScrobbleService v2

(In reply to comment #30)
> Created an attachment (id=207835) [details] [review]
> Refactoring of AudioscrobblerService
> 
> New version based on suggestions from Andrés. I also noticed a check in
> OnPlayerEvent that essentially performs the same verifications as my new method
> so I switched to use it.

Cool.


> One question, Andrés. I was confused by what was meant by this:
> 
> >>As you have the TotalSeconds property as well, you could just use it so you
> don't need to write 240000 but 240.
> 
> The SongTimer internal class tracks the playtime in milliseconds, which was why
> I originally made the constant a long.

Ah, you're right! I thought it was in seconds, so I thought it would be even better this way so the two consts are using the same time unit.

> In order to work with the constant in
> seconds I used the TimeSpan class to convert the SongTimer value. Another
> approach would have been to change the SongTimer internal class to do things in
> seconds or give it multiple getters for the different formats but to me it
> seems like six of this, half a dozen of another. I don't have enough experience
> with C# to know if one approach is 'better' so I will defer to your judgment,
> Andrés.

Well, the ideal thing is always use TimeSpan elements, because when declaring them you see clearly what unit you're using, so it improves readability a lot. I have taken a look at the file again and it turns out that Duration property from taglib-sharp API is actually already using TimeSpans, so I've removed the conversions from this case (so we compare TimeSpan with TimeSpan), and left the 240 seconds in a TimeSpan as well (not worth it to use long for the micro-optimization). Created a new patch myself (but with your name), to avoid increasing the back&forth for such a small patch :)


> I've said it before but I'll say it again: thanks for taking the time to help
> me out! I really, really appreciate it. I'm learning a lot and having a blast
> going through this process.

No problem! Glad I'm helping.
You can work on the next patch (the one that actually solves this bug :) on top of this one (which we'll anyway commit soon).
Comment 32 Bertrand Lorentz 2012-02-19 17:04:47 UTC
Review of attachment 207992 [details] [review]:

Please reword the commit message a bit to remove the bullet points, and "Remove magic numbers" should also be clearer.
And the maximum number of characters per line in the commit message is 72, you seem to have much less.

With those changes, I'm OK for this to be committed.

::: src/Extensions/Banshee.Lastfm/Banshee.Lastfm.Audioscrobbler/AudioscrobblerService.cs
@@ +73,3 @@
 
+        private readonly TimeSpan MINIMUM_TRACK_DURATION_FOR_AUDIOSCROBBLING = TimeSpan.FromSeconds (30);
+        private readonly TimeSpan MINIMUM_TRACK_PLAYTIME_FOR_AUDIOSCROBBLING = TimeSpan.FromSeconds (240);

the _FOR_AUDIOSCROBBLING suffix is not necessary, we're in the AudioscrobblerService class ;)
Comment 33 Andrés G. Aragoneses (IRC: knocte) 2012-02-19 17:28:26 UTC
Comment on attachment 207992 [details] [review]
Refactoring of AudioScrobbleService v2

(In reply to comment #32)
> Review of attachment 207992 [details] [review]:

Good points thanks! committed/pushed to master.
Comment 34 Phil Trimble 2012-03-29 04:56:38 UTC
Created attachment 210841 [details] [review]
Enables DAP scrobbling support (1/2)

I apologize for the delay, I haven't had much time in the last month to really dig into this.

I split the patch into two pieces to (hopefully) make the review a bit easier. This portion contains the changes to the DAP classes only. It should be applied first and doesn't require the second patch to compile.

I will provide a short summary of my overall updates with the second patch.
Comment 35 Phil Trimble 2012-03-29 05:10:42 UTC
Created attachment 210842 [details] [review]
Enables DAP scrobbling support (2/2)

Second half of the patch. This contains the changes for the audioscrobbler-related classes. It requires the first patch to be applied in order to compile.

The highlights:
- Scrobbling now occurs on device load instead of sync. This covers a wider range of use cases
- Updated various variable names based on suggestions from Andrés
- Refactored for readability based on suggestions from Andrés
- User is notified if she/he attempts to quit from banshee while device scrobbling is taking place, just like it does with a device sync
- Now keeps track of recent plays via xml file (see below for more)

On that last point: since Apple products only reset the 'recent playcounts' file on the device at the time of a database write and we are no longer tying the scrobbling to a sync event we are forced to keep track of the plays between each load, otherwise we start submitting the same plays on each subsequent device connection. This is where the XML solution comes from. The XML file is cleared out at each database write. Based on my testing this works both with syncing the entire library and manually managing the device.
Comment 36 Andrés G. Aragoneses (IRC: knocte) 2012-03-30 00:39:04 UTC
(In reply to comment #35)
> Created an attachment (id=210842) [details] [review]
> Enables DAP scrobbling support (2/2)

Awesome Phil! For a while I thought you had vanished, phew :)

So I hope to keep your enthusiasm even when I'm going to review this again! hehe. I'll need some days anyway to review it fully because I want to understand everything properly.

I saw your recent improvements that you applied from my suggestions and I think they are great. But we can improve this even more.

> +        public event EventHandler DapSourceLoaded;

You're adding an event called "DapSourceLoaded" to a class called "DapSource". See the redundancy? I guess you can rename it to just "Loaded" ;)

Another improvement for this event: you declared it as "EventHandler" and then you use empty event args. Well, then why not stop using event args at all? You could declare it as just "Action<DapSource>".

Now let's look at how you use it:


> +   DapSource dap_source = (DapSource) args.Source;
> +   dap_source.DapSourceLoaded += OnDapSourceLoaded;
> + }
> +
> + private void OnDapSourceLoaded (object sender, EventArgs e)
> + {
> +   DapSource dap_source = (DapSource) sender;
> +
> +   if (!((ToggleAction)actions["AudioscrobblerDeviceEnableAction"]).Active) {
> +     return;

See what you did here? You subscribed to an event even when the preference is not enabled. Why not do this instead?:

+   if (!((ToggleAction)actions["AudioscrobblerDeviceEnableAction"]).Active) {
+     return;
+   DapSource dap_source = (DapSource) args.Source;
+   dap_source.Loaded += OnDapSourceLoaded;
+ }
+
+ private void OnDapSourceLoaded (object sender, EventArgs e)
+ {
+   DapSource dap_source = (DapSource) sender;
+

Now let's think a bit more what you're doing here:
- You subscribe to the Loaded event.
- When the event is fired, you ask the device for the recent playcount.
So then you're adding two elements in the API (Loaded & ReturnRecentlyPlayedTracks() ) when you could just add one: yes, you guessed it, the EventArgs.Empty can actually the args you're returning from ReturnRecentlyPlayedTracks.

Indeed, we even don't need to add that API to DapSource class if AppleDeviceSource is really the only one who is going to consume (override) it. Why not just adding an interface?

For instance:

+ private void OnSourceAdded (SourceEventArgs args)
+ {
+   if (!((ToggleAction)actions["AudioscrobblerDeviceEnableAction"]).Active) {
+     return;
+   var scrobbler_source = args.Source as IScrobblerSource;
+   if (scrobbler_source == null) {
+     return;
+   }
+
+   scrobbler_source.ReadyToScrobble += OnReadyToScrobble ();
+ }
+
+ private void OnReadyToScrobble (IList<TrackInfo> tracks)
+ {

This way you just have the interface IScrobblerSource:

    public interface ITrackModelSource : ISource
    {
      event System.Action<IEnumerable<TrackInfo>> ReadyToScrobble;
    }

Even better, you could place this interface in the assembly Banshee.Services (Banshee.Sources namespace) to sit near ISource and IFilterableSource instead of being in Banshee.Dap assembly. What do we achieve by that? That LastFM doesn't gain a dependency on Dap, thus users will still be able to enable the LastFM extension without forcing them to enable device support. You can then fire ReadyToScrobble event in the LoadFromDevice() method of AppleDeviceSource, without the need to modify DapSource class at all.

Introducing the interface IScrobblerSource into Banshee Core may also be interesting in case in the future somebody makes a Facebook extension (Facebook can scrobble media too).

Now I'm kind of tired so I need extra time to digest/understand why you need to write and read from/to an XML file, so how about if you cook new patches with the suggestions above, and meanwhile I'll read more carefully the rest of the patch?

Thanks again!
Comment 37 Andrés G. Aragoneses (IRC: knocte) 2012-03-30 00:44:04 UTC
Ooops, just re-read my comment and there were some typos (it's late here!). I'll just correct the piece of code I proposed, but without the typos:

+ private void OnSourceAdded (SourceEventArgs args)
+ {
+   if (!((ToggleAction)actions["AudioscrobblerDeviceEnableAction"]).Active) {
+     return;
+   }
+   var scrobbler_source = args.Source as IScrobblerSource;
+   if (scrobbler_source == null) {
+     return;
+   }
+
+   scrobbler_source.ReadyToScrobble += OnReadyToScrobble ();
+ }
+
+ private void OnReadyToScrobble (IList<TrackInfo> tracks)
+ {
...
+   public interface IScrobblerSource
+   {
+     event System.Action<IList<TrackInfo>> ReadyToScrobble;
+   }

See what I mean? :)
Comment 38 Andrés G. Aragoneses (IRC: knocte) 2012-03-30 23:25:29 UTC
I'll review now the fudge times thing:

+ // Apple products do not save individual playtimes so if there are multiple plays we
+ // need to fudge the times.

Even with this comment in place, it took me long to figure out what you meant. From an outsider point of view I would improve the comment with something even more explicit, like:

+ // Apple products do not save DateTime info for each track play, only a total
+ // sum of number of plays (playcount) of each track


+ private void GenerateFakePlaytimes (List<TrackInfo> recently_played_tracks,
+                                     AppleDeviceTrackInfo ipod_track, int prev_playcount)
+ {
+   DateTime most_recent_playtime = ipod_track.LastPlayed;
+
+   for (int i = prev_playcount; i < ipod_track.RecentPlayCount; i++) {
+
+     var played_track = new AppleDeviceTrackInfo (ipod_track);
+
+     if (i > prev_playcount) {
+       played_track.LastPlayed = most_recent_playtime - played_track.Duration;
+       most_recent_playtime = played_track.LastPlayed;
+     }
+
+     recently_played_tracks.Add (played_track);
+   }
+ }

It took me a while to figure out what you were really doing here, because:
1) You're starting the loop from prev_playcount, but you really "do stuff" (apart from adding to the list) since prev_playcount + 1. You maybe should have two loops to make it more clear: one for the tracks that need fudging and a global one for adding to recently_played_tracks.
2) You're modifying a list that comes as a parameter in a method that is void. In general, it's better to avoid mutating parameters if you can just use return values. I would rather make the method return a List of tracks (and later the caller can merge lists) rather than modifying a parameter (the intent would also be more clear and more readable).
3) You're creating yet another AppleDeviceTrackInfo instance from the original instance of the same type that is passed as a parameter. Why creating so many objects? This is a smell. If you think about it, the LastPlayed field of TrackInfo class is not used by the code in AudioScrobbleService that enqueues elements, so you might as well give AudioScrobblerService a list of n DateTime elements for each track instead of creating the same track n times with different LastPlayed values.

Then, I propose:

+ private List<DateTime> GenerateFakePlaytimes (AppleDeviceTrackInfo ipod_track, int prev_playcount)
+ {
+   ...
+ }

And then, the interface I talked about in my previous comment would change slightly:

+   public interface IBatchScrobblingSource
+   {
+     event System.Action<Dictionary<TrackInfo,List<DateTime>>> ReadyToScrobble;
+   }

I didn't write the implementation of the new GenerateFakePlaytimes() because I guess you can transform it very well :) And also because I'm not sure if the approach to follow is actually desirable. Let me elaborate:
- Let's say we had to scrobble 2 songs X and Y.
- X.LastPlayed is 11:00 AM, playcount 2, and duration 5 minutes.
- Y.LastPlayed is 11:05 AM, playcount 2, and duration 3 minutes.

If I understood your algorithm well, we would scrobble:
- X at 10:55 and 11:00.
- Y at 11:02 and 11:05.
This not only generate fake times that would overlap, but also would make songs be repeated in the list (X, X, Y, Y). I would try to come up with an algorithm that tried to mix the songs so the same track doesn't follow the same track (X,Y,X,Y), no?


The last thing that I still don't understand is why you need an intermediate XML file. Can you elaborate on that please?

Thanks again Phil!
Comment 39 Phil Trimble 2012-03-31 17:25:46 UTC
(In reply to comment #38)

Hi Andrés, here are answers to just a handful of your points. Everything you wrote in the previous comments I agree with and will work on this weekend. 

1) To your point about 'doing stuff' in GenerateFakePlaytime() at only prev_playcount+1, the reason this is done is that the first play doesn't need to be modified. The 'most recent' play has a valid datetime that doesn't need to be touched. I am doing something by adding the track to the list with the first iteration, it just does not touch the playtime. 

I agree that this is not clear and should have been done differently, which I will take in consideration. This should be clearer with a change to using the Dictionary approach instead of copying each track over and over.

2) To your point about overlapping playtimes: I consciously decided that I would not worry about whether the playtimes themselves were entirely accurate in order to keep things as simple as possible. It all depends on how accurate you want the fake times to be. :)

In my experience using those ruby scripts I mentioned in my first comment Last.fm would not reject songs that had overlapping playtimes. I am definitely willing to come up with something that is more comprehensive if we feel that it is clearer/more safe. I will work in this direction.

3) Now, the XML file. I was positive that this would come up because I felt I was stretching outside of my comfort area and wasn't entirely sold that I was doing the right thing. Let me lay out a scenario that will hopefully make it clear why I felt this was necessary (even if my solution was not correct):

* I listen to Song A on Monday. On the ipod internally it now thinks that the recent playcount=1 and the 'last played' time is on Monday. I connect to Banshee and the track is scrobbled. I do not sync/write to the ipod so it does *not* do clear away the internal recent playcount info.
* I listen to Song A again on Tuesday. On the ipod Song A has playcount=2 and the 'last played' is Tuesday. I also listen to Song B, which now has playcount=1 and 'last played' as Tuesday. 

Here is the rub: I connect the ipod to Banshee on Tuesday and it sees that Song A has 2 recent plays. It doesn't know that I already processed the first play on Monday. It submits 2 plays for Song A and 1 play for Song B.

This is the basic scenario I was attempting to solve. The ipod internal 'recent play' information is only cleared upon a sync/write. This is why I clear out the XML file upon a successful MediaDatabase.write() only.

Because of this I saw two possible solutions:

* Every time we scrobble we use libgpod to initiate a 'write' operation. This would clear away the internal 'recent play' information and leave us in the clear. I discarded this because I felt it was not appropriate to force a write solely to clear away recent playcount information when we weren't doing anything else.
* We would need some kind of persistence in played tracks between connections. The XML file would be checked against to make sure we were not submitting duplicates.

The most simple solution that I saw was a simple XML file local to DAP that kept track of recent plays using the MetadataHash as a key and the playcount as the value. I placed this at the DAP level rather than at the Last.fm/Audioscrobbler level because only the device itself would know when it was safe to clear away the information and passing around the information to say when it was safe to clear away seemed wrong.

I briefly considered looking into using the internal database but figured that it was not appropriate to go digging around in there for what amounts to a fringe feature. I also put this in DapSource thinking that other types of devices could use it in the future. 

I'll stop here, let me know if you would like to discuss this particular point more.
Comment 40 Andrés G. Aragoneses (IRC: knocte) 2012-03-31 18:42:11 UTC
Thanks for the clear explanations! See below my reply:

(In reply to comment #39)
> (In reply to comment #38)
> 
> Hi Andrés, here are answers to just a handful of your points. Everything you
> wrote in the previous comments I agree with and will work on this weekend. 
> 
> 1) To your point about 'doing stuff' in GenerateFakePlaytime() at only
> prev_playcount+1, the reason this is done is that the first play doesn't need
> to be modified. The 'most recent' play has a valid datetime that doesn't need
> to be touched. I am doing something by adding the track to the list with the
> first iteration, it just does not touch the playtime. 
> 
> I agree that this is not clear and should have been done differently, which I
> will take in consideration. This should be clearer with a change to using the
> Dictionary approach instead of copying each track over and over.

Oh, I somehow thought there were more iterations (>1) that were not doing stuff. Given this, my point is kind of moot... But it doesn't matter anyway because you're going to change it to use a Dictionary...


> 2) To your point about overlapping playtimes: I consciously decided that I
> would not worry about whether the playtimes themselves were entirely accurate
> in order to keep things as simple as possible. It all depends on how accurate
> you want the fake times to be. :)
> 
> In my experience using those ruby scripts I mentioned in my first comment
> Last.fm would not reject songs that had overlapping playtimes. I am definitely
> willing to come up with something that is more comprehensive if we feel that it
> is clearer/more safe. I will work in this direction.

Well, if last.fm is fine with non-overlapping songs, let it be this way for now so the patch is simpler/easier (you can also add something like "//FIXME: avoid sequences of overlapping playtimes?" so we don't forget).

The same goes having consecutive tracks (X, X, X, Y, Y, Y). I'm not honestly a heavy LastFM user to really know if it really poses a drawback, so you can leave another FIXME in there for this if you want.


> 3) Now, the XML file. I was positive that this would come up because I felt I
> was stretching outside of my comfort area and wasn't entirely sold that I was
> doing the right thing. Let me lay out a scenario that will hopefully make it
> clear why I felt this was necessary (even if my solution was not correct):
> 
> * I listen to Song A on Monday. On the ipod internally it now thinks that the
> recent playcount=1 and the 'last played' time is on Monday. I connect to
> Banshee and the track is scrobbled. I do not sync/write to the ipod so it does
> *not* do clear away the internal recent playcount info.
> * I listen to Song A again on Tuesday. On the ipod Song A has playcount=2 and
> the 'last played' is Tuesday. I also listen to Song B, which now has
> playcount=1 and 'last played' as Tuesday. 
> 
> Here is the rub: I connect the ipod to Banshee on Tuesday and it sees that Song
> A has 2 recent plays. It doesn't know that I already processed the first play
> on Monday. It submits 2 plays for Song A and 1 play for Song B.
> 
> This is the basic scenario I was attempting to solve. The ipod internal 'recent
> play' information is only cleared upon a sync/write. This is why I clear out
> the XML file upon a successful MediaDatabase.write() only.
> 
> Because of this I saw two possible solutions:
> 
> * Every time we scrobble we use libgpod to initiate a 'write' operation. This
> would clear away the internal 'recent play' information and leave us in the
> clear. I discarded this because I felt it was not appropriate to force a write
> solely to clear away recent playcount information when we weren't doing
> anything else.
> * We would need some kind of persistence in played tracks between connections.
> The XML file would be checked against to make sure we were not submitting
> duplicates.
> 
> The most simple solution that I saw was a simple XML file local to DAP that
> kept track of recent plays using the MetadataHash as a key and the playcount as
> the value. I placed this at the DAP level rather than at the
> Last.fm/Audioscrobbler level because only the device itself would know when it
> was safe to clear away the information and passing around the information to
> say when it was safe to clear away seemed wrong.
> 
> I briefly considered looking into using the internal database but figured that
> it was not appropriate to go digging around in there for what amounts to a
> fringe feature. I also put this in DapSource thinking that other types of
> devices could use it in the future. 
> 
> I'll stop here, let me know if you would like to discuss this particular point
> more.

That's fine, I see your reasoning now!

Well, I would like to explore the alternative about initiating a reset/write operation via libgpod to clean the playcount, because of various reasons:
1) Introducing yet another kind of storage brings always problems about consumer-producer algorithms, like race conditions or loss of data in abnormal situations (if the process dies somehow in the middle of an operation for instance?).
2) When firing the event ReadyToScrobble, you will need to save the handler into a variable first (like advised in http://www.mono-project.com/Gendarme.Rules.Concurrency#ProtectCallToEventDelegatesRule ), so in that moment, as you're already checking for null, you know if you have someone that has subscribed to the event. If someone has subscribed, you can write to the database, but avoid it if not (this way you would only call a libgpod-write operation if you have LastFM's device+scrobbling preference enabled, without causing unnecessary writes in the rest of the cases).
3) It's less code to maintain :)

What do you think?
Comment 41 Phil Trimble 2012-04-14 23:29:38 UTC
Sorry again for the delay! Life has been kicking my butt all over the place lately. :(

I am fine proceeding with your suggestions. I have finished all of the changes to use the event as you suggested and it is scrobbling as intended. My only hurdle now is figuring out the correct way to perform the ipod/libgpod write operation once the scrobbling is completed.

Based on my (extremely quick) reading of the existing sync threading logic in DapSync/AppleDeviceSource I am concerned with the following two scenarios:

A) A user has the 'Sync when first plugged in' option selected and also has this new last.fm scrobbling functionality enabled, causing both to trigger at nearly the same time upon device load.
B) Someone manually syncs their entire library immediately upon the device loading and the new last.fm scrobbling logic kicks off at the same time.

Or some combination thereof. I am experiencing some weird behavior when either scenario takes place, things like Banshee attempting to sync my entire library to the ipod again or the 'Preparing to synchronize' message staying up until I force a close of Banshee. At this point my guess is that I have chosen an incorrect way to initiate the write/sync operation after the scrobbling is done.

I will try to find some time in the near future to dig deeper and increase my understanding of what exactly occurs when these sync/write operations are triggered. Hopefully then a solution will become apparent to me.
Comment 42 Andrés G. Aragoneses (IRC: knocte) 2012-04-15 20:25:01 UTC
(In reply to comment #41)
> Sorry again for the delay! Life has been kicking my butt all over the place
> lately. :(

No pressure!

> ... or the 'Preparing to synchronize' message staying up until I
> force a close of Banshee...

Take in account that may be bug 657769 (which has a patch that may fix it, btw, but nobody has tested yet).

I hope you solve it soon. BTW try to not get entangled in the code that Banshee uses to sync because it may be very coupled to the Sync modes and stuff we don't care about (I mean, for this small write operation in which only playcount attribute of songs will change, I don't think there is need of displaying any UI informing to the user about a sync operation, so just use libgpod-sharp API directly!).
Comment 43 Phil Trimble 2012-04-21 05:18:41 UTC
Created attachment 212482 [details] [review]
Enables device scrobbling support

Here is a new patch that incorporates your suggestions. It creates the new scrobbler interface and uses it to pass the recent play information via the event.

Have at it, my friend. :)
Comment 44 Andrés G. Aragoneses (IRC: knocte) 2012-04-21 11:58:25 UTC
Great! I'm reviewing. If it needs final small touches I'll fix them myself and add a new patch.
Comment 45 Andrés G. Aragoneses (IRC: knocte) 2012-04-21 14:14:02 UTC
Created attachment 212505 [details] [review]
Enables device scrobbling support v4

(In reply to comment #44)
> Great! I'm reviewing. If it needs final small touches I'll fix them myself and
> add a new patch.

I finally found some nitpicks, but they are so small that I decided to fix the patch myself so you don't have to work more on it again (you must be tired, and sick of me! hehe).

However before we go ahead with it, can you do a final thing Phil? Just revert your patch from your working copy of Banshee, apply this version of mine with my final touches, and test it to just make sure I didn't break anything?

I'll explain below the changes I did:


> diff --git a/src/Core/Banshee.Services/Banshee.Collection.Database/DatabaseTrackInfo.cs b/src/Core/Banshee.Services/Banshee.Collection.Database/DatabaseTrackInfo.cs
> index 6b8fe23..1d6b95d 100644
> --- a/src/Core/Banshee.Services/Banshee.Collection.Database/DatabaseTrackInfo.cs
> +++ b/src/Core/Banshee.Services/Banshee.Collection.Database/DatabaseTrackInfo.cs
> @@ -657,6 +657,12 @@ namespace Banshee.Collection.Database
>              get { return base.PlayCount; }
>              set { base.PlayCount = value; }
>          }
> +        
> +        private int recent_play_count;
> +        public int RecentPlayCount {
> +            get { return recent_play_count; }
> +            set { recent_play_count = value; }
> +        }

This property is only used in the AppleDeviceExtension so there's no need to pollute the core object model of Banshee with it, I've removed it (using directly GPod.Track type instead of AppleTrackInfo, in the places you were using it).


>  
>          [DatabaseColumn]
>          public override int SkipCount {
> diff --git a/src/Core/Banshee.Services/Banshee.Sources/IBatchScrobblerSource.cs b/src/Core/Banshee.Services/Banshee.Sources/IBatchScrobblerSource.cs
> new file mode 100644
> index 0000000..bf8e161
> --- /dev/null
> +++ b/src/Core/Banshee.Services/Banshee.Sources/IBatchScrobblerSource.cs
> +using System;
> +using System.Collections.Generic;
> +using Banshee.Collection.Database;
> +
> +namespace Banshee.Sources
> +{
> +    public interface IBatchScrobblerSource
> +    {
> +        event System.Action<IDictionary<DatabaseTrackInfo, IList<DateTime>>> ReadyToScrobble;

I made a small mistake when recommending you to use Action. I mean, it's much better now having this interface in place with an event, but I've realised all events in C# should have the convention of being an EventHandler<T> type, so I've corrected this.


> +    }
> +}
> +
> diff --git a/src/Dap/Banshee.Dap.AppleDevice/Banshee.Dap.AppleDevice/AppleDeviceSource.cs b/src/Dap/Banshee.Dap.AppleDevice/Banshee.Dap.AppleDevice/AppleDeviceSource.cs
> index bbe3773..826f110 100644
> --- a/src/Dap/Banshee.Dap.AppleDevice/Banshee.Dap.AppleDevice/AppleDeviceSource.cs
> +++ b/src/Dap/Banshee.Dap.AppleDevice/Banshee.Dap.AppleDevice/AppleDeviceSource.cs
> @@ -3,6 +3,7 @@
>  //
>  // Author:
>  //   Alan McGovern <amcgovern@novell.com>
> +//   Phil Trimble <philtrimble@gmail.com>
>  //
>  // Copyright (C) 2010 Novell, Inc.

I added a Copyright line for you as well as me (as I helped a bit in this patch :) )

>  //
> @@ -247,6 +251,8 @@ namespace Banshee.Dap.AppleDevice
>                  pl_src.UpdateCounts ();
>                  AddChildSource (pl_src);
>              }
> +
> +            OnReadyToScrobble ();

I've renamed this to "ReadyToScrobble". Take in account that the convention of using the name On$(NameOfEvent) doesn't apply here, because this class is not a subscriber of the event, but the one that fires it.


>          }
>  
>  #endregion
> @@ -679,5 +690,66 @@ namespace Banshee.Dap.AppleDevice
>  
>  #endregion
>  
> +#region Scrobbling
> +
> +        private void OnReadyToScrobble ()
> +        {
> +            System.Action<IDictionary<DatabaseTrackInfo, IList<DateTime>>> handler = ReadyToScrobble;
> +            if (handler != null) {
> +                IDictionary<DatabaseTrackInfo, IList<DateTime>> recent_plays = GatherRecentPlayInfo ();
> +                if (recent_plays.Count != 0) {
> +                    handler (recent_plays);
> +
> +                    // We must perform a write to clear out the recent playcount information so we do not
> +                    // submit duplicate plays on subsequent invocations.
> +                    lock (write_mutex) {

Not sure we need the write_mutex, but well, as an extra security measure it's fine.

> +                        MediaDatabase.Write ();
> +                    }
> +                }
> +            }
> +        }
> +
> +        private IDictionary<DatabaseTrackInfo, IList<DateTime>> GatherRecentPlayInfo ()
> +        {
> +            Dictionary <DatabaseTrackInfo, IList<DateTime>> recent_plays
> +                = new Dictionary <DatabaseTrackInfo, IList<DateTime>> ();

I've used "var" here to have a shorter line.


> +
> +            foreach (var ipod_track in MediaDatabase.Tracks) {
> +
> +                if (String.IsNullOrEmpty (ipod_track.IpodPath)) {
> +                    continue;
> +                }
> +
> +                if (ipod_track.RecentPlayCount == 0) {
> +                    continue;
> +                }

I've combined these 2 if clauses into one with an OR operator.


> +            
> +                AppleDeviceTrackInfo track = new AppleDeviceTrackInfo (ipod_track);
> +                IList<DateTime> playtimes = GenerateFakePlaytimes (track);
> +
> +                recent_plays.Add (track, playtimes);
> +            }
> +
> +            return recent_plays;
> +        }
> +
> +        // Apple products do not save DateTime info for each track play, only a total
> +        // sum of number of plays (playcount) of each track.
> +        private IList<DateTime> GenerateFakePlaytimes (AppleDeviceTrackInfo track)
> +        {
> +            IList<DateTime> playtimes = new List<DateTime> ();
> +
> +            //FIXME: avoid sequences of overlapping playtimes?
> +            DateTime current_playtime = track.LastPlayed;
> +            for (int i = 0; i < track.RecentPlayCount; i++) {
> +                playtimes.Add (current_playtime);
> +                current_playtime = current_playtime - track.Duration;
> +            }
> +
> +            return playtimes;
> +        }
> +
> +#endregion
> +
>      }
>  }
> diff --git a/src/Dap/Banshee.Dap.AppleDevice/Banshee.Dap.AppleDevice/AppleDeviceTrackInfo.cs b/src/Dap/Banshee.Dap.AppleDevice/Banshee.Dap.AppleDevice/AppleDeviceTrackInfo.cs
> index b8549f7..fe4c78f 100644
> --- a/src/Dap/Banshee.Dap.AppleDevice/Banshee.Dap.AppleDevice/AppleDeviceTrackInfo.cs
> +++ b/src/Dap/Banshee.Dap.AppleDevice/Banshee.Dap.AppleDevice/AppleDeviceTrackInfo.cs


Changes in this file are no longer necessary because I removed the RecentPlayCount property.


> diff --git a/src/Extensions/Banshee.Lastfm/Banshee.Lastfm.Audioscrobbler/AudioscrobblerService.cs b/src/Extensions/Banshee.Lastfm/Banshee.Lastfm.Audioscrobbler/AudioscrobblerService.cs
> index dc87acb..f4c0db8 100644
> --- a/src/Extensions/Banshee.Lastfm/Banshee.Lastfm.Audioscrobbler/AudioscrobblerService.cs
> +++ b/src/Extensions/Banshee.Lastfm/Banshee.Lastfm.Audioscrobbler/AudioscrobblerService.cs
> @@ -136,10 +143,20 @@ namespace Banshee.Lastfm.Audioscrobbler
>  
>              actions.Add (new ToggleActionEntry [] {
>                  new ToggleActionEntry ("AudioscrobblerEnableAction", null,
> -                    Catalog.GetString ("_Enable Song Reporting"), null,
> -                    Catalog.GetString ("Enable song reporting"), OnToggleEnabled, Enabled)
> +                    Catalog.GetString ("_Enable Song Reporting From Banshee"), null,
> +                    Catalog.GetString ("Enable song reporting From Banshee"), OnToggleEnabled, Enabled)
> +            });
> +
> +            actions.Add (new ToggleActionEntry [] {
> +                new ToggleActionEntry ("AudioscrobblerDeviceEnableAction", null,
> +                    Catalog.GetString ("_Enable Song Reporting From Device"), null,
> +                    Catalog.GetString ("Enable song reporting From Device"), OnToggleDeviceEnabled, DeviceEnabled)
>              });
>  
> +            if (!((ToggleAction) actions["AudioscrobblerEnableAction"]).Active) {
> +                ((ToggleAction) actions["AudioscrobblerDeviceEnableAction"]).Sensitive = false;
> +            }
> +
>              action_service.UIManager.InsertActionGroup (actions, 0);
>              ui_manager_id = action_service.UIManager.AddUiFromResource ("AudioscrobblerMenu.xml");
>  
> @@ -159,6 +176,9 @@ namespace Banshee.Lastfm.Audioscrobbler
>  
>              ServiceManager.Get<Network> ().StateChanged -= HandleNetworkStateChanged;
>  
> +            ServiceManager.SourceManager.SourceAdded -= OnSourceAdded;
> +            ServiceManager.SourceManager.SourceRemoved -= OnSourceRemoved;
> +
>              // When we stop the connection, queue ends up getting saved too, so the
>              // track we queued earlier should stay until next session.
>              connection.Stop ();
> @@ -306,14 +326,102 @@ namespace Banshee.Lastfm.Audioscrobbler
>              Enabled = ((ToggleAction) o).Active;
>          }
>  
> +        private void OnToggleDeviceEnabled (object o, EventArgs args)
> +        {
> +            DeviceEnabled = ((ToggleAction) o).Active;
> +        }
> +
>          internal bool Enabled {
>              get { return EngineEnabledSchema.Get (); }
>              set {
>                  EngineEnabledSchema.Set (value);
>                  ((ToggleAction) actions["AudioscrobblerEnableAction"]).Active = value;
> +
> +                if (!value) {
> +                    DeviceEnabled = false;
> +                    ((ToggleAction) actions["AudioscrobblerDeviceEnableAction"]).Sensitive = false;
> +                } else {
> +                    ((ToggleAction) actions["AudioscrobblerDeviceEnableAction"]).Sensitive = true;
> +                }

I've changed my mind about this, why not allowing someone to scrobble their device if they don't have scrobbling within Banshee enabled? There's really no reason for avoiding it, unless I'm missing something. So I've changed it so one setting is not dependent on the other one.

> +            }
> +        }
> +
> +        internal bool DeviceEnabled {
> +            get { return DeviceEngineEnabledSchema.Get (); }
> +            set {
> +                DeviceEngineEnabledSchema.Set (value);
> +                ((ToggleAction) actions["AudioscrobblerDeviceEnableAction"]).Active = value;
> +            }
> +        }
> +
> +#region scrobbling
> +
> +        private void OnSourceAdded (SourceEventArgs args)
> +        {
> +            if (!((ToggleAction) actions["AudioscrobblerDeviceEnableAction"]).Active) {
> +                return;
> +            }

I've reworked a bit all the logic about how to subcribe to sources being added (to avoid this check above, and to only subscribe when we really have the setting enabled). Let me know if you see any problems with it.

> +
> +            var scrobbler_source = args.Source as IBatchScrobblerSource;
> +            if (scrobbler_source == null) {
> +                return;
> +            }
> +
> +            scrobbler_source.ReadyToScrobble += OnReadyToScrobble;
> +        }
> +
> +        private void OnSourceRemoved (SourceEventArgs args)
> +        {
> +            var scrobbler_source = args.Source as IBatchScrobblerSource;
> +            if (scrobbler_source == null) {
> +                return;
>              }
> +
> +            scrobbler_source.ReadyToScrobble -= OnReadyToScrobble;
>          }
>  
> +        private void OnReadyToScrobble (IDictionary<DatabaseTrackInfo, IList<DateTime>> tracks)
> +        {
> +            UserJob scrobble_job = new UserJob (Catalog.GetString ("Scrobbling from device"),
> +                                                Catalog.GetString ("Scrobbling from device..."));
> +
> +            scrobble_job.PriorityHints = PriorityHints.DataLossIfStopped;
> +            scrobble_job.Register ();
> +
> +            if (!connection.Started) {
> +                connection.Start ();
> +            }
> +
> +            int added_track_count = 0, processed_track_count = 0;
> +            String message = Catalog.GetString ("Processing track {0} of {1} ...");
> +
> +            foreach (KeyValuePair<DatabaseTrackInfo, IList<DateTime>> track_entry in tracks) {
> +                TrackInfo track = track_entry.Key;
> +
> +                if (IsValidForSubmission (track)) {
> +                    IList<DateTime> playtimes = track_entry.Value;
> +
> +                    foreach (DateTime playtime in playtimes) {
> +                        queue.Add (track, playtime);
> +                        added_track_count++;
> +                    }
> +                    Log.DebugFormat ("Added to Last.fm queue: {0} - Number of plays: {1}", track, playtimes.Count);
> +                } else {
> +                    Log.DebugFormat ("Track {0} failed validation check for Last.fm submission, skipping...",
> +                                     track);
> +                }
> +
> +                scrobble_job.Status = string.Format (message, ++processed_track_count, tracks.Count);
> +                scrobble_job.Progress = processed_track_count / (double) tracks.Count;
> +            }
> +
> +            Log.InformationFormat ("Number of played tracks from device added to Last.fm queue: {0}", added_track_count);
> +
> +            scrobble_job.Finish ();

I've put this call to Finish into a finally block.


> +        }
> +
> +#endregion
> +
>          public static readonly SchemaEntry<string> LastUserSchema = new SchemaEntry<string> (
>              "plugins.lastfm", "username", "", "Last.fm user", "Last.fm username"
>          );
Comment 46 Phil Trimble 2012-04-21 16:49:36 UTC
Looks good to me! I applied it and I didn't see any problems. I also tested it with a few plays from my ipod, no issues as far as I could see. Everything was processed as expected.

Can't say enough how much I appreciate your help on this Andres. I obviously bit off more than I could chew and it is only where it is because of you. 

As I am sure was blindingly obvious this was my first experience working on anything substantial with C#, my first time working on an app anything like Banshee, and also the first time I have ever submitted something to an open source project. Thanks again for taking all the time to essentially walk me through this effort.

Basically I am saying that I owe you a drink. ;)
Comment 47 Andrés G. Aragoneses (IRC: knocte) 2012-05-01 12:57:49 UTC
Comment on attachment 212505 [details] [review]
Enables device scrobbling support v4

Pushed this.

(My mistake, didn't mean to separate it in two commits: 

http://git.gnome.org/browse/banshee/commit/?id=3973d135783933ff6e9ab665429100fb8ee09bf9

http://git.gnome.org/browse/banshee/commit/?id=32063085c8d4d6d1f6d7ebfaecc543b1acc799bd
Comment 48 Andrés G. Aragoneses (IRC: knocte) 2012-05-01 13:01:27 UTC
(In reply to comment #46)
> Basically I am saying that I owe you a drink. ;)

Sounds good! Looking forward to it :)

The feature is now in master branch.