Add more detailed address error message

w0xlt commented at 10:43 am on January 21, 2022: contributor

When reviewing https://github.com/bitcoin/bitcoin/pull/24106, I noticed that GUI does not provide a clear reason for invalid addresses. Basically it just changes the QLineEdit to red color.

This PR adds a more detailed error message. The motivations are:

. Provide more information about the error and improve the user experience. . When a more subtle error occurs, as the one detailed in https://github.com/bitcoin/bitcoin/pull/24106, the user can understand what is happening.

To test this PR, enter invalid addresses in the Pay To field on the Send tab and also in the address manager (icon next to Pay To).

PR

ghost commented at 11:40 am on January 21, 2022: none

Concept ACK

in src/qt/forms/editaddressdialog.ui:59 in 1c119d93d1 outdated

52@@ -53,6 +53,16 @@
53        </property>
54       </widget>
55      </item>
56+     <item row="2" column="1">
57+      <widget class="QLabel" name="errorMessage">
58+       <property name="styleSheet">
59+        <string notr="true">color:red;font-weight:bold;</string>

unknown commented at 11:41 am on January 21, 2022:

nit:

0        <string notr="true">color:red;</string>

w0xlt commented at 12:22 pm on January 21, 2022:

Agreed. Done. Thanks.

in src/qt/forms/sendcoinsentry.ui:223 in 1c119d93d1 outdated

228-      <property name="buddy">
229-       <cstring>payAmount</cstring>
230+    <item row="0" column="1">
231+     <widget class="QLabel" name="errorMessage">
232+      <property name="styleSheet">
233+       <string notr="true">color:red;font-weight:bold;</string>

unknown commented at 11:41 am on January 21, 2022:

nit:

0       <string notr="true">color:red;</string>

w0xlt commented at 12:22 pm on January 21, 2022:

Done. Thanks.

in src/qt/forms/sendcoinsentry.ui:78 in 1c119d93d1 outdated

78+       </widget>
79+      </item>
80+      <item>
81+       <widget class="QCheckBox" name="checkboxSubtractFeeFromAmount">
82+        <property name="toolTip">
83+         <string>The fee will be deducted from the amount being sent. The recipient will receive less bitcoins than you enter in the amount field. If multiple recipients are selected, the fee is split equally.</string>

unknown commented at 11:42 am on January 21, 2022:

0         <string>The fee will be deducted from the amount being sent. The recipient will receive less bitcoin than you enter in the amount field. If multiple recipients are selected, the fee is split equally.</string>

w0xlt commented at 12:22 pm on January 21, 2022:

Done. Thanks.

jonatack commented at 10:11 am on January 24, 2022:

Per git grep "less bitcoin" the codebase currently uses “bitcoins” in this context.

ghost commented at 11:47 am on January 21, 2022: none

Tested with and without BOLD error message:

1.

2.

I would prefer 2 but no strong opinion. There is already so much red in that UI once user moves the cursor with wrong address that bold looks weird.

ghost commented at 12:04 pm on January 21, 2022: none

Some errors in CI: https://github.com/bitcoin-core/gui/pull/533/checks?check_run_id=4895179955

gui: add more detailed address error message d90ca3737c

w0xlt force-pushed on Jan 21, 2022

w0xlt commented at 12:21 pm on January 21, 2022: contributor

Force-pushed with following changes: . Addressed @prayank23 suggestions in #533 (review), #533 (review), #533 (review) and #533 (comment) .

. Manually changed qt/forms/sendcoinsentry.ui. Qt Creator is messing up the file with unnecessary code and components like QPalette. This caused errors in CI:

0In file included from qt/sendcoinsentry.cpp:10:
1./qt/forms/ui_sendcoinsentry.h: In member function ‘void Ui_SendCoinsEntry::setupUi(QStackedWidget*)’:
2./qt/forms/ui_sendcoinsentry.h:231:54: error: ‘PlaceholderText’ is not a member of ‘QPalette’
3         palette.setBrush(QPalette::Active, QPalette::PlaceholderText, brush7);

kristapsk commented at 8:44 am on January 22, 2022: contributor

Concept ACK

jarolrod commented at 3:11 am on January 23, 2022: member

I don’t think, at the GUI level, we need to present the user with such detail. A translatable message that conveys the entered address is not correct is enough. If presenting the internal error message is useful for developing purposes, perhaps detailed error messages can be enabled through GUI settings added under a developer mode tab.

We can’t display a translated error message with this approach and per our translation policy

https://github.com/bitcoin-core/gui/blob/e3ce019667fba2ec50a59814a26566fb67fa9125/doc/translation_strings_policy.md#L26-L27

DrahtBot commented at 8:43 am on January 23, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	Rspigler
Concept ACK	ghost, kristapsk

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#612 (refactor: Drop unused QFrames in SendCoinsEntry by hebasto)
#560 (Make error message layout consistent by w0xlt)
#534 (Add translatable address error message by w0xlt)

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.

ghost commented at 8:53 am on January 23, 2022: none

Do not translate technical or extremely rare errors.

These are technical errors so policy is still followed.

katesalazar commented at 9:45 pm on January 23, 2022: contributor

I built and ran this, I think it’s an interesting experiment, but it seems there’s reserved space for the label even when it wouldn’t be used. Sorry to say I would love if the space for the label isn’t there until necessary. 🤷‍♀️

(this is not nak, but yet far from concept ack as well)

in src/qt/forms/editaddressdialog.ui:59 in d90ca3737c

52@@ -53,6 +53,16 @@
53        </property>
54       </widget>
55      </item>
56+     <item row="2" column="1">
57+      <widget class="QLabel" name="errorMessage">
58+       <property name="styleSheet">
59+        <string notr="true">color:red;</string>

hebasto commented at 7:58 am on January 24, 2022:

Is hardcoded red color visually compatible with dark themes/appearance? Maybe add some relevant screenshots?

w0xlt commented at 7:13 pm on January 24, 2022:

There is a screenshot posted by @jonatack here: #534#pullrequestreview-860753411

But this style is already used in sendcoinsdialog.ui and sendcoinsdialog.cpp for invalid custom change addresses and insufficient funds , so it seems to be a well tested configuration.

hebasto added the label UX on Jan 24, 2022

hebasto renamed this:
~~gui: add more detailed address error message~~
Add more detailed address error message
on Feb 9, 2022

DrahtBot added the label Needs rebase on Jun 21, 2022

DrahtBot commented at 10:31 am on June 21, 2022: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

hebasto added the label Wallet on Jul 19, 2022

hebasto requested review from achow101 on Jul 19, 2022

achow101 commented at 3:34 pm on July 19, 2022: member

I agree with others that it looks a bit weird when there is no error. Perhaps move it to the tooltip?

w0xlt commented at 9:06 pm on July 31, 2022: contributor

@achow101 I proposed an alternative version in #560, which keeps the layout consistent.

Rspigler commented at 9:50 pm on August 1, 2022: contributor

I’ve changed my opinion on this and I agree with @jarolrod, I don’t think this is necessary at the GUI level, and could just confuse users further. All that is necessary is the address is invalid. Concept NACK from me.

I think #560 is a much better approach

DrahtBot commented at 10:30 am on October 31, 2022: contributor

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

DrahtBot commented at 0:50 am on January 29, 2023: contributor

There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

DrahtBot commented at 1:08 am on April 29, 2023: contributor

There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

hebasto commented at 3:57 pm on May 19, 2023: member

Closing due to lack of interest.

hebasto closed this on May 19, 2023

hebasto referenced this in commit 7b39702513 on Feb 7, 2024

bitcoin-core locked this on May 18, 2024

Add more detailed address error message #533

Reviews

Conflicts