-
Notifications
You must be signed in to change notification settings - Fork 678
Configurable hotkeys + fastforward hotkey as example #1519
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
base: main
Are you sure you want to change the base?
Conversation
8b4d7f5
to
3baf0a0
Compare
fixed modifiers not saving to config + cut down hotkey values to 2bytes instead of 4bytes. |
3baf0a0
to
b22cae4
Compare
added missing init call (needed for mapping configured hotkeys on startup) |
Question; |
if this gets approved, either me or anyone else could add a hotkey for exiting the emulator. |
src/gui/input/HotkeySettings.cpp
Outdated
static std::unordered_map<uHotkey*, std::function<void(wxFrame&)>> cfg_hotkey_to_func_map{ | ||
{&cfg_hotkeys.toggle_fullscreen, [](wxFrame& frame) {frame.ShowFullScreen(!frame.IsFullScreen());}}, | ||
{&cfg_hotkeys.toggle_fullscreen_alt, [](wxFrame& frame) {frame.ShowFullScreen(!frame.IsFullScreen());}}, | ||
{&cfg_hotkeys.exit_fullscreen, [](wxFrame& frame) {frame.ShowFullScreen(false);}}, | ||
{&cfg_hotkeys.take_screenshot, [](wxFrame&) {g_window_info.has_screenshot_request = true;}}, | ||
{&cfg_hotkeys.toggle_fastforward, [](wxFrame& frame) {ActiveSettings::SetTimerShiftFactor((ActiveSettings::GetTimerShiftFactor() < 3) ? 3 : 1);}}, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems arbitrary to have the MainWindow frame passed as an argument for all hotkey functions when not all of them use it. Could you call the frame functions via some other means?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm ya, mainly did this for fullscreen hotkeys. will see if i can store a static pointer to mainwindow on init or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest moving the init call to CemuApp::OnInit() I see you already did!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
switched to storing pointer to main window on init.
src/gui/input/HotkeySettings.h
Outdated
wxFlexGridSizer* m_sizer; | ||
std::vector<HotkeyEntry> m_hotkeys; | ||
std::unordered_map<wxButton*, uHotkey*> m_input_to_cfg_map; | ||
bool need_to_save = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General note: Please conform to the style guidelines. Admittedly some of Cemu's code already strays from it, but at the very least need_to_save should have an m_
prefix for consistency. I don't know how strictly we should enforce the style guidelines, but I think it's best to adhere to it for new code. I'll defer to @Exzap on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's my style too, somehow missed this one though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
src/gui/input/HotkeySettings.cpp
Outdated
if (new_hotkey.raw != old_hotkey.raw) | ||
{ | ||
auto& cfg_hotkey = *m_input_to_cfg_map[obj]; | ||
need_to_save |= (new_hotkey.raw != old_hotkey.raw); | ||
cfg_hotkey = new_hotkey; | ||
hotkey_to_func_map.erase(old_hotkey.raw); | ||
hotkey_to_func_map[cfg_hotkey.raw] = cfg_hotkey_to_func_map[&cfg_hotkey]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m_input_to_cfg_map can be eliminated. wxButton inherits from wxEvtHandler which means you can call SetClientData() to store any arbitrary pointer, which can later be retrieved with GetClientData(). As wxWidgets' documentation points out Normally, any extra data the programmer wishes to associate with the object should be made available by deriving a new class with new data members.
but I think in this case defining a whole new button class is more trouble than it's worth.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, that's exactly what i was looking for and was surprised when i wasn't able to find it. thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed m_input_to_cfg_map
and done everything with SetClientData
and GetClientData
as was suggested.
src/gui/input/HotkeySettings.cpp
Outdated
hotkey_to_func_map.erase(old_hotkey.raw); | ||
hotkey_to_func_map[cfg_hotkey.raw] = cfg_hotkey_to_func_map[&cfg_hotkey]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this account for an edge case where a user selects the same key combination for multiple hotkeys? Should that be allowed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, it doesn't. probably will block duplicate setups (to avoid having to parse vectors during key press captures).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blocked attempts at trying to configure duplicate hotkeys.
e99dcae
to
6403f0c
Compare
fixed alternative hardcoded fullscreen hotkey (apparently |
src/gui/input/HotkeySettings.cpp
Outdated
static std::unordered_map<uHotkey*, std::function<void(void)>> cfg_hotkey_to_func_map{ | ||
{&cfg_hotkeys.toggle_fullscreen, [](void) {s_main_window->ShowFullScreen(!s_main_window->IsFullScreen());}}, | ||
{&cfg_hotkeys.toggle_fullscreen_alt, [](void) {s_main_window->ShowFullScreen(!s_main_window->IsFullScreen());}}, | ||
{&cfg_hotkeys.exit_fullscreen, [](void) {s_main_window->ShowFullScreen(false);}}, | ||
{&cfg_hotkeys.take_screenshot, [](void) {g_window_info.has_screenshot_request = true;}}, | ||
{&cfg_hotkeys.toggle_fastforward, [](void) {ActiveSettings::SetTimerShiftFactor((ActiveSettings::GetTimerShiftFactor() < 3) ? 3 : 1);}}, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the number of distinct hotkey bindings is known at compile time, wouldn't a fixed size array with enum indices suffice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm, sounds like a bit of an extra step when adding new hotkeys though. maybe could get away with making this map const
?
as for the only other map left - hotkey_to_func_map
- i pretty much go from input to func via hash by using it, so i really don't want to change that one especially.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've thought about it and while it would be possible it would make the code more complex. Still, something about using pointers as keys in a std::map feels off. Though I can't really justify that feeling. I guess it's fine since the pointed to objects have static storage duration anyway.
Otherwise you would have to use the enum as a key everywhere including the button ClientData and map the enum to both uHotkey instances from the config class and also to the functions in separate arrays. (or use std::pair and reduce that to one)
All in all how it currently is is probably cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya, pointers as keys are scary, but since it's pointers to static data, i guess it's a edge-case where it stops being scary 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made the map const
anyways, maybe some of unordered_map data will go to .rodata, who knows.
409f626
to
3208cc6
Compare
fixed issue where you pressing another hotkey input button wouldn't restore the previously clicked hotkey input button. |
3208cc6
to
b2b4a2a
Compare
cleaned up some formatting issues after setting up vscode environment with project's coding style rules in mind. |
b2b4a2a
to
69c541e
Compare
src/gui/input/HotkeySettings.cpp
Outdated
if (active_hotkey_input_btn) | ||
{ | ||
wxKeyEvent revert_event{}; | ||
revert_event.SetEventObject(active_hotkey_input_btn); | ||
on_key_up(revert_event); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of invoking the event handler, perhaps it would be better to separate out the part of on_key_up that resets the button into a separate method and call that from both on_key_up and on_hotkey_input_click?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be better now.
src/gui/input/HotkeySettings.h
Outdated
wxPanel* m_panel; | ||
wxFlexGridSizer* m_sizer; | ||
std::vector<HotkeyEntry> m_hotkeys; | ||
wxButton* active_hotkey_input_btn = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing m_
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added missing prefix
69c541e
to
16c7b13
Compare
will adapt coding style to conform with repo's coding style rules. putting on draft till that is done. |
16c7b13
to
74edd9a
Compare
fixed all occurrences (i hope atleast) of code style being broken. was less annoying than i thought it'd be. |
74edd9a
to
56fb4e6
Compare
moved globals into class scope (i think looks cleaner) |
56fb4e6
to
1facb8a
Compare
disabled default hotkey for the added fast-forward hotkey to not mess up anyone's current setups with unexpected behaviors when pressing SPACE. |
1facb8a
to
dc2d090
Compare
added an ability to cancel hotkey input and clear configured hotkey with a right mouse button click |
@goeiecool9999 since many structural things changed after adding controller hotkey support in #1523 (mainly due to |
I think there's no need for that. We'll have to review everything anyway. It looks like the sHotkeyCfg struct is specifically intended to support binding both keyboard and controller inputs. I don't think it makes much sense to backport that to a PR that only supports keyboard bindings. But to be honest I don't really even see the point in having a separate PR to add the controller support to the hotkeys. I think we should merge it all at once. |
aight, i'll make it so this PR has both keyboard and controller support then. |
dc2d090
to
0de34da
Compare
restructured PR so that it would include the new structure + controller hotkey support. |
0de34da
to
2e7f587
Compare
fixed some code styling issues (snake notation stuff) |
Hm, guess i didn't commit something when it comes to the icon. Will check tomorrow. |
2e7f587
to
d273a82
Compare
seems like vcpkg issue, synced the fork and rebased to latest main. hopefully this works. |
d273a82
to
f666350
Compare
included resources.h for Linux and MacOS (monkey see, monkey do). hopefully linux and macos build pipelines pass now. |
f666350
to
d505ca1
Compare
made sure everything builds on my fork repo, so ready to be reviewed. |
The UI needs a bit more work. There should be spacing between text labels and the window border. You can look at the code of other windows for common values that we use. The buttons should be centered on the same height as the labels next to them, right now they look misaligned. |
src/gui/input/HotkeySettings.cpp
Outdated
{ | ||
if (hotkey.alt) | ||
{ | ||
ret.append("ALT + "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make these strings localizable wrap it inside _("string")
. E.g. the whole line becomes:
ret.append(_("ALT + "));
Also a nitpick: I think only capitalizing the first letter is what is more commonly done and at least to me looks better. E.g. Ctrl
instead of CTRL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made strings localizable and made modifier's names like suggested
src/input/api/Controller.cpp
Outdated
@@ -67,6 +69,22 @@ const ControllerState& ControllerBase::update_state() | |||
|
|||
#undef APPLY_AXIS_BUTTON | |||
|
|||
/* hotkey capturing */ | |||
const auto& hotkey_mod = HotkeySettings::s_cfgHotkeys.modifiers.controller; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working with internal state of HotkeySettings
here. Preferably you would encapsulate this into function(s) of HotkeySettings. Right now the logic of Hotkey tracking leaks into other classes and thats something that should be avoided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
encapsulated logic in CaptureInput
methods for both keyboard and controller hotkeys
src/gui/input/HotkeySettings.cpp
Outdated
}; | ||
|
||
HotkeySettings::HotkeySettings(wxWindow* parent) | ||
: wxFrame(parent, wxID_ANY, "Hotkey Settings") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make the strings in this function localizable. E.g. _("Hotkey Settings")
, same for the hotkey row labels
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
src/gui/input/HotkeySettings.cpp
Outdated
s_keyboardHotkeyToFuncMap[keyboardHotkey] = func; | ||
} | ||
auto controllerHotkey = cfgHotkey->controller; | ||
if (controllerHotkey > 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be >= 0
because -1 is the value for disabled controller hotkeys and not 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, fixed. also defined None
values in sHotkeyCfg
struct. should look better now
d505ca1
to
9855aac
Compare
will address this later, since it will take more time to analyze the code. |
9855aac
to
d617277
Compare
reworked the UI (updated pictures in PR description) had to rework the key/controller hotkey differentiation logic in order to change button colors to be the same as in took some inspiration from Elden Ring for displaying unbound/edit-mode hotkeys 😄 |
Demos (old UI):
Implemented configurable hotkeys and tried to make it as easy as possible to add new hotkeys.
Second commit is an example of how to add a new hotkey and it's also a pretty useful hotkey in my opinion.
im bad at design but it works:


keyboard hotkeys also support modifiers:

can also add non-configurable hotkeys (as example, added
toggle_fullscreen_alt
andexit_fullscreen
, since i doubt anyone would want to change those)