339221 - Simplify nsMacEventHandler::InitializeKeyEvent()

Status: RESOLVED INCOMPLETE

(Core Graveyard :: Widget: Mac, defect)

Description

[Katsuhiro MIHARA]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; ja-jp) AppleWebKit/418 (KHTML, like Gecko) Safari/417.9.3
Build Identifier: 

As mentioned in Bug 337199 #47 and #48,
we can simplify nsMacEventHandler::InitializeKeyEvent() now.

1. remove var aMessage and aConvertChar
   All callers set aMessage = NS_KEY_PRESS and aConvertChar = PR_FALSE
2. add var aUnicodeChar to set aKeyEvent.charCode
3. Copy comment from Bug 337199 #48
   about trickey situations.
4. tidying-up.
5. remove a method nsMacEventHandler::ConvertKeyEventToUnicode()
   No methods call this now.


Reproducible: Always

Actual Results:  
nsMacEventHandler::InitializeKeyEvent() is complex.


Expected Results:  
We should simplify.

Comment 1

[Katsuhiro MIHARA]

Attachment: implements points described above

Comment 2

[Phil Ringnalda (:philor)]

Comment on attachment 223406 [details] [diff] [review]
implements points described above

Note to whichever Bugzilla programmer decided that moving products and confirming a bug should clear review requests: you were wrong!

Comment 3

[Katsuhiro MIHARA]

(In reply to comment #2)
Phil, Sorry for going wrong way.

I requested Mark Mentovai to review and set his mail address and looked setting just before, but made you requestee. I also get confused.

Comment 4

[Katsuhiro MIHARA]

(In reply to comment #3)
> looked setting just before

This is set by you.

I might failed copying mail address and doesn't check.

Comment 5

[Katsuhiro MIHARA]

(In reply to comment #4)

I notice that I understood nothing. I will read and observe rules.

Comment 6

[Mark Mentovai]

Comment on attachment 223406 [details] [diff] [review]
implements points described above

First, some stylistic comments:

We hate tabs, and it's good practice when overhaulilng a method like this to detab the entire thing (and bring it up to other current stylistic standards.)  Specifically, two spaces for each level, and no tab characters anywhere, and no space characters at all on blank lines.  Also, when you have a multi-statement block, put the opening curly brace on the same line as the if/else/while, as in:

  if (!IsSpecialRaptorKey(keyCode)) {

If you're only conditionalizing a single statement and it doesn't affect readability, feel free to omit the { braces }.  Sometimes, that improves readability.

Now, to the actual code itself.

> #include <ToolUtils.h>
> #include <TextServices.h>
>-#include <UnicodeConverter.h>

It's current practice to get rid of all of the individual headers and just #include <Carbon/Carbon.h>.  You can make that change here.

>-// InitializeKeyEvent
>+// InitializeKeyPressEvent

Good, I was hoping you'd do that!

>+	aKeyEvent.time       = PR_IntervalNow();

Don't leave all that space there.  There should only be one space on either side of the =, unless you're trying to line things up.

>+	aKeyEvent.isShift    = ((aOSEvent.modifiers & shiftKey)   != 0);
>+	aKeyEvent.isControl  = ((aOSEvent.modifiers & controlKey) != 0);
>+	aKeyEvent.isAlt      = ((aOSEvent.modifiers & optionKey)  != 0);
>+	aKeyEvent.isMeta     = ((aOSEvent.modifiers & cmdKey)     != 0);

And when you do line things up like you do here, only put one space before the = at the longest line - so you should remove one space before each = above.

>+    UInt32 keyCode       = (aOSEvent.message & keyCodeMask) >> 8;

This is distinct from the assignments above and shouldn't be lined up with them.

On to the meat...

> 	//
> 	// nsKeyEvent parts
> 	//
>+    if (!IsSpecialRaptorKey(keyCode))
>+    {
>+      if ((!aKeyEvent.isControl) && (!aKeyEvent.isMeta))
>       {
>+        /*

Inline comments in C++ should be // C++-style.

>+         * see Bug 337199 #48

And a nit: should be "See bug 337199 comment 48"

>+         * and control up, it's supposed to generate a character
>+         * (usually a symbol).
>+         *  Leaving the alt flag on in the NS_KEY_PRESS event will prevent

And another: no leading space there.  Separate paragraphs with a // blank line if you want.

>+        aKeyEvent.isAlt = 0;
>       }
>+      aKeyEvent.keyCode = 0;
>+      if (aUnicodeChar!=0)
>       {

Spaces around the != operator, and this is a case where you'd do well to just get rid of the { braces } altogether.  But...

>+        aKeyEvent.charCode = aUnicodeChar;

You should just get rid of the conditional altogether.  It doesn't hurt to reassign the same value, and it makes the method a bit more general-purpose in case anyone decides to reuse an already-initialized nsKeyPressEvent.

>       }
>+    } // if (!IsSpecialRaptorKey(keyCode))

With the simplification going on here and soon the reindentation too, it's probably no longer necessary to mark the ends of conditionals with comments like this.

>+    else
>+    {
>+      aKeyEvent.keyCode = ConvertMacToRaptorKeyCode(aOSEvent.message & charCodeMask, keyCode, aOSEvent.modifiers);

Maximum line length is 80 characters, so wrap this somehow.  It might prove difficult to wrap cleanly, but there's nothing wrong with introducing |char charCode| if it helps.

>+      aKeyEvent.charCode = 0;
>+    } // else for  if (!IsSpecialRaptorKey(keyCode))
>   
>+    //
>+    // obscure cursor if appropriate
>+    //
>+    if (  !aKeyEvent.isMeta 
>+        && aKeyEvent.keyCode != nsIDOMKeyEvent::DOM_VK_PAGE_UP 
>+        && aKeyEvent.keyCode != nsIDOMKeyEvent::DOM_VK_PAGE_DOWN

This is a pet peeve of mine.  I don't know why we leave the cursor visible for the pageup and pagedown keys.  We can get rid of those two cases for sure.

As for the meta key, we should leave the cursor visible when it's down, but note that it's not exactly right.  We don't want to hide the cursor when the key combination is attached to some menu command, but we DO want to hide it when it's used for purposes like moving the insertion point, and I'd say, even for navigating history back and forward.  So for this, even though it's a little hackish, I wouldn't mind writing the condition to test for the arrow keys.  I also wouldn't mind this:

  if (!aKeyEvent.isMeta || (keyCode >= 96 && keyCode < 127))

which allows the cursor to be obscured for all keystrokes that include the arrow keys, navigation keys (Home, End, PgUp, PgDn), F-keys (including PrtScr, ScrLk, and Pause/Break), Ins/Help, and Del.  Based on tooling around in TextEdit and a few other apps, I found that the cursor DOES hide when any of those keys are pressed with command down.  If you choose to do this, be sure to include a comment saying which keys are in the affected range.

(You can also substitute kF5KeyCode and kUpArrowKeyCode for the numbers above, but because the range isn't obvious, you'd still need the comment.)

>+        // also consider:  function keys and sole modifier keys
>+	   ) 
>+      ::ObscureCursor();
>+    }

In HandleUKeyEvent:

>   // simulate key press events
>   if (!IsSpecialRaptorKey((aOSEvent.message & keyCodeMask) >> 8))
>   {
>     // it is a message with text, send all the unicode characters
>     PRInt32 i;
>     for (i = 0; i < charCount; i++)
>     {
>       nsKeyEvent keyPressEvent(PR_TRUE, NS_KEY_PRESS, nsnull);
>-      InitializeKeyEvent(keyPressEvent, aOSEvent, focusedWidget, NS_KEY_PRESS, PR_FALSE);
>-      keyPressEvent.charCode = text[i];
>+      InitializeKeyPressEvent(keyPressEvent, aOSEvent, focusedWidget, text[i]);
> 
>       // If keydown default was prevented, do same for keypress
>       if (mKeyHandled)
>         keyPressEvent.flags |= NS_EVENT_FLAG_NO_DEFAULT;
> 
>       // control key is special in that it doesn't give us letters
>       // it generates a charcode of 0x01 for control-a
>       // so we offset to do the right thing for gecko
>-      // this doesn't happen for us in InitializeKeyEvent because we pass
>-      // PR_FALSE so no character translation occurs.
>       // I'm guessing we don't want to do the translation there because
>       // translation already occurred for the string passed to this method.
>       if (keyPressEvent.isControl && keyPressEvent.charCode <= 26)       
>       {
>         if (keyPressEvent.isShift)
>           keyPressEvent.charCode += 'A' - 1;
>         else
>           keyPressEvent.charCode += 'a' - 1;

Why don't you move this logic into InitializeKeyPressEvent, as well as the logic that follows responsible for unshifting characters?  And when you get to this:

  keyPressEvent.charCode -= 32;

it would be better written as:

  keyPressEvent.charCode -= 'a' - 'A';

> #if 1

You can get rid of the #if 1.

>-		virtual void InitializeKeyEvent(nsKeyEvent& aKeyEvent, EventRecord& aOSEvent, 
>-                              nsWindow* aFocusedWidget, PRUint32 aMessage, 
>-                              PRBool aConvertChar=PR_TRUE);
>+		virtual void InitializeKeyPressEvent(nsKeyEvent& aKeyEvent, EventRecord& aOSEvent, 
>+                              nsWindow* aFocusedWidget, PRUnichar aUnicodeChar=0);

Good call here too, bringing aUnicodeChar in as an argument.

And the last word: you should also clean up IsSpecialRaptorKey.  There's no nead to do this:

  case kEscapeKeyCode:  isSpecial = PR_TRUE; break;

so many times repeatedly.  Everything that sets isSpecial to PR_TRUE should be combined into a single multi-label case.  The default case should be dropped, instead initializing isSpecial to PR_FALSE.

Comment 7

[Katsuhiro MIHARA]

Attachment: patch v2

> >+        aKeyEvent.charCode = aUnicodeChar;
> 
> You should just get rid of the conditional altogether.
> It doesn't hurt to reassign the same value,
> and it makes the method a bit more general-purpose
> in case anyone decides to reuse an already-initialized nsKeyPressEvent.

I got rid of the conditional. I didn't read nsKeyEvent::nsKeyEvent() assinged charCode = 0.


>   if (!aKeyEvent.isMeta || (keyCode >= 96 && keyCode < 127))
> 
> which allows the cursor to be obscured for all keystrokes that include the arrow keys,
> navigation keys (Home, End, PgUp, PgDn), F-keys (including PrtScr, ScrLk, and Pause/Break),
> Ins/Help, and Del.

I changed the conditional to this and wrote a keycode list in comment.


> In HandleUKeyEvent:
> 
> >       if (keyPressEvent.isControl && keyPressEvent.charCode <= 26)       
> >       {
> >         if (keyPressEvent.isShift)
> >           keyPressEvent.charCode += 'A' - 1;
> >         else
> >           keyPressEvent.charCode += 'a' - 1;
> 
> Why don't you move this logic into InitializeKeyPressEvent,
> as well as the logic that follows responsible for unshifting characters?

I moved this logic into InitializeKeyPressEvent.

When a user presses control-key and a character key, Mac OS X tells not Unicode value of character but index from A by Apple event kUnicodeNotFromInputMethod even if shift-key, option-key and command-key are down or not.
A -> 1, B -> 2, ... Z -> 26
a -> 1, b -> 2, ... z -> 26
I wrote this reason to comment.


> And when you get to this:
> 
>   keyPressEvent.charCode -= 32;
> 
> it would be better written as:
> 
>   keyPressEvent.charCode -= 'a' - 'A';

I changed assignment.

When a user press command-shift-?, Mac OS X tells a lowercase letter by Apple event kUnicodeNotFromInputMethod. I wrote this reason to comment.

> And the last word: you should also clean up IsSpecialRaptorKey.

I tidied IsSpecialRaptorKey.

Comment 8

[Mark Mentovai]

Comment on attachment 223592 [details] [diff] [review]
patch v2

This is substantially r+.  Let's just see one more with some comments fixed up and a few behaviors clarified.

>+  UInt32 keyCode = (aOSEvent.message & keyCodeMask) >> 8;

Blank line goes here.

>+  //
>+  // nsKeyEvent parts
>+  //

>+    // When a user presses control-key and a character key,
>+    // Mac OS X tells not Unicode value of character but index from A
>+    // by Apple event kUnicodeNotFromInputMethod
>+    // even if shift-key, option-key and command-key are down or not.

Simplify: "When a user types a control combination that can be represented as a control code in ASCII's 0..31 range, aUnicodeChar will contain the control code instead of the character pressed."

>+    // A -> 1, B -> 2, ... Z -> 26
>+    // a -> 1, b -> 2, ... z -> 26
>+    // 
>+    // To tell key pressed to Gecko,
>+    // assign Unicode value of a character.
>+    if (aKeyEvent.isControl && aUnicodeChar <= 26)

This is annoying.  We also get low control codes in aUnicodeChar for other values < 32: control-[, \, ], and _.  Control-shift-^ and control-6 always gives aUnicodeChar = '6'  Control-_, unshifted, always gives aUnicodeChar='_', even though '_' is ordinarily shifted.  I'm sure that much of this is dependent on keyboard and keyboard layout.

I'm OK using 26 as the upper bound here (without further investigation into how other layouts handle this case), but you should at least comment that unexpected things might happen in the 27..31 range.

>+    // When a user press command-shift-?,

Say command-shift-letter, otherwise it looks like you mean the key with the '?' on it.

>+    // Mac OS X tells a lowercase letter
>+    // by Apple event kUnicodeNotFromInputMethod.
>+    // 
>+    // To tell key pressed to Gecko,
>+    // assign Unicode value of a character.

Instead of these lines, say "aUnicodeChar will contain a lowercase letter.  Convert it to the uppercase variant if necessary."

Note also here that this applies for any command-shift combination, not just letters.  aUnicodeChar will ALWAYS contain the code associated with the unshifted variant.  Again, it's OK to do ths only for the 'a'..'z' range because doing the proper unshifted-to-shifted conversion outside of the ASCII alphabet is difficult - just comment the omission.

>+  // kScrollLockKeyCode  (=kF14KeyCode)
>+  // kPauseKeyCode       (=kF15KeyCode,

Oops, where did that comman come from at the end of the line?

>+	// 

Oops, where did that tab come from?

>+  // kHelpKeyCode       (0x72) also insert key
>+  // kDeleteKeyCode     (0x75) also forward delete key
>+  // kHomeKeyCode       (0x73)
>+  // kEndKeyCode        (0x77)
>+  // kPageUpKeyCode     (0x74)
>+  // kPageDownKeyCode   (0x79)
>+  // kLeftArrowKeyCode  (0x7B)
>+  // kRightArrowKeyCode (0x7C)
>+  // kUpArrowKeyCode    (0x7E)
>+  // kDownArrowKeyCode  (0x7D)
>+  
>+  {

Why is this still in its own {block}?

>+    PRUint32 keyCode = aKeyEvent.keyCode;
>+    if (!aKeyEvent.isMeta || (keyCode >= 0x60 && keyCode < 0x7F))
>+      ::ObscureCursor();
>+  }

>+    default:
>+      // assigned PR_FALSE at declaration
>+      break;

If your default case is just going to break, you don't need a default case at all.

Comment 9

[Katsuhiro MIHARA]

Attachment: patch v3

fixed up some comments.

Comment 10

[Mark Mentovai]

Comment on attachment 224350 [details] [diff] [review]
patch v3

Nice, looks good!

Comment 11

[Mark Mentovai]

Waiting on superreview.