Uh oh!
There was an error while loading. Please reload this page.
Fix various edge case bugs in QValidatedLineEdit - #404
Conversation
There was a problem hiding this comment.
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 33setStyleSheet("");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
Aug 30, 2021
Yes. Without it, the tooltip bg colour is changed too. That is incorrect behaviour.
No, that doesn't make sense... Do you know CSS? :/
It's the logically correct thing to do to ensure a consistent state. |
shaavan
commented
Aug 31, 2021
That makes sense. And I would agree with it.
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. |
promag
left a comment
There was a problem hiding this comment.
Concept ACK fixing and improving this custom widget. Does anyone know why https://doc.qt.io/qt-5/qlineedit.html#setValidator is not used?
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
jarolrod
left a comment
There was a problem hiding this comment.
@luke-jr, just following up on this comment: bitcoin/bitcoin#15987 (comment)
Why would bitcoin/bitcoin#15987 depend on this?
Uh oh!
There was an error while loading. Please reload this page.
luke-jr
commented
Jan 24, 2022
Because without this, validators don't work correctly. bitcoin/bitcoin#15987 uses a validator. Additionally, inline marking of bech32 error detection (#527) also depends on this. |
Uh oh!
There was an error while loading. Please reload this page.
…ineEdit aeb18b6 Bugfix: GUI: Check validity when QValidatedLineEdit::setText is called (Luke Dashjr) b1a544b Bugfix: GUI: Re-check validity after QValidatedLineEdit::setCheckValidator (Luke Dashjr) 2385b50 Bugfix: GUI: Only apply invalid style to QValidatedLineEdit, not its tooltip (Luke Dashjr) Pull request description: 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 bitcoin#15987 or any other PR that adds a warning for valid addresses. Moved from bitcoin#18133 (just concept ACKs) ACKs for top commit: Sjors: tACK aeb18b6 hebasto: ACK aeb18b6, tested on Linux Mint 20.3 (Qt 5.12.8). Tree-SHA512: b6fa8ee4dec76e1c759095721240e6fa5071a02993cb28406e96a0fa2e819f5dddc03d2e7c9073354d7865c2b09eb263afaf853ecba42e9fc4f50ef4ae20bf0f
setText(eg, via the address book). Probably no-op in practice UNTIL merging Wallet, GUI: Warn when sending to already-used Bitcoin addresses bitcoin/bitcoin#15987 or any other PR that adds a warning for valid addresses.Moved from bitcoin/bitcoin#18133 (just concept ACKs)