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.

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

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

Merged
merged 1 commit into from
Jun 20, 2022
Merged

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

merged 1 commit into from
Jun 20, 2022

Conversation

jadijadi
Copy link
Contributor

@jadijadi jadijadi commented Jun 8, 2022

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

@ryanofsky
Copy link
Contributor

ryanofsky commented Jun 8, 2022

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
Copy link
Contributor Author

jadijadi commented Jun 8, 2022

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.

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

tACK 329c966

Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

Code Review ACK 329c966

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

Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

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

Code Review ACK 329c966

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

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 hebasto added the UX All about "how to get things done" label Jun 12, 2022
@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 12, 2022

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

Conflicts

No conflicts as of last run.

@ryanofsky
Copy link
Contributor

ryanofsky commented Jun 15, 2022

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.

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

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)

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

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]>
@jadijadi
Copy link
Contributor Author

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.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

re-ACK 40566e2

@hebasto hebasto merged commit 09a76e4 into bitcoin-core:master Jun 20, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 21, 2022
…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
@bitcoin-core bitcoin-core locked and limited conversation to collaborators Jun 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
UX All about "how to get things done"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants