Use new Qt5 connect syntax #13529

pull promag wants to merge 2 commits into bitcoin:master from promag:2018-06-use-qt5-connect-syntax changing 34 files +324 −304
  1. 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).

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

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

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

    Thanks, I'll complete it then.

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

  6. fanquake added the label GUI on Jun 24, 2018
  7. fanquake added the label Refactoring on Jun 24, 2018
  8. jonasschnelli commented at 7:36 PM on June 26, 2018: contributor

    Concept ACK

  9. fanquake commented at 6:44 AM on July 1, 2018: member

    Concept ACK

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

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

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

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

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

  15. MarcoFalke added the label Up for grabs on Jul 21, 2018
  16. promag force-pushed on Jul 21, 2018
  17. promag force-pushed on Jul 21, 2018
  18. promag force-pushed on Jul 22, 2018
  19. promag force-pushed on Jul 22, 2018
  20. promag force-pushed on Jul 22, 2018
  21. promag renamed this:
    wip: Use new Qt5 connect syntax
    Use new Qt5 connect syntax
    on Jul 22, 2018
  22. promag commented at 12:47 AM on July 22, 2018: member

    @fanquake ready to review.

  23. promag commented at 12: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.

  24. MarcoFalke removed the label Up for grabs on Jul 22, 2018
  25. DrahtBot cross-referenced this on Jul 22, 2018 from issue Qt: Fix for bitcoin-qt becoming unresponsive during shutdown (issue #13217) by LeandroRocha84
  26. DrahtBot commented at 1:42 PM on July 22, 2018: contributor

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

  27. promag force-pushed on Jul 22, 2018
  28. promag force-pushed on Jul 23, 2018
  29. promag cross-referenced this on Jul 23, 2018 from issue [gui] Make proxy icon from statusbar clickable by mess110
  30. laanwj added this to the milestone 0.18.0 on Jul 23, 2018
  31. ken2812221 cross-referenced this on Jul 24, 2018 from issue [qt] TransactionView: highlight replacement tx after fee bump by Sjors
  32. DrahtBot cross-referenced this on Jul 24, 2018 from issue scripted-diff: Remove trailing whitespaces by promag
  33. DrahtBot commented at 11:22 AM on July 25, 2018: contributor

    <!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase

  34. DrahtBot added the label Needs rebase on Jul 25, 2018
  35. promag force-pushed on Jul 25, 2018
  36. promag commented at 1:25 PM on July 25, 2018: member

    Rebased.

  37. fanquake removed the label Needs rebase on Jul 25, 2018
  38. DrahtBot cross-referenced this on Jul 31, 2018 from issue More intuitive GUI settings behavior when -proxy is set by Sjors
  39. Sjors cross-referenced this on Aug 1, 2018 from issue Missing and unaccesible sync modal when using QT without wallet by Sjors
  40. Sjors cross-referenced this on Aug 1, 2018 from issue Unhide QT window doesn't work when hidden with ⌘ + H by Sjors
  41. 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.

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

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

  44. Sjors changes_requested
  45. 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.

  46. DrahtBot cross-referenced this on Aug 2, 2018 from issue gui: don't disable the sync overlay when wallet is disabled by fanquake
  47. DrahtBot cross-referenced this on Aug 14, 2018 from issue gui: When private key is disabled, only show watch-only balance by ken2812221
  48. promag commented at 2:24 PM on August 16, 2018: member

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

  49. laanwj added this to the "Blockers" column in a project
  50. promag force-pushed on Aug 19, 2018
  51. promag force-pushed on Aug 20, 2018
  52. promag commented at 12: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.

  53. Sjors commented at 4:32 PM on August 20, 2018: member

    Can confirm these issues are fixed in ac8661e.

  54. promag force-pushed on Aug 20, 2018
  55. promag commented at 5:08 PM on August 20, 2018: member

    Squashed and rebased. Thanks @Sjors!

  56. DrahtBot commented at 6:18 PM on August 20, 2018: contributor

    <!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase

  57. DrahtBot added the label Needs rebase on Aug 20, 2018
  58. 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

  59. qt: Use new Qt5 connect syntax f78558f1e3
  60. test: Add lint to prevent SIGNAL/SLOT connect style 3567b247f4
  61. promag force-pushed on Aug 21, 2018
  62. laanwj merged this on Aug 21, 2018
  63. laanwj closed this on Aug 21, 2018
  64. laanwj referenced this in commit af4fa72fac on Aug 21, 2018
  65. promag deleted the branch on Aug 21, 2018
  66. laanwj removed this from the "Blockers" column in a project
  67. promag cross-referenced this on Aug 29, 2018 from issue [RFC] gui: Minimum required Qt5 by fanquake
  68. Bushstar cross-referenced this on Sep 4, 2018 from issue commits from bitcoin/master by Bushstar
  69. ryanofsky cross-referenced this on Sep 26, 2018 from issue Add BitcoinApplication & RPCConsole tests by ryanofsky
  70. Fuzzbawls cross-referenced this on Oct 20, 2019 from issue [Qt] Switch to newer connect syntax by Fuzzbawls
  71. laanwj removed the label Needs rebase on Oct 24, 2019
  72. promag cross-referenced this on Mar 2, 2020 from issue wip: gui: Drop connectSlotsByName usage by promag
  73. random-zebra referenced this in commit 521d13b6f0 on Mar 23, 2020
  74. wqking referenced this in commit 2aad804552 on Aug 24, 2020
  75. linuxsh2 referenced this in commit 321cd6e256 on Aug 17, 2021
  76. linuxsh2 referenced this in commit 57e895a9c6 on Aug 23, 2021
  77. bitcoin locked this on Dec 16, 2021
Contributors
promagMarcoFalkelaanwjjonasschnellifanquakeDrahtBotSjors
Labels
GUIRefactoring

Milestone
0.18.0

Linked (view graph)
#11625 Add BitcoinApplication & RPCConsole tests#12818 [qt] TransactionView: highlight replacement tx after fee bump#13248 [gui] Make proxy icon from statusbar clickable#13478 [RFC] gui: Minimum required Qt5#13828 Missing and unaccesible sync modal when using QT without wallet#13829 Unhide QT window doesn't work when hidden with ⌘ + H#18246 wip: gui: Drop connectSlotsByName usage
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:55 UTC