Reworked the keymap so it is more flexible, so to support modifiers

[vinz6751]

Everything is explained in the updated help files.
Removed SDL 1.2 support as per recent conversations on the hatari mailing list.

[huth]

Hi Vinz! Sorry for the long delay - I tried to review this a couple of times, but it is incredibly hard to digest in the current state. Could you please try to break up the big patch in smaller, logical chunks that make it clear how you got there? Also the current merge request seems to be merged with an older version on your side already, so it is not clear which is the really recent version of the patch. Please rebase cleanly to the master branch first.

Apart from that, here are some quick notes that I spotted while looking at your patch:

• #define MAX_SDLK_SCANCODES 284+1 is certainly wrong, my SDL headers have already higher values than that. SDL_NUM_SCANCODES is likely what you want?
• There are potential buffer overflows in your new code, e.g. the strcpy(hostSpec,token) or strcpy(guestSpec,token)
• Using "return -1; /* Success */" is very unconventional and confusing - either return 0 for success and a proper negative error code otherwise, or use "true" for success and "false" for failure.

[vinz6751]

Hi Thomas ! Thanks for having spent the time to review. Short things first: 1 About MAX_SDLK_SCANCODES, yes I also thought I could use SDL_NUM_SCANCODES (512) but vs the "real" number (286 it seems, though  285 and 286  keys "audio back/forward" would be really weird to use), but since that I thought that would waste a lot of memory so went for a more conservative array size. Maybe I'm just to used to code for the ST :D. 2 Buffer overflow / true-false: you are absolutely right, my bad sorry. 3 Sorry but I don't know how to break patches in smaller chunks. Now about the though process: The existing keymap was a one to one (SDL key to ST scancode), which is not enough to map keys with modifiers from the host (PC) to keys with modifiers from the guest (emulated ST). So instead of having LoadedKeymap as simple mapping table (SDL keysym to ST scancode), I had to introduce something more sophisticated, which is the KeyMapping structure. This KeyMapping structure allows to specify keys with modifiers from the host, to key with modifier from the guest so there are two parts in this structure: one for the host (expressed in SDL terms and one for the ST (expressed as ST scancodes). When we load keymap table (Keymap_LoadRemapFile), we read the file then need to parse the host part (done by HostSpecToSDLKeysym), and the guest part (done by GuestSpecToSTScanCodes). When you press a key, Keymap_KeyDown is called. If a keymap is loaded we try to lookup a corresponding KeyMapping using Keymap_RemapKeyToSTScanCodes then "press" all the scancodes indicated by the guest part. The modifiers (shift, alt) are "pressed" first, but only in case they were not pressed already. And we have to keep them down so the character can repeat correctly if the user keeps the key down. It means we have to remember which modifiers we had to effectively press as part of handling that KeyDown, so we can "release" them when the key is up. That's why we store them in PressedModifiers member of the KeyMapping structure. If you press a key that is already press, we'll have a problem releasing the modifiers from the first key, that's why there is a warning saying we don't support pressing already pressed keys. When the key is up, Keymap_KeyUp is called. There we release the key, then only the modifiers which we had to press. The KeyMapping.PressedModifiers tells us which ones should be released. The KeyMapping.modmask thingy is there mainly to filter out NumLock. Obviously, all these sophistications I had to add because things were not behaving as I wanted. I didn't add anything "wishfull that might be used one day". Vinz

…

On 29/05/2021 15:27, Thomas Huth wrote: Hi Vinz! Sorry for the long delay - I tried to review this a couple of times, but it is incredibly hard to digest in the current state. Could you please try to break up the big patch in smaller, logical chunks that make it clear how you got there? Also the current merge request seems to be merged with an older version on your side already, so it is not clear which is the really recent version of the patch. Please rebase cleanly to the master branch first. Apart from that, here are some quick notes that I spotted while looking at your patch: * #define MAX_SDLK_SCANCODES 284+1 is certainly wrong, my SDL headers have already higher values than that. SDL_NUM_SCANCODES is likely what you want? * There are potential buffer overflows in your new code, e.g. the strcpy(hostSpec,token) or strcpy(guestSpec,token) * Using "return -1; /* Success */" is very unconventional and confusing - either return 0 for success and a proper negative error code otherwise, or use "true" for success and "false" for failure. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#11 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACJCL24LY2EJHG6NTBXMWMLTQDTU3ANCNFSM4VSFJWNA>.