More intuitive GUI settings behavior when -proxy is set

Sjors commented at 2:23 pm on July 31, 2018: member

Given proxy=127.0.0.1:9051 bitcoin.conf (or using -proxy):

Before:

There are number of confusing aspects to this:

it shows the default proxy URL, rather than the one in bitcoin.conf
the check box is unchecked, even though the proxy is enabled. If the user checks it themselves (for aesthetic reasons?), that leads to problem (4) and (5).
the phrase “options that override above options” (it sometimes confuses me if that means anything)
if the user wants to turn off the proxy by unchecking it, that won’t actually work
if the user tries to change the IP or port that won’t do anything

So I changed the behavior to check the box and populate the initial setting (only done at first launch or after you reset QT settings) if -proxy is set (perhaps it’s even to always do this?).

In addition the user can no longer disable the check box or edit the settings when -proxy is set. They have to remove the entry from bitcoin.conf first.

After:

#11082 and #12833 are a more rigorous solution, but there’s some overlap. E.g. if a setting exists in the read-only bitcoin.conf it should be similarly disabled in the GUI, along with an instruction to remove it from bitcoin.conf if the user wishes to edit it (using the read-write bitcoin-rw.conf).

This UX pattern can be applied to a few other settings as well. I found proxy particularly useful because it’s used with Tor where confusion is really not a good thing.

I’m guessing that a separate Tor proxy through -onion is less common, so I didn’t touch that (yet). I find that setting confusing in general.

Sjors force-pushed on Jul 31, 2018

fanquake added the label GUI on Jul 31, 2018

DrahtBot commented at 3:13 pm on July 31, 2018: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15936 (Unify bitcoin-qt and bitcoind persistent settings by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/qt/optionsdialog.cpp:71 in 6bde499f27 outdated

69+    if (!gArgs.GetArg("-proxy", "").empty()) {
70+        ui->connectSocks->setEnabled(false);
71+    } else {
72+        connect(ui->connectSocks, SIGNAL(toggled(bool)), ui->proxyIp, SLOT(setEnabled(bool)));
73+        connect(ui->connectSocks, SIGNAL(toggled(bool)), ui->proxyPort, SLOT(setEnabled(bool)));
74+        connect(ui->connectSocks, SIGNAL(toggled(bool)), this, SLOT(updateProxyValidationState()));

MarcoFalke commented at 4:36 pm on July 31, 2018:

nit: Could use new connect syntax to avoid having to change it again in the future.

Sjors commented at 4:26 pm on August 1, 2018:

@MarcoFalke since I’m only moving these lines, it’s probably better to keep them the same. It probably makes rebasing easier regardless of whether #13529 or this is merged first.

MarcoFalke commented at 3:33 pm on August 8, 2018:

Since this pull has to be tested anyway, testing would also cover the transition to the new syntax. And wouldn’t have to be repeated later on. Though, no strong opinion, just a nit.

Sjors commented at 8:29 am on September 4, 2018:

Rebased, so uses the new syntax now.

kallewoof commented at 5:22 am on August 1, 2018: member

utACK 6bde499

jonasschnelli assigned jonasschnelli on Aug 2, 2018

laanwj commented at 2:37 pm on August 8, 2018: member

Concept ACK This is code I really wish we (could) have tests for, it has grown so complex.

in src/qt/optionsdialog.cpp:69 in 6bde499f27 outdated

67+    // Disable and don't connect proxy checkbox interaction if proxy= is set by
68+    // a param, to make it clear that removing the param / config is prerequisite to disabling.
69+    if (!gArgs.GetArg("-proxy", "").empty()) {
70+        ui->connectSocks->setEnabled(false);
71+    } else {
72+        connect(ui->connectSocks, SIGNAL(toggled(bool)), ui->proxyIp, SLOT(setEnabled(bool)));

laanwj commented at 2:38 pm on August 8, 2018:

Not a big issue but to me it seems overkill to both disable the control and not connect the signals, isn’t only disabling enough?

Sjors commented at 3:27 pm on August 8, 2018:

These signal connections re-enable those fields, even if I disable them in the next line. I probably could have fixed with an async call somewhere, but this was easier.

DrahtBot added the label Needs rebase on Aug 21, 2018

Initial GUI proxy settings reflect -proxy param 00cf5cbcc0

[GUI] settings: if -proxy is set, disable interaction b39ca20e77

Sjors force-pushed on Sep 4, 2018

DrahtBot removed the label Needs rebase on Sep 4, 2018

jonasschnelli commented at 7:03 pm on October 17, 2018: contributor

utACK b39ca20e771e572350bace4ac61821e14fc6e7f9 I feel uncomfortable merging this. We need tests for the proxy settings (in general for the settings window).

Sjors commented at 11:51 am on November 30, 2018: member

Not strictly necessary, but it may also be safer to wait for #12833.

I agree with the need to add test, though it may be a while before I get to it. Happy to cherry-pick and review tests by someone else in the mean time.

Sjors commented at 11:04 am on February 22, 2019: member

Recently increased enthusiasm for rw_config means it’s better to work on top of that once merged. I agree that adding tests would be a good idea; it’s probably going to be a while before I get to that though.

DrahtBot closed this on May 7, 2019

DrahtBot reopened this on May 7, 2019

MarkLTZ referenced this in commit 2ffd1bfc2f on Nov 17, 2019

DrahtBot closed this on Mar 9, 2020

DrahtBot commented at 8:35 pm on March 9, 2020: member

DrahtBot reopened this on Mar 9, 2020

jonasschnelli commented at 7:04 am on May 29, 2020: contributor

@Sjors: could you have a look in a better automated test approach for this?

jonasschnelli added the label Waiting for author on May 29, 2020

Sjors commented at 1:34 pm on June 10, 2020: member

Closing this for now until the smoke clears around other settings PRs.

Sjors closed this on Jun 10, 2020

DrahtBot locked this on Feb 15, 2022

More intuitive GUI settings behavior when -proxy is set #13818

Conflicts