gui icon indicating copy to clipboard operation
gui copied to clipboard

Add more detailed address error message

Open w0xlt opened this issue 3 years ago • 12 comments

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
addr_invalid_base58
addr_invalid_bech32
send_invalid_base58
send_invalid_bech32
send_invalid_checksum

w0xlt avatar Jan 21 '22 10:01 w0xlt

Concept ACK

ghost avatar Jan 21 '22 11:01 ghost

Tested with and without BOLD error message:

image

image

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 avatar Jan 21 '22 11:01 ghost

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

ghost avatar Jan 21 '22 12:01 ghost

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);

w0xlt avatar Jan 21 '22 12:01 w0xlt

Concept ACK

kristapsk avatar Jan 22 '22 08:01 kristapsk

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 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.

DrahtBot avatar Jan 23 '22 08:01 DrahtBot

Do not translate technical or extremely rare errors.

These are technical errors so policy is still followed.

ghost avatar Jan 23 '22 08:01 ghost

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)

katesalazar avatar Jan 23 '22 21:01 katesalazar

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

DrahtBot avatar Jun 21 '22 10:06 DrahtBot

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

achow101 avatar Jul 19 '22 15:07 achow101

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

w0xlt avatar Jul 31 '22 21:07 w0xlt

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

Rspigler avatar Aug 01 '22 21:08 Rspigler

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 avatar Oct 31 '22 10:10 DrahtBot