#4759 closed defect (fixed)
GUI: Regression in numpad handling
| Reported by: | Templier | Owned by: | bluddy |
|---|---|---|---|
| Priority: | normal | Component: | GUI |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
r46808 introduced support for keypad direction keys to SCI and the GUI.
Since SCI has its own event handler that tracks all needed modifier keys (including num lock) and keeps the flag states internally, it works properly in games (see SciEvent::getFromScummVM() in sci/event.cpp:104)
But the patch also changed the GUI widgets event handlers and then now handle the keypad keys as if numlock was pressed all the time, even though the sdl backend never checks for that particular modifier key (see SDLModToOSystemKeyFlags(SDLMod mod) in backends/platform/sdl/events.cpp:155)
This can be seen by trying to input values from the keypad in the editable widgets (or in the debugger console).
I'm going to try to come up with a patch for sticky-key support, so that the SCI engine-specific code can be modified to rely on the common event code.In the meantime, would it be possible to revert all the non-SCI changes in r46808, since it breaks the console and all editable widgets (including the savegame dialog).
Ticket imported from: #2943361. Ticket imported from: bugs/4759.
Attachments (7)
Change History (25)
comment:1 by , 15 years ago
comment:2 by , 15 years ago
Assigning to bluddy for now, who committed r46808.
Littleboy, thanks for the patches. Personally, I'd really prefer if "flags" and "modifier" were not separate fields, but rather a single one. IMHO code which checks modifiers using "==", is simply buggy...
Gotta ask some other folks what they think. And/or maybe we'll just apply the patch as-is and merge the two fields again later.
comment:3 by , 15 years ago
| Owner: | set to |
|---|---|
| Summary: | Regression in numpad handling → GUI: Regression in numpad handling |
comment:5 by , 15 years ago
I first tried to add them to the flags field, but it ended up breaking the GMM (and a few engines).
It seems some engine and common code treat it as a single value and not as a bitmask. I won't mind changing it back to use a single flags field and fix all uses, but it will need more testing than the current solution. It certainly is the best solution in the long term (along with adding the word _bitmask_ in the documentation :D).
Affected "users": (might include other backends too) - OSystem (3 in default-events.cpp) - agi (6 in agi.cpp/preagi_*.cpp/) - agos (5 in event.cpp) - cruise (1 in cruise_main.cpp) - draci (1 in draci.cpp) - gob (1 in util.cpp) - kyra (5 in gui.cpp/kyra_v1.cpp) - m4 (1 in events.cpp and another I'm not too sure) - queen (1 in input.cpp) - saga (1 in input.cpp) - scumm (7 in input.cpp) - teenagent (3 in teenagent.cpp) - tinsel (1 in tinsel.cpp) - touche (1 in touche.cpp)
comment:6 by , 15 years ago
Sorry for this issue -- my main drive in making this patch was allowing the PSP (or any other console) to have a consistent D-Pad to use for SCI0, AGI, Indy3 and the GUI. Since the scumm code doesn't allow games like Indy3 to receive arrow key input (and I didn't want to cause regressions in other Scumm games), the arrow keys were not an option, and this seemed like the simpler route. I forgot some people may want to use the keypad to enter numbers in the GUI, since I never use it that way myself. Also, the SCI0 arrow key support puts the diagonals on home, end, pgup and pgdown, which are not really diagonal. Basically, it would really make life simpler if we had consistent key support :)
So much for my rationale. Unfortunately I don't have the time right now to deal with this patch, and I'd rather it was assigned to someone else, what with a brand new baby girl and heavy pressure at work.
Just to give my 2 cents' worth, I think there should be one flags field as well. If we do keep the modifiers as an interim solution, I think it should be given a very clear name like 'lock_modifiers' or 'sticky_lock_modifiers' to prevent any further confusion i.e. so no-one goes and puts things that should be in flags inside modifiers. Come to think of it, flags is not a very descriptive name either, and maybe it should be changed to modifiers if we're going to go through with the single field solution.
Nevertheless, good job on the patches Julien.
comment:7 by , 15 years ago
bluddy: no problem, it makes perfect sense to add support for it, and it highlighted some inconsistencies in how numpad handling worked (always getting num keys, even when numlock is off) and a crash bug in the console (already fixed by lordhoto).
fingolfin: did you get more feedback on it? If the single field solution is the preferred one, I'll rework the original patch and update the related code to fix wrong usage of ==.
comment:8 by , 15 years ago
Hey Julien. I don't see further comments by fingolfin, and I'd really like to restore the lost functionality of the keypad :) Also, I really do think using flags for everything just makes sense and avoids confusion. If you have the time, go ahead and update the patch. If not, I'll do it.
comment:9 by , 15 years ago
Hi bluddy.
I'm going to work on updating it to use a single flags field and fix all the engines using the flags incorrectly. I'll also try to add support for CAPS LOCK (not treated as a sticky key by SDL) so we can drop the "hack" in SCI (in event.cpp:134).
I hope to be done with it early next week (or maybe a little earlier). This probably won't make the branch though...
by , 15 years ago
| Attachment: | 2 - gui_check_for_modifiers_in_keypad_handling.patch added |
|---|
2 - Check of modifiers in widgets (fixes regression in r46808)
by , 15 years ago
| Attachment: | 4 - sci_check_for_modifiers_instead_of_keyboard_events.patch added |
|---|
4 - Remove special handling of numlocks and capslocks key events in SCI (use flags instead)
comment:11 by , 15 years ago
I updated the patches and added a new one for all engine code not using the flags field properly.
Scroll Lock is not handled as a modifier yet and I still need to take a look at the virtual keyboard and at the keymapper.
by , 15 years ago
| Attachment: | 1 - osystem_add_support_for_sticky_key_modifiers.patch added |
|---|
1 - Add support for numlocks & capslocks sticky modifiers
comment:12 by , 15 years ago
Hope I didn't miss anything, but I'm all for flags and modifiers being merged too (in case that didn't happen yet) and "just" fixing all client code to handle that properly.
by , 15 years ago
| Attachment: | 3 - convert_engines_to_use_new_hasFlags_method.patch added |
|---|
3 - Convert engines to use the new hasFlags method
by , 15 years ago
| Attachment: | 5 - osystem_add_support_for_scroll_lock_sci_remove_hack.patch added |
|---|
5 - Add support for Scroll lock as a modifier key and remove hack in SCI event code
comment:13 by , 15 years ago
lordhoto: the updated patch does use a single flags field. I don't have all the games needed to test all engines, but hopefully I didn't miss any improper uses of the flags field.
I added another patch to handle scroll lock as a sticky flag (this allows to remove the hack in the sci event code). Since SDL doesn't treat scroll lock as a modifier, we need to track its value and so keep it as an OSystem private variable. The key state unfortunately doesn't follow the system state and is unique to the ScummVM windows (also a limitation due to how SDL handles it).
comment:14 by , 15 years ago
littleboy: Good (and fast) job. patch 5 looks like a git patch -- no big deal, just not quite as easy to use if someone wants to test it.
BTW can you list which engines you've tested? I'll try to test some others.
by , 15 years ago
| Attachment: | 5_-_osystem_add_support_for_scroll_lock_sci_remove_hack (SVN).patch added |
|---|
5 - Add support for Scroll lock as a modifier key and remove hack in SCI event code (SVN patch)
comment:15 by , 15 years ago
Sorry about the last patch, I manually converted it and hopefully it will be easier to apply now (I don't know how to export patches depending on each other from a subversion tree in an easy way, so I just exported it from my git clone)
Here is the list of engines touched by the patch (and how) and the ones I tested: - AGI (CTRL+d), Troll (CTRL+s): not tested - Agos (CTRL/ALT+0/9 - CTRL+a/f/d/s/i - SHIFT): not tested - Cruise (CTRL+d/f): not tested - Draci (F5): OK - Gob (CTRL+f/g/p): OK (but getting an error in the pause dialog: "PauseDialog" does not seem to be defined in the theme. Works in 1.0, not sure if it is a regression) - Kyra (CTRL+q - CTRL/ALT+0/9 - CTRL+d/q): OK (but loading an non existant save quits the game after opening the debugger - will open a separate issue) - Lure (CTRL+d): OK - M4 (CTRL+d / CTRL+widepipe): Not tested - Parallaction (CTRL+d): not tested - Queen (CTRL+d/f): OK - Saga (CTRL+d/~/o): not tested - Scumm (CTRL+t/v/d/f/g/s - CTRL/ALT+0/9 - CTRL/ALT+F1/F5/F8): Tested in a couple of games, but each one exposes a different set. Everything seems to work (only one not tested is CTRL+s, not sure what it's supposed to do) - Sky (CTRL+f/g/d): - Sword2 (CTRL+d/#/~ - SHIFT+p - CTRL+f): OK - Teenagent (CTRL+d/f - F5): OK - Tinsel (ALT+m - CTRL/ALT+q - CTRL+d): - Touche (CTRL+f): not tested - Tucker (CTRL+f): not tested
comment:16 by , 15 years ago
I posted the complete patch if anyone still wants to test it. I included a tiny omission - the sdl.h declaration of member variable _scrollLock.
I also tested every remaining engine but Troll. Looks like it works fine and I'm going to commit unless anyone has any objections.
Great work littleboy!
comment:17 by , 15 years ago
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
comment:18 by , 6 years ago
| Component: | --Unset-- → GUI |
|---|
The first patch adds support for the numlock and capslock modifiers. I added a new field in the KeyState struct for sticky modifiers instead of storing everything in the flags field because a lot of engines (and the core scumm code) use == to test for flags.
The second patch fixes the regression in GUI widgets by testing for the numlock modifier before handling keypad keys. It also changes the behavior of the keypad keys when numlock is off: before it treated it as always on, now it ignores the keys that aren't used as special function keys (such as left/right, etc.)
The last patch simplifies the sticky modifiers handling in SCI by using the modifiers directly instead of checking for key up/down events. The special handling is still needed for scroll-lock as it doesn't seem to be treated as a modifier key in SDL.
Note: I did not touch the keymapper (or the other backends), but they might need to be updated too, if only to force the numlock modifier as always on.