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

[GUI] Use QRegexValidator instead of the QDoubleValidator. #1124

Merged
merged 1 commit into from Dec 13, 2019

Conversation

@furszy
Copy link
Collaborator

furszy commented Nov 22, 2019

Have received reports from linux users experiencing problems with the amount editable box, not being able to write large numbers with decimals (even when the QDoubleValidator decimal limit was set to 8..).
Have checked it in macOS and everything is fine there, so.. the issue is pointing to an internal QT QDoubleValidator class problem that has nothing to do with our UI code in some linux distributions.

To solve it without over complicate the code, have changed the QDoubleValidator for a QRegexValidator.
Running fine in linux now too here, more eyes are welcome.

@furszy furszy self-assigned this Nov 22, 2019
@furszy furszy requested a review from random-zebra Nov 22, 2019
Copy link
Collaborator

random-zebra left a comment

Mhm, have few points here.

1) QRegExp is deprecated, qt docs state:

Note: In Qt 5, the new QRegularExpression class provides a Perl compatible implementation of regular expressions and is recommended in place of QRegExp.


2) Even though the first two are not visible (due to inline validation), this expression has some error:

  • it rejects whole numbers (i.e. it needs the decimal point even for integers).
  • it allows for numbers ending with the decimal point, like 123. (though that turns to 123.0 in BitcoinUnits::parse)
  • it accepts only 7 digits after the decimal point.

A possible solution could be:

^(?:0|[1-9][0-9]{0,7})(\\.[0-9]{1,8})?$

3) Switching to regular expression validation makes SendMultiRow::amountChanged redundant.


4) More importantly, I think that the inline validation provided with widget->setValidator() offers a bad user experience in general.

Let me explain this with an example.
User wants to send 2.53 PIVs, but mistakenly enters 1.53.
The natural thing he would try to do is: put the cursor after the 1, delete it, and then enter 2.
He cannot delete 1 though (as the validator refuses .53) and no feedback is given.
Another example.
He wants to send 201 PIVs, but mistakenly inputs 301. He cannot delete the 3 because 01 would not be an acceptable input.
Again no feedback at all.

There are plenty solutions for this problem.
We could do validation only when "Send" is clicked (and maybe highlight the wrong fields), or leverage the editingFinished() signal, or write a custom validator returning Intermediate state.


Of course, any of these solutions would require several changes so we can delay its implementation to 4.1

@random-zebra random-zebra added this to the 4.0.0 milestone Nov 22, 2019
@furszy

This comment has been minimized.

Copy link
Collaborator Author

furszy commented Nov 22, 2019

answering:

If my memory does't fail me, QRegularExpression has no simple implementation over the lineEdit component validator as QRegExp has (have to subclass the line edit and implement own validation yada yada.. which shouldn't be even needed for a simple validation as this one). But will give a look and see if there is something over that area in the framework to not have to do it now as we are in a feature request period.

k good, can even modify the [0-9] for \d and simplify it a little bit more.
^(?:0|[1-9]\d{0,7})(\\.\d{1,8})?$

The wallet has it implemented already ;) . It will highlight the invalid field when the user press the send button and paint the borders in red.

@random-zebra

This comment has been minimized.

Copy link
Collaborator

random-zebra commented Dec 11, 2019

Currently cannot be tested on a wallet with staking addresses due to the segfault fixed in #1142. This needs to be rebased on latest master (possibly squashing the two commits).

Other than that, it almost fixes points 1-3. 👍

Only one last edge case remains: you can enter 10000000.1234567 but not 10000000.12345678. This is because lineEditAmount has a limitation on the number of chars:

<property name="maxLength">
	<number>16</number>
</property>

and the dot counts as one char.
To accept numbers up to 99999999.99999999 the maxLength should be set to 17 in sendMultiRow ui file.


Then, only point 4 remains.
A quick fix for it, we could already implement, is to use this, much simpler, regex:

QRegularExpression rx("^(\\d{0,8})((\\.|,)\\d{1,8})?$");

which allows for numbers starting with more than one 0 and numbers starting with ..
Without modifications to BitcoinUnits::parse (no need to strip extra leading zeros, or add 0 when the string starts with .) we could already avoid the two situations described in the previous comment and offer a better user experience.

To complete the flow, we should just prevent the move of the cursor to end of the line when modifying (which is done by setText()).
Simple fix like this in amountChanged, does the trick:

// set text without moving the cursor
const int cpos = ui->lineEditAmount->cursorPosition();
ui->lineEditAmount->setText(amountStr);
ui->lineEditAmount->setCursorPosition(cpos);

Result:
sendmultirow_fix


Lastly, the comma , is now accepted by the regex but not in BitcoinUnits::parse.
I would use

QStringList parts = removeSpaces(value).replace(",", ".").split(".");

there, to treat both chars (comma and dot) as decimal separators now.

@furszy

This comment has been minimized.

Copy link
Collaborator Author

furszy commented Dec 11, 2019

awesome review, pretty detailed 👌

Just one point about the cursor.
if users paste the complete number and cpos > amountStr.size() it will crash or have an unexpected behavior. Going to move the cursor to the end of the line in that occasions and done.

The rest is great 🚀 .

@random-zebra

This comment has been minimized.

Copy link
Collaborator

random-zebra commented Dec 11, 2019

amountChanged is called with the signal SIGNAL(textChanged(const QString&)), thus after the actual edit of the text on the QLineEdit.
Therefore this shouldn't lead to any unexpected behaviour due to out of bounds cursor position (also, copy-paste seems to work just fine here).

@random-zebra

This comment has been minimized.

Copy link
Collaborator

random-zebra commented Dec 11, 2019

Actually, I think we can remove amountChanged all together (this was point n.3) and simply emit onValueChanged there (so there's not even need to move the cursor at all).

@furszy

This comment has been minimized.

Copy link
Collaborator Author

furszy commented Dec 11, 2019

Check the amountChanged method, it parses the string into a cAmount object, validating the string, and then transforms the value into a string again (removing extra every extra char like the thin space).

@furszy furszy force-pushed the furszy:2019_regex_validator branch 2 times, most recently from 3c5e039 to b6b37e7 Dec 11, 2019
@furszy

This comment has been minimized.

Copy link
Collaborator Author

furszy commented Dec 11, 2019

All points tackled.

Copy link
Collaborator

random-zebra left a comment

Alright, all good now 💃
ACK b6b37e7

@random-zebra

This comment has been minimized.

Copy link
Collaborator

random-zebra commented Dec 11, 2019

mhm, penultime test of uriTests should be updated/removed (1,000 is valid as 1 now).

@furszy furszy force-pushed the furszy:2019_regex_validator branch from b6b37e7 to 675986c Dec 11, 2019
… QEditLine amounts.

[GUI] amount widget using the new QRegularExpression instead of the instead of the obsolete QRegExp class.
@furszy furszy force-pushed the furszy:2019_regex_validator branch from 675986c to 8341d7e Dec 11, 2019
Copy link
Collaborator

random-zebra left a comment

ACK 8341d7e

@furszy furszy requested review from Warrows and Fuzzbawls Dec 12, 2019
Copy link
Collaborator

Fuzzbawls left a comment

ACK 8341d7e

Fuzzbawls added a commit that referenced this pull request Dec 13, 2019
8341d7e [GUI] Use QRegexValidator instead of the QDoubleValidator to validate QEditLine amounts. (furszy)

Pull request description:

  Have received reports from linux users experiencing problems with the amount editable box, not being able to write large numbers with decimals (even when the QDoubleValidator decimal limit was set to 8..).
  Have checked it in macOS and everything is fine there, so.. the issue is pointing to an internal QT QDoubleValidator class problem that has nothing to do with our UI code in some linux distributions.

  To solve it without over complicate the code, have changed the QDoubleValidator for a QRegexValidator.
  Running fine in linux now too here, more eyes are welcome.

ACKs for top commit:
  random-zebra:
    ACK 8341d7e
  Fuzzbawls:
    ACK 8341d7e

Tree-SHA512: 908451fc8f2f8b4b23d0d00ba4199e2ba24367bfa509bffc4518d6f0d7b225067ce712a544fb94cbe2d7db2313256f82e7f809cf9ca076a724a2d1c42eba9c25
@Fuzzbawls Fuzzbawls merged commit 8341d7e into PIVX-Project:master Dec 13, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
akshaynexus added a commit to VitaeTeam/Vitae-ReverseHF that referenced this pull request Dec 13, 2019
…ubleValidator.

8341d7e [GUI] Use QRegexValidator instead of the QDoubleValidator to validate QEditLine amounts. (furszy)

Pull request description:

  Have received reports from linux users experiencing problems with the amount editable box, not being able to write large numbers with decimals (even when the QDoubleValidator decimal limit was set to 8..).
  Have checked it in macOS and everything is fine there, so.. the issue is pointing to an internal QT QDoubleValidator class problem that has nothing to do with our UI code in some linux distributions.

  To solve it without over complicate the code, have changed the QDoubleValidator for a QRegexValidator.
  Running fine in linux now too here, more eyes are welcome.

ACKs for top commit:
  random-zebra:
    ACK 8341d7e
  Fuzzbawls:
    ACK 8341d7e

Tree-SHA512: 908451fc8f2f8b4b23d0d00ba4199e2ba24367bfa509bffc4518d6f0d7b225067ce712a544fb94cbe2d7db2313256f82e7f809cf9ca076a724a2d1c42eba9c25
Fuzzbawls added a commit that referenced this pull request Dec 14, 2019
… QEditLine amounts.

[GUI] amount widget using the new QRegularExpression instead of the instead of the obsolete QRegExp class.

Github-Pull: #1124
Rebased-From: 8341d7e
random-zebra added a commit that referenced this pull request Dec 15, 2019
fa1987b [GUI] ColdStaking widget: fix "total staking" amount (random-zebra)
7ad5385 Update copyright headers (Fuzzbawls)
31433e2 [Docs] 4.0.0 release notes (Fuzzbawls)
8cfd8b5 [GUI] Border image workaround. (furszy)
82069c3 [Qt] Update translations from transifex (Fuzzbawls)
1894425 [GUI][Wallet] Allow spending of P2CS without coincontrol selection Github-Pull: #1193 Rebased-From: b8e4df8 (random-zebra)
102dfd5 [Consensus] Define MainNet changeover block for PIVX v4.0 Github-Pull: #1191 Rebased-From: 8f7dd00 (random-zebra)
9b9dc62 [GUI][BUG] Accept P2CS locked coins in coincontrol (random-zebra)
9d9909b [GUI] Use QRegexValidator instead of the QDoubleValidator to validate QEditLine amounts. (furszy)
ed0a93a [GUI] Feature/Bug CoinControl Update on Open (Liquid369)
6009c24 [GUI] Validate wallet password on enter key press (warrows)
3caf895 [Network] Add SPORK 17 and 18 to the fMissingSporks flag + move the validation below the old protocol and the connected to ourself check. No need to send the message if we are going to end up disconnecting from the peer. (furszy)
a7205ad [GUI] set focus when showing dialogs (random-zebra)
5f0d3e6 [GUI][Trivial] Make amount optional in staking address gen dialog Github-Pull: #1185 Rebased-From: a2d67d6 (random-zebra)
630e040 [GUI][Trivial] Fix tx detail dialog expanding policy Github-Pull: #1187 Rebased-From: 7e5253d (random-zebra)
96f240b [GUI][Trivial] move caps lock warning in askpassphrase dialog Github-Pull: #1186 Rebased-From: 5bd5bbc (random-zebra)
08ee144 [GUI][Bug] cold staking screen, fixing show receive addresses list in owner address editLine. (furszy)
c71bc13 [GUI] Remove every pushButton focus property. (furszy)
01c95c6 [Doc] Remove stale image files (Fuzzbawls)
3cafa3a [Doc] Update gitian building docs (Fuzzbawls)
90d27bd [Utils] Update gitian-build.py for multi-os compatibility (Fuzzbawls)
ddbceb4 Dead link to wrong PIVX website (NoobieDev12)
c99d712 [Build] Add random-zebra gitian GPG public key fingerprint Github-Pull: #1180 Rebased-From: a88bff8 (random-zebra)
6cae0c9 [Trivial] fix minColdStakingAmount comment and type Github-Pull: #1174 Rebased-From: 35a5065 (random-zebra)
a7f6d17 [GUI][Trivial] access Params() only from walletModel Github-Pull: #1174 Rebased-From: 4ad9492 (random-zebra)
d41f9c6 [Cleanup] Remove extra calls to getDisplayUnit in ColdStakingWidget Github-Pull: #1174 Rebased-From: 5c3162b (random-zebra)
5c1a1a9 [GUI] Cold Staking Widget: set minColdStakingAmount from chain params Github-Pull: #1174 Rebased-From: 2369644 (random-zebra)
c8e1762 [GUI] Removing cold staking "unconfirmed balance will not be shown" warning label. (furszy)
3a302aa [Wallet] GetAvailableP2CSCoins permitting unconfirmed tx. (furszy)
53901c0 [GUI] Cold staking, always update staking total amount (furszy)
9527147 [GUI][Bug] Cold staking screen, delegations list, total amount was not being updated. This solves the issue + re organize the UI form to present it aligned to the texts in the left. (furszy)
c3d8f60 [Qt] properly copy IPv6 externalip MN info (Fuzzbawls)
b1641f4 [Trivial] Clean time offset warning when it gets back within range Github-Pull: #1167 Rebased-From: c24d2b1 (random-zebra)
c3ec18b [Qt] Periodic make translate (Fuzzbawls)
02b2a04 [GUI][Model] Do not request password when the wallet is unlocked to lock it for staking only (furszy)
3971d91 [GUI] Custom change address, editable box getting focus when the dialog is launched. (furszy)
6565097 [GUI] Masternodes wizard, IPv6 correctly stored between brackets. (furszy)
6cb9217 [GUI] tooltip css changes. (furszy)
4dff02c [GUI] Decreasing the tooltip padding for #1076 windows issue. (furszy)
0e2dd1d Rewording text under Change Wallet Passphrase Github-Pull: #1151 Rebased-From: 95bba7a (NoobieDev12)
38d3218 [GUI][Trivial] Rewording of Error message while trying to send with the transaction with wallet unlocked for staking only (NoobieDev12)
107a452 [Travis] Increase functional tests reserved time (Warrows)
7d94126 [Bug] Coded properly the URI from file read.. (furszy)
32a32f3 [GUI] Regtest corrections. (furszy)
b4d5b57 [Bug] Fix segfault on GUI initialization for cold staker wallet (random-zebra)
33c7084 [GUI] Cold staking, alert user if the owner address is not from the wallet. (furszy)
de5cf4a [Deployment] Windows taskbar icon pixelated. (furszy)
5dbb514 [GUI] Inform if open pivx conf and open backups folder failed due some OS incapacity/restriction. Plus minor cleanup over the touched files. (furszy)
ffd7dce [Qt] Fix segfault when running GUI client with --help or -? (Fuzzbawls)
3d39ce6 Revert "[Params] TESTNET ONLY, do not merge in master." (Fuzzbawls)

Pull request description:

  This updates the `4.0` branch with the PRs that have been merged since the branch-off point. Additionally, it reverts the locking to testnet (2e85666).

  List of included PRs:
  #1139
  #1125
  #1127
  #1121
  #1142
  #1141
  #1131
  #1157
  #1159
  #1151
  #1156
  #1158
  #1161
  #1160
  #1162
  #1167
  #1166
  #1165
  #1174
  #1180
  #1179
  #1176
  #1173
  #1178
  #1186
  #1187
  #1185
  #1183
  #1177
  #1163
  #1190
  #1124
  #1192
  #1191
  #1193
  #1195
  #1188
  #1181
  #1197
  #1194

ACKs for top commit:
  Warrows:
    utACK fa1987b, diff looks good.
  random-zebra:
    utACK fa1987b and merging...

Tree-SHA512: f65d04e5764fda9a9899276bb0de8d2b8b6e8152d471d32a8fccfd7f79acdd526d4f9b03d848f07c54defe0173448c01b573e39e26164281cfd05c66c9e9af5a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.