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.

Sign up for GitHub

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

Jump to bottom

TGUI Keybind Menu #21681

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

TGUI Keybind Menu #21681

wants to merge 15 commits into from
+301 −55

Conversation

Garash2k
Copy link
Contributor

@Garash2k Garash2k commented Dec 14, 2024 •
edited
Loading

About the PR

Ports the Keybind Menu from CHUI to TGUI
image
image

White theme:
image

Draft points:

  • I'm too tired to make sure everything is fine right now :V
  • After changing, keybind order tends to shuffle. It's more egregious now that updates are faster.
  • dubious code such as having multiple current_keymap = owner.keymap and a unparse = changed_keys["[current_keymap.keys[key]]"] || current_keymap.unparse_keybind(key),
  • todo: capturing keyboard input in lieu of typing characters.

Why's this needed?

Besides the technology upgrade, current CHUI menu is kinda broken, opening it while spectating permanently bricks trying to use it while alive (#19151.) Bug is fixed with this version

Note: it seems like the keybinds menu was somewhat fragile too regarding binding already bound keys. New UI is somewhat as fragile regarding that, a cleanup of the keybinding code would probably be required, and I'd like to think is out of scope of this PR, either that or I'd need help :V

Old UI:
261892373-7317fd5f-3030-4feb-ae88-58c9bbdfbd5a

[UI][QoL][Feature]

@keywordlabeler keywordlabeler bot added A-UI Modifies UI in some way. Automatically applied on a change to tgui/ C-Feature A new feature or enhancements to existing features C-QoL A quality of life improvement that makes the game easier to play labels Dec 14, 2024
@github-actions github-actions bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 14, 2024
@ZeWaka
Copy link
Member

ZeWaka commented Dec 14, 2024

How does The binding mechanism work? Is it a straight character input or does it capture keyboard input? The latter would be a lot nicer, other servers have implementations we can use

@Garash2k
Copy link
Contributor Author

Straight character input is how both the old ui and this version works, but I agree capturing keyboard input is absolutely the way to go, adding it as a todo

@Garash2k Garash2k mentioned this pull request Dec 14, 2024
Open
@FlameArrow57
Copy link
Contributor

Nice

@Garash2k Garash2k marked this pull request as ready for review December 15, 2024 01:14
@github-actions github-actions bot added the S-Merge-Conflict Applied and removed when a PR has or no longer has a merge conflict label Dec 23, 2024
@Garash2k Garash2k mentioned this pull request Dec 25, 2024
Open
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 7, 2025

This PR has been inactive for two weeks, and has been automatically marked as stale. This means it is at risk of being auto closed in another week. Please address any outstanding review items and ensure your PR is finished. If you are auto-staled anyway, ask developers if your PR will be merged. Once you have done any of the previous actions then you should request a developer remove the stale label on your PR, to reset the stale timer. If you feel no developer will respond in that time, you may wish to close this PR youself, while you seek developer comment, as you will then be able to reopen the PR yourself.

@github-actions github-actions bot added the S-Stale An inactive PR that has had no updates in the past two weeks label Jan 7, 2025
@ZeWaka
Copy link
Member

ZeWaka commented Jan 8, 2025

X

@TobleroneSwordfish TobleroneSwordfish added E-Certified-Organic Will not be marked stale. and removed S-Stale An inactive PR that has had no updates in the past two weeks labels Jan 8, 2025
@TobleroneSwordfish
Copy link
Contributor

TobleroneSwordfish commented Jan 8, 2025 •
edited
Loading

This looks good, will need testmerging but we have another TGUI PR testmerged right now so will have to wait.
The only thing I'd say it could use is a modified notification like the character setup page, oh and in theory a way to block the browser shortcuts so you can bind something to CTRL + N without getting IE jumpscared. Not sure how possible that is but suuurely we can capture that somehow?? (do we have preventDefault?)

@github-actions github-actions bot added S-Merge-Conflict Applied and removed when a PR has or no longer has a merge conflict and removed S-Merge-Conflict Applied and removed when a PR has or no longer has a merge conflict labels Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mordent-goonstation mordent-goonstation Awaiting requested review from mordent-goonstation mordent-goonstation is a code owner

@ZeWaka ZeWaka Awaiting requested review from ZeWaka ZeWaka is a code owner

Assignees
No one assigned
Labels
A-UI Modifies UI in some way. Automatically applied on a change to tgui/ C-Feature A new feature or enhancements to existing features C-QoL A quality of life improvement that makes the game easier to play E-Certified-Organic Will not be marked stale. S-Merge-Conflict Applied and removed when a PR has or no longer has a merge conflict size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
TGUI Interface Conversion
Status: In Progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Garash2k @ZeWaka @FlameArrow57 @TobleroneSwordfish