gui: Fix intro dialog labels when the prune button is toggled #17453

hebasto · 2019-11-12T15:25:56Z

On master (a6f6333) and on 0.19.0.1 the intro dialog with prune enabled (checkbox "Discard blocks..." is checked) provides a user with wrong info about the required disk space:

Also the paragraph "If you have chosen to limit..." is missed.

With this PR when prune checkbox is toggled, the related text labels and the amount of required space shown are updated (previously they were only updated when the data directory was updated):

This PR is an alternative to #17035.

ryanofsky's suggestion also has been implemented.

hebasto · 2019-11-12T15:27:14Z

ping @ryanofsky @Sjors @promag

src/qt/optionsmodel.h

src/qt/bitcoin.cpp

src/qt/intro.cpp

cvengler · 2019-11-12T20:28:57Z

Thanks, Concept ACK

hebasto · 2019-11-13T20:57:10Z

@ryanofsky

Thank you for your review.
All yours comments have been addressed: implemented, fixed or replied to.

Sjors · 2019-11-14T13:03:19Z

When you select a volume with low disk space (e.g. a RAM disk), when the user unchecks the prune box, it automatically checks itself again.

hebasto · 2019-11-14T13:54:20Z

@Sjors

When you select a volume with low disk space (e.g. a RAM disk), when the user unchecks the prune box, it automatically checks itself again.

It seems reasonable (the same behavior was in #17035). How should it be in your opinion?

Sjors · 2019-11-15T08:20:21Z

Either the user should be allowed to go ahead anyway (maybe they'll delete some movies during IBD), or the checkbox should be disabled.

hebasto · 2019-11-15T18:09:41Z

@Sjors

Thank you for your review.

Either the user should be allowed to go ahead anyway (maybe they'll delete some movies during IBD), or the checkbox should be disabled.

The former suggestion has been implemented in the latest push 9672ea8.

…in#17453)

ryanofsky · 2019-11-26T20:08:00Z

PR description doesn't describe PR. Can you update it to say what the problem with intro dialog labels is and how the problems is fixed? Also if behavior or code organization is changing in other ways because this seems to do more than just change labels...

ryanofsky

Partial code review ACK 9672ea8

I reviewed the settings changes I'm more familiar with. Some the GUI commits went over my head though. I think they'd make more sense to me if they described the effects they have.

f2afd07 util: Replace magics with DEFAULT_PRUNE_TARGET_GB (1/8)
f1ae041 gui: Fix prune in QSettings after intro (2/8)
162dafa util: Add PruneMiBtoGB() function (3/8)
0f8830b util: Add PruneGBtoMiB() function (4/8)
5d8dbfa refactor: Replace static variable with data member (5/8)
6dd682a refactor: Add Intro::UpdatePruneLabels() (6/8)
f07dd7c gui: Fix intro dialog when prune is toggled (7/8)
9672ea8 gui: Let user to uncheck prune on low-space dirs (8/8)

src/qt/bitcoin.cpp

src/qt/intro.cpp

ryanofsky · 2019-11-26T20:28:29Z

src/qt/intro.cpp

-    const int prune_target_gb = PruneMiBtoGB(prune_target_mib);
-    ui->prune->setText(tr("Discard blocks after verification, except most recent %1 GB (prune)").arg(prune_target_gb ? prune_target_gb : DEFAULT_PRUNE_TARGET_GB));
-    UpdatePruneLabels(prune_target_gb);
+    m_prune_target_gb = prune_target_mib ? PruneMiBtoGB(prune_target_mib) : DEFAULT_PRUNE_TARGET_GB;


In commit "gui: Fix intro dialog when prune is toggled" (f07dd7c)

I think this should say prune_target_mib > 1 ? ... instead of prune_target_mib ? ...

If the pruning value is 1, pruning is still effectively disabled and more than 1GB will be required unless manual pruning is done.

Could also combine this check into the if (prune_target_mib > 1) check above.

Fixed in the latest push.

ryanofsky · 2019-11-26T20:41:54Z

src/qt/intro.cpp

+    connect(ui->prune, &QCheckBox::toggled, [this](bool prune_checked) {
+        UpdatePruneLabels(prune_checked);
+        Q_EMIT requestCheck();
+    });


In commit "gui: Fix intro dialog when prune is toggled" (f07dd7c)

Why trigger a new disk space check here? Can you add explanatory comments? I think I understand updating the labels because the label text depends on the check state, but I don't get the disk space thing.

It would also be good to update commit description to say what behavior was before and after this change, and how it works.

Why trigger a new disk space check here?

Reworked. No unneeded disk space check anymore.

ryanofsky · 2019-11-26T20:46:57Z

src/qt/intro.cpp

@@ -255,11 +255,11 @@ void Intro::setStatus(int status, const QString &message, quint64 bytesAvailable
        if (bytesAvailable < m_required_space_gb * GB_BYTES) {
            freeString += " " + tr("(of %n GB needed)", "", m_required_space_gb);
            ui->freeSpace->setStyleSheet("QLabel { color: #800000 }");
-            ui->prune->setChecked(true);
+            if (!keep_prune) ui->prune->setChecked(true);


In commit "gui: Let user to uncheck prune on low-space dirs" (9672ea8)

Can you update the commit description to say behavior was before and after this change? It seems without this code the previous commit f07dd7c will be broken, because you added a new space check when the checkbox is toggled, so the box would automatically toggle itself back on after the check completed? All of this is very unclear to me.

It seems without this code the previous commit f07dd7c will be broken...

You are right. Fixed in the latest push.

DrahtBot · 2019-12-04T01:37:27Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17877 (qt, refactor: Make enums in BitcoinUnits class scoped by hebasto)
#17822 (refactor: Use uint16_t instead of unsigned short by ahook)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

hebasto · 2019-12-08T09:42:40Z

@ryanofsky

PR description doesn't describe PR. Can you update it to say what the problem with intro dialog labels is and how the problems is fixed?

The OP has been updated.

hebasto · 2019-12-08T10:51:27Z

@ryanofsky

In commit "gui: Fix prune in QSettings after intro" (f1ae041)

I can't figure out what this commit is doing. Can the commit description be updated to say what the behavior is before and after this commit?

The first two commits has been split of into #17696, as they are dedicated to an orthogonal bug.
The related conversations are marked as resolved here.

hebasto · 2019-12-08T22:02:20Z

@ryanofsky
Thank you for your review. All your comments have been addressed.

Would you mind re-reviewing this PR?

…ialog af112ab qt: Rename SetPrune() to InitializePruneSetting() (Hennadii Stepanov) b0bfbe5 refactor: Drop `bool force' parameter (Hennadii Stepanov) 68c9bbe qt: Force set nPruneSize in QSettings after intro (Hennadii Stepanov) a82bd8f util: Replace magics with DEFAULT_PRUNE_TARGET_GB (Hennadii Stepanov) Pull request description: On master (5622d8f), having `QSettings` set already ``` $ grep nPruneSize ~/.config/Bitcoin/Bitcoin-Qt-testnet.conf nPruneSize=6 ``` enabling prune option in the intro dialog ``` $ ./src/qt/bitcoin-qt -choosedatadir -testnet ``` ![DeepinScreenshot_select-area_20191208120425](https://user-images.githubusercontent.com/32963518/70388183-eed68580-19b6-11ea-9aa1-f9ad9aaa68a6.png) has no effect: ``` $ grep Prune ~/.bitcoin/testnet3/debug.log 2019-12-08T10:04:41Z Prune configured to target 5722 MiB on disk for block and undo files. ``` --- With this PR: ``` $ grep Prune ~/.bitcoin/testnet3/debug.log 2019-12-08T10:20:35Z Prune configured to target 1907 MiB on disk for block and undo files. ``` This PR has been split of #17453 (the first two commits) as it fixes an orthogonal bug. Refs: - #17453 (comment) - #17453 (comment) ACKs for top commit: Sjors: Code review re-ACK af112ab ryanofsky: Code review ACK af112ab. Just suggested changes since last review (thanks!) promag: Tested ACK af112ab. Latest suggestions and changes look good to me. Tree-SHA512: 8ddad34b30dcc2cdcad6678ba8a0b36fa176e4e3465862ef6eee9be0f98d8146705138c9c7995dd8c0990af41078ca743fef1a90ed9240081f052f32ddec72b9

Now the text of prune QCheckBox shows the space in GB instead of thousands MiB, which is consistent with other parts of the GUI.

This commit does not change behavior.

hebasto · 2020-01-08T15:23:27Z

Rebased after #17696 has been merged.

ryanofsky

Code review ACK 3f28e10. This looks good. Description of the PR is very clear, and the commits are well organized and mostly a breeze to review. There's a lot going on in the final commit, and I left some comments there to help clarify it, but feel free to ignore my suggestions if they don't make sense

ryanofsky · 2020-01-08T16:27:40Z

src/qt/intro.cpp

@@ -138,21 +135,21 @@ Intro::Intro(QWidget *parent, uint64_t blockchain_size, uint64_t chain_state_siz
    }
    const int prune_target_gb = PruneMiBtoGB(prune_target_mib);
    ui->prune->setText(tr("Discard blocks after verification, except most recent %1 GB (prune)").arg(prune_target_gb ? prune_target_gb : DEFAULT_PRUNE_TARGET_GB));
-    requiredSpace = m_blockchain_size;
+    m_required_space_gb = m_blockchain_size;


In commit "refactor: Replace static variable with data member" (a745453)

It might be clearer to initialize this directly next to m_blockchain_size and m_chain_state_size above, instead of initializing this to 0 before setting it to m_blockchain_size.

The code is reorganized to make assigning values to m_required_space_gb variable more clear.

ryanofsky · 2020-01-08T16:52:43Z

src/qt/intro.cpp

-        } else if (bytesAvailable / GB_BYTES - m_required_space_gb < 10) {
-            freeString += " " + tr("(%n GB needed for full chain)", "", m_required_space_gb);
-            ui->freeSpace->setStyleSheet("QLabel { color: #999900 }");
-            ui->prune->setChecked(true);


In commit "gui: Add Intro::UpdateFreeSpaceLabel()" (2ee1e1c)

Change in behavior: if new datadir has enough size, the prune checkbox unchecks.

Just for clarification, it seems like on initial setup both before and after this PR, the checkbox would automatically be checked if the default datadir drive couldn't hold m_required_space_gb + 10, and unchecked otherwise.

The difference with this PR is just that the checkbox gets unchecked when a new datadir is selected, where previously it would never get unchecked?

The difference with this PR is just that the checkbox gets unchecked when a new datadir is selected, where previously it would never get unchecked?

Steps to reproduce:

the default datadir not having enough free space causes the prune checkbox gets checked (both on master and with this PR)

a user chooses a custom datadir with enough free space:

on master the prune checkbox remains checked

with this PR the prune checkbox gets unchecked

In commit "gui: Add Intro::UpdateFreeSpaceLabel()" (a208b68)

The difference with this PR is just that the checkbox gets unchecked when a new datadir is selected, where previously it would never get unchecked?

Steps to reproduce: [...]

Thanks for the description. It sounds like the answer is yes.

src/qt/intro.cpp

ryanofsky · 2020-01-08T19:00:37Z

src/qt/intro.cpp

-    const int prune_target_gb = PruneMiBtoGB(prune_target_mib);
-    ui->prune->setText(tr("Discard blocks after verification, except most recent %1 GB (prune)").arg(prune_target_gb ? prune_target_gb : DEFAULT_PRUNE_TARGET_GB));
-    UpdatePruneLabels(prune_target_gb);
+    ui->prune->setText(tr("Discard blocks after verification, except most recent %1 GB (prune)").arg(m_prune_target_gb));


In commit "gui: Make intro consistent with prune checkbox" (3f28e10)

Note: I think there's no change in behavior on this line except in the -prune=1 case. Now the prune size text will show "2 GB" instead of "1 GB" there. (Before the earlier commit "util: Add PruneMiBtoGB() function" it showed "0 GB").

"2 GB" seems like the correct value to show in this case, since after #17696 2GB is the size that will actually be used if automatic pruning is enabled later.

ryanofsky · 2020-01-08T19:17:04Z

src/qt/intro.cpp

+    connect(ui->prune, &QCheckBox::toggled, [this](bool prune_checked) {
+        UpdatePruneLabels(prune_checked);
+        UpdateFreeSpaceLabel();
+    });


In commit "gui: Make intro consistent with prune checkbox" (3f28e10)

Current commit description is vague. It would be good to say what behavior is changing. As far as I can tell, there are two fixes in this commit and the other changes are refactoring:

Main fix: when prune checkbox in the intro dialog is toggled, the amount of required space shown is updated. (Previously it was only updated when the data directory was updated.)

Corner case fix: prune size shown in the case where pruning is disabled is now consistently shown as 2GB, instead of 1GB in the corner case where -prune=1 is set from an external config

Current commit description is vague.

Fixed.

src/qt/intro.h

…intro dialog af112ab qt: Rename SetPrune() to InitializePruneSetting() (Hennadii Stepanov) b0bfbe5 refactor: Drop `bool force' parameter (Hennadii Stepanov) 68c9bbe qt: Force set nPruneSize in QSettings after intro (Hennadii Stepanov) a82bd8f util: Replace magics with DEFAULT_PRUNE_TARGET_GB (Hennadii Stepanov) Pull request description: On master (5622d8f), having `QSettings` set already ``` $ grep nPruneSize ~/.config/Bitcoin/Bitcoin-Qt-testnet.conf nPruneSize=6 ``` enabling prune option in the intro dialog ``` $ ./src/qt/bitcoin-qt -choosedatadir -testnet ``` ![DeepinScreenshot_select-area_20191208120425](https://user-images.githubusercontent.com/32963518/70388183-eed68580-19b6-11ea-9aa1-f9ad9aaa68a6.png) has no effect: ``` $ grep Prune ~/.bitcoin/testnet3/debug.log 2019-12-08T10:04:41Z Prune configured to target 5722 MiB on disk for block and undo files. ``` --- With this PR: ``` $ grep Prune ~/.bitcoin/testnet3/debug.log 2019-12-08T10:20:35Z Prune configured to target 1907 MiB on disk for block and undo files. ``` This PR has been split of bitcoin#17453 (the first two commits) as it fixes an orthogonal bug. Refs: - bitcoin#17453 (comment) - bitcoin#17453 (comment) ACKs for top commit: Sjors: Code review re-ACK af112ab ryanofsky: Code review ACK af112ab. Just suggested changes since last review (thanks!) promag: Tested ACK af112ab. Latest suggestions and changes look good to me. Tree-SHA512: 8ddad34b30dcc2cdcad6678ba8a0b36fa176e4e3465862ef6eee9be0f98d8146705138c9c7995dd8c0990af41078ca743fef1a90ed9240081f052f32ddec72b9

This is a move-only commit and it does not change behavior.

If a new custom datadir has enough free space, the prune checkbox gets unchecked, unless -prune=NNN command-line option is provided.

When prune checkbox is toggled, the related text labels and the amount of required space shown are updated (previously they were only updated when the data directory was updated).

hebasto · 2020-01-14T17:18:21Z

Updated 91cc02d -> 4f7127d (pr17453.11 -> pr17453.12, compare).

@ryanofsky
Thank you for your review. The bug you mentioned has been fixed.

ryanofsky

Code review ACK 4f7127d. It seems like there are a few visible changes here:

Main change is amount of required disk space display now being updated when PR is checked and unchecked. This change is mentioned in the PR description and implemented in commit "gui: Make Intro consistent with prune checkbox"
Another change is required space for pruning now being converted correctly to GB instead of GiB in commit "util: Add PruneMiBtoGB() function"
Another change is that the pruning checkbox will automatically get unchecked if a newly selected datadir has enough free space to hold the chainstate and blocks in commit "gui: Add Intro::UpdateFreeSpaceLabel()"
Another change is that "Discard blocks after verification, except most recent %1 GB" label now shows 2GB instead of 0GB if bitcoin is started with -prune=1 and does the MiB to GB conversion more correctly in other cases where -prune= is set, using the right conversion factor and rounding up instead of down

src/qt/intro.cpp

cvengler · 2020-01-20T14:17:56Z

I tested it and it works and the code also looks OK to me.
Please squash and I will ACK.
This should get into 0.19.1 btw. Maybe it's worth talking about it on the next meeting

hebasto · 2020-01-20T14:55:28Z

@emilengler

Please squash...

It seems it could make reviewing a bit harder for other reviewers, no?

cvengler · 2020-01-20T15:16:36Z

It seems it could make reviewing a bit harder for other reviewers, no?

If the commit message matters, put them into the longer description. But I don't think it is worth looking at the diffs from commit to commit. It is probably the easiest to just look at the diff from HEAD to master

sipa · 2020-01-20T15:50:41Z

@emilengler Even if there are multiple commits, you're free as a reviewer to look at the entire diff at once. The decision whether to break things up is mostly up to the author, with the caveat that each commit must make sense on its own, and ideally, the code compiles and passes tests for every individual step. I don't see a reason here to request squashing - that's usually only done when a PR contains "fixups" as a result of review that really should be integrated into the body.

cvengler · 2020-01-20T19:23:23Z

ACK 4f7127d

Sjors · 2020-01-22T17:18:38Z

tACK 4f7127d

jonasschnelli

utACK 4f7127d

…oggled 4f7127d gui: Make Intro consistent with prune checkbox (Hennadii Stepanov) 4824a7d gui: Add Intro::UpdateFreeSpaceLabel() (Hennadii Stepanov) daa3f3f refactor: Add Intro::UpdatePruneLabels() (Hennadii Stepanov) e4caa82 refactor: Replace static variable with data member (Hennadii Stepanov) 2bede28 util: Add PruneGBtoMiB() function (Hennadii Stepanov) e35e4b2 util: Add PruneMiBtoGB() function (Hennadii Stepanov) Pull request description: On master (a6f6333) and on 0.19.0.1 the intro dialog with prune enabled (checkbox "Discard blocks..." is checked) provides a user with wrong info about the required disk space: ![DeepinScreenshot_bitcoin-qt_20191208112228](https://user-images.githubusercontent.com/32963518/70387510-8daab400-19ae-11ea-9338-29add9c31118.png) Also the paragraph "If you have chosen to limit..." is missed. --- With this PR when prune checkbox is toggled, the related text labels and the amount of required space shown are updated (previously they were only updated when the data directory was updated): ![Screenshot from 2019-12-08 11-34-53](https://user-images.githubusercontent.com/32963518/70387542-eed28780-19ae-11ea-9565-49d8a64b2f33.png) --- This PR is an alternative to #17035. **ryanofsky**'s [suggestion](#17035 (comment)) also has been implemented. ACKs for top commit: emilengler: ACK 4f7127d Sjors: tACK 4f7127d ryanofsky: Code review ACK 4f7127d. It seems like there are a few visible changes here: jonasschnelli: utACK 4f7127d Tree-SHA512: fa0bbdcfafde97d7906cda066cbd4608b936a71cae1b4cda3ee3aa2eed3a9795f279f14c6b1b4997278e094db891c7d3bb695368ba0882347aa42165a86e5172

…on is toggled 4f7127d gui: Make Intro consistent with prune checkbox (Hennadii Stepanov) 4824a7d gui: Add Intro::UpdateFreeSpaceLabel() (Hennadii Stepanov) daa3f3f refactor: Add Intro::UpdatePruneLabels() (Hennadii Stepanov) e4caa82 refactor: Replace static variable with data member (Hennadii Stepanov) 2bede28 util: Add PruneGBtoMiB() function (Hennadii Stepanov) e35e4b2 util: Add PruneMiBtoGB() function (Hennadii Stepanov) Pull request description: On master (a6f6333) and on 0.19.0.1 the intro dialog with prune enabled (checkbox "Discard blocks..." is checked) provides a user with wrong info about the required disk space: ![DeepinScreenshot_bitcoin-qt_20191208112228](https://user-images.githubusercontent.com/32963518/70387510-8daab400-19ae-11ea-9338-29add9c31118.png) Also the paragraph "If you have chosen to limit..." is missed. --- With this PR when prune checkbox is toggled, the related text labels and the amount of required space shown are updated (previously they were only updated when the data directory was updated): ![Screenshot from 2019-12-08 11-34-53](https://user-images.githubusercontent.com/32963518/70387542-eed28780-19ae-11ea-9565-49d8a64b2f33.png) --- This PR is an alternative to bitcoin#17035. **ryanofsky**'s [suggestion](bitcoin#17035 (comment)) also has been implemented. ACKs for top commit: emilengler: ACK 4f7127d Sjors: tACK 4f7127d ryanofsky: Code review ACK 4f7127d. It seems like there are a few visible changes here: jonasschnelli: utACK 4f7127d Tree-SHA512: fa0bbdcfafde97d7906cda066cbd4608b936a71cae1b4cda3ee3aa2eed3a9795f279f14c6b1b4997278e094db891c7d3bb695368ba0882347aa42165a86e5172

Fix intro dialog labels when the prune button is toggled (bitcoin#17453) Rename debug window (bitcoin#17096) Add close window shortcut (bitcoin#15768) Add privacy to the Overview page (bitcoin#16432) Remove WalletView and BitcoinGUI circular dependency (bitcoin#17937) Force set nPruneSize in QSettings after the intro dialog (bitcoin#17696)

…intro dialog af112ab qt: Rename SetPrune() to InitializePruneSetting() (Hennadii Stepanov) b0bfbe5 refactor: Drop `bool force' parameter (Hennadii Stepanov) 68c9bbe qt: Force set nPruneSize in QSettings after intro (Hennadii Stepanov) a82bd8f util: Replace magics with DEFAULT_PRUNE_TARGET_GB (Hennadii Stepanov) Pull request description: On master (5622d8f), having `QSettings` set already ``` $ grep nPruneSize ~/.config/Bitcoin/Bitcoin-Qt-testnet.conf nPruneSize=6 ``` enabling prune option in the intro dialog ``` $ ./src/qt/bitcoin-qt -choosedatadir -testnet ``` ![DeepinScreenshot_select-area_20191208120425](https://user-images.githubusercontent.com/32963518/70388183-eed68580-19b6-11ea-9aa1-f9ad9aaa68a6.png) has no effect: ``` $ grep Prune ~/.bitcoin/testnet3/debug.log 2019-12-08T10:04:41Z Prune configured to target 5722 MiB on disk for block and undo files. ``` --- With this PR: ``` $ grep Prune ~/.bitcoin/testnet3/debug.log 2019-12-08T10:20:35Z Prune configured to target 1907 MiB on disk for block and undo files. ``` This PR has been split of bitcoin#17453 (the first two commits) as it fixes an orthogonal bug. Refs: - bitcoin#17453 (comment) - bitcoin#17453 (comment) ACKs for top commit: Sjors: Code review re-ACK af112ab ryanofsky: Code review ACK af112ab. Just suggested changes since last review (thanks!) promag: Tested ACK af112ab. Latest suggestions and changes look good to me. Tree-SHA512: 8ddad34b30dcc2cdcad6678ba8a0b36fa176e4e3465862ef6eee9be0f98d8146705138c9c7995dd8c0990af41078ca743fef1a90ed9240081f052f32ddec72b9

…on is toggled 4f7127d gui: Make Intro consistent with prune checkbox (Hennadii Stepanov) 4824a7d gui: Add Intro::UpdateFreeSpaceLabel() (Hennadii Stepanov) daa3f3f refactor: Add Intro::UpdatePruneLabels() (Hennadii Stepanov) e4caa82 refactor: Replace static variable with data member (Hennadii Stepanov) 2bede28 util: Add PruneGBtoMiB() function (Hennadii Stepanov) e35e4b2 util: Add PruneMiBtoGB() function (Hennadii Stepanov) Pull request description: On master (a6f6333) and on 0.19.0.1 the intro dialog with prune enabled (checkbox "Discard blocks..." is checked) provides a user with wrong info about the required disk space: ![DeepinScreenshot_bitcoin-qt_20191208112228](https://user-images.githubusercontent.com/32963518/70387510-8daab400-19ae-11ea-9338-29add9c31118.png) Also the paragraph "If you have chosen to limit..." is missed. --- With this PR when prune checkbox is toggled, the related text labels and the amount of required space shown are updated (previously they were only updated when the data directory was updated): ![Screenshot from 2019-12-08 11-34-53](https://user-images.githubusercontent.com/32963518/70387542-eed28780-19ae-11ea-9565-49d8a64b2f33.png) --- This PR is an alternative to bitcoin#17035. **ryanofsky**'s [suggestion](bitcoin#17035 (comment)) also has been implemented. ACKs for top commit: emilengler: ACK 4f7127d Sjors: tACK 4f7127d ryanofsky: Code review ACK 4f7127d. It seems like there are a few visible changes here: jonasschnelli: utACK 4f7127d Tree-SHA512: fa0bbdcfafde97d7906cda066cbd4608b936a71cae1b4cda3ee3aa2eed3a9795f279f14c6b1b4997278e094db891c7d3bb695368ba0882347aa42165a86e5172

…o dialog Summary: af112ab62895b145660f4cd7ff842e9cfea2a530 qt: Rename SetPrune() to InitializePruneSetting() (Hennadii Stepanov) b0bfbe50282877a1eee87118902901a280d6656d refactor: Drop `bool force' parameter (Hennadii Stepanov) 68c9bbe9bc91f882404556998666b1b5acea60e4 qt: Force set nPruneSize in QSettings after intro (Hennadii Stepanov) a82bd8fa5708c16d1db3edc4e82d70788eb5af19 util: Replace magics with DEFAULT_PRUNE_TARGET_GB (Hennadii Stepanov) Pull request description: On master (5622d8f3156a293e61d0964c33d4b21d8c9fd5e0), having `QSettings` set already ``` $ grep nPruneSize ~/.config/Bitcoin/Bitcoin-Qt-testnet.conf nPruneSize=6 ``` enabling prune option in the intro dialog ``` $ ./src/qt/bitcoin-qt -choosedatadir -testnet ``` ![DeepinScreenshot_select-area_20191208120425](https://user-images.githubusercontent.com/32963518/70388183-eed68580-19b6-11ea-9aa1-f9ad9aaa68a6.png) has no effect: ``` $ grep Prune ~/.bitcoin/testnet3/debug.log 2019-12-08T10:04:41Z Prune configured to target 5722 MiB on disk for block and undo files. ``` --- With this PR: ``` $ grep Prune ~/.bitcoin/testnet3/debug.log 2019-12-08T10:20:35Z Prune configured to target 1907 MiB on disk for block and undo files. ``` This PR has been split of #17453 (the first two commits) as it fixes an orthogonal bug. Refs: - bitcoin/bitcoin#17453 (comment) - bitcoin/bitcoin#17453 (comment) --- Backport of Core [[bitcoin/bitcoin#17696 | PR17696]] Test Plan: ninja all check check-functional ./src/qt/bitcoin-qt -choosedatadir -testnet start a new datadir with prune option selected, make sure the debug.log returns the correct value (~2GB) Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D8913

…s toggled Summary: 4f7127d1e3a51f0f55d42a08439c516dcc8d1a26 gui: Make Intro consistent with prune checkbox (Hennadii Stepanov) 4824a7d36cf47e766865e0fefe952ec860eb82dd gui: Add Intro::UpdateFreeSpaceLabel() (Hennadii Stepanov) daa3f3fa9071a229275dd6a1b8445237ddc3fa97 refactor: Add Intro::UpdatePruneLabels() (Hennadii Stepanov) e4caa82a03df5c6a6d5d29f34ab006d732c6dac1 refactor: Replace static variable with data member (Hennadii Stepanov) 2bede28cd9ec638d8bb32c187ccf12d89345218e util: Add PruneGBtoMiB() function (Hennadii Stepanov) e35e4b2ba052c9a533626286026dbe0a2d546c5b util: Add PruneMiBtoGB() function (Hennadii Stepanov) Pull request description: On master (a6f6333ba253cda83221ee529810cacf930e413f) and on 0.19.0.1 the intro dialog with prune enabled (checkbox "Discard blocks..." is checked) provides a user with wrong info about the required disk space: ![DeepinScreenshot_bitcoin-qt_20191208112228](https://user-images.githubusercontent.com/32963518/70387510-8daab400-19ae-11ea-9338-29add9c31118.png) Also the paragraph "If you have chosen to limit..." is missed. --- With this PR when prune checkbox is toggled, the related text labels and the amount of required space shown are updated (previously they were only updated when the data directory was updated): ![Screenshot from 2019-12-08 11-34-53](https://user-images.githubusercontent.com/32963518/70387542-eed28780-19ae-11ea-9565-49d8a64b2f33.png) --- This PR is an alternative to #17035. **ryanofsky**'s [suggestion](bitcoin/bitcoin#17035 (comment)) also has been implemented. --- Backport of Core [[bitcoin/bitcoin#17453 | PR17453]] Depends on D8913 Test Plan: ninja all check check-functional ./src/qt/bitcoin-qt -choosedatadir check that correct information is provided, whether prune checkbox is ticked or not Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Subscribers: PiRK Differential Revision: https://reviews.bitcoinabc.org/D8914

fanquake added the GUI label Nov 12, 2019

ryanofsky reviewed Nov 12, 2019

View reviewed changes

hebasto force-pushed the 20191112-fix-intro-prune branch from 4824eeb to f07dd7c Compare November 13, 2019 20:46

MarkLTZ added a commit to litecoinz-core/litecoinz that referenced this pull request Nov 17, 2019

[gui] Fix intro dialog labels when the prune button is toggled (bitco…

ac8cef4

…in#17453)

ryanofsky approved these changes Nov 26, 2019

View reviewed changes

hebasto mentioned this pull request Dec 8, 2019

qt: Force set nPruneSize in QSettings after the intro dialog #17696

Merged

hebasto force-pushed the 20191112-fix-intro-prune branch from 9672ea8 to 00fd42f Compare December 8, 2019 21:43

DrahtBot added the Needs rebase label Jan 8, 2020

hebasto added 2 commits January 8, 2020 17:21

util: Add PruneMiBtoGB() function

e35e4b2

Now the text of prune QCheckBox shows the space in GB instead of thousands MiB, which is consistent with other parts of the GUI.

util: Add PruneGBtoMiB() function

2bede28

This commit does not change behavior.

hebasto force-pushed the 20191112-fix-intro-prune branch from 00fd42f to 3f28e10 Compare January 8, 2020 15:22

DrahtBot removed the Needs rebase label Jan 8, 2020

ryanofsky approved these changes Jan 8, 2020

View reviewed changes

hebasto force-pushed the 20191112-fix-intro-prune branch 2 times, most recently from ed96c2b to 91cc02d Compare January 10, 2020 23:33

hebasto added 3 commits January 14, 2020 18:47

refactor: Replace static variable with data member

e4caa82

refactor: Add Intro::UpdatePruneLabels()

daa3f3f

This is a move-only commit and it does not change behavior.

gui: Add Intro::UpdateFreeSpaceLabel()

4824a7d

If a new custom datadir has enough free space, the prune checkbox gets unchecked, unless -prune=NNN command-line option is provided.

hebasto force-pushed the 20191112-fix-intro-prune branch from 91cc02d to 95761b4 Compare January 14, 2020 16:58

gui: Make Intro consistent with prune checkbox

4f7127d

When prune checkbox is toggled, the related text labels and the amount of required space shown are updated (previously they were only updated when the data directory was updated).

hebasto force-pushed the 20191112-fix-intro-prune branch from 95761b4 to 4f7127d Compare January 14, 2020 17:14

ryanofsky approved these changes Jan 14, 2020

View reviewed changes

cvengler reviewed Jan 18, 2020

View reviewed changes

src/qt/intro.cpp Show resolved Hide resolved

hebasto requested a review from jonasschnelli January 23, 2020 19:08

jonasschnelli approved these changes Jan 27, 2020

View reviewed changes

jonasschnelli merged commit 4f7127d into bitcoin:master Jan 27, 2020

hebasto deleted the 20191112-fix-intro-prune branch January 27, 2020 17:43

hebasto mentioned this pull request Jan 27, 2020

qt, refactor: Make BitcoinUnits::Unit a scoped enum #17877

Closed

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

ryanofsky mentioned this pull request Jun 5, 2020

interfaces: Expose settings.json methods to GUI #15936

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

gui: Fix intro dialog labels when the prune button is toggled #17453

gui: Fix intro dialog labels when the prune button is toggled #17453

Uh oh!

Conversation

hebasto commented Nov 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Nov 12, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cvengler commented Nov 12, 2019

Uh oh!

hebasto commented Nov 13, 2019

Uh oh!

Sjors commented Nov 14, 2019

Uh oh!

hebasto commented Nov 14, 2019

Uh oh!

Sjors commented Nov 15, 2019

Uh oh!

hebasto commented Nov 15, 2019

Uh oh!

ryanofsky commented Nov 26, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Dec 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

hebasto commented Dec 8, 2019

Uh oh!

hebasto commented Dec 8, 2019

Uh oh!

hebasto commented Dec 8, 2019

Uh oh!

hebasto commented Jan 8, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto commented Jan 14, 2020

hebasto commented Nov 12, 2019 •

edited

Loading

DrahtBot commented Dec 4, 2019 •

edited

Loading

cvengler commented Jan 20, 2020 •

edited

Loading