/bitcoin

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

Merged
merged 2 commits into from Jul 15, 2017

Conversation

Reviewers

@TheBlueMatt TheBlueMatt

@promag promag

@morcos morcos

Assignees

@laanwj laanwj

Labels
GUI
Projects
None yet
Milestone
  0.15.0
10 participants
@jonasschnelli @flack @morcos @luke-jr @gmaxwell @sipa @achow101 @TheBlueMatt @promag @laanwj
@jonasschnelli
Member

jonasschnelli commented Jul 7, 2017

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

bildschirmfoto 2017-07-07 um 21 56 19

jonasschnelli added the GUI label Jul 7, 2017

@jonasschnelli
Member

jonasschnelli commented Jul 7, 2017

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.
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} };
@promag

promag Jul 7, 2017

Contributor

Rename conf_targets.

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())) {
@promag

promag Jul 7, 2017

Contributor 
if (index >= ...
@promag

promag Jul 7, 2017

Contributor

Or just:

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

flack commented Jul 8, 2017

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
Contributor

morcos commented Jul 8, 2017

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
Member

luke-jr commented Jul 8, 2017

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
Member

gmaxwell commented Jul 9, 2017

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

jonasschnelli added this to the 0.15.0 milestone Jul 9, 2017

@jonasschnelli
Member

jonasschnelli commented Jul 10, 2017

Re-added targets 3, 5, 24 and removed the tilde in front of the target-in-blocks/-time.
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} };
@promag

promag Jul 10, 2017

Contributor

Snake case.

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) {
@promag

promag Jul 10, 2017

Contributor

Update block style. Same for getIndexForConfTarget.

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)));
@promag

promag Jul 10, 2017

Contributor

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

@jonasschnelli

jonasschnelli Jul 13, 2017

Member

Yes. Your right.. will fix.

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);
@promag

promag Jul 10, 2017

Contributor

Should call settings.remove("nSmartFeeSliderPosition")?

@jonasschnelli

jonasschnelli Jul 13, 2017

Member

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

@morcos

morcos Jul 13, 2017

Contributor

That doesn't seem particularly important to me.

@morcos
Contributor

morcos commented Jul 10, 2017

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 self-assigned this Jul 11, 2017

jonasschnelli referenced this pull request Jul 11, 2017

Merged

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

@TheBlueMatt

TheBlueMatt reviewed Jul 12, 2017
View changes

Really think we need this for 15.

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} };
@TheBlueMatt

TheBlueMatt Jul 12, 2017

Contributor

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

@TheBlueMatt

TheBlueMatt Jul 12, 2017

Contributor

Also, why the extra set of {}?

@jonasschnelli

jonasschnelli Jul 12, 2017

Member

Also, why the extra set of {}?

Compiler warning.

src/qt/sendcoinsdialog.cpp
+ return i;
+ }
+ }
+ return confTargets.back();
@TheBlueMatt

TheBlueMatt Jul 12, 2017

Contributor

I think you meant .size() - 1.

@jonasschnelli

jonasschnelli Jul 12, 2017

Member

Oh. Yes. Will fix.

@sipa
Owner

sipa commented Jul 12, 2017

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

achow101 commented Jul 12, 2017

utACK 19eae9c 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.

image
@morcos
Contributor

morcos commented Jul 12, 2017

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.

jonasschnelli added some commits Jul 7, 2017

@jonasschnelli jonasschnelli [Qt] replace fee slider with a Dropdown, extend conf. targets
This commit was signed with a verified signature.
@jonasschnelli jonasschnelli Jonas Schnelli
GPG key ID: 1EB776BB03C7922D Learn about signing commits
bc1be90
@jonasschnelli jonasschnelli [Qt] migrate old fee slider value to new dropbown 
Always round up (conservative)
This commit was signed with a verified signature.
@jonasschnelli jonasschnelli Jonas Schnelli
GPG key ID: 1EB776BB03C7922D Learn about signing commits
2aef1f1
@jonasschnelli
Member

jonasschnelli commented Jul 13, 2017

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
Contributor

morcos commented Jul 13, 2017

Tested ACK 2aef1f1
@TheBlueMatt
Contributor

TheBlueMatt commented Jul 14, 2017

utACK 2aef1f1

@sipa sipa merged commit 2aef1f1 into bitcoin:master Jul 15, 2017
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sipa sipa added a commit that referenced this pull request Jul 15, 2017

@sipa sipa Merge #10769: [Qt] replace fee slider with a Dropdown, extend conf. t… 
…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
This commit was signed with a verified signature.
@sipa sipa Pieter Wuille
GPG key ID: A636E97631F767E0 Learn about signing commits
8fdd23a

This was referenced Jul 15, 2017

Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment