GNOME Bugzilla – Bug 666696
Doesn't sync playlist(s) if you quit Banshee instead of clicking on the Disconnect menu in the device
Last modified: 2015-03-09 22:42:58 UTC
I just noticed this bug. (At least reproduced it with a USB disk that had an .is_audio_player file) 1. Connect your device. 2. Drag and drop a song to your device. 3. Create a playlist on the device. 4. Drag and drop the transferred song in the step 2, from the device source to the playlist created in step 3. 5. Quit Banshee. Current results: Playlist is not created in the device's filesystem. Expected results: Should be created when exiting Banshee (the workaround is to right-click on the device and click on "Disconnect" before quitting Banshee).
Created attachment 204272 [details] [review] Proposed patch
Review of attachment 204272 [details] [review]: Is it going to sync twice if you eject manually and then quit?
(In reply to comment #2) > Review of attachment 204272 [details] [review]: > > Is it going to sync twice if you eject manually and then quit? No because in the moment you quit there is no DapSource anymore. I guess your real question is: "if you eject manually is it going to sync twice"? And the answer is yes, because I just tested it (apparently Eject() causes the Volume to be unmounted, which fires an event on the hw backend which is caught by DapService.OnHardwareDeviceRemoved() which then disposes the source), so good catch! So after thinking a bit about it, I wrote a 2nd patch. However when I tested it a 2nd time before posting it here, Banshee threw an exception, so I'll have to debug it tomorrow. Thanks!
(In reply to comment #2) > Review of attachment 204272 [details] [review]: Apparently, by reviewing a patch you don't get added to CC list by default, so I'm doing it now :)
Created attachment 204291 [details] [review] Proposed patch, v2 This is my second attempt (in order to fix the bug but at the same time not cause a double playlist-sync, as caught by Alex). So, to avoid the double-sync there needs to be some kind of flag that tells Banshee that the sync was already performed by the Eject() operation when the Dispose() is called, and a thought, why not checking if its sync object is already disposed? Because, when Eject() happens, it seems the sync object is just used the last time to check if playlists need to be synced, and not used anymore since then [1], so then we can dispose it at that moment, and assign its field to null after disposing it, so we know later that it has already been disposed, so we cannot check it's state again. [1] Not 100% true, as you can see in this patch I had to do some "null" guarding on some places where it wasn't done. This is not really problematic because seems to be only related to cleanup code (unsubscribing of events from the observers; but anyway if an object has event subscribers, it can just get rid of them by assigning the event to null, without the need of waiting for the cleanup from its consumers, so I decided to do that on the Dispose() method of DapSync).
Review of attachment 204291 [details] [review]: It feels overcomplicated. Why not use the first patch and just add a private `bool flushed = false` which is checked and set to `true` before Flush()ing in both places?
Thanks for the patches Andrés ! I've just committed the first patch, taking into account Alex's suggestion to simply add a "flushed" flag.
Created attachment 298930 [details] [review] Proposed Patch v3 Unfortunately this bug appears to have regressed. I don't seem to be able to re-open it though... This patch is my first stab at an alternative approach, for consideration. Thanks.