Closed pablomartin4btc closed 3 months ago
Just tested on Android mobile. This is a nice update.
I don't think the input field needs to turn orange when it goes from disabled to enabled state (by toggling the line above).
Also interesting how the "Enable" title briefly turns orange when tapping. Feels a bit surprising and unnecessary. I think the title could just remain white.
The copy for the Tor proxy is too confusing still.
Enable to run Tor connections through a dedicated proxy.
When disabled, Tor connections will use the default proxy (if enabled).
Can we remove that second line?
@GBKS
I don't think the input field needs to turn orange when it goes from disabled to enabled state (by toggling the line above).
Also interesting how the "Enable" title briefly turns orange when tapping. Feels a bit surprising and unnecessary. I think the title could just remain white.
I've incorrectly used a different State
in the code of the input control (ACTIVE
), I've fixed it with the correct one now (FILLED
).
IPAddressValueInput
control follows now the same pattern as the ValueInput
control ("Block Storage Limit (GB)") in the "Storage Settings" page.Can we remove that second line?
Yes, that wasn't touched by this PR, but corrected it as requested.
Noticed some missing pieces and a bug in initial state. I left comments and suggested changes. Wrong actionItem type was used and showErrorText needed to be connected to the loadedItem.validInput property that you added.
It seems I did something wrong when I updated the 1st commit. I'll correct it in a bit with your validInput
initialisation value suggestion.
- errorText is displayed at both, while only one is invalid
Sorry, you've found another bug in the code, the Tor IP input points to the Default Proxy one. I'll fix it immediately.
- it's unintuitive imo that the errorText shows up only after enabling/disabling and/or closing the dialog or window
The errorText
will be shown only onEditingFinished
(another way could be onTextChanged
, but it's too intrusive I think as the user perhaps didn't finish to enter the value and already sees the error message), but it's a good point that even the IP is invalid we should not display the error if we disable the input field. Is that your point? (edited:) If so, should we go back to the default value 127.0.0.1:9050
or leave it there even invalid (please check latest update 6b2621ee7457051a655b4add76f4e35cdf3c9279)?
but it's too intrusive I think as the user perhaps didn't finish to enter the value and already sees the error message)
I understand your point, but for me I'd prefer that behavior. The few users who use this and enter it will only enter it rarely. (in contrast to maybe regularly manually typing in a BTC address, then yes such behavior might be annoying.) Because now I enter it, make a typo and navigate back. There was no indication for me that I made a mistake, until I navigate back to the dialog.
is invalid we should not display the error if we disable the input field. Is that your point?
no, the above was. no strong feelings about this one, both are ok imo
If so, should we go back to the default value 127.0.0.1:9050 or leave it there even invalid
yes I'd say to leave it, as user intent was to enter something else
6b2621ee7457051a655b4add76f4e35cdf3c9279 is a bit glitchy when hovering
Because now I enter it, make a typo and navigate back. There was no indication for me that I made a mistake, until I navigate back to the dialog.
Ok, let me update that. It would be also good to see others reviewers takes on this.
is a bit glitchy when hovering
Thanks, I'll investigate what's going on.
IPAddressValueInput
field being glitchy when hovering;onEditingFinished
event into onTextChanged
one, as requested according to the specified use case.Tested on macOS M1, very nice!
One thing I would change:
Tor connections can also be run through a seperate Tor proxy
I feel like this is redundant here a bit, and Tor proxy description says it all
Adding a new improved
ValueInput
control to handle bothIP
address and port values combined in the same field (as it was designed forQML
, currentQT
project have them separately) with the correct visual formatting (255.255.255.255:65535
).In order to add/ change the
IP
address and port value, user needs to enable the fields (currently inmain
branch this doesn't work and this PR fixes it). Currently theIP
address and port input fields don't allow more than 5 digits and this PR also correct it so the user can enter a completeIP
address and port value.If an invalid
![image](https://github.com/bitcoin-core/gui-qml/assets/110166421/7c7b85ef-5be7-436a-b4a5-3747d79ed563)IP
address (e.g.127.0.:9050
) and/ or port (e.g.127.0.0.1:
) are entered an error message will be shown as we do in other fields.Current
![image](https://github.com/bitcoin-core/gui-qml/assets/110166421/fbd09cec-7edf-49ac-bfc3-7dbc48d0e842)master
/main
branch screenshot.Current master branch in
![image](https://github.com/bitcoin-core/gui-qml/assets/110166421/e96f64a6-be55-4a01-be89-7bef37d65c5c)QT
repo (just for reference).This PRs branch screenshot.
![image](https://github.com/bitcoin-core/gui-qml/assets/110166421/3c31c728-42aa-4751-9ff7-4bd411d46396)Design in Figma.
![image](https://github.com/bitcoin-core/gui-qml/assets/110166421/0fd0b112-1166-463a-85d7-a2bae6d68e7e)Implementation details:
In this iteration, validation for this control is implemented using both a function within the control itself focused solely on ensuring correct formatting and a simple regex validator for the valid input characters. Future versions will see integration with network model classes, enabling parsing of IP addresses and ports and additional checks, such as those outlined in
doc/p2p-bad-ports.md
.Note:
Persistence/ wiring of the settings and creating the proper network connection thru the Proxy will be performed in a different PR, the idea is to isolate UI experience/ testing in case we need to make changes on that aspect separately and we concentrate on the functionality behind later.