EFH: Add complete keymapper support#6814
Conversation
sev-
left a comment
There was a problem hiding this comment.
I made a cursory review, and this PR changes way more than required. I explained how to avoid relying on global variables in the code, thus, it will lead to much smaller code changes.
I'll continue my review once this is addressed.
engines/efh/efh.cpp
Outdated
| displayRawDataAtPos(winSeqSubFilesArray1[0], 0, 0); | ||
| input = getInput(32); | ||
| if (input != Common::KEYCODE_ESCAPE) { | ||
| getInput(32); |
There was a problem hiding this comment.
I do not like this function with side-effect, e.g. that it changes global state.
Instead, please keep it just as it was, e.g.:
intpu = getInput(32);
if (input != kActionSkipVideo)etc.
Just keep it changing _customAction as it does now, but also return this value for code readability.
engines/efh/efh.cpp
Outdated
|
|
||
| setKeymap(kKeymapMenu); | ||
|
|
||
| waitForKey(); |
There was a problem hiding this comment.
Same here, make waitForKey() also returning _customAction, so you could write if (input == kActionYes). The current code is confusing.
513649f to
a7cc63b
Compare
sev-
left a comment
There was a problem hiding this comment.
Thank you! Much cleaner as you see.
However, now the PR requires rebasing
c6fd692 to
5723398
Compare
| act->addDefaultInputMapping("JOY_X"); | ||
| fightKeymap->addAction(act); | ||
|
|
||
| act = new Action("CHARACTER1", _("Select character 1")); |
There was a problem hiding this comment.
| act = new Action("CHARACTER1", _("Select character 1")); | |
| // Character is a hero in-game | |
| act = new Action("CHARACTER1", _("Select character 1")); |
E.g. it is not a symbol. And add similar message to the translators in other places, just copy/paste
5723398 to
557aba0
Compare
|
Thank you! |
No description provided.