Last Comment Bug 775414 - InitKeyEvent() should decide input string instead of InitKeyPressEvent()
: InitKeyEvent() should decide input string instead of InitKeyPressEvent()
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: Trunk
: All Mac OS X
: -- normal (vote)
: mozilla17
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
: Markus Stange [:mstange]
Mentors:
Depends on: 784783 821329
Blocks: 680830 758420
  Show dependency treegraph
 
Reported: 2012-07-18 23:08 PDT by Masayuki Nakano [:masayuki] (Mozilla Japan)
Modified: 2013-01-09 22:15 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (19.56 KB, patch)
2012-07-18 23:08 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
smichaud: review+
Details | Diff | Splinter Review

Description Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-07-18 23:08:50 PDT
Created attachment 643731 [details] [diff] [review]
Patch

In bug 758420, we used a hecky way, that is, setting charCode before calling InitKeyEvent() if the caller (InsertText()) wants to override NSEvent's characters value with specified value.

When I fix bug 680830, the inputting characters are also set to char attribute of D3E even if the key event is NS_KEY_DOWN or NS_KEY_UP.

Therefore, it makes the patch simpler if I move the code deciding inputting character from InitKeyPressEvent() to InitKeyEvent().
Comment 1 Steven Michaud [:smichaud] (Retired) 2012-07-20 09:27:49 PDT
Comment on attachment 643731 [details] [diff] [review]
Patch

This basically looks fine to me.

But it does change the logic in the following case:

if (aKeyEvent.IsMeta() && !(aKeyEvent.IsControl() || aKeyEvent.IsAlt()))

The changes look reasonable, but I can't tell whether they're correct or not.
Comment 2 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-07-20 09:48:12 PDT
(In reply to Steven Michaud from comment #1)
> But it does change the logic in the following case:
> 
> if (aKeyEvent.IsMeta() && !(aKeyEvent.IsControl() || aKeyEvent.IsAlt()))
> 
> The changes look reasonable, but I can't tell whether they're correct or not.

Oh, really? I didn't expect to change anything. I just ported some parts from InitKeyPressEvent(). Would you explain what's changed?
Comment 3 Steven Michaud [:smichaud] (Retired) 2012-07-20 10:08:57 PDT
This code in TISInputSourceWrapper::InitKeyPressEvent():

-  if (computeCharCode &&
-      aKeyEvent.IsMeta() && !(aKeyEvent.IsControl() || aKeyEvent.IsAlt())) {
-    // The character to use for charCode.
-    PRUint32 preferredCharCode = 0;
-    preferredCharCode = aKeyEvent.IsShift() ? cmdedShiftChar : cmdedChar;
-
-    if (preferredCharCode) {
-      aKeyEvent.charCode = preferredCharCode;
-      PR_LOG(gLog, PR_LOG_ALWAYS,
-        ("%p TISInputSourceWrapper::InitKeyPressEvent, "
-         "aKeyEvent.charCode=U+%X",
-         this, aKeyEvent.charCode));
-    }
-  }

was replaced by this code in TISInputSourceWrapper::InitKeyEvent():

+    else if (aKeyEvent.IsMeta() &&
+             !(aKeyEvent.IsControl() || aKeyEvent.IsAlt())) {
+      UInt32 numLockState =
+        aKeyEvent.IsNumLocked() ? kEventKeyModifierNumLockMask : 0;
+      UInt32 capsLockState = aKeyEvent.IsCapsLocked() ? alphaLock : 0;
+      UInt32 shiftState = aKeyEvent.IsShift() ? shiftKey : 0;
+      PRUint32 uncmdedChar =
+        TranslateToChar(nativeKeyCode, numLockState, kbType);
+      PRUint32 cmdedChar =
+        TranslateToChar(nativeKeyCode, cmdKey | numLockState, kbType);
+      // If we can make a good guess at the characters that the user would
+      // expect this key combination to produce (with and without Shift) then
+      // use those characters.  This also corrects for CapsLock.
+      PRUint32 ch = 0;
+      if (uncmdedChar == cmdedChar) {
+        // The characters produced with Command seem similar to those without
+        // Command.
+        ch = TranslateToChar(nativeKeyCode,
+                             shiftState | capsLockState | numLockState, kbType);
+      } else {
+        TISInputSourceWrapper USLayout("com.apple.keylayout.US");
+        PRUint32 uncmdedUSChar =
+          USLayout.TranslateToChar(nativeKeyCode, numLockState, kbType);
+        // If it looks like characters from US keyboard layout when Command key
+        // is pressed, we should compute a character in the layout.
+        if (uncmdedUSChar == cmdedChar) {
+          ch = USLayout.TranslateToChar(nativeKeyCode,
+                          shiftState | capsLockState | numLockState, kbType);
+        }
+      }
+

They look different to me :-)

But maybe I just don't fully understand the context of your changes.
Comment 4 Steven Michaud [:smichaud] (Retired) 2012-07-20 11:29:52 PDT
I looked again and found this block of code in TISInputSourceWrapper::InitKeyPressEvent():

http://hg.mozilla.org/mozilla-central/annotate/eea94a9b40a1/widget/cocoa/TextInputHandler.mm#l920

I'm no longer sure the logic has actually changed.  But neither can I say it *hasn't* changed.
Comment 5 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-07-20 16:02:37 PDT
Ah, I see. I ported the each variable's initialization too. Ideally, these parts should be shared by making another method. However, I couldn't find reasonable way for that. But I think that it's very risky to change the key hell part, so, I believe we will never touch the part again. Therefore, I just picked up the part for InitKeyEvent().
Comment 6 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-07-20 16:27:09 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/440ac3414c64

Thank you.
Comment 7 Ryan VanderMeulen [:RyanVM] 2012-07-20 21:02:24 PDT
https://hg.mozilla.org/mozilla-central/rev/440ac3414c64

Note You need to log in before you can comment on or make changes to this bug.