Add translatable address error message #534

w0xlt commented at 8:38 AM on January 23, 2022: contributor

This PR is an alternative to #533 that addresses #533#pullrequestreview-860261164 .

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

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

in src/qt/forms/editaddressdialog.ui:59 in 7a9de81ebe 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;</string>

hebasto commented at 8:02 AM on January 24, 2022:

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

w0xlt commented at 7:14 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

in src/qt/editaddressdialog.cpp:13 in 7a9de81ebe outdated

   9 | @@ -10,6 +10,7 @@
  10 |  
  11 |  #include <QDataWidgetMapper>
  12 |  #include <QMessageBox>
  13 | +#include <key_io.h>

jonatack commented at 10:28 AM on January 24, 2022:

nit, order

 #include <qt/editaddressdialog.h>
-#include <qt/forms/ui_editaddressdialog.h>
 
+#include <key_io.h>
 #include <qt/addresstablemodel.h>
+#include <qt/forms/ui_editaddressdialog.h>
 #include <qt/guiutil.h>
 
 #include <QDataWidgetMapper>
 #include <QMessageBox>
-#include <key_io.h>

w0xlt commented at 7:38 PM on January 24, 2022:

Done. Thanks.

in src/qt/editaddressdialog.cpp:188 in 7a9de81ebe outdated

 183 | +    }
 184 | +    else // Valid address
 185 | +    {
 186 | +        ui->errorMessage->setText("");
 187 | +    }
 188 | +}

jonatack commented at 10:33 AM on January 24, 2022:

clang-format
missing newline at EOF
can be simpified

@@ -171,18 +171,9 @@ void EditAddressDialog::setAddress(const QString &_address)
 // Address changed
 void EditAddressDialog::addressEdited(const QString& text)
 {
-    if (text.isEmpty()) // Nothing entered
-    {
-        ui->errorMessage->setText("");
-        return;
-    }
-
-    if (!IsValidDestination(DecodeDestination(text.toStdString()))) // Invalid address
-    {
+    if (!text.isEmpty() && !IsValidDestination(DecodeDestination(text.toStdString()))) {
         ui->errorMessage->setText(tr("Warning: Invalid Bitcoin address"));
-    }
-    else // Valid address
-    {
+    } else {
         ui->errorMessage->setText("");
     }
-}
\ No newline at end of file
+}

w0xlt commented at 7:38 PM on January 24, 2022:

Done. Thanks.

in src/qt/editaddressdialog.h:46 in 7a9de81ebe outdated

  41 | @@ -42,6 +42,9 @@ class EditAddressDialog : public QDialog
  42 |  public Q_SLOTS:
  43 |      void accept() override;
  44 |  
  45 | +private Q_SLOTS:
  46 | +    void addressEdited(const QString &);

jonatack commented at 10:33 AM on January 24, 2022:

    void addressEdited(const QString& address);

in src/qt/editaddressdialog.cpp:172 in 7a9de81ebe outdated

 166 | @@ -164,3 +167,22 @@ void EditAddressDialog::setAddress(const QString &_address)
 167 |      this->address = _address;
 168 |      ui->addressEdit->setText(_address);
 169 |  }
 170 | +
 171 | +// Address changed
 172 | +void EditAddressDialog::addressEdited(const QString& text)

jonatack commented at 10:36 AM on January 24, 2022:

Perhaps name the argument like in setAddress() just above.

void EditAddressDialog::addressEdited(const QString& address)

jonatack commented at 10:40 AM on January 24, 2022: contributor

Concept ACK. Here's a screenshot on Debian in dark mode.

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

gui: add more detailed address error message

Co-authored-by: Jon Atack <jon@atack.com>

dd8bd454d8

w0xlt force-pushed on Jan 24, 2022

luke-jr commented at 4:11 AM on January 31, 2022: member

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

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

DrahtBot commented at 10:30 AM on June 21, 2022: contributor

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

<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>

DrahtBot added the label Needs rebase on Jun 21, 2022

DrahtBot commented at 10:30 AM on October 31, 2022: 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 12: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

bitcoin-core locked this on May 18, 2024