yairm210 / Unciv

Open-source Android/Desktop remake of Civ V
Mozilla Public License 2.0
8.57k stars 1.58k forks source link

Refactor key handling #7010

Closed doublep closed 2 years ago

doublep commented 2 years ago

Background: I wanted to add a shortcut to the tile overlay created in WorldMapHolder. In addTileOverlays() I added worldScreen.keyPressDispatcher[KeyCharAndCode.TAB] = { button.act(0f) }, but it never fired up. Turns out, it gets immediately removed in WorldScreen.update() via a call to keyPressDispatcher.revertToCheckPoint().

I generally dislike how KeyPressDispatcher operates. We have a tree of UI components. Completely separately we have a map of keycodes to performed actions, as arbitrary functions. I would like it better to have keycodes and actions attached to UI components. Then, when a key is pressed, processing code would look through the current tree of active components, check if some non-disabled one "wants" to handle this key, and if yes, dispatch the keypress to it. I can see two advantages:

I'm willing to perform this, but here I'd like some general idea approval first, since this is not going to be easy, and without immediate benefits.

SomeTroglodyte commented 2 years ago

You're not the first with that idea. It was there before the class was created. Problem is, key focus lives on the stage, it's not working quite like the normal Gdx event cascade. If you get key listeners to work on an Actor, kudos. Maybe even the lwjgl3 move changes something helpful that I missed, but I doubt it. That's why after a lot of cursing I homed it on the screen. Oh, and if you're coming from some other Forms framework - listeners get served the wrong way round, don't be surprised.

And if you succeed, you'd automatically solve another outstanding problem - main menu loses its key bindings when entering a less aggressive screen and leaving, like Civilopedia! (the easiest ad-hoc bandaid - use KeyPressDispatcher.isPaused instead of what happens now with dispose in setScreen... But too lazy to test)

And maybe you have some idea how to fix the lwjgl3-caused regression where keycodes now are by geometry and ignore layout, so QWERTZ and AZERTY can't use some key as advertised by the tooltips. It's not Gdx - it has no international key mappings. It maps like crazy from the GLFW codes which are completely different but locale independent, and does scancode- char mapping for a handful of keys (return backspace tab..), that's all. InputEvent.character is effectively dead and always zero except for those few. And reaching through to the underlying library bypassing Gdx - too high for me.

Which is why I still toy with the idea to offer configurable key bindings, then every user can re-map themselves. Or revert to keyPressed - but there was a reason which I forgot, someone was quite insistent. Cyrillic keyboards involved. (...) Huh? Actually it was #5614 which replaced keyPressed with keyDown, I assumed it was #4542... Goes to show. Weak memory.

So if you manage to get a working Widget-bound key binding, kudos, you have not only my support but my admiration :+1: :tada: :balloon: :partying_face: .

SomeTroglodyte commented 2 years ago

Extra fat & juicy congratulations with cream on top!