[Qt] replace fee slider with a Dropdown, extend conf. targets #10769

jonasschnelli commented at 8:08 PM on July 7, 2017: contributor

Addresses #10590.

This PR replaces the fee slider with a Dropbox box. The Dropdown contains the target in ~blocks and ~estimated time to confirm.

The current supported confirmation targets are: 2, 4, 6, 12, 48, 144, 504, 1008

jonasschnelli added the label GUI on Jul 7, 2017

jonasschnelli commented at 8:19 PM on July 7, 2017: contributor

I probably need to mention why a dropbox and not a slider.

Sliders tend to be linear (otherwise nobody would understand them and I bet it would require a custom, non standard UI object) which would make it useless if we would support conf. targets up to 1008.
A dropbox allows one to give a better overview (slider: you only see the underlaying value if you change it).
Dropbox would allow to show the feerate and or a text ("fast", "economy", etc,), ... and eventual the absolute fee by running the coin-selections if the parameters are provided.

in src/qt/sendcoinsdialog.cpp:34 in e20f45fe85 outdated

  30 | @@ -31,6 +31,25 @@
  31 |  #include <QTextDocument>
  32 |  #include <QTimer>
  33 |  
  34 | +static const std::array<int, 8> confTargets = { {2, 4, 6, 12, 48, 144, 504, 1008} };

promag commented at 11:15 PM on July 7, 2017:

Rename conf_targets.

in src/qt/sendcoinsdialog.cpp:36 in e20f45fe85 outdated

  30 | @@ -31,6 +31,25 @@
  31 |  #include <QTextDocument>
  32 |  #include <QTimer>
  33 |  
  34 | +static const std::array<int, 8> confTargets = { {2, 4, 6, 12, 48, 144, 504, 1008} };
  35 | +int getConfTargetForIndex(int index) {
  36 | +    if (index+1 > static_cast<int>(confTargets.size())) {

promag commented at 11:18 PM on July 7, 2017:

if (index >= ...

promag commented at 11:25 PM on July 7, 2017:

Or just:

  return conf_targets[max(0, min(conf_targets.size() - 1, index)]

flack commented at 4:09 PM on July 8, 2017: contributor

Looking at the screenshot, I would drop the tilde before the number of blocks, i.e. 2 Blocks, ~20 minutes instead of ~2 Blocks, ~20 minutes. It already says "confirmation target", so it should be obvious that it's not exactly x blocks. In other words, make it say "target x blocks (approx. y minutes)" instead of "target approx. x blocks (approx. y minutes)".

Also, shouldn't blocks have a lowercase b?

morcos commented at 4:14 PM on July 8, 2017: member

Will review shortly. Thanks for doing!

Also please add a target for 24 for compatibility with the old estimates (where 25 was the highest possible target). It may be a target that people are used to using.

luke-jr commented at 9:10 PM on July 8, 2017: member

Confirmation is when it's 6 blocks deep, so +5 to all the numbers here? Or change it to "begin confirmation" like we currently have...

gmaxwell commented at 11:43 AM on July 9, 2017: contributor

I like how it looks, @morcos any concerns about missing 3,5 making users fee choices more clumpy?

jonasschnelli added this to the milestone 0.15.0 on Jul 9, 2017

jonasschnelli force-pushed on Jul 10, 2017

jonasschnelli commented at 2:41 PM on July 10, 2017: contributor

Re-added targets 3, 5, 24 and removed the tilde in front of the target-in-blocks/-time.

in src/qt/sendcoinsdialog.cpp:181 in 19eae9c360 outdated

 198 | @@ -177,10 +199,10 @@ void SendCoinsDialog::setModel(WalletModel *_model)
 199 |  
 200 |          // set the smartfee-sliders default value (wallets default conf.target or last stored value)
 201 |          QSettings settings;
 202 | -        if (settings.value("nSmartFeeSliderPosition").toInt() == 0)
 203 | -            ui->sliderSmartFee->setValue(ui->sliderSmartFee->maximum() - model->getDefaultConfirmTarget() + 2);

promag commented at 3:14 PM on July 10, 2017:

Should call settings.remove("nSmartFeeSliderPosition")?

jonasschnelli commented at 8:09 AM on July 13, 2017:

Hmm... I think an addition is required: we probably need to migrate the old slider value to the new dropdown?

morcos commented at 8:37 AM on July 13, 2017:

That doesn't seem particularly important to me.

in src/qt/sendcoinsdialog.cpp:175 in 19eae9c360 outdated

 170 | @@ -152,9 +171,12 @@ void SendCoinsDialog::setModel(WalletModel *_model)
 171 |          coinControlUpdateLabels();
 172 |  
 173 |          // fee section
 174 | -        connect(ui->sliderSmartFee, SIGNAL(valueChanged(int)), this, SLOT(updateSmartFeeLabel()));
 175 | -        connect(ui->sliderSmartFee, SIGNAL(valueChanged(int)), this, SLOT(updateGlobalFeeVariables()));
 176 | -        connect(ui->sliderSmartFee, SIGNAL(valueChanged(int)), this, SLOT(coinControlUpdateLabels()));
 177 | +        for (const int &n : confTargets) {
 178 | +            ui->confTargetSelector->addItem(tr("%1 blocks, %2").arg(n).arg(GUIUtil::formatNiceTimeOffset(n*Params().GetConsensus().nPowTargetSpacing)));

promag commented at 3:20 PM on July 10, 2017:

Alternative format: "%2 (%1 blocks)" since the label is Confirmation time.

jonasschnelli commented at 8:07 AM on July 13, 2017:

Yes. Your right.. will fix.

in src/qt/sendcoinsdialog.cpp:35 in 19eae9c360 outdated

  30 | @@ -31,6 +31,25 @@
  31 |  #include <QTextDocument>
  32 |  #include <QTimer>
  33 |  
  34 | +static const std::array<int, 11> confTargets = { {2, 3, 4, 5, 6, 12, 24, 48, 144, 504, 1008} };
  35 | +int getConfTargetForIndex(int index) {

promag commented at 3:27 PM on July 10, 2017:

Update block style. Same for getIndexForConfTarget.

in src/qt/sendcoinsdialog.cpp:34 in 19eae9c360 outdated

  30 | @@ -31,6 +31,25 @@
  31 |  #include <QTextDocument>
  32 |  #include <QTimer>
  33 |  
  34 | +static const std::array<int, 11> confTargets = { {2, 3, 4, 5, 6, 12, 24, 48, 144, 504, 1008} };

promag commented at 3:27 PM on July 10, 2017:

Snake case.

TheBlueMatt commented at 7:18 PM on July 12, 2017:

nit: not sure its worth having 5 here, maybe not 3 either.

TheBlueMatt commented at 7:19 PM on July 12, 2017:

Also, why the extra set of {}?

jonasschnelli commented at 7:20 PM on July 12, 2017:

Also, why the extra set of {}?

Compiler warning.

morcos commented at 5:28 PM on July 10, 2017: member

ACK 19eae9c @gmaxwell Actually my preference would be to remove the 3 and 5 targets. I think it gives a false sense of accuracy with the estimates. I don't imagine much clumping concern as those estimates often aren't that different from each other and there is a much larger range of estimates to choose from as well as time variability. I also think its a bit cleaner not to be faced with too many choices in a dropdown.

laanwj assigned laanwj on Jul 11, 2017

in src/qt/sendcoinsdialog.cpp:50 in 19eae9c360 outdated

  45 | +    for (unsigned int i = 0; i < confTargets.size(); i++) {
  46 | +        if (confTargets[i] >= target) {
  47 | +            return i;
  48 | +        }
  49 | +    }
  50 | +    return confTargets.back();

TheBlueMatt commented at 7:18 PM on July 12, 2017:

I think you meant .size() - 1.

jonasschnelli commented at 7:20 PM on July 12, 2017:

Oh. Yes. Will fix.

TheBlueMatt commented at 7:18 PM on July 12, 2017: member

Really think we need this for 15.

sipa commented at 7:19 PM on July 12, 2017: member

GUI-screenshot-looks-nice-ACK. Didn't look at the code.

achow101 commented at 10:57 PM on July 12, 2017: member

utACK 19eae9c3602c784d55f8edc94300da1ece686a6c modulo the thing @TheBlueMatt pointed out.

I don't think we need the 3, 4, or 5 block target estimates.

It may also be useful to include a tooltip that explains why you see the line "Estimated to begin confirmation within N blocks" where N is less than the confirmation target that you chose as seen in the screenshot below. This discrepancy may confuse users.

morcos commented at 11:21 PM on July 12, 2017: member

tooltip that explains why you see the line "Estimated to begin confirmation within N blocks" where N is less than the confirmation target that you chose

Yes would be a nice improvement, could be left for another PR as it's a preexisting issue. Would be great to get than in for 0.15 as well, but wouldn't want it to hold up this.

The other feature that I'd ideally like is the ability to force estimates conservative or economical regardless of the Replace-By-Fee setting. Again just a wish list item.

[Qt] replace fee slider with a Dropdown, extend conf. targets bc1be90e37

jonasschnelli force-pushed on Jul 13, 2017

[Qt] migrate old fee slider value to new dropbown

Always round up (conservative)

2aef1f1829

jonasschnelli force-pushed on Jul 13, 2017

jonasschnelli commented at 10:22 AM on July 13, 2017: contributor

Removed again target 3 and 5 and fixed points found by @TheBlueMatt and @promag. Added another commit that add a tiny migration logic.

Thanks for a retest/re-ack.

morcos commented at 2:47 PM on July 13, 2017: member

Tested ACK 2aef1f1

TheBlueMatt commented at 8:36 PM on July 14, 2017: member

utACK 2aef1f18296fcd3aa3c91afdf152add8a8e80bd4

sipa merged this on Jul 15, 2017

sipa closed this on Jul 15, 2017

sipa referenced this in commit 8fdd23a224 on Jul 15, 2017

PastaPastaPasta referenced this in commit 2a76f974df on Aug 6, 2019

PastaPastaPasta referenced this in commit 0b45af7b07 on Aug 6, 2019

PastaPastaPasta referenced this in commit f3d7e5f87e on Aug 6, 2019

PastaPastaPasta referenced this in commit 0002628934 on Aug 7, 2019

PastaPastaPasta referenced this in commit 4089462323 on Aug 8, 2019

PastaPastaPasta referenced this in commit 1a75762b58 on Aug 12, 2019

barrystyle referenced this in commit 0e68ea4b88 on Jan 22, 2020

DrahtBot locked this on Sep 8, 2021