gui
gui copied to clipboard
Add more detailed address error message
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 |
---|
![]() |
![]() |
![]() |
![]() |
![]() |
Concept ACK
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.
Some errors in CI: https://github.com/bitcoin-core/gui/pull/533/checks?check_run_id=4895179955
Force-pushed with following changes: . Addressed @prayank23 suggestions in https://github.com/bitcoin-core/gui/pull/533#discussion_r789588871, https://github.com/bitcoin-core/gui/pull/533#discussion_r789589090, https://github.com/bitcoin-core/gui/pull/533#discussion_r789589875 and https://github.com/bitcoin-core/gui/pull/533#issuecomment-1018435991 .
. 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);
Concept ACK
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Conflicts
Reviewers, this pull request conflicts with the following ones:
-
#612 (refactor: Drop unused
QFrame
s inSendCoinsEntry
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.
Do not translate technical or extremely rare errors.
These are technical errors so policy is still followed.
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)
🐙 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".
I agree with others that it looks a bit weird when there is no error. Perhaps move it to the tooltip?
@achow101 I proposed an alternative version in #560, which keeps the layout consistent.
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
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.