Hotkey support for MMapper client #421
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideIntroduces a centralized HotkeyManager with CLI and config-editor integrations, refactors client input handling to route key events through the hotkey system (including command separators and PageUp/PageDown scrolling), adds an auto-start client option, and includes WASM build/docs plus test wiring for the new functionality. Sequence diagram for keypress handling with HotkeyManagersequenceDiagram
actor User
participant QtEventLoop
participant InputWidget
participant HotkeyManager
participant StackedInputWidgetOutputs
participant ClientWidgetDisplay as ClientDisplay
User->>QtEventLoop: Press key (e.g. CTRL+NUMPAD8)
QtEventLoop->>InputWidget: QEvent ShortcutOverride
InputWidget->>InputWidget: classifyKey(key, modifiers)
alt classification.shouldHandle and has modifiers
alt FunctionKey/NumpadKey/NavigationKey/ArrowKey/MiscKey/PageKey
InputWidget->>HotkeyManager: getCommandQString(key, realModifiers, isNumpad)
alt command found
HotkeyManager-->>InputWidget: command QString
InputWidget->>InputWidget: sendCommandWithSeparator(command)
InputWidget->>StackedInputWidgetOutputs: sendUserInput(cmd parts)
StackedInputWidgetOutputs-->>ClientWidgetDisplay: virt_sendUserInput
InputWidget->>InputWidget: m_handledInShortcutOverride = true
InputWidget-->>QtEventLoop: accept ShortcutOverride (handled)
else no command
HotkeyManager-->>InputWidget: empty QString
InputWidget-->>QtEventLoop: accept ShortcutOverride (let KeyPress through)
end
else TerminalShortcut/BasicKey
InputWidget-->>QtEventLoop: accept ShortcutOverride (handled in KeyPress)
end
else not handled
InputWidget-->>QtEventLoop: ignore ShortcutOverride
end
QtEventLoop->>InputWidget: QEvent KeyPress
alt m_handledInShortcutOverride is true
InputWidget->>InputWidget: reset m_handledInShortcutOverride
InputWidget-->>QtEventLoop: accept KeyPress (no further work)
else not prehandled
InputWidget->>InputWidget: classifyKey(key, modifiers)
alt FunctionKey/NumpadKey/NavigationKey/ArrowKey/MiscKey/PageKey/TerminalShortcut/BasicKey
InputWidget->>HotkeyManager: getCommandQString(...) (where applicable)
alt command found
HotkeyManager-->>InputWidget: command QString
InputWidget->>InputWidget: sendCommandWithSeparator(command)
InputWidget->>StackedInputWidgetOutputs: sendUserInput(cmd parts)
else no command
alt ArrowKey without modifiers
InputWidget->>InputWidget: backwardHistory/forwardHistory
else PageKey without modifiers
InputWidget->>StackedInputWidgetOutputs: scrollDisplay(pageUp)
else TerminalShortcut
InputWidget->>InputWidget: handleTerminalShortcut(delete, clear line, delete word)
else BasicKey (Enter/Tab)
InputWidget->>InputWidget: gotInput() or tryHistory(Tab)
else
InputWidget-->>QtEventLoop: let base handler move cursor
end
end
InputWidget-->>QtEventLoop: accept KeyPress
else Other
InputWidget->>InputWidget: base::keyPressEvent
end
end
Sequence diagram for _config edit and remote client configuration editorsequenceDiagram
actor User
participant MudClient as MUD
participant AbstractParser
participant Proxy
participant AbstractParserOutputs as ParserOutputs
participant RemoteEditWidget
participant HotkeyManager
participant QSettings
User->>MudClient: _config edit
MudClient->>AbstractParser: command line "_config edit"
AbstractParser->>AbstractParser: doConfig("edit ...")
AbstractParser->>AbstractParser: eval("config", configSyntax, cmd)
AbstractParser->>ParserOutputs: onOpenClientConfigEditor()
ParserOutputs->>Proxy: virt_onOpenClientConfigEditor()
Proxy->>HotkeyManager: exportToCliFormat()
HotkeyManager->>QSettings: read SETTINGS_RAW_CONTENT_KEY
QSettings-->>HotkeyManager: rawContent (or defaults)
HotkeyManager-->>Proxy: CLI formatted content
Proxy->>RemoteEditWidget: create(editSession=true, title=MMapper_Client_Configuration, content=rawContent)
Proxy->>RemoteEditWidget: show() and activateWindow()
User->>RemoteEditWidget: edit hotkey lines
User->>RemoteEditWidget: save
RemoteEditWidget-->>Proxy: sig_save(editedContent)
Proxy->>HotkeyManager: importFromCliFormat(editedContent)
HotkeyManager->>HotkeyManager: parseRawContent()
HotkeyManager->>QSettings: set SETTINGS_RAW_CONTENT_KEY
HotkeyManager->>QSettings: remove legacy IntegratedClient/Hotkeys group
HotkeyManager-->>Proxy: hotkeyCount
Proxy->>MudClient: onSendToUser("N hotkeys imported.")
Class diagram for HotkeyManager and client input integrationclassDiagram
class HotkeyKey {
+int key
+Qt::KeyboardModifiers modifiers
+bool isNumpad
+HotkeyKey()
+HotkeyKey(int k, Qt::KeyboardModifiers m, bool numpad)
+bool operator==(HotkeyKey other)
}
class HotkeyKeyHash {
+size_t operator()(HotkeyKey k)
}
class HotkeyManager {
-unordered_map~HotkeyKey, string, HotkeyKeyHash~ m_hotkeys
-vector~pair~QString, string~~ m_orderedHotkeys
-QString m_rawContent
+HotkeyManager()
+void loadFromSettings()
+void saveToSettings() const
+void setHotkey(QString keyName, QString command)
+void removeHotkey(QString keyName)
+string getCommand(int key, Qt::KeyboardModifiers modifiers, bool isNumpad) const
+string getCommand(QString keyName) const
+QString getCommandQString(int key, Qt::KeyboardModifiers modifiers, bool isNumpad) const
+QString getCommandQString(QString keyName) const
+bool hasHotkey(QString keyName) const
+const vector~pair~QString, string~~& getAllHotkeys() const
+void resetToDefaults()
+void clear()
+vector~QString~ getAllKeyNames() const
+QString exportToCliFormat() const
+int importFromCliFormat(QString content)
+static vector~QString~ getAvailableKeyNames()
+static vector~QString~ getAvailableModifiers()
-void parseRawContent()
-static QString normalizeKeyString(QString keyString)
-static bool isValidBaseKey(QString baseKey)
-static HotkeyKey stringToHotkeyKey(QString keyString)
-static QString hotkeyKeyToString(HotkeyKey hk)
-static int baseKeyNameToQtKey(QString keyName)
-static QString qtKeyToBaseKeyName(int qtKey)
}
class Configuration {
+IntegratedMudClientSettings integratedClient
+HotkeyManager hotkeyManager
+void read(QSettings conf)
+void write(QSettings conf) const
}
class IntegratedMudClientSettings {
+QString font
+QColor foregroundColor
+QColor backgroundColor
+int columns
+int rows
+int linesOfScrollback
+int linesOfPeekPreview
+bool audibleBell
+bool visualBell
+bool autoStartClient
+bool useCommandSeparator
+QString commandSeparator
+void read(QSettings conf)
+void write(QSettings conf) const
}
class InputWidgetOutputs {
+void sendUserInput(QString msg)
+void displayMessage(QString msg)
+void showMessage(QString msg, int timeout)
+void gotPasswordInput(QString password)
+void scrollDisplay(bool pageUp)
..virtual..
-void virt_sendUserInput(QString msg) = 0
-void virt_displayMessage(QString msg) = 0
-void virt_showMessage(QString msg, int timeout) = 0
-void virt_gotPasswordInput(QString password) = 0
-void virt_scrollDisplay(bool pageUp) = 0
}
class KeyClassification {
+KeyType type
+QString keyName
+Qt::KeyboardModifiers realModifiers
+bool shouldHandle
}
class InputWidget {
-InputWidgetOutputs& m_outputs
-InputHistory m_inputHistory
-PaletteManager m_paletteManager
-bool m_tabbing
-bool m_handledInShortcutOverride
+InputWidget(QWidget parent, InputWidgetOutputs outputs)
+~InputWidget()
+void keyPressEvent(QKeyEvent event)
+bool event(QEvent event)
-void gotInput()
-bool tryHistory(int key)
-bool numpadKeyPressed(int key, Qt::KeyboardModifiers modifiers)
-bool navigationKeyPressed(int key, Qt::KeyboardModifiers modifiers)
-bool arrowKeyPressed(int key, Qt::KeyboardModifiers modifiers)
-bool miscKeyPressed(int key, Qt::KeyboardModifiers modifiers)
-void functionKeyPressed(int key, Qt::KeyboardModifiers modifiers)
-QString buildHotkeyString(QString keyName, Qt::KeyboardModifiers modifiers)
-bool handleTerminalShortcut(int key)
-bool handleBasicKey(int key)
-bool handlePageKey(int key, Qt::KeyboardModifiers modifiers)
-void tabComplete()
-void sendUserInput(QString msg)
-void sendCommandWithSeparator(QString command)
}
class StackedInputWidgetOutputs {
+void sendUserInput(QString msg)
+void displayMessage(QString msg)
+void showMessage(QString msg, int timeout)
+void requestPassword()
+void scrollDisplay(bool pageUp)
..virtual..
-void virt_sendUserInput(QString msg) = 0
-void virt_displayMessage(QString msg) = 0
-void virt_showMessage(QString msg, int timeout) = 0
-void virt_requestPassword() = 0
-void virt_scrollDisplay(bool pageUp) = 0
}
class ClientWidget {
+bool isUsingClient() const
+void displayReconnectHint()
+void playMume()
-void relayMessage(QString msg)
}
class AbstractParserOutputs {
+void onSetMode(MapModeEnum mode)
+void onOpenClientConfigEditor()
..virtual..
-void virt_onSendToMud(QString msg) = 0
-void virt_onSendToUser(QString msg, bool echo) = 0
-void virt_onSetMode(MapModeEnum mode) = 0
-void virt_onOpenClientConfigEditor() = 0
}
class AbstractParser {
+using HelpCallback
+void parseHelp(StringView words)
+void parseHotkey(StringView input)
+void doConfig(StringView cmd)
-void openClientConfigEditor()
-void initSpecialCommandMap()
}
class Proxy {
+void allocParser()
..nested outputs impl..
+void virt_onOpenClientConfigEditor()
}
class RemoteEditWidget {
+RemoteEditWidget(bool editSession, QString title, QString content, QWidget parent)
+signal sig_save(QString edited)
+void show()
+void activateWindow()
}
class MainWindow {
+void readSettings()
+void writeSettings()
-ClientWidget* m_clientWidget
-QDockWidget* m_dockDialogClient
}
Configuration o-- IntegratedMudClientSettings
Configuration o-- HotkeyManager
InputWidget --> InputWidgetOutputs : uses
StackedInputWidgetOutputs --> InputWidgetOutputs : forwards scrollDisplay
ClientWidget --> StackedInputWidgetOutputs : implements virt_scrollDisplay
ClientWidget --> InputWidget : manages
InputWidget --> Configuration : getConfig().hotkeyManager
HotkeyManager --> QSettings : loadFromSettings/write
AbstractParser --> AbstractParserOutputs : uses
AbstractParser --> Configuration : setConfig().hotkeyManager
AbstractParser --> HotkeyManager : parseHotkey operations
Proxy --> AbstractParserOutputs : implements virt_onOpenClientConfigEditor
Proxy --> RemoteEditWidget : creates
Proxy --> HotkeyManager : exportToCliFormat/importFromCliFormat
MainWindow --> Configuration : getConfig().integratedClient
MainWindow --> ClientWidget : uses auto-start
ClientWidget --> Telnet : connectToHost(m_listener)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- HotkeyManager currently normalizes and accepts any last token as a base key (just uppercasing it); consider validating the base key against the known set of key names so typos in configuration (e.g.
F13,NUMPDA8) are rejected rather than silently stored. - HotkeyManager uses the default-constructed QSettings both in production and in the new unit tests, which means tests will read/write the real application settings; consider injecting a QSettings instance or scoping it to a different organization/application name to avoid polluting user config during testing.
- The new build-wasm.sh and server.py scripts are added at the repository root with hardcoded assumptions (e.g. emsdk path, fixed port); if these are meant for developer convenience rather than end users, consider moving them under a dedicated tools/ or scripts/ directory and/or making the paths/port configurable to reduce accidental misuse.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- HotkeyManager currently normalizes and accepts any last token as a base key (just uppercasing it); consider validating the base key against the known set of key names so typos in configuration (e.g. `F13`, `NUMPDA8`) are rejected rather than silently stored.
- HotkeyManager uses the default-constructed QSettings both in production and in the new unit tests, which means tests will read/write the real application settings; consider injecting a QSettings instance or scoping it to a different organization/application name to avoid polluting user config during testing.
- The new build-wasm.sh and server.py scripts are added at the repository root with hardcoded assumptions (e.g. emsdk path, fixed port); if these are meant for developer convenience rather than end users, consider moving them under a dedicated tools/ or scripts/ directory and/or making the paths/port configurable to reduce accidental misuse.
## Individual Comments
### Comment 1
<location> `src/preferences/clientpage.cpp:29-38` </location>
<code_context>
+ {}
~CustomSeparatorValidator() final;
void fixup(QString &input) const override
{
- mmqt::toLatin1InPlace(input); // transliterates non-latin1 codepoints
+ // Remove any non-printable or whitespace characters
+ QString cleaned;
+ for (const QChar &c : input) {
+ if (c.isPrint() && !c.isSpace()) {
+ cleaned.append(c);
+ }
+ }
+ input = cleaned;
}
</code_context>
<issue_to_address>
**issue (bug_risk):** CustomSeparatorValidator::fixup doesn’t strip backslashes even though validate rejects them.
Because `validate()` rejects backslashes, `fixup()` should strip them as well so it doesn’t leave the line edit showing values that can never validate. Consider updating `fixup()` to skip `\` explicitly:
```cpp
for (const QChar &c : input) {
if (c == '\\') {
continue;
}
if (c.isPrint() && !c.isSpace()) {
cleaned.append(c);
}
}
```
so that `fixup()` only produces values `validate()` will accept.
</issue_to_address>
### Comment 2
<location> `src/client/inputwidget.cpp:589-591` </location>
<code_context>
}
+// Process _hotkey commands and return true if handled
+static bool processHotkeyCommand(const QString &input, InputWidgetOutputs &outputs)
+{
+ if (!input.startsWith("_hotkey")) {
+ return false;
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** _hotkey/_config command detection is too loose and may trigger on unrelated input.
Currently these functions only check `input.startsWith("_hotkey")` / `"_config"`, so inputs like `_hotkeyX` or `_configFoo` will also be treated as control commands and never reach the MUD.
To avoid intercepting regular commands, restrict matches to the bare keyword or keyword + space, e.g.:
```cpp
if (!(input == "_hotkey" || input.startsWith("_hotkey "))) {
return false;
}
```
(and similarly for `_config`).
Suggested implementation:
```cpp
if (!(input == "_hotkey" || input.startsWith("_hotkey "))) {
return false;
}
```
You should make the same tightening change in the `_config` handling function (or wherever `_config` is parsed), replacing a loose `input.startsWith("_config")` check with:
```cpp
if (!(input == "_config" || input.startsWith("_config "))) {
return false;
}
```
This will ensure both `_hotkey` and `_config` are only intercepted when used as standalone commands or followed by a space, and other inputs like `_hotkeyX` or `_configFoo` are passed through to the MUD.
</issue_to_address>
### Comment 3
<location> `tests/TestHotkeyManager.cpp:139-148` </location>
<code_context>
+void TestHotkeyManager::resetToDefaultsTest()
</code_context>
<issue_to_address>
**suggestion (testing):** Avoid asserting on the exact number of default hotkeys to reduce test brittleness
The test currently asserts `getAllHotkeys().size() == 30`, which couples it to the exact contents of `DEFAULT_HOTKEYS_CONTENT`. Any change in the default set will fail the test even if behavior is still correct.
Instead, rely on the specific representative defaults you already assert (NUMPAD8, CTRL+NUMPAD8, etc.), and optionally assert that the set is non‑empty. This keeps the test validating that defaults are loaded and contain key entries without blocking legitimate changes to the default set size.
Suggested implementation:
```cpp
void TestHotkeyManager::resetToDefaultsTest()
{
HotkeyManager manager;
// Import custom hotkeys
manager.importFromCliFormat("_hotkey F1 custom\n_hotkey F2 another");
QCOMPARE(manager.getCommand("F1"), QString("custom"));
QCOMPARE(manager.getAllHotkeys().size(), 2);
// Reset to defaults
manager.resetToDefaults();
// Ensure defaults are loaded without asserting on the exact count
QVERIFY(!manager.getAllHotkeys().isEmpty());
```
```cpp
// Reset to defaults
manager.resetToDefaults();
```
If the assertion on the exact size uses a different value or appears with slightly different spacing (e.g., `QCOMPARE(manager.getAllHotkeys().size(), 30 );`), adjust the second SEARCH block accordingly to remove that assertion entirely. Keep the existing checks for specific default hotkeys (e.g., `NUMPAD8`, `CTRL+NUMPAD8`) as they are; they already validate representative defaults without depending on the full set size.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #421 +/- ##
==========================================
+ Coverage 66.48% 67.69% +1.21%
==========================================
Files 85 89 +4
Lines 4186 4597 +411
Branches 255 322 +67
==========================================
+ Hits 2783 3112 +329
- Misses 1403 1485 +82 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1. Added #include "../global/TextUtils.h" for the mmqt:: functions 2. Replaced QString::fromStdString() → mmqt::toQStringUtf8() 3. Replaced QString::toStdString() → mmqt::toStdStringUtf8()
Contributor
Author
|
Addressed the review feedback:
|
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- The
scripts/build-wasm.shscript currently setsMMAPPER_SRCto thescripts/directory (dirname "$0"), which breaks all the subsequent paths (Qt, build directory, etc.); this should instead resolve to the project root (e.g.,MMAPPER_SRC="$(cd "$(dirname "$0")/.." && pwd)"). - In
scripts/build-wasm.shthe final usage hint printspython3 ../../server.py, but the server script is committed asscripts/server.py(and your docs use../../scripts/server.py), so the echoed command should be corrected to match the actual path.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `scripts/build-wasm.sh` script currently sets `MMAPPER_SRC` to the `scripts/` directory (`dirname "$0"`), which breaks all the subsequent paths (Qt, build directory, etc.); this should instead resolve to the project root (e.g., `MMAPPER_SRC="$(cd "$(dirname "$0")/.." && pwd)"`).
- In `scripts/build-wasm.sh` the final usage hint prints `python3 ../../server.py`, but the server script is committed as `scripts/server.py` (and your docs use `../../scripts/server.py`), so the echoed command should be corrected to match the actual path.
## Individual Comments
### Comment 1
<location> `src/preferences/clientpage.cpp:125-128` </location>
<code_context>
ui->commandSeparatorLineEdit->setEnabled(isChecked);
});
connect(ui->commandSeparatorLineEdit, &QLineEdit::textChanged, this, [](const QString &text) {
- if (text.length() == 1) {
+ if (!text.isEmpty()) {
setConfig().integratedClient.commandSeparator = text;
}
</code_context>
<issue_to_address>
**suggestion:** Clearing the command separator in the UI leaves the old separator in config, which may be confusing.
Because `textChanged` only updates `integratedClient.commandSeparator` when `!text.isEmpty()`, clearing the field leaves the previous non-empty value in config while the UI shows an empty separator. As soon as the user types a new non-empty string, `sendCommandWithSeparator` will still use the old separator until the next update. If clearing the field is meant to clear the stored separator, also write an empty string when `text.isEmpty()` and rely on `useCommandSeparator` to decide whether it’s applied.
```suggestion
connect(ui->commandSeparatorCheckBox, &QCheckBox::toggled, this, [this](bool isChecked) {
setConfig().integratedClient.useCommandSeparator = isChecked;
ui->commandSeparatorLineEdit->setEnabled(isChecked);
});
connect(ui->commandSeparatorLineEdit, &QLineEdit::textChanged, this, [](const QString &text) {
// Keep config in sync with the UI, including when the separator is cleared.
setConfig().integratedClient.commandSeparator = text;
});
```
</issue_to_address>
### Comment 2
<location> `src/configuration/HotkeyManager.cpp:250-259` </location>
<code_context>
+QString HotkeyManager::normalizeKeyString(const QString &keyString)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Normalization silently drops unknown modifiers and invalid base keys without feedback.
In `normalizeKeyString`, unrecognized modifiers are ignored and an invalid base key returns an empty string. For user-editable sources (`_config edit` / import), this means typos like `CTL+F1` or bases like `NUMPAD11` are silently dropped and the hotkey is effectively ignored. Consider surfacing these cases (e.g., special marker return value, warning log, or CLI feedback via `_hotkey config` / debug output) rather than failing silently.
Suggested implementation:
```cpp
QString HotkeyManager::normalizeKeyString(const QString &keyString)
{
// Split by '+' to get individual parts
QStringList parts = keyString.split('+', Qt::SkipEmptyParts);
if (parts.isEmpty()) {
qWarning() << "HotkeyManager::normalizeKeyString: empty or invalid key string, ignoring hotkey:" << keyString;
return QString();
}
// The last part is always the base key (e.g., F1, F2)
QString baseKey = parts.last();
```
To fully implement your suggestion about surfacing unknown modifiers and invalid base keys (beyond the empty/invalid input case above), the following changes are needed in the parts of `normalizeKeyString` that are not visible here:
1. **Unknown modifiers:**
- Wherever the code currently ignores or skips an unrecognized modifier token (likely in a loop over `parts` before/after `parts.removeLast()`), add a warning log before it is discarded:
```cpp
qWarning() << "HotkeyManager::normalizeKeyString: unrecognized modifier in hotkey" << keyString << "modifier:" << modifierToken;
```
- Keep the existing behavior of not including that modifier in the normalized result so you don't change semantics, but you now surface the issue.
2. **Invalid base key:**
- Wherever the function decides that `baseKey` is invalid and returns `QString()` (or otherwise bails out), add a similar warning:
```cpp
qWarning() << "HotkeyManager::normalizeKeyString: invalid base key in hotkey" << keyString << "base:" << baseKey;
return QString();
```
- This ensures that `_config edit` / import typos like `CTL+F1` or `NUMPAD11` produce a visible warning instead of failing silently.
3. **Optional special marker (if desired):**
- If you want a machine-detectable marker instead of an empty string, you can change the invalid-return sites to something like:
```cpp
static const QString kInvalidHotkeyMarker = QStringLiteral("__INVALID_HOTKEY__");
return kInvalidHotkeyMarker;
```
- Then update callers of `normalizeKeyString` (e.g., hotkey config / debug output paths) to check for this marker and surface a user-facing message. This is more invasive and should be done consistently across all call sites.
The added `qWarning()` calls use Qt’s built-in logging and do not require additional includes in typical Qt projects, but if your file does not already include `<QDebug>` or equivalent, you may need to add it at the top of `HotkeyManager.cpp`.
</issue_to_address>
### Comment 3
<location> `scripts/build-wasm.sh:9-11` </location>
<code_context>
+source "$HOME/dev/emsdk/emsdk_env.sh"
+
+# Paths - automatically detect script location
+MMAPPER_SRC="$(cd "$(dirname "$0")" && pwd)"
+QT_WASM="$MMAPPER_SRC/6.5.3/wasm_multithread"
+QT_HOST="$MMAPPER_SRC/6.5.3/macos"
+
+# Configure with qt-cmake
</code_context>
<issue_to_address>
**issue (bug_risk):** Script computes MMAPPER_SRC as the scripts directory, which likely breaks relative Qt and build paths.
Because `MMAPPER_SRC` is `dirname "$0"`, it ends up as the `scripts/` directory but is later treated as the project root (e.g. `-S "$MMAPPER_SRC"`, `$MMAPPER_SRC/6.5.3/wasm_multithread`). In a typical checkout this will cause the Qt and source paths to be wrong.
Consider resolving the project root instead, for example:
```bash
ROOT_DIR="$(cd "$(dirname "$0")/.." && pwd)"
QT_WASM="$ROOT_DIR/6.5.3/wasm_multithread"
QT_HOST="$ROOT_DIR/6.5.3/macos"
"$QT_WASM/bin/qt-cmake" -S "$ROOT_DIR" -B "$ROOT_DIR/build-wasm" ...
```
so the paths match the repo layout while keeping the script relocatable.
</issue_to_address>
### Comment 4
<location> `tests/TestHotkeyManager.cpp:55` </location>
<code_context>
+ QCOMPARE(manager.getCommand("NUMPAD8"), QString("north"));
+}
+
+void TestHotkeyManager::importExportRoundTripTest()
+{
+ HotkeyManager manager;
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests that cover QSettings persistence (loadFromSettings/saveToSettings), including default and legacy formats.
Right now we only verify in-memory behavior; we don’t confirm that hotkeys survive a full QSettings round trip or that legacy `IntegratedClient/Hotkeys` data migrates correctly.
Please add a test that, for example:
- Starts with a clean QSettings backend.
- Writes legacy keys under `IntegratedClient/Hotkeys` and checks that `loadFromSettings()` migrates them into `m_rawContent` and `m_hotkeys` as expected.
- Calls `saveToSettings()`, then constructs a fresh HotkeyManager and verifies it reads back the same configuration (including defaults/comments where relevant).
This will ensure the persistence and migration behavior in this PR is actually exercised and protected against regressions.
Suggested implementation:
```cpp
// Test simple key without modifiers
manager.setHotkey("f7", "test7");
QCOMPARE(manager.getCommand("F7"), QString("test7"));
// Test numpad keys
manager.setHotkey("numpad8", "north");
QCOMPARE(manager.getCommand("NUMPAD8"), QString("north"));
}
void TestHotkeyManager::importExportRoundTripTest()
{
// Use a dedicated QSettings scope for this test and start from a clean backend
QSettings settings(QSettings::IniFormat,
QSettings::UserScope,
"TestHotkeyManagerCompany",
"TestHotkeyManagerApp");
settings.clear();
settings.sync();
// Simulate legacy IntegratedClient/Hotkeys format
settings.beginGroup("IntegratedClient");
settings.beginGroup("Hotkeys");
// NOTE: assuming legacy format maps command -> key sequence
settings.setValue("north", "NUMPAD8");
settings.setValue("attack", "CTRL+F1");
settings.endGroup(); // Hotkeys
settings.endGroup(); // IntegratedClient
settings.sync();
// First load should migrate legacy data into the manager
HotkeyManager manager;
manager.loadFromSettings(settings);
QCOMPARE(manager.getCommand("NUMPAD8"), QString("north"));
QCOMPARE(manager.getCommand("CTRL+F1"), QString("attack"));
// Save back in the new format
manager.saveToSettings(settings);
settings.sync();
// Verify a fresh manager can read the persisted configuration
HotkeyManager reloadedManager;
reloadedManager.loadFromSettings(settings);
QCOMPARE(reloadedManager.getCommand("NUMPAD8"), QString("north"));
QCOMPARE(reloadedManager.getCommand("CTRL+F1"), QString("attack"));
}
// Test that modifiers are normalized to canonical order: CTRL+SHIFT+ALT+META
```
Because only part of the file and class interface is visible, you may need to:
1. **Ensure the method is declared** in the corresponding header (e.g. `TestHotkeyManager.h`), matching existing test method declarations:
- Add `void importExportRoundTripTest();` to the `TestHotkeyManager` class.
- If you use `QTest`’s auto-discovery, add `private slots:` or equivalent as per existing tests.
2. **Verify `HotkeyManager`'s API**:
- If `loadFromSettings` / `saveToSettings` are static or have different signatures (e.g. parameterless, or take no `QSettings`), adjust the calls accordingly:
- e.g. `manager.loadFromSettings();` / `manager.saveToSettings();` and construct `QSettings` in the same way `HotkeyManager` does, so the test and implementation share the same backend.
3. **Confirm the legacy key format**:
- If the legacy format is `keySequence -> command` instead of `command -> keySequence`, swap the arguments in the `setValue` calls and adjust the expectations.
4. **Includes**:
- If `QSettings` is not yet included in `TestHotkeyManager.cpp`, add `#include <QSettings>` near the other Qt includes at the top of the file.
</issue_to_address>
### Comment 5
<location> `tests/TestHotkeyManager.cpp:195` </location>
<code_context>
+ QVERIFY(posF4 < posCtrlF5);
+}
+
+void TestHotkeyManager::setHotkeyTest()
+{
+ HotkeyManager manager;
</code_context>
<issue_to_address>
**question (testing):** Clarify and test behavior when the same key is defined multiple times in imported content.
`parseRawContent()` currently appends every `_hotkey` line to `m_orderedHotkeys` while overwriting the key in `m_hotkeys`, so duplicates:
- Show up multiple times in `getAllHotkeys()` (and `_hotkey config`), and
- Use the last definition at runtime via `getCommand()`.
Please add a test that exercises this with something like:
```ini
_hotkey F1 first
_hotkey F1 second
```
and asserts that `getCommand("F1") == "second"` and that `getAllHotkeys()` either preserves both entries in order or collapses duplicates, depending on the intended behavior. This will lock in the expected semantics for duplicates.
</issue_to_address>
### Comment 6
<location> `tests/TestHotkeyManager.cpp:110` </location>
<code_context>
+ QCOMPARE(manager.getCommand("F12"), QString("stand"));
+}
+
+void TestHotkeyManager::importEdgeCasesTest()
+{
+ HotkeyManager manager;
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test that explicitly verifies preservation of comments and formatting across import/export.
The tests currently only validate parsing and the presence of `_hotkey` lines in `exportToCliFormat()`, but not that comments/formatting survive a round trip.
Please extend `importEdgeCasesTest` (or add a new one) to:
- Build a multi-line CLI string with comments, blank lines, and unusual indentation.
- Run it through `importFromCliFormat()` and then `exportToCliFormat()`.
- Assert that the result matches the original (or at least that all comment lines and their relative positions are preserved).
This will guard against refactors that might drop or reorder non-command lines.
Suggested implementation:
```cpp
manager.importFromCliFormat("_hotkey F12 stand\n");
QCOMPARE(manager.getAllHotkeys().size(), 1);
QCOMPARE(manager.getCommand("F10"), QString()); // Should be cleared
QCOMPARE(manager.getCommand("F12"), QString("stand"));
}
void TestHotkeyManager::importEdgeCasesTest()
{
HotkeyManager manager;
const QString cliFormat =
"# Leading comment before any hotkeys\n"
"\n"
" # Indented comment\n"
"_hotkey F1 open\n"
"\t# Tab-indented comment after a hotkey\n"
"\n"
"# Trailing comment after all hotkeys\n";
manager.importFromCliFormat(cliFormat);
const QString exported = manager.exportToCliFormat();
auto stripHotkeyLines = [](const QString &text) {
QStringList lines = text.split('\n');
QStringList filtered;
filtered.reserve(lines.size());
for (const QString &line : lines) {
if (line.trimmed().startsWith("_hotkey"))
continue;
filtered << line;
}
return filtered.join('\n');
};
// Non-command lines (comments, blank lines, indentation) should survive a round trip
QCOMPARE(stripHotkeyLines(exported), stripHotkeyLines(cliFormat));
}
```
- Ensure `tests/TestHotkeyManager.cpp` includes `<QStringList>` if it is not already included:
- `#include <QStringList>`
- If the test class declaration (likely in `tests/TestHotkeyManager.h` or similar) explicitly lists test methods, add `void importEdgeCasesTest();` and register it with the Qt test framework (e.g., using `QTEST_MAIN`/`QTEST_APPLESS_MAIN` macros or `QTEST_ADD_GPULESS_TEST` depending on your existing pattern).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
nschimme
reviewed
Dec 17, 2025
nschimme
reviewed
Dec 17, 2025
nschimme
reviewed
Dec 17, 2025
HotkeyManager.h:
- Added HotkeyKey struct with key, modifiers, and isNumpad fields for efficient lookup
- Added qHash() function for HotkeyKey to use in QHash
- Changed internal storage from QHash<QString, QString> to QHash<HotkeyKey, QString>
- Added new getCommand(int key, Qt::KeyboardModifiers modifiers, bool isNumpad) method for direct lookup
- Added helper methods: stringToHotkeyKey(), hotkeyKeyToString(), baseKeyNameToQtKey(), qtKeyToBaseKeyName()
HotkeyManager.cpp:
- Added Qt::Key to key name mappings and vice versa
- Implemented stringToHotkeyKey() to convert "CTRL+NUMPAD8" → HotkeyKey{Qt::Key_8, ControlModifier, true}
- Updated parseRawContent() to store HotkeyKey instead of normalized strings
- The string-based API (getCommand(QString), setHotkey(), etc.) still works for _hotkey command
inputwidget.cpp:
- Updated functionKeyPressed() to take int key instead of QString keyName
- Updated numpadKeyPressed(), navigationKeyPressed(), arrowKeyPressed(), miscKeyPressed(), handlePageKey() to use direct lookup: getCommand(key, modifiers, isNumpad)
- No more string building and normalization on every key press!
Tests:
- Added directLookupTest() to test the new getCommand(int, modifiers, bool) API
- exportToCliFormat() for opening editor (preserves comments) - importFromCliFormat() for saving (centralized parsing logic)
out of the hot path: - HotkeyManager now stores commands as std::string internally - Added getCommandQString() convenience methods for Qt UI layer - QString→std::string conversion happens at config load time (cold path) - Updated inputwidget.cpp to use getCommandQString() for all 6 callers - Updated AbstractParser-Hotkey.cpp for new std::string return type This improves consistency with the parser layer (which uses std::string) and prepares for potential future Qt independence.
Contributor
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 6 issues, and left some high level feedback:
- Key name↔Qt::Key mapping logic is now duplicated between
InputWidgetandHotkeyManager(and partly in the parser help text); consider centralizing these tables/definitions in a single shared helper to avoid future inconsistencies when adding or renaming keys. - The default hotkey configuration in
DEFAULT_HOTKEYS_CONTENT(e.g., NUMPAD5 used for south/door actions and NUMPAD2 mapped tolead) differs from the previously hard-coded keypad behavior and from the defaults described in the PR description; it would be good to double‑check that these bindings are intentional to avoid surprising existing users.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Key name↔Qt::Key mapping logic is now duplicated between `InputWidget` and `HotkeyManager` (and partly in the parser help text); consider centralizing these tables/definitions in a single shared helper to avoid future inconsistencies when adding or renaming keys.
- The default hotkey configuration in `DEFAULT_HOTKEYS_CONTENT` (e.g., NUMPAD5 used for south/door actions and NUMPAD2 mapped to `lead`) differs from the previously hard-coded keypad behavior and from the defaults described in the PR description; it would be good to double‑check that these bindings are intentional to avoid surprising existing users.
## Individual Comments
### Comment 1
<location> `src/parser/AbstractParser-Hotkey.cpp:45-54` </location>
<code_context>
+ static const auto abb = syntax::abbrevToken;
+
+ // _hotkey set KEY command
+ auto setHotkey = Accept(
+ [](User &user, const Pair *const args) {
+ auto &os = user.getOstream();
+ const auto v = getAnyVectorReversed(args);
+
+ const auto keyName = mmqt::toQStringUtf8(v[1].getString());
+ const std::string cmdStr = concatenate_unquoted(v[2].getVector());
+ const auto command = mmqt::toQStringUtf8(cmdStr);
+
+ setConfig().hotkeyManager.setHotkey(keyName, command);
+ os << "Hotkey set: " << mmqt::toStdStringUtf8(keyName.toUpper()) << " = " << cmdStr
+ << "\n";
+ send_ok(os);
+ },
+ "set hotkey");
</code_context>
<issue_to_address>
**issue (bug_risk):** setHotkey reports success even when the key name is invalid and nothing is stored
`HotkeyManager::setHotkey` silently returns when `normalizeKeyString` finds an invalid base key, but this handler always prints `Hotkey set ...` and calls `send_ok(os)` even if nothing was stored (e.g., `F13`). This yields incorrect user feedback. Either validate the key name before calling `setHotkey`, or have `setHotkey` return a success flag and only report success when it actually stores the hotkey (otherwise return an error).
</issue_to_address>
### Comment 2
<location> `src/configuration/HotkeyManager.cpp:198` </location>
<code_context>
+}
+
+// Static set of valid base key names for validation
+const QSet<QString> &getValidBaseKeys()
+{
+ static const QSet<QString> validKeys{// Function keys
</code_context>
<issue_to_address>
**suggestion:** The list of valid base keys is duplicated in getValidBaseKeys() and getAvailableKeyNames(), increasing drift risk
Defining the base key set both in `getValidBaseKeys()` and `HotkeyManager::getAvailableKeyNames()` introduces duplication and risk of them getting out of sync (e.g. validation vs `hotkey keys` output). Please centralize the list (e.g. define it once and reuse it, or have `getAvailableKeyNames()` derive from `getValidBaseKeys()`).
Suggested implementation:
```cpp
// Static set of valid base key names for validation
const QSet<QString> &getValidBaseKeys()
{
// Centralize the base key list by deriving it from HotkeyManager::getAvailableKeyNames()
// to avoid drift between validation and reported hotkey keys.
static const QSet<QString> validKeys = HotkeyManager::getAvailableKeyNames().toSet();
```
This change assumes:
1. `HotkeyManager::getAvailableKeyNames()` already exists and returns a `QStringList` (or a type with `.toSet()` that yields `QSet<QString>`).
2. `HotkeyManager.cpp` already includes the corresponding header that declares `HotkeyManager::getAvailableKeyNames()`.
If `getAvailableKeyNames()` has a different return type, adjust the construction of `validKeys` accordingly (e.g. use `QSet<QString>::fromList(...)` or manual insertion from the container).
Also remove the now-unused duplicated literal list that followed `"F10",` in the original `getValidBaseKeys()` implementation.
</issue_to_address>
### Comment 3
<location> `tests/TestHotkeyManager.cpp:467-476` </location>
<code_context>
+void TestHotkeyManager::directLookupTest()
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding parser-level tests for the `_hotkey` CLI command interface
These tests exercise HotkeyManager directly, but the user-facing entry point is the `_hotkey` command (set/remove/config/keys/reset) in `AbstractParser-Hotkey.cpp`. Please also add parser tests that:
- Invoke `_hotkey set/remove/config/keys/reset` via the existing parser test harness (like other special-command tests).
- Check that user-visible output matches expectations (success, appropriate errors, and listing format for `config`/`keys`).
- Confirm that these commands correctly update HotkeyManager (e.g., via `getConfig().hotkeyManager.getCommandQString(...)` or equivalent parser-facing checks).
This will ensure the command wiring is covered and less likely to regress.
Suggested implementation:
```cpp
void TestHotkeyManager::directLookupTest()
{
HotkeyManager manager;
// Import hotkeys for testing
manager.importFromCliFormat("_hotkey F1 look\n"
"_hotkey CTRL+F2 flee\n"
"_hotkey NUMPAD8 n\n"
"_hotkey CTRL+NUMPAD5 s\n"
"_hotkey SHIFT+ALT+UP north\n");
// Direct lookups should map to the expected commands
QCOMPARE(manager.getCommandQString("F1"), QString("look"));
QCOMPARE(manager.getCommandQString("CTRL+F2"), QString("flee"));
QCOMPARE(manager.getCommandQString("NUMPAD8"), QString("n"));
QCOMPARE(manager.getCommandQString("CTRL+NUMPAD5"), QString("s"));
QCOMPARE(manager.getCommandQString("SHIFT+ALT+UP"), QString("north"));
// Unknown key should yield an empty / null command QString (parser-facing behavior)
QVERIFY(manager.getCommandQString("F3").isEmpty());
}
// Parser-level tests for `_hotkey` CLI interface
void TestHotkeyManager::parserHotkeyCommandsTest()
{
//
// Arrange: create a parser harness wired to a Config with a HotkeyManager
//
ParserTestHarness harness; // existing test harness type for parser tests
Config &config = harness.getConfig(); // provides access to the HotkeyManager used by the parser
HotkeyManager &manager = config.hotkeyManager;
//
// 1. `_hotkey set` should bind a key to a command and print a success message
//
QString output = harness.runCommand("_hotkey set F1 look\n");
QVERIFY(output.contains("F1")); // user-visible confirmation mentions the key
QVERIFY(output.contains("look")); // and the bound command
QCOMPARE(manager.getCommandQString("F1"), QString("look"));
//
// 2. `_hotkey keys` should list the configured bindings in the expected format
//
output = harness.runCommand("_hotkey keys\n");
// The exact format should match AbstractParser-Hotkey.cpp; we at least assert that:
// - the line starts with `_hotkey`
// - the key and command are present on the same line
QVERIFY(output.contains("_hotkey F1 look"));
//
// 3. `_hotkey config` should show the same configuration (human-readable form)
//
output = harness.runCommand("_hotkey config\n");
QVERIFY(output.contains("F1"));
QVERIFY(output.contains("look"));
//
// 4. `_hotkey remove` should unbind a key and print a success / acknowledgement
//
output = harness.runCommand("_hotkey remove F1\n");
QVERIFY(output.contains("F1"));
QVERIFY(output.contains("removed") || output.contains("unbound"));
QVERIFY(manager.getCommandQString("F1").isEmpty());
//
// 5. `_hotkey reset` should reset all bindings and report success
//
// First set multiple bindings
harness.runCommand("_hotkey set CTRL+F2 flee\n");
harness.runCommand("_hotkey set NUMPAD8 n\n");
QCOMPARE(manager.getCommandQString("CTRL+F2"), QString("flee"));
QCOMPARE(manager.getCommandQString("NUMPAD8"), QString("n"));
// Then reset
output = harness.runCommand("_hotkey reset\n");
QVERIFY(output.contains("reset") || output.contains("defaults"));
// After reset, previously set keys should no longer map to the custom commands
QVERIFY(manager.getCommandQString("CTRL+F2").isEmpty() ||
manager.getCommandQString("CTRL+F2") != QString("flee"));
QVERIFY(manager.getCommandQString("NUMPAD8").isEmpty() ||
manager.getCommandQString("NUMPAD8") != QString("n"));
//
// 6. Error handling: invalid invocations should produce user-facing errors
//
output = harness.runCommand("_hotkey set\n"); // missing args
QVERIFY(output.contains("error") || output.contains("usage"));
output = harness.runCommand("_hotkey remove\n"); // missing key
QVERIFY(output.contains("error") || output.contains("usage"));
output = harness.runCommand("_hotkey unknownsubcmd\n"); // invalid subcommand
QVERIFY(output.contains("error") || output.contains("unknown"));
}
```
The above parser test uses placeholder types and methods (`ParserTestHarness`, `Config`, `Config::hotkeyManager`, and `ParserTestHarness::runCommand`/`getConfig`) that must be aligned with your existing parser test harness:
1. **Reuse existing parser harness**
- Replace `ParserTestHarness` with the actual helper used for parser-level tests (e.g. `AbstractParserTestHarness`, `ParserFixture`, or similar) already present elsewhere in your test suite.
- Replace `runCommand(const QString &)` with whatever method your harness uses to feed a line to the parser and capture its user-visible output (often something like `executeLine`, `run`, or `parseAndExecute`).
2. **Accessing `Config` and `HotkeyManager`**
- Adjust `Config &config = harness.getConfig();` to the real way of accessing the parser’s configuration object (it might be `harness.config()`, `harness.state().config`, etc.).
- Adjust `config.hotkeyManager` to match the actual member name and accessor (`config.hotkeyManager`, `config.hotkeyManager()`, `config.getHotkeyManager()`, etc.).
3. **Assertions for user-visible output format**
- Tighten the `QVERIFY(output.contains(...))` expectations to match the exact text / format produced by `AbstractParser-Hotkey.cpp` (e.g. capitalization, wording like “Hotkey removed”, exact `keys`/`config` output layout), using existing tests for other special commands as a reference.
4. **Test registration**
- Ensure `TestHotkeyManager::parserHotkeyCommandsTest` is declared in the corresponding header (`tests/TestHotkeyManager.h` or equivalent) and that the test class is registered with Qt’s test framework (e.g. via `QTEST_MAIN(TestHotkeyManager)` or your existing macro), so the new test method is executed.
</issue_to_address>
### Comment 4
<location> `tests/TestHotkeyManager.cpp:112` </location>
<code_context>
+ QCOMPARE(manager.getCommandQString("F12"), QString("stand"));
+}
+
+void TestHotkeyManager::importEdgeCasesTest()
+{
+ HotkeyManager manager;
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for command separator behavior when hotkeys trigger multi-command strings
Since this suite only covers HotkeyManager, consider adding tests that cover how hotkey-bound strings are actually executed via `sendCommandWithSeparator` in `InputWidget`. In particular, verify:
- Commands containing the separator (e.g. `"kill orc;;get all corpse"`) produce multiple `sendUserInput` calls.
- Escaped separators (e.g. `"kill orc\;;say hi"`) are handled correctly by the regex and unescaping.
- Multi-character separators and edge cases like leading/trailing separators or empty segments.
These can live in an InputWidget-focused test (or a small harness using `sendCommandWithSeparator` and a fake `InputWidgetOutputs`) to cover the end-to-end behavior of the new hotkey feature.
Suggested implementation:
```cpp
QCOMPARE(manager.getAllHotkeys().size(), 1);
QCOMPARE(manager.getCommandQString("F10"), QString()); // Should be cleared
QCOMPARE(manager.getCommandQString("F12"), QString("stand"));
}
// Verify that hotkey-bound multi-command strings are split and executed correctly
// via InputWidget::sendCommandWithSeparator.
void TestHotkeyManager::sendCommandWithSeparator_multiCommandStrings()
{
// Fake sink for InputWidget outputs; adjust to match the real type/signature.
struct FakeInputWidgetOutputs
{
QStringList sentInputs;
void sendUserInput(const QString &cmd)
{
sentInputs << cmd;
}
// Add any other methods required by InputWidgetOutputs interface here.
};
FakeInputWidgetOutputs outputs;
// Construct an InputWidget bound to our fake outputs.
// Adjust constructor arguments to match the real InputWidget API.
InputWidget widget(/* parent = */ nullptr, /* outputs = */ &outputs);
// Use a multi-character separator for tests.
const QString separator = ";;";
widget.setCommandSeparator(separator);
// 1. Basic multi-command string with separator
outputs.sentInputs.clear();
widget.sendCommandWithSeparator("kill orc;;get all corpse");
QCOMPARE(outputs.sentInputs, (QStringList() << "kill orc" << "get all corpse"));
// 2. Escaped separator: literal ';' should not split the command
outputs.sentInputs.clear();
widget.sendCommandWithSeparator("kill orc\\;;say hi");
// Expect the first command to contain a literal ';' and the second to be a normal command.
QCOMPARE(outputs.sentInputs, (QStringList() << "kill orc;"";say hi"));
// 3. Leading separator produces an empty segment that should be skipped
outputs.sentInputs.clear();
widget.sendCommandWithSeparator(";;look");
QCOMPARE(outputs.sentInputs, (QStringList() << "look"));
// 4. Trailing separator produces an empty segment that should be skipped
outputs.sentInputs.clear();
widget.sendCommandWithSeparator("look;;");
QCOMPARE(outputs.sentInputs, (QStringList() << "look"));
// 5. Multiple separators in a row should not produce empty commands
outputs.sentInputs.clear();
widget.sendCommandWithSeparator("look;;;;inventory");
QCOMPARE(outputs.sentInputs, (QStringList() << "look" << "inventory"));
// 6. Separator in the middle, with whitespace around commands
outputs.sentInputs.clear();
widget.sendCommandWithSeparator(" kill orc ;; get all corpse ");
QCOMPARE(outputs.sentInputs, (QStringList() << "kill orc" << "get all corpse"));
}
// Test that modifiers are normalized to canonical order: CTRL+SHIFT+ALT+META
```
1. Ensure `tests/TestHotkeyManager.cpp` includes the appropriate headers:
- Add `#include "InputWidget.h"` (or the correct header where `InputWidget` and `InputWidgetOutputs` are declared).
- Add `#include <QStringList>` if not already present.
2. Replace `FakeInputWidgetOutputs` with a concrete mock or adapt it to implement the real `InputWidgetOutputs` (or equivalent) interface:
- Ensure method names and signatures (e.g., `sendUserInput(const QString &)` or similar) match what `InputWidget::sendCommandWithSeparator` actually calls.
3. Adjust the `InputWidget` constructor call:
- Update argument list (`parent`, `outputs`, and any additional parameters) to match the existing `InputWidget` constructor signature.
4. Confirm the separator configuration API:
- If the separator is configured differently (e.g., setter name, global setting, or constructor parameter), replace `widget.setCommandSeparator(separator);` with the appropriate mechanism.
5. Validate the escaping semantics:
- If the actual implementation uses a different escape scheme or unescaping logic, adjust the expectations, especially for the escaped separator case:
- The intended behavior is that `"kill orc\;;say hi"` yields `"kill orc;"` and `"say hi"` as separate commands; if the concrete unescaping is different, tweak the expected `QStringList` accordingly.
</issue_to_address>
### Comment 5
<location> `tests/TestHotkeyManager.cpp:307` </location>
<code_context>
+ QVERIFY(manager.hasHotkey("CTRL+F2")); // Other key still exists
+}
+
+void TestHotkeyManager::invalidKeyValidationTest()
+{
+ HotkeyManager manager;
</code_context>
<issue_to_address>
**suggestion (testing):** Consider tests for the command separator validator, especially multi-character and invalid inputs
With the new `CustomSeparatorValidator` and support for multi-character separators, please add focused tests for:
- Accepting valid multi-character, printable, non-whitespace, non-`'\'` separators (e.g. `";;"`, `"::"`, `"@@"`).
- Rejecting inputs with spaces, backslashes, control characters, or mixed valid/invalid characters.
- `fixup` behavior: cleaning inputs by removing invalid characters, and turning fully invalid strings into empty/`Intermediate`.
This keeps the separator configuration UI aligned with the expectations of the splitting logic.
Suggested implementation:
```cpp
QVERIFY(manager.hasHotkey("CTRL+F2")); // Other key still exists
}
void TestHotkeyManager::customSeparatorValidatorTest()
{
CustomSeparatorValidator validator;
auto validateInput = [&](const QString &input, QValidator::State *stateOut = nullptr) {
QString s = input;
int pos = s.size();
QValidator::State sState = validator.validate(s, pos);
if (stateOut) {
*stateOut = sState;
}
return s;
};
// Accept valid multi-character, printable, non-whitespace, non-'\' separators
{
QValidator::State state;
validateInput(";;", &state);
QCOMPARE(state, QValidator::Acceptable);
validateInput("::", &state);
QCOMPARE(state, QValidator::Acceptable);
validateInput("@@", &state);
QCOMPARE(state, QValidator::Acceptable);
}
// Reject inputs with spaces, backslashes, control characters, or mixed valid/invalid characters
{
QValidator::State state;
validateInput(" ", &state);
QCOMPARE(state, QValidator::Invalid);
validateInput("\t", &state);
QCOMPARE(state, QValidator::Invalid);
validateInput("\n", &state);
QCOMPARE(state, QValidator::Invalid);
validateInput("\\", &state);
QCOMPARE(state, QValidator::Invalid);
validateInput("a b", &state);
QCOMPARE(state, QValidator::Invalid);
validateInput("a\\b", &state);
QCOMPARE(state, QValidator::Invalid);
validateInput(";\n;", &state);
QCOMPARE(state, QValidator::Invalid);
}
// fixup: remove invalid characters from mixed input, result should be acceptable
{
QString mixed = " ;;\\ \n::";
validator.fixup(mixed);
QCOMPARE(mixed, QString(";;::"));
QString tmp = mixed;
int pos = tmp.size();
QValidator::State state = validator.validate(tmp, pos);
QCOMPARE(state, QValidator::Acceptable);
}
// fixup: fully invalid input becomes empty and validates as Intermediate
{
QString invalid = " \t\\\n";
validator.fixup(invalid);
QCOMPARE(invalid, QString());
QString tmp = invalid;
int pos = tmp.size();
QValidator::State state = validator.validate(tmp, pos);
QCOMPARE(state, QValidator::Intermediate);
}
}
// Test that modifiers are normalized to canonical order: CTRL+SHIFT+ALT+META
```
1. Declare the new test function in the corresponding header (e.g. `tests/TestHotkeyManager.h`) inside the `TestHotkeyManager` class:
```cpp
private slots:
void customSeparatorValidatorTest();
```
(or add it alongside the other existing test slots, matching their visibility/section).
2. Ensure the header for `CustomSeparatorValidator` is included in `tests/TestHotkeyManager.cpp`, for example:
```cpp
#include "CustomSeparatorValidator.h"
```
if it is not already included.
3. If you use a different naming convention for the validator or test method, adjust `CustomSeparatorValidator` and `customSeparatorValidatorTest` to match the existing codebase conventions.
</issue_to_address>
### Comment 6
<location> `tests/TestHotkeyManager.cpp:440` </location>
<code_context>
+ QVERIFY(posAnother < posF2);
+}
+
+void TestHotkeyManager::settingsPersistenceTest()
+{
+ // Test that the HotkeyManager constructor loads settings and
</code_context>
<issue_to_address>
**suggestion (testing):** Isolate HotkeyManager tests from global QSettings state to avoid cross-test interference
Even though this test notes the limits of QSettings-based persistence, the suite still constructs `HotkeyManager`, which uses the real global `QSettings` and `IntegratedClient/Hotkeys*` keys. This can introduce test-order dependence and, in some environments, modify real user settings.
To avoid that, either configure a test-only QSettings namespace in test setup (e.g. via `QCoreApplication::setOrganizationName` / `setApplicationName`), or refactor `HotkeyManager` to accept a QSettings-like dependency or group prefix so tests can use an isolated settings instance.
Suggested implementation:
```cpp
void TestHotkeyManager::settingsPersistenceTest()
{
// Test that the HotkeyManager constructor loads settings and
// that saveToSettings() can be called without error.
// Note: Full persistence testing would require dependency injection
// of QSettings, which is beyond the scope of this test.
//
// To avoid cross-test interference and accidental modification of real
// user settings, use a test-only QSettings namespace via organization
// and application names.
const QString previousOrganization = QCoreApplication::organizationName();
const QString previousApplication = QCoreApplication::applicationName();
QCoreApplication::setOrganizationName(QStringLiteral("IntegratedClientTest"));
QCoreApplication::setApplicationName(QStringLiteral("HotkeyManagerSettingsPersistenceTest"));
HotkeyManager manager;
// Manager should have loaded something (either defaults or saved settings)
// Just verify it's in a valid state
QVERIFY(!manager.exportToCliFormat().isEmpty());
// Restore original QSettings namespace to avoid affecting other tests
QCoreApplication::setOrganizationName(previousOrganization);
QCoreApplication::setApplicationName(previousApplication);
```
If `tests/TestHotkeyManager.cpp` does not already include the necessary headers, add:
- `#include <QCoreApplication>`
- `#include <QString>`
These should be placed with the other Qt includes at the top of the file.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| QVERIFY(manager.hasHotkey("CTRL+F2")); // Other key still exists | ||
| } | ||
|
|
||
| void TestHotkeyManager::invalidKeyValidationTest() |
There was a problem hiding this comment.
suggestion (testing): Consider tests for the command separator validator, especially multi-character and invalid inputs
With the new CustomSeparatorValidator and support for multi-character separators, please add focused tests for:
- Accepting valid multi-character, printable, non-whitespace, non-
'\'separators (e.g.";;","::","@@"). - Rejecting inputs with spaces, backslashes, control characters, or mixed valid/invalid characters.
fixupbehavior: cleaning inputs by removing invalid characters, and turning fully invalid strings into empty/Intermediate.
This keeps the separator configuration UI aligned with the expectations of the splitting logic.
Suggested implementation:
QVERIFY(manager.hasHotkey("CTRL+F2")); // Other key still exists
}
void TestHotkeyManager::customSeparatorValidatorTest()
{
CustomSeparatorValidator validator;
auto validateInput = [&](const QString &input, QValidator::State *stateOut = nullptr) {
QString s = input;
int pos = s.size();
QValidator::State sState = validator.validate(s, pos);
if (stateOut) {
*stateOut = sState;
}
return s;
};
// Accept valid multi-character, printable, non-whitespace, non-'\' separators
{
QValidator::State state;
validateInput(";;", &state);
QCOMPARE(state, QValidator::Acceptable);
validateInput("::", &state);
QCOMPARE(state, QValidator::Acceptable);
validateInput("@@", &state);
QCOMPARE(state, QValidator::Acceptable);
}
// Reject inputs with spaces, backslashes, control characters, or mixed valid/invalid characters
{
QValidator::State state;
validateInput(" ", &state);
QCOMPARE(state, QValidator::Invalid);
validateInput("\t", &state);
QCOMPARE(state, QValidator::Invalid);
validateInput("\n", &state);
QCOMPARE(state, QValidator::Invalid);
validateInput("\\", &state);
QCOMPARE(state, QValidator::Invalid);
validateInput("a b", &state);
QCOMPARE(state, QValidator::Invalid);
validateInput("a\\b", &state);
QCOMPARE(state, QValidator::Invalid);
validateInput(";\n;", &state);
QCOMPARE(state, QValidator::Invalid);
}
// fixup: remove invalid characters from mixed input, result should be acceptable
{
QString mixed = " ;;\\ \n::";
validator.fixup(mixed);
QCOMPARE(mixed, QString(";;::"));
QString tmp = mixed;
int pos = tmp.size();
QValidator::State state = validator.validate(tmp, pos);
QCOMPARE(state, QValidator::Acceptable);
}
// fixup: fully invalid input becomes empty and validates as Intermediate
{
QString invalid = " \t\\\n";
validator.fixup(invalid);
QCOMPARE(invalid, QString());
QString tmp = invalid;
int pos = tmp.size();
QValidator::State state = validator.validate(tmp, pos);
QCOMPARE(state, QValidator::Intermediate);
}
}
// Test that modifiers are normalized to canonical order: CTRL+SHIFT+ALT+META
- Declare the new test function in the corresponding header (e.g.
tests/TestHotkeyManager.h) inside theTestHotkeyManagerclass:(or add it alongside the other existing test slots, matching their visibility/section).private slots: void customSeparatorValidatorTest(); - Ensure the header for
CustomSeparatorValidatoris included intests/TestHotkeyManager.cpp, for example:if it is not already included.#include "CustomSeparatorValidator.h"
- If you use a different naming convention for the validator or test method, adjust
CustomSeparatorValidatorandcustomSeparatorValidatorTestto match the existing codebase conventions.
Contributor
Author
Code Review FixesAddressed the following comments from the latest sourcery-ai review: Comment 1: setHotkey reports success even when key is invalid ✅
Comment 2: Duplicate key lists (getValidBaseKeys/getAvailableKeyNames) ✅
Comment 6: Isolate tests from global QSettings ✅
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Adds comprehensive hotkey/macro support to MMapper's integrated MUD client, allowing users to bind keyboard shortcuts to commands. Includes CLI commands for management and support for command separators to send multiple commands with one keypress.
New Features
Hotkey System
std::unordered_mapwith custom hashPerformance Optimizations
std::stringinternally to minimize QString↔std::string conversions on the hot pathgetCommandQString()convenience methods for Qt UI callersShortcutOverrideinterceptionCLI Commands
_hotkey set KEY command- Set a hotkey_hotkey remove KEY- Remove a hotkey_hotkey config- List all configured hotkeys_hotkey keys- Show available key names and modifiers_hotkey reset- Reset to default configuration_config edit- Open all configuration (including hotkeys) in external editorCommand Separator
;;to sendkill orc;;get all corpse)Other Improvements
Documentation
docs/WEBASSEMBLY_BUILD.md)scripts/build-wasm.sh)AGENTS.mdSupported Keys
Modifier keys (CTRL, ALT, META, SHIFT) can be combined with any key below:
Default Hotkeys
Testing
Summary by Sourcery
Add a configurable hotkey system to the integrated MUD client, including command parsing, configuration editing, and input handling updates, along with WebAssembly build tooling and tests.
New Features:
_hotkeycommand (set/remove/config/keys/reset) and aconfig editcommand that opens a remote editor for client configuration and hotkeys.Bug Fixes:
Enhancements:
configand newhotkeycommands.Build:
Documentation:
Tests: