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

Fix Ctrl keycodes #7191

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix Ctrl keycodes #7191

wants to merge 3 commits into from

Conversation

M374LX
Copy link

@M374LX M374LX commented Jul 20, 2023

Fixes #7189.

Frosty-J

This comment was marked as outdated.

Copy link
Contributor

@Frosty-J Frosty-J left a comment

Choose a reason for hiding this comment

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

Sorry, I was too quick to react. It's probably okay, so long as everyone's using the constants rather than hardcoding the values. Just add something in the CHANGES file such as [BREAKING CHANGE] CONTROL_LEFT and CONTROL_RIGHT are now keycodes 113 and 114 for all platforms, to match Android so it's less likely to catch someone out by surprise.

I checked these files, so it looks okay for all platforms:

@M374LX M374LX requested a review from Frosty-J July 21, 2023 12:01
@mgsx-dev
Copy link
Contributor

It's probably okay, so long as everyone's using the constants rather than hardcoding the values.

Could be a big problem for users that store key mapping in a file. Maybe having a mapping for these keys in DefaultAndroidInput would be better no?

@Frosty-J
Copy link
Contributor

Depends how they store their mapping. If it's just the raw values and not a human-readable format, it's a minor inconvenience.

// Psuedocode of sorts for updating outdated key mapping
if (userKey == 129) userKey = Input.Keys.CONTROL_LEFT;
if (userKey == 130) userKey = Input.Keys.CONTROL_RIGHT;

Remapping in DefaultAndroidInput or somewhere would indeed be an option. I'd rather libGDX stick to some sort of standard, as it seems unnecessarily complicated not to.

@M374LX
Copy link
Author

M374LX commented Jul 21, 2023

We could add a note about storing key mapping in a file to CHANGES.

An alternative is to keep the current keycodes and add a check like this one to DefaultAndroidInput's onKey() method:

int keycode = e.getKeyCode();

if (keycode == 113) {
    keycode = Input.Keys.CONTROL_LEFT; //129
} else if (keycode == 114) {
    keycode = Input.Keys.CONTROL_RIGHT; //130
}

@MrStahlfelge
Copy link
Member

If someone maps the key values by configuration, it should be no problem as the real key values were saved.

MrStahlfelge
MrStahlfelge previously approved these changes Jul 23, 2023
crykn
crykn previously approved these changes Jul 24, 2023
@Berstanio
Copy link
Contributor

If someone maps the key values by configuration, it should be no problem as the real key values were saved.

I don't quite understand, how this does not break. If I write 129 as a key mapping in a config file and compare that "old" value for CONTROL_LEFT (129) to a key press event, which has the "new" value for CONTROL_LEFT (113), it will not work, right?
So I would need to write migration logic, as I understand it?

@MrStahlfelge
Copy link
Member

MrStahlfelge commented Jul 24, 2023

Usually you write custom key mapping configuration in a way that users press the keys they want. The bug here is that a constant in our framework has the wrong value. Independent from that, if a user presses a key, the system gives the real value and not a wrong value from our framework.
After we fix the wrong value in our framework, if a user presses a key, the system still gives the correct value. So no migration is needed.

If you really wrote the old value 129 in your mapping file, the mapping just did not work before the change and after the change. At least it did not work when the user pressed Ctrl key

@Berstanio
Copy link
Contributor

How I understand it is the following:
The user presses a key and the backend specific key code gets converted to a gdx keycode, which gets propagated to the listeners, like here:

key = getGdxKeyCode(key);
eventQueue.keyDown(key, System.nanoTime());

I then receive the gdx keycode the user pressed for the custom mapping and write it to a file. After that I can compare the saved gdx keycode to the pressed gdx keycode. If now the pressed gdx keycode changes, this wont work anymore in my understanding.

I can test this pr someday later.

If you really wrote the old value 129 in your mapping file, the mapping just did not work before the change and after the change. At least it did not work when the user pressed Ctrl key

I just tested it, mapping the Ctrl key works on desktop.

@MrStahlfelge
Copy link
Member

My fault, I only thought about Android and forgot that all other platforms use the constants for internal mapping. Sorry.

@crykn crykn added this to the 1.12.1 milestone Jul 24, 2023
@M374LX M374LX dismissed stale reviews from crykn and MrStahlfelge via 2951e29 July 24, 2023 21:06
@M374LX
Copy link
Author

M374LX commented Jul 24, 2023

In fact, I did not think about the key mappings being saved in a file while preparing this PR.

On platforms other than Android, there might be problems when saving the old values (129 and 130) from an old libGDX version, upgrading libGDX, and then loading the values, which will no longer correspond to those of Keys.CONTROL_LEFT and Keys.CONTROL_RIGHT.

Does anyone prefer the approach suggested here? #7191 (comment)

@obigu obigu removed this from the 1.12.1 milestone Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong Ctrl keycodes on Android
7 participants