-
Notifications
You must be signed in to change notification settings - Fork 124
GUI/OptionsDialog: Fix proxy input validation feedback (fixes #166) #182
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
base: 29.x-knots
Are you sure you want to change the base?
GUI/OptionsDialog: Fix proxy input validation feedback (fixes #166) #182
Conversation
35123a8
to
b379c2c
Compare
Note: We can use multiple proxies in v29.1 although GUI doesn't support this yet and it would be out of scope for this pull request. |
b379c2c
to
ca057ff
Compare
i'm glad you posted the screenshots.. it pointed out an edge case that should NOT be happening - the proxy fields (QValidatedLineEdit widgets) did NOT originally validate until after focusOut. This leads to an edge case where you can input invalid IP or Port and then click OK to save the invalid data. This is fixed in this latest version. Also, found a bug where the checkboxes connect code was incorrect with >=QT6.7.. Luke has a fix for this already for OptionsDialog 'prune' checkbox. Had to use this pattern to cover the connectSocks and connectSocksTor checkboxes. Lastly, the proxyvalidator used here does not mark 'error locations' and the default behavior of QValidatedLineEdit is to always set the first character position as an error, then marking the first char of the string as invalid (red, underlined, bold). Most validators don't do this - except the bitcoin address validator. So, i've changed qvalidatedlineedit to not mark ANY chars as 'errors' as it doesn't make sense. i.e. only validators that support error locations should result in the chars in the string being marked as errors. Another approach might be to extend ProxyValidator to actually support error locations, which gives visual feedback (red text) indicating problem parts of the string. seemed overly complicated for this use case, but the possibility is there. |
ca057ff
to
db24f29
Compare
db24f29
to
ddf80f5
Compare
ddf80f5
to
bcba04c
Compare
bcba04c
to
0839298
Compare
0839298
to
acd88f2
Compare
acd88f2
to
2cafcb8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Tested, everything looks fine
Side note:
There is a bug on how signverifymessagedialogue handles changing the window to any other app on your computer and then coming back to bitcoin knots.
But this is out of scope for this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 2cafcb8
Quick note: I had been thinking the original validation code handled hostnames…but when reviewing, turns out it didn’t. Hence, after changes to use inet_pton, the original code was redundant - it was just using NumericLookup to validate numeric input, but less competently than inet pton |
… highlighting - Add setAllowEmptyInput() to control whether empty input is valid - Add setAllowValidationWhileEditing() for real-time validation - Fix focusInEvent to handle validation-while-editing fields correctly - Fix setEnabled to clear focus before disabling to ensure proper cleanup of invalid visual state - Add changeEvent() handler to update validation colors on palette/theme changes
2cafcb8
to
22b37db
Compare
- Enable validation while typing for proxy fields (proxyIp, proxyPort, proxyIpTor, proxyPortTor) - Fix checkbox signal handling for Qt 6.7+ compatibility - Ensure proxy fields are properly enabled/disabled based on checkbox state
…validation on theme change
22b37db
to
bb70122
Compare
Fixes #166
QValidatedLineEdit:
OptionsDialog