Re: [hatari-devel] Suggestion for enhanced keymap table format

[Eero Tamminen <oak@xxxxxxxxxxxxxx>]

Hi Vincent,

I've been going through changes in your pull request, splitting them 
into separate commits
and updating / improving things.

However, there's one thing that I do not understand in your 
Keymap_KeyUp() changes:
---------------------------------
if (Keyboard.KeyStates[scancode])
{
	LOG_TRACE(TRACE_KEYMAP,"  %02x was down, will be up'ed\n",scancode);

	Keyboard.KeyStates[scancode]--;
	IKBD_PressSTKey(scancode, false);
}
else
	LOG_TRACE(TRACE_KEYMAP,"  %02x will be kept down (presses: 
%d)\n",scancode,Keyboard.KeyStates[scancode]);
----------------------------------

You've changed Keyboard.KeyStates[] to keep status
of how many presses each key ST key has gotten,
not just whether it's down.

But if it's zero (i.e. should be up), why you're
outputting a trace message that says the key will still be kept down?

Shouldn't it be rather:
----------------------------------
if (!Keyboard.KeyStates[scancode])
{
	Log_Printf(LOG_WARN, "key %02x got up event, although it's not down\n", 
scancode);
	continue;
}

if (--Keyboard.KeyStates[scancode])
{
	LOG_TRACE(TRACE_KEYMAP,"  %02x will be kept down (remaining presses: 
%d)\n",
	          scancode, Keyboard.KeyStates[scancode]);
	continue;
}
LOG_TRACE(TRACE_KEYMAP,"key %02x was down, will be up'ed\n", scancode);
IKBD_PressSTKey(scancode, false);
----------------------------------
?

	- Eero

On 12.1.2021 22.57, Vincent Barrilliot wrote:
> Hello
>
>
> I have refreshed my forked repo from the reference one, resolved 
> conflicts, tested and created patch file again after having rebased.
>
> Kind regards
>
>
> Vinz
>
>
> On 10/01/2021 23:05, Eero Tamminen wrote:
> > Hi,
> >
> > On 1/10/21 7:15 PM, Vincent Barrilliot wrote:
> > > Here is a quick demo:
> > >
> > > https://youtu.be/4qWL23X77aI
> > >
> > > To enable that, I selected the "keymap" Keyboard option with the 
> > > updated doc/fr/clavier-exemple.txt
> > >
> > >
> > > The patch is attached. I produced it using
> > >
> > > git format-patch master --stdout > improved-kbd-support.patch
> > >
> > > I'm not sure how to split that though, sorry! For next changes I will 
> > > try to split things better.
> >
> > That's easiest if you commit your changes in small logical steps. It's 
> > much easier to afterwards
> > group small changes to larger ones, and skip
> > ones that turned out to be non-relevant ones,
> > with:
> >     git rebase -i <start commit>
> >
> > Than to split large changes into smaller ones
> > afterwards.
> >
> >
> > > For this change I think it's much easier to read the keymap.c file as 
> > > a whole.
> >
> > It still contains remains of SDL v1 support.
> >
> > Please rebase the changes to the latest Hatari code.
> >
> >
> >     - Eero
> >
> >
> > > On 01/01/2021 10:15, Thomas Huth wrote:
> > > > Am Tue, 3 Nov 2020 14:39:05 +0100
> > > > schrieb Nicolas Pomarède <npomarede@xxxxxxxxxxxx>:
> > > >
> > > > > Le 03/11/2020 à 14:24, Eero Tamminen a écrit :
> > > > > > Hi,
> > > > > >
> > > > > > I did a diff between your and current keymap.c
> > > > > > version (attached).
> > > > > Hi
> > > > >
> > > > > thanks for posting the diff
> > > > >
> > > > > > For your intended change to be reviewable, please
> > > > > > split all non-functional & unrelated code changes
> > > > > > (white space/indenting, const, ! -> ==0, type
> > > > > > changes, commenting out trace outputs) to
> > > > > > a separate patch, or remove them.
> > > > > >
> > > > > > Then either rebase your changes to latest Hatari
> > > > > > git version of keymap.c, or otherwise make sure
> > > > > > they apply and still work with it, before posting
> > > > > > a patch / "git diff" against it.
> > > > > looking at the diff you posted, I don't see that much unrelated
> > > > > changes, there're a few indents changes here and there, but seeing
> > > > > the patch is rather big it looks ok to me if some indents or some log
> > > > > levels are changed to fit the patch, but nothing that seems to
> > > > > prevent from reviewing the changes.
> > > > >
> > > > > Also, keymap.c was last modified in dec 2019, so I guess the patch
> > > > > should apply to dev version without loosing code.
> > > > >
> > > > > As for reviewing/testing the patch itself, unfortunately I don't have
> > > > > time at the moment. Maybe Thomas can have a look at it if he got time.
> > > > I just had a quick look, but the patch is indeed rather big. Any chance
> > > > that you could split it up logically into more digestible parts (i.e.
> > > > multiple patches)? Also, there are still cosmetic changes in here (like
> > > > the last hunk that changes "if (!keycode)" into "if (keycode == 0)"
> > > > etc.) ... that really should either be dropped or put into separate
> > > > clean-up patches.
> > > >
> > > > > As a french user of Hatari, this looks like an interesting patch
> > > > > anyway, as some characters are indeed "hard" to enter by default.
> > > > I have to say that I hardly use the keymapping stuff on my own, since
> > > > the German keyboard layout is pretty similar to the US layout anyway
> > > > and since I normally only edit my files on the host side and avoid text
> > > > editors in the emulated environment. So Nicolas, it would be great if
> > > > you could have a look at these modifications if they make sense from a
> > > > French keyboard user perspective.
> > > >
> > > >   Thomas
> > > >
> > > >
> > > > PS: Would it help if we'd remove the SDL1.2 support first? I guess
> > > > keymap.c would be simplified quite a bit by that removal...