Fix various edge case bugs in QValidatedLineEdit

luke-jr commented at 7:45 pm on August 12, 2021: member

Use a CSS selector to avoid changing the background colour of the tooltip.
Re-check validity of input when we first set the validator (probably a no-op in practice).
Check validity of input when it is set programmatically via setText (eg, via the address book). Probably no-op in practice UNTIL merging https://github.com/bitcoin/bitcoin/pull/15987 or any other PR that adds a warning for valid addresses.

Moved from https://github.com/bitcoin/bitcoin/pull/18133 (just concept ACKs)

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

jarolrod added the label Bug on Aug 20, 2021

shaavan commented at 5:20 pm on August 30, 2021: contributor

I looked through the PR and the changes that are made. I have got some doubts that I want to ask the op regarding some changes made here:

The default color property is now enclosed in a CSS selector to avoid changing the tooltip’s background color. But was that a problem in the first place?
If enclosing the color property has its benefits, shouldn’t a similar thing be done in line 33 setStyleSheet(""); to maintain consistency?
The validity of text is checked after setting checkValidator in the setCheckValidator function. Why is there a need to check the validity here?

It would be helpful if op could clarify these doubts. Thank You.

luke-jr commented at 6:54 pm on August 30, 2021: member

The default color property is now enclosed in a CSS selector to avoid changing the tooltip’s background color. But was that a problem in the first place?

Yes. Without it, the tooltip bg colour is changed too. That is incorrect behaviour.

If enclosing the color property has its benefits, shouldn’t a similar thing be done in line 33 setStyleSheet(""); to maintain consistency?

No, that doesn’t make sense… Do you know CSS? :/

The validity of text is checked after setting checkValidator in the setCheckValidator function. Why is there a need to check the validity here?

It’s the logically correct thing to do to ensure a consistent state.

shaavan commented at 6:29 pm on August 31, 2021: contributor

Yes. Without it, the tooltip bg color is changed too. That is incorrect behavior.

That makes sense. And I would agree with it.

No, that doesn’t make sense… Do you know CSS? :/

As a matter of fact, I do know CSS. I wrongly interpreted the changes introduced here, and that was a genuine mistake on my part.

Let me go through some further testings to check if I am not missing something. In the meantime, @luke-jr, it would be great if you could post some screenshots that clearly express the differences introduced in this PR.

in src/qt/qvalidatedlineedit.cpp:109 in b1a544be10 outdated

105@@ -106,6 +106,7 @@ void QValidatedLineEdit::checkValidity()
106 void QValidatedLineEdit::setCheckValidator(const QValidator *v)
107 {
108     checkValidator = v;
109+    checkValidity();

promag commented at 10:33 am on September 5, 2021:

b1a544be109d336c0b53722e3f8b51687972c94e

What bug is this change fixing? Or is this just making the code more correct?

jarolrod commented at 4:30 am on November 2, 2021:

bump

this is also not evident to me

luke-jr commented at 5:39 pm on January 24, 2022:

Re-check validity of input when we first set the validator (probably a no-op in practice).

Sjors commented at 10:12 am on January 25, 2022:

Maybe add “Probably a no-op in practice.” to the commit message.

in src/qt/qvalidatedlineedit.cpp:20 in aeb18b665c

14@@ -15,6 +15,12 @@ QValidatedLineEdit::QValidatedLineEdit(QWidget *parent) :
15     connect(this, SIGNAL(textChanged(QString)), this, SLOT(markValid()));
16 }
17 
18+void QValidatedLineEdit::setText(const QString& text)
19+{
20+    QLineEdit::setText(text);

promag commented at 10:38 am on September 5, 2021:

aeb18b665c616c3326671b4c7e9d6421306564f0

Maybe add another connection? See https://github.com/bitcoin-core/gui/blob/6490a3ef6c4c6525d169818d18a99050f780c5b7/src/qt/qvalidatedlineedit.cpp#L15

luke-jr commented at 5:41 pm on January 24, 2022:

That’s less correct

promag commented at 10:39 am on September 5, 2021: contributor

Concept ACK fixing and improving this custom widget. Does anyone know why https://doc.qt.io/qt-5/qlineedit.html#setValidator is not used?

jarolrod commented at 4:30 am on November 2, 2021: member

@luke-jr, just following up on this comment: https://github.com/bitcoin/bitcoin/pull/15987#issuecomment-933027946

Why would https://github.com/bitcoin/bitcoin/pull/15987 depend on this?

luke-jr commented at 5:43 pm on January 24, 2022: member

Because without this, validators don’t work correctly. bitcoin/bitcoin#15987 uses a validator.

Additionally, inline marking of bech32 error detection (https://github.com/bitcoin-core/gui/issues/527) also depends on this.

Sjors approved

Sjors commented at 10:18 am on January 25, 2022: member

This PR builds on rather ancient commits (from 2017; in case anyone wonders why building fails over missing OpenSSL), but I tested a rebased version.

tACK aeb18b665c616c3326671b4c7e9d6421306564f0

Before 2385b508d5f2db118513c3e0b343d2309cdfdcd8:

After:

hebasto approved

hebasto commented at 8:48 am on January 26, 2022: member

ACK aeb18b665c616c3326671b4c7e9d6421306564f0, tested on Linux Mint 20.3 (Qt 5.12.8).

hebasto added this to the milestone 23.0 on Jan 26, 2022

hebasto merged this on Feb 9, 2022

hebasto closed this on Feb 9, 2022

sidhujag referenced this in commit f2bd0d1359 on Feb 9, 2022

bitcoin-core locked this on Feb 9, 2023

Fix various edge case bugs in QValidatedLineEdit #404