Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable BUTTON_FORWARD and BUTTON_BACK mouse buttons on Android #95460

Merged

Conversation

m4gr3d
Copy link
Contributor

@m4gr3d m4gr3d commented Aug 13, 2024

Follow up to #36910 for Android; Android mouse's BUTTON_FORWARD and BUTTON_BACK buttons are dispatched as key events.

@m4gr3d m4gr3d added this to the 4.4 milestone Aug 13, 2024
@m4gr3d m4gr3d requested a review from a team as a code owner August 13, 2024 02:26
@m4gr3d m4gr3d force-pushed the enable_mouse_forward_back_buttons branch from 90959ca to 2fcdb44 Compare August 13, 2024 13:45
@m4gr3d m4gr3d requested a review from a team as a code owner August 13, 2024 13:45
@m4gr3d m4gr3d requested a review from Calinou August 13, 2024 19:06
@m4gr3d m4gr3d force-pushed the enable_mouse_forward_back_buttons branch from 2fcdb44 to d4ac225 Compare August 27, 2024 13:23
@m4gr3d m4gr3d force-pushed the enable_mouse_forward_back_buttons branch 2 times, most recently from a917a0f to 9d8d5ee Compare August 31, 2024 06:29
@KoBeWi
Copy link
Member

KoBeWi commented Sep 1, 2024

The documentation for KEY_BACK says "Media back key. Not to be confused with the Back button on an Android device.". Is it outdated, or does it refer to a different BACK key?

@m4gr3d m4gr3d force-pushed the enable_mouse_forward_back_buttons branch from 9d8d5ee to d1891ad Compare September 1, 2024 18:02
@m4gr3d m4gr3d requested a review from a team as a code owner September 1, 2024 18:02
Media back key. Not to be confused with the Back button on an Android device.
Back key.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure about this? This was documented by @Calinou in #41581 based on this comment: #19325 (comment)

Did the mapping change since then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least on Linux it seems to refer to the "Internet" browsing keys of the keyboards of yore: https://gitlab.freedesktop.org/xorg/proto/xorgproto/-/blob/master/include/X11/XF86keysym.h#L64

image

As well as possibly the (also history-related) "back" key on some Chromebooks keyboards, according to our code (scancode_map[0xA6] = Key::BACK; // On Chromebooks).

image

Seems to be a similar situation on Windows: vk_map[VK_BROWSER_BACK] = Key::BACK; // (0xA6)

And on Android it's indeed

47:     { AKEYCODE_BACK, Key::BACK }, // (4) Back key.

But the Android NDK docs don't really say more about this. They also have a mapping for AKEYCODE_FORWARD, what's a Forward key on an Android device?

We would need to check whether #19325 is still true or not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Android mouse's BUTTON_FORWARD and BUTTON_BACK buttons are dispatched as key events.

I may be wrong but to me it looks like this PR makes it so that those Android-exclusive inputs are sent through KEY_BACK and KEY_FORWARD inputs.

With that said, please do not change the description to be unusually succinct again. On desktop platforms these keys really refer to the Media keys, which are definitely not the same as the additional mouse buttons.
At least clarify that on Android, these correspond to certain other keys.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh boy, after reading your reply, there's a whole rabbit hole to dissect about the purpose of these keys on various platforms, huh.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They also have a mapping for AKEYCODE_FORWARD, what's a Forward key on an Android device?

The forward key on Android is mapped to KEYCODE_FORWARD:

Key code constant: Forward key. Navigates forward in the history stack. Complement of KEYCODE_BACK.

We would need to check whether #19325 is still true or not.

I don't think the documentation update was ever correct on Android. back events on Android are special but are still sent as back key events and dispatched via numerous devices (e.g: touch button, mouse, keyboard, etc..), so the mapping in Godot should indeed be KEY_BACK and should be available to the game logic which this PR fixes.

Media back key. Not to be confused with the Back button on an Android device.
Back key.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Android mouse's BUTTON_FORWARD and BUTTON_BACK buttons are dispatched as key events.

I may be wrong but to me it looks like this PR makes it so that those Android-exclusive inputs are sent through KEY_BACK and KEY_FORWARD inputs.

With that said, please do not change the description to be unusually succinct again. On desktop platforms these keys really refer to the Media keys, which are definitely not the same as the additional mouse buttons.
At least clarify that on Android, these correspond to certain other keys.

Comment on lines +4186 to +4193
file_menu->get_popup()->add_shortcut(
ED_SHORTCUT_ARRAY("script_editor/history_previous", TTR("History Previous"),
{ int32_t(KeyModifierMask::ALT | Key::LEFT), int32_t(Key::BACK) }),
WINDOW_PREV);
file_menu->get_popup()->add_shortcut(
ED_SHORTCUT_ARRAY("script_editor/history_next", TTR("History Next"),
{ int32_t(KeyModifierMask::ALT | Key::RIGHT), int32_t(Key::FORWARD) }),
WINDOW_NEXT);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned above, these keys really refer to the Media keys. They are used for video and music playback, so this mapping is rather unorthodox on desktop.

@m4gr3d
Copy link
Contributor Author

m4gr3d commented Dec 17, 2024

@akien-mga @KoBeWi @Mickeon Ping on this PR. From the last comment, it doesn't look like there are blockers to going ahead with adding this functionality.

@akien-mga akien-mga merged commit 66b8101 into godotengine:master Dec 17, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants