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

pull l2a5b1 wants to merge 1 commits into bitcoin:master from l2a5b1:patch/12623/default-port-not-shown changing 1 files +21 −2

l2a5b1 commented at 11:47 PM on March 8, 2018: contributor

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.
meshcollider added the label GUI on Mar 9, 2018
laanwj commented at 12:48 PM on March 10, 2018: member

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 force-pushed on Mar 10, 2018
l2a5b1 commented at 5:18 PM on March 10, 2018: contributor

Thanks @laanwj! I have added a comment to both conditional statements to clarify the purpose of the second boolean condition.
randolf approved
MarcoFalke added this to the milestone 0.16.1 on Mar 11, 2018
bitcoin deleted a comment on Mar 15, 2018
MarcoFalke cross-referenced this on Mar 17, 2018 from issue gui: Fix proxy setting options dialog crash by laanwj

in src/qt/optionsmodel.cpp:129 in 9ff17ee9ef outdated

 124 | @@ -125,8 +125,10 @@ void OptionsModel::Init(bool resetSettings)
 125 |  
 126 |      if (!settings.contains("fUseProxy"))
 127 |          settings.setValue("fUseProxy", false);
 128 | -    if (!settings.contains("addrProxy"))
 129 | -        settings.setValue("addrProxy", QString("%1:%2").arg(DEFAULT_GUI_PROXY_HOST, DEFAULT_GUI_PROXY_PORT));
 130 | +    // The second condition in the boolean expression is a workaround to overwrite the 'addrProxy' setting
 131 | +    // in case it has been set to an illegal default value by release 0.16.0 (see issue #12623; PR #12650).

Sjors commented at 4:52 PM on April 3, 2018:

Move to OptionsModel::checkAndMigrate()?

Sjors commented at 4:53 PM on April 3, 2018: member

Concept ACK
l2a5b1 force-pushed on Apr 6, 2018
l2a5b1 commented at 10:45 PM on April 6, 2018: contributor

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 force-pushed on Apr 7, 2018

in src/qt/optionsmodel.cpp:497 in 0592609eb6 outdated

 492 | @@ -485,4 +493,16 @@ void OptionsModel::checkAndMigrate()
 493 |  
 494 |          settings.setValue(strSettingsVersionKey, CLIENT_VERSION);
 495 |      }
 496 | +
 497 | +    // Overwrite the 'addrProxy' setting in case it has been set to an illegal

MarcoFalke commented at 2:43 PM on April 7, 2018:

Trailing whitespace in this line

l2a5b1 force-pushed on Apr 7, 2018
l2a5b1 force-pushed on Apr 7, 2018
l2a5b1 commented at 9:07 PM on April 7, 2018: contributor

Thanks @MarcoFalke! 1cd545e is rebased onto the master (048ac8326) and addresses the trailing whitespace character.
l2a5b1 force-pushed on Apr 7, 2018
l2a5b1 force-pushed on Apr 7, 2018
l2a5b1 commented at 10:13 PM on April 7, 2018: contributor

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

Move to OptionsModel::checkAndMigrate()?
Sjors approved
Sjors commented at 9:51 AM on April 9, 2018: member

tACK 2f3ce02

Nit: maybe change "Addresses" to "Fix" in your commit message, if only so it fits in 72 characters.
jonasschnelli commented at 6:30 PM on April 10, 2018: contributor

utACK 2f3ce024eb36f93f82f4fc644d57c45639b9f3de

Nods for new migration/auto-fix code without EOL strategy.
Fix illegal default `addProxy` and `addrSeparateProxyTor` settings. 40c58866c7
l2a5b1 force-pushed on Apr 10, 2018
l2a5b1 commented at 8:31 PM on April 10, 2018: contributor

Thanks @jonasschnelli, @sjors! @sjors: I have addressed your feedback and changed the commit message in 40c5886 to make it fit in 72 characters.
Sjors commented at 10:37 AM on April 11, 2018: member

reACK 40c58866c7e7af3582cefb9b1810a20902e44167
laanwj merged this on Apr 11, 2018
laanwj closed this on Apr 11, 2018
laanwj referenced this in commit f15b72f482 on Apr 11, 2018
bitcoin deleted a comment on Apr 11, 2018
MarcoFalke added the label Needs backport on Apr 11, 2018
MarcoFalke commented at 11:30 PM on April 11, 2018: member

Tagged for backport in case there is a 16.1 release
fanquake referenced this in commit 882f520642 on Apr 12, 2018
fanquake cross-referenced this on Apr 12, 2018 from issue [0.16] Backports by fanquake
fanquake referenced this in commit 298b92596e on Apr 16, 2018
MarcoFalke referenced this in commit 4b9eb00936 on Apr 20, 2018
fanquake referenced this in commit f118a7a35b on Apr 26, 2018
laanwj referenced this in commit feba12fe85 on May 16, 2018
fanquake removed the label Needs backport on May 16, 2018
fanquake commented at 2:28 PM on May 16, 2018: member

Backported in #12967
HashUnlimited referenced this in commit afcfda04cd on Jun 29, 2018
ccebrecos referenced this in commit 74f25659ba on Sep 14, 2018
Fabcien referenced this in commit 97ff16fa0f on Aug 30, 2019
jonspock referenced this in commit 5932fd780f on Dec 8, 2019
jonspock referenced this in commit 102093c190 on Dec 8, 2019
proteanx referenced this in commit 47bc951e08 on Dec 12, 2019
PastaPastaPasta referenced this in commit 547aef1b0b on Apr 3, 2020
ckti referenced this in commit 82290b48bf on Mar 28, 2021
bitcoin locked this on Sep 8, 2021

Contributors

Labels

Milestone
0.16.1

Linked (view graph)

#11809 gui: Fix proxy setting options dialog crash #12623 gui: default port not shown correctly in settings dialog #12967 [0.16] Backports