[kmscon-devel] About keyboard shortcuts and consumed modifiers

[David Herrmann]

Hi Ran

On Wed, Mar 13, 2013 at 7:46 PM, Ran Benita <ran234 at gmail.com> wrote:
> On Mon, Mar 11, 2013 at 12:01:37AM +0100, David Herrmann wrote:
>> Hi Ran
>>
>> On Sun, Mar 10, 2013 at 11:30 PM, Ran Benita <ran234 at gmail.com> wrote:
>> > On Sun, Mar 10, 2013 at 04:27:07PM +0100, David Herrmann wrote:
>> >> Hi Ran
>> >>
>> >> On Sun, Mar 10, 2013 at 11:21 AM, Ran Benita <ran234 at gmail.com> wrote:
>> >> > I noticed the last commit added a nice feature for dynamically
>> >> > increasing/decreasing the font size with a shortcut. The default is
>> >> > <Ctrl><+> and <Ctrl><->.
>> >> >
>> >> > When I tried it I noticed that you need to use <Ctrl><Shift><=> to get
>> >> > the effect of <Ctrl><+>, because on this keymap the <+> is shifted. I
>> >> > think though that the expected behavior is that you don't actually need
>> >> > to use Shift there.. I wrote some explanation on this a while back, I
>> >> > thought it might intrest you:
>> >> > http://cgit.freedesktop.org/libxkbcommon/tree/xkbcommon/xkbcommon.h?id=bb620df7aa98c129#n1323
>> >>
>> >> I am aware of the problem, but didn't have the time to fix it. Thanks
>> >> for pointing me to the xkb_*_consumed_*() helpers.
>> >
>> > Well in a German layout (if you use that) I see that + is not shifted,
>> > so you might have missed that.
>>
>> Yes, indeed.
>>
>> >> So to fix this bug, I basically want to call
>> >> xkb_state_mod_mask_remove_consumed() on the keycode+modifiers for each
>> >> key-event and pass this as "effective_mods" or something like this
>> >> along, right? And I should call this _after_ xkb_state_update_key(),
>> >> right?
>> >
>> > I might have confused two separate issues in my message.
>> >
>> > First to solve the issue with the + and =, I don't see a better solution
>> > than just adding two bindings, one one + and one with =. This boils down
>> > to the fact that eventually you are comparing the keysyms. Solving it in
>> > another way, like probing the keymap levels etc., would just be more
>> > complicated (but I haven't thought of it too much). Of course for
>> > letters (e.g. <Ctrl><A> vs. <Ctrl><a>) it's easier, because you just
>> > convert to lower or upper case. But with + and = it's completely
>> > arbitrary.
>>
>> I think I understand that now. So you are basically saying the
>> ctrl+'-' shortcut is just _wrong_ on your layout because you want
>> ctrl+'='. Even if I fix the modifier-comparison, you still have to
>> press "Shift", but that's not what you want, right?
>
> Yes.
>
>> I could fix that by comparing all keysyms of all the levels of the
>> single key. However, I think that's overkill and we might run into
>> several other problems here. Keymaps can be weird and what happens if
>> the user has "+" and "-" on the same key in different levels? That
>> just gets nasty.
>> We would have to first check the active level, then the other
>> levels... Argh, no, I don't want to go that way. If someone has the
>> time to investigate into that, I'd appreciate it. But I think that's
>> indeed a waste of time.
>>
>> I think the fix here is that you simply have to change the shortcut
>> via --grab-zoom-out="<ctrl>equal,<ctrl>KP_Equal" right?
>> A default <ctrl>minus seems legit to me. I cannot expect to find
>> shortcuts that are easy and simple on all layouts. And at least with
>> correct modifier-comparison, I still allow users to use these
>> shortcuts (with the drawback that they have tohold Shift or whatever).
>>
>> Or what do you think is the right fix for this specific problem?
>> (apart from the mod-comparison fix)
>
> Yes, figuring out "automatically" probably can be done, but it's what I
> wanted to avoid. I guess what I wanted, in essence, is to bind keys to
> actions (many possible combinations to one action), rather to bind actions
> to keys (one action <-> one combination). This seems a bit more common,
> but it works better in config files than on the command line.

You can do that. Just separate each shortcut with a comma as I showed
in the example:
  --grab-zoom-out="<ctrl>equal,<ctrl>KP_Equal"

I know, the inverse approach would be better. Something like:
  --bind <ctrl>KP_Equal=zoom-out

But that's ugly on the command-line. And I am still working on a
better ini-file parser that will allow such stuff.

>> > Second, about the consumed modifiers. I suggest being explicit, i.e. add
>> > a 'consumed_mods' field to the event, and do the comparison as described
>> > in the mod_index_is_consumed comment. The function to calculate this
>> > mask should be similar to shl_xkb_get_mods (or maybe just make this one
>> > return the two masks at once). Then you can just use the
>> > xkb_state_mod_index_is_consumed() function. The remove_consumed() function
>> > wouldn't work with a mask of SHL_*_MASK, because those may be different
>> > than the xkbcommon modifier indexes which it expects.
>>
>> Maybe I wasn't clear enough, but that's what I meant. Extending
>> shl_xkb_get_mods() to return both fields. I was coing for all_mods and
>> all_but_consumed_mods. But your way "all_mods and consumed_mods" is
>> maybe the better choice.
>> Bit-operations are cheap anyway...
>>
>> >> I think that's the easiest way. Any comments?
>> >
>> > Hope I haven't confused you too much :)
>>
>> I still wonder how to support all the other modifiers that keymaps may
>> advertise. I currently hardcode the standard modifiers, but what if a
>> keymap also provides MOD_FOO?
>> If the user adds "<mod_foo>Keysym" as shortcut, I currently return an
>> error because the modifier is unknown. However, I should instead look
>> it up in the layout and use it if available. But how do I communicate
>> the xkb_mod_index_t between uterm-input and any input-listener? And
>> what happens when the layout is changed during runtime? Hm.. That
>> requires some thought...
>> Anayway, not directly related to the problem.
>
> Daniel designed the API with the notion that the application uses the
> xkb_state directly without a mediator. But I'm assuming that you don't
> want such tight coupling, and the 'key event with modifier mask' idiom
> is common enough... So what I'd say is just add them as you need them :)
> If someone asks for a modifier it's easy to add.

I think I can simply add "xkb_state" to "uterm_input_event" so the
callbacks can access the current state. (That's one reason why I
removed the dumb-keyboard backend so xkb_state can be part of the
API).

But there are some other things I have to rewrite to make that
possible. Anyway, I will try to get that working in the next weeks.

>> One more thing. Lets look only at the modifiers. The code is:
>>
>> @code
>> (state_modifiers & ~consumed_modifiers & significant_modifiers) ==
>> shortcut_modifiers
>> @endcode
>>
>> This still fails with the following example:
>>   state_mods = CTRL | SHIFT
>>   consumed_mods = SHIFT
>>   significant_mods = CTRL | SHIFT
>>   shortcut_mods = CTRL | SHIFT
>>
>> In this case, the SHIFT is consumed but we also require it as part of
>> the shortcut. But the shift is cleared by "~consumed_mods" and the
>> comparison will return false.
>>
>> So I recommend changing this to:
>>
>> @code
>> (state_modifiers & ~consumed_modifiers & significant_modifiers) ==
>> (shortcut_modifiers & ~consumed_mods)
>> @endcode
>>
>> This will remove consumed mods also from the shortcut.
>
> Ah, good point. I remember I had considered it, but I can't recall why
> I had decided against it! Of course it generally "shouldn't happen"..
> Only thing I could find now is here:
> https://developer.gnome.org/gdk/stable/gdk-Keyboard-Handling.html#gdk-keymap-translate-keyboard-state
> But GTK is a mess here (they keep changing how they handle this stuff
> about once a year), And I don't think their reason is relevant for us.
> So you should do it as you say, and if I can remember again I'll be
> back :)

I don't get what the gnome-comment tries to explain.. Anyway, I will
think a bit more about it. If nothing comes up, I will just implement
it that way. It's at least better than the current way.

Thanks for the help!
David