| Re: [hatari-devel] Suggestion for enhanced keymap table format |
[ Thread Index |
Date Index
| More lists.tuxfamily.org/hatari-devel Archives
]
Hi,
On 14.10.2021 9.44, Vincent Barrilliot wrote:
1. Adding support for modmask, instead of just always masking out lock
modifiers. Are PC numlock & capslock likely to be modifiers that user
needs when adding ST key mappings?
My only intent for this was that most should behave the same, regardless
of the numlock (as the ST has no "numlock"), so I needed to mask it out.
I have to think again about CAPS LOCK though.
Ok, unless you tell me later otherwise, I'll leave
modmask out, and always mask numlock & capslock
modifiers out. It's simpler for users.
2. Need for array[4] of ST scancodes per keyboard mapping, which is
mainly used for handling modifiers. It seems strange half-way solution.
My intent was never to support macros, but only be able to support the
PC->ST keyboard translation, that's why this array is limited in length.
I agree that your pseudo code which works with bits rather than full
scancodes should be sufficient to achieve that. The only difference is
that with it you won't have flexibility in controlling the order in
which extra modifiers will be pressed/released, but I can't immediately
think why this should have any importance.
Ok, good.
This applies to PressedModifiers only. STScanCodes still needs to be an
array so we can specify ALT-xxx sequences.
Ah, that's a very good point!
(Somehow I had not understood that your
"Max is alt-a-b-c" comment in code meant those.)
This does not require array though.
Instead of:
ALT|x|x|x
It could be specified in keymap like this:
ALT_SEQ|xxx
Which can be stored as modifier & "scancode".
IKBD inserting would then special-case ALT_SEQ
modifier to split the "scancode" to separate
ALT-xxx sequence digits.
I'll try that and compare it to your solution
before deciding which one looks more maintainable.
Thanks for the tip about Python UI extra buttons !
The UI is quite configurable. It's just shame
that embedding Hatari to the GUI works only under
X11. [1]
I initially did that stuff so that Hatari would be
more usable on tablets & phones which do not have
physical keyboard (or if they do, there are only
limited amount of keys).
However, only the old Nokia Linux tablets and N900 / N9 did run X11
server where that is useful. It
might be useful with Rasberry Pi too though.
- Eero
[1] The reason why it does not work under other
platforms is that although SDL itself supports
embedding, that disables all SDL keyboard event
handling, which makes it useless.
I.e. Hatari Python UI needs to tell (with env var)
to Hatari to which window it should embed itself,
and Hatari needs to implement embedding itself.
And I have only X11 code for that.
Years ago when I implemented that, I did not find
a way to do it under Wayland. So on everything
else than X11 (including Mac & Windows), Hatari
and Python GUI run on separate windows.
Le 14/10/2021 à 01:48, Eero Tamminen a écrit :
Hi Vincent,
Could you provide reasoning for following:
1. Adding support for modmask, instead of just always masking out lock
modifiers. Are PC numlock & capslock likely to be modifiers that user
needs when adding ST key mappings?
2. Need for array[4] of ST scancodes per keyboard mapping, which is
mainly used for handling modifiers. It seems strange half-way
solution.
I mean, if the intention is to map single PC keybinding to multiple ST
key presses, not just support modifiers, why that would be limited to
measly 4 keys (minus any needed modifiers), instead of supporting
arbitrary sized keyboard macros? [1]
Or if the intention is just to support modifiers, why not just use
bitmask, similarly to the PC key info, like in this pseudo-code:
-----------------
insert_modifiers(mods, state):
if mods & ST_ALT_BIT:
insert_key(ST_ALT, state)
if mods & ST_SHIFT_BIT:
insert_key(ST_SHIFT, state)
key_down(sdlkey):
...
insert_modifiers(mapping.mods, KEY_DOWN)
insert_key(mapping.key, KEY_DOWN)
key_up(sdlkey):
...
insert_key(mapping.key, KEY_UP)
insert_modifiers(mapping.mods, KEY_UP)
-----------------
- Eero
[1] IMHO such functionality should remain outside of Hatari as it's
unnecessary complication.
Users can ask Hatari Python UI to provide buttons that inject
specified strings into Hatari emulation.
Try for example this:
./hatariui.py --embed --panel
"Keys,F1=59,F2=60,F3=61,F4=62,F5=63,F6=64,F7=65,F8=66,F9=67,F10=68,>,Macro=Test,Undo=97,Help=98,Enter=114,>,close"
--top "about,|,run,pause,|,Keys,|,reset,debug,|,quit"
That tells the GUI to create additional panel/dialog called "Keys",
which has 2 rows of buttons that have individual ST keys and text
strings bound to them, and which opens from the "Keys" button on
middle of the (specified) GUI top panel.
On 11.10.2021 14.01, Eero Tamminen wrote:
Hi,
On 11.10.2021 9.59, Vincent Barrilliot wrote:
Thanks for having a look. It's a bit early Monday morning here and
I'm not quite awake yet so I don't understand why I wrote that and
your code looks better at first sight.
Thanks, I'll continue working based on that
assumption, unless you'll mail me later otherwise.
The situation I know a situation that we have to handle is that, for
exemple on French PC there are some keys that you can just press to
get a character, while on the ST it requires shift or alt.
For example, '_' you can get directly on the French PC but on the
Atari it requires shift+'-'.
So pressing _ results in a shift+'_'+unshift written to the IKBD.
But if you press hold shift and press '_' on the PC then what shoud
happen ? We have to write shift+'_' **but not 'unshift' because
shift is still maintained**.
This or the fact that if you keep "_" down
I guess the trace message is there to flag up that case.
That's why we have to keep track of how many times we "pressed"
modifiers.
I'll document that somewhere in the code
(in function or file comment, not inside
function).
I don't have enough time this morning to reread the code in detail
but if you read it along the lines of what I just explained, it may
make more sense. I'll take a closer look tonight if you haven't told
me it's all clear in the meantime.
I've already pushed several smaller commits
based on your changes, but those do not have
any functional changes yet.
Rest I'm going to split / rewrite into following
kind of commits:
* Add your new ST scancode defines
* Introduce the new structures & arrays, where SDL
and ST codes are separate and have different
array sizes, without multi-scancode support
* Add multi-scancode support on top of that,
plus parsing of the new mapping file format
* Add examples
I'll mail them here so that you and others can
check/test that the code still works OK (I have
not broken anything), before I push them to
the repository.
(Then I'll sync the ikbd.c & keymap.c ST scancode
array sizes.)
- Eero
Le 11/10/2021 à 01:39, Eero Tamminen a écrit :
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...