Changeset View
Standalone View
source/ps/Hotkey.cpp
/* Copyright (C) 2019 Wildfire Games. | /* Copyright (C) 2020 Wildfire Games. | ||||
* This file is part of 0 A.D. | * This file is part of 0 A.D. | ||||
* | * | ||||
* 0 A.D. is free software: you can redistribute it and/or modify | * 0 A.D. is free software: you can redistribute it and/or modify | ||||
* it under the terms of the GNU General Public License as published by | * it under the terms of the GNU General Public License as published by | ||||
* the Free Software Foundation, either version 2 of the License, or | * the Free Software Foundation, either version 2 of the License, or | ||||
* (at your option) any later version. | * (at your option) any later version. | ||||
* | * | ||||
* 0 A.D. is distributed in the hope that it will be useful, | * 0 A.D. is distributed in the hope that it will be useful, | ||||
▲ Show 20 Lines • Show All 139 Lines • ▼ Show 20 Lines | bool isNegated(const SKey& key) | ||||
else if ((int)key.code < UNIFIED_LAST && (int)key.code > SDL_SCANCODE_TO_KEYCODE(SDL_NUM_SCANCODES) && unified[key.code - UNIFIED_SHIFT] == key.negated) | else if ((int)key.code < UNIFIED_LAST && (int)key.code > SDL_SCANCODE_TO_KEYCODE(SDL_NUM_SCANCODES) && unified[key.code - UNIFIED_SHIFT] == key.negated) | ||||
return false; | return false; | ||||
else | else | ||||
return true; | return true; | ||||
} | } | ||||
InReaction HotkeyStateChange(const SDL_Event_* ev) | InReaction HotkeyStateChange(const SDL_Event_* ev) | ||||
{ | { | ||||
if (ev->ev.type == SDL_HOTKEYDOWN) | if (ev->ev.type == SDL_HOTKEYDOWN || ev->ev.type == SDL_HOTKEYPRESS) | ||||
bb: One could notice that this change actually doesn't do anything since a HOTKEYDOWN is always… | |||||
Not Done Inline ActionsThis isn't necessary if you implement the change below. wraitii: This isn't necessary if you implement the change below. | |||||
g_HotkeyStatus[static_cast<const char*>(ev->ev.user.data1)] = true; | g_HotkeyStatus[static_cast<const char*>(ev->ev.user.data1)] = true; | ||||
else if (ev->ev.type == SDL_HOTKEYUP) | else if (ev->ev.type == SDL_HOTKEYUP) | ||||
g_HotkeyStatus[static_cast<const char*>(ev->ev.user.data1)] = false; | g_HotkeyStatus[static_cast<const char*>(ev->ev.user.data1)] = false; | ||||
return IN_PASS; | return IN_PASS; | ||||
} | } | ||||
InReaction HotkeyInputHandler(const SDL_Event_* ev) | InReaction HotkeyInputHandler(const SDL_Event_* ev) | ||||
{ | { | ||||
▲ Show 20 Lines • Show All 45 Lines • ▼ Show 20 Lines | InReaction HotkeyInputHandler(const SDL_Event_* ev) | ||||
} | } | ||||
// Somewhat hackish: | // Somewhat hackish: | ||||
// Create phantom 'unified-modifier' events when left- or right- modifier keys are pressed | // Create phantom 'unified-modifier' events when left- or right- modifier keys are pressed | ||||
// Just send them to this handler; don't let the imaginary event codes leak back to real SDL. | // Just send them to this handler; don't let the imaginary event codes leak back to real SDL. | ||||
SDL_Event_ phantom; | SDL_Event_ phantom; | ||||
phantom.ev.type = ((ev->ev.type == SDL_KEYDOWN) || (ev->ev.type == SDL_MOUSEBUTTONDOWN)) ? SDL_KEYDOWN : SDL_KEYUP; | phantom.ev.type = ((ev->ev.type == SDL_KEYDOWN) || (ev->ev.type == SDL_MOUSEBUTTONDOWN)) ? SDL_KEYDOWN : SDL_KEYUP; | ||||
if (phantom.ev.type == SDL_KEYDOWN) | |||||
Not Done Inline ActionsThe consequent statement reads from a property that isn't defined in all cases where the condition is true. elexis: The consequent statement reads from a property that isn't defined in all cases where the… | |||||
Not Done Inline ActionsThis is better. thanks. elexis: This is better. thanks. | |||||
phantom.ev.key.repeat = ev->ev.type == SDL_KEYDOWN ? ev->ev.key.repeat : 0; | |||||
Not Done Inline ActionsImarok and we two have already wondered about the number. The specs https://wiki.libsdl.org/SDL_KeyboardEvent say:
So I see a possible way to write this line a bit closer to the specs (s/int/bool). The next bullet point to be examined is the completeness (cases in which the property is set). Third To Consider (TOCO?) is this line:
elexis: Imarok and we two have already wondered about the number. The specs https://wiki.libsdl. | |||||
Not Done Inline ActionsSDL likely passes an u8 range here because it can only pass entire bytes, not a single bit, likely due to the platforms it can work on. elexis: SDL likely passes an u8 range here because it can only pass entire bytes, not a single bit… | |||||
Not Done Inline Actionsphantom.ev.key is a SDL_KeyEvent thus the repeat flag is a u8, and as the phantom will be rerun through this function the code below expects that, so seeing no other solution than keeping the u8. bb: phantom.ev.key is a SDL_KeyEvent thus the repeat flag is a u8, and as the phantom will be rerun… | |||||
if ((keycode == SDLK_LSHIFT) || (keycode == SDLK_RSHIFT)) | if ((keycode == SDLK_LSHIFT) || (keycode == SDLK_RSHIFT)) | ||||
{ | { | ||||
phantom.ev.key.keysym.sym = (SDL_Keycode)UNIFIED_SHIFT; | phantom.ev.key.keysym.sym = (SDL_Keycode)UNIFIED_SHIFT; | ||||
unified[0] = (phantom.ev.type == SDL_KEYDOWN); | unified[0] = (phantom.ev.type == SDL_KEYDOWN); | ||||
HotkeyInputHandler(&phantom); | HotkeyInputHandler(&phantom); | ||||
} | } | ||||
else if ((keycode == SDLK_LCTRL) || (keycode == SDLK_RCTRL)) | else if ((keycode == SDLK_LCTRL) || (keycode == SDLK_RCTRL)) | ||||
{ | { | ||||
Show All 27 Lines | if (g_Console && g_Console->IsActive() && keycode < SDL_SCANCODE_TO_KEYCODE(SDL_NUM_SCANCODES)) | ||||
consoleCapture = true; | consoleCapture = true; | ||||
// Here's an interesting bit: | // Here's an interesting bit: | ||||
// If you have an event bound to, say, 'F', and another to, say, 'Ctrl+F', pressing | // If you have an event bound to, say, 'F', and another to, say, 'Ctrl+F', pressing | ||||
// 'F' while control is down would normally fire off both. | // 'F' while control is down would normally fire off both. | ||||
// To avoid this, set the modifier keys for /all/ events this key would trigger | // To avoid this, set the modifier keys for /all/ events this key would trigger | ||||
// (Ctrl, for example, is both group-save and bookmark-save) | // (Ctrl, for example, is both group-save and bookmark-save) | ||||
// but only send a HotkeyDown event for the event with bindings most precisely | // but only send a HotkeyPress/HotkeyDown event for the event with bindings most precisely | ||||
// matching the conditions (i.e. the event with the highest number of auxiliary | // matching the conditions (i.e. the event with the highest number of auxiliary | ||||
// keys, providing they're all down) | // keys, providing they're all down) | ||||
bool typeKeyDown = ( ev->ev.type == SDL_KEYDOWN ) || ( ev->ev.type == SDL_MOUSEBUTTONDOWN ) || (ev->ev.type == SDL_MOUSEWHEEL); | bool typeKeyDown = ( ev->ev.type == SDL_KEYDOWN ) || ( ev->ev.type == SDL_MOUSEBUTTONDOWN ) || (ev->ev.type == SDL_MOUSEWHEEL); | ||||
// -- KEYDOWN SECTION -- | // -- KEYDOWN SECTION -- | ||||
std::vector<const char*> closestMapNames; | std::vector<const char*> closestMapNames; | ||||
Show All 30 Lines | if (accept && !(consoleCapture && hotkey.name != "console.toggle")) | ||||
closestMapNames.push_back(hotkey.name.c_str()); | closestMapNames.push_back(hotkey.name.c_str()); | ||||
} | } | ||||
} | } | ||||
} | } | ||||
for (size_t i = 0; i < closestMapNames.size(); ++i) | for (size_t i = 0; i < closestMapNames.size(); ++i) | ||||
{ | { | ||||
SDL_Event_ hotkeyNotification; | // Send a KeyPress event when a key is pressed initially and on mouseButton and mouseWheel events. | ||||
Not Done Inline ActionsThis is still uninitialized data if its not a keydown, isnt it? elexis: This is still uninitialized data if its not a keydown, isnt it? | |||||
Not Done Inline Actionshow can it be keydown? (see the lines above) bb: how can it be keydown? (see the lines above) | |||||
Not Done Inline ActionsThe SDL_HOTKEYDOWN could be triggered by an SDL_MouseButtonEvent instead of an keydown, can't it? I was wondering if it's zero-initialized or not, but it appears like the repeat property is not defined at all then. (SDL_Event_ is defined in source/lib/external_libraries/libsdl.h: struct SDL_Event_ { SDL_Event ev; }; So this statement creates an SDL_Event. elexis: The `SDL_HOTKEYDOWN` could be triggered by an `SDL_MouseButtonEvent` instead of an keydown… | |||||
Not Done Inline Actionsconvinced. bb: convinced. | |||||
Not Done Inline ActionsIt's the first time for me to work with unions too, but we should be more skeptical about the things we code here. SDL_HOTKEYDOWN is our own event type, it's not part of SDL2. So it is an SDL_UserEvent (as we can see from accessing the user property, see link above and https://wiki.libsdl.org/SDL_UserEvent. elexis: It's the first time for me to work with unions too, but we should be more skeptical about the… | |||||
Not Done Inline Actionshmm yes, the phantom one above is correct then however as tht is a custom KEYDOWN bb: hmm yes, the phantom one above is correct then however as tht is a custom KEYDOWN | |||||
Not Done Inline ActionsThe new line of code is good. "custom KEYDOWN" means SDL_HOTKEYDOWN? (Can you reprhrase or elaborate, I didn't get the sentence) Either way, when reading from or assigning a property we have to lookup if that property exists at all. Compiler complains in most cases, but not this one by the looks of things. (As long as SDL_HOTKEYDOWN is a https://wiki.libsdl.org/SDL_UserEvent it doesn't have a data field called repeat.) Depending on the existence of other input devices that trigger a different SDL events that also have a repeat flag, this line below here might have a duplicate condition the above new line. elexis: The new line of code is good.
"custom KEYDOWN" means `SDL_HOTKEYDOWN`? (Can you reprhrase or… | |||||
Not Done Inline Actionsdon't like that compiler warning however. (KEYDOWN stuff, nvm ) bb: don't like that compiler warning however.
(KEYDOWN stuff, nvm ) | |||||
Not Done Inline Actionsstatic_cast. Also write here a comment, why and how do use this hack. vladislavbelov: `static_cast`. Also write here a comment, why and how do use this hack. | |||||
hotkeyNotification.ev.type = SDL_HOTKEYDOWN; | if (ev->ev.type != SDL_KEYDOWN || ev->ev.key.repeat == 0) | ||||
hotkeyNotification.ev.user.data1 = const_cast<char*>(closestMapNames[i]); | { | ||||
in_push_priority_event(&hotkeyNotification); | SDL_Event_ hotkeyDownNotification; | ||||
hotkeyDownNotification.ev.type = SDL_HOTKEYPRESS; | |||||
hotkeyDownNotification.ev.user.data1 = const_cast<char*>(closestMapNames[i]); | |||||
Not Done Inline ActionsHelper function ? Stan: Helper function ? | |||||
in_push_priority_event(&hotkeyDownNotification); | |||||
} | |||||
Not Done Inline ActionsI would trigger this after HOTKEYDOWN to be honest. This would make it possible to not handle HOTKEYPRESSED explicitly in HotkeyStateChange since we are guaranteed that HOTKEYDOWN triggers first. wraitii: I would trigger this after HOTKEYDOWN to be honest. This would make it possible to not handle… | |||||
Done Inline ActionsThis is not the whole story, is it? HotkeyPress is triggered once, while HOTKEYDOWN is several times, but a Press is always triggered. So in fact it is better to leave the order this way (first press then keydown), and change the check above to Press. bb: This is not the whole story, is it? HotkeyPress is triggered once, while HOTKEYDOWN is several… | |||||
Not Done Inline ActionsThat would work too, yes. wraitii: That would work too, yes. | |||||
// Send a HotkeyDown event on every key, mouseButton and mouseWheel event. | |||||
// For keys the event is repeated depending on hardware and OS configured interval. | |||||
// On linux, modifier keys (shift, alt, ctrl) are not repeated, see https://github.com/SFML/SFML/issues/122. | |||||
SDL_Event_ hotkeyPressNotification; | |||||
hotkeyPressNotification.ev.type = SDL_HOTKEYDOWN; | |||||
hotkeyPressNotification.ev.user.data1 = const_cast<char*>(closestMapNames[i]); | |||||
in_push_priority_event(&hotkeyPressNotification); | |||||
Not Done Inline ActionsSeems you've switched hotkeyPressNotification and hotkeyDownNotification here, buth that's certainly not why it doesn't work in tests. wraitii: Seems you've switched hotkeyPressNotification and hotkeyDownNotification here, buth that's… | |||||
} | } | ||||
Not Done Inline ActionsMaybe this is a bit easier? Something like this? // Send a KeyPress event when the key, mouse button or mousewheel was pressed initially if (ev->ev.type != SDL_KEYDOWN || ev->ev.key.repeat == 0) // Send a HotkeyDown event while the key combination is held. // The event is repeated depending on hardware and OS configured interval. // On linux, modifier keys (shift, alt, ctrl) are not repeated, see https://github.com/SFML/SFML/issues/122. if (ev->ev.type == SDL_KEYDOWN) elexis: Maybe this is a bit easier? Something like this?
(The linux comment should be on a separate… | |||||
Not Done Inline ActionsThere are three functions that run these 4 lines of code. elexis: There are three functions that run these 4 lines of code.
So I wonder if a… | |||||
// -- KEYUP SECTION -- | // -- KEYUP SECTION -- | ||||
for (const SHotkeyMapping& hotkey : g_HotkeyMap[keycode]) | for (const SHotkeyMapping& hotkey : g_HotkeyMap[keycode]) | ||||
{ | { | ||||
// If it's a keydown event, won't cause HotKeyUps in anything that doesn't | // If it's a keydown event, won't cause HotKeyUps in anything that doesn't | ||||
// use this key negated => skip them | // use this key negated => skip them | ||||
// If it's a keyup event, won't cause HotKeyUps in anything that does use | // If it's a keyup event, won't cause HotKeyUps in anything that does use | ||||
Show All 9 Lines | for (const SKey& k : hotkey.requires) | ||||
if (!accept) | if (!accept) | ||||
break; | break; | ||||
} | } | ||||
if (accept) | if (accept) | ||||
{ | { | ||||
SDL_Event_ hotkeyNotification; | SDL_Event_ hotkeyNotification; | ||||
hotkeyNotification.ev.type = SDL_HOTKEYUP; | hotkeyNotification.ev.type = SDL_HOTKEYUP; | ||||
hotkeyNotification.ev.user.data1 = const_cast<char*>(hotkey.name.c_str()); | hotkeyNotification.ev.user.data1 = const_cast<char*>(hotkey.name.c_str()); | ||||
Not Done Inline Actions(Was wondering if we should do the consistency thing here, but actually no since these are two different user events) elexis: (Was wondering if we should do the consistency thing here, but actually no since these are two… | |||||
in_push_priority_event(&hotkeyNotification); | in_push_priority_event(&hotkeyNotification); | ||||
} | } | ||||
} | } | ||||
return IN_PASS; | return IN_PASS; | ||||
} | } | ||||
bool HotkeyIsPressed(const CStr& keyname) | bool HotkeyIsPressed(const CStr& keyname) | ||||
{ | { | ||||
return g_HotkeyStatus[keyname]; | return g_HotkeyStatus[keyname]; | ||||
} | } |
One could notice that this change actually doesn't do anything since a HOTKEYDOWN is always followed after a HOTKEYPRESS and stops at HOTKEUYP