[Qt] allow users to set -onion via GUI #4587

Merged
merged 1 commit into from Aug 7, 2015

Conversation

Projects
None yet
5 participants
@Diapolo

Diapolo commented Jul 25, 2014

  • also allow users to see, if the default proxy (-proxy) is used for IPv6
    and Onion
  • based on #4871 (formerly #2575)

proxy

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 26, 2014

Member

Looks good.

As discussed in #2575 I'd like to simplify this to:

  • A clearnet proxy for IPv4 and IPv6 and name-based lookups (except *.onion, and possibly other 'darknet's in the future)
  • A proxy for .onion. This defaults to the clearnet proxy *if the onion network is enabled at all.
Member

laanwj commented Jul 26, 2014

Looks good.

As discussed in #2575 I'd like to simplify this to:

  • A clearnet proxy for IPv4 and IPv6 and name-based lookups (except *.onion, and possibly other 'darknet's in the future)
  • A proxy for .onion. This defaults to the clearnet proxy *if the onion network is enabled at all.

@laanwj laanwj added the GUI label Jul 31, 2014

@Diapolo Diapolo changed the title from [Qt] allow users to set -proxy6 and -tor via GUI to [Qt] allow users to set -tor via GUI Sep 23, 2014

@Diapolo Diapolo changed the title from [Qt] allow users to set -tor via GUI to [Qt] allow users to set -onion via GUI Sep 23, 2014

@Diapolo

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Sep 23, 2014

@laanwj Removed the -proxy6 stuff and rebased on the recently updated core pull.

Diapolo commented Sep 23, 2014

@laanwj Removed the -proxy6 stuff and rebased on the recently updated core pull.

@BitcoinPullTester

This comment has been minimized.

Show comment
Hide comment
@BitcoinPullTester

BitcoinPullTester Sep 23, 2014

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4587_776d006492d75c94cbff1edf7fae760f0a95ebcf/ for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4587_776d006492d75c94cbff1edf7fae760f0a95ebcf/ for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jan 9, 2015

Member

Needs rebase or close due inactivity.

Member

jonasschnelli commented Jan 9, 2015

Needs rebase or close due inactivity.

@Diapolo

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Jan 9, 2015

No, this needs review and help with #4871, which no one is interessted in :D.

Diapolo commented Jan 9, 2015

No, this needs review and help with #4871, which no one is interessted in :D.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jan 9, 2015

Member

Sorry. Your right. It's on top of #4871

Member

jonasschnelli commented Jan 9, 2015

Sorry. Your right. It's on top of #4871

@Diapolo

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo May 14, 2015

Rebased to latest #4871 and fixed a small bug that was causing -onion to be missing in the options dialog, when the GUI setting was overridden.

Diapolo commented May 14, 2015

Rebased to latest #4871 and fixed a small bug that was causing -onion to be missing in the options dialog, when the GUI setting was overridden.

@Diapolo

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Jun 15, 2015

Same here, this will be rebased after #6272 is merged.

Diapolo commented Jun 15, 2015

Same here, this will be rebased after #6272 is merged.

@Diapolo

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Jun 19, 2015

Rebased and passing travis :).

Diapolo commented Jun 19, 2015

Rebased and passing travis :).

@Diapolo

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Jun 23, 2015

@laanwj @jonasschnelli Mind taking a closer look now?

Diapolo commented Jun 23, 2015

@laanwj @jonasschnelli Mind taking a closer look now?

@Diapolo

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Jul 21, 2015

@jonasschnelli @laanwj This should be ready.

Diapolo commented Jul 21, 2015

@jonasschnelli @laanwj This should be ready.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jul 21, 2015

Member

Gave it a short test.

Why can't i unselect/select the IPv4/IPv6/Tor checkboxes (i didn't start reading why in the PRs description because IMO this must be clear within the app). If it's just as a visual prove what networks are supported by the proxy, it should not use read-only checkboxes.

The new height of the settings window does somehow look strange on the other tabs.
Auto-Resizing like this (https://vid.me/F53C) would be a better approach (check: http://stackoverflow.com/questions/11441809/resize-window-using-qpropertyanimation).

IMO the autoresizing would be nice but should not hold this PR back.

Member

jonasschnelli commented Jul 21, 2015

Gave it a short test.

Why can't i unselect/select the IPv4/IPv6/Tor checkboxes (i didn't start reading why in the PRs description because IMO this must be clear within the app). If it's just as a visual prove what networks are supported by the proxy, it should not use read-only checkboxes.

The new height of the settings window does somehow look strange on the other tabs.
Auto-Resizing like this (https://vid.me/F53C) would be a better approach (check: http://stackoverflow.com/questions/11441809/resize-window-using-qpropertyanimation).

IMO the autoresizing would be nice but should not hold this PR back.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
Member

jonasschnelli commented Jul 21, 2015

bildschirmfoto 2015-07-21 um 21 28 22

@Diapolo

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Jul 21, 2015

@jonasschnelli I choose the read-only checkbox approach as that seemed easy to do. What elemet do you suggest? As you discovered the state is not changable via the boxes, they just give information.

The resize issue is a different one and as far as I understand the link you gave me, this is just for getting a nice and smooth animation, but I don't even know how to resize a window so that the contents and size are matching and that it looks good ;). IMHO our client suffers from these issue nearly everywhere.

Diapolo commented Jul 21, 2015

@jonasschnelli I choose the read-only checkbox approach as that seemed easy to do. What elemet do you suggest? As you discovered the state is not changable via the boxes, they just give information.

The resize issue is a different one and as far as I understand the link you gave me, this is just for getting a nice and smooth animation, but I don't even know how to resize a window so that the contents and size are matching and that it looks good ;). IMHO our client suffers from these issue nearly everywhere.

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Jul 23, 2015

Contributor

ut ACK

Is this ready for merging? Blockers appear to be resolved.

Contributor

jgarzik commented Jul 23, 2015

ut ACK

Is this ready for merging? Blockers appear to be resolved.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jul 23, 2015

Member

Readonly-Checkboxes as visual indicator for a available option (but while actually supporting no interaction) should be avoided. But i can address this in another upcoming PR after merging.

I would recommend to use the checkmark icon instead (https://github.com/bitcoin/bitcoin/blob/master/src/qt/res/icons/transaction2.png).

Member

jonasschnelli commented Jul 23, 2015

Readonly-Checkboxes as visual indicator for a available option (but while actually supporting no interaction) should be avoided. But i can address this in another upcoming PR after merging.

I would recommend to use the checkmark icon instead (https://github.com/bitcoin/bitcoin/blob/master/src/qt/res/icons/transaction2.png).

@Diapolo

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Jul 24, 2015

@jonasschnelli I'd love to get this in or you could just add a commit to it. But I'd love to move forward.

Diapolo commented Jul 24, 2015

@jonasschnelli I'd love to get this in or you could just add a commit to it. But I'd love to move forward.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jul 24, 2015

Member

Doesn't compile at the moment because of the recent "emit" changes (#6433).

Member

jonasschnelli commented Jul 24, 2015

Doesn't compile at the moment because of the recent "emit" changes (#6433).

Philip Kaufmann
[Qt] allow users to set -onion via GUI
- also allow users to see, if the default proxy (-proxy) is used for
  reaching peers via IPv6 or Tor
@Diapolo

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Jul 24, 2015

@jonasschnelli fixed the emit problem.

Diapolo commented Jul 24, 2015

@jonasschnelli fixed the emit problem.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jul 24, 2015

Member

bildschirmfoto 2015-07-24 um 12 43 12

bildschirmfoto 2015-07-24 um 12 55 46

I think this looks better.
Commit: jonasschnelli@3ae2434

Member

jonasschnelli commented Jul 24, 2015

bildschirmfoto 2015-07-24 um 12 43 12

bildschirmfoto 2015-07-24 um 12 55 46

I think this looks better.
Commit: jonasschnelli@3ae2434

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Aug 7, 2015

Member

@jonasschhnelli I like what you're trying to do, but I'm not sure it's better:

  • The checkbox icon is always black, also in themes that themselves have a black background
  • It doesn't fit in with the rest of the UI (check styles are slightly different)
Member

laanwj commented Aug 7, 2015

@jonasschhnelli I like what you're trying to do, but I'm not sure it's better:

  • The checkbox icon is always black, also in themes that themselves have a black background
  • It doesn't fit in with the rest of the UI (check styles are slightly different)
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Aug 7, 2015

Member

Going to merge this as-is for now. You can improve it later.

Member

laanwj commented Aug 7, 2015

Going to merge this as-is for now. You can improve it later.

@laanwj laanwj merged commit ed166df into bitcoin:master Aug 7, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Aug 7, 2015

Merge pull request #4587
ed166df [Qt] allow users to set -onion via GUI (Philip Kaufmann)
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
Member

jonasschnelli commented Aug 7, 2015

ACK.

@Diapolo Diapolo deleted the Diapolo:proxy-gui branch Aug 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment