Fix Shift+Down/Up Accessibility

[shubham-shinde-442]

Resolves: #24864

Fix Shift+Down/Up Accessibility in Edit Shortcut Dialog.

• I signed the CLA
• The title of the PR describes the problem it addresses
• Each commit's message describes its purpose and effects, and references the issue it resolves
• If changes are extensive, there is a sequence of easily reviewable commits
• The code in the PR follows the coding rules
• There are no unnecessary changes
• The code compiles and runs on my machine, preferably after each commit individually
• I created a unit test or vtest to verify the changes I made (if applicable)

[Jojo-Schmitz]

What about left and right arrow? They seem to have the same issue
As do Ins, Home, Del, End, PageUp and Pagedown
NumPad keys als have an issue, they seem to interprete the Shift key wrongly, a Shift+NumPad+1 turns into NumPad+End, as if the Shift sets NumLock

[Jojo-Schmitz]

Shouldn't that comment rather be // remove shift-modifier for keys that don't need it: all non-letters, except a few special keys

[shubham-shinde-442]

@Jojo-Schmitz Initially, I thought the issue only occurred with Shift+Down or Shift+Up, based on the provided issue description. Now I got it, Thanks!

[Jojo-Schmitz]

Yes, indeed, the description in the issue misses those too.

[Jojo-Schmitz]

How about something like this:

    // remove shift-modifier for keys that don't need it: all non-letters except some special keys
    if ((k & Qt::ShiftModifier) && ((e->key() < Qt::Key_A) || (e->key() > Qt::Key_Z) || (e->key() >= Qt::Key_Escape))) {
        switch (e->key()) {
            case Qt::Key_Up:
            case Qt::Key_Down:
            case Qt::Key_Left:
            case Qt::Key_Right:
            case Qt::Key_Insert:
            case Qt::Key_Delete:
            case Qt::Key_Home:
            case Qt::Key_End:
            case Qt::Key_PageUp:
            case Qt::Key_PageDown:
            case Qt::Key_Space:
                break;
            default:
                qDebug() << k;
                k &= ~Qt::ShiftModifier;
                qDebug() << k;
        }
    }

Actually I'm not sure why that || (e->key() >= Qt::Key_Escape) is there (and was there before the PR), it seems completly superfluous: if e->key() > Qt::Key_Z is true, that conditions is never checked for, and if e->key() >= Qt::Key_Escape is true, e->key() > Qt::Key_Z is always true too.
OTOH, if removing that condition, why not also allowing Shift+Esc, i.e. add it to the exceptions in that switch/case.

[Jojo-Schmitz]

All function keys (F1 - F12 at least, some keybords may have more) are affected too

[shubham-shinde-442]

NumPad keys als have an issue, they seem to interprete the Shift key wrongly, a Shift+NumPad+1 turns into NumPad+End, as if the Shift sets NumLock

@Jojo-Schmitz I believe this is a different issue - #22743

[shoogle]

It's not clear to me why we remove the Shift modifier from any shortcuts. If the user wants to perform a different action with Shift vs. without Shift then we should let them.

Maybe it's related to keyboard layouts? For example, on my UK QWERTY PC keyboard I have to type Shift = (equals) to get a + (plus) sign, but on other keyboards + might have a dedicated key.

If Qt reports my keypress as Shift + (when it was really Shift =) we might indeed prefer to shorten it to just + when displaying the shortcut in the UI. However, ideally we shouldn't prevent people with dedicated + keys from assigning Shift + to something else.

Perhaps we could check the event text to see whether Shift + actually resulted in a + being typed?

(Shortcuts like Shift = are written as <kbd>Shift</kbd>&nbsp;<kbd>=</kbd> on GitHub.)

[shoogle]

You should use affirmative names to avoid creating a double negative:

// BAD

if (!checkNotSpecialKey(key)) {
    // special
}

if (checkNotSpecialKey(key)) {
    // not special
}

Also, 'special' is a relative term. You should try to be precise:

// GOOD

if (isShiftAllowed(key)) {
    // allowed
}

if (!isShiftAllowed(key)) {
    // not allowed
}

You should put all the conditions in this function, not just the "specialness".

[Jojo-Schmitz]

It's not clear to me why we remove the Shift modifier from any shortcuts. If the user wants to perform a different action with Shift vs. without Shift then we should let them.

Shift+2 is " on a German QWERTZ, so better use " directly
Same for Shift+<, which is > on a German QWERTZ

Maybe it's related to keyboard layouts? For example, on my UK QWERTY PC keyboard I have to type Shift = (equals) to get a + (plus) sign, but on other keyboards + might have a dedicated key.

Ýes it is. On a German QWERTZ Shift++ is *

Perhaps we could check the event text to see whether Shift + actually resulted in a + being typed?

That would work for some, but not all, not for Ins, Del, Home, End, PageUp, PageDown, Left, Right, Up, and Down, or would it?

[cbjeukendrup]

This is the situation as I understand it:
EditShortcutModel records keyboard shortcuts by character; the model to call shortcuts works by key combination.

To illustrate that: on a Dutch keyboard, the rightmost key on the upper row is the = key, and with Shift it will produce a +.

Now, if I would press Shift and that key in EditShortcutModel, i.e. to record it, it is seen as Shift++. In this case, the Shift is indeed redundant, and should be discarded, because it is already "included" by the fact that it is + rather than =.

But when I press Shift and that key normally, i.e. to call it, then it is seen as Shift+=. But for Shift+=, no action was recorded; only Shift++ was recorded. So, nothing happens.

EDIT: This may be true on macOS only.

[shoogle]

@cbjeukendrup

on a Dutch keyboard, the rightmost key on the upper row is the = key, and with Shift it will produce a +

Same for me with UK QWERTY layout for both PC and Mac keyboards.

if I would press Shift and that key in EditShortcutModel, i.e. to record it, it is seen as Shift++

Same for me. On Windows, it arrives as Shift + but it's displayed as + because our code removes the Shift modifier.

I can't edit code on macOS right now, so I can't see what the shorcut arrives as internally, but I can confirm it's displayed in the UI as + like on WIndows. (Sadly, I can't test on Linux at all right now.)

But when I press Shift and that key normally, i.e. to call it, then it is seen as Shift+=. [...] So, nothing happens.

Not for me! On WIndows, when called, it still arrives as Shift +, and it works to perform actions assigned to +. This includes the default "Toggle accidental: sharp" action as well as other actions that I have manually assigned it to.

Again, I can't check the internals on macOS, but I can confirm it still works to perform actions assigned to +, including default and non-default actions. This is in the latest nightly build for master on both Windows and macOS.

@Jojo-Schmitz

Perhaps we could check the event text to see whether Shift + actually resulted in a + being typed?

That would work for some, but not all, not for Ins, Del, Home, End, PageUp, PageDown, Left, Right, Up, and Down

We would use a combination of the reported text, keys, and modifiers to determine which physical keys were actually pressed, and how this should be represented "conceptually" in the UI.

At least, that's what I was thinking, but it may not work for a combination like Ctrl Shift = (i.e. Ctrl +). In that situation, Qt says the event text that is returned depends on the platform and may be empty.