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

Load web search icons asynchronously (fixes #637) #805

Merged
merged 2 commits into from Apr 23, 2012

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Apr 11, 2012

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

Move web search icon setting into QSObject_URLHandling
@skurfer
Copy link
Member

@skurfer skurfer commented Apr 11, 2012

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
Copy link
Member

@skurfer skurfer commented Apr 11, 2012

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
Copy link
Member Author

@pjrobertson pjrobertson commented Apr 11, 2012

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
Copy link
Member Author

@pjrobertson pjrobertson commented Apr 11, 2012

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
Copy link
Contributor

@HenningJ HenningJ commented Apr 11, 2012

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
Copy link
Member Author

@pjrobertson pjrobertson commented Apr 16, 2012

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
Copy link
Member

@skurfer skurfer commented Apr 16, 2012

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
Copy link
Contributor

@HenningJ HenningJ commented Apr 16, 2012

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
Copy link
Member

@skurfer skurfer commented Apr 17, 2012

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.

…urrentOperationCount` for preview image loading
@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Apr 19, 2012

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

I've added another commit

skurfer added a commit that referenced this issue Apr 23, 2012
Load web search icons asynchronously (fixes #637)
@skurfer skurfer merged commit 73c1f3f into quicksilver:master Apr 23, 2012
@skurfer
Copy link
Member

@skurfer skurfer commented Apr 23, 2012

Merged

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

3 participants