Re: [hatari-devel] Recent symbolic keyboard mapping change

[Eero Tamminen <oak@xxxxxxxxxxxxxx>]

Hi,

Thanks for looking into this!

On 4.10.2024 19.33, Thomas Huth wrote:
> Am Thu, 3 Oct 2024 21:54:02 -0400
> schrieb Brad Smith <rainwarrior@xxxxxxxxx>:
> > The layouts I had completed I considered complete: US, DE, FR, UK, ES, IT,
> > SE, CHFR, and CHDE
> >
> > Layouts I have not yet researched: FI, NO, DK, SA, NL, CS, HU
>
> At least FI seems to be the same as Swedish, so you can drop that of your
> list.

Yes, Finnish and Swedish keyboards are identical, so comment on that can 
be updated in code.


> Well, what I did was basically a complete rewrite of your patches.

While changes look OK in general, please use the defines from "tos.h" 
instead of magic numbers in Keymap_SetCountry()!


Btw. In the patch set I was working years ago, I initially had similar 
"overlay" approach, but I later switched to using per-language mapping 
tables because there just were too many things that had to be overridden 
with modifier key combos, overriding wasn't really maintainable then any 
more.

But "perfect is enemy of good", and this clearly improves what Hatari 
had earlier...


> > I've tried to do a careful review of the changes, and I've made and tested
> > a PR, which can be viewed here:
> >
> > https://github.com/hatari/hatari/pull/36

Yes, it's good to include more comments where these come from, otherwise 
it's really hard to check/verify things afterwards.


> I can try to have closer look at this at the weekend, but from a first
> glance, you're changing too much stuff in one go, so it's difficult to
> review and apply this patch:
>
>   https://github.com/hatari/hatari/pull/36/commits/a05f01ac1b7949b712d8a9cd326b626dd6bd
>
> Can you split up that work into individual, coherent steps, please?

I would suggest having comment updates/additions as separate commit, and 
larger code changes split per language (updates for just couple of lines 
could maybe go to same commit).


	- Eero