Conversation
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ((ignore_mods) || (key_modifiers == settings.kbkeys[key_id].mods)) { | ||
| i = settings.kbkeys[key_id].code; | ||
| result = lbKeyOn[i]; | ||
| } else { | ||
| result = 0; |
There was a problem hiding this comment.
ignore_mods currently bypasses the modifier check entirely (if (ignore_mods || key_modifiers == ...)), which means a binding like Alt+X will also match plain X when ignore_mods is true. Several new call sites pass true expecting to allow extra modifiers, so this can cause unintended actions (eg. exit) without the required modifier. Consider changing the ignore_mods logic to still require the configured modifiers (eg. (key_modifiers & required_mods) == required_mods) while allowing additional modifiers, or rename/split the flag to avoid misuse.
| //hardcoded alternative keys | ||
| if (!result) | ||
| { | ||
| if ((key_id == GKey_FrameSkipIncrease) && is_key_pressed(KC_EQUALS, KMod_CONTROL)) | ||
| result = 1; |
There was a problem hiding this comment.
The hardcoded alternative-key path sets result = 1 but never updates *val to the actual alternative key code. Callers that do clear_key_pressed(val) will clear the primary binding instead of the key that was actually pressed, leaving the alternative key latched and potentially retriggering every frame. When an alternative matches, set *val accordingly (and consider covering GKey_CheatMenu3 too if Shift+Enter is meant to keep working in dungeon view).
| int32_t val; | ||
| if (is_game_key_pressed(Gkey_ExitGame, &val ,true)) | ||
| { | ||
| clear_key_pressed(KC_X); | ||
| clear_key_pressed(val); | ||
| exit_keeper = true; |
There was a problem hiding this comment.
is_game_key_pressed(Gkey_ExitGame, &val, true) combined with the current ignore_mods behavior makes the ExitGame action trigger on the bound keycode regardless of modifiers (eg. plain X instead of Alt+X). Until ignore_mods is adjusted to only ignore extra modifiers, this should not pass true here (or should explicitly enforce the configured modifier).
| GKey_ScreenRecord, | ||
| GKey_ScreenShot, | ||
| GKey_FrameSkipIncrease, | ||
| GKey_FrameSkipDecrease, | ||
| GKey_ZoomMinimapIn, |
There was a problem hiding this comment.
The GameKeys enum introduces mixed naming (GKey_* vs existing Gkey_*). This inconsistency makes the API harder to use and is easy to misuse when adding future keys. Please standardize the new entries to the established Gkey_ prefix (and update call sites) so all game key identifiers follow one convention.
| #: guitext:10 90 | ||
| msgctxt "Game controls" | ||
| msgid "Prev Instance" | ||
| msgstr "Vorherige Instanz" |
There was a problem hiding this comment.
The source reference comment has a typo: #: guitext:10 90 contains a space, which likely breaks tooling/searchability for the guitext:1090 entry. Please correct it to #: guitext:1090.
| #define STRINGS_MAX 2000 | ||
| #define DK_STRINGS_MAX 941 | ||
| #define GUI_STRINGS_COUNT 1083 | ||
| #define GUI_STRINGS_COUNT GuiStrEnd - STRINGS_MAX |
There was a problem hiding this comment.
GUI_STRINGS_COUNT is now defined as GuiStrEnd - STRINGS_MAX without parentheses. As a macro, this is prone to operator-precedence bugs if used in expressions (eg. GUI_STRINGS_COUNT/2 would expand unexpectedly). Wrap the expression in parentheses (and ideally the whole macro) to make it safe: #define GUI_STRINGS_COUNT (GuiStrEnd - STRINGS_MAX).
|
|
||
| #: guitext:1089 | ||
| msgctxt "Game controls" | ||
| msgid "Next Instance" |
There was a problem hiding this comment.
Let's go with 'Ability' over instance, as referred to in string 25
|
|
||
| #: guitext:1090 | ||
| msgctxt "Game controls" | ||
| msgid "Prev Instance" |
| msgid "Prev Instance" | ||
| msgstr "" | ||
|
|
||
| #: guitext:1091 |
There was a problem hiding this comment.
I really do not think it's a good idea to list the cheat menu controls in the define keys list.
|
|
||
| #: guitext:1096 | ||
| msgctxt "Game controls" | ||
| msgid "Landview Show All Ensigns" |
There was a problem hiding this comment.
These 3 cheat controls do not belong either
No description provided.