Use new Qt5 connect syntax #13529

promag · 2018-06-24T15:19:14Z

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

maflcko · 2018-06-24T15:35:27Z

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

laanwj · 2018-06-24T15:40:37Z

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

promag · 2018-06-24T16:11:20Z

Thanks, I'll complete it then.

promag · 2018-06-24T17:15:41Z

@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 · 2018-06-26T19:36:26Z

Concept ACK

fanquake · 2018-07-01T06:44:03Z

Concept ACK

laanwj · 2018-07-05T17:10:16Z

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

LGTM

laanwj · 2018-07-10T15:11:09Z

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 · 2018-07-10T15:18:35Z

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 · 2018-07-10T15:38:39Z

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 · 2018-07-21T05:27:16Z

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

promag · 2018-07-22T00:47:06Z

@fanquake ready to review.

promag · 2018-07-22T00:49:54Z

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

@laanwj at the moment I count 25 cases.

DrahtBot · 2018-07-22T13:42:23Z

Note to reviewers: This pull request conflicts with the following ones:

gui: When private key is disabled, only show watch-only balance #13966 (gui: When private key is disabled, only show watch-only balance by ken2812221)
gui: don't disable the sync overlay when wallet is disabled #13848 (gui: don't disable the sync overlay when wallet is disabled by fanquake)
More intuitive GUI settings behavior when -proxy is set #13818 (More intuitive GUI settings behavior when -proxy is set by Sjors)
[qt] Removed "Pay only the required fee" checkbox #13280 ([qt] Removed "Pay only the required fee" checkbox by GreatSock)
[gui] Make proxy icon from statusbar clickable #13248 ([gui] Make proxy icon from statusbar clickable by mess110)
gui: Add dynamic wallets support #13100 (gui: Add dynamic wallets support by promag)
[qt] TransactionView: highlight replacement tx after fee bump #12818 ([qt] TransactionView: highlight replacement tx after fee bump by Sjors)
[Qt] Add option to pause/resume block downloads #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.

Sjors

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.

Sjors · 2018-08-01T14:27:28Z

src/qt/bitcoingui.cpp

-    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);


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

Sjors · 2018-08-01T15:13:46Z

src/qt/overviewpage.cpp

-    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);


Duplicate line

Sjors · 2018-08-01T15:33:26Z

src/qt/transactionview.cpp

+    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);


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

promag · 2018-08-16T14:24:14Z

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

promag · 2018-08-20T00:34:10Z

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 · 2018-08-20T16:32:11Z

Can confirm these issues are fixed in ac8661e.

promag · 2018-08-20T17:08:50Z

Squashed and rebased. Thanks @Sjors!

DrahtBot · 2018-08-20T18:18:28Z

Needs rebase

laanwj · 2018-08-21T08:17:23Z

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

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

c54fd9d [Qt] Switch to newer connect syntax (Fuzzbawls) Pull request description: Switch all Qt connections to the newer functor-based syntax. Marked [WIP] for now until larger PRs can be merged. Mostly ported from bitcoin#13529 Requires the following PR to be merged first in order to adhere to standards: - [x] #1351 ACKs for top commit: furszy: ACK c54fd9d random-zebra: ACK c54fd9d and merging... Tree-SHA512: d5222264566a8920ab97e8c586f134156d9d0514d014b430bdd47a34b0620ab94a1c6d341b6a57322f1131a8fad91a5e1af907f3bd7112d0ab5e04c4cc9f5ae5

c54fd9dfded28f2b4ff86914422fb592bd86f320 [Qt] Switch to newer connect syntax (Fuzzbawls) Pull request description: Switch all Qt connections to the newer functor-based syntax. Marked [WIP] for now until larger PRs can be merged. Mostly ported from bitcoin/bitcoin#13529 Requires the following PR to be merged first in order to adhere to standards: - [x] #1351 ACKs for top commit: furszy: ACK c54fd9d random-zebra: ACK c54fd9dfded28f2b4ff86914422fb592bd86f320 and merging... Tree-SHA512: d5222264566a8920ab97e8c586f134156d9d0514d014b430bdd47a34b0620ab94a1c6d341b6a57322f1131a8fad91a5e1af907f3bd7112d0ab5e04c4cc9f5ae5 # Conflicts: # .travis/lint_06_script.sh # src/qt/optionsdialog.cpp # src/qt/rpcconsole.cpp # src/qt/vitae.cpp # src/qt/vitae/navmenuwidget.cpp # src/qt/vitae/privacywidget.cpp # src/qt/vitae/send.cpp

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

fanquake added GUI Refactoring labels Jun 24, 2018

maflcko added the Up for grabs label Jul 21, 2018

promag force-pushed the 2018-06-use-qt5-connect-syntax branch 5 times, most recently from 42f59ce to 4d54e33 Compare July 22, 2018 00:41

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

maflcko removed the Up for grabs label Jul 22, 2018

DrahtBot mentioned this pull request Jul 22, 2018

Qt: Fix for bitcoin-qt becoming unresponsive during shutdown (issue #13217) #13674

Closed

promag force-pushed the 2018-06-use-qt5-connect-syntax branch 2 times, most recently from 9443c85 to 9813bf1 Compare July 23, 2018 09:54

promag mentioned this pull request Jul 23, 2018

[gui] Make proxy icon from statusbar clickable #13248

Merged

laanwj added this to the 0.18.0 milestone Jul 23, 2018

ken2812221 mentioned this pull request Jul 24, 2018

[qt] TransactionView: highlight replacement tx after fee bump #12818

Merged

Sjors suggested changes Aug 1, 2018

View reviewed changes

DrahtBot mentioned this pull request Aug 2, 2018

gui: don't disable the sync overlay when wallet is disabled #13848

Closed

DrahtBot mentioned this pull request Aug 14, 2018

gui: When private key is disabled, only show watch-only balance #13966

Merged

promag force-pushed the 2018-06-use-qt5-connect-syntax branch 2 times, most recently from de93a06 to ac8661e Compare August 20, 2018 00:31

promag force-pushed the 2018-06-use-qt5-connect-syntax branch from ac8661e to 01b5c26 Compare August 20, 2018 17:07

DrahtBot added the Needs rebase label Aug 20, 2018

promag added 2 commits August 21, 2018 09:43

qt: Use new Qt5 connect syntax

f78558f

test: Add lint to prevent SIGNAL/SLOT connect style

3567b24

promag force-pushed the 2018-06-use-qt5-connect-syntax branch from 01b5c26 to 3567b24 Compare August 21, 2018 08:45

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

promag deleted the 2018-06-use-qt5-connect-syntax branch August 21, 2018 09:34

promag mentioned this pull request Aug 29, 2018

[RFC] gui: Minimum required Qt5 #13478

Closed

Bushstar mentioned this pull request Sep 4, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#359

Merged

ryanofsky mentioned this pull request Sep 26, 2018

Add BitcoinApplication & RPCConsole tests #11625

Merged

Fuzzbawls mentioned this pull request Oct 20, 2019

[Qt] Switch to newer connect syntax PIVX-Project/PIVX#1049

Merged

1 task

laanwj removed the Needs rebase label Oct 24, 2019

promag mentioned this pull request Mar 2, 2020

wip: gui: Drop connectSlotsByName usage #18246

Closed

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Use new Qt5 connect syntax #13529

Use new Qt5 connect syntax #13529

Uh oh!

Conversation

promag commented Jun 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jun 24, 2018

Uh oh!

laanwj commented Jun 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Jun 24, 2018

Uh oh!

promag commented Jun 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented Jun 26, 2018

Uh oh!

fanquake commented Jul 1, 2018

Uh oh!

laanwj commented Jul 5, 2018

Uh oh!

laanwj commented Jul 10, 2018

Uh oh!

promag commented Jul 10, 2018

Uh oh!

laanwj commented Jul 10, 2018

Uh oh!

fanquake commented Jul 21, 2018

Uh oh!

promag commented Jul 22, 2018

Uh oh!

promag commented Jul 22, 2018

Uh oh!

DrahtBot commented Jul 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors Aug 1, 2018

Choose a reason for hiding this comment

Uh oh!

Sjors Aug 1, 2018

Choose a reason for hiding this comment

Uh oh!

promag Aug 19, 2018

Choose a reason for hiding this comment

Uh oh!

Sjors Aug 1, 2018

Choose a reason for hiding this comment

Uh oh!

promag commented Aug 16, 2018

Uh oh!

promag commented Aug 20, 2018

Uh oh!

Sjors commented Aug 20, 2018

Uh oh!

promag commented Aug 20, 2018

Uh oh!

DrahtBot commented Aug 20, 2018

Uh oh!

laanwj commented Aug 21, 2018

Uh oh!

Uh oh!

promag commented Jun 24, 2018 •

edited

Loading

laanwj commented Jun 24, 2018 •

edited

Loading

promag commented Jun 24, 2018 •

edited

Loading

DrahtBot commented Jul 22, 2018 •

edited

Loading

Sjors left a comment •

edited

Loading