Allow hotkeys to be undefined.
ClosedPublic
Actions

Authored by wraitii on Jan 9 2021, 11:24 AM.

Details

Reviewers: None
Commits: rP24618: Allow hotkeys/config settings to be empty.

Summary

Reported by @Feldfeld.
Hotkeys currently cannot be 'empty'. There is special handling for 'unused', but those don't appear in the hotkey setup window.
The root cause is that ConfigDB wasn't set up to expect empty values, but I think it's fair to accept those.
Adjust hotkey files accordingly.

Test Plan

Test empty hotkeys.

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

wraitii created this revision.Jan 9 2021, 11:24 AM

Build is green

builderr-debug-macos.txt
/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/libgui_dbg.a(precompiled.o) has no symbols
ld: warning: text-based stub file /System/Library/Frameworks//CoreAudio.framework/CoreAudio.tbd and library file /System/Library/Frameworks//CoreAudio.framework/CoreAudio are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/Frameworks//AudioToolbox.framework/AudioToolbox.tbd and library file /System/Library/Frameworks//AudioToolbox.framework/AudioToolbox are out of sync. Falling back to library file f

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

Build has FAILED

builderr-release-gcc7.txt
g++: internal compiler error: Segmentation fault signal terminated program cc1plus
Please submit a full bug report,
with preprocessed source if appropriate.
See <file:///usr/share/doc/gcc-8/README.Bugs> for instructions.
make[1]: *** [engine.make:273: obj/engine_Release/CConsole.o] Error 4
make: *** [Makefile:119: engine] Error 2

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/4389/display/redirect
See console output for more information: https://jenkins.wildfiregames.com/job/docker-differential/4389/display/redirectconsole

Harbormaster completed remote builds in B14801: Diff 15068.Jan 9 2021, 11:35 AM

wraitii requested review of this revision.Jan 9 2021, 11:35 AM

Stan added a subscriber: Stan.Jan 10 2021, 11:25 AM

Stan added inline comments.

source/ps/ConfigDB.cpp
399 ↗	(On Diff #15068)	This change is only for KeyMap hotkeys right? Or can it break other stuff, like for instance vsync?

wraitii added inline comments.Jan 10 2021, 2:19 PM

source/ps/ConfigDB.cpp
399 ↗	(On Diff #15068)	How is vsync supposed to work? This affects all config options.

Stan added inline comments.Jan 11 2021, 11:12 AM

source/ps/ConfigDB.cpp
399 ↗	(On Diff #15068)	I mean how does it affect things relying on numeric values?

wraitii added inline comments.Jan 11 2021, 1:35 PM

source/ps/ConfigDB.cpp
399 ↗	(On Diff #15068)	Well the only thing it does is allow empty values, so I guess it could? But I don't think we really have any at the moment.

Add tests that validate that this doesn't crash.

Build is green

builderr-debug-macos.txt
/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/libgui_dbg.a(precompiled.o) has no symbols
ld: warning: text-based stub file /System/Library/Frameworks//CoreAudio.framework/CoreAudio.tbd and library file /System/Library/Frameworks//CoreAudio.framework/CoreAudio are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/Frameworks//AudioToolbox.framework/AudioToolbox.tbd and library file /System/Library/Frameworks//AudioToolbox.framework/AudioToolbox are out of sync. Falling back to library file f

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

Harbormaster completed remote builds in B14915: Diff 15181.Jan 12 2021, 4:02 PM

vladislavbelov added a subscriber: vladislavbelov.Jan 12 2021, 4:19 PM

vladislavbelov added inline comments.

source/ps/ConfigDB.cpp
102–104 ↗	(On Diff #15181)	Why do you need to request value at all for such case?
447–450 ↗	(On Diff #15181)	Useless, `c_str()` always return a valid C-string.
source/ps/Hotkey.cpp
52 ↗	(On Diff #15181)	Why do you need to store unused hotkeys?

wraitii added inline comments.Jan 12 2021, 4:21 PM

source/ps/ConfigDB.cpp
102–104 ↗	(On Diff #15181)	mh, yeah, good point.
447–450 ↗	(On Diff #15181)	Not sure what you mean, this is writing `""` ?
source/ps/Hotkey.cpp
52 ↗	(On Diff #15181)	So they appear in the hotkey editor. There is no 'central' list of hotkeys, and doing it via the config doesn't seem much better.

Stan added inline comments.Jan 12 2021, 5:42 PM

source/ps/ConfigDB.cpp
447–450 ↗	(On Diff #15181)	I guess he means you don't need sprintf?
source/ps/Hotkey.cpp
54 ↗	(On Diff #15181)
source/ps/scripting/JSInterface_Hotkey.cpp
87 ↗	(On Diff #15181)	Not sure what that does.
source/ps/tests/test_ConfigDB.h
33 ↗	(On Diff #15181)
40 ↗	(On Diff #15181)

wraitii marked an inline comment as done.Jan 13 2021, 9:16 AM

wraitii added inline comments.

source/ps/ConfigDB.cpp
447–450 ↗	(On Diff #15181)	I think it's convenient and unlikely to be much worse than a straight concatenation here (which is harder to write)

Address comments.

Build is green

builderr-debug-macos.txt
/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/libgui_dbg.a(precompiled.o) has no symbols
ld: warning: text-based stub file /System/Library/Frameworks//CoreAudio.framework/CoreAudio.tbd and library file /System/Library/Frameworks//CoreAudio.framework/CoreAudio are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/Frameworks//AudioToolbox.framework/AudioToolbox.tbd and library file /System/Library/Frameworks//AudioToolbox.framework/AudioToolbox are out of sync. Falling back to library file f

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

Harbormaster completed remote builds in B14979: Diff 15234.Jan 13 2021, 9:43 AM

This revision was not accepted when it landed; it landed in state Needs Review.Jan 15 2021, 9:05 AM

Closed by commit rP24618: Allow hotkeys/config settings to be empty. (authored by wraitii). · Explain Why

This revision was automatically updated to reflect the committed changes.

wraitii added a commit: rP24618: Allow hotkeys/config settings to be empty..