-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Fix Shift+Down/Up Accessibility #24958
base: master
Are you sure you want to change the base?
Fix Shift+Down/Up Accessibility #24958
Conversation
What about left and right arrow? They seem to have the same issue |
Backport of musescore#24958
@@ -87,7 +87,8 @@ void EditShortcutModel::inputKey(Qt::Key key, Qt::KeyboardModifiers modifiers) | |||
} | |||
|
|||
// remove shift-modifier for keys that don't need it: letters and special keys |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't that comment rather be // remove shift-modifier for keys that don't need it: all non-letters, except a few special keys
@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! |
Yes, indeed, the description in the issue misses those too. |
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 |
All function keys (F1 - F12 at least, some keybords may have more) are affected too |
@Jojo-Schmitz I believe this is a different issue - #22743 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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> <kbd>=</kbd>
on GitHub.)
@@ -110,6 +111,31 @@ void EditShortcutModel::inputKey(Qt::Key key, Qt::KeyboardModifiers modifiers) | |||
emit newSequenceChanged(); | |||
} | |||
|
|||
bool EditShortcutModel::checkNotSpecialKey(Qt::Key key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".
Shift+2 is " on a German QWERTZ, so better use " directly
Ýes it is. On a German QWERTZ Shift++ is *
That would work for some, but not all, not for Ins, Del, Home, End, PageUp, PageDown, Left, Right, Up, and Down, or would it? |
This is the situation as I understand it: To illustrate that: on a Dutch keyboard, the rightmost key on the upper row is the Now, if I would press Shift and that key in EditShortcutModel, i.e. to record it, it is seen as But when I press Shift and that key normally, i.e. to call it, then it is seen as EDIT: This may be true on macOS only. |
Same for me with UK QWERTY layout for both PC and Mac keyboards.
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.)
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
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. |
Resolves: #24864
Fix Shift+Down/Up Accessibility in Edit Shortcut Dialog.