[Building/Packaging] ‘constexpr QKeyCombination::operator int() const’ is deprecated

[Vekhir]

Describe the problem PR #1070 changed the way shortcuts are handled. This means lots of new code dealing with keys, and it seems that the relevant PR treats them as int at times. This usage is deprecated in Qt6 (at least in Qt 6.7). This involves only warnings - no errors -, but it's not recommended to use deprecated behaviour in new code.

Versions Provide version information for all components and libraries.

• OpenBoard: latest dev
• Qt version: 6.7.2

Build commands I'm using the cmake community build for Arch Linux, though qmake should be similarly affected. Full instructions: PKGBUILD

Warnings The warning is always the same:

warning: ‘constexpr QKeyCombination::operator int() const’ is deprecated: Use QKeyCombination instead of int

Their resolution might require different approaches. These are all 5 of the warnings in Qt 6.7.2:

/build/openboard-git/src/OpenBoard/src/core/UBShortcutManager.cpp: In member function ‘bool UBShortcutManager::hasCtrlConflicts(const QKeySequence&) const’:
/build/openboard-git/src/OpenBoard/src/core/UBShortcutManager.cpp:609:71: warning: ‘constexpr QKeyCombination::operator int() const’ is deprecated: Use QKeyCombination instead of int [-Wdeprecated-declarations]
  609 |             ctrlShortcuts << QKeySequence(action->shortcut()[0] ^ Qt::CTRL);
      |                                                                       ^~~~
In file included from /usr/include/qt6/QtCore/qbytearray.h:9,
                 from /usr/include/qt6/QtCore/qstringview.h:8,
                 from /usr/include/qt6/QtCore/qchar.h:656,
                 from /usr/include/qt6/QtCore/qstring.h:14,
                 from /usr/include/qt6/QtCore/qhashfunctions.h:8,
                 from /usr/include/qt6/QtCore/qhash.h:10,
                 from /usr/include/qt6/QtCore/qabstractitemmodel.h:8,
                 from /usr/include/qt6/QtCore/QAbstractTableModel:1,
                 from /build/openboard-git/src/OpenBoard/src/core/UBShortcutManager.h:31,
                 from /build/openboard-git/src/OpenBoard/src/core/UBShortcutManager.cpp:27:
/usr/include/qt6/QtCore/qnamespace.h:1902:26: note: declared here
 1902 |     constexpr Q_IMPLICIT operator int() const noexcept
      |                          ^~~~~~~~
/build/openboard-git/src/OpenBoard/src/core/UBShortcutManager.cpp:617:67: warning: ‘constexpr QKeyCombination::operator int() const’ is deprecated: Use QKeyCombination instead of int [-Wdeprecated-declarations]
  617 |         ctrlShortcuts << QKeySequence(additionalShortcut[0] ^ Qt::CTRL);
      |                                                                   ^~~~
/usr/include/qt6/QtCore/qnamespace.h:1902:26: note: declared here
 1902 |     constexpr Q_IMPLICIT operator int() const noexcept
      |                          ^~~~~~~~
/build/openboard-git/src/OpenBoard/src/core/UBShortcutManager.cpp: In member function ‘void UBShortcutManager::ignoreCtrl(bool)’:
/build/openboard-git/src/OpenBoard/src/core/UBShortcutManager.cpp:689:75: warning: ‘constexpr QKeyCombination::operator int() const’ is deprecated: Use QKeyCombination instead of int [-Wdeprecated-declarations]
  689 |                 if (ignore && !shortcuts.empty() && shortcuts[0][0] & Qt::CTRL) {
      |                                                                           ^~~~
/usr/include/qt6/QtCore/qnamespace.h:1902:26: note: declared here
 1902 |     constexpr Q_IMPLICIT operator int() const noexcept
      |                          ^~~~~~~~
/build/openboard-git/src/OpenBoard/src/core/UBShortcutManager.cpp:690:63: warning: ‘constexpr QKeyCombination::operator int() const’ is deprecated: Use QKeyCombination instead of int [-Wdeprecated-declarations]
  690 |                     QKeySequence noCtrl(shortcuts[0][0] ^ Qt::CTRL);
      |                                                               ^~~~
/usr/include/qt6/QtCore/qnamespace.h:1902:26: note: declared here
 1902 |     constexpr Q_IMPLICIT operator int() const noexcept
      |                          ^~~~~~~~
/build/openboard-git/src/OpenBoard/src/core/UBShortcutManager.cpp: In member function ‘bool UBActionGroupHistory::keyReleased(QKeyEvent*)’:
/build/openboard-git/src/OpenBoard/src/core/UBShortcutManager.cpp:830:58: warning: ‘constexpr QKeyCombination::operator int() const’ is deprecated: Use QKeyCombination instead of int [-Wdeprecated-declarations]
  830 |             int actionKey = action->shortcut()[0] & ~Qt::KeyboardModifierMask;
      |                                                          ^~~~~~~~~~~~~~~~~~~~
/usr/include/qt6/QtCore/qnamespace.h:1902:26: note: declared here
 1902 |     constexpr Q_IMPLICIT operator int() const noexcept
      |                          ^~~~~~~~

Suggestions, solutions @letsfindaway As the author of the PR, would you take a look at it to see which changes might be necessary, seeing as you know the code best? I'll take a look at it aswell, but I won't work on this before next week.

[letsfindaway]

Thanks for finding out and reporting. I will have a look on that.

Edit: I can reproduce the warning with Qt 6.6.3, which is what I have here on Leap 15.6. The class QKeyCombination is new with Qt 6.0, so we can probably not avoid a version check.

The easiest way I found for UBShortcutmanager is to add .toCombined() before doing the bit manipulations. I think this could be applied to all 5 occurrences of the warning.,

We could either apply a #if at each of the 5 occurrences or define a macro, e.g. KEY_COMBINATION_AS_INT which expands either to its parameter (for Qt 5) or appends .toCombined() (for Qt6).

[Vekhir]

If we need an #if anyway, might it make sense to take advantage of QKeyCombination in a broader way? Overall, working with ints is a smell for me, though maybe that's the only way in Qt5. I haven't looked at the code in detail.

[letsfindaway]

If we need an #if anyway, might it make sense to take advantage of QKeyCombination in a broader way? Overall, working with ints is a smell for me, though maybe that's the only way in Qt5. I haven't looked at the code in detail.

I have not found a way to remove a key from a QKeyCombination. But I need this in order e.g. to remove the Ctrl modifier for a shortcut. So converting to int may be the only way, even for Qt6.

Edit: Ok, I could get the Key from the combination and the KeyboardModifiers, then manipulate the modifiers and create another KeyCombination. This would indeed work and would be cleaner, but much more code. I struggle with this, too.

[letsfindaway]

We have three different usages where the int conversion is performed:

• Flip the Ctrl modifier: set it when it is not set and vice versa. Happens three times.
• Ask if the key combination has the Ctrl modifier. Happens once.
• Get the key of the key combination. Happens once.

The second and third usage could be much cleaner implemented using the features of QKeyCombination. So we should probably provide a version check for those.

The first usage happens three times. Here probably a helper function can be provided, either as a static class member or even a function which is not a class member at all. I would prefer the second because the function it provides has nothing to do with shortcut management, but just provides an additional means to deal with key combinations.

The signature of the function could even be different for Qt5 and Qt6:

#if (QT_VERSION >= QT_VERSION_CHECK(6, 0, 0))
    QKeyCombination flipModifier(const QKeyCombination& keyCombination, Qt::KeyboardModifier modifier)
    {
        Qt::KeyboardModifiers mod = keyCombination.keyboardModifiers();
        mod.setFlag(modifier, !mod.testFlag(modifier));
        return QKeyCombination(mod, keyCombination.key());
    } 
#else
    int flipModifier(int keyCombination, Qt::KeyboardModifier modifier)
    {
        return keyCombination ^ modifier;
    } 
#endif

[Vekhir]

I've been thinking about hasCtrlConflicts - and maybe I missed an edge case - but to get rid of the flipping entirely:

1. Collect all shortcuts as they are in a QSet (say distinctShortcuts)
2. For each shortcut, add Ctrl if not there via | and put the result in another QSet (distinctShortcutsUpToCtrl)
3. Compare the length of both QSets. Equal means no conflicts (all are distinct), if distinctShortcutsUpToCtrl is shorter, there are conflicts.

This only requires the | operator of QKeyCombination which also works with int. Thoughts?

[letsfindaway]

This only requires the | operator of QKeyCombination which also works with int. Thoughts?

The | operator does not help here, because it is not defined with a QKeyCombination on one of the sides. It is only defined to create a QKeyCombination from a key and a modifier in various combinations.

But the general idea sounds good. Will think about it later.

[letsfindaway]

I've been thinking about hasCtrlConflicts - and maybe I missed an edge case - but to get rid of the flipping entirely:

1. Collect all shortcuts as they are in a QSet (say distinctShortcuts)
2. For each shortcut, add Ctrl if not there via | and put the result in another QSet (distinctShortcutsUpToCtrl)
3. Compare the length of both QSets. Equal means no conflicts (all are distinct), if distinctShortcutsUpToCtrl is shorter, there are conflicts.

This only requires the | operator of QKeyCombination which also works with int. Thoughts?

Your proposed algorithm would work, but I do not see that it is a major improvement to the current code. It still requires manipulation of modifiers in a QKeyCombination and also performance-wise it is equivalent to the current code. A benefit of the current code is that it could determine the conflicting key combinations if this would be required for some user interface, while just comparing counts cannot.

As I already said: QKeyCombination is an immutable class and there is no operator or function which modifies an existing instance. So it is equivalent whether you implement | or ^.

[Vekhir]

@letsfindaway Yeah, I missed that | isn't defined to add a modifier to a QKeyCombination. If it was, it would be much cleaner; without it we'd need to implement it ourselves, which isn't difficult, but doesn't save us anything as you said.

In terms of performance, the algorithm skips the loop to find the intersection (which is O(n)), so possibly a bit faster, though not by much. Determining conflicting key combinations could be achieved by comparing the size before and after insert which is always constant, though additional constraints might apply.

Anyway, probably not worth arguing about.