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

[GUI][Bug] Don't clear address label during send address validation #1589

Merged

Conversation

Fuzzbawls
Copy link
Collaborator

The current sending flow clears and/or overwrites the user-supplied
address label multiple times, including when doing final address
validation after clicking the send button. This results in the entered
label never being used.

This stops the default clear and the overzealous replacing/setting of
the label text.

@Fuzzbawls Fuzzbawls self-assigned this May 1, 2020
@Fuzzbawls Fuzzbawls added this to the 4.1.0 milestone May 3, 2020
@Fuzzbawls Fuzzbawls added the Needs Backport Placeholder tag for anything needing a backport to prior version branches label May 3, 2020
random-zebra
random-zebra previously approved these changes May 3, 2020
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK d9721b3

Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK, nice catch.
Left two comments.

@@ -727,7 +727,8 @@ void SendWidget::onContactsClicked(SendMultiRow* entry)
menuContacts->setWalletModel(walletModel, AddressTableModel::Send);
connect(menuContacts, &ContactsDropdown::contactSelected, [this](QString address, QString label) {
if (focusedEntry) {
focusedEntry->setLabel(label);
if (!label.isNull() && label != "(no label)")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When this is happening? it shouldn't never happen.

An address coming from the contacts menu always has a label. Otherwise it wouldn't be stored as a contact.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was seeing null labels in my early testing of this fix, but not after re-testing with a fresh wallet. I've left the check for the string literal (no label) because there really isn't a point in populating the label lineedit with that.

The string literal used here is also used when adding addresses to the address book when the user has not provided a label of their own

Copy link

@furszy furszy May 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the current code, the problem would arise when the label field is already complete.
When a contact is selected, the address will be replaced but the label will remain being the same, which could cause confusion as the two addresses would appear to have the same label.

If we are not going to populate the (no label) string, we should clear the label edit line.

src/qt/pivx/sendmultirow.cpp Outdated Show resolved Hide resolved
The current sending flow clears and/or overwrites the user-supplied
address label multiple times, including when doing final address
validation after clicking the send button. This results in the entered
label never being used.

This stops the default clear and the overzealous replacing/setting of
the label text.
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 80aba61

Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix what suggested in my comment in another PR.

utACK 80aba6

@random-zebra random-zebra merged commit 507f75f into PIVX-Project:master May 5, 2020
Fuzzbawls added a commit to Fuzzbawls/PIVX that referenced this pull request May 5, 2020
The current sending flow clears and/or overwrites the user-supplied
address label multiple times, including when doing final address
validation after clicking the send button. This results in the entered
label never being used.

This stops the default clear and the overzealous replacing/setting of
the label text.

Github-Pull: PIVX-Project#1589
Rebased-From: 80aba61
@random-zebra random-zebra removed the Needs Backport Placeholder tag for anything needing a backport to prior version branches label May 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants