Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 3 commits into from

Conversation

luke-jr
Copy link
Member

@luke-jr luke-jr commented Nov 13, 2019

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 fanquake added the GUI label Nov 13, 2019
@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 13, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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
Copy link
Member

laanwj commented Nov 14, 2019

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
Copy link
Member Author

luke-jr commented Nov 14, 2019

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.

Copy link
Member

@promag promag left a comment

Concept ACK.

src/qt/sendcoinsdialog.cpp Outdated Show resolved Hide resolved
Sjors
Sjors approved these changes Nov 15, 2019
Copy link
Member

@Sjors Sjors left a comment

ACK d6adf3b

The simplification of retval could be its own commit.

src/qt/sendcoinsdialog.cpp Show resolved Hide resolved
@jonasschnelli
Copy link
Contributor

jonasschnelli commented Nov 16, 2019

Tested ACK d6adf3b

Before:
Bildschirmfoto 2019-11-15 um 14 04 47
Bildschirmfoto 2019-11-15 um 13 57 28

With this PR:
Bildschirmfoto 2019-11-15 um 14 04 04

Bildschirmfoto 2019-11-15 um 14 00 05

@Sjors
Copy link
Member

Sjors commented Nov 23, 2019

Needs a small rebase due to #16944 getting merged.

@gwillen
Copy link
Contributor

gwillen commented Dec 14, 2019

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
Copy link
Member Author

luke-jr commented Jan 3, 2020

Rebased

@Sjors
Copy link
Member

Sjors commented Jan 3, 2020

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);

Schermafbeelding 2020-01-03 om 15 51 27

Otherwise 3502340 looks good.

Copy link
Member

@promag promag left a comment

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.

src/qt/sendcoinsdialog.cpp Show resolved Hide resolved
src/qt/sendcoinsdialog.cpp Outdated Show resolved Hide resolved
src/qt/sendcoinsdialog.cpp Show resolved Hide resolved
@Sjors
Copy link
Member

Sjors commented Apr 23, 2020

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

@hebasto
Copy link
Member

hebasto commented Jun 29, 2020

Approach ACK.

luke-jr added 3 commits Aug 20, 2020
Both buttons can be replaced with other standard buttons
The SendConfirmationDialog is used for bumping the fee, where "Send" doesn't really make sense
@luke-jr
Copy link
Member Author

luke-jr commented Aug 20, 2020

Rebased

@fanquake
Copy link
Member

fanquake commented Sep 19, 2020

Let move this over to the GUI repo.

@fanquake fanquake closed this Sep 19, 2020
MarcoFalke added a commit to bitcoin-core/gui that referenced this issue Jan 13, 2021
…t to "Yes"

8775691 Bugfix: GUI: Restore SendConfirmationDialog button default to "Yes" (Luke Dashjr)

Pull request description:

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

  Originally bitcoin/bitcoin#17463, but rewritten here much simpler based on other merged changes.

ACKs for top commit:
  hebasto:
    ACK 8775691, tested on Linux Mint 20.1 (x86_64, Qt 5.12.8):

Tree-SHA512: 3953cc9c09613c9a629def8b4dc061b537f148ddcb378430645602e0be0f3a9f1cff083aa685b94b2e9372300d02ec97e0d9ea89db6e3c6feec86795090f0f77
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants