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

Add option "switch to US keyboard when activated" #987

Merged
merged 6 commits into from
Aug 9, 2012

Conversation

ybian
Copy link
Contributor

@ybian ybian commented Jul 12, 2012

When this option is turned on, once QS is activated, the input source is
changed to US automatically.

This is a frequently requested feature by non-English natives:

#371
https://groups.google.com/forum/?fromgroups#!topic/blacktree-quicksilver/Kb2yje7MI4I

When this option is turned on, once QS is activated, the input source is
changed to US automatically.
@pjrobertson
Copy link
Member

Great to see this greatly wanted feature being implemented, thanks!

A few small things: it appears you have a memory leak in both your bits of code you wrote. I think usInputSource should be released in the 1st bit, and in the 2nd bit, savedInputSource should be released (using CFRelease()).
It is also probably worth checking that savedInputSource is not nil in your 2nd bit of code. I have not tested it, but perhaps if the user ticks the checkbox whilst the Quicksilver window is still open, then, when the user closes the window your code will call TISSelectInputSource(savedInputSource); where savedInputSource is nil.

Also, I've just read the documentation:

For TISSelectInputSource to succeed, the input source must be selectable (that is, kTISPropertyInputSourceIsSelectCapable is set to true) and the input source must be enabled (that is, kTISPropertyInputSourceIsEnabled is set to true). Furthermore, if the input source is an input mode, its parent must be enabled for it to be selected.

What happens if the user doesn't have the US keyboard layout enabled on their system? If Quicksilver uses the US keyboard layout, I don't think the user should have to enable that keyboard layout in the system preferences.

Finally, it might be nice to have a drop down instead of a checkbox, to allow users to select their own keyboard layout. E.g. British, French etc.

@ybian
Copy link
Contributor Author

ybian commented Jul 12, 2012

Thanks for your quick review and response! I should be able to fix those things when I get some time tomorrow.

@pjrobertson
Copy link
Member

Great, thanks Bian!

On 12 July 2012 09:05, Bian Ying <
reply@reply.github.com

wrote:

Thanks for your quick review and response! I should be able to fix those
things when I get some time tomorrow.


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

@tiennou
Copy link
Member

tiennou commented Jul 12, 2012

+1 for the dropdown, because I won't like my 'a' to turn into 'q', my 'z' into 'w', and my ';' into 'm' (and I'm missing a bunch) ;-). You can use TISCreateInputSourceList() to get to the list of enabled Input Methods (to build the menu), and kTISPropertyLocalizedName to localize their name. Bonus points if you register for the CF notification and rebuild the list when appropriate ;-).

@skurfer
Copy link
Member

skurfer commented Jul 12, 2012

FYI, @ybian if you add something like “fix #371” to the commit message, a link to here will automatically appear on the issue, and when the commit gets merged to master, the issue will automatically close. GitHub is full of hidden tricks. :-)

@ybian
Copy link
Contributor Author

ybian commented Jul 13, 2012

Thanks for all your comments. Very useful! I am trying to add a dropdown for user to select his perferred keyboard, but I haven't figured it out how to save the value of type TISInputSourceRef into NSUserDefaults. Can anyone shed some light?

@tiennou
Copy link
Member

tiennou commented Jul 13, 2012

I think you can try storing the kTISPropertyInputSourceID of the Input Source instead.

@ybian
Copy link
Contributor Author

ybian commented Jul 13, 2012

Never mind. I don't have to save TISInputSourceRef. Just to save the property kTISPropertyInputSourceID will be OK.

@ybian
Copy link
Contributor Author

ybian commented Jul 13, 2012

@tiennou Thank you! I did not refresh this page before my last message. :-)

- Added a drop down besides checkbox so user can choose his own
  keyboard layout
- Memory leak and a few other fixes
With notificaiton observer added, it is no longer needed to popup the
menu every time when the pane is selected.
- (void)updateKeyboardPopUp {
[keyboardPopUp removeAllItems];

CFArrayRef sourceList= TISCreateInputSourceList(NULL,false);
Copy link
Member

Choose a reason for hiding this comment

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

Quick question, would it be possible to restrict the search for layouts or input mode there instead of looping over the results afterward ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, unless I missed something obvious. The filtering parameter is simply a CFDictionaryRef with key/value pairs. I don't think it can express the logic "or" in L122.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you can't express an or in the filter, can't you just call TISCreateInputSourceList twice, once for kTISTypeKeyboardLayouts and once for kTISTypeKeyboardInputModes. And then merge the two lists? I don't know if that's better than it is right now, but it might be worth a try.

Copy link
Member

Choose a reason for hiding this comment

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

That's what I was thinking too ;-). I just feel uncomfortable with the note under TISCreateInputSourceList's docs w.r.t to 120Kb usage...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 120Kb usage only occurs if the second parameter is true, but we all call it with false. Although it is arguable if it is better, it seems your guys feel more comfortable with the call twice way. I pushed a new commit. Please review. By the way, I think the real benefit of this change is that all kTISKeyboardLayout items which are more likely to be chosen by users appear naturally first in the list.

Use 2 calls to TISCreateInputSourceList instead of one call plus custom
filtering.
@ybian
Copy link
Contributor Author

ybian commented Aug 1, 2012

Guys, any more comments on this? I've been using this feature for weeks personally and I really think someone else would like this as well. ;-)

@pjrobertson
Copy link
Member

Sorry for not getting on this sooner Brian,

I was waiting on @tiennou to perhaps get back with his thoughts on the 120Kb usage thing, but from what I see, it looks fine.

I'll test this now to see how things stand

@pjrobertson
Copy link
Member

A few small/pedantic things:

  • I'd bind the 'Enabled' property of the popup in the prefs to QSSwitchKeyboardOnActivation so things look a bit more obvious to the user

  • I think it would be good to add a tooltip to the popup saying something like "To enable more input keyboards, open the Input Sources preferences and add them to your list of active keyboards" (or something slightly better worded)

  • Just run this for the first time, and I got a crash at QSMainPreferencePanes.m line 117. For me, sourceList is nil. Not sure why, but maybe you should check it isn't nil before continuing. I'm not sure if casting sourceRef to an NSArray * might help.
    A little more info: the code runs fine when TISPropertyInputSourceType = TISTypeKeyboardLayout;, but I get nil when TISPropertyInputSourceType = TISTypeKeyboardInputMode;. I only have one keyboard layout enabled, and that's British English. I can confirm that adding this to line 117 works:

    if (!sourceList) {
        continue;
    }
    
  • Is there any particular reason why you've chosen 5 as the number of sourceNames? (line 112) Might be over ambitious in most cases, but really won't make much of a difference overall

  • I thought I'd be able to break things if I disabled the input source I'd selected in QS, but you've thought about this already ;-)

  • Awesome. Works really well apart from the small crash. You're gonna make a lot of people, very, very happy!

@ybian
Copy link
Contributor Author

ybian commented Aug 2, 2012

Patrick, thanks a lot for your time to do the testing! See my comments:

  • Good suggestion. I will do that.
  • This does not sound necessary to me. I think users that care about this option should already have the experience with input sources preferences. The sources of this list are very obvious.
  • I cannot reproduce it here, but anyway, I think a safe check like what you suggested is a good thing to add. I will add it.
  • There is no particular reason for the value 5. I just guess most users should not have more than 5 keyboards configured.
  • Thanks to comments of others in previous review of my code.
  • I hope so as well!

A new commit will be added soon.

1. Bind "Enabled" of the popup to "QSSwitchKeyboardOnActivation"
2. Add a safe check for 'sourceList'
QSActionsPrefPane.xib should not be touched in previous commit
@pjrobertson
Copy link
Member

This all looks fine to me. @tiennou are you about to comment? Do you have any further queries?

@tiennou
Copy link
Member

tiennou commented Aug 9, 2012

Looks good to me ;-). Thanks !

El 4 août 2012, a las 02:06, Patrick Robertsonreply@reply.github.com escribió:

This all looks fine to me. @tiennou are you about to comment? Do you have any further queries?


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

@pjrobertson
Copy link
Member

Merged.

pjrobertson added a commit that referenced this pull request Aug 9, 2012
Add option "switch to US keyboard when activated"
@pjrobertson pjrobertson merged commit 560e9e9 into quicksilver:master Aug 9, 2012
@philostein
Copy link
Contributor

Since ß70, I can no longer search QS in Kana - Katakana or Hiragana. Text mode still works. It doesn't matter what input method I select in QS's prefs.

Edit It does work, there's just no visual feedback when typing. Pressing the down arrow shows the kanjii substitution list. Selecting a kanjii and pressing enter twice shows its matches in the search results list.

It's normal behaviour, when typing kana, for space to substitute the characters for the most common kanjii. QS implements its default spacebar behaviour instead. FYI, don't have call to use it much for searching. :)

@pjrobertson
Copy link
Member

Can you be more specific on your problems please Phil?

If you search using Kana etc., what do you get in the 1st pane? Is this
related to #1017 perhaps? Not a 10.8 problem altogether?
If you turn off the option in the prefs, what do you get? If you don't have
it enabled, then this code won't change anything in the way QS works

On 25 August 2012 06:13, philostein notifications@github.com wrote:

Since ß70, I can no longer search QS in Kana - Katakana or Hiragana. Text
mode still works. It doesn't matter what input method I select in QS's
prefs.


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

@philostein
Copy link
Contributor

In search mode in pane 1, typing hiragana or katakana shows nothing, but pressing enter returns the correct QS results. For kanjii, typing, pressing the down arrow, selecting a kanjii and pressing enter twice, causes QS to return search results.

Turning off the input option makes no difference.

I don't think it's a filesystem vs label issue, but I obviously could be wrong. :) I put it here because QS now specifically allows different language inputs on activation, and kana input works, but with no visual feedback of what's being typed. It's always been flaky, but I think using space to pull in the most common kanjii used to work, but now QS overrides it for its default spacebar behaviour.

@pjrobertson
Copy link
Member

but I think using space to pull in the most common kanjii used to work,
but now QS overrides it for its default spacebar behaviour.

Could you just confirm for us if things work on ß69? I'm stumbling around
blind here trying to figure out how to debug. I'm getting there with my
Japanese, but only know the Kana at the moment ;-)

On 26 August 2012 05:19, philostein notifications@github.com wrote:

In search mode in pane 1, typing hiragana or katakana shows nothing, but
pressing enter returns the correct QS results. For kanjii, typing, pressing
the down arrow, selecting a kanjii and pressing enter twice, causes QS to
return search results.

Turning off the input option makes no difference.

I don't think it's a filesystem vs label issue, but I obviously could be
wrong. :) I put it here because QS now specifically allows different
language inputs on activation, and kana input works, but with no visual
feedback of what's being typed. It's always been flaky, but I think using
space to pull in the most common kanjii used to work, but now QS overrides
it for its default spacebar behaviour.


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

@philostein
Copy link
Contributor

Downgraded, and, sorry, it doesn't work. System wide, typed kana is underlined. Pressing space results in the 'expected kanjii' to appear in its place. Pressing space again shows the alternatives list. Pressing return in the list pastes the selected characters over the typed text. Pressing return on underlined text 'fixes' the text as completed.

@pjrobertson
Copy link
Member

OK, so downgrading didn't fix the problem? I've just tried and I see that
typing Katagana/kana does nothing to the 1st pane, although the symbols are
shown
in the results window if you bring it up (in red, since there are no
matches)

With this new version, at least it's possible to switch to an input source
that does work, so are we losing anything? I'm still in the dark about what
exactly is/isn't working for you ;-)

On 26 August 2012 09:39, philostein notifications@github.com wrote:

Downgraded, and, sorry, it doesn't work. System wide, typed kana is
underlined. Pressing space results in the 'expected kanjii' to appear in
its place. Pressing space again shows the alternatives list. Pressing
return in the list pastes the selected characters over the typed text.
Pressing return on underlined text 'fixes' the text as completed.


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

@philostein
Copy link
Contributor

It works, just invisibly until the text is finalised. Well, if anyone else gets confused about it, it's easy enough for me to explain what to do. :)

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

6 participants