Skip to content

Conversation

@w0xlt
Copy link
Contributor

@w0xlt w0xlt commented Jan 23, 2022

This PR is an alternative to #533 that addresses #533 (review) .

While this PR no longer provides a detailed error message, it still brings some UI improvements:

. Improve user experience by showing an error message instead of just changing the color of QLineEdit
. Currently, when an invalid address is entered in custom change address, an error message is displayed. But the same does not happen in the Pay To field or when adding a new address in the address manager. This PR standardizes error handling for all address fields.
. The message added in this PR is the same as used in the custom change address, which is already translated into several languages.

PR
invalid_new_address
invalid_pay_to

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 23, 2022

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 luke-jr
Concept ACK jonatack

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)
  • #533 (Add more detailed 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.

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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

@jonatack jonatack 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. Here's a screenshot on Debian in dark mode.

Screenshot from 2022-01-24 11-36-49

@w0xlt w0xlt force-pushed the 2_error_message_addr branch from 48bd233 to dd8bd45 Compare January 24, 2022 19:37
@luke-jr
Copy link
Member

luke-jr commented Jan 31, 2022

A constant message seems as useless as the red background colour. Concept NACK

@hebasto hebasto changed the title gui: add translatable address error message Add translatable address error message Feb 9, 2022
@DrahtBot
Copy link
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".

@DrahtBot
Copy link
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
Copy link
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.

1 similar comment
@DrahtBot
Copy link
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
Copy link
Member

hebasto commented May 19, 2023

Closing due to lack of interest.

@hebasto hebasto closed this May 19, 2023
@bitcoin-core bitcoin-core locked and limited conversation to collaborators May 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Needs rebase UX All about "how to get things done"

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants