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

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

0SendConfirmationDialog confirmationDialog(tr("Confirm fee bump"), ...);
1confirmationDialog.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:

0    // 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 0:07 am on November 16, 2019: contributor

Tested ACK d6adf3bc2db17ed3a17caa2ac5cf9095d70b53fb

Before:

With this PR:

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 0: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 0: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:

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

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

Bugfix: GUI: Restore SendConfirmationDialog button default to “Yes” #17463

Conflicts