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 548366 - Sometimes when importing DAAP duplicates each song entry
Sometimes when importing DAAP duplicates each song entry
Status: RESOLVED FIXED
Product: banshee
Classification: Other
Component: DAAP
1.2.1
Other Linux
: Normal normal
: 1.x
Assigned To: Banshee Maintainers
Banshee Maintainers
: 595558 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2008-08-18 23:53 UTC by H3g3m0n
Modified: 2012-02-19 16:15 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
proposed patch (8.72 KB, patch)
2009-11-16 01:50 UTC, Neil Loknath
none Details | Review
slightly revised (8.94 KB, patch)
2009-11-16 06:47 UTC, Neil Loknath
needs-work Details | Review
next iteration (8.81 KB, patch)
2010-01-26 06:40 UTC, Neil Loknath
committed Details | Review

Description H3g3m0n 2008-08-18 23:53:50 UTC
For some reason on some occasions DAAP will add the entire database of songs twice. 

The duplicate songs don't play. The song count in the side bar is 17044 instead of 8528, although the yellow bar that says its importing gives the 8528 song count

On some occasions I have seen 2 copies of all the songs.

The urls listed for a sample song:
http://127.0.0.1:8089/1218654208/21908 <- Broken song
http://127.0.0.1:8089/1841651200/21908 <- Plays fine

The DAAP share isn't on 127.0.0.1 I assume there is a proxy in use here.

The server is mt-daap.
Comment 1 Nick HS 2008-10-05 09:09:50 UTC
I can confirm this bug on Banshee 1.2.1 using Ubuntu 8.04 from the banshee teams ppa repository. The share is a mt-daapd one.
Comment 2 boondoklife 2009-09-11 13:19:33 UTC
Same thing here using 1.5.0, it would seem that it is caching the list sometimes as the last imported date will match the previous time that I loaded the list.

The share in my case is mt-daap 0.2.4.2

This does not happen every time, and if I close the app and then reload it, it will load properly.

If you need any log or what not please let me know what to capture.
Comment 3 Neil Loknath 2009-11-16 01:50:48 UTC
Created attachment 147841 [details] [review]
proposed patch

I hit this if a DAAP source is loaded and Banshee crashes / is killed, then the same source is loaded again after restarting banshee.

The proposed patch resolves by making use of the IsTemporary field of the CorePrimarySources table. Also, the DAAP playlists are marked as temporary.

I'm not sure how I feel about the implementation. It may be better implemented with a static PrimarySource.ClearTemporary method called maybe in the initialization of the SourceManager. Somewhat like the PlaylistSource.ClearTemporary method and how it's called in LoadAll. Or, maybe this shouldn't even be implemented at the PrimarySource level?

Anyway, here's a patch for one solution.
Comment 4 Neil Loknath 2009-11-16 06:47:21 UTC
Created attachment 147847 [details] [review]
slightly revised

Just realized that another call to PurgeTracks () is needed in case there are old DaapSource records in the CorePrimarySources table that get reused.
Comment 5 Gabriel Burt 2009-12-24 20:16:59 UTC
Review of attachment 147847 [details] [review]:

::: src/Core/Banshee.Services/Banshee.Sources/DatabaseSource.cs
@@ +297,3 @@
 
+        private bool is_temporary = false;
+        public bool IsTemporary {

change change this to an auto-property, eg
public bool IsTemporary { get; set; }

::: src/Core/Banshee.Services/Banshee.Sources/PrimarySource.cs
@@ +122,3 @@
         ");
 
+        protected HyenaSqliteCommand purge_self_command = new HyenaSqliteCommand (@"

Instead of creating one of these for every PrimarySource and keeping it around for as long as it exists, just inline the query in the Purge call.

::: src/Extensions/Banshee.Daap/Banshee.Daap/DaapSource.cs
@@ +202,3 @@
         {
             Disconnect (true);
+            PurgeSelfIfTemporary ();

Should do this in PrimarySource.Dispose, no?
Comment 6 Neil Loknath 2010-01-26 06:40:06 UTC
Created attachment 152287 [details] [review]
next iteration

Sorry this took so long, Gabriel. Applied your comments.

I don't know why I didn't put the purge call in PrimarySource. Anyway, I've put it there now.

Not sure about this part, however:
         
         public virtual void Dispose ()
         {
+			PurgeSelfIfTemporary ();
+			
             if (Application.ShuttingDown)
                 return;

Figured I should put the purge call before the if statement to ensure it gets called. Not sure if that is appropriate, however.
Comment 7 Gabriel Burt 2010-01-26 18:55:47 UTC
*** Bug 595558 has been marked as a duplicate of this bug. ***
Comment 8 Michał Sawicz 2010-02-01 22:13:07 UTC
BTW, is it at all possible to cache the share between Banshee sessions? I have a ~20k share that takes a minute or so to load, it would be nice if Banshee would store the list between instances and only reconnect to the share as fast as possible and refresh the share in the background.
Comment 9 Gabriel Burt 2010-05-07 22:18:20 UTC
We could cache it, but then what if it's out of date?  Then we have to do consistency checks, which will be even slower.  The slowness of loading DAAP tracks (and the bug where it should be decided whether to cache) is bug #527192, though.
Comment 10 Bertrand Lorentz 2010-05-29 21:23:56 UTC
Review of attachment 152287 [details] [review]:

The patch looks good, but I don't have a DAAP server to test it. It'd be great if someone could test the patch and report the result here.

::: src/Core/Banshee.Services/Banshee.Sources/PrimarySource.cs
@@ +464,3 @@
+            if (!IsTemporary) {
+        {
+        protected virtual void PurgeSelfIfTemporary ()

Should be indented with spaces here
Comment 11 Dan Elliott 2012-01-18 16:22:06 UTC
Is there a temporary fix that I can use while I am waiting for my distro to include this patch?

Thank you, for everyone's hard work.  I enjoy Banshee.
Comment 12 Dan Elliott 2012-01-18 16:27:59 UTC
(In reply to comment #2)
> Same thing here using 1.5.0, it would seem that it is caching the list
> sometimes as the last imported date will match the previous time that I loaded
> the list.
> 
> The share in my case is mt-daap 0.2.4.2
> 
> This does not happen every time, and if I close the app and then reload it, it
> will load properly.
> 
> If you need any log or what not please let me know what to capture.

FYI, I got up to 5 copies of each song and the stop-restart thing is not clearing them out.
Comment 13 Bertrand Lorentz 2012-02-19 16:15:24 UTC
Comment on attachment 152287 [details] [review]
next iteration

I've committed this with a few minor changes and updates:
http://git.gnome.org/browse/banshee/commit/?id=70303ded98c
Comment 14 Bertrand Lorentz 2012-02-19 16:15:41 UTC
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.