But you are mixing multiple topics in one big patch, and that makes it very
hard to discuss and apply single topics.
Okay, I should clarify, the PR is a mechanism for code review here. This is not something I want you to merge as is. I didn't think you even used the github's PR system directly, I was just doing this for illustration.
I needed some way to show you the code changes I am proposing. When we are finished discussing it, I would be happy to prepare a patch or PR or set of commits or whatever that is shaped the way you wish.
First, why did you switch back from "return 0x..." to "code = 0x..." in all
the new functions? Using "return" was way less source code, so I don't
quite see the point here?
You introduced this disparity when you had the _default map in one style and other maps in another style. During my review I found it much easier to compare and review if they were all in the same style, so I simply assumed the style from the _default which had the majority of code, and was pre-existing before your changes.
However, I really don't care what style this is in. It's a completely unimportant issue to me. If you want the code style one way or the other, tell me what it is and I'll be happy to amend. It sounds like the change I should have made would be to convert the _default to return style, rather than convert the newly added code to _default's style.
Second, I'm not convinced that e.g. mapping SDLK_EXCLAIM to 0x02 is really
such a good idea. I thought we agreed that keeping the symbolic mapping
available in Hatari was good for people being able to get the same
character in their emulated system when they press the corresponding key on
their host system. Now if a user of a French keyboard presses "!" on their
host system (which seems to be a key in the lower right of their keyboard),
they end up with a "1" in their emulated system. That's certainly not what
they expected. Ok, if the press Shift-! they finally get the "!" on the ST
side, but from a host keyboard perspective, that's not a "!" anymore, but a
"µ". So I think that's rather confusing than helpful. So in my opinion we
should 1) only map keys when there is a 1:1 mapping possible (without
pressing shift or another modifier key) and 2) provide some mappings for
keys that are not reachable at all by other means (e.g. the ST's UNDO key).
I think it's important to have, but there are multiple goals with this in mind.
The primary problem, the worst problem a user can have in this area is there being an ST key that they want to press, but cannot figure out any way to press from their keyboard. We should minimize this problem as much as possible. Obviously 100% coverage is not possible for all host to TOS mappings, but it's definitely possible to provide this with every matched host vs. TOS language at least, but a small handful of cases will be arbitrarily decided (4 so far, by my count).
As a secondary goal, I think we should make it as easy as is reasonable to run a TOS other than your host language and be able to use it. If there is a concise and easy to implement mapping that doesn't seem to be a code burden, I think it should exist.
Lastly, I feel that for every host keyboard covered, it is best if every key does something. In my view, having a small number of redundant keys that map to the same ST key is a benefit for the user.
So, at the end of this, allowing SDLK_EXCLAIM in particular to have a mapping for all languages is only a minor benefit, but I still see it as a benefit. I don't believe that restricting to 1:1 is a benefit at all, especially when the mappings are non-obvious. As a user, I can hit keys until I find one that does the right thing, that's easy and intuitive. Once I've found the key, I'm happy, I can use it. If there's more than one, that's not much of a problem. If I hit every key looking for something that hits a particular ST key, and can't find it, that's a big problem. I feel that having a small number of redundant mappings makes the search easier for users, not harder.
Also, consider that with the default fallback, you were not excluding SDLK_EXCLAIM for US TOS, you were simply allowing it to have the wrong code. What you're really suggesting I think would instead properly map SDLK_EXCLAIM explicitly to ST_NO_SCANCODE instead. I'm not proposing that either, as I don't think that would be beneficial, but it seems like the logical way to do 1:1 if that was the goal. (I don't think strict 1:1 should be the goal.)
In general I feel that every named SDLK_* semantic should be accounted for in each language. I think the difference we are talking about is only a few lines of code, and I feel it is a net benefit.
-- Brad Smith