Introducing an improved IP Address and Port input control in ProxySettings component #391

pablomartin4btc · 2024-03-22T16:51:06Z

Adding a new improved ValueInput control to handle both IP address and port values combined in the same field (as it was designed for QML, current QT 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 in main branch this doesn't work and this PR fixes it). Currently the IP address and port input fields don't allow more than 5 digits and this PR also correct it so the user can enter a complete IP address and port value.

If an invalid 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 master/ main branch screenshot.

Current master branch in QT repo (just for reference).

This PRs branch screenshot.

Design in Figma.

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.

johnny9 · 2024-03-23T18:02:14Z

src/qml/components/ProxySettings.qml

            onEditingFinished: {
-                defaultProxy.forceActiveFocus()
+                if (isValidIPPort(text)) {


I think it might be a good idea to encapsulate the validation logic in the IPAddressValueInput component itself. I think one possible way to do that is to add a Connections element to onEditingFinished signal in the IPAddressValueInput definition so the validation can't be overridden. Then just expose a bool for if the input is valid or not.

Ok, it makes sense, I'll try to do that. Thanks!

Updated it with the Connection element and removed the onEditingFinished from IPAddressValueInput, please let me know what you think.

GBKS · 2024-03-25T12:41:29Z

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?

D33r-Gee

tested fe547af on Ubuntu 22.04 and Android(armv7)

Looking good!

Ubuntu screenshot

Android screenshot

I agree with @GBKS :

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?

Making the option switches work to enable and disable the IP Address and Port fields (Proxy and Tor - ValueInput) accordingly to the user's intentions.

pablomartin4btc · 2024-03-26T02:48:58Z

@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).

The new 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.

pablomartin4btc · 2024-03-26T02:51:30Z

Updates: I've addressed feedback from @johnny9, @GBKS and @D33r-Gee. Thanks for your valuable reviews!

johnny9

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.

src/qml/components/ProxySettings.qml

src/qml/controls/IPAddressValueInput.qml

pablomartin4btc · 2024-03-26T13:15:03Z

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.

pablomartin4btc · 2024-03-26T13:31:41Z

Updates: I've solved the issue reported by @johnny9.

MarnixCroes

it's unintuitive imo that the errorText shows up only after enabling/disabling and/or closing the dialog or window
errorText is displayed at both, while only one is invalid

src/qml/controls/IPAddressValueInput.qml

pablomartin4btc · 2024-03-26T15:43:22Z

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 6b2621e)?

pablomartin4btc · 2024-03-26T16:41:32Z

Updates: I've addressed @MarnixCroes's feedback.

MarnixCroes · 2024-03-26T19:00:40Z

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

MarnixCroes · 2024-03-26T19:03:33Z

6b2621e is a bit glitchy when hovering

proxysettings.webm

pablomartin4btc · 2024-03-26T19:09:07Z

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.

pablomartin4btc · 2024-03-26T19:09:54Z

is a bit glitchy when hovering

Thanks, I'll investigate what's going on.

Adding a new improved ValueInput control to handle both IP address and port values combined in the same field with the correct visual formatting (255.255.255.255:65535). 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.

pablomartin4btc · 2024-03-26T20:43:08Z

Updates: I've addressed latest @MarnixCroes's feedback.
- Fixed the reported issue on new IPAddressValueInput field being glitchy when hovering;
- Moved validation logic from the onEditingFinished event into onTextChanged one, as requested according to the specified use case.

johnny9

ACK d47bc1a

MarnixCroes

ack d47bc1a

stackingsaunter · 2024-04-01T15:57:20Z

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

Follow-up from bitcoin-core#391 (comment). Removing redundant Tor Proxy description from the top Default Proxy label.

Follow-up from bitcoin-core#391 (comment). Removing redundant Tor Proxy description from the top Default Proxy header label. Rewording Tor Proxy description to be consistent with the top header label.

… - Follow-up #391 d558b05 qml, component: Fix labelling on ProxySettings (pablomartin4btc) Pull request description: This is a follow-up from #391 (comment). <details> <summary>Currently/ <code>main</code> branch screenshot.</summary> ![Screenshot from 2024-04-07 23-00-31](https://github.com/bitcoin-core/gui-qml/assets/110166421/ffb86641-9f98-46d0-98d4-3c7ffd826d4e) </details> <details> <summary> This PR branch screenshot.</summary> ![image](https://github.com/bitcoin-core/gui-qml/assets/110166421/279abc03-8417-4345-bb21-06a7ff53eae6) </details> ACKs for top commit: GBKS: tACK d558b05 D33r-Gee: tACK d558b05 Tree-SHA512: b849bf2addafeac85e645281be162cf22d2f34da613f3da3fcef42377546a33e1b19beacb66660e06bc2e55b6b3c10bae08cf0ecdafb1e470fb8d44b040b6ff0

pablomartin4btc force-pushed the qml-ipaddress-valueinput branch from a4f4263 to fe547af Compare March 22, 2024 18:31

johnny9 reviewed Mar 23, 2024

View reviewed changes

D33r-Gee reviewed Mar 25, 2024

View reviewed changes

pablomartin4btc force-pushed the qml-ipaddress-valueinput branch 5 times, most recently from aefd53b to 7994009 Compare March 26, 2024 02:34

qml, component: correct ProxySettings behaviour

a7e1b25

Making the option switches work to enable and disable the IP Address and Port fields (Proxy and Tor - ValueInput) accordingly to the user's intentions.

pablomartin4btc force-pushed the qml-ipaddress-valueinput branch from 7994009 to 7893dfb Compare March 26, 2024 02:36

johnny9 suggested changes Mar 26, 2024

View reviewed changes

pablomartin4btc force-pushed the qml-ipaddress-valueinput branch from 7893dfb to c294e59 Compare March 26, 2024 13:20

MarnixCroes reviewed Mar 26, 2024

View reviewed changes

src/qml/controls/IPAddressValueInput.qml Outdated Show resolved Hide resolved

pablomartin4btc force-pushed the qml-ipaddress-valueinput branch from c294e59 to 6b2621e Compare March 26, 2024 16:37

pablomartin4btc force-pushed the qml-ipaddress-valueinput branch from 6b2621e to d47bc1a Compare March 26, 2024 20:29

johnny9 approved these changes Mar 27, 2024

View reviewed changes

MarnixCroes approved these changes Mar 27, 2024

View reviewed changes

hebasto merged commit b6bf91f into bitcoin-core:main Mar 27, 2024
8 of 9 checks passed

pablomartin4btc mentioned this pull request Apr 3, 2024

Don't permit port in proxy IP option bitcoin-core/gui#813

Merged

pablomartin4btc added a commit to pablomartin4btc/bitcoin-core-gui-qml that referenced this pull request Apr 8, 2024

qml, component: Fix label on ProxySettings

3495832

Follow-up from bitcoin-core#391 (comment). Removing redundant Tor Proxy description from the top Default Proxy label.

pablomartin4btc mentioned this pull request Apr 8, 2024

Correct Default Proxy header description on ProxySettings - Follow-up #391 #395

Merged

pablomartin4btc mentioned this pull request Apr 8, 2024

Allow for selecting and copying text from the UI #394

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introducing an improved IP Address and Port input control in ProxySettings component #391

Introducing an improved IP Address and Port input control in ProxySettings component #391

pablomartin4btc commented Mar 22, 2024

johnny9 Mar 23, 2024

pablomartin4btc Mar 24, 2024

pablomartin4btc Mar 26, 2024

GBKS commented Mar 25, 2024

D33r-Gee left a comment

pablomartin4btc commented Mar 26, 2024

pablomartin4btc commented Mar 26, 2024

johnny9 left a comment •

edited

pablomartin4btc commented Mar 26, 2024

pablomartin4btc commented Mar 26, 2024

MarnixCroes left a comment

pablomartin4btc commented Mar 26, 2024 •

edited

pablomartin4btc commented Mar 26, 2024

MarnixCroes commented Mar 26, 2024

MarnixCroes commented Mar 26, 2024

pablomartin4btc commented Mar 26, 2024

pablomartin4btc commented Mar 26, 2024

pablomartin4btc commented Mar 26, 2024

johnny9 left a comment

MarnixCroes left a comment

stackingsaunter commented Apr 1, 2024

Introducing an improved IP Address and Port input control in ProxySettings component #391

Introducing an improved IP Address and Port input control in ProxySettings component #391

Conversation

pablomartin4btc commented Mar 22, 2024

johnny9 Mar 23, 2024

Choose a reason for hiding this comment

pablomartin4btc Mar 24, 2024

Choose a reason for hiding this comment

pablomartin4btc Mar 26, 2024

Choose a reason for hiding this comment

GBKS commented Mar 25, 2024

D33r-Gee left a comment

Choose a reason for hiding this comment

pablomartin4btc commented Mar 26, 2024

pablomartin4btc commented Mar 26, 2024

johnny9 left a comment • edited

Choose a reason for hiding this comment

pablomartin4btc commented Mar 26, 2024

pablomartin4btc commented Mar 26, 2024

MarnixCroes left a comment

Choose a reason for hiding this comment

pablomartin4btc commented Mar 26, 2024 • edited

pablomartin4btc commented Mar 26, 2024

MarnixCroes commented Mar 26, 2024

MarnixCroes commented Mar 26, 2024

pablomartin4btc commented Mar 26, 2024

pablomartin4btc commented Mar 26, 2024

pablomartin4btc commented Mar 26, 2024

johnny9 left a comment

Choose a reason for hiding this comment

MarnixCroes left a comment

Choose a reason for hiding this comment

stackingsaunter commented Apr 1, 2024

johnny9 left a comment •

edited

pablomartin4btc commented Mar 26, 2024 •

edited