make optionsmodel query real proxy state for ::data() #1899

Merged
merged 1 commit into from Oct 25, 2012

Conversation

Projects
None yet
6 participants
@Diapolo

Diapolo commented Oct 2, 2012

  • don't rely on the QSettings for cases ProxyUse and ProxySocksVersion and
    query the real values via the GetProxy() call
  • add a missing "succesful =" for case ProxyUse in ::setData()

This is based on #1859!

@TheBlueMatt

This comment has been minimized.

Show comment Hide comment
@TheBlueMatt

TheBlueMatt Oct 2, 2012

Contributor

See-also: TheBlueMatt/bitcoin@22e96a1 which was the first commit in #1453

Contributor

TheBlueMatt commented Oct 2, 2012

See-also: TheBlueMatt/bitcoin@22e96a1 which was the first commit in #1453

@laanwj

This comment has been minimized.

Show comment Hide comment
@laanwj

laanwj Oct 2, 2012

Member

@TheBlueMatt why is that part of the windows upgrader pull? :)

Member

laanwj commented Oct 2, 2012

@TheBlueMatt why is that part of the windows upgrader pull? :)

@TheBlueMatt

This comment has been minimized.

Show comment Hide comment
@TheBlueMatt

TheBlueMatt Oct 2, 2012

Contributor

Because there was discussion of the proxy stuff in that pull, so I coded it while I was on that branch...probably should have been separate, but...meh

Contributor

TheBlueMatt commented Oct 2, 2012

Because there was discussion of the proxy stuff in that pull, so I coded it while I was on that branch...probably should have been separate, but...meh

@Diapolo

This comment has been minimized.

Show comment Hide comment
@Diapolo

Diapolo Oct 2, 2012

@TheBlueMatt I didn't know that you had worked on that part, sorry. Apart from that I think my pull is smoother in the end, as we don't need a GetProxySocksVersion() function, which is the result from a discussion with @sipa and @laanwj.

I'm currently trying to overhaul the whole proxy stuff in the core and afterwards want to extend the Qt proxy / networking options with it.

Btw. funny thing is, that the starting point for my work on that part was your issue that we miss a lock for proxy structures :-D.

Diapolo commented Oct 2, 2012

@TheBlueMatt I didn't know that you had worked on that part, sorry. Apart from that I think my pull is smoother in the end, as we don't need a GetProxySocksVersion() function, which is the result from a discussion with @sipa and @laanwj.

I'm currently trying to overhaul the whole proxy stuff in the core and afterwards want to extend the Qt proxy / networking options with it.

Btw. funny thing is, that the starting point for my work on that part was your issue that we miss a lock for proxy structures :-D.

@TheBlueMatt

This comment has been minimized.

Show comment Hide comment
@TheBlueMatt

TheBlueMatt Oct 2, 2012

Contributor

No big deal, I just found it funny since IIRC that proxy stuff that I wrote was a result of a discussion with you on the auto-update thread.

Contributor

TheBlueMatt commented Oct 2, 2012

No big deal, I just found it funny since IIRC that proxy stuff that I wrote was a result of a discussion with you on the auto-update thread.

@Diapolo

This comment has been minimized.

Show comment Hide comment
@Diapolo

Diapolo Oct 2, 2012

I have to admit I did not have that in my mind anymore :-/, sorry. But I want to support you, to get that Windows update stuff in before 0.8. I hope this proxy stuff get's in soon (unsure about 0.7.1 though), so you don't need to rebase too often as some other code parts that are touched by your pull have changed already.

The more time I'm working on Bitcoin-Qt and with other devs the more insight I get, which leads to question former views I had on certain pulls / ideas :).

Diapolo commented Oct 2, 2012

I have to admit I did not have that in my mind anymore :-/, sorry. But I want to support you, to get that Windows update stuff in before 0.8. I hope this proxy stuff get's in soon (unsure about 0.7.1 though), so you don't need to rebase too often as some other code parts that are touched by your pull have changed already.

The more time I'm working on Bitcoin-Qt and with other devs the more insight I get, which leads to question former views I had on certain pulls / ideas :).

@laanwj

This comment has been minimized.

Show comment Hide comment
@laanwj

laanwj Oct 3, 2012

Member

@Diapolo yes we really need your help as windows dev to get that windows auto-updating stuff in :) Personally, I think that's more urgent than perfecting the settings dialog.

Member

laanwj commented Oct 3, 2012

@Diapolo yes we really need your help as windows dev to get that windows auto-updating stuff in :) Personally, I think that's more urgent than perfecting the settings dialog.

@Diapolo

This comment has been minimized.

Show comment Hide comment
@Diapolo

Diapolo Oct 4, 2012

@laanwj The current proxy related pulls are not there, only because I have some GUI ideas in my mind, they make proxy usage better and extend it with nice new stuff + fix the missing lock.

@TheBlueMatt As you opened the proxy lock issue, can you perhaps take a look at the code and ACK or give a short feedback? Can you rebase your Windows-update pull to current master or (if you want to) onto this one? I'll then try that whole stuff out in the following days :).

Edit: Btw., is the Pull tester offline?

Diapolo commented Oct 4, 2012

@laanwj The current proxy related pulls are not there, only because I have some GUI ideas in my mind, they make proxy usage better and extend it with nice new stuff + fix the missing lock.

@TheBlueMatt As you opened the proxy lock issue, can you perhaps take a look at the code and ACK or give a short feedback? Can you rebase your Windows-update pull to current master or (if you want to) onto this one? I'll then try that whole stuff out in the following days :).

Edit: Btw., is the Pull tester offline?

@sipa

This comment has been minimized.

Show comment Hide comment
@sipa

sipa Oct 4, 2012

Member

Didn't test, but ACK on changes to core.

Member

sipa commented Oct 4, 2012

Didn't test, but ACK on changes to core.

@TheBlueMatt

This comment has been minimized.

Show comment Hide comment
@TheBlueMatt

TheBlueMatt Oct 5, 2012

Contributor

@Diapolo Ill look at this in the next few days, and...not sure when Ill get around to updating the auto-update stuff... re: pull-tester, dont think so, but it seems like the jenkins server has mysteriously slowed down even more recently...not sure whats up with that (I chose to blame dnsseed, but I havent looked into it, so I cant blame @sipa for that)...Ive got some new hardware I may be able to throw into the mix, though, if I get the time to set up some sync mechanism...

Contributor

TheBlueMatt commented Oct 5, 2012

@Diapolo Ill look at this in the next few days, and...not sure when Ill get around to updating the auto-update stuff... re: pull-tester, dont think so, but it seems like the jenkins server has mysteriously slowed down even more recently...not sure whats up with that (I chose to blame dnsseed, but I havent looked into it, so I cant blame @sipa for that)...Ive got some new hardware I may be able to throw into the mix, though, if I get the time to set up some sync mechanism...

@BitcoinPullTester

This comment has been minimized.

Show comment Hide comment
@BitcoinPullTester

BitcoinPullTester Oct 6, 2012

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/63275485658818fa52ae5245e9a5dfd7f87ee84c for binaries and test log.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/63275485658818fa52ae5245e9a5dfd7f87ee84c for binaries and test log.

Philip Kaufmann
make optionsmodel query real proxy state for ::data()
- don't rely on the QSettings for cases ProxyUse and ProxySocksVersion and
  query the real values via the GetProxy() call
- add a missing "succesful =" for case ProxyUse in ::setData()
@BitcoinPullTester

This comment has been minimized.

Show comment Hide comment
@BitcoinPullTester

BitcoinPullTester Oct 20, 2012

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/5e5c102f2f901a19fda6ff877ab4195a88e4b703 for binaries and test log.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/5e5c102f2f901a19fda6ff877ab4195a88e4b703 for binaries and test log.

sipa added a commit that referenced this pull request Oct 25, 2012

Merge pull request #1899 from Diapolo/proxy_optionsmodel
make optionsmodel query real proxy state for ::data()

@sipa sipa merged commit e74d0ab into bitcoin:master Oct 25, 2012

@luke-jr

This comment has been minimized.

Show comment Hide comment
@luke-jr

luke-jr Nov 12, 2012

Any reason this is a fixed value of 5 instead of nSocksVersion setting?

Any reason this is a fixed value of 5 instead of nSocksVersion setting?

This comment has been minimized.

Show comment Hide comment
@Diapolo

Diapolo Nov 13, 2012

Owner

Because SOCKS5 is default, take a look above. The same syntax was used before for port or IP.

Owner

Diapolo replied Nov 13, 2012

Because SOCKS5 is default, take a look above. The same syntax was used before for port or IP.

@luke-jr

This comment has been minimized.

Show comment Hide comment
@luke-jr

luke-jr Nov 12, 2012

Is this a bug fix, or a behaviour change?

Is this a bug fix, or a behaviour change?

This comment has been minimized.

Show comment Hide comment
@Diapolo

Diapolo Nov 13, 2012

Owner

It harmonizes the behaviour with what was there...

Owner

Diapolo replied Nov 13, 2012

It harmonizes the behaviour with what was there...

laudney pushed a commit to reddcoin-project/reddcoin that referenced this pull request Mar 19, 2014

Merge pull request #1899 from Diapolo/proxy_optionsmodel
make optionsmodel query real proxy state for ::data()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment