GNOME Bugzilla – Bug 544415
The next button only works one time out of two when an item is selected in the browsers (filters)
Last modified: 2008-09-06 23:01:58 UTC
Steps to reproduce : 1. Start banshee 2. Select an item in one of the browsers (genre, artist or album), but not the first in the list 3. Play a track 4. Click on the "Next" button Banshee then either does not go to the next track, or jumps to a track way down the list. If you click again on the "Next" button, it works correctly. I'm pretty sure this has started to happen after r4238. After applying the following patch, the output shows that the current track index is wrong when it doesn't work. The value is the index without the filter applied. ------------------------------------------- +++ src/Core/Banshee.Services/Banshee.PlaybackController/PlaybackControllerService.cs (working copy) @@ -308,6 +308,7 @@ return null; int index = Source.TrackModel.IndexOf (CurrentTrack); + Log.DebugFormat ("QueryTrackLinear {0} - {1}", index, Source.TrackModel.Count); if (index == -1) { return Source.TrackModel[0]; } else { ------------------------------------------- For example, I got the following output : [Debug 20:21:43.946] QueryTrackLinear begin 71 - 19
Great, thanks for tracking this down! I've noticed this too, I think. Let me know if you have a fix. :)
Created attachment 115195 [details] Relevant parts of --debug-sql output, with inline comments I tried to track this one further but got lost in the caching code. The attached file shows my findings : it seems that at some point the cache is loaded without the filter applied, and then the current track index is determined from this bad cache.
You need to have the track list sorted by track to reproduce this bug. OK, here's what happens : 1. I click on "next" 2. The source starts to reload itself by reload the cache without filter 3. Before the filters are applied to the cache, the current track index is determined and wrong 4. The filters are applied But why is the source reloaded ? A TracksChanged event is triggered because the "skippedcount" has changed. Banshee thinks the column on which the track list is sorted has changed because "CoreTracks.skippedcount" contains "Track", so the source is reloaded I'll post a hacky patch that hopefully will make that clearer.
Created attachment 115200 [details] [review] Fix the bug by not uselessly reloading the source This patch fixes the comparison between the field column and the current sort column, so that "CoreTracks.skippedcount" doesn't match "Track". The proper way is of course to actually fix the FIXME...
Hrm, this is definitely a good patch (please commit), but it doesn't fix this bug - it just makes it less likely to happen. It will still happen if you have your list sorted by skip or play count, though.
Committed. You're right, the underlying race condition still needs to be fixed, so leaving this open.
Created attachment 115316 [details] [review] Lock the cache when it's not filtered yet This patch adds a lock on the cache while it's reloaded and the filters applied. This should fix the race condition. Strangely, I couldn't reproduce the bug with the "Skipped count" column, so I revert the previous patch and reproduced it with the "Track" column, to make sure this patch fixes the issue. I just hope this doesn't any deadlock, so please test !
Bertrand, why does this fix work? What else locks on the cache object? Couldn't this be fixed by putting a lock (this) {} around the IndexOf method?
Some of the methods in SqliteModelCache.cs do a lock (this) {}. In particular, SqliteModelCache.IndexOf () has such a statement. So my added lock (cache) is on the same instance, and prevents SqliteModelCache.IndexOf from being executed while the cache is reloaded. But I guess your approach of adding a lock (this) {} in the DatabaseTrackListModel.IndexOf method would also work, and is more consistent with the rest of the code. I'll try to come up with a patch to do this and test it.
Created attachment 116741 [details] [review] Add a lock in IndexOf Better approach, as suggested by Gabriel. This also fixes the underlying issue. As before, I had to undo some changes to be able to reproduce the bug.
I committed this (and applied the same fix to the new FirstIndexOf method). Thanks!