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

Don't discriminate the ranking of actions, fixes #853 #961

Merged
merged 3 commits into from Aug 21, 2012

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jun 26, 2012

Treat capitalised and non-capitalised keys in the same way (i.e. treat all characters as lower case chars)

Treat capitalised and non-capitalised keys in the same way
@skurfer
Copy link
Member

@skurfer skurfer commented Jun 26, 2012

[self isEqual:[self actionSelector]] means the 2nd pane has focus, right? What if you’re in the first pane holding shift?

What about just lower-casing everything prior to comparison in the matching algorithm (and storing the lower-case string as the mnemonic)?

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jul 3, 2012

What about just lower-casing everything prior to comparison in the matching algorithm (and storing the lower-case string as the mnemonic)?

So you're suggesting we don't discriminate by case ANYWHERE (not for catalog items or actions)?

@skurfer
Copy link
Member

@skurfer skurfer commented Jul 12, 2012

So you're suggesting we don't discriminate by case ANYWHERE (not for catalog items or actions)?

Right. We’re not supposed to be now, are we? Isn’t that the bug you’re trying to fix?

If you’re holding ⇧ in the first pane, the keystrokes affect the action (depending on prefs). So there’s no capitalization taking place in the first pane because you’re not “typing” there, and since existing mnemonics for the actions are probably all lower case, we should compare against those to take advantage of training, so that shouldn’t be capitalized either.

And if you holding ⇧ in the second pane, you wouldn’t want to consider case for the same reason (previously stored mnemonics for actions are probably all lower case).

I mean, we should consider capitalized keys in the sense that holding ⇧ should respect the pref and change the action instead of the dObject, but… you get what I mean, right?

Hopefully I didn’t just confuse things more. :-)

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jul 13, 2012

If you’re holding ⇧ in the first pane, the keystrokes affect the action
(depending on prefs)

But if you have that prefs disabled, would you want capital keys to mean
something different? So that in the 1st pane you could type "A" to get
"Address Book" and "a" to get "another object"?
Of course, I'll have the option turned on like most 'power' users, so I'm
not really sure what typical users will do. It is disabled by default
though.

On 12 July 2012 21:41, Rob McBroom <
reply@reply.github.com

wrote:

So you're suggesting we don't discriminate by case ANYWHERE (not for
catalog items or actions)?

Right. We’re not supposed to be now, are we? Isn’t that the bug you’re
trying to fix?

If you’re holding ⇧ in the first pane, the keystrokes affect the action
(depending on prefs). So there’s no capitalization taking place in the
first pane because you’re not “typing” there, and since existing mnemonics
for the actions are probably all lower case, we should compare against
those to take advantage of training, so that shouldn’t be capitalized
either.

And if you holding ⇧ in the second pane, you wouldn’t want to consider
case for the same reason (previously stored mnemonics for actions are
probably all lower case).

I mean, we should consider capitalized keys in the sense that holding ⇧
should respect the pref and change the action instead of the dObject, but…
you get what I mean, right?

Hopefully I didn’t just confuse things more. :-)


Reply to this email directly or view it on GitHub:
#961 (comment)

@skurfer
Copy link
Member

@skurfer skurfer commented Jul 13, 2012

But if you have that prefs disabled, would you want capital keys to mean something different?

Well, I don’t know. I thought it always ignored case, but I see some uppercase letters in my Mnemonics.plist. So I guess it depends on what newish users expect, which can only guess on. I think the fact that “saf” matches “Safari” gives users the impression that case doesn’t matter, so I think we can proceed as though it doesn’t.

But either way, my original question still stands: Does your change only apply to typing in the second pane? The main bug was encountered when typing in the first pane while holding ⇧. In fact, if your change does what I think it does, it’s actually worse because typing with ⇧ in the first pane is still looking for uppercase mnemonics, but there’s no way to “train” for those mnemonics because you can’t enter uppercase in the 2nd pane any more. Right?

I guess I could pull this and actually try it, but that’s hard. :-)

Don't treat capital and lowercase searches as the same
@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Aug 12, 2012

OK, so I'm still going to avoid answering your original question.... :)

I think you might be happy now, in that QS no longer discriminates anywhere by case (1st pane, 2nd pane, 3rd pane, wherever). Lowercase is always set in abbreviations etc.
I agree with you now that this is probably the most consistent behaviour.

Maybe after a week or so off you might be feeling adventurous enough to pull and try this now? ;-)

At the moment, pressing ⇧letter when the 1st pane is empty causes all sorts of problems
@skurfer
Copy link
Member

@skurfer skurfer commented Aug 21, 2012

Looks good. Any chance you could sneak in a fix for #1030?

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Aug 21, 2012

Once I have a 10.8 machine to test, sure thing :)

On 21 August 2012 16:01, Rob McBroom notifications@github.com wrote:

Looks good. Any chance you could sneak in a fix for #1030#1030
?


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

@skurfer
Copy link
Member

@skurfer skurfer commented Aug 21, 2012

Oh, 10.8. Sorry, wasn't paying attention. :-)

skurfer added a commit that referenced this issue Aug 21, 2012
Don't discriminate the ranking of actions, fixes #853
@skurfer skurfer merged commit e1ba0a4 into quicksilver:master Aug 21, 2012
@philostein
Copy link
Contributor

@philostein philostein commented Aug 25, 2012

Just updated to ß70 pre, and hitting ⇧letter changes to object not the action. 'Allow capitalised…' is checked. I see that case is ignored correctly otherwise - 'a' and 'A' get the same result. Hitting ⇧letter in pane 2 gets the correct action.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Aug 25, 2012

Hmmm.... if I press Shift letter when I'm in the first pane, focus is
shifted to the 2nd pane and the action is changed. Do you not get this?
Not sure what "hitting ⇧letter changes to object not the action" means?

On 25 August 2012 05:57, philostein notifications@github.com wrote:

Just updated to ß70 pre, and hitting ⇧letter changes to object not the
action. 'Allow capitalised…' is checked. I see that case is ignored
correctly otherwise - 'a' and 'A' get the same result. Hitting ⇧letter in
pane 2 gets the correct action.


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

@skurfer
Copy link
Member

@skurfer skurfer commented Aug 25, 2012

Not sure what "hitting ⇧letter changes to object not the action" means?

I think you're talking about #1030, which is a problem in 10.8. I don't think it matters which version of QS you use.

@philostein
Copy link
Contributor

@philostein philostein commented Aug 26, 2012

@pjrobertson Sorry, "changes the object", not "changes to object". Basically, pane 1's object is changed before the focus is moved to pane 2 (where the action remains the same).

@skurfer OK, I'll move it there.

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