-
Notifications
You must be signed in to change notification settings - Fork 38.7k
More intuitive GUI settings behavior when -proxy is set #13818
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
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
utACK 6bde499 |
|
Concept ACK |
src/qt/optionsdialog.cpp
Outdated
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.
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?
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.
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.
6bde499 to
b39ca20
Compare
|
utACK b39ca20 |
|
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. |
|
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. |
| The last travis run for this pull request was 307 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened. |
|
@Sjors: could you have a look in a better automated test approach for this? |
|
Closing this for now until the smoke clears around other settings PRs. |
Given
proxy=127.0.0.1:9051bitcoin.conf (or using-proxy):Before:

There are number of confusing aspects to this:
bitcoin.confSo 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
-proxyis 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
-proxyis set. They have to remove the entry frombitcoin.conffirst.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.confit should be similarly disabled in the GUI, along with an instruction to remove it frombitcoin.confif the user wishes to edit it (using the read-writebitcoin-rw.conf).This UX pattern can be applied to a few other settings as well. I found
proxyparticularly useful because it's used with Tor where confusion is really not a good thing.I'm guessing that a separate Tor proxy through
-onionis less common, so I didn't touch that (yet). I find that setting confusing in general.