Bugfix: GUI: Restore SendConfirmationDialog button default to "Yes" #17463

pull luke-jr wants to merge 3 commits into bitcoin:master from luke-jr:gui_custom_sendyes changing 4 files +35 −21
  1. luke-jr commented at 5:49 AM on November 13, 2019: member

    The SendConfirmationDialog is used for bumping the fee, where "Send" doesn't really make sense

    Customisation of the dialog is also needed for #16944 and #15987 so might as well use it for this too.

  2. fanquake added the label GUI on Nov 13, 2019
  3. DrahtBot commented at 5:54 AM on November 13, 2019: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #18789 (qt: Add Create Unsigned button to SendConfirmationDialog by achow101)

    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.

  4. laanwj commented at 11:50 AM on November 14, 2019: member

    The SendConfirmationDialog is used for bumping the fee, where "Send" doesn't really make sense

    I think it still does make some sense, as in 'send bumped transaction'. This adds a lot of complexity just to set a button text.

  5. luke-jr commented at 1:50 PM on November 14, 2019: member

    The prompt could be rephrased to make sense with "Send", but IMO it currently isn't.

    More than just button text is needed for #15987 so might as well just get the interface change over with all at once.

  6. in src/qt/sendcoinsdialog.cpp:920 in d6adf3bc2d outdated 
     891 | +    : QMessageBox(parent),
 892 | +    m_yes_button_text(yes_button_text),
 893 | +    secDelay(_secDelay)
 894 |  {
 895 | -    setIcon(QMessageBox::Question);
 896 | +    setIcon(icon);
    promag commented at 9:34 AM on November 15, 2019:

    Instead of another constructor argument, let the caller change the icon? Like

    SendConfirmationDialog confirmationDialog(tr("Confirm fee bump"), ...);
confirmationDialog.setIcon(QMessageBox::Question);
    promag commented at 11:09 AM on January 3, 2020:

    No reply?

    luke-jr commented at 5:06 PM on January 3, 2020:

    I don't know that it makes sense to create a SCD without a chosen icon...

    promag commented at 5:10 PM on January 3, 2020:

    I'd be happy to have QMessageBox::Question by default, and the caller is free to change.

    luke-jr commented at 5:30 PM on January 3, 2020:

    Ok, done

  7. promag commented at 9:34 AM on November 15, 2019: member

    Concept ACK.

  8. in src/qt/sendcoinsdialog.cpp:961 in 6721cb8412 outdated 
     904 | +    setStandardButtons(yes_button | cancel_button);
 905 | +
 906 | +    // We need to ensure the buttons have Yes/No roles, or they'll get ordered weird
 907 | +    QAbstractButton * const cancel_button_obj = button(cancel_button);
 908 | +    removeButton(cancel_button_obj);
 909 | +    addButton(cancel_button_obj, QMessageBox::NoRole);
    Sjors commented at 4:00 PM on November 15, 2019:

    Why QMessageBox::NoRole instead of QMessageBox::Cancel?

    luke-jr commented at 4:37 PM on November 15, 2019: 
        // We need to ensure the buttons have Yes/No roles, or they'll get ordered weird
    Sjors commented at 4:44 PM on November 15, 2019:

    That's not very clear; NoRole is neither Yes or No.

    Sjors commented at 4:59 PM on November 15, 2019:

    Operating systems have different rules about which button needs to be on the left (I don't care too deeply though). Is that the "weird" ordering you're seeing?

    luke-jr commented at 5:10 PM on November 15, 2019:

    https://doc.qt.io/qt-5/qmessagebox.html

    NoRole The button is a "No"-like button.

    Yes, it has to do with which button is left/right.

    Sjors commented at 5:59 PM on November 15, 2019:

    Oh wait, I miss-read NoRole as having no role :-)

  9. Sjors approved
  10. Sjors commented at 4:14 PM on November 15, 2019: member

    ACK d6adf3bc2db17ed3a17caa2ac5cf9095d70b53fb

    The simplification of retval could be its own commit.

  11. jonasschnelli commented at 12:07 AM on November 16, 2019: contributor

    Tested ACK d6adf3bc2db17ed3a17caa2ac5cf9095d70b53fb

    Before: <img width="451" alt="Bildschirmfoto 2019-11-15 um 14 04 47" src="https://user-images.githubusercontent.com/178464/68983816-f0929c00-07b0-11ea-84b8-49251b7f5b16.png"> <img width="585" alt="Bildschirmfoto 2019-11-15 um 13 57 28" src="https://user-images.githubusercontent.com/178464/68983822-f4262300-07b0-11ea-9723-67da2696862b.png">

    With this PR: <img width="451" alt="Bildschirmfoto 2019-11-15 um 14 04 04" src="https://user-images.githubusercontent.com/178464/68983829-01431200-07b1-11ea-8f41-e5679c7bd393.png">

    <img width="585" alt="Bildschirmfoto 2019-11-15 um 14 00 05" src="https://user-images.githubusercontent.com/178464/68983857-29cb0c00-07b1-11ea-99d7-750244effd26.png">

  12. Sjors cross-referenced this on Nov 18, 2019 from issue gui: save and load PSBT by Sjors
  13. DrahtBot added the label Needs rebase on Nov 22, 2019
  14. Sjors commented at 8:39 AM on November 23, 2019: member

    Needs a small rebase due to #16944 getting merged.

  15. in src/qt/sendcoinsdialog.cpp:956 in d6adf3bc2d outdated 
     899 |      setInformativeText(informative_text);
 900 |      setDetailedText(detailed_text);
 901 | -    setStandardButtons(QMessageBox::Yes | QMessageBox::Cancel);
 902 | -    setDefaultButton(QMessageBox::Cancel);
 903 | -    yesButton = button(QMessageBox::Yes);
 904 | +    setStandardButtons(yes_button | cancel_button);
    gwillen commented at 12:01 AM on December 14, 2019:

    Does this line actually do anything, if we subsequently remove both buttons and re-add them? Can we just call addButton twice to the same effect, and not have to remove them first?

    Sjors commented at 9:59 AM on January 2, 2020:

    @gwillen I think that has something to do with ordering: #17463 (review)

    promag commented at 10:57 AM on January 3, 2020:

    This is required so that the buttons are instanced otherwise button() returns nullptr.

  16. gwillen commented at 12:04 AM on December 14, 2019: contributor

    utACK; I'm looking to build on #17509 to which this is mentioned as a prereq, so if this is not too controversial I'd love to see it go in soon. It seems pretty small.

  17. luke-jr force-pushed on Jan 3, 2020
  18. luke-jr commented at 3:43 AM on January 3, 2020: member

    Rebased

  19. DrahtBot removed the label Needs rebase on Jan 3, 2020
  20. Sjors commented at 7:57 AM on January 3, 2020: member

    As @gwillen points out inline, I'm able to drop this entire code block with no visible change. At least not on macOS with QT 5.13.2:

        // We need to ensure the buttons have Yes/No roles, or they'll get ordered weird
    QAbstractButton * const cancel_button_obj = button(cancel_button);
    removeButton(cancel_button_obj);
    addButton(cancel_button_obj, QMessageBox::NoRole);
    setEscapeButton(cancel_button_obj);
    setDefaultButton(cancel_button);

    <img width="345" alt="Schermafbeelding 2020-01-03 om 15 51 27" src="https://user-images.githubusercontent.com/10217/71712726-85288a80-2e41-11ea-8347-35286599ed72.png">

    Otherwise 350234028786a139b7faf08655f27d97c9ff66e8 looks good.

  21. in src/qt/sendcoinsdialog.cpp:978 in 3502340287 outdated 
     942 | @@ -935,7 +943,8 @@ int SendConfirmationDialog::exec()
 943 |  {
 944 |      updateYesButton();
 945 |      countDownTimer.start(1000);
 946 | -    return QMessageBox::exec();
 947 | +    QMessageBox::exec();
 948 | +    return (buttonRole(clickedButton()) == QMessageBox::YesRole);
    promag commented at 11:16 AM on January 3, 2020:

    Just noting that clickedButton() can return nullptr - in that case buttonRole(nullptr) returns InvalidRole so this is safe.

    I wonder why not compare the exec() return agains QMessageBox::Yes?

    luke-jr commented at 5:21 PM on January 3, 2020:

    The YesRole button might not be Yes.

    luke-jr commented at 5:21 PM on January 3, 2020:

    And no, according to the Qt documentation, it can't return nullptr...

    promag commented at 5:23 PM on January 3, 2020:

    But it can? https://doc.qt.io/qt-5/qmessagebox.html#clickedButton

    promag commented at 5:25 PM on January 3, 2020:

    Oh because of setEscapeButton?

    luke-jr commented at 5:31 PM on January 3, 2020:

    Right

  22. promag commented at 11:19 AM on January 3, 2020: member

    Sorry but I really dislike the "super" constructor - I don't see how that's preferable over calling a couple of setters, which is more expressive and obvious.

    It's also unfortunately that it needs to remove/add buttons to fix the order.

  23. DrahtBot cross-referenced this on Feb 11, 2020 from issue "PSBT Operations" dialog by gwillen
  24. DrahtBot cross-referenced this on Feb 11, 2020 from issue gui: Fix manual coin control with multiple wallets loaded by promag
  25. DrahtBot cross-referenced this on Feb 11, 2020 from issue build: Enable -Wsuggest-override if available by hebasto
  26. DrahtBot cross-referenced this on Feb 22, 2020 from issue UI external signer support (e.g. hardware wallet) by Sjors
  27. DrahtBot cross-referenced this on Feb 26, 2020 from issue Multiprocess bitcoin by ryanofsky
  28. DrahtBot cross-referenced this on Mar 6, 2020 from issue interfaces: Describe and follow some code conventions by ryanofsky
  29. DrahtBot added the label Needs rebase on Mar 23, 2020
  30. luke-jr force-pushed on Apr 3, 2020
  31. DrahtBot removed the label Needs rebase on Apr 3, 2020
  32. DrahtBot cross-referenced this on Apr 13, 2020 from issue rpc: gui: Don't change behavior based on private keys disabled, instead add new buttons/rpcs/menu items by achow101
  33. DrahtBot cross-referenced this on Apr 15, 2020 from issue gui: Add a `Make unsigned` button next to `Send` by achow101
  34. DrahtBot added the label Needs rebase on Apr 23, 2020
  35. Sjors commented at 7:41 AM on April 23, 2020: member

    This needs a rebase now that #17509 (Save / Load PSBT is merged).

  36. hebasto commented at 2:17 PM on June 29, 2020: member

    Approach ACK.

  37. GUI: Rename SendConfirmationDialog.confirmButtonText to m_yes_button_text e0bde21bb6
  38. GUI: SendConfirmationDialog: Enable customisation of dialog
    Both buttons can be replaced with other standard buttons
    30205a6fde
  39. Bugfix: GUI: Restore SendConfirmationDialog button default to "Yes"
    The SendConfirmationDialog is used for bumping the fee, where "Send" doesn't really make sense
    b4603854fe
  40. luke-jr commented at 6:25 PM on August 20, 2020: member

    Rebased

  41. luke-jr force-pushed on Aug 20, 2020
  42. DrahtBot removed the label Needs rebase on Aug 20, 2020
  43. DrahtBot cross-referenced this on Aug 21, 2020 from issue qt: Add Create Unsigned button to SendConfirmationDialog by achow101
  44. fanquake commented at 12:15 PM on September 19, 2020: member

    Let move this over to the GUI repo.

  45. fanquake closed this on Sep 19, 2020
  46. luke-jr cross-referenced this on Dec 9, 2020 from issue Bugfix: GUI: Restore SendConfirmationDialog button default to "Yes" by luke-jr
  47. MarcoFalke referenced this in commit cb2c578451 on Jan 13, 2021
  48. bitcoin locked this on Aug 16, 2022
Contributors
luke-jrDrahtBotlaanwjpromagSjorsjonasschnelligwillenhebastofanquake
Labels
GUI
Linked (view graph)
#15987 Wallet, GUI: Warn when sending to already-used Bitcoin addresses#16944 gui: create PSBT with watch-only wallet#17509 gui: save and load PSBT
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-19 06:54 UTC