Getting ready to Qt 6 (3/n). Do not use QKeyEvent copy constructor #580

1. 
   hebasto commented at 8:17 am on April 9, 2022: member

   This PR is preparation for Qt 6, and it fixes an experimental build with Qt 6.2.4 as copying of QEvent has been disabled in Qt 6.0.0.
2. shaavan approved
3. 
   shaavan commented at 5:43 pm on April 10, 2022: contributor

   ACK d543515e635786214215e5b30a2874c4e8a676dc

   • I was able to verify that with Qt 6, the copying of QEvent has been disabled. See for reference {second and third paragraph}.
   • Therefore, instead of copying the key event, this PR uses its data to create a new event.

   nit:

   As a minor nit, I would suggest creating a variable type instead of calling keyevt→type() multiple times.

   0QEvent::Type type = keyevt->type()
   

4. hebasto force-pushed on Apr 10, 2022
5. 
   hebasto commented at 8:13 pm on April 10, 2022: member

   Updated d543515e635786214215e5b30a2874c4e8a676dc -> 8691a444add29d62dade34bf2965cf76a5d6c863 (pr580.01 -> pr580.02, diff):

   • addressed @shaavan’s comment
6. in src/qt/rpcconsole.cpp:614 in 8691a444ad outdated

   609@@ -610,6 +610,7 @@ bool RPCConsole::eventFilter(QObject* obj, QEvent *event)
   610     if(event->type() == QEvent::KeyPress) // Special key handling
   611     {
   612         QKeyEvent *keyevt = static_cast<QKeyEvent*>(event);
   613+        const auto type = keyevt->type();
   614         int key = keyevt->key();
   615         Qt::KeyboardModifiers mod = keyevt->modifiers();
   

   ---

   ---

   

   shaavan commented at 7:48 am on April 11, 2022:

   Since we are refactoring this part of the code, how about we initialize the key and mod variables as const since they are unchanged in their lifetime.

   0        const int key = keyevt->key();
   1        const auto mod = keyevt->modifiers();
   

   ---

   

   hebasto commented at 8:13 am on April 11, 2022:

   Ok, I’ll implement this suggestion if/when other changes will be required.
7. shaavan approved
8. 
   shaavan commented at 7:48 am on April 11, 2022: contributor

   reACK 8691a444add29d62dade34bf2965cf76a5d6c863

   Changes since my last review:

   • Declared a type variable beforehand and used it instead of keyevt→type().
9. hebasto added the label Qt 6 on Apr 12, 2022
10. 
    luke-jr commented at 3:04 am on April 13, 2022: member

    Why are we creating a new event instead of forwarding the original one on? Seems like this PR would simply be bypassing the behaviour Qt is trying to prevent…
11. 
    hebasto commented at 8:05 am on April 13, 2022: member

    Why are we creating a new event instead of forwarding the original one on? Seems like this PR would simply be bypassing the behaviour Qt is trying to prevent…

    QCoreApplication::postEvent:

    The event must be allocated on the heap since the post event queue will take ownership of the event and delete it once it has been posted. It is not safe to access the event after it has been posted.

    “forwarding the original one on” ends with double deletion of an QKeyEvent instance.

12. 
    luke-jr commented at 4:51 pm on April 13, 2022: member

    Hmm, how about using https://doc.qt.io/qt-5/qcoreapplication.html#sendEvent ?
13. qt: Do not use `QKeyEvent` copy constructor

    This change is preparation for Qt 6, and it fixes an experimental build
    with Qt 6.2.4 as copying of `QEvent` has been disabled in Qt 6.0.0 (see
    19f9b0d5f54379151eb71e98555b203ad6756276 upstream commit).

    3ec6504a2e
14. hebasto force-pushed on Apr 13, 2022
15. 
    hebasto commented at 6:57 pm on April 13, 2022: member

    Updated 8691a444add29d62dade34bf2965cf76a5d6c863 -> 3ec6504a2e5b4afb7a2719a82191e0b96fe23214 (pr580.02 -> pr580.03, diff):

    • addressed @luke-jr’s comment
16. w0xlt approved
17. 
    w0xlt commented at 10:14 pm on April 18, 2022: contributor

    tACK https://github.com/bitcoin-core/gui/pull/580/commits/3ec6504a2e5b4afb7a2719a82191e0b96fe23214 on Ubuntu 21.10, Qt 5.15.2
18. shaavan approved
19. 
    shaavan commented at 12:14 pm on April 19, 2022: contributor

    reACK 3ec6504a2e5b4afb7a2719a82191e0b96fe23214

    Changes since my last review:

    • Instead of creating a new event for each call of postEvent, send Event is used with keyevt event.

    Since sendEvent does not delete the event argument after the completion of its execution, the same keyevt could be used multiple times instead of creating a separate event for each instance of using postEvent. Therefore I agree with this change.

    References:

    • postEvent: https://doc.qt.io/qt-6/qcoreapplication.html#postEvent
    • sendEvent: https://doc.qt.io/qt-6/qcoreapplication.html#sendEvent
    • Comparison between the two: https://doc.qt.io/archives/qq/qq11-events.html (under section Synthetic Events)
20. hebasto merged this on Apr 19, 2022
21. hebasto closed this on Apr 19, 2022

    ---

22. hebasto deleted the branch on Apr 19, 2022
23. sidhujag referenced this in commit 1fcb269d44 on Apr 19, 2022
24. bitcoin-core locked this on Apr 19, 2023

Contributors
hebasto shaavan luke-jr w0xlt

Labels
Qt 6