[qt] Intro: Display required space #7298

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:Mf1601-qtDataDir changing 1 files +15 −5

MarcoFalke commented at 6:14 PM on January 5, 2016: member

Required space depends on the user's choice: -prune=0 -prune=<n>

[qt] Intro: Display required space

Required space depends on the user's choice:
 -prune=0
 -prune=<n>

faf3299b73

MarcoFalke force-pushed on Jan 5, 2016
jonasschnelli added the label GUI on Jan 5, 2016
in src/qt/intro.cpp:None in faf3299b73
```
 117 | @@ -112,7 +118,11 @@ Intro::Intro(QWidget *parent) :
 118 |      signalled(false)
 119 |  {
 120 |      ui->setupUi(this);
 121 | -    ui->sizeWarningLabel->setText(ui->sizeWarningLabel->text().arg(BLOCK_CHAIN_SIZE/GB_BYTES));
 122 | +    uint64_t pruneTarget = std::max<int64_t>(0, GetArg("-prune", 0));
 123 | +    requiredSpace = BLOCK_CHAIN_SIZE;
 124 | +    if (pruneTarget)
 125 | +        requiredSpace = CHAIN_STATE_SIZE + std::ceil(pruneTarget * 1024 * 1024.0 / GB_BYTES);
 126 | +    ui->sizeWarningLabel->setText(ui->sizeWarningLabel->text().arg(requiredSpace));
```
jonasschnelli commented at 2:14 PM on January 6, 2016:

It would be nice of the required disk space would be bold. Would something like setText(ui->sizeWarningLabel->text().arg(QString("<strong>%1</strong>").arg(requiredSpace)) work?

MarcoFalke commented at 5:05 PM on January 6, 2016:

Works, but GB is not bold:

jonasschnelli commented at 7:53 AM on January 7, 2016:

Could/should we take the GB out of the translation string and place it in the source code directly?

MarcoFalke commented at 11:57 AM on January 7, 2016:

Makes sense. What about merging this as is and then backport to 0.12, so translators don't have to go through all translations to update two chars.

Afterward, we can apply your proposed changes to this in 0.13 and maybe additionally add another sentence to the translation which tells that pruning is enabled?
in src/qt/intro.cpp:None in faf3299b73
```
 117 | @@ -112,7 +118,11 @@ Intro::Intro(QWidget *parent) :
 118 |      signalled(false)
 119 |  {
 120 |      ui->setupUi(this);
 121 | -    ui->sizeWarningLabel->setText(ui->sizeWarningLabel->text().arg(BLOCK_CHAIN_SIZE/GB_BYTES));
 122 | +    uint64_t pruneTarget = std::max<int64_t>(0, GetArg("-prune", 0));
 123 | +    requiredSpace = BLOCK_CHAIN_SIZE;
 124 | +    if (pruneTarget)
 125 | +        requiredSpace = CHAIN_STATE_SIZE + std::ceil(pruneTarget * 1024 * 1024.0 / GB_BYTES);
```
jonasschnelli commented at 2:20 PM on January 6, 2016:

Shouldn't we check for if (pruneTarget < MIN_DISK_SPACE_FOR_BLOCK_FILES) or something like std::min(MIN_DISK_SPACE_FOR_BLOCK_FILES, pruneTarget)?

MarcoFalke commented at 2:24 PM on January 6, 2016:

I don't see a valid reason to use -prune=1000000 etc. A warning might be appropriate but clearly it should not be silently ignored, so the user does not even notice.

jonasschnelli commented at 2:26 PM on January 6, 2016:

I don't see a valid reason to use -prune=1000000 etc. A warning might be appropriate but clearly it should not be silently ignored, so the user does not even notice.

Right. But -prune=1 should fall back to MIN_DISK_SPACE_FOR_BLOCK_FILES (currently 550MB). Otherwise – i guess – you calculate the wrong min space requirement?

MarcoFalke commented at 2:37 PM on January 6, 2016:

I can't test right now but -prune=1 and -prune=550 should display the same required space. (3 GB)

MarcoFalke commented at 2:38 PM on January 6, 2016:

-prune=1 will fail anyway (after you click OK) but I didn't plan to fix this within this pull

jonasschnelli commented at 2:43 PM on January 6, 2016:

I can't test right now but -prune=1 and -prune=550 should display the same required space. (3 GB)

Right. But what if we change the constants. -prune=1 only results in 3GB because you use std::ceil and a chainstate size of 2GB. I just think the require space should be calculated with respecting the MIN_DISK_SPACE_FOR_BLOCK_FILES, otherwise we give wrong promises to the users (even if it fails afterwards).
jonasschnelli commented at 2:20 PM on January 6, 2016: contributor

Concept ACK, slightly tested. https://bitcoin.jonasschnelli.ch/pulls/7298/
jonasschnelli added the label Needs backport on Jan 7, 2016
jonasschnelli commented at 12:03 PM on January 7, 2016: contributor
Tested ACK faf3299b73ccb5044b7eaced229ac0c904aa25f5. Definitively an improvement.

Next steps could be:
- backport for 0.12.
- extract GB from translation and make the whole size information bold.
- maybe include MIN_DISK_SPACE_FOR_BLOCK_FILES in the size calculation
jonasschnelli merged this on Jan 7, 2016
jonasschnelli closed this on Jan 7, 2016
jonasschnelli referenced this in commit b1cf0058d9 on Jan 7, 2016
MarcoFalke deleted the branch on Jan 7, 2016
MarcoFalke commented at 7:11 PM on January 7, 2016: member

@laanwj Are you doing the backport for rc1?
MarcoFalke commented at 4:34 PM on January 8, 2016: member

Backported as b1a8374
luke-jr referenced this in commit e8ccc90bd8 on Jan 10, 2016
luke-jr referenced this in commit 4d056221a9 on Jan 10, 2016
laanwj removed the label Needs backport on Feb 4, 2016
MarcoFalke locked this on Sep 8, 2021

Contributors

MarcoFalke

jonasschnelli

Labels

GUI

Linked (view graph)

#7319 [Backport 0.12][Qt] Intro: Display required space