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

Icon loading fixes #1142

Merged
merged 9 commits into from Oct 30, 2012
Merged

Icon loading fixes #1142

merged 9 commits into from Oct 30, 2012

Conversation

tiennou
Copy link
Member

@tiennou tiennou commented Sep 27, 2012

This fixes icon loading in the children result view, drops a private API, and switch the icon loader to a gcd queue.

You can try adjust the queue priority (I started at high, but finally settled on low as you can't really tell the difference anyway. Also we don't want to make people without SSDs unhappy, right ? ;-))

[[self retain] autorelease];
loadThread = [NSThread currentThread];
// [NSThread setThreadPriority:0.0];
dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_LOW, 0), ^{
NSArray *sourceArray = nil;
Copy link
Member

@skurfer skurfer Sep 27, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we just pass nil as the parameter on line 57 and get rid of this variable? It's value never gets changed that I can see.

Copy link
Member Author

@tiennou tiennou Sep 27, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's supposed to pass the array ivar. I will fix that.

Copy link
Member

@skurfer skurfer Sep 27, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, cool. I wondered if it should pass the ivar (and actually tried it), but couldn't see any difference in the way the interface behaved.

Copy link
Member Author

@tiennou tiennou Sep 27, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it won't change anything, since the delegate actually ignore it. But I will fix that anyway in the name of consistency ;-).

@skurfer
Copy link
Member

@skurfer skurfer commented Sep 28, 2012

OK, so I thought this was to replace the NSOperationQueue in QSLibrarian, but looking at the code, I see it isn’t. So is there any relationship between this and that? Does this make the NSOperationQueue unnecessary? The reason I started looking into it: Something in these changes causes Web Search icons to block the UI. They use the NSOperationQueue, which seems unaffected, so I don’t know what the issue is. But most of this is unfamiliar to me anyway. :-)

@tiennou
Copy link
Member Author

@tiennou tiennou commented Sep 28, 2012

I've checked that, and it looks like you're right, they are separate "queues". QSIconLoader is only responsible for loading the icons in the result controller (and the children view, which was buggy, and that this pulls fixes).

Now about the NSOperationQueue, maybe we should drop that and use one of the global dispatch queues. I tried removing it altogether (from QSURLObjectHandler and QSFileSystemObjectHandler) and moving that to dispatch_async, but maybe it could be done one level above, by wrapping the call of -[handler loadIconForObject:] in -[QSObject loadIcon] so that all handlers would benefit from async icon loading (as long as the QSObjectIconModified notification is set at the end). Only issue with that is that you can't return a BOOL from that but it looks like it's actually unused.

And the lag seems to be caused by -[QSURLObjectHandler loadChildrenForObject:] when you select a QSSearchURLType object, which happens on the main thread (the task that runs gave it away ;-)).

@skurfer
Copy link
Member

@skurfer skurfer commented Sep 28, 2012

And the lag seems to be caused by -[QSURLObjectHandler loadChildrenForObject:] when you select a QSSearchURLType object

Right you are. And even better… it gets called 3 times! It shouldn't be called at all when selecting an object (unless you enable that "Show children in results list" preference). Wait, now that I look back over this… Is that what this pull request is all about? The behavior with that preference enabled? :-) If so, great, but I think most people turn it off just to avoid the lag we're talking about.

@tiennou
Copy link
Member Author

@tiennou tiennou commented Sep 28, 2012

Yeah, that's look like the culprit. This now tries to load children icons even when the children result view is disabled. I'll fix that, but this means user of the children result view will still see the lag, unless we find a way to remove the call to +[NSURLConnection sendSynchronousRequest:...] in QSParser.m.

@skurfer
Copy link
Member

@skurfer skurfer commented Sep 28, 2012

Looks good. I'll test it out for a while.

One problem I ran into when trying to test this with some other branches: There’s a conflict with your other GCD branch. For some reason, when you try to merge them both, it wants to remove references to both previewImageQueue and indexationQueue. And if I resolve the conflict manually (leaving references to indexationQueue in tact), it will build, but it crashes immediately on scanning. :-( There’s no crash with either branch on its own. Just thought I’d let you know.

@tiennou
Copy link
Member Author

@tiennou tiennou commented Sep 28, 2012

Huh ? I've tried it, and while the merge is strange (since it deletes the indexationQueue) I haven't had crash (tried with my old Caches, then rm -Rf them, no difference). Can you get a backtrace ? Were you running with other branches merged ?

@skurfer
Copy link
Member

@skurfer skurfer commented Sep 28, 2012

Were you running with other branches merged?

Yes. Let me try starting from master and only merging those two.

@skurfer
Copy link
Member

@skurfer skurfer commented Sep 28, 2012

Hmmm, OK. Made a new branch from master, merged the two branches in question, resolved the conflict, and no crash. So then I merged in all the other junk from my current mine branch and still no crash. I can only guess that I resolved the conflict incorrectly before (though I tried more than once before mentioning it). So maybe it's nothing.

Hold on, got a crash in the background while typing this, but I'll see if it keeps happening before I waste your time with it.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Oct 30, 2012

Apart from the very small use of __block BOOL, this looks and works fine for me.
Perhaps c849dc3 could be removed instead of just commented out?

As mentioned by Rob, I also get get a few icons that don't necessarily load unless you click the object (mainly .avi), but I can reproduce in ß70 so not a problem here.

If there's no reply from @tiennou on the __block business by tomorrow then I'll merge this and add it myself ;-)

@skurfer
Copy link
Member

@skurfer skurfer commented Oct 30, 2012

As mentioned by Rob, I also get get a few icons that don't necessarily load unless you click the object (mainly .avi), but I can reproduce in ß70 so not a problem here.

I can reproduce this every time by going into ~/Music and finding the folder for an album. The first (selected) file's icon shows, but the others usually don't. Selecting a file makes the icon appear, but I've also found that simply scrolling the list makes them all appear immediately. So I think they are being loaded, but the display isn't updating.

If there's no reply from @tiennou on the __block business by tomorrow then I'll merge this and add it myself ;-)

Sounds good. I've been testing this for a long time without issue.

@skurfer
Copy link
Member

@skurfer skurfer commented Oct 30, 2012

Found a fix for the icons in the results list. I'll put it in another pull request after this is merged.

pjrobertson added a commit that referenced this issue Oct 30, 2012
@pjrobertson pjrobertson merged commit a58266e into quicksilver:master Oct 30, 2012
@skurfer skurfer mentioned this pull request Oct 30, 2012
@tiennou tiennou deleted the icon-loading-fixes branch Jul 20, 2016
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