[Qt] let OptionsModel::getProxySettings() directly query proxy #3452

pull Diapolo wants to merge 1 commits into bitcoin:master from Diapolo:paymentserv-proxy changing 4 files +39 −19

Diapolo commented at 5:56 PM on December 20, 2013: none
- as a proxy set via GUI can be overridden via -proxy, directly query the core to get active proxy
- give a warning, if active proxy is not SOCKS5 (needs to be SOCKS5 for the Qt networking code to work)
- also remove an obsolete connect() call from optionsdialog.cpp and a reference to Bitcoin-Qt (now just GUI)
laanwj commented at 9:20 AM on December 21, 2013: member

The OptionsModel is supposed to read and write the settings from the core. That's the whole point of it, to represent the settings in the core in a format more useful to the GUI.

Whatever the problem is, don't try to bypass it, but fix OptionsModel instead :)
Diapolo commented at 9:27 AM on December 22, 2013: none

It currently doesn't handle non SOCKS5 proxies, I can move the code to OptionsModel.
laanwj commented at 4:13 PM on January 7, 2014: member

Yes, please do that. I agree with these changes, but not with moving option querying functionality into PaymentServer
Diapolo commented at 7:59 AM on January 8, 2014: none

Reworked the code and moved changes to OptionsModel as you suggested.
laanwj commented at 5:58 PM on January 8, 2014: member

ACK on code changes, still need to test
laanwj commented at 11:58 AM on January 10, 2014: member

Needs rebase after #3347
Diapolo commented at 3:02 PM on January 10, 2014: none

Rebased and also removed an obsolete connect() call.
in src/qt/optionsmodel.cpp:None in 93311a00dc outdated
```
 365 | @@ -366,15 +366,20 @@ bool OptionsModel::setData(const QModelIndex & index, const QVariant & value, in
 366 |      return successful;
 367 |  }
 368 |  
 369 | -bool OptionsModel::getProxySettings(QString& proxyIP, quint16 &proxyPort) const
 370 | +bool OptionsModel::getProxySettings(QString& proxyIP, quint16 &proxyPort, qint32 &proxySocksVersion) const
```
laanwj commented at 10:55 AM on January 11, 2014:

Just a thought: wouldn't it be nicer to make this return a QNetworkProxy instead of IP/Port/Version?

bool OptionsModel::getProxySettings(QNetworkProxy &proxy)

If there is no proxy, it could set the type of the returned proxy to QNetworkProxy::NoProxy and return success.

The return boolean could be used for error status (if there is a proxy, but it is unsupported by Qt).
Diapolo commented at 6:06 PM on January 11, 2014:

Seems like a nice suggestion, I'll re-work this tomorrow, thanks.
Diapolo commented at 3:46 PM on January 16, 2014: none

@laanwj Reworked to pass a reference to QNetworkProxy to getProxySettings().
laanwj commented at 11:50 AM on January 17, 2014: member

ACK

[Qt] let OptionsModel::getProxySettings() directly query proxy

- as a proxy set via GUI can be overridden via -proxy, directly query the
  core to get active proxy
- give a warning, if active proxy is not SOCKS5 (needs to be SOCKS5 for
  the Qt networking code to work)
- also remove an obsolete connect() call from optionsdialog.cpp and a
  reference to Bitcoin-Qt (now just GUI)

1ba3560fe8

Diapolo commented at 2:44 PM on January 17, 2014: none

Just updated a formatting and removed an obsolete note from the commit-msg.
BitcoinPullTester commented at 3:22 PM on January 17, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/1ba3560fe870dac8d27d75671c483eaa4e0009ff 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.
laanwj referenced this in commit 2f06b5965a on Jan 22, 2014
laanwj merged this on Jan 22, 2014
laanwj closed this on Jan 22, 2014
Diapolo deleted the branch on Jan 22, 2014
DrahtBot locked this on Sep 8, 2021

Contributors

Diapolo

laanwj

BitcoinPullTester