GNOME Bugzilla – Bug 635951
Song continues playing after 'Remove From Library'
Last modified: 2011-03-22 20:32:28 UTC
The fix to bug 540525 stops playback when the playing track is deleted from the drive, but not when it's removed from the library if the library is also the current source. A patch to fix this is on its way.
Created attachment 175388 [details] [review] Patch to stop playback when removing from library This fixes the problem by adding the same "IsPlaying" check to the OnRemoveTracks method. However, while getting git to format the patch (which may not be correct -- this is only my second patch), I started to understand what exactly is going on here. When you delete a track from the drive, everything works correctly. When you're listening to a playlist and you choose "Remove From Library" on the currently playing song, everything works as expected. The problem is, when you're listening to the Music Library and you choose "Remove From Library", the song keeps playing. This is because the Music Library is the active source, so it uses the OnRemoveTracks method instead of the OnRemoveTracksFromLibrary method. By adding the check into the OnRemoveTracks method as well, I've fixed the "song keeps playing after removal" problem, but now the track also stops playing when you're listening to a playlist and you "Remove From Playlist" the currently playing song. This may or may not be the expected behavior. If that isn't the behavior that we want, maybe the best solution would be to wrap the foreach loop that I'm adding in an if statement that checks to see if the current source is a LibrarySource. Would that work?
Created attachment 175390 [details] [review] same as first, but limited to LibrarySource Assuming we don't want playback to stop when you remove the currently playing song from a playlist, this fixes that behavior by only running the 'IsPlaying' check in the OnRemoveTracks method if the source is a LibrarySource. I've tested all the scenarios that I could think of, and this works for me. The only other thing is that the exact same foreach loop now occurs in TrackActions.cs three times. Should that be pulled out into its own method?
I would split that out, it may only be a couple of lines but let's be beautiful.
Review of attachment 175390 [details] [review]: I agree with David, let's split that out. Also, Gabriel mentioned on IRC that iterating on the entire selection can potentially be slow, and indeed it takes about 2 seconds to remove 10k tracks from a play list. An alternative is to check if the playing track is still in the database when deleting from any source or removing from a primary source; and if it's in the source when removing from a non-primary one.
(In reply to comment #4) > Review of attachment 175390 [details] [review]: > > I agree with David, let's split that out. I agree too; that much is now done in my local branch. > Also, Gabriel mentioned on IRC that iterating on the entire selection can > potentially be slow, and indeed it takes about 2 seconds to remove 10k tracks > from a play list. I don't doubt that iterating over the selection to see if one of the tracks is playing could take a long time. Especially if the selection includes several thousand tracks and the currently playing song isn't one of them (or is near the end of the selection). > An alternative is to check if the playing track is still in > the database when deleting from any source or removing from a primary source; > and if it's in the source when removing from a non-primary one. Let's see if I understand this... :) The plan is to get rid of the foreach loop before deleting/removing tracks, then, after the delete/remove operation is finished, we check to see if the currently playing song is still in the database. If not, we stop playing that song. If I got that much correct, then I have a couple more questions: 1. Do we want to stop playing a song when it is removed from the playlist in which it is playing? I'm assuming not, but an executive decision would be great. 2. In OnRemoveTracksFromLibrary, the actual removing of tracks happens in its own thread. We shouldn't check to see if the track is still in the library until track removal is finished, so how do I know when the library.RemoveTracks operation has completed? Threading confuses me. 3. I got the player engine to tell me which song is currently playing, but I'm not sure how to determine if that song is in the library/database. Tips?
Just wanted to follow up on this so that people don't think I've disappeared or given up. I took a shot at the new approach a couple weeks ago. My current progress is a few callbacks short of actually working -- the check to see if the song is still in the database/library is happening before the songs are done being removed -- but the approach is much cleaner. I'll try to get it working sometime this week and I'll attach another patch soon.
Created attachment 183832 [details] [review] 2nd attempt to stop playback when removing from library This patch gets us a couple steps closer. There's no more iterating through the entire selection before the tracks are removed; instead it now checks the library for the playing track after removal is finished and stops if necessary. The only real problem I'm having is with the OnDeleteTracksFromDrive method. I thought I set the callback correctly so that HandleRemovePlayingTrack is called after source.DeleteTracks has finished. Looking at the log with my debugging, you can see that my message correctly gets triggered after "finished deleting...", but it says the number of tracks in Music is the same before and after deleting, so obviously it hasn't actually finished deleting at that point. Is there a better way to make sure deleting is finished before checking the library to see if it still includes the track? [1 Debug 21:04:54.682] Music contains 27 tracks before DeleteTracks is called. [7 Debug 21:04:54.708] Starting [7 Debug 21:04:54.730] Finished - Deleting 1 of 1 From Music [7 Debug 21:04:54.741] Finished - Deleting 1 of 1 From Music [7 Debug 21:04:54.744] The playing song was found in the library. [7 Debug 21:04:54.744] Music contains 27 tracks after DeleteTracks is called. I also asked about this line on irc, but didn't wait long enough to get a response: LibrarySource library = source is LibrarySource ? source as LibrarySource : source.Parent as LibrarySource; Is it safe to assume that either a source or its parent must be a LibrarySource?
Comment on attachment 183832 [details] [review] 2nd attempt to stop playback when removing from library I committed an alternative patch that utilizes database queries to avoid iterating over the selected tracks in managed code: http://git.gnome.org/browse/banshee/commit/?id=958a5e2743341ec87a6d17714815ca28f1ce6a1a It also handles continuing to the first not-removed track when in non-shuffle mode.