background icon loading #1369
background icon loading #1369
Conversation
The previous commit starts icon loading on another thread, but for the UI to know when the icon is ready, every instance of `loadIconForObject` would have to be modified to call `updateIcon`. Almost all of them currently call `setIcon`. This should allow `setIcon` to correctly handle updated icons, but without sending a notification every time it's called.
loadIconForObject is already on a background thread
For file objects, `loadIconForObject` was doing two things: loading a more generic icon that can be gotten to quickly, then calling `previewIcon` to handle the more expensive preview generation in the background. The first task is the job of `setQuickIconForObject`, so that code has been relocated to that method. The expensive preview generation was moved from `previewIcon` to `loadIconForObject`, since it always runs on a background thread now.
|
I meant to say this isn't necessarily intended for the next release. Just as well, since I've got a reproducible crash when browsing iTunes. Not sure if it's caused by these changes, but the timing definitely suggests that. |
There are lots of crashes on the server relating to icon loading. I'm pretty sure they were introduced by the #1142 (sorry @tiennou!) $ ssh qs
$ cd [hidden]/3942/
# get number of ß71 crash report files containing 'QSIconLoader'
$ grep -r 'QSIconLoader' ./* | grep -c '.'
>> 126
# get total number of files
$ find ./* | grep -c '.'
>> 608So from that, 126/608 (21%) of crashes are related to QSIconLoader and I'm 99% sure #1142. But not to blame that pull too much, since we've always has QSIcon multi-threading problems, just like we have QSCatalogEntry :) |
|
No, pretty sure this is new and different. Think it might be that the notification is posted before the change happens. It should probably come after. I'll try it when I get home. |
It was used in very few places.
|
Tracked it to an exception in the iTunes plug-in, so this is fine. (But for some reason, I can't reproduce the exception unless I use this branch.) I've found some stuff that needs fixing over there anyway, so I'll look at that as well. |
|
BTW, I was still investigating these changes a bit. As far as what I see in the interface, it all seems to work exactly as it should. I just can't figure out why, because the modified notification is almost never sent when browsing files. But, like I said, seems to work. |
@neurolepsy pointed out that the iTunes plug-in was still generating artwork and previews on the main thread. I fixed it last night. It wasn't too difficult, but when I think about every developer having to go through that process for every plug-in, I don't like it. So I started thinking about a way for all the plug-ins to get this behavior "for free" and it turns out the way things were designed allows for this pretty easily.
We already have a method that stays on the main thread and throws up an inexpensive generic icon:
setQuickIconForObject. AndloadIconForObjectexists to take care of anything more expensive later on. Previously, we were modifying the latter in individual object sources to spawn yet another method in the background, which would post a notification when the icon was ready.I've changed things so
loadIconForObjectis itself always run on a background thread, so individual object sources don't need to worry about that.The only hitch was that the all the plug-ins' object sources would still need to be modified to call
updateIconinstead ofsetIconin order to post the notification. But I think I've found a way to makesetIcondo the right thing, eliminating the need to ever callupdateIcon. Basically, it only posts aQSObjectIconModifiednotification if an icon was previously set, and the one being currently set is different.As a result, the iTunes plug-in now generates artwork and previews in the background without any modifications to the plug-in itself. So will any others that make use of the two icon methods.
Seems to me like this is how it should work. :-)