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

Use new Qt5 connect syntax #13529

Merged
merged 2 commits into from Aug 21, 2018

Conversation

Projects
None yet
7 participants
@promag
Copy link
Member

commented Jun 24, 2018

Pros&cons in https://wiki.qt.io/New_Signal_Slot_Syntax.

Note that connecting to/from overloaded slot/signal is ugly before qt 5.7 (see https://stackoverflow.com/a/16795664).

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jun 24, 2018

Concept ACK. The new syntax has compile time checks, removing the need to debug impossible to find syntax errors during run time.

@laanwj

This comment has been minimized.

Copy link
Member

commented Jun 24, 2018

Concept ACK (because of @MarcoFalke's reasoning)
Would make sense to change all of these at once.

@promag

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2018

Thanks, I'll complete it then.

@promag

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2018

@MarcoFalke @laanwj see new lint script, it can be improved later with other checks related to Qt. I don't know of a Qt way to prevent the old syntax.

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Jun 26, 2018

Concept ACK

1 similar comment
@fanquake

This comment has been minimized.

Copy link
Member

commented Jul 1, 2018

Concept ACK

@laanwj

This comment has been minimized.

Copy link
Member

commented Jul 5, 2018

see new lint script, it can be improved later with other checks related to Qt

LGTM

@laanwj

This comment has been minimized.

Copy link
Member

commented Jul 10, 2018

  • found a connection to a private slot that doesn't work with the new syntax.
  • connection to/from overloaded slot/signal is ugly before qt 5.7 (see https://stackoverflow.com/a/16795664)

does this mean it's better to postpone this to a (far) future in which we can drop pre-5.7 support?

@promag

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2018

does this mean it's better to postpone this to a (far) future in which we can drop pre-5.7 support?

Not really, until now just a couple of these where I have to cast the slot.

@laanwj

This comment has been minimized.

Copy link
Member

commented Jul 10, 2018

oh if it can be worked around and is just a couple of cases, I agree it's not a big issue, just need to be careful to test all behavior afterwards.

@fanquake

This comment has been minimized.

Copy link
Member

commented Jul 21, 2018

@promag What's the status of this? Would it be good to get into 0.17.0?

@promag promag force-pushed the promag:2018-06-use-qt5-connect-syntax branch 5 times, most recently Jul 21, 2018

@promag promag changed the title wip: Use new Qt5 connect syntax Use new Qt5 connect syntax Jul 22, 2018

@promag

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2018

@fanquake ready to review.

@promag

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2018

oh if it can be worked around and is just a couple of cases

@laanwj at the moment I count 25 cases.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2018

Note to reviewers: This pull request conflicts with the following ones:
  • #13966 (gui: When private key is disabled, only show watch-only balance by ken2812221)
  • #13848 (gui: don't disable the sync overlay when wallet is disabled by fanquake)
  • #13818 (More intuitive GUI settings behavior when -proxy is set by Sjors)
  • #13280 ([qt] Removed "Pay only the required fee" checkbox by GreatSock)
  • #13248 ([gui] Make proxy icon from statusbar clickable by mess110)
  • #13100 (gui: Add dynamic wallets support by promag)
  • #12818 ([qt] TransactionView: highlight replacement tx after fee bump by Sjors)
  • #9502 ([Qt] Add option to pause/resume block downloads by jonasschnelli)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@promag promag force-pushed the promag:2018-06-use-qt5-connect-syntax branch 2 times, most recently Jul 22, 2018

@laanwj laanwj added this to the 0.18.0 milestone Jul 23, 2018

@promag promag force-pushed the promag:2018-06-use-qt5-connect-syntax branch Jul 25, 2018

@promag

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2018

Rebased.

@Sjors
Copy link
Member

left a comment

Concept ACK. I'm a big fan of compile time checks. The new syntax also helps readability because you no longer have to guess the class name (e.g. QTableView) from the the variable name (e.g. ui->tableView).

Found a few problems with ba34538 on macOS 10.13.6 though (Qt 5.11.1 and 5.9.6):

  • application hides when I navigate to a new tab (e.g. from Overview to Transactions); maybe some event needs to be not propagated?
  • all console window commands cause a segfault, so much for compile time checks :-(

Other than that I tested most connect instances and they all seem to work.

I ran into a bunch of existing issues, none of which are end of the world, but they're a distraction when reviewing if you don't know about them.

src/qt/bitcoingui.cpp Outdated
connect(receiveCoinsMenuAction, SIGNAL(triggered()), this, SLOT(gotoReceiveCoinsPage()));
connect(historyAction, SIGNAL(triggered()), this, SLOT(showNormalIfMinimized()));
connect(historyAction, SIGNAL(triggered()), this, SLOT(gotoHistoryPage()));
connect(overviewAction, &QAction::triggered, this, &BitcoinGUI::showNormalIfMinimized);

This comment has been minimized.

Copy link
@Sjors

Sjors Aug 1, 2018

Member

I don't think you can actually reach this when the application is hidden, but that can be addressed another time.

src/qt/overviewpage.cpp Outdated
connect(ui->labelWalletStatus, SIGNAL(clicked()), this, SLOT(handleOutOfSyncWarningClicks()));
connect(ui->labelTransactionsStatus, SIGNAL(clicked()), this, SLOT(handleOutOfSyncWarningClicks()));
connect(ui->labelWalletStatus, &QPushButton::clicked, this, &OverviewPage::handleOutOfSyncWarningClicks);
connect(ui->labelWalletStatus, &QPushButton::clicked, this, &OverviewPage::handleOutOfSyncWarningClicks);

This comment has been minimized.

Copy link
@Sjors

Sjors Aug 1, 2018

Member

Duplicate line

This comment has been minimized.

Copy link
@promag

promag Aug 19, 2018

Author Member

Removed.

connect(editLabelAction, &QAction::triggered, this, &TransactionView::editLabel);
connect(showDetailsAction, &QAction::triggered, this, &TransactionView::showDetails);
// Double-clicking on a transaction on the transaction history page shows details
connect(this, &TransactionView::doubleClicked, this, &TransactionView::showDetails);

This comment has been minimized.

Copy link
@Sjors

Sjors Aug 1, 2018

Member

Note for other reviewers: this was moved from src/qt/walletview.cpp

@promag

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2018

@Sjors thanks for the review, I'll rebase and address your comments!

@laanwj laanwj added this to Blockers in High-priority for review Aug 16, 2018

@promag promag force-pushed the promag:2018-06-use-qt5-connect-syntax branch 2 times, most recently Aug 19, 2018

@promag

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2018

all console window commands cause a segfault, so much for compile time checks :-(

@Sjors fixed in 5261ab0.

application hides when I navigate to a new tab (e.g. from Overview to Transactions); maybe some event needs to be not propagated?

Fixed in ac8661e.

@Sjors

This comment has been minimized.

Copy link
Member

commented Aug 20, 2018

Can confirm these issues are fixed in ac8661e.

@promag promag force-pushed the promag:2018-06-use-qt5-connect-syntax branch Aug 20, 2018

@promag

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2018

Squashed and rebased. Thanks @Sjors!

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2018

Needs rebase
@laanwj

This comment has been minimized.

Copy link
Member

commented Aug 21, 2018

ehm... sorry, needs rebase again, if the remaining issues are solved we should merge this

@promag promag force-pushed the promag:2018-06-use-qt5-connect-syntax branch to 3567b24 Aug 21, 2018

@laanwj laanwj merged commit 3567b24 into bitcoin:master Aug 21, 2018

2 checks passed

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

laanwj added a commit that referenced this pull request Aug 21, 2018

Merge #13529: Use new Qt5 connect syntax
3567b24 test: Add lint to prevent SIGNAL/SLOT connect style (João Barbosa)
f78558f qt: Use new Qt5 connect syntax (João Barbosa)

Pull request description:

  Pros&cons in https://wiki.qt.io/New_Signal_Slot_Syntax.

  Note that connecting to/from overloaded slot/signal is ugly before qt 5.7 (see https://stackoverflow.com/a/16795664).

Tree-SHA512: ab81f035099fecd34be546f7091bc29595349f2fd0fea26f6414242702955fca27faa4fe19ebfe105c01217908b51db762cb5a9f6ce25bc5e8e6f64c77428c22

@promag promag deleted the promag:2018-06-use-qt5-connect-syntax branch Aug 21, 2018

@laanwj laanwj removed this from Blockers in High-priority for review Aug 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.