Added permanent key mappings

[donniebreve]

Added functionality for permanent key remapping. See #15.

[Adda0]

(Draft review) LGTM. The remap functionality works flawlessly. And I must say that permanent remapping is definitely a useful option to have.

I would like to see a better explanation of what remap does in the configuration file (as it is with the other table headers ([...])).

• permanent remapping without the need to hold a hyper key
• ability to swap two keys as follows:

[Remap]
KEY_T=KEY_M
KEY_M=KEY_T

I think that for clarity, it is important to have the remap table before the hyper key and hyper key bindings tables in the config file to emphasize the independency of permanent mappings to the hyper key.

Furthermore, I think that it is worth mentioning in the config file that if you want to define a specific behaviour to a permanently remapped key (KEY_T) with a hyper key, you will get a behaviour of a remapped key (KEY_M) when pressing the original key (t): When pressing hyper key + t, you get e. I am not sure if this is desirable or whether it would be better to allow explicitly defining a different hyper key behaviour for KEY_T: To get r when pressing hyper key + t and to get e when pressing hyper key + m.

[Bindings]
KEY_T=KEY_R
KEY_M=KEY_E

# Permanent remappings
[Remap]
KEY_T=KEY_M

In the same manner, I feel like there should be an option to use a permanently remapped key (t in this case) as a hyper key. As of now, that is not possible.

As of now, I would approve the PR to be merged if we do not want to act on the points above. If additional functionality is planned to be added to the PR, I will reevaluate the review.

[donniebreve]

I would like to see a better explanation of what remap does in the configuration file (as it is with the other table headers ([...])).

What would you like to see as a comment?

I think that for clarity, it is important to have the remap table before the hyper key and hyper key bindings tables in the config file to emphasize the independency of permanent mappings to the hyper key.

I am wondering if this is just confirmation since that's how the config file is.

Furthermore, I think that it is worth mentioning in the config file that if you want to define a specific behaviour to a permanently remapped key (KEY_T) with a hyper key, you will get a behaviour of a remapped key (KEY_M) when pressing the original key (t): When pressing hyper key + t, you get e. I am not sure if this is desirable or whether it would be better to allow explicitly defining a different hyper key behaviour for KEY_T: To get r when pressing hyper key + t and to get e when pressing hyper key + m.

In the same manner, I feel like there should be an option to use a permanently remapped key (t in this case) as a hyper key. As of now, that is not possible.

It seems like you're conceptualizing this in a different way than I did. You're expecting that 't' is always recognized as 't' for input, regardless of the configuration. Is it more intuitive to always refer to a key as the physical keyboard key, regardless of remapping?

I thought of remapping as if it were an external application. If you remap 't' to 'm', you shouldn't expect 't' as input anymore. If you remap 't' to 'm', but then want to map 't' to 'y' when space is held, you would remap 'm' to 'y' for the layer.

I wrote this with the remap layer being the first layer, and I believe you're imagining it as the last layer.

How do other key mapping projects (xkb, xmodmap) handle remapped keys. Do later configuration lines still detect the key as 't'?

I'm open to reworking this, although the proposed logic is a bit more difficult to implement. The logic change should allow you to use a remapped key as the hyper key but you would refer to it as the physical key in the hyper configuration.

Proposed example:

[Hyper]
Hyper1=KEY_SPACE

[Bindings]
KEY_T=KEY_E

[Remap]
KEY_T=KEY_M
KEY_E=KEY_B

1. Pressing 't' outputs 'm'
2. Pressing space+'t' outputs 'e'
3. Pressing 'e' outputs 'b'

[auouymous]

"Permanent" remapping sounds like it modifies something so the key is forever remapped even if touchcursor is removed.

How do other key mapping projects (xkb, xmodmap) handle remapped keys. Do later configuration lines still detect the key as 't'?

Xmodmap only binds symbols to keycodes. For keycode 28 = t T and keycode 58 = m M, the physical t key always produces keycode 28 even if you change it to keycode 28 = m M. X11 programs that don't read the symbols will bypass any mapping done by Xmodmap.

The proposed example seems the most logical, it produces exactly what you see in the file.

[donniebreve]

Xmodmap only binds symbols to keycodes.

After some cursory research, I noticed this too. I will move forward with the proposed changes, i.e. moving the remap layer to the last layer.

"Permanent" remapping sounds like it modifies something so the key is forever remapped even if touchcursor is removed.

Any suggestions for different wording?

[auouymous]

Any suggestions for different wording?

Drop the word "permanent" and only call it key or keycode remapping.

[Adda0]

I am wondering if this is just confirmation since that's how the config file is.

Just a confirmation, yes.

Any suggestions for different wording?

Drop the word "permanent" and only call it key or keycode remapping.

I would rather keep the Permanent remappings as the official name for the feature (and therefore keep this in the config file). What I was thinking about is more in the terms of adding additional explanation with examples. Something as follows:

# Permanent remappings.
# Allows to set permanent remappings when running the service without a need to hold a hyper key.
# Each remapping will be active permanently when the service is running.
# The permamently remapped key can be remapped differently while holding the hyper key in table `[Bindings]` with the original key name:
# [Remap]
# KEY_T=KEY_M
# [Bindings]
# KEY_T=KEY_D
#
# You can swap the functionality of two keys as follows:
# [Remap]
# KEY_T=KEY_M
# KEY_M=KEY_T 
[Remap]

If we want to further discuss the exact wording, I can create a PR to be merged onto this branch when we are in an agreement. Actually, looking at the config file now, We have two separate tables allowing setting some remappings ([Remap] and [Bindings]). We should probably expand the description of table [Bindings] as well, otherwise users may not understand that the bindings in the table [Bindings] have an effect only when holding a hyper key. Something as follows for the table [Bindings] then:

# Hyper key bindings.
# The following bindings remaps keys only when holding a hyper key.
# The remapping has the format:
# KEY_1=KEY_2
#
# For usable key names, see 'github.com/torvalds/linux/blob/master/include/uapi/linux/input-event-codes.h'.
#
# For example, when holding the hyper key, key 't' will output key 'k':
# [Bindings]
# KEY_T=KEY_K
[Bindings]

Xmodmap only binds symbols to keycodes.

After some cursory research, I noticed this too. I will move forward with the proposed changes, i.e. moving the remap layer to the last layer.

Both approaches are possible. I think, however, that having the permanent remappings as the last layer would be more beneficial to the user when the original keys are to be remapped with the hyper key functionality as well. I agree with the chosen approach. Exactly as @donniebreve described, I prefer the following approach much more for the ease of use for the user:

The logic change should allow you to use a remapped key as the hyper key but you would refer to it as the physical key in the hyper configuration.

[auouymous]

Those three comments read exactly the same if "permanent" is removed.

# Allows to set remappings when running the service without a need to hold a hyper key.
# Each remapping will be active when the service is running.
# The remapped key can be remapped differently while holding the hyper key in table `[Bindings]` with the original key name:

[Adda0]

Those three comments read exactly the same if "permanent" is removed.

Maybe, but if you want to refer to the feature anywhere else without all these exhaustive comments, you have to have a single self-explanatory name for the feature. Without the permanent adjective, we have two different features present in the config file: bindings and bindings. That does not help us determine which is which, does it? One is for the hyper key bindings and the second is for permanent bindings. I think using permanent makes sense and describes pretty precisely what they are. And no one will think that when the service is not running, the new “permanent” bindings will be active, I believe. And if they do, they have the comments to explain it.

Furthermore, due to backward compatibility, we cannot rename the hyper key bindings under the table [Bindings] unless we want to break everyone's configs. Therefore, we have to add a specific, distinguishable name each time we introduce a new feature (new types of bindings).

[Adda0]

Looks great to me now. The mapping behaves exactly how I would have intuitively understood it to, and every discussed combination of permanent remappings and hyper key remappings works. Even permanent remapping of a key and using the same key as a hyper key.

I would consider this PR to be in a ready-to-merge stage. Up to you to decide whether to undraft the PR and move towards merging. Great job.

[Adda0]

@donniebreve What do you think about this PR? I have been using the permanent remapping since opening this PR and I think it works great. I cannot say that I found any problems with it.

As you mentioned somewhere, it might be a good idea to review the whole of mapper.c to probably fix and improve the emitting logic, but that would be a task for another PR.

As far as this PR goes, I think it is a great addition to the application and is ready to be merged, except for maybe addressing the debug prints.

[donniebreve]

Sure, let's merge this in until I get some motivation to rewrite some things.

[Adda0]

LGTM. It is a great feature to have (I cannot express enough how much this PR helped me – having enter key in a place of semicolon, so thanks). As agreed upon, I agree that we can merge this PR. Additional changes may come later. Feel free to merge the PR at your leisure.