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 commented at 1:42 PM on July 22, 2018: member

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

  26. promag force-pushed on Jul 22, 2018
  27. promag force-pushed on Jul 23, 2018
  28. laanwj added this to the milestone 0.18.0 on Jul 23, 2018
  29. DrahtBot commented at 11:22 AM on July 25, 2018: member

    <!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase

  30. DrahtBot added the label Needs rebase on Jul 25, 2018
  31. promag force-pushed on Jul 25, 2018
  32. promag commented at 1:25 PM on July 25, 2018: member

    Rebased.

  33. fanquake removed the label Needs rebase on Jul 25, 2018
  34. 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.

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

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

  37. Sjors changes_requested
  38. 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.

  39. promag commented at 2:24 PM on August 16, 2018: member

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

  40. laanwj added this to the "Blockers" column in a project
  41. promag force-pushed on Aug 19, 2018
  42. promag force-pushed on Aug 20, 2018
  43. 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.

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

    Can confirm these issues are fixed in ac8661e.

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

    Squashed and rebased. Thanks @Sjors!

  47. DrahtBot commented at 6:18 PM on August 20, 2018: member

    <!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase

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

  50. qt: Use new Qt5 connect syntax f78558f1e3
  51. test: Add lint to prevent SIGNAL/SLOT connect style 3567b247f4
  52. promag force-pushed on Aug 21, 2018
  53. laanwj merged this on Aug 21, 2018
  54. laanwj closed this on Aug 21, 2018
  55. laanwj referenced this in commit af4fa72fac on Aug 21, 2018
  56. promag deleted the branch on Aug 21, 2018
  57. laanwj removed this from the "Blockers" column in a project
  58. laanwj removed the label Needs rebase on Oct 24, 2019
  59. random-zebra referenced this in commit 521d13b6f0 on Mar 23, 2020
  60. wqking referenced this in commit 2aad804552 on Aug 24, 2020
  61. linuxsh2 referenced this in commit 321cd6e256 on Aug 17, 2021
  62. linuxsh2 referenced this in commit 57e895a9c6 on Aug 23, 2021
  63. DrahtBot locked this on Dec 16, 2021
Contributors
promagMarcoFalkelaanwjjonasschnellifanquakeDrahtBotSjors
Labels
GUIRefactoring

Milestone
0.18.0

Linked (view graph)
#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-04-13 21:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me