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

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.
fanquake added the label GUI on Nov 13, 2019
DrahtBot commented at 5:54 AM on November 13, 2019: member


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



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.
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.
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.
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
promag commented at 9:34 AM on November 15, 2019: member

Concept ACK.
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 :-)
Sjors approved
Sjors commented at 4:14 PM on November 15, 2019: member

ACK d6adf3bc2db17ed3a17caa2ac5cf9095d70b53fb

The simplification of retval could be its own commit.
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">
DrahtBot added the label Needs rebase on Nov 22, 2019
Sjors commented at 8:39 AM on November 23, 2019: member

Needs a small rebase due to #16944 getting merged.
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.
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.
luke-jr force-pushed on Jan 3, 2020
luke-jr commented at 3:43 AM on January 3, 2020: member

Rebased
DrahtBot removed the label Needs rebase on Jan 3, 2020
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.
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
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.
DrahtBot added the label Needs rebase on Mar 23, 2020
luke-jr force-pushed on Apr 3, 2020
DrahtBot removed the label Needs rebase on Apr 3, 2020
DrahtBot added the label Needs rebase on Apr 23, 2020
Sjors commented at 7:41 AM on April 23, 2020: member

This needs a rebase now that #17509 (Save / Load PSBT is merged).
hebasto commented at 2:17 PM on June 29, 2020: member

Approach ACK.
GUI: Rename SendConfirmationDialog.confirmButtonText to m_yes_button_text e0bde21bb6
GUI: SendConfirmationDialog: Enable customisation of dialog
Both buttons can be replaced with other standard buttons
30205a6fde

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

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

b4603854fe

luke-jr commented at 6:25 PM on August 20, 2020: member

Rebased
luke-jr force-pushed on Aug 20, 2020
DrahtBot removed the label Needs rebase on Aug 20, 2020
fanquake commented at 12:15 PM on September 19, 2020: member

Let move this over to the GUI repo.
fanquake closed this on Sep 19, 2020
MarcoFalke referenced this in commit cb2c578451 on Jan 13, 2021
DrahtBot locked this on Aug 16, 2022

Contributors

Labels

Linked (view graph)

#15987 Wallet, GUI: Warn when sending to already-used Bitcoin addresses #16944 gui: create PSBT with watch-only wallet

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

Conflicts