Add more detailed address error message #533

w0xlt · 2022-01-21T10:43:11Z

When reviewing bitcoin/bitcoin#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 bitcoin/bitcoin#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 · 2022-01-21T11:40:17Z

Concept ACK

src/qt/forms/editaddressdialog.ui

src/qt/forms/sendcoinsentry.ui

ghost · 2022-01-21T11:47:54Z

Tested with and without BOLD error message:

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 · 2022-01-21T12:04:52Z

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

w0xlt · 2022-01-21T12:21:47Z

Force-pushed with following changes:
. Addressed @prayank23 suggestions in #533 (comment), #533 (comment), #533 (comment) 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:

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

kristapsk · 2022-01-22T08:44:01Z

Concept ACK

jarolrod

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

gui/doc/translation_strings_policy.md

Lines 26 to 27 in e3ce019

    
           Do not translate technical or extremely rare errors. 
        
           Anything else that appears to the user in the GUI is to be translated. This includes labels, menu items, button texts, tooltips and window titles.

DrahtBot · 2022-01-23T08:43:52Z

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 · 2022-01-23T08:53:13Z

Do not translate technical or extremely rare errors.

These are technical errors so policy is still followed.

katesalazar · 2022-01-23T21:45:21Z

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)

hebasto · 2022-01-24T07:58:47Z

src/qt/forms/editaddressdialog.ui

+     <item row="2" column="1">
+      <widget class="QLabel" name="errorMessage">
+       <property name="styleSheet">
+        <string notr="true">color:red;</string>


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

There is a screenshot posted by @jonatack here:
#534 (review)

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.

DrahtBot · 2022-06-21T10:31:02Z

🐙 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".}

achow101 · 2022-07-19T15:34:51Z

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

w0xlt · 2022-07-31T21:06:42Z

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

Rspigler · 2022-08-01T21:50:12Z

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 · 2022-10-31T10:30:34Z

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 · 2023-01-29T00:50:27Z

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 · 2023-04-29T01:08:16Z

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 · 2023-05-19T15:57:47Z

Closing due to lack of interest.

fe7c81e qt: change QLineEdit error background (w0xlt) Pull request description: This PR proposes a small change in QLineEdit when there is an error in the input. master | --- | ![image](https://user-images.githubusercontent.com/94266259/154762427-b816267e-ec70-4a8f-a7fb-1317ebacf1a4.png) PR | --- | ![image](https://user-images.githubusercontent.com/94266259/154761933-15eb3d81-ca81-4498-b8ec-cf1139ae2f8a.png) | This also shows good results when combined with other open PRs. #537 | --- | ![image](https://user-images.githubusercontent.com/94266259/154763411-6266a283-2d8a-4365-b3f2-a5cb545e773e.png) #533 | --- | ![image](https://user-images.githubusercontent.com/94266259/154765638-f38b13d6-a4f8-4b46-a470-f882668239f3.png) | ACKs for top commit: GBKS: ACK fe7c81e jarolrod: ACK fe7c81e shaavan: ACK fe7c81e Tree-SHA512: eccc53f42d11291944ccb96efdbe460cb10af857f1d4fa9b5348ddcb0796c82faf1cdad9040aae7a25c5d8f4007d6284eba868d7af14acf56280f6acae170b91

ghost reviewed Jan 21, 2022

View reviewed changes

src/qt/forms/editaddressdialog.ui Outdated Show resolved Hide resolved

ghost reviewed Jan 21, 2022

View reviewed changes

src/qt/forms/sendcoinsentry.ui Outdated Show resolved Hide resolved

ghost reviewed Jan 21, 2022

View reviewed changes

src/qt/forms/sendcoinsentry.ui Outdated Show resolved Hide resolved

gui: add more detailed address error message

d90ca37

w0xlt force-pushed the 1_error_message_addr branch from 1c119d9 to d90ca37 Compare January 21, 2022 12:11

jarolrod reviewed Jan 23, 2022

View reviewed changes

w0xlt mentioned this pull request Jan 23, 2022

Add translatable address error message #534

Closed

hebasto reviewed Jan 24, 2022

View reviewed changes

hebasto added the UX All about "how to get things done" label Jan 24, 2022

meshcollider mentioned this pull request Jan 24, 2022

Add address error location to GUI #536

Closed

hebasto changed the title ~~gui: add more detailed address error message~~ Add more detailed address error message Feb 9, 2022

w0xlt mentioned this pull request Feb 18, 2022

Change address / amount error background #553

Merged

DrahtBot mentioned this pull request Feb 26, 2022

Make error message layout consistent #560

Closed

DrahtBot mentioned this pull request May 31, 2022

refactor: Drop unused QFrames in SendCoinsEntry #612

Merged

DrahtBot added the Needs rebase label Jun 21, 2022

hebasto added the Wallet label Jul 19, 2022

hebasto requested a review from achow101 July 19, 2022 13:36

Rspigler mentioned this pull request Jul 22, 2022

Point out position of invalid characters in Bech32 addresses #537

Closed

hebasto closed this May 19, 2023

bitcoin-core locked and limited conversation to collaborators May 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more detailed address error message #533

Add more detailed address error message #533

w0xlt commented Jan 21, 2022

ghost commented Jan 21, 2022

ghost commented Jan 21, 2022

ghost commented Jan 21, 2022

w0xlt commented Jan 21, 2022 •

edited

Loading

kristapsk commented Jan 22, 2022

jarolrod left a comment

DrahtBot commented Jan 23, 2022 •

edited

Loading

ghost commented Jan 23, 2022

katesalazar commented Jan 23, 2022 •

edited

Loading

hebasto Jan 24, 2022 •

edited

Loading

w0xlt Jan 24, 2022

DrahtBot commented Jun 21, 2022

achow101 commented Jul 19, 2022

w0xlt commented Jul 31, 2022

Rspigler commented Aug 1, 2022

DrahtBot commented Oct 31, 2022

DrahtBot commented Jan 29, 2023

DrahtBot commented Apr 29, 2023

hebasto commented May 19, 2023

	Do not translate technical or extremely rare errors.
	Anything else that appears to the user in the GUI is to be translated. This includes labels, menu items, button texts, tooltips and window titles.

Add more detailed address error message #533

Add more detailed address error message #533

Conversation

w0xlt commented Jan 21, 2022

ghost commented Jan 21, 2022

ghost commented Jan 21, 2022

ghost commented Jan 21, 2022

w0xlt commented Jan 21, 2022 • edited Loading

kristapsk commented Jan 22, 2022

jarolrod left a comment

Choose a reason for hiding this comment

DrahtBot commented Jan 23, 2022 • edited Loading

Reviews

Conflicts

ghost commented Jan 23, 2022

katesalazar commented Jan 23, 2022 • edited Loading

hebasto Jan 24, 2022 • edited Loading

Choose a reason for hiding this comment

w0xlt Jan 24, 2022

Choose a reason for hiding this comment

DrahtBot commented Jun 21, 2022

achow101 commented Jul 19, 2022

w0xlt commented Jul 31, 2022

Rspigler commented Aug 1, 2022

DrahtBot commented Oct 31, 2022

DrahtBot commented Jan 29, 2023

DrahtBot commented Apr 29, 2023

hebasto commented May 19, 2023

w0xlt commented Jan 21, 2022 •

edited

Loading

DrahtBot commented Jan 23, 2022 •

edited

Loading

katesalazar commented Jan 23, 2022 •

edited

Loading

hebasto Jan 24, 2022 •

edited

Loading