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

qt: Replace deprecated QSignalMapper by lambda expressions #16706

Merged
merged 2 commits into from Aug 29, 2019

Conversation

@hebasto
Copy link
Member

commented Aug 24, 2019

The QSignalMapper class has been deprecated since Qt 5.10.

This PR replaces it by lambdas and does not change behavior.

qt: Remove QSignalMapper from RPCConsole
The QSignalMapper class is obsolete since Qt 5.10.
@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

@fanquake

This comment has been minimized.

Copy link
Member

commented Aug 24, 2019

QSignalMapper has actually been un-deprecated, see QSignalMapper deprecation and Un-deprecate QSignalMapper.

However Concept ACK assuming there's no change in behaviour and the new code is still compatible with Qt 5.5.1.

@hebasto

This comment has been minimized.

Copy link
Member Author

commented Aug 24, 2019

@fanquake

QSignalMapper has actually been un-deprecated, see QSignalMapper deprecation and Un-deprecate QSignalMapper.

I didn't know about that. Thank you.

From Un-deprecate QSignalMapper:

... Note that in most cases you can use lambdas for passing custom parameters to slots. This is less costly and will simplify the code.

FWIW, this PR is a part of my work on warnings raised during compiling on Bionic against Qt 5.13.

@practicalswift

This comment has been minimized.

Copy link
Member

commented Aug 24, 2019

Concept ACK -- more readable and less code

@promag
Copy link
Member

left a comment

Concept ACK.

Are the style changes really necessary? I'd prefer just dropping QSignalMapper.

qt: Remove QSignalMapper from TransactionView
The QSignalMapper class is obsolete since Qt 5.10.

@hebasto hebasto force-pushed the hebasto:20190824-obsolete-qsignalmapper branch from c4e40a4 to 0912134 Aug 25, 2019

@hebasto

This comment has been minimized.

Copy link
Member Author

commented Aug 25, 2019

@promag

Are the style changes really necessary? I'd prefer just dropping QSignalMapper.

Done.

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

Concept ACK

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2019

Gitian builds for commit db67101 (master):

Gitian builds for commit e05543d (master and this pull):

@jonasschnelli
Copy link
Member

left a comment

utACK 0912134

laanwj added a commit that referenced this pull request Aug 29, 2019
Merge #16706: qt: Replace deprecated QSignalMapper by lambda expressions
0912134 qt: Remove QSignalMapper from TransactionView (Hennadii Stepanov)
9e0c1d6 qt: Remove QSignalMapper from RPCConsole (Hennadii Stepanov)

Pull request description:

  The [`QSignalMapper`](https://doc.qt.io/qt-5/qsignalmapper.html) class has been [deprecated](https://doc-snapshots.qt.io/qt5-5.10/obsoleteclasses.html) since Qt 5.10.

  This PR replaces it by lambdas and does not change behavior.

ACKs for top commit:
  jonasschnelli:
    utACK 0912134

Tree-SHA512: 0c102d5cab4adc8b6252f72e07123ac87c65434c88cada3e72816ecea8fc4803f15b9c050fb5e1c7e8a96f709265521fd6813ab1890dbf5634032f7ee0d50675

@laanwj laanwj merged commit 0912134 into bitcoin:master Aug 29, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@hebasto hebasto deleted the hebasto:20190824-obsolete-qsignalmapper branch Aug 29, 2019

@Sjors

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

Post merge ACK. This totally gets rid of the flood of warnings on macOs with QT 5.13 from homebrew. It's also shorter and more readable. Tested that external explorer links still work, as well as banning peers.

Building with depends (QT 5.9.7) also works, though this still throws trillions of (unrelated) warnings.

sidhujag added a commit to syscoin/syscoin that referenced this pull request Aug 29, 2019
Merge bitcoin#16706: qt: Replace deprecated QSignalMapper by lambda e…
…xpressions

0912134 qt: Remove QSignalMapper from TransactionView (Hennadii Stepanov)
9e0c1d6 qt: Remove QSignalMapper from RPCConsole (Hennadii Stepanov)

Pull request description:

  The [`QSignalMapper`](https://doc.qt.io/qt-5/qsignalmapper.html) class has been [deprecated](https://doc-snapshots.qt.io/qt5-5.10/obsoleteclasses.html) since Qt 5.10.

  This PR replaces it by lambdas and does not change behavior.

ACKs for top commit:
  jonasschnelli:
    utACK 0912134

Tree-SHA512: 0c102d5cab4adc8b6252f72e07123ac87c65434c88cada3e72816ecea8fc4803f15b9c050fb5e1c7e8a96f709265521fd6813ab1890dbf5634032f7ee0d50675
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.