Use handleEvent instead of interpretKeyEvents(again). #279

Closed
wants to merge 2 commits into
from

Projects

None yet

4 participants

@tai2
tai2 commented Apr 21, 2016 edited

Hi, I'm an AquaSKK IM user and encountered the same problem @mzp reported on issue #202 .
This patch is based on @mzp 's work and rebased to the current master.

In the issue, you pointed out the problem that patch breaks key repeating, as I met too, so I added additional condition check wether the event represents key repeating or not using event.isARepeat.

Honestly, I don't understand the iTerm2 codebase any little, but, at least, this patch didn't cause any problem while a week I was using this version.

@tai2 tai2 Use handleEvent instead of interpretKeyEvents.
This patch is based on #202
5dad811
@coveralls

Coverage Status

Coverage decreased (-0.007%) to 32.206% when pulling 5dad811 on tai2:fix_for_aquaskk into cfdceb9 on gnachman:master.

@coveralls

Coverage Status

Coverage decreased (-0.0003%) to 32.212% when pulling 5dad811 on tai2:fix_for_aquaskk into cfdceb9 on gnachman:master.

@gnachman
Owner

I can't understand what handleEvent: is supposed to do. Apple's documentation is not helpful. Do you have any sources of information on this? I'm worried about unexpected consequences and I want to understand it better.

@gnachman
Owner

One idea: what about calling handleEvent: only when there is marked text? My guess is that it's only useful when there's an input method editor.

@gnachman
Owner

I dug up this link which I'll add here for future reference (search for handleEvent:)

https://developer.apple.com/library/mac/documentation/TextFonts/Conceptual/CocoaTextArchitecture/TextEditing/TextEditing.html

@gnachman
Owner

From https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/EventOverview/HandlingKeyEvents/HandlingKeyEvents.html:

[keyDown:] can pass the event to Cocoa’s text input management system by invoking the NSResponder method interpretKeyEvents:. The input management system checks the pressed key against entries in all relevant key-binding dictionaries and, if there is a match, sends a doCommandBySelector: message back to the view. Otherwise, it sends an insertText: message back to the view, and the view implements this method to extract and display the text.

vs https://developer.apple.com/library/mac/documentation/TextFonts/Conceptual/CocoaTextArchitecture/TextEditing/TextEditing.html

During the processing of a keyboard event, the event passes through the NSMenu object, then to the first responder via the keyDown: method. The default implementation of the method provided by the NSResponder class propagates the message up the responder chain until an overridden keyDown: implementation stops the propagation. Typically, an NSResponder subclass can choose to process certain keys and ignore others (for example, in a game) or to send the handleEvent: message to its input context.

The input context checks the event to see if it matches any of the keystrokes in the user’s key-bindings dictionary. A key-bindings dictionary maps a keystroke (including its modifier keys) to a method name. For example, the default key-bindings dictionary maps ^d (Control-D) to the method name deleteForward:. If the keyboard event is in the dictionary, then the input context calls the text view’s doCommandBySelector: method with the selector associated with the dictionary entry.

If the input context cannot match the keyboard event to an entry in the key-bindings dictionary, it passes the event to the Input Method Kit for mapping to characters.

Then it goes on to talk about specifics of NSTextViews that aren't relevant to us.

@gnachman gnachman and 1 other commented on an outdated diff Apr 21, 2016
sources/PTYTextView.m
@@ -4831,8 +4833,10 @@ - (void)doCommandBySelector:(SEL)aSelector
// See comment in -keyDown:
DLog(@"Rerouting doCommandBySelector from %@ to %@", self, gCurrentKeyEventTextView);
[gCurrentKeyEventTextView doCommandBySelector:aSelector];
+ [self.delegate keyDown:_keydownEvent];
@gnachman
gnachman Apr 21, 2016 Owner

Why do you need this?

@mzp
mzp Apr 29, 2016

To preserve behaviour.

For example, when press Enter key, current behaviour is following:

  1. A key event is passed to interpretKeyEvents:.
  2. doCommandBySelector: is called.
  3. Because _inputMethodIsInserting is NO, [delegate keyDown:_keydownEvent](at 1445 line) is called.

But at our pull request:

    1. A key event is passed to handleEvent:.
  1. doCommandBySelector: is called. (same as current behaviour)
  2. _inputMethodIsInserting becomes YES, because input method handles it by calling doCommandBySelector:.
  3. [delegate keyDown:_keydownEvent](at 1445 line) is not called.

To prevent this, we add this line.

@gnachman
gnachman May 3, 2016 Owner

Thanks for the explanation. I see why this was done this way: doCommandBySelector: is a more appropriate place to handle the keystroke than in keyDown: when possible.

I can't find a case where -handleEvent: returns NO in this code path. Can you get it to return NO? Maybe it's specific to the input method (I tried US and Japanese Katakana) or some other system configuration.

@mzp
mzp May 4, 2016

-handleEvent: only handles keyboard event, but its signature accepts all NSEvent*. So when we pass mouse event, -handlEvent: return NO.

As you say, it seems always return YES as long as keyboard event.

@gnachman gnachman commented on the diff Apr 21, 2016
sources/PTYTextView.m
@@ -142,6 +142,7 @@ @implementation PTYTextView {
double _charHeightWithoutSpacing;
// NSTextInputClient support
+ NSEvent* _keydownEvent;
BOOL _inputMethodIsInserting;
@gnachman
gnachman Apr 21, 2016 Owner

This ivar is assigned to but never read so it can be removed

@mzp
mzp Apr 29, 2016

This instance variable is used at #279 (comment).

@gnachman gnachman commented on the diff Apr 21, 2016
sources/PTYTextView.m
@@ -1434,7 +1435,8 @@ - (void)keyDown:(NSEvent*)event {
// track the instance of PTYTextView that is currently handling a key event and rerouting
// calls as needed in -insertText and -doCommandBySelector.
gCurrentKeyEventTextView = [[self retain] autorelease];
- [self interpretKeyEvents:[NSArray arrayWithObject:event]];
+ _keydownEvent = event;
+ _inputMethodIsInserting = !event.isARepeat && [self.inputContext handleEvent:event];
@gnachman
gnachman Apr 21, 2016 Owner

Are you about to reproduce the problem with key repeat? On OS 10.11 it doesn't seem to happen.

@tai2
tai2 Apr 21, 2016

I'm using OS 10.11.4. The problem is still reproduced on this branch: tai2/iTerm2@b8cf2bc .

@gnachman
gnachman May 5, 2016 Owner

I disassembled -[NSView interpretKeyEvents:] on the 10.11 SDK and here's what it does:

- (void)interpretKeyEvents:(NSArray *)events {
  NSTextInputContext *inputContext = [self inputContext];
  if (!inputContext) {
    [super interpretKeyEvents:events];
    return;
  }
  for (NSEvent *event in events) {
    [inputContext handleEvent:event];
  }
}

(The annotated disassembly is here: https://gist.github.com/gnachman/f827f205e8b99fe35c9b5fbd2235aa28)

Are you absolutely sure that it's the call to handleEvent: that causes issues with key repeat?

@tai2
tai2 May 7, 2016 edited

Sure.

Follows are the steps exactly I did.

  • Repository: git@github.com:tai2/iTerm2.git
  • OSX 10.11.4
  • Input source is preinstalled U.S.
  • Build target is iTerm2
  1. git checkout tai2/iTerm2@cfdceb9 (use interpretKeyEvents)
  2. Clean and Run.
  3. Hold k in bash, it puts characters repeatedly.
  4. git checkout tai2/iTerm2@b8cf2bc (use handleEvent)
  5. Clean and Run.
  6. Hold k in bash, just one character is put.
  7. git checkout tai2/iTerm2@5dad811 (use handleEvent if not event.isARepeat)
  8. Clean and Run.
  9. Hold k in bash, the character repeats.

The key k is just an example, but here is a strange thing.
In step 6, some keys didn't repeat but the others did.

Unrepeatable characters: bdhjkmpqrtvwx01234567890
Repeatable characters: acefgilnosuyz`~!@#$%^&*()-_=+[]{}|;:'",<.>/?

I hope this information gives you any hint.
Anyway, insert a check for event.isARepeat, then it works fine as I expect.

interpretKeyEvents implementation you showed has a return path, so handleEvent might not been called in our case.

@gnachman
gnachman May 10, 2016 Owner

I think I see what's happening. Key repeat is funny in Mac OS these days. If you go to TextEdit and hold down j it does not repeat. If you hold down n it gives you a menu with ñ and ń to choose. So I guess it really is just key repeat that's special. It's nothing to do with handleEvent: vs interpretKeyEvents:, though. The key difference is whether we call [self.delegate keyDown:] in -[PTYTextView keyDown:] since insertText:replacementRange: does not get called when there is key repeat.

@mzp
mzp commented Apr 23, 2016

One idea: what about calling handleEvent: only when there is marked text? My guess is that it's only useful when there's an input method editor.

It's not enough. InputMethod can consume key event, even if there is no marked text.

@mzp
mzp commented Apr 23, 2016

Apple's documentation is not helpful

I agree...

As you quote, some document says interpretKeyEvents: method sends a doCommandBySelector:, or insertText.

But other document about input methods https://developer.apple.com/library/mac/documentation/Cocoa/Reference/IMKServerInput_Additions/index.html#//apple_ref/doc/uid/TP40006161 does not say about its restriction. So I understand(and implement) that input method can send doCommandBySelector:, insertText, or nothing with consuming event.

By this mismatch, this problem happens.

@mzp
mzp commented Apr 23, 2016

In my opinion, because PTYTextView is similar to NSTextView, using handleEvents: is reasonable choice.

@gnachman
Owner

I'm ok with pulling this behind a preference (for now; pref to be made on by default in 3.1) but please address the remaining line comments first.

@mzp
mzp commented Apr 29, 2016

Thanks! I replied each line comment.

@tai2
tai2 commented Apr 30, 2016 edited

@gnachman If you decided to adopt this patch, please wait a moment.
I'd like to remake this patch before it's merged, because this commit removed the original contributor's name. So I will cherry-pick the original commit and add my modification to it.

@gnachman gnachman and 2 others commented on an outdated diff May 3, 2016
sources/PTYTextView.m
return;
}
+ [self.delegate keyDown:_keydownEvent];
@gnachman
gnachman May 3, 2016 Owner

This is definitely wrong. We only get here when an OS bug is reproduced where events get sent to the wrong window.

@mzp
mzp May 7, 2016

Really? I reached here when press enter key.

Would you talk about line 4836(https://github.com/gnachman/iTerm2/pull/279/files#diff-9b0660c8ca6c9370c3a473d7c2b172b8R4836)?

@gnachman
gnachman May 10, 2016 Owner

You're right, I commented on the wrong line. You shouldn't call keyDown: inside the if statement, but after it is OK.

@mzp
mzp May 13, 2016

OK, I aggree with it.

@tai2 Please remove that line.

@tai2
tai2 May 13, 2016

@gnachman @mzp Done and tested for regression!

@tai2
tai2 May 13, 2016

Whoops! a test failed. checking...

@tai2
tai2 May 13, 2016 edited

I removed our commits and re-run tests, results are the same. doGoldenTestForInput fails whether our modifications are added or not.

@gnachman
Owner

There seem to be two things in this change:

  1. Call -handleEvent: instead of -interpretKeyEvents:
  2. Invoke -[self.delegate keyDown:] from -doCommandBySelector: instead of from -[PTYTextView keyDown:].

I'm not sure why either would have an effect, but maybe I've overlooked something.

How do I reproduce the problem? The original report says to switch to hiragana (which I think is the input method labeled "ひらかな") and press I to enter ASCII mode. That doesn't work for me in TextEdit. Pressing I gives me this:

screen shot 2016-05-10 at 9 26 31 am

@mzp
mzp commented May 13, 2016

How do I reproduce the problem? The original report says to switch to hiragana (which I think is the input method labeled "ひらかな") and press I to enter ASCII mode. That doesn't work for me in TextEdit. Pressing I gives me this:

We talk about small caps L, not large I.

@gnachman
Owner

Please take a look at my fixAquaSKK branch. I think I got the code cleaned up and sorted out some remaining issues (such as handling repeats in AquaSKK's ASCII keyboard), but it needs more testing so please see if you can find any bugs.

@tai2
tai2 commented May 14, 2016

I tried fixAquaSKK branch.
It repeats whole printable ASCII characters in U.S. keyboard, also it doesn't conflict with AquaSKK.

@tai2
tai2 commented May 14, 2016 edited

I'll use this branch for daily work. If it caused any problem, I'll report it here.

@gnachman
Owner

I've merged my fixAquaSKK branch into master. To enable the new behavior, turn on Prefs > Advanced > Improved support for input method editors like AquaSKK. I'll close this PR, but please do let me know if you find any issues.

@gnachman gnachman closed this May 15, 2016
@tai2 tai2 deleted the unknown repository branch May 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment