Possible bug?

[KevinH]

In Sigil Prefs (see Sigil/src/Dialogs/PreferenceWidgets/KeyboardShortcutsWidget.cpp) when assigning key sequences we capture the QKeyEvent and the key's generated key() and text(). The key() function is supposed to be independent of modifiers included according to Qt docs.

In our routine called translateModifiers() in this file we check to see if we need/should include the Shift Modifier by looking at the unicode character groupings of the text() which should be a '!' point (part of the Other Punctuation unicode group) and so the shift modifier should *not* be added in and the Ctrl+Shift+1 should be detected as Ctrl+! and that is what should be written to the ini file.

int KeyboardShortcutsWidget::translateModifiers(Qt::KeyboardModifiers state, const QString &text)
{
    int result = 0;

    // The shift modifier only counts when it is not used to type a symbol
    // that is only reachable using the shift key anyway
    if ((state & Qt::ShiftModifier) && (text.size() == 0
                                        || !text.at(0).isPrint()
                                        || text.at(0).isLetterOrNumber()
                                        || text.at(0).isSpace()
                                       )
       ) {
        result |= Qt::SHIFT;
    }

But that does not appear to be true somehow.

I would love to see what QKeyEvent 's key() and text() returns immediately after setting nextKey in KeyboardShortcutsWidget::handleKeyEvent() when Ctrl+Shift+1 is entered on impacted machines so we can see why this is not being handled properly.

Thanks!

[DiapDealer]

When I hit the Ctrl+Shift+1 keys in Sigil's Keyboard shortcuts dialog in Linux, it is Ctrl+! that actually gets registered (and appears in the .ini file) and works to successfully trigger the clip replacement.

When I do the same thing in Windows, it is Ctrl+Shift+! that gets registered (and appears in the .ini file) and FAILS to successfully trigger the clip replacement.

[BeckyEbook]

Debug:

Spoiler:

Debug: key before:              "?\uDC20"
Debug: nextKey before:          16777248
Debug: key before:              "?\uDC21"
Debug: nextKey before:          16777249
Debug: key before:              "!"
Debug: nextKey before:          33
Debug: event-modifiers before:  QFlags<Qt::KeyboardModifier>(ShiftModifier|ControlModifier)
Debug: event-text before:       "1"
Debug: state:  QFlags<Qt::KeyboardModifier>(ShiftModifier|ControlModifier)
Debug: text:  "1"
Debug: event-modifiers after:   QFlags<Qt::KeyboardModifier>(ShiftModifier|ControlModifier)
Debug: event-text after:        "1"
Debug: nextKey after:           100663329
Debug: string mac:      "Ctrl+Shift+!"
Debug: string win-lin:  "Ctrl+Shift+!"

[KevinH]

It is 100% backwards on Windows than what the Qt docs say.

The key() should not reflect modifiers and it should be "1" not "!".
The text() should reflect all modifiers and should return "!" since Shift+1 is the "!" not "1".

So these functions are reversed somehow. If it is like that for all Windows keyboards we can reverse the values for just Windows. For macOS and Linux this works as documented by Qt.

How strange!

[KevinH]

Actually on macOS it is different. If I click Command+Shift+1 I get Ctrl+Shift+1 and not Ctrl+! in the field in Sigil's Preferences and in the ini file.

So all platforms appear to be slightly different.

KevinH

ps: Here is what happens on macOS:

Debug: key(): "49" '1'
Debug: text(): ""
Debug: modifiers: QFlags<Qt::KeyboardModifier>(ShiftModifier|Control Modifier)

So here the text() call is wrong completely as it is a null string but at least the key() call returned the value WITHOUT modifiers so the code just worked.

[KevinH]

I found the following in the Qt Docs ...

QString QKeyEvent::text() const

Returns the Unicode text that this key generated.

Return values when modifier keys such as Shift, Control, Alt, and Meta are pressed differ among platforms and could return an empty string.

Note: key() will always return a valid value, independent of modifier keys.

and

int QKeyEvent::key() const

Returns the code of the key that was pressed or released.

See Qt::Key for the list of keyboard codes. These codes are independent of the underlying window system. Note that this function does not distinguish between capital and non-capital letters, use the text() function (returning the Unicode text the key generated) for this purpose.

A value of either 0 or Qt::Key_unknown means that the event is not the result of a known key; for example, it may be the result of a compose sequence, a keyboard macro, or due to key event compression.

So this is pretty much a platform specific nightmare. And it appears to be based on other things as well.

On macOS here are some test cases:

- entered small h:
key() returns 'H', text() returns 'h', no modifiers
result: H

- enter Shift + H:
key() returns 'H', text() returns 'H', shift modifier
result: Shift+H

- entered Ctrl+Shift+1:
key() returns '1', text() returns '', ctrl and shift modifiers
result: Ctrl+Shift+1

- entered Meta+Shift+1:
key() returns '!', text returns '', shift and meta modifiers
result: Meta+Shift+! (this is wrong I think!)

- entered Alt+Shift+1:
key() returns '!', text() returns '?', alt and shift modifiers
result: Alt+!

So it appears on macOS the text() value returned is either the unicode character formed, or a null string, or a "?" depending on what other modifiers are being used at the time.

The bottom line is that the existing code seems to work on Linux as expected, seems to work on macOS - most of the time but text() is basically not as advertised. And on Windows it does not seem to work for at least some modifiers on at least some Windows platforms with some key combinations.

I would love to see the output of key() and text() and modifiers() and the result stored for all of the test cases shown above on Linux and Windows so that I can begin to understand what it is going on and what commonalities exist so we can rewrite this.

The official Qt docs are clearly worthless in this regard.

BTW: To make things even worse on macOS
- using the apple/command key results in the Ctrl modifier being set
- using the option key results in the Alt modifier being set
- using the control key results in the Meta modifier being set

Thanks,

KevinH

[BeckyEbook]

Debug:

Spoiler:

small h
Debug: key before:              "H"
Debug: nextKey before:          72
Debug: key before:              "H"
Debug: nextKey before:          72
Debug: event-modifiers before:  QFlags<Qt::KeyboardModifier>(NoModifier)
Debug: text:                    "h"
Debug: nextKey after:           72
Debug: string:                  "H"

Shift+H
Debug: key before:              "?\uDC20"
Debug: nextKey before:          16777248
Debug: key before:              "H"
Debug: nextKey before:          72
Debug: event-modifiers before:  QFlags<Qt::KeyboardModifier>(ShiftModifier)
Debug: text:                    "H"
Debug: nextKey after:           33554504
Debug: string:                  "Shift+H"

Ctrl+Shift+1
Debug: key before:              "?\uDC20"
Debug: nextKey before:          16777248
Debug: key before:              "?\uDC21"
Debug: nextKey before:          16777249
Debug: key before:              "!"
Debug: nextKey before:          33
Debug: event-modifiers before:  QFlags<Qt::KeyboardModifier>(ShiftModifier|ControlModifier)
Debug: text:                    "1"
Debug: nextKey after:           100663329
Debug: string:                  "Ctrl+Shift+!"

Alt+Shift+1
Debug: key before:              "?\uDC20"
Debug: nextKey before:          16777248
Debug: key before:              "?\uDC23"
Debug: nextKey before:          16777251
Debug: key before:              "?\uDC20"
Debug: nextKey before:          16777248
Debug: key before:              "!"
Debug: nextKey before:          33
Debug: event-modifiers before:  QFlags<Qt::KeyboardModifier>(ShiftModifier|AltModifier)
Debug: text:                    "!"
Debug: nextKey after:           134217761
Debug: string:                  "Alt+!"

[KevinH]

Thanks BeckyEbook. Btw, the meta modifier is supposed to be the Windows key.
So it appears that Windows treats Ctrl + Shift plus key differently in just some cases which is just as strange as macOS.

Very odd. I am going to have to really think about how to do this that will work for all keyboard layouts.

[KevinH]

I have looked and looked at this. The only way that I can find to really do this properly is to know the keyboard layout being used so that we know if the key()/text() generated requires the SHIFT key or not.

For the US this is easy as the following holds true for both the QWERTY and DVORAK keyboards:

shift: ! @ # $ % ^ & * ( ) _ + { } | : " < > ? ~
  key: 1 2 3 4 5 6 7 8 9 0 - = [ ] \ ; ' , . / `

and seems to be the norm BUT for international keyboard layouts, this is simply not true!

So there really is no way to tell when shift is pressed and a key() of '!' is returned (which violates the Qt docs since it should ignore modifiers on US style keyboard layout) and text() returns '1' (which again violates the Qt docs since a '1' was NOT the generated unicode text) that it is okay to fix it without knowing the exact keyboard layout.

I did search for how to determine the keyboard layout being used in Qt but according to the search results this can not be done since it happens under the Qt layer.

So I think we will have to live with this mess (of needing to fix the ini file by hand) until Qt gets off its butt and actually fixes its code to do what the docs/api says it should do. If all platforms followed the api, then detecting if the shift would be at least theoretically possible.

But right now it is broken for Ctrl+Shift on Windows and broken for Meta+Shift on macOS. I have no other data for Linux as of yet other than it seems to handle Ctrl+Shift better than Windows.

[KevinH]

A thought ...

if I compare the key() to the text() and they fit into the QWERTY and DVORAK table above, then I assume it would be safe to fix things.

If anyone has an intl keyboard, it would be interesting to see a similar table

[RbnJrg]

Originally Posted by KevinH

A thought ...

if I compare the key() to the text() and they fit into the QWERTY and DVORAK table above, then I assume it would be safe to fix things.

If anyone has an intl keyboard, it would be interesting to see a similar table

Here is an image of the layout of my keyboard (latinoamerican):

Spoiler:

Wouldn't be possible for us to edit the entered shortcut in the Keyboard Shortcut dialog? So, the correct input would be stored in the .ini file.

[BeckyEbook]

@KevinH:
Perhaps the solution for Windows would be to use the system MAPVK_VK_TO_CHAR check?

I quote from the documentation:

The uCode parameter is a virtual-key code and is translated into an unshifted character value in the low order word of the return value. Dead keys (diacritics) are indicated by setting the top bit of the return value. If there is no translation, the function returns 0.

Then we will know what key was really pressed with Shift.

I did a try and I think it should work, but you would need to add a condition (Windows only) that would specify modifiers + unshifted key (if one of the modifiers is Shift) and then display that value in the Preferences window and save it to sigil.ini.

#include <Windows.h>

Add to void KeyboardShortcutsWidget::handleKeyEvent(QKeyEvent *event)

const auto unshifted_key = MapVirtualKeyA(vk, MAPVK_VK_TO_CHAR);
qDebug() << "Unshifted key:" << (char)unshifted_key;

Debug:

Debug: Original key: !
Debug: Unshifted key: 1
Debug: Full key sequence: "Shift+!"

Debug: Original key: @
Debug: Unshifted key: 2
Debug: Key sequence: "Shift+@"

Debug: Original key: #
Debug: Unshifted key: 3
Debug: Key sequence: "Shift+#"

[KevinH]

Looks promising! Where is vk getting set to pass it in? Are you using event->key() for it? Or event->nativeScanCode() for it?

[BeckyEbook]

Originally Posted by KevinH

Looks promising! Where is vk getting set to pass it in? Are you using event->key() for it? Or event->nativeScanCode() for it?

const auto vk = event->nativeVirtualKey();

[KevinH]

I will try coding that up tomorrow in master. There are similar virtual key codes for macOS but converting them to a char is not straightforward as the Windows call approach you found.