790775 - Sony Ericsson Xperia Play's "circle" and "cross" D-pad buttons generate unexpected keycodes

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Keyboards and IME, defect, P4)

Description

[Chris Peterson [:cpeterson]]

The Sony Ericsson Xperia Play's D-pad button layout (unsurprisingly) matches the PlayStation game controller.

AFAIK, the expected mapping from PlayStation to Xbox controller buttons is:

  PS Cross    -> Xbox A
  PS Circle   -> Xbox B
  PS Square   -> Xbox X
  PS Triangle -> Xbox Y

The actual keycodes generated by the Xperia Play buttons is:

  Cross    -> Expected KEYCODE_BUTTON_A; got KEYCODE_DPAD_CENTER (keycode 23)
  Circle   -> Expected KEYCODE_BUTTON_B; got META_ALT_ON + KEYCODE_BACK (keycode 4)
  Square   -> Expected and got KEYCODE_BUTTON_X (keycode 99)
  Triangle -> Expected and got KEYCODE_BUTTON_Y (keycode 100)

Comment 1

[Chris Peterson [:cpeterson]]

Chris, you may be interested in this bug. :)

Comment 2

[Chris Lord [:cwiiis]]

If we think of X as 'select' and O as 'back' (which it is in Western games - interestingly, O is 'select' and I think triangle or X is back in Japan...), then the mappings make some sense... Still a bit annoying, but we should take it into account as many bluetooth gamepads emulate the Xperia Play key codes.

Comment 3

[Chris Peterson [:cpeterson]]

If the Gamepad API defines gamepad button events, we should ensure that we map these Java keycodes to the correct gamepad codes.

https://wiki.mozilla.org/GamepadAPI#Gamepad_Button_Events

Comment 4

[Chris Peterson [:cpeterson]]

This is a good first bug.

Comment 5

[Shane Tully (:stully:]

Attachment: Remap the circle and cross buttons to KEYCODE_BUTTON_A or KEYCODE_BUTTON_B depending on region

The attached patch remaps the cross key to KEYCODE_BUTTON_A and the circle key to KEYCODE_BUTTON_B or vice verse if the keys are swapped (for devices in certain regions)

Comment 6

[Chris Peterson [:cpeterson]]

Comment on attachment 752466 [details] [diff] [review]
Remap the circle and cross buttons to KEYCODE_BUTTON_A or KEYCODE_BUTTON_B depending on region

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

Looking good. Here are some suggestions:

::: mobile/android/base/GeckoInputConnection.java
@@ +24,5 @@
>  import android.util.Log;
>  import android.view.KeyEvent;
>  import android.view.View;
> +import android.view.InputDevice;
> +import android.view.KeyCharacterMap;

Please alphabetize these new android.view.* imports.

@@ +759,5 @@
>          }
>          return false;
>      }
>  
> +    private KeyEvent translateSonyXperiaGamepad(int keyCode, KeyEvent event) {

I would rename this method to something like `translateSonyXperiaGamepadKey()`.

@@ +762,5 @@
>  
> +    private KeyEvent translateSonyXperiaGamepad(int keyCode, KeyEvent event) {
> +        // The cross and circle button mappings may be swapped in the different
> +        // regions so determine if they are swapped so the proper key codes
> +        // can be mapped to the keys

Mozilla's C++ coding style requires 80 column lines, but we allow Java files to use 100 columns (because Java code is so verbose). So you can use wider comments, if you like.

@@ +764,5 @@
> +        // The cross and circle button mappings may be swapped in the different
> +        // regions so determine if they are swapped so the proper key codes
> +        // can be mapped to the keys
> +        boolean areKeysSwapped = areSonyXperiaGamepadKeysSwapped();
> +        

Please remove trailing whitespace.

@@ +768,5 @@
> +        
> +        // If a Sony Xperia, remap the cross and circle buttons to buttons
> +        // A and B for the gamepad API
> +        int newKeyCode = keyCode;
> +        switch(keyCode) {

Please insert a space between before the first paren.

@@ +778,5 @@
> +                newKeyCode = (areKeysSwapped ? KeyEvent.KEYCODE_BUTTON_B : KeyEvent.KEYCODE_BUTTON_A);
> +                break;
> +        }
> +
> +        return new KeyEvent(event.getAction(), newKeyCode);

Instead of allocating a new KeyEvent every time, let's add a default case to the switch statement that just returns the original `event` parameter. You will probably be able to skip the initialization of `newKeyCode = keyCode` (because the compiler should be able tell that newKeyCode will always be initialized in every code path).

@@ +790,5 @@
> +
> +        boolean swapped = false;
> +        int[] ids = InputDevice.getDeviceIds();
> +
> +        for (int i= 0; ids != null && i<ids.length; i++) {

I know this code was written by someone else, but please add spaces before and after the '<' operator.

@@ +815,5 @@
>      }
>  
>      private boolean processKey(int keyCode, KeyEvent event, boolean down) {
> +        // If the key event came from a Sony Xperia keypad, remap the keys
> +        // if necessary

We can probably remove this comment because the translateSonyXperiaGamePad() method name is pretty self-explanatory. :)

@@ +816,5 @@
>  
>      private boolean processKey(int keyCode, KeyEvent event, boolean down) {
> +        // If the key event came from a Sony Xperia keypad, remap the keys
> +        // if necessary
> +        if (event.getDeviceId() == SONY_XPERIA_GAMEPAD_DEVICE_ID) {

I'm worried that some other (Sony) device might return the same device ID. I think we should double-check the android.os.Build.MANUFACTURER and MODEL, too. Check device ID first because it is cheap integer comparison.

Now that the Xperia device check is getting more complicated, I think we should extract that logic into a static helper method. Something like `private static boolean isSonyXperiaKeyEvent(KeyEvent event)`.

Comment 7

[Shane Tully (:stully:]

Attachment: 02: Remap the circle and cross buttons to KEYCODE_BUTTON_A or KEYCODE_BUTTON_B depending on region

Revised patch with changes requested by :cpeterson

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Drive-by comment: If some of these new functions are meant to be reusable it might be better to put them in the util/GamepadUtils.java file. I'm not sure if that's more appropriate or not.

Comment 9

[Chris Peterson [:cpeterson]]

Comment on attachment 752849 [details] [diff] [review]
02: Remap the circle and cross buttons to KEYCODE_BUTTON_A or KEYCODE_BUTTON_B depending on region

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

kats makes a good point. Even though these new methods are specific to the Sony Xperia devices, I think we should move them to util/GamepadUtils.java. We can just expose isSonyXperiaGamepadKeyEvent() and translateSonyXperiaGamepadKeys() from GamepadUtils.java and encapsulate all the implementation details in private methods.

Plus, GeckoInputConnection.java is already a huge file, so moving code to other files is a good idea in general. :)

::: mobile/android/base/GeckoInputConnection.java
@@ +769,5 @@
> +        // A and B for the gamepad API
> +        switch (keyCode) {
> +            case KeyEvent.KEYCODE_BACK:
> +                keyCode = (areKeysSwapped ? KeyEvent.KEYCODE_BUTTON_A : 
> +                                            KeyEvent.KEYCODE_BUTTON_B);

Please remove the trailing whitespace at the end of the line and move the colon to the next line, indented to line up with the question mark. Mozilla coding style is to put && and || at the end of a continued line, but put other operators like + or - at the beginning of the continuing line.

@@ +774,5 @@
> +                break;
> +
> +            case KeyEvent.KEYCODE_DPAD_CENTER:
> +                keyCode = (areKeysSwapped ? KeyEvent.KEYCODE_BUTTON_B : 
> +                                            KeyEvent.KEYCODE_BUTTON_A);

Same as above.

@@ +805,5 @@
> +        return swapped;
> +    }
> +
> +    private static boolean isSonyXperiaGamepadKeyEvent(KeyEvent event) {
> +        return (event.getDeviceId() == SONY_XPERIA_GAMEPAD_DEVICE_ID && 

Please remove trailing whitespace.

@@ +807,5 @@
> +
> +    private static boolean isSonyXperiaGamepadKeyEvent(KeyEvent event) {
> +        return (event.getDeviceId() == SONY_XPERIA_GAMEPAD_DEVICE_ID && 
> +                android.os.Build.MANUFACTURER.equals("Sony Ericsson") &&
> +                (android.os.Build.MODEL.equals("R800") || android.os.Build.MODEL.equals("R800i")));

* You can just specify `Build.MODEL` instead of `android.os.Build.MODEL` because this file already imported android.os.Build above.

* Our Java coding style for string comparisons is to put the string constant first (like `"Sony Ericsson".equals(Build.MANUFACTURER)`. This style is a little ugly but it protects against NullPointerExceptions (because the equal() method checks for null) and may, in theory, allow Java to make some micro-optimization for the method call because the object, the string constant, is known statically at compile-time.

Comment 10

[Shane Tully (:stully:]

Attachment: 03: Remap the circle and cross buttons to KEYCODE_BUTTON_A or KEYCODE_BUTTON_B depending on region

Revised patch with changes requested by :cpeterson and :kats

Comment 11

[Chris Peterson [:cpeterson]]

Comment on attachment 752890 [details] [diff] [review]
03: Remap the circle and cross buttons to KEYCODE_BUTTON_A or KEYCODE_BUTTON_B depending on region

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

Looks good to me! I can land your patch on mozilla-inbound today.

Comment 12

[Chris Peterson [:cpeterson]]

I landed stully's patch on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd0d9ac28abb

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cd0d9ac28abb