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

Getting objects by type #1162

Merged
merged 6 commits into from
Nov 10, 2012
Merged

Getting objects by type #1162

merged 6 commits into from
Nov 10, 2012

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Oct 9, 2012

I've added a new method to QSLibrarian that will return an array of objects of a certain type sorted by score. So instead of calling something like

[QSLib scoredArrayForString:nil inSet:[QSLib arrayForType:@"QSBlahType"] mnemonicsOnly:YES];

you can just call

[QSLib scoredArrayForType:@"QSBlahType"];

I've also made a small change to the method that builds type arrays so it will include proxy objects. This means when calling QSLib arrayForType: or QSLib scoredArrayForType:, relevant proxy objects will be returned along with everything else.

Some real-world examples of where this is useful:

  • Text ⇥ Find With… ⇥ Current Website (Safari)
  • File ⇥ Copy To… ⇥ Finder Selection

And, in returning results sorted by score, I uncovered (and fixed) some bugs in the scoring algorithm. The first three commits address that.

What it was supposed to do: If you search for "xy" and that's never been used before, but "xyz" has been used (or vice versa), increase the score for "xy" a bit based on how often "xyz" has been used.

What it was doing: If the search string was empty, the comparison would always think there was a matching prefix for every abbreviation ever used, so the score would be increased a bit for each one. As a result, if something had 4 abbreviations used 10 times each (40 total) and something else had 3 abbreviations used 1000 times each (3000 total), the thing used 40 times would get a higher rank.

So, I told it not to use that technique for empty strings, and additionally defined specific logic for scoring objects when the search string is nil. (The total usage for all abbreviations is considered. The actual number of abbreviations has no effect.)

`prefixCompare()` will always return NSOrderedSame if one of the
strings is empty, causing an unreasonable increase in score.
Calling `types` for two reasons

  1. It's simpler. All we need is an array of type strings, not the
entire dictionary.
  2. Calling it on a proxy object returns the types for the resolved
object, allowing the proxy to be correctly categorized.
ndreas added a commit to quicksilver/GoogleChrome-qsplugin that referenced this pull request Oct 11, 2012
@skurfer
Copy link
Member Author

skurfer commented Oct 16, 2012

I see NSAccessibilityException over and over when selecting the Move To…/Copy To… actions, but this appears to be a problem with the Current Document proxy. I see the same thing when selecting that proxy object directly in the first pane.

I've also noticed that the inclusion of proxy objects slows down the selection of those actions quite a bit (because they get resolved on the spot). I'm not sure what they best solution is for that. Possibilities:

  • Don't resolve proxies until an action is run. Now that they are resolved automatically when actions are run, I wonder if we need to resolve them upon selection. The only reason I can think of is that some might alter their types (and the available actions) depending on context. But most have predictable types. I wonder if we should add a flag to the QSProxies dictionary to identify those that need to be resolved immediately.
  • Allow them to be resolved when selected in the first pane, but not the third. That seems like a bit of a hack though.
  • Don't pre-populate the third pane for Move To…/Copy To…. (I thought we just did something like this. Was it only when those actions are called via a trigger?)

@pjrobertson
Copy link
Member

Allow them to be resolved when selected in the first pane, but not the third. That seems like a bit of a hack though.

Although you say this sounds like a hack, I think it sounds the most plausible. If the proxy isn't actually the right type then you just get an NSBeep or something. Option 1 is pretty similar to this option, except this still resolves the proxy when it's in the 1st pane - a must

Don't pre-populate the third pane for Move To…/Copy To…. (I thought we just did something like this. Was it only when those actions are called via a trigger?)

Yeah, just for triggers so that the default option isn't run. We specifically changed the Move To.../Copy To... actions not too long ago to show the 'current folder' in the 3rd pane to make it easier to move/copy the file(s) to a folder 'close by'

@pjrobertson
Copy link
Member

What's the outcome of this? Are you planning on doing this:

Allow them to be resolved when selected in the first pane, but not the third. That seems like a bit of a hack though.

@skurfer
Copy link
Member Author

skurfer commented Nov 5, 2012

Yes, I’m still planning to look into it.

@skurfer
Copy link
Member Author

skurfer commented Nov 5, 2012

for B71 I think the fix for this depends on #1180. Once we have that, when populating the third pane for Move/Copy To…, we can just test [thisObject isFolder] && ![thisObject isPackage] instead of resolving the path and doing tests on it with NSFileManager and NSWorkspaceManager. That's why it takes so long.

It allowed proxy objects to appear in the third pane, but it could cause serious problems when validating those objects for some actions. Hopefully we can revisit this one day.
@skurfer
Copy link
Member Author

skurfer commented Nov 9, 2012

Reversed the last commit until I can figure out why it causes so many problems when populating the third pane for Moe To/Copy To. It was a minor thing, anyway.

This should be OK to merge now.

pjrobertson added a commit that referenced this pull request Nov 10, 2012
@pjrobertson pjrobertson merged commit 31f2b08 into master Nov 10, 2012
@pjrobertson
Copy link
Member

@skurfer any ideas how I can reproduce the exception? Other than re-instating your reverted commit?

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.

2 participants