If -prune=0 is set, Uncheck Prune on Intro page#615
If -prune=0 is set, Uncheck Prune on Intro page#615hebasto merged 1 commit intobitcoin-core:masterfrom jadijadi:jadi-prune-uncheck
Conversation
|
This is a probably a good-enough fix and better than the status quo, but it could be improved in the futue:
But this fix is probably good enough to fix problem at hand |
|
Thanks for the valuable comments. The first one is already working as intended. I took a note about the 2nd one and will work on it separately. |
hebasto
left a comment
hebasto
left a comment
There was a problem hiding this comment.
When passing -prune= >1 to the command line, it forces the prune checkbox to be checked and disables it.
With this PR, when passing -prune=0 to the command line, it forces the prune checkbox to be unchecked but does not disable it.
Should behavior be consistent?
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
This is a good point. It would be better for behavior to be consistent and it would be better to disable the setting if leaving it enabled gives user the misleading impression that setting they are chosing in the checkbox will be applied. For example, if bitcoin is started with -prune=0 and checkbox is enabled and user checks, they could have misleading impression that pruning will be applied when it won't be. (Or at least it won't be applied until next startup. It may be applied next startup if command line argument is omitted at that time) Even so, the PR in it's current form is still an improvement over the status quo. Disabling the checkbox would be an even bigger improvement, but just having checkbox show correct value is an improvement by itself. |
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
Code review ACK f4b0c0c
(Stylistically, I slightly prefer previous version of the PR 329c966 which does not use IsArgSet function, because I consider the IsArgSet method a major footgun that is used incorrectly 75% of the places where it is called. But in this case it is used correctly and both versions of the PR are equivalent)
hebasto
left a comment
hebasto
left a comment
There was a problem hiding this comment.
ACK f4b0c0c
Even so, the PR in it's current form is still an improvement over the status quo. Disabling the checkbox would be an even bigger improvement, but just having checkbox show correct value is an improvement by itself.
Agree. Could leave a bigger improvement for an follow up.
@jadijadi Please squash all commits into one before merging.
If the bitcoin-qt is started with -prune=0 arg, On the Intro page, the Prune Checkbox will be unchecked too, to prevent confusions. refs: bitcoin/bitcoin#25052 Co-authored-by: Hennadii Stepanov <[email protected]>
Thanks @hebasto , I squashed the commits into one. |
…o page 40566e2 If -prune=0 is set, Uncheck Prune on Intro page (Jadi) Pull request description: If the bitcoin-qt is started with -prune=0 arg, On the Intro page, the Prune Checkbox will be unchecked too, to prevent confusions. refs: bitcoin#25052 ACKs for top commit: hebasto: re-ACK 40566e2 Tree-SHA512: d5e0b76a7d20ae806e61a416fd907650f15a744a5823d0f8b57a634cb099bb135199e69a787bd54ecde2cf84e95633f40ff407a722350f337b27de395a6e0f78
7 participants
If the bitcoin-qt is started with -prune=0 arg, On the Intro page,
the Prune Checkbox will be unchecked too, to prevent confusions.
refs: bitcoin/bitcoin#25052