[Qt] extend validate line edit and btc address validator #3286

pull Diapolo wants to merge 1 commits into bitcoin:master from Diapolo:Qt_validator changing 11 files +123 −56
  1. Diapolo commented at 3:00 PM on November 20, 2013: none
    • remove btc address length from address validator
    • add an optional btc address check in validated line edit that defaults to off and is used in GUIUtil::setupAddressWidget()
    • an isAcceptable() check is added to validated line edit on focus out which only kicks in, when a validator is used with that widget
    • remove an isAcceptable() check from sendcoinsentry.cpp
    • remove obsolete attributes from ui files, which are set by calling GUIUtil::setupAddressWidget()
    • move some more things to GUIUtil::setupAddressWidget() and remove them from normal code e.g. placeholder text
  2. Diapolo commented at 5:55 PM on November 21, 2013: none

    @laanwj Am I right that MaxAddressLength is not the MinAddressLength ;)? And if this is the case, what is the minimum length of a BTC address? This is not yet mergable in the current state ^^.

  3. Diapolo commented at 8:07 AM on November 22, 2013: none

    @laanwj Re-worked, can you check for correctness?

  4. laanwj commented at 8:15 AM on November 22, 2013: member

    Looks ok, but I'm not sure we should really be checking the length of the address at all, as we have much more reliable methods to check the validity of an address.

  5. Diapolo commented at 9:57 AM on November 22, 2013: none

    This is just a pre-check, if that fails why would we check with CBitcoinAddress anyway ;)? The big plus is that too short addresses will already mark the field as invalid, when focusing out, I like that behaviour, try it.

  6. Diapolo commented at 1:20 PM on December 1, 2013: none

    @laanwj Anything I can do here, to help you merge/test it?

  7. laanwj commented at 6:46 AM on December 2, 2013: member

    The problem I have with enforcing the length is that the 'address' is supposed to just be a data blob that encodes a TxDest. New types of address that are longer or shorter could be added later, and then we'd have to update all these checks. I feel pretty strongly that the only test we need to do is the CBitcoinAddress one.

    Edit: but we could do this check on focus out of course, I think that idea is good

  8. Diapolo commented at 12:26 PM on December 2, 2013: none

    So you suggest to remove MaxAddressLength and MinAddressLength altogether? But this leads to the fact one can input addresses with "unlimited" (maximum Qt accepts) length into our BTC address fields... if this doesn't matter yeah, I can remove these checks and hope the underlying CBitcoinAddress class can handle them ^^.

  9. Diapolo commented at 4:51 PM on December 9, 2013: none

    @laanwj Massive update, can you take a look?

  10. in src/qt/qvalidatedlineedit.cpp:None in a3ece1848e outdated 
      46 | +    // Second check is true if no validator was set or,
  47 | +    // if set, the input is valid.
  48 | +    if (text().isEmpty() || hasAcceptableInput())
  49 | +    {
  50 | +        // If used with BTC addresses an additional check can be enabled
  51 | +        // via setCheckAddress(true), default is false.
    laanwj commented at 5:00 PM on December 9, 2013:

    I like the overall idea, but putting this code in ValidatedLineEdit is wrong. This needs to be in a separate policy object instead that does the validation. I suppose putting it in BitcoinAddressValidator would make sense in this case.

    Diapolo commented at 10:36 AM on December 20, 2013:

    Can you please explain what a separate policy means in that context? I'd like to finish this soon :). I pushed my recent version, but need you to help me out ^^.

    laanwj commented at 11:30 AM on December 20, 2013:

    It means that there should be no bitcoin address specific code in the "generic" class QValidatedLineEdit, but it should call into a validator object to do the validation. This keeps the code general.

    A policy class is a class such as QValidator subclasses that implements one aspect, in this case validation, and can be plugged into another object (a QLineEdit or QValidatedLineEdit, in this case).

    On Fri, Dec 20, 2013 at 11:36 AM, P. Kaufmann notifications@github.comwrote:

    In src/qt/qvalidatedlineedit.cpp:

    @@ -37,6 +41,34 @@ void QValidatedLineEdit::focusInEvent(QFocusEvent *evt) QLineEdit::focusInEvent(evt); }

    +void QValidatedLineEdit::focusOutEvent(QFocusEvent *evt) +{

    • // Second check is true if no validator was set or,
    • // if set, the input is valid.
    • if (text().isEmpty() || hasAcceptableInput())
    • {
    •    // If used with BTC addresses an additional check can be enabled
    •    // via setCheckAddress(true), default is false.

    Can you please explain what a separate policy means in that context? I'd like to finish this soon :).

    — Reply to this email directly or view it on GitHubhttps://github.com/bitcoin/bitcoin/pull/3286/files#r8496583 .

    Diapolo commented at 2:38 PM on December 20, 2013:

    I moved the check into a static function in the QValidator subclass, but I don't get the clue, how to really implement what you suggest. Can you give a hint with pseudo-code (only if you've got a little time for this)? Thanks for helping :).

  11. in src/qt/qvalidatedlineedit.cpp:None in 2a3780519c outdated 
      87 | +        setValid(true);
  88 | +
  89 | +        // Check BTC address, if this is a BTC address field
  90 | +        if (isBtcAddressField)
  91 | +        {
  92 | +            if (BitcoinAddressValidator::validateAddress(text()))
    laanwj commented at 2:38 PM on December 20, 2013:

    A concrete alternative that creates no dependency specifically on BTC addresses would be to adapt BitcoinAddressValidator's validate() function to return QValidator::Acceptable only on a valid bitcoin address.

    Then change this code that if a validator() is set, setValid() here based on hasAcceptableInput().

    Diapolo commented at 1:24 PM on January 6, 2014:

    The problem with changing the validate() function of BitcoinAddressValidator is, that it re-checks after every new character you enter in a QValidatedLineEdit, which then results in problems, because you don't know when the final address is in.

    laanwj commented at 1:31 PM on January 6, 2014:

    What's the problem with calling it for every character? What behavior would it result in?

    addr.IsValid() knows when the whole address is entered. An incomplete subset of a bitcoin address is never a valid address.

    Diapolo commented at 1:37 PM on January 6, 2014:

    For example it prevented adding additional characters into the field, if input is considered invalid.

    laanwj commented at 1:47 PM on January 6, 2014:

    Hmm makes sense; so maybe return Intermediate instead of Invalid if the address isn't fully valid?

    Diapolo commented at 2:24 PM on January 6, 2014:

    IMHO this gives a hint that a final isValid check should only happen when focusing out (address is considered valid by user), which perhaps makes it easier to explicitly mark a QValidatedLineEdit to be a btc input field via isBtcAddressField flag?

    laanwj commented at 2:29 PM on January 6, 2014:

    Yes, that's a good solution. So the flag would be checkOnFocusOut instead of isBtcAddressField, which would check on focus out instead of while typing, and it would work generally for other kinds of validators too.

  12. Diapolo commented at 1:26 PM on January 6, 2014: none

    @laanwj Can you try to give a view or do some testing without beeing implementation specific :)? I would like to know if the functionality at least is something you consider valuable in the current state of this pull.

  13. laanwj commented at 1:52 PM on January 6, 2014: member

    Sure!

  14. Diapolo commented at 2:57 PM on January 17, 2014: none

    @laanwj Renamed isBtcAddressField to checkOnFocusOut and reworked a comment. Can you give some more feedback on this and test the pull?

  15. [Qt] extend validate line edit and btc address validator
    - remove btc address length from address validator
- add an optional btc address check in validated line edit that defaults
  to off and is used in GUIUtil::setupAddressWidget()
- an isAcceptable() check is added to validated line edit on focus out
  which only kicks in, when a validator is used with that widget
- remove an isAcceptable() check from sendcoinsentry.cpp
- remove obsolete attributes from ui files, which are set by calling
  GUIUtil::setupAddressWidget()
- move some more things to GUIUtil::setupAddressWidget() and remove them
  from normal code e.g. placeholder text
    33e863dfcd
  16. BitcoinPullTester commented at 9:53 AM on January 22, 2014: none

    Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/33e863dfcdb9f92a8c85460fd321b9bca18d9a3d for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

  17. Diapolo commented at 1:14 PM on January 22, 2014: none

    @laanwj Did you do any testing yet?

  18. laanwj commented at 4:27 PM on January 22, 2014: member

    @Diapolo yes, I like the functionality, going to try to refactor the code a bit

  19. Diapolo commented at 10:20 PM on January 22, 2014: none

    @laanwj Good idea, better than me trying to do what you have in your brain ^^,

  20. Diapolo commented at 10:01 AM on January 28, 2014: none

    @laanwj Any progress with the refactor?

  21. laanwj commented at 12:36 PM on January 28, 2014: member

    Yes, I've thought about it. I think I'm going to give a QValidatedLineEdit two QValidators (both which are optional). One for the normal QLineEdit business, and one for the 'exit check'.

    Similarly I'll split up BitcoinAddressValidator into a class BitcoinAddressEntryValidator with the current ::validate and a class BitcoinAddressCheckValidator that has ::validateAddress as ::validate.

  23. laanwj commented at 1:16 PM on January 29, 2014: member

    Merged via c78bd937017212c89c1c7aab07399cec5b6b3bdd

  24. laanwj closed this on Jan 29, 2014
  25. Diapolo deleted the branch on Jan 30, 2014
  26. Bushstar referenced this in commit b84482ac57 on Apr 8, 2020
  27. Bushstar referenced this in commit 6f4ca74f1c on Apr 8, 2020
  28. DrahtBot locked this on Sep 8, 2021
Contributors
DiapololaanwjBitcoinPullTester
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-21 18:16 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me