Skip to content

Conversation

@Sjors
Copy link
Member

@Sjors Sjors commented Jul 31, 2018

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

Before:
schermafbeelding 2018-07-31 om 16 07 37

There are number of confusing aspects to this:

  1. it shows the default proxy URL, rather than the one in bitcoin.conf
  2. 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).
  3. the phrase "options that override above options" (it sometimes confuses me if that means anything)
  4. if the user wants to turn off the proxy by unchecking it, that won't actually work
  5. 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:
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 Sjors force-pushed the 2018/07/gui-proxy branch from dd6a03a to 6bde499 Compare July 31, 2018 14:24
@fanquake fanquake added the GUI label Jul 31, 2018
@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 31, 2018

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

Conflicts

Reviewers, 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.

@kallewoof
Copy link
Contributor

utACK 6bde499

@jonasschnelli jonasschnelli self-assigned this Aug 2, 2018
@laanwj
Copy link
Member

laanwj commented Aug 8, 2018

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

Copy link
Member

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?

Copy link
Member Author

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.

@jonasschnelli
Copy link
Contributor

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

@Sjors
Copy link
Member Author

Sjors commented Nov 30, 2018

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
Copy link
Member Author

Sjors commented Feb 22, 2019

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 DrahtBot closed this May 7, 2019
@DrahtBot DrahtBot reopened this May 7, 2019
MarkLTZ added a commit to litecoinz-core/litecoinz that referenced this pull request Nov 17, 2019
@DrahtBot DrahtBot closed this Mar 9, 2020
@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 9, 2020

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.

@DrahtBot DrahtBot reopened this Mar 9, 2020
@jonasschnelli
Copy link
Contributor

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

@Sjors
Copy link
Member Author

Sjors commented Jun 10, 2020

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

@Sjors Sjors closed this Jun 10, 2020
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants