Well, that's the point, we don't use github in the Hatari project, it's
just a mirror, so please don't open PRs there (I still try to handle them
in case someone opens one, but I'm more or less the only one looking at
them, the other developers normally are not aware of patches that are
posted there, so this is bad). For reviewing patches, please send them here
to the mailing list, or stick them in a branch in your public repository
somewhere and post the URLs here to the mailing list.
Okay, I did post the URL to the mailing list, but I could put a PR on my own fork if that's better for you, I just assumed the public Hatari one was a more appropriate place to leave something for public discussion than my own fork.
Ok, so for French TOS, why shouldn't SDLK_EXCLAIM be mapped to 0x09 ?
You've mapped it to 0x0D in your patch, but that key is already covered by
"-" ?
I did this because SDLK_UNDERSCORE is not normally present on the FR host layout. $0D should have a mapping to some key on a standard French keyboard, and unfortunately there is no direct analog. Because SDLK_EXCLAIM doesn't have a direct analog on the FR TOS, that was the one I selected.
This also applies to SDLK_ASTERISK and case 178, which also had no direct analog, and I felt could be used to cover ST keys which also did not have a direct analog.
https://listengine.tuxfamily.org/lists.tuxfamily.org/hatari-devel/2024/02/msg00050.html
that also does not help with "!" I think. If a user tries to
press the ST's "2" key on a non-French TOS, they certainly don't expect
to hit the "!" on their host keyboard to get this key.
Okay, there is a simpler way to describe the goal here.
The primary goal is a good layout for the matching host + TOS case, we cover all those keys first.
After this, all remaining SDLK_* enumerations are semantics. They are not for any specific keyboard, the code does not know what the host keyboard is, it only knows an "EXCLAIM" semantic key was used, therefore map it to whatever is best.
I think every named semantic symbol should be mapped to whatever is closest. This is not specifically matching "French Exclaim to ST 2", it's matching any keyboard that can produce SDLK_EXCLAIM. I'm not able to account for the hundreds of possible keyboards, but I think it's worth trying to find a best fit for every semantic symbol SDL offers. Some people will want to use Hatari from a language that is not covered by a TOS, and they should get as good a default mapping to their chosen TOS as we can provide in that case.
Yes, some of the mappings will be weak, but I strongly feel that after meeting the primary matching language goal, every remaining named semantic should be assigned. Even if the mapping was just French Exclaim to ST 2, if the user doesn't know what's on the ST numerical keys but is looking for something that will produce !, if they press ! on their keyboard and get 2, that's still a small benefit in my view. Probably at that point they would realize they could also just press 2, but if it teaches them that, good, I think this is better than the key doing nothing for them.
My viewpoint remains that not being able to press the ST key that you're hunting for is the worst problem the user can have with the mapping, the problem I'm trying to minimize. Best case, they press a key and get what they expect. Fallback case, they're not sure what key to press, but they try other keys on their keyboard and find one that presses it. Worst case, they press all the keys on their keyboard and none of them do what they want. The fallback case isn't ideal, but it's far, far better than the worst case, and that's my goal, to prevent the worst case when the ideal isn't possible.
For other keys, your choices also seem to be rather random and
questionable... e.g. SDLK_QUOTE is currently mapped to 0x28. That's a valid
mapping in 4 languages (US, UK, NL, PL). You changed the default mapping to
0x0C which is only valid in 3 languages (IT, CHFR, CHDE). So what's the
advantage of your change?
So, in my initial review, I was trying to find some way to avoid arguing about what should be in the "default" keymap, so I was trying to pick the majority version, so at least creating the least number of override lines could be a non arbitrary goal to prevent arguing which of these should be chosen. So, to answer your question there, PL is not implemented so I wasn't counting it, and NL is one I haven't been able to independently review yet. However, I don't think it's actually important to resolve this specific question.
After thinking about it I don't think it was a good approach. I think we should not have "default" at all, the base case should simply be US, and define all other languages as overrides to that. I feel this way everything is much easier to compare and maintain.
The majority idea was a poor one on my part, in part because the idea of majority would have to be re-evaluated for every new language implemented, but mostly because as I continued working with it and reviewing it I found it made it more work to figure out which one was supposed to apply. I wasn't comparing against something that could be self consistent, like the US mapping, I was comparing against an arbitrary abstract default which was some mangled combination of all languages at once. So... I think the "default" should be abandoned in favour of a more practical point of reference. I made that change in the PR branch already.
Ultimately I don't want to be arguing about choices in the abstract "default" set, as I don't think it's productive. What matters is the output scancodes for each language state the user could have active. That's the goal here. The internal details I'm willing to do many different ways, but choosing a "default" for SDLK_QUOTE is ultimately irrelevant to the output, so I don't have an argument for or against any particular value, other than I don't really think we should have that "default" code section at all.
This was also my way of thinking with my original branch of work, I simply had a complete switch table for each language, inputs to outputs, and there was no "default" abstract layer between them. In your rewrite I saw the default layer as an attempt to reduce how many lines there are in each table, and I tried to follow that to its conclusion, but that was probably a bad idea on my part.
Anyway, the practical internal difference between having a "default" base beneath US, or using US as the base, really just means a couple of extra switch cases in most languages, versus the problem of having to arbitrate and understand whatever "default" should mean, which to me doesn't help the goal.
My entire goal is about inputs vs. outputs, and that's all I want to argue for, and that's what I review and compare in my own testing. I test keypresses on my keyboard and through OS mechanisms, and I make dumps of the resulting maps to make sure I haven't missed something. If you've got needs for the internal structure of the code, I'm amenable to whatever that happens to be, the same way I don't really mind that you didn't like the complete tables in my initial approach.