Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix various edge case bugs in QValidatedLineEdit #18133

Open
wants to merge 3 commits into
base: master
from

Conversation

@luke-jr
Copy link
Member

luke-jr commented Feb 13, 2020

  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.
@fanquake fanquake added the GUI label Feb 13, 2020
Copy link
Member

hebasto left a comment

Concept ACK aeb18b6

@@ -28,7 +34,7 @@ void QValidatedLineEdit::setValid(bool _valid)
}
else
{
setStyleSheet(STYLE_INVALID);
setStyleSheet("QValidatedLineEdit { " STYLE_INVALID "}");

This comment has been minimized.

Copy link
@hebasto

hebasto Feb 13, 2020

Member

Are quotes around STYLE_INVALID macro really needed?

This comment has been minimized.

Copy link
@luke-jr

luke-jr Feb 15, 2020

Author Member

What?

This comment has been minimized.

Copy link
@hebasto

hebasto Feb 15, 2020

Member

Oh, misreading. Sorry for noise.

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

This comment has been minimized.

Copy link
@hebasto

hebasto Feb 13, 2020

Member

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.

This comment has been minimized.

Copy link
@luke-jr

luke-jr Feb 15, 2020

Author Member

I don't know why it would be unwanted.

This comment has been minimized.

Copy link
@promag

promag Feb 16, 2020

Member

b1a544b

@hebasto don't understand what side effect you are talking about nor why you point to that code, setValidator() is not called.

Copy link
Member

promag left a comment

Concept ACK.

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

This comment has been minimized.

Copy link
@promag

promag Feb 16, 2020

Member

b1a544b

@hebasto don't understand what side effect you are talking about nor why you point to that code, setValidator() is not called.

@@ -29,6 +29,7 @@ class QValidatedLineEdit : public QLineEdit
const QValidator *checkValidator;

public Q_SLOTS:
void setText(const QString&);

This comment has been minimized.

Copy link
@promag

promag Feb 16, 2020

Member

aeb18b6

Maybe drop this and add in the constructor:

connect(this, &QValidatedLineEdit::textChanged, this, &QValidatedLineEdit::checkValidity);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.