Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

background icon loading #1369

Merged
merged 7 commits into from Mar 6, 2013
Merged

background icon loading #1369

merged 7 commits into from Mar 6, 2013

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Feb 2, 2013

@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. And loadIconForObject exists 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 loadIconForObject is 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 updateIcon instead of setIcon in order to post the notification. But I think I've found a way to make setIcon do the right thing, eliminating the need to ever call updateIcon. Basically, it only posts a QSObjectIconModified notification 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. :-)

skurfer added 4 commits Feb 2, 2013
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.
@skurfer
Copy link
Member Author

@skurfer skurfer commented Feb 3, 2013

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.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Feb 3, 2013

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!)
If you can get a reproducible crash then don't ignore it as it may be related!

$ 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 '.'
>> 608

So 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 :)

@skurfer
Copy link
Member Author

@skurfer skurfer commented Feb 3, 2013

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.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Feb 3, 2013

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.

pjrobertson added a commit that referenced this issue Mar 6, 2013
@pjrobertson pjrobertson merged commit d64d884 into quicksilver:master Mar 6, 2013
@skurfer skurfer deleted the bgIconLoading branch Mar 6, 2013
@skurfer
Copy link
Member Author

@skurfer skurfer commented Mar 6, 2013

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants