Use new Qt5 connect syntax

promag commented at 3:19 pm on June 24, 2018: member

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 commented at 3:35 pm on June 24, 2018: member

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

laanwj commented at 3:40 pm on June 24, 2018: member

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

promag commented at 4:11 pm on June 24, 2018: member

Thanks, I’ll complete it then.

promag commented at 5:15 pm on June 24, 2018: member

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

fanquake added the label GUI on Jun 24, 2018

fanquake added the label Refactoring on Jun 24, 2018

jonasschnelli commented at 7:36 pm on June 26, 2018: contributor

Concept ACK

fanquake commented at 6:44 am on July 1, 2018: member

Concept ACK

laanwj commented at 5:10 pm on July 5, 2018: member

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

LGTM

laanwj commented at 3:11 pm on July 10, 2018: member

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 commented at 3:18 pm on July 10, 2018: member

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 commented at 3:38 pm on July 10, 2018: member

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 commented at 5:27 am on July 21, 2018: member

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

MarcoFalke added the label Up for grabs on Jul 21, 2018

promag force-pushed on Jul 21, 2018

promag force-pushed on Jul 22, 2018

promag renamed this:
~~wip: Use new Qt5 connect syntax~~
Use new Qt5 connect syntax
on Jul 22, 2018

promag commented at 0:47 am on July 22, 2018: member

@fanquake ready to review.

promag commented at 0:49 am on July 22, 2018: member

oh if it can be worked around and is just a couple of cases @laanwj at the moment I count 25 cases.

MarcoFalke removed the label Up for grabs on Jul 22, 2018

DrahtBot commented at 1:42 pm on July 22, 2018: member

#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 force-pushed on Jul 22, 2018

promag force-pushed on Jul 23, 2018

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

DrahtBot commented at 11:22 am on July 25, 2018: member

DrahtBot added the label Needs rebase on Jul 25, 2018

promag force-pushed on Jul 25, 2018

promag commented at 1:25 pm on July 25, 2018: member

Rebased.

fanquake removed the label Needs rebase on Jul 25, 2018

in src/qt/bitcoingui.cpp:268 in ba34538e86 outdated

275-    connect(receiveCoinsAction, SIGNAL(triggered()), this, SLOT(gotoReceiveCoinsPage()));
276-    connect(receiveCoinsMenuAction, SIGNAL(triggered()), this, SLOT(showNormalIfMinimized()));
277-    connect(receiveCoinsMenuAction, SIGNAL(triggered()), this, SLOT(gotoReceiveCoinsPage()));
278-    connect(historyAction, SIGNAL(triggered()), this, SLOT(showNormalIfMinimized()));
279-    connect(historyAction, SIGNAL(triggered()), this, SLOT(gotoHistoryPage()));
280+    connect(overviewAction, &QAction::triggered, this, &BitcoinGUI::showNormalIfMinimized);

Sjors commented at 2:27 pm on August 1, 2018:

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

in src/qt/overviewpage.cpp:141 in ba34538e86 outdated

139     // start with displaying the "out of sync" warnings
140     showOutOfSyncWarning(true);
141-    connect(ui->labelWalletStatus, SIGNAL(clicked()), this, SLOT(handleOutOfSyncWarningClicks()));
142-    connect(ui->labelTransactionsStatus, SIGNAL(clicked()), this, SLOT(handleOutOfSyncWarningClicks()));
143+    connect(ui->labelWalletStatus, &QPushButton::clicked, this, &OverviewPage::handleOutOfSyncWarningClicks);
144+    connect(ui->labelWalletStatus, &QPushButton::clicked, this, &OverviewPage::handleOutOfSyncWarningClicks);

Sjors commented at 3:13 pm on August 1, 2018:

Duplicate line

promag commented at 10:59 pm on August 19, 2018:

Removed.

in src/qt/transactionview.cpp:203 in ba34538e86 outdated

220+    connect(copyTxHexAction, &QAction::triggered, this, &TransactionView::copyTxHex);
221+    connect(copyTxPlainText, &QAction::triggered, this, &TransactionView::copyTxPlainText);
222+    connect(editLabelAction, &QAction::triggered, this, &TransactionView::editLabel);
223+    connect(showDetailsAction, &QAction::triggered, this, &TransactionView::showDetails);
224+    // Double-clicking on a transaction on the transaction history page shows details
225+    connect(this, &TransactionView::doubleClicked, this, &TransactionView::showDetails);

Sjors commented at 3:33 pm on August 1, 2018:

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

Sjors changes_requested

Sjors commented at 3:39 pm on August 1, 2018: member

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.

promag commented at 2:24 pm on August 16, 2018: member

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

laanwj added this to the "Blockers" column in a project

promag force-pushed on Aug 19, 2018

promag force-pushed on Aug 20, 2018

promag commented at 0:34 am on August 20, 2018: member

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 commented at 4:32 pm on August 20, 2018: member

Can confirm these issues are fixed in ac8661e.

promag force-pushed on Aug 20, 2018

promag commented at 5:08 pm on August 20, 2018: member

Squashed and rebased. Thanks @Sjors!

DrahtBot commented at 6:18 pm on August 20, 2018: member

DrahtBot added the label Needs rebase on Aug 20, 2018

laanwj commented at 8:17 am on August 21, 2018: member

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

qt: Use new Qt5 connect syntax f78558f1e3

test: Add lint to prevent SIGNAL/SLOT connect style 3567b247f4

promag force-pushed on Aug 21, 2018

laanwj merged this on Aug 21, 2018

laanwj closed this on Aug 21, 2018

laanwj referenced this in commit af4fa72fac on Aug 21, 2018

promag deleted the branch on Aug 21, 2018

laanwj removed this from the "Blockers" column in a project

laanwj removed the label Needs rebase on Oct 24, 2019

random-zebra referenced this in commit 521d13b6f0 on Mar 23, 2020

wqking referenced this in commit 2aad804552 on Aug 24, 2020

linuxsh2 referenced this in commit 321cd6e256 on Aug 17, 2021

linuxsh2 referenced this in commit 57e895a9c6 on Aug 23, 2021

DrahtBot locked this on Dec 16, 2021

Use new Qt5 connect syntax #13529