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

remove the moreComing test #780

Merged
merged 1 commit into from Mar 30, 2012
Merged

remove the moreComing test #780

merged 1 commit into from Mar 30, 2012

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Mar 29, 2012

I can’t find a purpose for this check, but it definitely causes a problem (#758).
I’ve been running with this for over a week and haven’t seen any ill effects.

If I understand what it was trying to do (avoid unnecessary expensive searching), this is handled quite well by the “Wait before searching” user preference so it seems redundant. Setting that preference seems to make it wait based on the last keystroke entered (not the first).

It could predate this change, but as I’ve started paying more attention, I’ve never seen Quicksilver faster. No matter how quickly I type, results are in the first pane immediately. So this certainly doesn’t hurt and might speed things up.

@tiennou
Copy link
Member

@tiennou tiennou commented Mar 29, 2012

Hi ;-). I'm chiming in because I remember trying to guess what was the point of that check too ;-).

It prevents a search in case more keydowns are waiting in the event loop, so that the search is only done after every typed key is registered.
For example when you enter 'fire', the keydown event will cause QS to start a search with the current search string (which is 'f' at this time) which will give lots of results, then the next runloop will update the search string again (causing a new search). Repeat for each key pressed ;-).

That check is just a runloop lookahead so that the search doesn't start until the whole user input has been processed. I've read #758 and daniels220's explanation looks plausible but I can't think of a valid reason why the return key would get processed before other events but I'm not looking at the source code, so there might be another runloop shortcut mechanism somewhere... HTH. Feel free to drop the check entirely if it's not causing issues, from the top of my head, if you have a massive catalog, it could cause stalls while the search gets restarted at each letter entered, which could slow things down.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Mar 29, 2012

More coming was always an alpha thing anyway. If you remember back into the day when you removed the feature level stuff (somehow, I can still remember your changes...) you'll see that it got moved from being an alpha feature to a full on feature: http://d.pr/CUnA

The comment on line 33 of the diff also shows what it was meant to do, as I think you've already figured out:

    // check for additional keydowns up to now so the search isn't done too often.

If it was an alpha feature, and it was definitely causing me the same pains as seen in #758 then I'm happy.

Merged :-)

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Mar 29, 2012

OK I haven't merged, as I've seen Etienne's comment.

@tiennou - as Rob suggests, I think the whole point of the 'wait before searching' preference is to stop what you're saying. If you have a slow computer, then you can just up that.
Plus it was originally an alpha feature, plus - who says that we typically spend 0.05s or less between each key stroke (as SEARCH_RESULT_DELAY suggests) :-)

I know you were just chiming in Etienne, and weren't against this, but I thought I'd share what more I know now.

As an aside: I've never liked this method (used for the QSDockingWindows) as it clogs the main run loop for up to the expiration NSDate. I'll be glad to see the back of it :)

- (NSEvent *)nextEventMatchingMask:(NSUInteger)mask untilDate:(NSDate *)expiration inMode:(NSString *)mode dequeue:(BOOL)flag

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Mar 29, 2012

P.S. sorry, I meant to attach a diff of Rob's featureLevel changes way back

Either run git diff 69d06b16e80d112dcf26526af7694201da06f10a 030aa135845d08735bfaf233c937dc016e0b710e
or look at this file: http://d.pr/CUnA

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Mar 30, 2012

I can't believe how much better this is!

Merged

pjrobertson added a commit that referenced this issue Mar 30, 2012
remove the `moreComing` test
@pjrobertson pjrobertson merged commit fb089d0 into quicksilver:master Mar 30, 2012
@skurfer
Copy link
Member Author

@skurfer skurfer commented Mar 30, 2012

I can't believe how much better this is!

So it’s not my imagination. Nice!

I can see the problem it would have prevented in theory, but if the user sets a delay before searching, Quicksilver should have no problem keeping up with keystrokes. That’s purely on faith. I have no proof. :-)

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