Load web search icons asynchronously (fixes #637)

[pjrobertson]

This uses bits of Henning's code (thanks!) which was originally used to make the loading of file icons asynchronous.

Also - general tidy up and I've moved the web search icon methods into QSObject_URLHandling.
There's also a memory leak fix in the getFavIcon method

[skurfer]

Awesome. We should wait on a B67 pre-release for this. (I’ll try to get it in quickly.) Between this and the moreComing/timers changes, we’ll have some pretty big performance improvements.

[skurfer]

So all in all, this is a pretty small change. Just one question (related to this, but nothing to do with your changes). I was poking around trying to understand this better and I noticed in QSLibrarian -init, we do this:

[previewImageQueue setMaxConcurrentOperationCount:1];

From the documentation:

If you specify the value NSOperationQueueDefaultMaxConcurrentOperationCount (which is recommended), the maximum number of operations can change dynamically based on system conditions.

Any reason we shouldn’t use that instead of 1?

Edit: I should @ mention @HenningJ since he’ll be better able to answer that.

[pjrobertson]

Cool. I'll try and comment on the Prefs clean up stuff so we can try and
wrap that up as well.

P.S. I've committed a couple of things to master - the QSURLTypeParsers got
deleted from the core info.plist in the deprecated pull request

Seems like Xcode 4 doesn't like the <key></key> (empty key) on line 2273
of the .plist. If you open the .plist up in Xcode you'll see. Put in a
space between the and you'll see Xcode sees it fine
Just a heads up - which I'm sure you know anyway - Xcode sucks for .plists!

On 11 April 2012 13:44, Rob McBroom <
reply@reply.github.com

wrote:

Awesome. We should wait on a B67 pre-release for this. (Ill try to get it
in quickly.) Between this and the moreComing/timers changes, well have
some pretty big performance improvements.

---

Reply to this email directly or view it on GitHub:
#805 (comment)

[pjrobertson]

Any reason we shouldnt use that instead of 1?

I think it was @HenningJ who created this code, so it would be best to find
out from him!
I don't know enough about it, but from the docs you've pasted, that looks
better.

On 11 April 2012 14:02, Rob McBroom <
reply@reply.github.com

wrote:

So all in all, this is a pretty small change. Just one question (related
to this, but nothing to do with your changes). I was poking around trying
to understand this better and I noticed in QSLibrarian -init, we do this:

[previewImageQueue setMaxConcurrentOperationCount:1];

From the documentation:

If you specify the value
NSOperationQueueDefaultMaxConcurrentOperationCount (which is
recommended), the maximum number of operations can change dynamically based
on system conditions.

Any reason we shouldnt use that instead of 1?

---

Reply to this email directly or view it on GitHub:
#805 (comment)

[HenningJ]

Any reason we shouldn’t use that instead of 1?

For the Quicklook Icon creation thing, 1 made sense, since that works one at a time anyways.

But for this one, NSOperationQueueDefaultMaxConcurrentOperationCount might make more sense. Or the maximum number of connections you can have to an URL (is such a thing exists).

I think they should be separate anyways, and not use the same queue.

[pjrobertson]

I think they should be separate anyways, and not use the same queue.

I personally don't know enough about NSOperations to do that, and actually don't think it's necessary to create a whole new queue for the perhaps 20 or so icons that typical users will require to be loaded. From my point of view this is 'complete', unless we decide to go for NSOperationQueueDefaultMaxConcurrentOperationCount.

Henning - you're saying that the quicklook task (quicklookd I think) can only update icons one at a time? Rubbish!

[skurfer]

For the Quicklook Icon creation thing, 1 made sense, since that works one at a time anyways.

Where do you see that? And will that always be the case? Is there any downside to using the default and letting the system manage this on-the-fly?

I think they should be separate anyways, and not use the same queue.

Since the existing NSOperationQueue handles slow-loading icons, it seems reasonable to use it for this. Can you elaborate a bit on why they should be split?

[HenningJ]

I've been thinking about this, but I can't remember where I got this from. I'm sure there was a good reason for it, but maybe I'm wrong. Just try setting it to NSOperationQueueDefaultMaxConcurrentOperationCount and if it works fine...great. :-)

Since the existing NSOperationQueue handles slow-loading icons, it seems reasonable to use it for this. Can you elaborate a bit on why they should be split?

Primarily, because it seems cleaner. Apart from that: If you just browsed through a folder with lots of big files, it takes a while to process all the preview icons. If you then try to create the web search icon, it has to wait until the preview icon processing is completed. But since all of that happens in the background anyway, it might not be a big deal.
And if it actually can process several at a time, the delay wouldn't be that long either.

So go ahead, do whatever you like. :-)

[skurfer]

Just try setting it to NSOperationQueueDefaultMaxConcurrentOperationCount and if it works fine...great. :-)

I did and it does, so I say we flip it on here.

But since all of that happens in the background anyway, it might not be a big deal.

Probably not enough to justify the changes. These delays are very small anyway. They’re awful if you have to sit and wait for them, but if they just happen as you work, it’s hardly noticeable.

FYI, in testing this I’ve seen a problem that I hadn’t noticed before. When icon loading completes, the interface doesn’t actually update unless you select an item, or scroll it off screen and back. (If part of the icon was off screen, just that part will be updated.) I went back and found that this started in B61, so it was almost certainly introduced in #388. I think the problem is in the updateObject: method of QSSearchObjectView.

[pjrobertson]

I had a play with NSOperationQueueDefaultMaxConcurrentOperationCount and it seemed to work well for me.

I've added another commit

[skurfer]

Merged