Display an error text when ValueInput has invalid user input #257
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closed
|
We talked about this a bit on Slack, so just repeating my thoughts (IIRC). If we evaluate the validity of the input on each key stroke, it's pretty annoying if the layout changes. So it's better to evaluate it when the input field loses focus, and in that case, the layout reflow should be OK. When entering nothing, a reset to the current value also makes sense. A more helpful error text would be "Enter a value between X and Y". Is it possible to use MB instead of MiB? I'd assume that more people know what MB are and might confuse MiB with the Men in Black. |
It has just been merged. Rebase? |
jarolrod
force-pushed
the
setting-error-state
branch
from
1e89928 to
00fac08
Compare
jarolrod
force-pushed
the
setting-error-state
branch
from
00fac08 to
8bfafde
Compare
|
Updated from 00fac08 to 8bfafde, compare Changes: Addressed Review feedback
TODO in follow-ups:
|
jarolrod
force-pushed
the
setting-error-state
branch
from
8bfafde to
d8cf8d0
Compare
|
Just tested it and I think the input validation needs a bit more work. While it worked well when I entered a low-ish number, I was able to instantly break the app by entering random text (or a really high number like 100 trillion).
|
It has been just merged. |
jarolrod
force-pushed
the
setting-error-state
branch
from
d8cf8d0 to
a51820f
Compare
Validates that the input for these settings is not less or larger than the allowed values. If so, displays an informational error text.
jarolrod
force-pushed
the
setting-error-state
branch
from
a51820f to
524decd
Compare
|
Updated from a51820f to 524decd, compare Changes: @GBKS your concerns should now be addressed here. I hadn't addressed those two points yet because I was thinking about what changes would be needed to ValueInput to allow for it to be used within the Proxy screen, but I'll leave that thinking to another PR.
|
This was referenced Mar 1, 2023
hebasto
added a commit
that referenced
this pull request
Mar 6, 2023
88a38a1 qml: Introduce Storage OptionButton that represents custom prune target (jarolrod) 1b2dde5 qml: expose currently loaded detailItem in InformationPage (jarolrod) aacab2e qml: move setting of default prune setting into StorageOptions (jarolrod) 524decd qml: Validate input for dbcache, threads, and prune target settings (jarolrod) 317b1a5 qml: Expose minimum and maximum valid dbcache values (Jarol Rodriguez) 4eace03 qml: Expose minimum and maximum valid Script Verif Threads values (Jarol Rodriguez) 6440078 qml: Allow to show an error text below Setting header text (jarolrod) 195d8c5 qml: Allow to specify subtext color (jarolrod) Pull request description: Based on #257 to not conflict This introduces a new OptionButton in StorageOptions that represents a custom prune target option. This is shown only when the user has gone into detailed settings and chosen a custom prune target. Additionally, this moves the logic of setting the default prune value from SettingsStorage into StorageOptions. It is the easy storage option button shown during onboarding that should set this. With the original logic, when onboarding is not shown because the user has already gone through it, the app will set prune to 2 gb again. | light | dark | | ----- | ---- | | <img width="752" alt="Screen Shot 2023-02-01 at 10 30 36 PM" src="https://user-images.githubusercontent.com/23396902/216224800-6bda5263-2661-4eed-b550-7cdf94731408.png"> | <img width="752" alt="Screen Shot 2023-02-01 at 10 30 16 PM" src="https://user-images.githubusercontent.com/23396902/216224815-8cdc3f36-6493-4d80-afc8-c4c26ddb2b5c.png"> | [](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/win64/insecure_win_gui.zip?branch=pull/245) [](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/macos/insecure_mac_gui.zip?branch=pull/245) [](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/macos_arm64/insecure_mac_arm64_gui.zip?branch=pull/245) [](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/android/insecure_android_apk.zip?branch=pull/245) ACKs for top commit: johnny9: ACK 88a38a1 Tree-SHA512: 931f79d4950476e74ebd24d5c9389f364e84ef199e4407e225f11792422be7b0acf4c0abd3cf3feb4f0b03d9a6b9c96af947dc6709249bc4857f7c3da66e7b80
|
Merged as a part of #245. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This allows for a ValueInput to signal when it currently has an invalid input from the user and display an error text to the user. In doing so, it is validating the input for the dbcache and threads settings; only writing to settings.json when there is valid input. Valid input for dbcache and thread is based on what are the minimum and maximum values the node will recognize for this. Proper validation for the prune setting will be handled in a future pr.