Skip to content

fix(input): Prevent modified key releases from firing plain hotkeys #3233

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

Open
CryoTheRenegade wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix(input): Prevent modified key releases from firing plain hotkeys #3233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5470941
refactor(input): Use KeyState and simplify keyboard state handling
CryoTheRenegade Sep 30, 2026
02e7cdb
fix(input): Preserve modifiers for each buffered key event
CryoTheRenegade Sep 30, 2026
5907158
fix(input): Trigger GUI hotkeys on unmodified key down
CryoTheRenegade Sep 30, 2026
e52d4a4
fix(input): Release held modifiers when keyboard focus resets
CryoTheRenegade Sep 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Core/GameEngine/Include/Common/MessageStream.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,8 @@ class GameMessage : public MemoryPoolObject
MSG_RAW_MOUSE_WHEEL, ///< (Real spin, + is away, - is toward user)
MSG_RAW_MOUSE_END,

MSG_RAW_KEY_DOWN, ///< (KeyDefType) the given key was pressed (uses Microsoft VK_ codes)
MSG_RAW_KEY_UP, ///< (KeyDefType) the given key was released
MSG_RAW_KEY_DOWN, ///< (KeyDefType, KeyState) the given key was pressed
MSG_RAW_KEY_UP, ///< (KeyDefType, KeyState) the given key was released

// Refined Mouse messages
// NOTE: All processing should attempt to use these refined mouse messages, rather than the
Expand Down
12 changes: 6 additions & 6 deletions Core/GameEngine/Include/GameClient/Keyboard.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ struct KeyboardIO

UnsignedByte key; // KeyDefType, key data
UnsignedByte status; // StatusType, above
UnsignedShort state; // KEY_STATE_* in KeyDefs.h
KeyState state; // KEY_STATE_* in KeyDefs.h
UnsignedInt keyDownTimeMsec; // real-time in milliseconds when key went down

};
Expand Down Expand Up @@ -112,7 +112,7 @@ class Keyboard : public SubsystemInterface
Bool isShift();
Bool isCtrl();
Bool isAlt();
Int getModifierFlags() { return m_modifiers; }
KeyState getModifierFlags() { return m_modifiers; }

// access methods for key data
void resetKeys(); ///< reset the state of the keys
Expand All @@ -124,7 +124,7 @@ class Keyboard : public SubsystemInterface
WideChar getPrintableKey( KeyDefType key, Int state );
enum { MAX_KEY_STATES = 3};
private:
void refreshAltKeys() const; ///< refresh the state of the alt keys, necessary after alt tab
void emitModifierKeyUps() const;
protected:

/** get the key data for a single key, KEY_NONE should be returned when
Expand All @@ -136,10 +136,10 @@ class Keyboard : public SubsystemInterface
void updateKeys(); ///< update the state of our key data
Bool checkKeyRepeat(); ///< check for repeating keys
UnsignedByte getKeyStatusData( KeyDefType key ); ///< get key status
Bool getKeyStateBit( KeyDefType key, Int bit ); ///< get key state bit
void setKeyStateData( KeyDefType key, UnsignedByte data ); ///< get key state
Bool getKeyStateBit( KeyDefType key, KeyState bit ); ///< get key state bit
void setKeyStateData( KeyDefType key, KeyState data ); ///< get key state

UnsignedShort m_modifiers;
KeyState m_modifiers;
// internal keyboard data members
//Bool m_capsState; // 1 if caps lock is on
//Bool m_shiftState; // 1 if either shift key is pressed
Expand Down
5 changes: 3 additions & 2 deletions Core/GameEngine/Include/GameClient/MetaEvent.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#pragma once

#include "Common/SubsystemInterface.h"
#include "GameClient/KeyDefs.h"
#include "GameClient/InGameUI.h"


Expand Down Expand Up @@ -447,10 +448,10 @@ class MetaEventTranslator : public GameMessageTranslator

void onKeyEvent(const GameMessage *msg, GameMessageDisposition &disp);
void onKeyModStateRemoved(GameMessageDisposition &disp, MappableKeyModState keyModState);
void onKeyPressed(GameMessageDisposition &disp, Int systemKeyState, MappableKeyType keyType, MappableKeyModState keyModState);
void onKeyPressed(GameMessageDisposition &disp, KeyState systemKeyState, MappableKeyType keyType, MappableKeyModState keyModState);

static MappableKeyType getActionKeyType(Int systemKey); ///< CRTL, ALT, SHIFT will be treated as MK_NONE
static MappableKeyModState getKeyModState(Int systemKeyState); ///< Extract CTRL, ALT, SHIFT key mod state
static MappableKeyModState getKeyModState(KeyState systemKeyState); ///< Extract CTRL, ALT, SHIFT key mod state
};

//-----------------------------------------------------------------------------
Expand Down
92 changes: 48 additions & 44 deletions Core/GameEngine/Source/GameClient/Input/Keyboard.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,23 @@ Keyboard *TheKeyboard = nullptr;
// PRIVATE PROTOTYPES /////////////////////////////////////////////////////////////////////////////
///////////////////////////////////////////////////////////////////////////////////////////////////

//-------------------------------------------------------------------------------------------------
static Bool isCtrlShiftAltKey(KeyDefType key)
{
switch (key)
{
case KEY_LCTRL:
case KEY_RCTRL:
case KEY_LSHIFT:
case KEY_RSHIFT:
case KEY_LALT:
case KEY_RALT:
return TRUE;
}

return FALSE;
}

//-------------------------------------------------------------------------------------------------
/** Given the state of the device, create messages from the input and
* place them on the message stream */
Expand Down Expand Up @@ -138,65 +155,48 @@ void Keyboard::updateKeys()
/** @todo -- if we don't have focus, we could destroy all the keys retrieved
here so that we don't process anything */

m_keyStatus[ m_keys[ index ].key ].state = m_keys[ index ].state;
m_keyStatus[ m_keys[ index ].key ].status = m_keys[ index ].status;
const KeyDefType key = (KeyDefType)m_keys[ index ].key;
const Bool isModifier = isCtrlShiftAltKey(key) || key == m_shift2Key;

m_keyStatus[ key ].state = m_keys[ index ].state;
m_keyStatus[ key ].status = m_keys[ index ].status;

// Update key down time for new key presses
if( BitIsSet( m_keys[ index ].state, KEY_STATE_DOWN ) )
{
m_keyStatus[ m_keys[ index ].key ].keyDownTimeMsec = m_keys[ index ].keyDownTimeMsec;
m_keyStatus[ key ].keyDownTimeMsec = m_keys[ index ].keyDownTimeMsec;
}

// prevent ALT-TAB from causing a TAB event
if( m_keys[ index ].key == KEY_TAB )
if( key == KEY_TAB )
{
if( BitIsSet( m_keyStatus[ KEY_LALT ].state, KEY_STATE_DOWN ) ||
BitIsSet( m_keyStatus[ KEY_RALT ].state, KEY_STATE_DOWN ) )
{
m_keys[index].status = KeyboardIO::STATUS_USED;
}
}
else if( m_keys[ index ].key == KEY_CAPS ||
m_keys[ index ].key == KEY_LCTRL ||
m_keys[ index ].key == KEY_RCTRL ||
m_keys[ index ].key == KEY_LSHIFT ||
m_keys[ index ].key == KEY_RSHIFT ||
m_keys[ index ].key == KEY_LALT ||
m_keys[ index ].key == KEY_RALT )
else if( key == KEY_CAPS || isModifier )

{

//
// this keeps our internal key state accurate event though we don't
// use the returned translation ... kinda weird I think
//
translateKey( m_keys[ index ].key );
translateKey( key );

}

BitSet( m_keys[ index ].state, m_modifiers );
Comment thread
xezon marked this conversation as resolved.

index++;

}

// check for key repeats
checkKeyRepeat();

if( m_modifiers )
{
index = 0;
while( m_keys[ index ].key != KEY_NONE )
{

// set in the modifier data into the already existing up/down state
BitSet( m_keys[ index ].state, m_modifiers );

// next key
index++;

}

}

}

//-------------------------------------------------------------------------------------------------
Expand Down Expand Up @@ -233,7 +233,7 @@ Bool Keyboard::checkKeyRepeat()
{
// Add key to this frame
m_keys[ index ].key = (UnsignedByte)key;
m_keys[ index ].state = KEY_STATE_DOWN | KEY_STATE_AUTOREPEAT; // note: not a bitset; this is an assignment
m_keys[ index ].state = KEY_STATE_DOWN | KEY_STATE_AUTOREPEAT | m_modifiers; // note: not a bitset; this is an assignment
m_keys[ index ].status = KeyboardIO::STATUS_UNUSED;

// Set End Flag
Expand Down Expand Up @@ -749,10 +749,9 @@ void Keyboard::update()
//-------------------------------------------------------------------------------------------------
void Keyboard::resetKeys()
{

// TheSuperHackers @fix Caball009 13/12/2025 Fix bug where game remains in waypoint mode
// because the key up state for the alt key is not detected after alt tab.
refreshAltKeys();
emitModifierKeyUps();

memset( m_keys, 0, sizeof( m_keys ) );
memset( m_keyStatus, 0, sizeof( m_keyStatus ) );
Expand All @@ -765,24 +764,29 @@ void Keyboard::resetKeys()
}

//-------------------------------------------------------------------------------------------------
// Refresh the state of the alt keys, necessary after alt tab
//-------------------------------------------------------------------------------------------------
void Keyboard::refreshAltKeys() const
static void emitRawKeyUpIfDown(const KeyboardIO *keyStatus, KeyDefType key)
{
if (BitIsSet(m_keyStatus[KEY_LALT].state, KEY_STATE_DOWN))
{
GameMessage* msg = TheMessageStream->appendMessage(GameMessage::MSG_RAW_KEY_UP);
msg->appendIntegerArgument(KEY_LALT);
msg->appendIntegerArgument(KEY_STATE_UP);
}
if (BitIsSet(m_keyStatus[KEY_RALT].state, KEY_STATE_DOWN))
if (BitIsSet(keyStatus[key].state, KEY_STATE_DOWN))
{
GameMessage* msg = TheMessageStream->appendMessage(GameMessage::MSG_RAW_KEY_UP);
msg->appendIntegerArgument(KEY_RALT);
msg->appendIntegerArgument(key);
msg->appendIntegerArgument(KEY_STATE_UP);
}
}

//-------------------------------------------------------------------------------------------------
void Keyboard::emitModifierKeyUps() const
{
emitRawKeyUpIfDown(m_keyStatus, KEY_LCTRL);
emitRawKeyUpIfDown(m_keyStatus, KEY_RCTRL);
emitRawKeyUpIfDown(m_keyStatus, KEY_LSHIFT);
emitRawKeyUpIfDown(m_keyStatus, KEY_RSHIFT);
emitRawKeyUpIfDown(m_keyStatus, KEY_LALT);
emitRawKeyUpIfDown(m_keyStatus, KEY_RALT);
Comment thread
xezon marked this conversation as resolved.
if (m_shift2Key != KEY_NONE && !isCtrlShiftAltKey(m_shift2Key))
emitRawKeyUpIfDown(m_keyStatus, m_shift2Key);
}

//-------------------------------------------------------------------------------------------------
/** get the first key in our current state of the keyboard */
//-------------------------------------------------------------------------------------------------
Expand Down Expand Up @@ -816,7 +820,7 @@ UnsignedByte Keyboard::getKeyStatusData( KeyDefType key )
//-------------------------------------------------------------------------------------------------
/** Get the key state data as a Bool for the specified key */
//-------------------------------------------------------------------------------------------------
Bool Keyboard::getKeyStateBit( KeyDefType key, Int bit )
Bool Keyboard::getKeyStateBit( KeyDefType key, KeyState bit )
{
return (m_keyStatus[ key ].state & bit) ? 1 : 0;
}
Expand All @@ -832,7 +836,7 @@ void Keyboard::setKeyStatusData( KeyDefType key, KeyboardIO::StatusType data )
//-------------------------------------------------------------------------------------------------
/** set the key state data */
//-------------------------------------------------------------------------------------------------
void Keyboard::setKeyStateData( KeyDefType key, UnsignedByte data )
void Keyboard::setKeyStateData( KeyDefType key, KeyState data )
{
m_keyStatus[ key ].state = data;
}
Expand Down
35 changes: 8 additions & 27 deletions Core/GameEngine/Source/GameClient/MessageStream/HotKey.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@
//-----------------------------------------------------------------------------
#include "GameClient/HotKey.h"
#include "GameClient/KeyDefs.h"
#include "GameClient/MetaEvent.h"
#include "GameClient/GameWindow.h"
#include "GameClient/GameWindowManager.h"
#include "GameClient/Keyboard.h"
Expand All @@ -72,35 +71,17 @@ GameMessageDisposition HotKeyTranslator::translateGameMessage(const GameMessage
GameMessageDisposition disp = KEEP_MESSAGE;
GameMessage::Type t = msg->getType();

if ( t == GameMessage::MSG_RAW_KEY_UP)
// TheSuperHackers @fix Run hotkeys on press so releasing a modifier first cannot trigger a plain hotkey.

@xezon xezon Sep 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Run hotkeys on press instead of release ..."

@xezon xezon Sep 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can also say that the press is consistent with the typical meta event key mappings.

if ( t == GameMessage::MSG_RAW_KEY_DOWN)
Comment thread
greptile-apps[bot] marked this conversation as resolved.
Comment thread
xezon marked this conversation as resolved.
{

//char key = msg->getArgument(0)->integer;
Int keyState = msg->getArgument(1)->integer;

// for our purposes here, we don't care to distinguish between right and left keys,
// so just fudge a little to simplify things.
Int newModState = 0;

if( keyState & KEY_STATE_CONTROL )
{
newModState |= CTRL;
}

if( keyState & KEY_STATE_SHIFT )
{
newModState |= SHIFT;
}

if( keyState & KEY_STATE_ALT )
{
newModState |= ALT;
}
if(newModState != 0)
const KeyDefType key = (KeyDefType)msg->getArgument(0)->integer;
const KeyState keyState = (KeyState)msg->getArgument(1)->integer;
Comment thread
xezon marked this conversation as resolved.
if( keyState & (KEY_STATE_MODIFIERS | KEY_STATE_AUTOREPEAT) )
return disp;
WideChar key = TheKeyboard->getPrintableKey((KeyDefType)msg->getArgument(0)->integer, 0);

WideChar printableKey = TheKeyboard->getPrintableKey(key, 0);
UnicodeString uKey;
uKey.concat(key);
uKey.concat(printableKey);
AsciiString aKey;
aKey.translate(uKey);
if(TheHotKeyManager && TheHotKeyManager->executeHotKey(aKey))
Expand Down
6 changes: 3 additions & 3 deletions Core/GameEngine/Source/GameClient/MessageStream/MetaEvent.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,7 +535,7 @@ void MetaEventTranslator::onMouseEvent(const GameMessage *msg)
void MetaEventTranslator::onKeyEvent(const GameMessage *msg, GameMessageDisposition &disp)
{
const Int systemKey = msg->getArgument(0)->integer;
const Int systemKeyState = msg->getArgument(1)->integer;
const KeyState systemKeyState = (KeyState)msg->getArgument(1)->integer;

const MappableKeyType keyType = getActionKeyType(systemKey);
const MappableKeyModState keyModState = getKeyModState(systemKeyState);
Expand Down Expand Up @@ -598,7 +598,7 @@ void MetaEventTranslator::onKeyModStateRemoved(GameMessageDisposition &disp, Map
}

//-------------------------------------------------------------------------------------------------
void MetaEventTranslator::onKeyPressed(GameMessageDisposition &disp, Int systemKeyState, MappableKeyType keyType, MappableKeyModState keyModState)
void MetaEventTranslator::onKeyPressed(GameMessageDisposition &disp, KeyState systemKeyState, MappableKeyType keyType, MappableKeyModState keyModState)
{
// TheSuperHackers @info The regular key handler only triggers events when the mapped key is pressed,
// not when the modifier (CTRL, ALT, SHIFT) is pressed, unless the key is MK_NONE.
Expand Down Expand Up @@ -701,7 +701,7 @@ MappableKeyType MetaEventTranslator::getActionKeyType(Int systemKey)
}

//-------------------------------------------------------------------------------------------------
MappableKeyModState MetaEventTranslator::getKeyModState(Int systemKeyState)
MappableKeyModState MetaEventTranslator::getKeyModState(KeyState systemKeyState)
{
// for our purposes here, we don't care to distinguish between right and left keys,
// so just fudge a little to simplify things.
Expand Down
5 changes: 4 additions & 1 deletion Generals/Code/GameEngine/Include/GameClient/KeyDefs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,10 +252,13 @@ enum
// modifier combinations when left/right isn't a factor
KEY_STATE_CONTROL = (KEY_STATE_LCONTROL | KEY_STATE_RCONTROL),
KEY_STATE_SHIFT = (KEY_STATE_LSHIFT | KEY_STATE_RSHIFT | KEY_STATE_SHIFT2 ),
KEY_STATE_ALT = (KEY_STATE_LALT | KEY_STATE_RALT)
KEY_STATE_ALT = (KEY_STATE_LALT | KEY_STATE_RALT),
KEY_STATE_MODIFIERS = (KEY_STATE_CONTROL | KEY_STATE_SHIFT | KEY_STATE_ALT)

};

typedef UnsignedShort KeyState;

// INLINING ///////////////////////////////////////////////////////////////////

// EXTERNALS //////////////////////////////////////////////////////////////////
3 changes: 2 additions & 1 deletion Generals/Code/GameEngine/Source/GameClient/GameClient.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,8 +286,9 @@ void GameClient::init()

// since we only allocate one of each, don't bother pooling 'em
m_translators[ m_numTranslators++ ] = TheMessageStream->attachTranslator( MSGNEW("GameClientSubsystem") WindowTranslator, 10 );
// TheSuperHackers @fix Let active GUI hotkeys handle keys before MetaEventTranslator consumes overlapping bindings.

@xezon xezon Sep 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did you come to the conclusion that this is the right thing to do?

If a unit has a button mapped on key "E", then pressing "E" will no longer select matching units for the selection: a deviation of the original, possibly confusing the player.

m_translators[ m_numTranslators++ ] = TheMessageStream->attachTranslator( MSGNEW("GameClientSubsystem") HotKeyTranslator, 15 );
Comment thread
xezon marked this conversation as resolved.

@xezon xezon Sep 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give an example what concrete button conflict this solves now or would solve on a conflict?

m_translators[ m_numTranslators++ ] = TheMessageStream->attachTranslator( MSGNEW("GameClientSubsystem") MetaEventTranslator, 20 );
m_translators[ m_numTranslators++ ] = TheMessageStream->attachTranslator( MSGNEW("GameClientSubsystem") HotKeyTranslator, 25 );
m_translators[ m_numTranslators++ ] = TheMessageStream->attachTranslator( MSGNEW("GameClientSubsystem") PlaceEventTranslator, 30 );
m_translators[ m_numTranslators++ ] = TheMessageStream->attachTranslator( MSGNEW("GameClientSubsystem") GUICommandTranslator, 40 );
m_translators[ m_numTranslators++ ] = TheMessageStream->attachTranslator( MSGNEW("GameClientSubsystem") SelectionTranslator, 50 );
Expand Down
5 changes: 4 additions & 1 deletion GeneralsMD/Code/GameEngine/Include/GameClient/KeyDefs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,10 +252,13 @@ enum
// modifier combinations when left/right isn't a factor
KEY_STATE_CONTROL = (KEY_STATE_LCONTROL | KEY_STATE_RCONTROL),
KEY_STATE_SHIFT = (KEY_STATE_LSHIFT | KEY_STATE_RSHIFT | KEY_STATE_SHIFT2 ),
KEY_STATE_ALT = (KEY_STATE_LALT | KEY_STATE_RALT)
KEY_STATE_ALT = (KEY_STATE_LALT | KEY_STATE_RALT),
KEY_STATE_MODIFIERS = (KEY_STATE_CONTROL | KEY_STATE_SHIFT | KEY_STATE_ALT)

};

typedef UnsignedShort KeyState;

// INLINING ///////////////////////////////////////////////////////////////////

// EXTERNALS //////////////////////////////////////////////////////////////////
3 changes: 2 additions & 1 deletion GeneralsMD/Code/GameEngine/Source/GameClient/GameClient.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,8 +294,9 @@ void GameClient::init()

// since we only allocate one of each, don't bother pooling 'em
m_translators[ m_numTranslators++ ] = TheMessageStream->attachTranslator( MSGNEW("GameClientSubsystem") WindowTranslator, 10 );
// TheSuperHackers @fix Let active GUI hotkeys handle keys before MetaEventTranslator consumes overlapping bindings.
m_translators[ m_numTranslators++ ] = TheMessageStream->attachTranslator( MSGNEW("GameClientSubsystem") HotKeyTranslator, 15 );
m_translators[ m_numTranslators++ ] = TheMessageStream->attachTranslator( MSGNEW("GameClientSubsystem") MetaEventTranslator, 20 );
m_translators[ m_numTranslators++ ] = TheMessageStream->attachTranslator( MSGNEW("GameClientSubsystem") HotKeyTranslator, 25 );
m_translators[ m_numTranslators++ ] = TheMessageStream->attachTranslator( MSGNEW("GameClientSubsystem") PlaceEventTranslator, 30 );
m_translators[ m_numTranslators++ ] = TheMessageStream->attachTranslator( MSGNEW("GameClientSubsystem") GUICommandTranslator, 40 );
m_translators[ m_numTranslators++ ] = TheMessageStream->attachTranslator( MSGNEW("GameClientSubsystem") SelectionTranslator, 50 );
Expand Down
Loading