Closed Bug 757688 Opened 8 years ago Closed 7 years ago

Refactor KeyboardLayout

Categories

(Core :: Widget: Win32, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(9 files, 6 obsolete files)

8.41 KB, patch
jimm
: review+
Details | Diff | Splinter Review
53.52 KB, patch
jimm
: review+
Details | Diff | Splinter Review
29.68 KB, patch
jimm
: review+
Details | Diff | Splinter Review
11.21 KB, patch
jimm
: review+
Details | Diff | Splinter Review
33.22 KB, patch
jimm
: review+
Details | Diff | Splinter Review
13.41 KB, patch
jimm
: review+
Details | Diff | Splinter Review
8.11 KB, patch
Details | Diff | Splinter Review
22.20 KB, patch
jimm
: review+
Details | Diff | Splinter Review
17.13 KB, patch
jimm
: review+
Details | Diff | Splinter Review
I'm now working on bug 680830. When I'm thinking about some complex cases of keyup events, current KeyboardLayout isn't useful because it's stateful but only stores latest keydown. So, it cannot handle if multiple keys are pressed. I think that most function can be non-stateful.
First of all, KeyboardLayout::OnKeyDown() stores a lot of information which include what can be computed anytime. That makes some methods stateful. We should make them stateless as far as possible.

This patch removes mLast* from KeyboardLayout. They are used for IsDeadKey() only. It doesn't make sense.
Attachment #626728 - Attachment is obsolete: true
Attachment #626729 - Attachment is obsolete: true
Attachment #626730 - Attachment is obsolete: true
Attachment #631306 - Flags: review?(jmathies)
nsModifierKeyState is useful class for initializing modifier state and specifies the state to other methods. So, it's useful for KeyboardLayout methods. And it's really handling key states. I think that it should be defined in KeyboardLayout.h.
Attachment #631307 - Flags: review?(jmathies)
This removes ShiftState from nsWindow. It is defined in KeyboardLayout.h and used by arguments of KeyboardLayout methods. Instead, we can use ModifierKeyState class or Modifiers flags. This makes other callers simpler.
Attachment #631309 - Flags: review?(jmathies)
KeyboardLayout::OnKeyDown() should take ModifierKeyState for its argument. Then, it doesn't need to get actual modifier key state from ::GetKeyboardState() and we can get rid of KeyboardLayout::GetShiftState().

Additionally, moves and renames KeyboardLayout::SetShiftState() to VirtualKey::FillKbdState() because ShiftState is now owned and used by VirtualKey class only.
Attachment #631312 - Flags: review?(jmathies)
If KeyboardLayout::OnKeyDown() returns the characters which are inputted by the key event, KeyboardLayout::GetUniChars() won't necessary. And also KeyboardLayout::mChars, mModifiersOfChars and mNumOfChars.

Then, we can rename GetUniCharsWithShiftState() to GetUniCharsAndModifiers().

However, there is a problem. It's too ugly to use PRUnichar[5], Modifiers[5] and PRUint8 for the result.

For solving this problem, this patch makes UniCharsAndModifiers struct. This makes nsWindow::OnKeyDown() simpler.
Attachment #631313 - Flags: review?(jmathies)
This registers the NumPad keys are printable keys. This makes nsWindow simpler. I'll post -w version of this patch.
Attachment #631315 - Flags: review?(jmathies)
This bug and bug 680830 change a lot of code in key event handling. And I'm not a Western language user. So, I may take some mistakes for dead key handling.

This patch allows to test dead key handling code in KeyboardLayout::OnKeyDown().

KeyboardLayout::LoadLayout() resets dead key state if different keyboard layout is loaded. This patch allows delayed loading when restores original keyboard layout. I.e., tests can perform testing for two or more key inputs.

And also I found a serious bug of the API. ::UnloadKeyboardLayout() unloads the loaded keyboard layout from the system rather than the caller's thread or process. So, it unloads keyboard layouts which are already added by the users. This patch also checks whether the keyboard layout has been loaded on the system already.
Attachment #631320 - Flags: review?(jmathies)
If neither Ctrl key nor Alt key is pressed, the result characters of KeyboardLayout::OnKeyDown() is never used for generating keypress event.

That means that most tests in test_keycodes.xul cannot test the result of KeyboardLayout::OnKeyDown(). That really wastes.

This patch adds debug code in handler of fake char message. If the result and fake char message which is specified by tests mismatches, it aborts. So, we can see red in tinderbox.

By this change, I can see some bugs in test_keycodes.xul.

1. The test suite doesn't handle alrGr:1 case.
  1. It doesn't send Ctrl key and Alt key flags to sendNativeKeyEvent().
  2. It doesn't assume that two or more modifier keys are pressed same time. therefore, checking keydown/keyup events' ctrlKey, metaKey, altKey and shiftKey doesn't work. They are checked in isStateChangingModifierKeyEvent() by this patch. And current checking code will run only when the event is keypress event.
  3. It doesn't assume that some modifier keys are consumed by widget when the sent key event causes text input. This adds SHOULD_NOT_CAUSE_INPUT flag for it.

2. Shift + NS_NUMPAD[0-9] don't generate any characters and the key combination generates NumLock unlocked key codes. So, it doesn't make sense to test them.

3. Testing French keyboard's </> key, the results are wrong.
Attachment #631324 - Flags: review?(jmathies)
I think that we can make nsWindow::OnKeyDown() simpler too. However, it should be done later. It doesn't block bug 680830 and is not so important.
Comment on attachment 631306 [details] [diff] [review]
part.1 Make KeyboardLayout::IsDeadKey() stateless

Review of attachment 631306 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsWindow.cpp
@@ +6296,5 @@
>      case NS_VK_WIN:
>        return noDefault;
>    }
>  
> +  PRUint8 currentShiftState = KeyboardLayout::GetShiftState(aModKeyState);

You don't appear to be using this call in part1 but I'm assuming I'll come across this in subsequent patches.
Attachment #631306 - Flags: review?(jmathies) → review+
Comment on attachment 631307 [details] [diff] [review]
part.2 Move nsModifierKeyState to KeyboardLayout and redesign it

Review of attachment 631307 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/KeyboardLayout.cpp
@@ +123,5 @@
> +    case NS_SIMPLE_GESTURE_EVENT:
> +    case NS_MOZTOUCH_EVENT:
> +      break;
> +    default:
> +      return;

Let's assert on this so we get a warning when someone calls InitMouseEvent with an unsupported event.
Attachment #631307 - Flags: review?(jmathies) → review+
Attachment #631309 - Flags: review?(jmathies) → review+
Comment on attachment 631312 [details] [diff] [review]
part.4 Remove GetShiftState() and move SetShiftState() to VirtualKey

Review of attachment 631312 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/KeyboardLayout.cpp
@@ +277,5 @@
>  
> +// static
> +void
> +VirtualKey::FillKbdState(PBYTE aKbdState,
> +                         ShiftState aShiftState)

make ShiftState const?
Attachment #631312 - Flags: review?(jmathies) → review+
Comment on attachment 631313 [details] [diff] [review]
part.5 Make KeyboardLayout stateless for non-dead keys

Review of attachment 631313 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/KeyboardLayout.h
@@ +118,5 @@
> +{
> +  // Dead-key + up to 4 characters
> +  PRUnichar chars[5];
> +  Modifiers modifiers[5];
> +  PRUint32  length;

I'm not sure what our convention is here, but I think I'd prefer we use normal member naming. That way there's no confusion in the cpp code which references them.
Attachment #631313 - Flags: review?(jmathies) → review+
Attachment #631315 - Flags: review?(jmathies) → review+
Comment on attachment 631320 [details] [diff] [review]
part.7 Make nsWindow for Windows possible to test dead keys

Review of attachment 631320 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/KeyboardLayout.h
@@ +364,5 @@
> +  /**
> +   * LoadLayout() loads the keyboard layout.  If aLoadLater is true,
> +   * it will be done when OnKeyDown() is called.
> +   */
> +  void LoadLayout(HKL aLayout, bool aLoadLater = false);

Curious, why would we want to delay the load until a key down?
Comment on attachment 631324 [details] [diff] [review]
part.8 Make sure test_keycodes.xul emulates correct key events

Review of attachment 631324 [details] [diff] [review]:
-----------------------------------------------------------------

With all these test changes please make sure a push to try and post results here.
Attachment #631324 - Flags: review?(jmathies) → review+
(In reply to Jim Mathies [:jimm] from comment #17)
> Comment on attachment 631306 [details] [diff] [review]
> part.1 Make KeyboardLayout::IsDeadKey() stateless
> 
> Review of attachment 631306 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/nsWindow.cpp
> @@ +6296,5 @@
> >      case NS_VK_WIN:
> >        return noDefault;
> >    }
> >  
> > +  PRUint8 currentShiftState = KeyboardLayout::GetShiftState(aModKeyState);
> 
> You don't appear to be using this call in part1 but I'm assuming I'll come
> across this in subsequent patches.

It's used in next line :-)
(In reply to Jim Mathies [:jimm] from comment #20)
> Comment on attachment 631313 [details] [diff] [review]
> part.5 Make KeyboardLayout stateless for non-dead keys
> 
> Review of attachment 631313 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/KeyboardLayout.h
> @@ +118,5 @@
> > +{
> > +  // Dead-key + up to 4 characters
> > +  PRUnichar chars[5];
> > +  Modifiers modifiers[5];
> > +  PRUint32  length;
> 
> I'm not sure what our convention is here, but I think I'd prefer we use
> normal member naming. That way there's no confusion in the cpp code which
> references them.

Some public members of struct/class are not prefixed by "m". E.g., most events in nsGUIEvent.h. However, the others have "m" prefix. So, I think that either is okay. I'll add "m" for them.
(In reply to Jim Mathies [:jimm] from comment #21)
> Comment on attachment 631320 [details] [diff] [review]
> part.7 Make nsWindow for Windows possible to test dead keys
> 
> Review of attachment 631320 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/KeyboardLayout.h
> @@ +364,5 @@
> > +  /**
> > +   * LoadLayout() loads the keyboard layout.  If aLoadLater is true,
> > +   * it will be done when OnKeyDown() is called.
> > +   */
> > +  void LoadLayout(HKL aLayout, bool aLoadLater = false);
> 
> Curious, why would we want to delay the load until a key down?

During the tests, LoadLayout is called twice a SynthesizeNativeKey() call. One is for changing testing keyboard layout. The other is for restoring original keyboard layout.

When we test dead keys, the restore breaks the dead key state. It's the reason why we cannot test dead keys.

By this change, the restoring is delayed to next keydown. Therefore, it's never restored actually during tests. So, the dead key state isn't broken during testing same keyboard layout.
Attachment #631320 - Flags: review?(jmathies) → review+
Finally, the tests passed! Windows machines are too slow :-(
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=7abf9311d4d5
You need to log in before you can comment on or make changes to this bug.