Uploaded image for project: 'Qt'
  1. Qt
  2. QTBUG-73826

Simplify QKeyEvent::modifiers() and keyboard testing code in qtestlib

    XMLWordPrintable

    Details

    • Platform/s:
      All
    • Technical Risk:
      Normal

      Description

      Currently it does this:

      Qt::KeyboardModifiers QKeyEvent::modifiers() const
      {
          if (key() == Qt::Key_Shift)
              return Qt::KeyboardModifiers(QInputEvent::modifiers()^Qt::ShiftModifier);
          if (key() == Qt::Key_Control)
              return Qt::KeyboardModifiers(QInputEvent::modifiers()^Qt::ControlModifier);
          if (key() == Qt::Key_Alt)
              return Qt::KeyboardModifiers(QInputEvent::modifiers()^Qt::AltModifier);
          if (key() == Qt::Key_Meta)
              return Qt::KeyboardModifiers(QInputEvent::modifiers()^Qt::MetaModifier);
          if (key() == Qt::Key_AltGr)
              return Qt::KeyboardModifiers(QInputEvent::modifiers()^Qt::GroupSwitchModifier);
          return QInputEvent::modifiers();
      }
      

      Which is not only ugly, but also obscure and cause of various bugs.

      1) QGuiApplication::keyboardModifiers() does not know about this "^" logic and therefore key press of Control modifier produces:

      on press: QFlags<Qt::KeyboardModifier>(NoModifier)
      on release: QFlags<Qt::KeyboardModifier>(ControlModifier)

      2) macOS [1] and Windows [2] invert the modifiers before sending them to QtGui so they would be reported correctly in QKeyEvent::modifiers(). This is why the modifiers are inverted in (1).

      [1] https://codereview.qt-project.org/#/c/56949/
      [2] search for "Invert state" in qwindowskeymapper.cpp

      X11 also can function without the current logic of QKeyEvent::modifiers():

      https://codereview.qt-project.org/#/c/253377/

      We can see that none of the major 3 platforms needs this.

      3) Code style. The QKeyEvent::modifiers() accessor should simply "return QInputEvent::modifiers()".

      4) qtestlib: With this fixed we won't need "the amazing magic" that we currently have in testlib/qtestkeyboard.h::sendKeyEvent():

      if (action == Press) {
                  if (modifier & Qt::ShiftModifier)
                      simulateEvent(window, true, Qt::Key_Shift, Qt::KeyboardModifiers(), QString(), false, delay);
      
                  if (modifier & Qt::ControlModifier)
                      simulateEvent(window, true, Qt::Key_Control, modifier & Qt::ShiftModifier, QString(), false, delay);
      
                  if (modifier & Qt::AltModifier)
                      simulateEvent(window, true, Qt::Key_Alt,
                                    modifier & (Qt::ShiftModifier | Qt::ControlModifier), QString(), false, delay);
                  if (modifier & Qt::MetaModifier)
                      simulateEvent(window, true, Qt::Key_Meta, modifier & (Qt::ShiftModifier
                                                                            | Qt::ControlModifier | Qt::AltModifier), QString(), false, delay);
      

      The XOR logic was added back in 1998:

      +/*!
      +  Returns the keyboard modifier flags that existed immediately after
      +  the event occurred.
      +
      +  \sa state()
       */
      +Qt::ButtonState QKeyEvent::stateAfter() const
      +{
      +    if ( key() == Key_Shift )
      +       return Qt::ButtonState(state()^ShiftButton);
      +    if ( key() == Key_Control )
      +       return Qt::ButtonState(state()^ControlButton);
      +    if ( key() == Key_Alt )
      +       return Qt::ButtonState(state()^AltButton);
      +    return state();
      +}
      

      It is clearly a legacy that has remained in Qt for way too long.

        Attachments

          Issue Links

          For Gerrit Dashboard: QTBUG-73826
          # Subject Branch Project Status CR V

            Activity

              People

              Assignee:
              paeglis Gatis Paeglis
              Reporter:
              paeglis Gatis Paeglis
              Votes:
              1 Vote for this issue
              Watchers:
              4 Start watching this issue

                Dates

                Created:
                Updated:

                  Gerrit Reviews

                  There are no open Gerrit changes