Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect group latch behavior #577

Open
mahkoh opened this issue Jan 4, 2025 · 10 comments · May be fixed by #580
Open

Incorrect group latch behavior #577

mahkoh opened this issue Jan 4, 2025 · 10 comments · May be fixed by #580
Labels
bug Indicates an unexpected problem or unintended behavior state Indicates a need for improvements or additions to the xkb_state API

Comments

@mahkoh
Copy link

mahkoh commented Jan 4, 2025

Consider this test case:

    assert(test_key_seq(keymap_,                                               \
                        KEY_H,          BOTH,  XKB_KEY_h,               NEXT,  \
                        /* No latch-to-lock */                                 \
                        KEY_COMPOSE,    BOTH,  XKB_KEY_ISO_Group_Latch, NEXT,  \
                        KEY_COMPOSE,    BOTH,  XKB_KEY_ISO_Group_Latch, NEXT,  \
                        KEY_H,          BOTH,  XKB_KEY_h,               NEXT,  \

where, effectively,

        key <compose> {
            [ ISO_Group_Latch ],
            [ LatchGroup(group=2) ],
        };
        key <h> {
            [ h ],
            [ hebrew_yod ],
            [ Cyrillic_ha ],
        };

The test passes but the behavior is not correct. The second compose cycle should set the latched group to Group3.

I noticed this when porting this test to my implementation. The following is the output of my test:

both h
    key_down(h)
    sym = h, char = 'h'
    key_up(h)
both compose
    key_down(compose)
    sym = ISO_Group_Latch
    group_pressed = 0x1
    group_effective = 0x1
    key_up(compose)
    group_pressed = 0x0
    group_latched = 0x1
both compose
    key_down(compose)
    sym = ISO_Group_Latch
    group_pressed = 0x1
    group_effective = 0x2
    key_up(compose)
    group_pressed = 0x0
    group_latched = 0x2
both h
    key_down(h)
    sym = Cyrillic_ha, char = 'х'
    group_latched = 0x0
    group_effective = 0x0
    key_up(h)

This follows from both the specification and the X server implementation.

@Jules-Bertholet
Copy link

My understanding of the spec is that the second cycle should keep the latched group at 2, no? To get the latch to increment by 1 each time, you would want LatchGroup(group=+1) (the + makes groupAbsolute false).

@mahkoh
Copy link
Author

mahkoh commented Jan 5, 2025

key release adds the key press delta to the latched keyboard group.

The delta, between the previously pressed group and the newly pressed group, is 1 both times. Relative changes only affect how the newly pressed group is calculated. The delta calculation is independent of that.

@Jules-Bertholet
Copy link

The delta, between the previously pressed group and the newly pressed group, is 1 both times.

The first latch increments the latched group from 1 to 2 (delta +1), and the second from 2 to 2 (delta +0), no?

@mahkoh
Copy link
Author

mahkoh commented Jan 5, 2025

The latched group is irrelevant for the pressed delta.

@Jules-Bertholet
Copy link

Jules-Bertholet commented Jan 6, 2025

SA_LatchGroup:

  • Key press and release events have the same effect as an SA_SetGroup action […]
  • Otherwise [if latchToLock did not take effect], key release adds the key press delta to the latched keyboard group.

SA_SetGroup:

If groupAbsolute is set, key press events change the base keyboard group to group ; otherwise, they add group to the base keyboard group.

So, the delta is presumably (group set after key press) - (group set before key press), and that delta is applied to the latched group on SA_LatchGroup release. Setting the group to 2 when it is already 2 (due to a pre-existing latch and lack of anything layering on top of said latch) results in a delta of 0, so the latch on release should also have a 0 delta according to the spec.

@mahkoh
Copy link
Author

mahkoh commented Jan 6, 2025

Setting the group to 2 when it is already 2

There is no such thing as "the group". Pressing the key affects the base group and releasing the key affects the base and latched group. The effective group is a derived property.

@mahkoh
Copy link
Author

mahkoh commented Jan 6, 2025

I was referring to the set group

The set group is 0 at the start of each line in the test.

@Jules-Bertholet
Copy link

Ah, I think I see my error. You are entirely correct. Apologies for the noise.

@Jules-Bertholet
Copy link

That being said, I think that the specified behavior is very counterintuitive. To determine the actual effect of a latch, you have to translate the 1-based absolute group index specified in the symbols file to a 0-based delta, then add the (also 0-based, even though symbols file uses 1-based) locked and base groups to get the 0-based effective group. This also makes groupAbsolute=true nearly useless. And it means there is no way to change the effective group via a set or latch in a way that does not depend on the currently set or latched group. TBQH the specified behavior is so bad I think libxkbcommon might be justified in deliberately departing from it, or at least making it configurable.

@wismill wismill added bug Indicates an unexpected problem or unintended behavior state Indicates a need for improvements or additions to the xkb_state API labels Jan 7, 2025
@wismill
Copy link
Member

wismill commented Jan 7, 2025

@mahkoh Good catch! I filed a fix in #580. I also updated the test with the third group locked.

This also makes groupAbsolute=true nearly useless. And it means there is no way to change the effective group via a set or latch in a way that does not depend on the currently set or latched group.

@Jules-Bertholet I agree that the computations related to groups may be counter-intuitive, but unfortunately this is the behavior defined by the XKB protocol. I got a similar issue a while ago.

TBQH the specified behavior is so bad I think libxkbcommon might be justified in deliberately departing from it, or at least making it configurable.

We cannot depart from it or make it configurable unless we introduce format versions. This is what currently blocks #527; see also #484. Meanwhile one has to use the group keysyms/actions carefully to achieve the desired behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior state Indicates a need for improvements or additions to the xkb_state API
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants