Fix various edge case bugs in QValidatedLineEdit #18133

pull luke-jr wants to merge 3 commits into bitcoin:master from luke-jr:bugfix_qvalidlineedit changing 2 files +9 −1

luke-jr commented at 2:38 AM on February 13, 2020: member
1. Use a CSS selector to avoid changing the background colour of the tooltip.
2. Re-check validity of input when we first set the validator (probably a no-op in practice).
3. Check validity of input when it is set programmatically via setText (eg, via the address book). Probably no-op in practice UNTIL merging #15987 or any other PR that adds a warning for valid addresses.
Bugfix: GUI: Only apply invalid style to QValidatedLineEdit, not its tooltip 2385b508d5
Bugfix: GUI: Re-check validity after QValidatedLineEdit::setCheckValidator b1a544be10
Bugfix: GUI: Check validity when QValidatedLineEdit::setText is called aeb18b665c
fanquake added the label GUI on Feb 13, 2020
in src/qt/qvalidatedlineedit.cpp:37 in aeb18b665c
```
  33 | @@ -28,7 +34,7 @@ void QValidatedLineEdit::setValid(bool _valid)
  34 |      }
  35 |      else
  36 |      {
  37 | -        setStyleSheet(STYLE_INVALID);
  38 | +        setStyleSheet("QValidatedLineEdit { " STYLE_INVALID "}");
```
hebasto commented at 9:00 AM on February 13, 2020:

~Are quotes around STYLE_INVALID macro really needed?~

luke-jr commented at 5:10 PM on February 15, 2020:

What?

hebasto commented at 6:51 PM on February 15, 2020:

Oh, misreading. Sorry for noise.
in src/qt/qvalidatedlineedit.cpp:115 in aeb18b665c
```
 111 | @@ -106,6 +112,7 @@ void QValidatedLineEdit::checkValidity()
 112 |  void QValidatedLineEdit::setCheckValidator(const QValidator *v)
 113 |  {
 114 |      checkValidator = v;
 115 | +    checkValidity();
```
hebasto commented at 9:08 AM on February 13, 2020:

This call might cause a side effect that is considered unwanted, no?

Here is an analogous code::

void QLineEdit::setValidator(const QValidator *v) { Q_D(QLineEdit); d->control->setValidator(v); }

I'd leave setCheckValidator() function untouched.
luke-jr commented at 5:11 PM on February 15, 2020:

I don't know why it would be unwanted.

promag commented at 2:12 AM on February 16, 2020:

b1a544be109d336c0b53722e3f8b51687972c94e @hebasto don't understand what side effect you are talking about nor why you point to that code, setValidator() is not called.
hebasto commented at 9:08 AM on February 13, 2020: member

Concept ACK aeb18b665c616c3326671b4c7e9d6421306564f0

in src/qt/qvalidatedlineedit.h:32 in aeb18b665c

  28 | @@ -29,6 +29,7 @@ class QValidatedLineEdit : public QLineEdit
  29 |      const QValidator *checkValidator;
  30 |  
  31 |  public Q_SLOTS:
  32 | +    void setText(const QString&);

promag commented at 2:15 AM on February 16, 2020:

aeb18b665c616c3326671b4c7e9d6421306564f0

Maybe drop this and add in the constructor:

connect(this, &QValidatedLineEdit::textChanged, this, &QValidatedLineEdit::checkValidity);

luke-jr commented at 12:14 PM on April 3, 2020:

Then it will run checkValidity twice for every keystroke?

promag commented at 2:16 AM on February 16, 2020: member

Concept ACK.
fanquake commented at 6:33 AM on August 14, 2020: member

A couple of Concept ACKs here, but no other comments for 6 months. Also seems that the 3rd commit is still a no-op (along with probably the second commit?), and #15987 has now been closed. I'm going to suggest re-opening in the GUI repo.
fanquake closed this on Aug 14, 2020
hebasto referenced this in commit b7942c9482 on Feb 9, 2022
sidhujag referenced this in commit f2bd0d1359 on Feb 9, 2022
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#15987 Wallet, GUI: Warn when sending to already-used Bitcoin addresses