fix:previous label remained for an address without a label #5781

fsb4000 commented at 1:43 AM on February 10, 2015: contributor

Bug description

Run Bitcoin-Qt
Go to the tab "Send coins"
Press "Address book button"
Choose address with label
Again press "Address book button" for the same recipient
Choose address without label
The previous label remained for the address without a label

gmaxwell added the label GUI on Feb 10, 2015

jonasschnelli commented at 8:26 AM on February 10, 2015: contributor

Tested. Indeed it fixed the problem listed above.

But why is there a if(!associatedLabel.isEmpty()) check? Maybe it makes sense to not clear the label field if one selects a address from address-book without label. The user could have set a string there before selecting an address from the address book.

But i also think, that this fix could make sense. Overwriting the label field from the value in the addressbook looks good IMO.

But you code is somehow strange. Will comment directly in the code.

in src/qt/sendcoinsentry.cpp:None in b5b3ad999b outdated

 249 | @@ -250,13 +250,13 @@ bool SendCoinsEntry::updateLabel(const QString &address)
 250 |      if(!model)
 251 |          return false;
 252 |  
 253 | -    // Fill in label from address book, if address has an associated label
 254 | +    // Fill in label from address book
 255 |      QString associatedLabel = model->getAddressTableModel()->labelForAddress(address);
 256 | -    if(!associatedLabel.isEmpty())
 257 | +    bool res = QString::compare(ui->addAsLabel->text(), associatedLabel, Qt::CaseSensitive) != 0;

jonasschnelli commented at 8:28 AM on February 10, 2015:

Why don't you pack the QString::compare(ui->addAsLabel->text(), associatedLabel, Qt::CaseSensitive) into the if: if (QString::compare(ui->addAsLabel->text(), associatedLabel, Qt::CaseSensitive))

And why we need a ::compare here? Wouldn't it be sufficient to just remove the if(!associatedLabel.isEmpty()). There is no drawback in overwriting the label string with the same string IMO.

in src/qt/sendcoinsentry.cpp:None in b5b3ad999b outdated

 260 |          ui->addAsLabel->setText(associatedLabel);
 261 | -        return true;
 262 |      }
 263 |  
 264 | -    return false;
 265 | +    return res;

jonasschnelli commented at 8:29 AM on February 10, 2015:

at this point, res can only be false. Changing line 261 makes no sense to me.

fsb4000 commented at 1:35 PM on February 10, 2015:

res can be and false and true. (Because I deleted return true from if) Ok, I will change the code now.

fsb4000 commented at 1:44 PM on February 10, 2015: contributor

@jonasschnelli

And why we need a ::compare here? Wouldn't it be sufficient to just remove the if(!associatedLabel.isEmpty()). There is no drawback in overwriting the label string with the same string IMO.

Actually yes. In Novacoin I deleted if(!associatedLabel.isEmpty()) and didn't use ::compare. https://github.com/fsb4000/novacoin/commit/470c4e06a2f6d379eef355b94fb6bee059c6cb12 But here this function is bool.... As I understand it the return value is updated label(true) or not updated label(false). So I use ::compare... And about !=0 after ::compare, ::compare return int, if we don't write !=0 we get C4800 https://msdn.microsoft.com/en-us/library/b6801kcy.aspx

laanwj commented at 12:11 PM on February 18, 2015: member

I remember this was 'fixed' quite a few times over git history. It's worth getting this right. Looks like quite brittle code with the compare, can't we distinguish the events some other way?

fsb4000 commented at 2:28 PM on February 18, 2015: contributor

Sure We can use:

if( ui->addAsLabel->text() != associatedLabel)

if you don't want use QString::compare.

laanwj commented at 10:18 AM on February 19, 2015: member

I wasn't complaining about usage of the compare function.

More that this behavior (AFAIK) has been changed back and forth over the history, which usually indicates something else is wrong. If I understand correctly this basically brings back the pre- f9f75f320eedbc33f80f3978703db72dca76a94b behavior of always changing the label and thus #840.

Diapolo commented at 11:23 AM on February 19, 2015: none

I also remember I was already on this a while ago... I can't recover what I did exactly :-/.

fix:previous label remained for an address without a label c19a0a6038

laanwj commented at 11:30 AM on March 16, 2015: member

So has anyone tested this? Does it bring back issue #840?

fsb4000 commented at 11:36 AM on March 16, 2015: contributor

Yes, it bring back the issue :(

laanwj commented at 11:22 AM on April 15, 2015: member

Do you intend to fix this so that doesn't re-introduce an old issue?

fsb4000 commented at 3:30 PM on April 16, 2015: contributor

no

fsb4000 closed this on Apr 16, 2015

MarcoFalke locked this on Sep 8, 2021