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

Ranker options #1212

Merged
merged 3 commits into from Jan 15, 2013
Merged

Ranker options #1212

merged 3 commits into from Jan 15, 2013

Conversation

tiennou
Copy link
Member

@tiennou tiennou commented Nov 14, 2012

Remove the boatload of arguments passed to QSDefaultRanker and pass them in an NSDictionary instead. I've tried to keep compatibility with other rankers (TextStart anyone ?) but didn't really test thoroughly (I'll try it and add commits if that's the case). Be aware that until the others rankers get updated, they will happily ignore unknown options.

Please consider that I did that because calling through [[NSUserDefault standardUserDefaults] boolForKey:@"QSUsePureStringRanking"] each time you'd be ranking something was apparent in an Instrument run I did to find out why the hell the result list took so long to keyboard through (both from the direct pane and the action pane).

My finding point out this thing as one culprit, the other being animating the interface when there's a indirect pane to show. But that's for another pull req ;-).

@skurfer
Copy link
Member

@skurfer skurfer commented Nov 16, 2012

Haven't looked to see exactly what the problem is, but if you do some text ⇥ Find With…, the third pane is empty. It should contain all of your web searches, in order by rank.

@tiennou
Copy link
Member Author

@tiennou tiennou commented Nov 18, 2012

Heh, silly me ;-).

NSMutableArray *rankObjects = [QSDefaultObjectRanker rankedObjectsForAbbreviation:searchString inSet:set inContext:searchString mnemonicsOnly:mnemonicsOnly];
if (!searchString) searchString = @"";

BOOL usePureStringRanking = [[NSUserDefaults standardUserDefaults] boolForKey:@"QSUsePureStringRanking"];
Copy link
Member

@skurfer skurfer Jan 2, 2013

Choose a reason for hiding this comment

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

There's a macro defined for this called QSUsePureStringRanking in QSObjectRanker.m. Should we move that to QSLibrarian.h and use it instead of assigning it to a variable?

Copy link
Member

@skurfer skurfer Jan 2, 2013

Choose a reason for hiding this comment

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

Oh, never mind. I should have kept reading. :-)

Copy link
Member Author

@tiennou tiennou Jan 2, 2013

Choose a reason for hiding this comment

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

Heh ;-). The actual reason was that asking for that default before ranking each object was slightly inefficient. And then I went overboard and did that options thing ;-).

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jan 10, 2013

Is your thinking that once this is merged, we can do something like you mentioned here. So I can close that pull request right?

Do you want to add it to this pull, before we forget?

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jan 10, 2013

I've made the changes, feel free to just cherry pick my commit 321a808

I now see @tiennou's point that if you omit items, you omit them for a reason - i.e. you do not want to see them at all. But I still think that the 'omit' option is to 'omit the items from showing in the catalog (when you search)', not 'omit the items from everywhere in Quicksilver'.
I think we have the right behaviour here

@skurfer
Copy link
Member

@skurfer skurfer commented Jan 10, 2013

Is your thinking that once this is merged, we can do something like you mentioned here. So I can close that pull request right?

I thought we were going to close this and do something similar on #1198, but either way is fine with me.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jan 10, 2013

I was going to close that, since it's a bit messy now (the commits I'd
originally made are redundant after Etienne's changes)

On 10 January 2013 14:13, Rob McBroom notifications@github.com wrote:

Is your thinking that once this is merged, we can do something like you
mentioned here. So I can close that pull request right?

I thought we were going to close this and do something similar on #1198#1198,
but either way is fine with me.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1212#issuecomment-12098101.

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