Option rightalt-as-rightctrl doesn't work

[almson]

MacBook11,4 with hid_apple.conf:

options hid_apple fnmode=2
options hid_apple swap_opt_cmd=1
options hid_apple swap_fn_leftctrl=1
options hid_apple rightalt_as_rightctrl=1
options hid_apple ejectcd_as_delete=0

[free5lot]

This feature was added in PR #14 by Sami (@sami-mw). Further investigation needed to fix it.

[Aetf]

I haven't test it yet, but the code shows that rightalt_as_rightctrl is handled after swap_opt_cmd. Then this won't work because whenever we got KEY_RIGHTALT in hidinput_apple_event, it will be caught in the swap_opt_cmd block and returns from there, leaving no chance for rightalt_as_rightctrl to remap it again.

Actually, problems raises whenever several parameters try to map the same key code. And in the current way the code works, only the first parameter will be in effect.

One probably better way is to defer the call to input_event after all parameters are processed. But even in this case the parameter-order problem exists. For example, if handle swap_opt_cmd before rightalt_as_rightctrl, we would have

• Right Cmd => Right Ctrl
• Right Alt => Right Cmd

And otherwise if rightalt_as_rightctrl before swap_opt_cmd, we would have

• Right Cmd => Right Alt
• Right Alt => Right Ctrl

In both cases none of the two parameters fully works.

I suppose the latter case is what @almson tries to achieve? (It seems reasonable for me at least ;-) ) So as a workaround, we can defer the call to input_event and handle rightalt_as_rightctrl before swap_opt_cmd. However, we really should come up with a better mapping scheme in the long run as we can't always add another new parameter for every different mapping requests.

[almson]

@Aetf I think the fine-grained options should be discarded in favor of a few all-in-one layouts. I would propose 4 layouts: As-Silkscreened, PC-like, Mac-like (similar to As-Silkscreened, but with Control and Command swapped to give a mac feel to keyboard shorcuts), and Lenovo-like (similar to PC, but Fn key is in the corner).

Ideally, the code should also be refactored to be logical. A single remap method should encapsulate all the magic (including function key assignment). Editing the code to customize the layouts should be easy.

[Aetf]

Yes that sounds more logical. But I'm thinking about iso_layout, which actually does the same mapping things, but not really belongs to any of those layouts.

As for the new remap method, I think it's a good idea. Just note that if we are including function key assignment (I suppose you mean swap_fn_leftctrl), then the method should be called at the beginning of hidinput_apple_event to get fn key functinoal correctly.

@free5lot what do you think?

[almson]

@Aetf I'm not sure that iso_layout should be handled in the driver. This sounds like a job for the regular international keyboard layout logic in the OS. I don't exactly know what iso_layout changes, but I was able to emulate this "ISO" keyboard by going into Xfce Keyboard Layout settings and choosing English (UK, international with dead keys). Is there a deeper incompatibility? Can it be fixed without exposing a configurable setting?

The remap method should handle all keycode remapping, including the core functionality of the Fn keys (not just swap_fn_leftctrl, but the transformation of Fn combinations themselves). It should be called as early as possible. The rest of the code should be agnostic to any remapping.

[Aetf]

@almson I'm not an expert on that, too. Just that in my case I have to disable iso_layout to get a normal 'As-Silkscreened' layout. But that's what included in the original hid-apple driver. So maybe there is some reason for that.

I'm not sure about merging Fn functionality into remap. Again, I'm not really familiar with the code. It seems there are lots of things going on in the fnmode block other than simple remapping. And from a semantic perspective, remap (not include Fn functionality) is kind of lower level than the Fn functionality and remains unchanged whatever key the user currently is pressing, while the Fn functionality/mapping changes based on what key is pressed

[almson]

@Aetf What is your hardware (type/region)? What is your OS/DE? What software layout? I use a US MacBookPro11,4 and English (US) layout in Xubuntu. I don't have problems with the default iso_layout=1.

I don't know enough about how the driver works to give a full recommendation on its architecture, but I think we're in agreement that conceptually the flow should be get raw input -> transform into useable input -> use input without a spaghetti of arrows to different parts of the code where the data may be transformed and/or used. The goal is for a user to add their own custom mappings with about as much difficulty as they'd have editing a complicated config file.

[free5lot]

I'm not sure what way should be used to solve the issue with multiple flags with the same buttons. The whole patch was not intended to do this complicated stuff.

About code refactoring, e.g. with some universal remap(). It's probably a good idea as the original hid-apple code is a mess and these additional flags add even more complications. For our defense - we just use the same magic as the original author of hid-apple used to add swap_opt_cmd, iso_layout and etc.

I think, it would be great if hid-apple module was updated in kernel (this patch was not accepted or even considered for 4.0.1 kernel, it was ignored). It could clean up the modules code, as patch is not something that is expected to change the code drastically. Any ideas?

[i-kuzmin]

Simple reordering mentioned at the very begging (by @Aetf) will solve the majority of the problem without any over-engineering (I mean handle rightalt_as_rightctrl before swap_opt_cmd). It will leave current behavior as is in case rightalt_as_rightctrl is disabled, and introduce desired layout (ctrl and alt keys under right hand) when it is enabled.

[Ndolam]

Hi, I wanted to fix this but I didn't have time to look into refactoring the code so that all of the options would be processed in turn, rather than returning after finding the first translation. So I added this code, which makes swap_opt_cmd and rightalt_as_rightctrl work together. You have my blessing and permission to add it to your module, should you so wish.

// Jim Diamond quick hack to make swap_opt_cmd and rightalt_as_rightctrl work together:

static const struct apple_key_translation swapped_option_cmd_right_ctl_keys[] = {
        { KEY_LEFTALT,  KEY_LEFTMETA },
        { KEY_LEFTMETA, KEY_LEFTALT },
        { KEY_RIGHTALT, KEY_RIGHTCTRL },
        { KEY_RIGHTMETA,KEY_RIGHTALT },
        { }
};

and before "if (swap_opt_cmd) {" in hidinput_apple_event()

    if (swap_opt_cmd && rightalt_as_rightctrl)
    {
        trans = apple_find_translation(swapped_option_cmd_right_ctl_keys,
                                       usage->code);
        if (trans)
        {
            input_event(input, usage->type, trans->to, value);
            return 1;
        }
    }

This was tested on a macbook pro and works there.

[almson]

@Ndolam It would help a lot if you submitted a Pull Request or at least a real diff. Cuz, you know, "put this here, that there" is error-prone.

[free5lot]

Good news, rightalt_as_rightctrl and swap_opt_cmd work together correctly now. Please check for your cases. I finally got time and macbook and checked - it works like a charm, thanks to @nopsphere and @Ndolam for pointing out the solutions. I'm closing the issue, but any comments after your testing are welcome.