[Qt] coin control change address handling update #3391

Diapolo commented at 2:19 PM on December 11, 2013: none

re-work change address handling so that default is CNoDestination(), until a verified and known change address was entered (easier code flow)
add a missing NULL pointer check for adresstablemodel
add a missing text when opening coin control address selection for priority and ensure the label is black

laanwj commented at 2:56 PM on December 11, 2013: member

Changes look good, but haven't tested.

A small general comment though: why do you call your updates 'massive' :p

Diapolo commented at 3:17 PM on December 11, 2013: none

Seems I like that word and I call a pull massive when the changes are "big" ^^. I'm trying to reduce usage of "massive" :-D.

Diapolo commented at 7:11 AM on December 13, 2013: none

Did any coin control user review this?

sipa commented at 1:01 PM on December 15, 2013: member

Calling something "massive" doesn't help encouraging people to review it :)

cozz commented at 1:56 PM on December 15, 2013: contributor

I did massive testing and code review on this, changes look good to me.

Nit: The label "none" appears red, which is distracting. You could add a if dPriority > 0, so that the label appears black.

Diapolo commented at 6:57 PM on December 15, 2013: none

@coss Thanks, I'll change that tomorrow :).

Diapolo commented at 8:46 AM on December 16, 2013: none

@cozz Updated to fix your nit :). And changed commit-message to remove massive :-P.

laanwj commented at 9:04 AM on December 16, 2013: member

Thanks for testing @cozz. @diapolo Yeah, the words that people like to hear with changes are 'localized' 'specific' 'small'. 'Massive' is a bad thing (except when marketing paid upgrades, then every new release is a major rewrite :monkey:)

[Qt] coin control change address handling update

- re-work change address handling so that default is CNoDestination(),
  until a verified and known change address was entered (easier code flow)
- add a missing NULL pointer check for adresstablemodel
- add a missing text when opening coin control address selection for
  priority and ensure the label is black
- add a missing . at the end of a sentence

3380713af5

in src/qt/sendcoinsdialog.cpp:None in 19ef6e14b0 outdated

 545 | @@ -546,44 +546,43 @@ void SendCoinsDialog::coinControlChangeChecked(int state)
 546 |  // Coin Control: custom change address changed
 547 |  void SendCoinsDialog::coinControlChangeEdited(const QString& text)
 548 |  {
 549 | -    if (model)
 550 | +    if (model && model->getAddressTableModel())
 551 |      {
 552 | -        CoinControlDialog::coinControl->destChange = CBitcoinAddress(text.toStdString()).Get();
 553 | +        // Default to no change address until verified
 554 | +        CoinControlDialog::coinControl->destChange = CNoDestination();

laanwj commented at 2:12 PM on December 20, 2013:

I still don't really like that we're using a static "coinControl" object (not a comment on this code change)

in src/qt/sendcoinsdialog.cpp:None in 19ef6e14b0 outdated

 550 | +    if (model && model->getAddressTableModel())
 551 |      {
 552 | -        CoinControlDialog::coinControl->destChange = CBitcoinAddress(text.toStdString()).Get();
 553 | +        // Default to no change address until verified
 554 | +        CoinControlDialog::coinControl->destChange = CNoDestination();
 555 | +        ui->labelCoinControlChangeLabel->setStyleSheet("QLabel{color:red;}");

laanwj commented at 2:24 PM on December 20, 2013:

Suggestion: factor out BitcoinAddress(text.toStdString()) and store it somewhere. It is repeated many times in the below code.

Diapolo commented at 2:51 PM on December 20, 2013:

I added a variable for this, see updated code :).

BitcoinPullTester commented at 3:15 PM on December 20, 2013: none

Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/3380713af5f47efba48fcbd153013e57c9004ded for binaries and test log.

This could happen for one of several reasons:

It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts (please tweak those patches in qa/pull-tester)
It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
It does not build on either Linux i386 or Win32 (via MinGW cross compile)
The test suite fails on either Linux i386 or Win32
The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)

If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here.

This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

laanwj commented at 3:20 PM on December 20, 2013: member

ACK Edit: pulltester failure is #3445 again, can't be caused by the changes in this pull

laanwj referenced this in commit 365350140a on Dec 20, 2013

laanwj merged this on Dec 20, 2013

laanwj closed this on Dec 20, 2013

Diapolo deleted the branch on Dec 20, 2013

Bushstar referenced this in commit e518ce4e13 on Apr 8, 2020

DrahtBot locked this on Sep 8, 2021