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

jonasschnelli · 2017-07-07T20:08:02Z

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 · 2017-07-07T20:19:15Z

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.

promag · 2017-07-07T23:15:09Z

src/qt/sendcoinsdialog.cpp

@@ -31,6 +31,25 @@
 #include <QTextDocument>
 #include <QTimer>

+static const std::array<int, 8> confTargets = { {2, 4, 6, 12, 48, 144, 504, 1008} };


Rename conf_targets.

promag · 2017-07-07T23:18:22Z

src/qt/sendcoinsdialog.cpp

@@ -31,6 +31,25 @@
 #include <QTextDocument>
 #include <QTimer>

+static const std::array<int, 8> confTargets = { {2, 4, 6, 12, 48, 144, 504, 1008} };
+int getConfTargetForIndex(int index) {
+    if (index+1 > static_cast<int>(confTargets.size())) {


if (index >= ...

Or just:

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

flack · 2017-07-08T16:09:02Z

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 · 2017-07-08T16:14:54Z

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 · 2017-07-08T21:10:05Z

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 · 2017-07-09T11:43:51Z

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

jonasschnelli · 2017-07-10T14:41:13Z

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

promag · 2017-07-10T15:14:36Z

src/qt/sendcoinsdialog.cpp

@@ -177,10 +199,10 @@ void SendCoinsDialog::setModel(WalletModel *_model)

        // set the smartfee-sliders default value (wallets default conf.target or last stored value)
        QSettings settings;
-        if (settings.value("nSmartFeeSliderPosition").toInt() == 0)
-            ui->sliderSmartFee->setValue(ui->sliderSmartFee->maximum() - model->getDefaultConfirmTarget() + 2);


Should call settings.remove("nSmartFeeSliderPosition")?

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

That doesn't seem particularly important to me.

promag · 2017-07-10T15:20:46Z

src/qt/sendcoinsdialog.cpp

-        connect(ui->sliderSmartFee, SIGNAL(valueChanged(int)), this, SLOT(updateGlobalFeeVariables()));
-        connect(ui->sliderSmartFee, SIGNAL(valueChanged(int)), this, SLOT(coinControlUpdateLabels()));
+        for (const int &n : confTargets) {
+            ui->confTargetSelector->addItem(tr("%1 blocks, %2").arg(n).arg(GUIUtil::formatNiceTimeOffset(n*Params().GetConsensus().nPowTargetSpacing)));


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

Yes. Your right.. will fix.

promag · 2017-07-10T15:27:42Z

src/qt/sendcoinsdialog.cpp

@@ -31,6 +31,25 @@
 #include <QTextDocument>
 #include <QTimer>

+static const std::array<int, 11> confTargets = { {2, 3, 4, 5, 6, 12, 24, 48, 144, 504, 1008} };
+int getConfTargetForIndex(int index) {


Update block style. Same for getIndexForConfTarget.

promag · 2017-07-10T15:27:48Z

src/qt/sendcoinsdialog.cpp

@@ -31,6 +31,25 @@
 #include <QTextDocument>
 #include <QTimer>

+static const std::array<int, 11> confTargets = { {2, 3, 4, 5, 6, 12, 24, 48, 144, 504, 1008} };


Snake case.

morcos · 2017-07-10T17:28:41Z

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.

TheBlueMatt

Really think we need this for 15.

TheBlueMatt · 2017-07-12T19:18:09Z

src/qt/sendcoinsdialog.cpp

@@ -31,6 +31,25 @@
 #include <QTextDocument>
 #include <QTimer>

+static const std::array<int, 11> confTargets = { {2, 3, 4, 5, 6, 12, 24, 48, 144, 504, 1008} };


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

Also, why the extra set of {}?

Also, why the extra set of {}?

Compiler warning.

TheBlueMatt · 2017-07-12T19:18:15Z

src/qt/sendcoinsdialog.cpp

+            return i;
+        }
+    }
+    return confTargets.back();


I think you meant .size() - 1.

Oh. Yes. Will fix.

sipa · 2017-07-12T19:19:35Z

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

achow101 · 2017-07-12T22:57:54Z

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 · 2017-07-12T23:21:28Z

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.

Always round up (conservative)

jonasschnelli · 2017-07-13T10:22:35Z

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 · 2017-07-13T14:47:53Z

Tested ACK 2aef1f1

TheBlueMatt · 2017-07-14T20:36:39Z

utACK 2aef1f1

…argets 2aef1f1 [Qt] migrate old fee slider value to new dropbown Always round up (conservative) (Jonas Schnelli) bc1be90 [Qt] replace fee slider with a Dropdown, extend conf. targets (Jonas Schnelli) Tree-SHA512: 53796cf0b434dd3db5d4680dbeb6231a7df8f15d88187178fd4db8917cd7fc60091ce2c1589fd93668fc94bb13f989aba5b7ef3792fa95ee1f9f21a15709e2d3

…conf. targets 2aef1f1 [Qt] migrate old fee slider value to new dropbown Always round up (conservative) (Jonas Schnelli) bc1be90 [Qt] replace fee slider with a Dropdown, extend conf. targets (Jonas Schnelli) Tree-SHA512: 53796cf0b434dd3db5d4680dbeb6231a7df8f15d88187178fd4db8917cd7fc60091ce2c1589fd93668fc94bb13f989aba5b7ef3792fa95ee1f9f21a15709e2d3

jonasschnelli added the GUI label Jul 7, 2017

promag reviewed Jul 7, 2017

View reviewed changes

jonasschnelli added this to the 0.15.0 milestone Jul 9, 2017

jonasschnelli force-pushed the 2017/07/qt_fee_slider branch 2 times, most recently from 979cd99 to 19eae9c Compare July 10, 2017 14:41

promag reviewed Jul 10, 2017

View reviewed changes

laanwj self-assigned this Jul 11, 2017

jonasschnelli mentioned this pull request Jul 11, 2017

Add PR description to merge commit in github-merge.py #10786

Merged

TheBlueMatt reviewed Jul 12, 2017

View reviewed changes

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

bc1be90

jonasschnelli force-pushed the 2017/07/qt_fee_slider branch from 19eae9c to ff54ece Compare July 13, 2017 10:20

[Qt] migrate old fee slider value to new dropbown

2aef1f1

Always round up (conservative)

jonasschnelli force-pushed the 2017/07/qt_fee_slider branch from ff54ece to 2aef1f1 Compare July 13, 2017 10:21

sipa merged commit 2aef1f1 into bitcoin:master Jul 15, 2017

This was referenced Jul 15, 2017

Improve wallet fee logic and fix GUI bugs #10706

Merged

Access to longer fee estimates using GUI #10590

Closed

jonasschnelli mentioned this pull request Jul 31, 2017

TODO for release notes 0.15.0 #9889

Closed

12 tasks

meshcollider mentioned this pull request Sep 14, 2017

Fee estimator should be more user-friendly. #9914

Closed

meshcollider mentioned this pull request Mar 7, 2018

Recommended Transaction Fee Slider: non-monotonic behavior #6437

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

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

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

Uh oh!

Conversation

jonasschnelli commented Jul 7, 2017

Uh oh!

jonasschnelli commented Jul 7, 2017

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!

flack commented Jul 8, 2017

Uh oh!

morcos commented Jul 8, 2017

Uh oh!

luke-jr commented Jul 8, 2017

Uh oh!

gmaxwell commented Jul 9, 2017

Uh oh!

jonasschnelli commented Jul 10, 2017

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!

Choose a reason for hiding this comment

Uh oh!

morcos commented Jul 10, 2017

Uh oh!

TheBlueMatt 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!

sipa commented Jul 12, 2017

Uh oh!

achow101 commented Jul 12, 2017

Uh oh!

morcos commented Jul 12, 2017

Uh oh!

jonasschnelli commented Jul 13, 2017

Uh oh!

morcos commented Jul 13, 2017

Uh oh!

TheBlueMatt commented Jul 14, 2017

Uh oh!

Uh oh!