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

Customizable keybindings #1979

Merged
merged 56 commits into from
Jul 8, 2024
Merged

Customizable keybindings #1979

merged 56 commits into from
Jul 8, 2024

Conversation

xsebek
Copy link
Member

@xsebek xsebek commented Jun 23, 2024

  • create a new SwarmEvent enumeration
    • define configuration names (swarmEvents)
    • define default keybindings (defaultSwarmBindings)
  • add KeyConfig and KeyDispatchers to AppState
    • load custom bindings from an INI file
  • migrate key event controller code to use onEvent
    • call handleKey with handlers
  • show the shortcuts in TUI
  • check for conflicts between parent/child handler (right now mainHandler and the rest)
  • generate the INI file (keybinding --init) with commented-out (;) settings
  • allow brick version 2.4 to get uppercase keybindings fix

@xsebek
Copy link
Member Author

xsebek commented Jun 27, 2024

It works! 🥳 Add an INI file:

$ cat /Users/xsebek/.config/swarm/config.ini
[keybindings]
debug CESK = Ctrl-u

Print the changed keybindings:

cabal run -O0 swarm -- keybindings --markdown
Output

Keybindings

main

Keybinding Event Name Description
C-q quit Open quit game dialog
F1 view help View Help screen
F2 view robots View Robots screen
F3 view recipes View Recipes screen
F4 view commands View Commands screen
F6 view structures View Structures screen
C-g view goal View scenario goal description
M-h hide robots Hide robots for a few ticks
C-u debug CESK Show active robot CESK machine debugging line

repl

Keybinding Event Name Description
C-c, Esc cancel running program Cancel running base robot program
M-p toggle piloting mode Toggle piloting mode
M-k toggle custom key handling Toggle custom key handling mode

The configuration file is at:
/Users/xsebek/.config/swarm/config.ini

@xsebek
Copy link
Member Author

xsebek commented Jun 29, 2024

After a lot of shuffling code back and forth, the main game handlers are customizable:

Output

Keybindings

main

Keybinding Event Name Description
Esc (non-customizable key) Close open modal
C-q quit Open quit game dialog
F1 view help View Help screen
F2 view robots View Robots screen
F3 view recipes View Recipes screen
F4 view commands View Commands screen
F5 view messages View Messages screen
F6 view structures View Structures screen
C-g view goal View scenario goal description
M-h hide robots Hide robots for a few ticks
M-d debug CESK Show active robot CESK machine debugging line
C-p pause Pause or unpause the game
C-o run single tick Run game for a single tick
C-x increse TPS Increase game speed by one tick per second
C-z decrease TPS Descrease game speed by one tick per second
M-w focus World Set focus on the World panel
M-e focus Robot Set focus on the Robot panel
M-r focus REPL Set focus on the REPL panel
M-t focus Info Set focus on the Info panel
C-v creative mode Toggle creative mode
C-e world editor Toggle world editor mode
M-, toggle REPL Collapse/Expand REPL panel

repl

Keybinding Event Name Description
C-c, Esc cancel running program Cancel running base robot program
M-p toggle piloting mode Toggle piloting mode
M-k toggle custom key handling Toggle custom key handling mode

world

Keybinding Event Name Description
c view base View the base robot
f show fps Show frames per second
k, Up move view north Scroll world view in the north direction
l, Right move view east Scroll world view in the east direction
j, Down move view south Scroll world view in the south direction
h, Left move view west Scroll world view in the west direction

robot

Keybinding Event Name Description
Enter (non-customizable key) Show entity description
m make entity Make the selected entity
0 show zero inventory entites Show entities with zero count in inventory
; cycle inventory sort Cycle inventory sorting type
: switch inventory direction Switch ascending/descending inventory sort
/ search inventory Start inventory search

@xsebek
Copy link
Member Author

xsebek commented Jun 29, 2024

@byorgey @kostmo, While this is not finished yet, I would appreciate it if you could test this out.

The original code could have behaved differently because the conditions allowed events to be handled by subhandlers.

If we really want that behavior, then those handlers have to stay in the old not-as-declarative code.
For example, Esc is used everywhere, and I made the mistake of putting it into the main handler.
But there could be more obscure bugs or just plain typos that I made.


By the way, this refactoring makes the Controller significantly smaller:

 src/swarm-tui/Swarm/TUI/Controller.hs | 827 ++---------------------------------------------
 27 insertions(+), 800 deletions(-)

It is necessary because the handlers are stored in the Model, which is imported into the Controller, and a cycle would form.

@xsebek xsebek requested review from kostmo and byorgey June 29, 2024 20:05
@byorgey
Copy link
Member

byorgey commented Jun 29, 2024

Just some small initial feeback: so far I'm just trying it out without having looked at the code yet (since that's what players will do). I found swarm keybindings quickly, which told me where the config file should go, but I don't know what format is expected. However, I see you have some todos for documentation and generating an initial .ini file, so maybe you have that under control!

@xsebek
Copy link
Member Author

xsebek commented Jun 29, 2024

$ cat /Users/xsebek/.config/swarm/config.ini
[keybindings]
debug CESK = Ctrl-u

@byorgey, exactly. The INI file looks like that, with Event Names as keys and values being parsed by parseBindingList.

I think showing by example will be much clearer than trying to explain the format in detail. 🙂

@byorgey
Copy link
Member

byorgey commented Jun 29, 2024

I accidentally wrote a keybinding in uppercase, like Ctrl-T, and it turns out this means it parses successfully, with no errors or warnings, but simply does not work --- both Ctrl-t and Ctrl-Shift-t do nothing. I suspect it turns into a keybinding which is impossible to ever see, because Ctrl-Shift-t is not the same as Ctrl-T. Well, I don't know, perhaps it is possible to get Ctrl-T by turning on caps lock and then typing Ctrl-t? In any case it seems like some kind of warning here would be nice.

Copy link
Member

@byorgey byorgey left a comment

Choose a reason for hiding this comment

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

I haven't looked at absolutely all the code yet, but so far it seems to work great. I was able to remap the key shortcuts for the four panels which is something I have wanted for a very long time. 😁

@byorgey
Copy link
Member

byorgey commented Jun 29, 2024

I think showing by example will be much clearer than trying to explain the format in detail.

Agreed, I think generating an initial .ini file with all the default keybindings commented out will be perfect.

app/Main.hs Outdated Show resolved Hide resolved
app/Main.hs Outdated Show resolved Hide resolved
src/swarm-tui/Swarm/TUI/Controller/EventHandlers.hs Outdated Show resolved Hide resolved
src/swarm-tui/Swarm/TUI/Model/KeyBindings.hs Outdated Show resolved Hide resolved
src/swarm-tui/Swarm/TUI/Model/KeyBindings.hs Outdated Show resolved Hide resolved
src/swarm-tui/Swarm/TUI/Model/Event.hs Outdated Show resolved Hide resolved
src/swarm-tui/Swarm/TUI/Model/Event.hs Outdated Show resolved Hide resolved
src/swarm-tui/Swarm/TUI/Controller/Util.hs Outdated Show resolved Hide resolved
src/swarm-tui/Swarm/TUI/Controller/Util.hs Outdated Show resolved Hide resolved
@byorgey
Copy link
Member

byorgey commented Jun 30, 2024

By the way, key shortcuts are shown in two places in the UI: in the bar between the REPL and world panels, and also in the F1 help modal. I think in the F1 modal we should mention something about how to customize keybindings.

app/Main.hs Outdated Show resolved Hide resolved
@byorgey
Copy link
Member

byorgey commented Jun 30, 2024

I just discovered a regression: on main, hitting Esc at the REPL has no effect. With this PR, hitting Esc makes the REPL display ... (as if the base is executing a program) and it's impossible to get back the normal prompt (even with Ctrl-C).

@xsebek
Copy link
Member Author

xsebek commented Jul 1, 2024

@byorgey I tried setting Esc as alternative to Ctrl-C to cancel running computation.

Does Ctrl-C break the game on main?

@byorgey
Copy link
Member

byorgey commented Jul 1, 2024

Does Ctrl-C break the game on main?

Aha, it does! So this is not your fault. I've created #2007.

@xsebek
Copy link
Member Author

xsebek commented Jul 5, 2024

Finally, I managed to show the custom keybindings in the UI:
Screenshot 2024-07-05 at 22 04 52

@xsebek xsebek marked this pull request as ready for review July 5, 2024 21:27
@xsebek xsebek requested a review from byorgey July 5, 2024 21:27
@xsebek
Copy link
Member Author

xsebek commented Jul 5, 2024

I think the custom keybindings are now ready. 😁

@byorgey @kostmo and anyone else interested, please give this a try.

I would also welcome your code review. There is not that much new code; it was mostly just moved from Controller.
Notable new modules are:

  • Swarm.TUI.Model.Event
  • Swarm.TUI.Controller.EventHandlers
  • Swarm.TUI.Model.KeyBindings

@jtdaugherty has promptly fixed uppercase keybindings parsing. You can test that, too, with Brick master. 🙂

@jtdaugherty
Copy link

Nice!!

@jtdaugherty
Copy link

The keybinding case normalization fix is now released in brick-2.4.

app/Main.hs Outdated Show resolved Hide resolved
src/swarm-tui/Swarm/TUI/Controller.hs Outdated Show resolved Hide resolved
src/swarm-tui/Swarm/TUI/Controller/EventHandlers.hs Outdated Show resolved Hide resolved
src/swarm-tui/Swarm/TUI/Controller/EventHandlers.hs Outdated Show resolved Hide resolved
src/swarm-tui/Swarm/TUI/Controller/EventHandlers.hs Outdated Show resolved Hide resolved
Comment on lines 165 to 171
data WorldEvent
= ViewBaseEvent
| ShowFpsEvent
| MoveViewNorthEvent
| MoveViewEastEvent
| MoveViewSouthEvent
| MoveViewWestEvent
Copy link
Member

Choose a reason for hiding this comment

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

This can simplify some code that is cardinal-direction-specific:

Suggested change
data WorldEvent
= ViewBaseEvent
| ShowFpsEvent
| MoveViewNorthEvent
| MoveViewEastEvent
| MoveViewSouthEvent
| MoveViewWestEvent
data WorldEvent
= ViewBaseEvent
| ShowFpsEvent
| MoveViewDirectionEvent AbsoluteDir

Copy link
Member Author

Choose a reason for hiding this comment

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

This makes me handwrite Enum and Bounded and add a few imports, but I admit I wanted to do this too.

The usage becomes much more elegant, I just wish Haskell supported this out of the box.

Copy link
Member

Choose a reason for hiding this comment

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

I asked about this on Discourse, and got some good suggestions:

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, @kostmo; there are some good tips in that discussion. 👍

I think I was considering finitary before, but there was not as much need. Did not know about universeGeneric.

I will split that as a separate Refactoring Issue.

src/swarm-tui/Swarm/TUI/View.hs Outdated Show resolved Hide resolved
src/swarm-tui/Swarm/TUI/Model/KeyBindings.hs Outdated Show resolved Hide resolved
Copy link
Member

@byorgey byorgey left a comment

Choose a reason for hiding this comment

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

This is looking great! Thanks for the hard work on this really nice feature.

src/swarm-tui/Swarm/TUI/Controller/EventHandlers/Main.hs Outdated Show resolved Hide resolved
@byorgey
Copy link
Member

byorgey commented Jul 6, 2024

@xsebek I assume you were already going to do this, but just to be sure, before merging we should make sure to allow brick-2.4 in the .cabal file. I don't think we need to require brick-2.4 (if anyone reports running into issues with lower vs upper keybindings we can just direct them to ensure they are building with brick-2.4; I doubt it will come up).

@byorgey byorgey added the CHANGELOG Once merged, this PR should be highlighted in the changelog for the next release. label Jul 6, 2024
@xsebek xsebek added the merge me Trigger the merge process of the Pull request. label Jul 8, 2024
@mergify mergify bot merged commit 687bad8 into main Jul 8, 2024
12 checks passed
@mergify mergify bot deleted the custom-keybindings branch July 8, 2024 11:44
@xsebek
Copy link
Member Author

xsebek commented Jul 8, 2024

Hooray! 🚀

Thanks, @byorgey and @kostmo, for your reviews and support. 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CHANGELOG Once merged, this PR should be highlighted in the changelog for the next release. merge me Trigger the merge process of the Pull request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Map editor shortcut conflicts with move-to-end-of-line shortcut Customizable keybindings
4 participants