Skip to content
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.

Sign up for GitHub

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

Jump to bottom

gui: Fix issue: "default port not shown correctly in settings dialog" #12650

Merged
merged 1 commit into from Apr 11, 2018
Merged

gui: Fix issue: "default port not shown correctly in settings dialog" #12650

merged 1 commit into from Apr 11, 2018
+21 −2

Conversation

l2a5b1
Copy link
Contributor

@l2a5b1 l2a5b1 commented Mar 8, 2018

In f05d349 the value of the addrProxy and addrSeparateProxyTor settings is set to an illegal default value, because the value of DEFAULT_GUI_PROXY_PORT is passed to the fieldWidth parameter of the QString QString::arg(const QString &a, int fieldWidth = 0, QChar fillChar = QLatin1Char( ' ' )) const method:

bitcoin/src/qt/optionsmodel.cpp

Line 129 in 29fad97

settings.setValue("addrProxy", QString("%1:%2").arg(DEFAULT_GUI_PROXY_HOST, DEFAULT_GUI_PROXY_PORT));

bitcoin/src/qt/optionsmodel.cpp

Line 139 in 29fad97

settings.setValue("addrSeparateProxyTor", QString("%1:%2").arg(DEFAULT_GUI_PROXY_HOST, DEFAULT_GUI_PROXY_PORT));

This will create a default proxy setting that consists of 9053 characters and ends with the string 127.0.0.1:%2.

This PR attempts to resolve #12623 by setting the correct value for the addrProxy and addrSeparateProxyTor settings (i) if the proxy setting does not exist; or (ii) if the proxy setting has an illegal value caused by to the aforementioned bug.

The second condition is only relevant if we don't want Bitcoin Core 0.16.0 users to explicitly reset their settings to see the correct default proxy port value.

@laanwj
Copy link
Member

laanwj commented Mar 10, 2018

Concept ACK, soryr for making such a stupid mistake.

The second condition is only relevant if we don't want Bitcoin Core 0.16.0 users to explicitly reset their settings to see the correct default proxy port value.

If we add that workaround, let's at least add a comment that explains that workaround, so that future maintainers know why it's there and when it can be removed.

@l2a5b1 l2a5b1 force-pushed the patch/12623/default-port-not-shown branch from 396b44f to 9ff17ee Compare March 10, 2018 17:08
@l2a5b1
Copy link
Contributor Author

l2a5b1 commented Mar 10, 2018

Thanks @laanwj! I have added a comment to both conditional statements to clarify the purpose of the second boolean condition.

@maflcko maflcko added this to the 0.16.1 milestone Mar 11, 2018
@bitcoin bitcoin deleted a comment from DavidWard1980 Mar 15, 2018
@maflcko maflcko mentioned this pull request Mar 17, 2018
Merged
Sjors
Sjors reviewed Apr 3, 2018
View reviewed changes
Copy link
Member

@Sjors Sjors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

src/qt/optionsmodel.cpp Outdated
if (!settings.contains("addrProxy"))
settings.setValue("addrProxy", QString("%1:%2").arg(DEFAULT_GUI_PROXY_HOST, DEFAULT_GUI_PROXY_PORT));
// The second condition in the boolean expression is a workaround to overwrite the 'addrProxy' setting
// in case it has been set to an illegal default value by release 0.16.0 (see issue #12623; PR #12650).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move to OptionsModel::checkAndMigrate()?

@l2a5b1 l2a5b1 force-pushed the patch/12623/default-port-not-shown branch from 9ff17ee to 6bb2adb Compare April 6, 2018 22:14
@l2a5b1
Copy link
Contributor Author

l2a5b1 commented Apr 6, 2018

He @Sjors, thanks! I took a stab at addressing your suggestion in 6bb2adb and rebased this feature branch onto master. I committed on top of 87bd26b in case you prefer that version and want me to revert the changes.

@l2a5b1 l2a5b1 force-pushed the patch/12623/default-port-not-shown branch from 6bb2adb to 0592609 Compare April 7, 2018 13:44
maflcko
maflcko reviewed Apr 7, 2018
View reviewed changes
src/qt/optionsmodel.cpp Outdated
@@ -485,4 +493,16 @@ void OptionsModel::checkAndMigrate()

settings.setValue(strSettingsVersionKey, CLIENT_VERSION);
}

// Overwrite the 'addrProxy' setting in case it has been set to an illegal
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trailing whitespace in this line

@l2a5b1 l2a5b1 force-pushed the patch/12623/default-port-not-shown branch 2 times, most recently from f120e8a to 40141b7 Compare April 7, 2018 21:03
@l2a5b1
Copy link
Contributor Author

l2a5b1 commented Apr 7, 2018 •
edited

Thanks @MarcoFalke! 1cd545e is rebased onto the master (048ac83) and addresses the trailing whitespace character.

@l2a5b1 l2a5b1 force-pushed the patch/12623/default-port-not-shown branch 2 times, most recently from 1cd545e to 2f3ce02 Compare April 7, 2018 22:09
@l2a5b1
Copy link
Contributor Author

l2a5b1 commented Apr 7, 2018

squashed previous work in 2f3ce02, which is based on @Sjors' suggestion:

Move to OptionsModel::checkAndMigrate()?

Sjors
Sjors approved these changes Apr 9, 2018
View reviewed changes
Copy link
Member

@Sjors Sjors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK 2f3ce02

Nit: maybe change "Addresses" to "Fix" in your commit message, if only so it fits in 72 characters.

@jonasschnelli
Copy link
Contributor

utACK 2f3ce024eb36f93f82f4fc644d57c45639b9f3de

Nods for new migration/auto-fix code without EOL strategy.

@l2a5b1 l2a5b1 force-pushed the patch/12623/default-port-not-shown branch from 2f3ce02 to 40c5886 Compare April 10, 2018 20:27
@l2a5b1
Copy link
Contributor Author

l2a5b1 commented Apr 10, 2018

Thanks @jonasschnelli, @Sjors! @Sjors: I have addressed your feedback and changed the commit message in 40c5886 to make it fit in 72 characters.

@Sjors
Copy link
Member

Sjors commented Apr 11, 2018 •
edited

reACK 40c5886

@laanwj laanwj merged commit 40c5886 into bitcoin:master Apr 11, 2018
laanwj added a commit that referenced this pull request Apr 11, 2018
@laanwj
Merge #12650: gui: Fix issue: "default port not shown correctly in se…
f15b72f 
…ttings dialog"

40c5886 Fix illegal default `addProxy` and `addrSeparateProxyTor` settings. (251)

Pull request description:

  In f05d349 the value of the `addrProxy` and `addrSeparateProxyTor` settings is set to an illegal default value, because the value of `DEFAULT_GUI_PROXY_PORT ` is passed to the `fieldWidth` parameter of the `QString QString::arg(const QString &a, int fieldWidth = 0, QChar fillChar = QLatin1Char( ' ' )) const` method:

  https://github.com/bitcoin/bitcoin/blob/29fad97c320c892ab6a480c81e2078ec22ab354b/src/qt/optionsmodel.cpp#L129

  https://github.com/bitcoin/bitcoin/blob/29fad97c320c892ab6a480c81e2078ec22ab354b/src/qt/optionsmodel.cpp#L139

  This will create a default proxy setting that consists of 9053 characters and ends with the string `127.0.0.1:%2`.

  This PR attempts to resolve #12623 by setting the correct value for the `addrProxy` and `addrSeparateProxyTor` settings (i) if the proxy setting does not exist; or (ii) if the proxy setting has an illegal value caused by to the aforementioned bug.

  The second condition is *only* relevant if we don't want Bitcoin Core 0.16.0 users to explicitly reset their settings to see the correct default proxy port value.

Tree-SHA512: 3dc3de2eb7da831f6e318797df67341ced2076b48f9b561c73677bf6beb67b259d8e413095f290356fb92e32e4e8162d48accbc575c4e612060fd5d6dde7ac8d
@bitcoin bitcoin deleted a comment from WorkShop-Office Apr 11, 2018
@maflcko
Copy link
Member

maflcko commented Apr 11, 2018

Tagged for backport in case there is a 16.1 release

@fanquake fanquake mentioned this pull request Apr 12, 2018
Merged
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Apr 26, 2018
@l2a5b1 @fanquake
Fix illegal default addProxy and addrSeparateProxyTor settings.
f118a7a 
GitHub-Pull: bitcoin#12650
Rebased-From: 40c5886
laanwj added a commit that referenced this pull request May 16, 2018
@laanwj
Merge #12967: [0.16] Backports
feba12f 
8fca086 List support for BIP173 in bips.md (Pieter Wuille)
9645aa6 Remove blockmaxsize option from init.cpp (fanquake)
7847b92 Default to defining endian-conversion DECLs in compat w/o config (Matt Corallo)
1720eb3 qt:Show the entire Window when double clicking on taskbar (Chun Kuan Lee)
e055bc0 depends: Fix Qt build with XCode 9.3 (fanquake)
0684cf9 Avoid launching as admin when NSIS installer ends. (JeremyRand)
e802c22 [config] Remove blockmaxsize option (John Newbery)
f118a7a Fix illegal default `addProxy` and `addrSeparateProxyTor` settings. (251)
f60e84d Limit the number of IPs we use from each DNS seeder (e0)

Pull request description:

  Backports:
  - #12626 Limit the number of IPs addrman learns from each DNS seeder
  - #12650 gui: Fix issue: "default port not shown correctly in settings dialog"
  - #12756 [config] Remove blockmaxsize option
  - #12985 Windows: Avoid launching as admin when NSIS installer ends.
  - #12946 depends: Fix Qt build with XCode 9.3
  - #12998 Default to defining endian-conversion DECLs in compat w/o config
  - #12999 qt: Show the Window when double clicking the taskbar icon
  - #13064 List support for BIP173 in bips.md

  to the 0.16 branch.

Tree-SHA512: 3e6b47c54b2cd2bdd81fbc6176cb31e46423f6e05988984d3a09b3535e3cee101ffb071cf753a4beff3c9f0521eb5de4b7c0424a3e97da801d56b4015847ac0f
@fanquake
Copy link
Member

Backported in #12967

HashUnlimited pushed a commit to chaincoin/chaincoin that referenced this pull request Jun 29, 2018
@l2a5b1
Fix illegal default addProxy and addrSeparateProxyTor settings.
afcfda0 
GitHub-Pull: bitcoin#12650
Rebased-From: 40c5886
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Aug 30, 2019
@laanwj
Merge #12650: gui: Fix issue: "default port not shown correctly in se…
97ff16f 
…ttings dialog"

Summary:
40c5886 Fix illegal default `addProxy` and `addrSeparateProxyTor` settings. (251)

Pull request description:

  In f05d349 the value of the `addrProxy` and `addrSeparateProxyTor` settings is set to an illegal default value, because the value of `DEFAULT_GUI_PROXY_PORT ` is passed to the `fieldWidth` parameter of the `QString QString::arg(const QString &a, int fieldWidth = 0, QChar fillChar = QLatin1Char( ' ' )) const` method:

  https://github.com/bitcoin/bitcoin/blob/29fad97c320c892ab6a480c81e2078ec22ab354b/src/qt/optionsmodel.cpp#L129

  https://github.com/bitcoin/bitcoin/blob/29fad97c320c892ab6a480c81e2078ec22ab354b/src/qt/optionsmodel.cpp#L139

  This will create a default proxy setting that consists of 9053 characters and ends with the string `127.0.0.1:%2`.

  This PR attempts to resolve #12623 by setting the correct value for the `addrProxy` and `addrSeparateProxyTor` settings (i) if the proxy setting does not exist; or (ii) if the proxy setting has an illegal value caused by to the aforementioned bug.

  The second condition is *only* relevant if we don't want Bitcoin Core 0.16.0 users to explicitly reset their settings to see the correct default proxy port value.

Tree-SHA512: 3dc3de2eb7da831f6e318797df67341ced2076b48f9b561c73677bf6beb67b259d8e413095f290356fb92e32e4e8162d48accbc575c4e612060fd5d6dde7ac8d

Backport of Core PR12650
bitcoin/bitcoin#12650

Test Plan:
  make check
  bitcoin-qt -> settings -> options -> network -> verify proxy port is 9050

Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3885
jonspock pushed a commit to jonspock/devault that referenced this pull request Dec 8, 2019
@laanwj @jonspock
Merge #12650: gui: Fix issue: "default port not shown correctly in se…
5932fd7 
…ttings dialog"

Summary:
40c5886 Fix illegal default `addProxy` and `addrSeparateProxyTor` settings. (251)

Pull request description:

  In f05d349 the value of the `addrProxy` and `addrSeparateProxyTor` settings is set to an illegal default value, because the value of `DEFAULT_GUI_PROXY_PORT ` is passed to the `fieldWidth` parameter of the `QString QString::arg(const QString &a, int fieldWidth = 0, QChar fillChar = QLatin1Char( ' ' )) const` method:

  https://github.com/bitcoin/bitcoin/blob/29fad97c320c892ab6a480c81e2078ec22ab354b/src/qt/optionsmodel.cpp#L129

  https://github.com/bitcoin/bitcoin/blob/29fad97c320c892ab6a480c81e2078ec22ab354b/src/qt/optionsmodel.cpp#L139

  This will create a default proxy setting that consists of 9053 characters and ends with the string `127.0.0.1:%2`.

  This PR attempts to resolve #12623 by setting the correct value for the `addrProxy` and `addrSeparateProxyTor` settings (i) if the proxy setting does not exist; or (ii) if the proxy setting has an illegal value caused by to the aforementioned bug.

  The second condition is *only* relevant if we don't want Bitcoin Core 0.16.0 users to explicitly reset their settings to see the correct default proxy port value.

Tree-SHA512: 3dc3de2eb7da831f6e318797df67341ced2076b48f9b561c73677bf6beb67b259d8e413095f290356fb92e32e4e8162d48accbc575c4e612060fd5d6dde7ac8d

Backport of Core PR12650
bitcoin/bitcoin#12650

Test Plan:
  make check
  bitcoin-qt -> settings -> options -> network -> verify proxy port is 9050

Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3885
jonspock pushed a commit to jonspock/devault that referenced this pull request Dec 8, 2019
@laanwj @jonspock
Merge #12650: gui: Fix issue: "default port not shown correctly in se…
102093c 
…ttings dialog"

Summary:
40c5886 Fix illegal default `addProxy` and `addrSeparateProxyTor` settings. (251)

Pull request description:

  In f05d349 the value of the `addrProxy` and `addrSeparateProxyTor` settings is set to an illegal default value, because the value of `DEFAULT_GUI_PROXY_PORT ` is passed to the `fieldWidth` parameter of the `QString QString::arg(const QString &a, int fieldWidth = 0, QChar fillChar = QLatin1Char( ' ' )) const` method:

  https://github.com/bitcoin/bitcoin/blob/29fad97c320c892ab6a480c81e2078ec22ab354b/src/qt/optionsmodel.cpp#L129

  https://github.com/bitcoin/bitcoin/blob/29fad97c320c892ab6a480c81e2078ec22ab354b/src/qt/optionsmodel.cpp#L139

  This will create a default proxy setting that consists of 9053 characters and ends with the string `127.0.0.1:%2`.

  This PR attempts to resolve #12623 by setting the correct value for the `addrProxy` and `addrSeparateProxyTor` settings (i) if the proxy setting does not exist; or (ii) if the proxy setting has an illegal value caused by to the aforementioned bug.

  The second condition is *only* relevant if we don't want Bitcoin Core 0.16.0 users to explicitly reset their settings to see the correct default proxy port value.

Tree-SHA512: 3dc3de2eb7da831f6e318797df67341ced2076b48f9b561c73677bf6beb67b259d8e413095f290356fb92e32e4e8162d48accbc575c4e612060fd5d6dde7ac8d

Backport of Core PR12650
bitcoin/bitcoin#12650

Test Plan:
  make check
  bitcoin-qt -> settings -> options -> network -> verify proxy port is 9050

Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3885
proteanx pushed a commit to devaultcrypto/devault that referenced this pull request Dec 12, 2019
@laanwj @proteanx
Merge #12650: gui: Fix issue: "default port not shown correctly in se…
47bc951 
…ttings dialog"

Summary:
40c5886 Fix illegal default `addProxy` and `addrSeparateProxyTor` settings. (251)

Pull request description:

  In f05d349 the value of the `addrProxy` and `addrSeparateProxyTor` settings is set to an illegal default value, because the value of `DEFAULT_GUI_PROXY_PORT ` is passed to the `fieldWidth` parameter of the `QString QString::arg(const QString &a, int fieldWidth = 0, QChar fillChar = QLatin1Char( ' ' )) const` method:

  https://github.com/bitcoin/bitcoin/blob/29fad97c320c892ab6a480c81e2078ec22ab354b/src/qt/optionsmodel.cpp#L129

  https://github.com/bitcoin/bitcoin/blob/29fad97c320c892ab6a480c81e2078ec22ab354b/src/qt/optionsmodel.cpp#L139

  This will create a default proxy setting that consists of 9053 characters and ends with the string `127.0.0.1:%2`.

  This PR attempts to resolve #12623 by setting the correct value for the `addrProxy` and `addrSeparateProxyTor` settings (i) if the proxy setting does not exist; or (ii) if the proxy setting has an illegal value caused by to the aforementioned bug.

  The second condition is *only* relevant if we don't want Bitcoin Core 0.16.0 users to explicitly reset their settings to see the correct default proxy port value.

Tree-SHA512: 3dc3de2eb7da831f6e318797df67341ced2076b48f9b561c73677bf6beb67b259d8e413095f290356fb92e32e4e8162d48accbc575c4e612060fd5d6dde7ac8d

Backport of Core PR12650
bitcoin/bitcoin#12650

Test Plan:
  make check
  bitcoin-qt -> settings -> options -> network -> verify proxy port is 9050

Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3885
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 3, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#12650: gui: Fix issue: "default port not shown correctl…
547aef1 
…y in settings dialog"

40c5886 Fix illegal default `addProxy` and `addrSeparateProxyTor` settings. (251)

Pull request description:

  In f05d349 the value of the `addrProxy` and `addrSeparateProxyTor` settings is set to an illegal default value, because the value of `DEFAULT_GUI_PROXY_PORT ` is passed to the `fieldWidth` parameter of the `QString QString::arg(const QString &a, int fieldWidth = 0, QChar fillChar = QLatin1Char( ' ' )) const` method:

  https://github.com/bitcoin/bitcoin/blob/29fad97c320c892ab6a480c81e2078ec22ab354b/src/qt/optionsmodel.cpp#L129

  https://github.com/bitcoin/bitcoin/blob/29fad97c320c892ab6a480c81e2078ec22ab354b/src/qt/optionsmodel.cpp#L139

  This will create a default proxy setting that consists of 9053 characters and ends with the string `127.0.0.1:%2`.

  This PR attempts to resolve bitcoin#12623 by setting the correct value for the `addrProxy` and `addrSeparateProxyTor` settings (i) if the proxy setting does not exist; or (ii) if the proxy setting has an illegal value caused by to the aforementioned bug.

  The second condition is *only* relevant if we don't want Bitcoin Core 0.16.0 users to explicitly reset their settings to see the correct default proxy port value.

Tree-SHA512: 3dc3de2eb7da831f6e318797df67341ced2076b48f9b561c73677bf6beb67b259d8e413095f290356fb92e32e4e8162d48accbc575c4e612060fd5d6dde7ac8d
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@maflcko maflcko maflcko left review comments

@Sjors Sjors Sjors approved these changes

@randolf randolf randolf approved these changes

Assignees
No one assigned
Labels
GUI
Projects
None yet
Milestone
0.16.1
Development

Successfully merging this pull request may close these issues.

gui: default port not shown correctly in settings dialog
8 participants
@l2a5b1 @laanwj @jonasschnelli @Sjors @maflcko @fanquake @randolf @meshcollider