Skip to content
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

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

Merged
merged 1 commit into from Apr 19, 2022

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Apr 9, 2022

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.

Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK d543515

  • 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.

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

@hebasto
Copy link
Member Author

hebasto commented Apr 10, 2022

Updated d543515 -> 8691a44 (pr580.01 -> pr580.02, diff):

Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reACK 8691a44

Changes since my last review:

  • Declared a type variable beforehand and used it instead of keyevt→type().

src/qt/rpcconsole.cpp Outdated Show resolved Hide resolved
@hebasto hebasto added the Qt 6 Transition to Qt 6 label Apr 12, 2022
@luke-jr
Copy link
Member

luke-jr commented Apr 13, 2022

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...

@hebasto
Copy link
Member Author

hebasto commented Apr 13, 2022

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.

@luke-jr
Copy link
Member

luke-jr commented Apr 13, 2022

Hmm, how about using https://doc.qt.io/qt-5/qcoreapplication.html#sendEvent ?

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).
@hebasto
Copy link
Member Author

hebasto commented Apr 13, 2022

Updated 8691a44 -> 3ec6504 (pr580.02 -> pr580.03, diff):

Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK 3ec6504 on Ubuntu 21.10, Qt 5.15.2

Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reACK 3ec6504

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:

@hebasto hebasto merged commit 37e49cc into bitcoin-core:master Apr 19, 2022
@hebasto hebasto deleted the 220409-event branch April 19, 2022 17:33
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 19, 2022
…QKeyEvent` copy constructor

3ec6504 qt: Do not use `QKeyEvent` copy constructor (Hennadii Stepanov)

Pull request description:

  This PR is preparation for [Qt 6](bitcoin#24798), and it fixes an experimental build with Qt 6.2.4 as copying of `QEvent` has been [disabled](qt/qtbase@19f9b0d) in Qt 6.0.0.

ACKs for top commit:
  w0xlt:
    tACK bitcoin-core/gui@3ec6504 on Ubuntu 21.10, Qt 5.15.2
  shaavan:
    reACK 3ec6504

Tree-SHA512: 583a9dad0c621d9f02f77ccaa9f55ee79e12e3c47f418911ef2dfe0de357d772d1928ae3ec19b6f0c0674da858bab9d4542a26cc14b06ed921370dfeabd1c194
@bitcoin-core bitcoin-core locked and limited conversation to collaborators Apr 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Qt 6 Transition to Qt 6
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants