Skip to content

Conversation

@hebasto
Copy link
Member

@hebasto hebasto commented Nov 25, 2019

ReceiveRequestDialog has only "Close" button in QDialogButtonBox, therefore nothing could emit QDialogButtonBox::accepted() signal.

@fanquake fanquake added the GUI label Nov 25, 2019
@promag
Copy link
Contributor

promag commented Nov 25, 2019

Isn't always clear to me which role should be used in these cases. Here the payment request was created right before opening the dialog - so what could we "reject"? I think the most appropriate role is "accept" as there's no alternative operation, there's nothing you can do with it. - or is there some guidelines I'm not aware of?

So maybe change the button role and drop the connection from reject signal?

Either way concept ACK.

@hebasto
Copy link
Member Author

hebasto commented Nov 26, 2019

@promag

Here the payment request was created right before opening the dialog - so what could we "reject"?

Maybe there should be "OK" button, not "Close"?

@jonasschnelli
Copy link
Contributor

Successfully tested by setting a breakpoint at ReceiveCoinsDialog::reject() (never reached).
I guess you should also remove the method and definition ReceiveCoinsDialog::reject() in receivecoinsdialog.h/.cpp`

@laanwj
Copy link
Member

laanwj commented Nov 27, 2019

Successfully tested by setting a breakpoint at ReceiveCoinsDialog::reject() (never reached).

Hhmm but doesn't this remove the slot setup for accept(), not reject()?

@hebasto
Copy link
Member Author

hebasto commented Nov 27, 2019

Successfully tested by setting a breakpoint at ReceiveCoinsDialog::reject() (never reached).

Hhmm but doesn't this remove the slot setup for accept(), not reject()?

Actually, ReceiveCoinsDialog and ReceiveRequestDialog are different classes ;)

And "yes", this PR removes connection to ReceiveRequestDialog::accept() slot.

@hebasto
Copy link
Member Author

hebasto commented Dec 5, 2019

Closed in favor of #17597.

@hebasto hebasto closed this Dec 5, 2019
@hebasto hebasto deleted the 20191125-unused-connection branch December 7, 2019 15:05
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
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.

5 participants