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 658803 - Prune CoverArtDownloads cache when removing tracks
Prune CoverArtDownloads cache when removing tracks
Status: RESOLVED FIXED
Product: banshee
Classification: Other
Component: Other Extensions
git master
Other Linux
: Normal normal
: ---
Assigned To: Banshee Maintainers
Banshee Maintainers
Depends on:
Blocks:
 
 
Reported: 2011-09-12 10:54 UTC by Age Bosma (IRC: Forage)
Modified: 2011-10-09 15:04 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Prune CoverArtDownloads cache when removing tracks (1.59 KB, patch)
2011-09-12 10:54 UTC, Age Bosma (IRC: Forage)
needs-work Details | Review
Prune CoverArtDownloads cache, rev II (1.51 KB, patch)
2011-09-12 21:50 UTC, Age Bosma (IRC: Forage)
none Details | Review
Prune CoverArtDownloads cache when removing tracks (1.86 KB, patch)
2011-10-07 13:00 UTC, Age Bosma (IRC: Forage)
committed Details | Review

Description Age Bosma (IRC: Forage) 2011-09-12 10:54:48 UTC
Created attachment 196244 [details] [review]
Prune CoverArtDownloads cache when removing tracks

When removing tracks, and indirectly complete albums, the cache table CoverArtDownloads is not pruned, unlike CoreArtists and CoreAlbums.
Coverart retrieval is therefore skipped when removing the latest added album and adding a new one again.

The attached patch fixes this issue.

Note:
Core code of Banshee should not actually have (hardcoded) knowledge of tables created by extensions, like the CoverArtDownloads table of the CoverArt extension. However, BansheeDbConnection and ArtworkManager ignore this fact so I did the same in this patch for PrimarySource. In a perfect world I would have liked to see a way for an extension to register actions to e.g. a track removal event.
Comment 1 Bertrand Lorentz 2011-09-12 17:05:58 UTC
Review of attachment 196244 [details] [review]:

Thanks for the patch !

I think it should be possible to do that pruning in CoverArtService : you can add a handler on the ServiceManager.SourceManager.MusicLibrary.TracksDeleted event, like it's already done on TracksChanged. You can then run the same query in that handler.

I'm not sure if that event is triggered for each track or just once when several tracks are deleted together. Could you check that ? If it's the first case, I'm afraid it'll slow things down a bit.
Comment 2 Age Bosma (IRC: Forage) 2011-09-12 21:50:52 UTC
Created attachment 196314 [details] [review]
Prune CoverArtDownloads cache, rev II

You are right. The fact that the other files weren't using the event handler made me conclude that there was non, while it was right there under my nose...

Fixed now.

Let me know if I should feed ServiceManager.DbConnection.Execute() a HyenaSqliteCommand instead of a literal string. Some code feed it a string directly, other code use this HyenaSqliteCommand, but I do not know what method is preferred and why.
Comment 3 Age Bosma (IRC: Forage) 2011-09-12 22:00:32 UTC
P.s. TracksDeleted is triggered only once for a selection of one or more tracks, including a complete album.
Comment 4 Age Bosma (IRC: Forage) 2011-10-07 13:00:43 UTC
Created attachment 198527 [details] [review]
Prune CoverArtDownloads cache when removing tracks

Identical but properly formatted patch
Comment 5 Bertrand Lorentz 2011-10-09 15:00:36 UTC
Comment on attachment 198527 [details] [review]
Prune CoverArtDownloads cache when removing tracks

Thanks for updating the patch.
Committed with some changes:
http://git.gnome.org/browse/banshee/commit/?id=9731c72
Comment 6 Bertrand Lorentz 2011-10-09 15:04:55 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.

To answer your previous question, there's no real difference between calling DbConnection.Execute() with a string or with a HyenaSqliteCommand :
DbConnection.Execute(string) is a convenience method that creates a HyenaSqliteCommand for you.
But if the same query is run often, it's better to store the HyenaSqliteCommand somewhere, to avoid recreating an instance every time. See what I did in the committed change linked above.