If -prune=0 is set, Uncheck Prune on Intro page

jadijadi commented at 12:42 pm on June 8, 2022: contributor

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: https://github.com/bitcoin/bitcoin/issues/25052

ryanofsky commented at 1:52 pm on June 8, 2022: contributor

This is a probably a good-enough fix and better than the status quo, but it could be improved in the futue:

~If -prune=size setting is given, size could be shown in intro dialog box~ (EDIT: Not true, this is actually already implemented)
If data directory path is changed, and path contains bitcoin.conf or settings.json files with a prune value, that value could be shown.

But this fix is probably good enough to fix problem at hand

jadijadi commented at 2:02 pm on June 8, 2022: contributor

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. Thanks.

jarolrod commented at 6:07 pm on June 10, 2022: member

tACK 329c96626b17e1b4dd093de19c9695f5eddd4699

shaavan approved

shaavan commented at 8:02 am on June 11, 2022: contributor

Code Review ACK 329c96626b17e1b4dd093de19c9695f5eddd4699

I find this to be a concise and straightforward fix to the issue it targets.

w0xlt approved

w0xlt commented at 8:50 am on June 11, 2022: contributor

Code Review ACK https://github.com/bitcoin-core/gui/pull/615/commits/329c96626b17e1b4dd093de19c9695f5eddd4699

in src/qt/intro.cpp:290 in 329c96626b outdated

286@@ -287,7 +287,7 @@ void Intro::setStatus(int status, const QString &message, quint64 bytesAvailable
287         ui->freeSpace->setText("");
288     } else {
289         m_bytes_available = bytesAvailable;
290-        if (ui->prune->isEnabled()) {
291+        if (ui->prune->isEnabled() && gArgs.GetIntArg("-prune", -1) != 0) { // prune checkbox is enabled and not manually set to 0

hebasto commented at 10:44 am on June 12, 2022:

Could comment be dropped as it says exactly what code does?

Also I doubt a bit about using -1 as the default value, because GetIntArg("-prune", 0) is used across the whole code base. It could be worked out as

0        if (ui->prune->isEnabled() && !(gArgs.IsArgSet("-prune") && gArgs.GetIntArg("-prune", 0) == 0)) {

jadijadi commented at 10:59 am on June 12, 2022:

Thanks for the improvements. You are right.

hebasto commented at 10:50 am on June 12, 2022: member

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?

hebasto added the label UX on Jun 12, 2022

DrahtBot commented at 9:43 pm on June 12, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

ryanofsky commented at 3:27 pm on June 15, 2022: contributor

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?

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 approved

ryanofsky commented at 3:31 pm on June 15, 2022: contributor

Code review ACK f4b0c0c5c7149ffc6392d5becba0ec885fe6296b

(Stylistically, I slightly prefer previous version of the PR 329c96626b17e1b4dd093de19c9695f5eddd4699 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 approved

hebasto commented at 3:52 pm on June 15, 2022: member

ACK f4b0c0c5c7149ffc6392d5becba0ec885fe6296b

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 -prune=0 is set, Uncheck Prune on Intro page

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: https://github.com/bitcoin/bitcoin/issues/25052

Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>

40566e21c0

jadijadi commented at 9:30 am on June 16, 2022: contributor

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.

Thanks @hebasto , I squashed the commits into one.

hebasto approved

hebasto commented at 9:59 am on June 16, 2022: member

re-ACK 40566e21c02ddec6458cd83fa76fea081c5469dc

hebasto merged this on Jun 20, 2022

hebasto closed this on Jun 20, 2022

sidhujag referenced this in commit 5963576ed5 on Jun 21, 2022

bitcoin-core locked this on Jun 20, 2023

If -prune=0 is set, Uncheck Prune on Intro page #615

Conflicts