Format hotkeys when an alternate variant is specified
ClosedPublic
Actions

Authored by s0600204 on Jan 14 2021, 7:29 AM.

Details

Reviewers

wraitii

Commits

rP24668: Format hotkey tooltip when an alternate key-name is used

Summary

This is what the tooltip for the Close Button in the civinfo page looks like right now:

The hotkey being displayed should be "Escape" (from the hotkey "Cancel" which is set by default to the keyname Escape), but as you can see instead it is an empty string.

This is because whilst Escape and Esc are both recognised as valid keynames, and indeed pressing the Escape key works whichever of these is specified, only Esc is present in the mapping returned by Engine.GetScancodeKeyNames().

The same happens if (manually in default.cfg) you assign a Keyboard {xxx} keyname (e.g. Keypad -) instead of its Num{xxx} variant (e.g. NumMinus): the latter is formatted in tooltips correctly, the former results in a blank string.

This revision seeks to fix that.

(Related: rP24215)

I don't know how common it is, but for me attempting to assign Equals to a hotkey in default.cfg brings up a message next time pyrogenesis is run that Equals isn't a recognised keyname. I've amended the keys.txt file, but if this isn't true for anyone else I'm happy to change it back.

Test Plan

Without and with the patch applied, look at the tooltip of the Close Button of a Reference Suite page (structree, civinfo, viewer) and confirm that the patch causes the hotkey to appear, formatted, in the tooltip.

Try changing a hotkey assignment of your choosing (preferably one that ends up being displayed in a tooltip (not the hotkey picker)) via the default.cfg file and look at the resultant tooltip within 0 A.D.. Note that - without this patch applied - some of the options listed in data/config/keys.txt appear as empty strings in tooltips. (And some cause a message that that keyname is not recognised.)

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

s0600204 created this revision.Jan 14 2021, 7:29 AM

Build is green

builderr-debug-macos.txt
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/liblobby_dbg.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libsimulation2_dbg.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libengine_dbg.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libgraphics_dbg.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libatlas_dbg.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../

See https://jenkins.wildfiregames.com/job/macos-differential/2856/display/redirect for more details.

Harbormaster completed remote builds in B15022: Diff 15284.Jan 14 2021, 7:41 AM

s0600204 requested review of this revision.Jan 14 2021, 7:41 AM

Stan added a reviewer: wraitii.Jan 14 2021, 9:05 AM

Relates to rP24504 most of all.

I guess we don't really have a choice, I didn't consider that tooltips read from the config directly and that's kind of broken, and rP24504 was obviously not enough to fix it.
A v2 diff will need to store hotkey in a more intelligent way so it's easy to get combinations for one specific hotkey.

binaries/data/config/keys.txt
63 ↗	(On Diff #15284)	Do you mean "Keypad +" not work? Because I think it should
binaries/data/mods/public/gui/common/color.js
175 ↗	(On Diff #15284)	This works but it's really inefficient. I guess we only show one tooltip at a time so it should be fine, but we really need a different system.

This revision is now accepted and ready to land.Jan 14 2021, 9:14 AM

s0600204 added inline comments.Jan 14 2021, 10:25 AM

binaries/data/config/keys.txt
63 ↗	(On Diff #15284)	Yep: using "Keyboard +" in `default.cfg` and then (re)starting 0 A.D. presents me with WARNING: Hotkey mapping used invalid key 'Keyboard +' ("NumPlus" is accepted.) ("Keyboard +/-" (line 160) causes a similar message to be emitted, and I haven't tried any of the other "Keyboard {x}" possibilities listed on lines 108 & 127-166. Then again, I don't have any of those keys on my keyboard, and I don't know if that matters or not.)