qt: Use fixed-point arithmetic in amount spinbox #4556

pull laanwj wants to merge 3 commits into bitcoin:master from laanwj:2014_07_fixedpoint_amountwidget changing 6 files +195 −146

laanwj commented at 2:42 PM on July 18, 2014: member
Use our own fixed-point (Satoshi-based) subclass of the spinbox instead of QDoubleSpinBox. Fixes various issues and cleans up code
- Fixes issue #4500: Amount widget +/- has floating point rounding artifacts
- Amount box can now be emptied again, without clearing to 0
Also aligns the amount to the right, as in other places.
laanwj commented at 8:37 AM on July 19, 2014: member

@cozz @roybadami @diapolo can you take a look here - I'd feel a lot better if we got rid of the last vestige of floating-point arithmetic with amounts in the GUI.

in src/qt/bitcoinamountfield.cpp:None in 788eb1a67c outdated

 196 | +            QKeyEvent *keyEvent = static_cast<QKeyEvent *>(event);
 197 | +            if (keyEvent->key() == Qt::Key_Comma)
 198 | +            {
 199 | +                // Translate a comma into a period
 200 | +                QKeyEvent periodKeyEvent(event->type(), Qt::Key_Period, keyEvent->modifiers(), ".", keyEvent->isAutoRepeat(), keyEvent->count());
 201 | +                QApplication::sendEvent(this, &periodKeyEvent);

laanwj commented at 8:37 AM on July 19, 2014:

TODO: this likely doesn't have to go through QApplication anymore, can we just call QAbstractSpinBox::event?

Diapolo commented at 6:14 PM on July 19, 2014:

I tend to agree as the scope is now our class and QAbstractSpinBox.

cozz commented at 8:56 PM on July 20, 2014:

Just calling QAbstractSpinBox::event works for me:

return QAbstractSpinBox::event(&periodKeyEvent);

laanwj added the label GUI on Jul 19, 2014
laanwj added the label Wallet on Jul 19, 2014

in src/qt/bitcoinamountfield.cpp:None in 9bf54bf9f3 outdated

   8 | @@ -9,63 +9,175 @@
   9 |  #include "qvaluecombobox.h"
  10 |  
  11 |  #include <QApplication>
  12 | -#include <QDoubleSpinBox>
  13 | +#include <QAbstractSpinBox>
  14 |  #include <QHBoxLayout>
  15 |  #include <QKeyEvent>
  16 | +#include <QLineEdit>
  17 |  #include <qmath.h> // for qPow()

Diapolo commented at 6:14 PM on July 19, 2014:

Seems this is obsolete now.

in src/qt/bitcoinamountfield.cpp:None in 9bf54bf9f3 outdated

  19 | -// QDoubleSpinBox that shows SI-style thin space thousands separators
  20 | -class AmountSpinBox: public QDoubleSpinBox
  21 | +/** QSpinBox that uses fixed-point numbers internally and uses our own
  22 | + * formatting/parsing functions.
  23 | + */
  24 | +class AmountSpinBox: public QAbstractSpinBox

Diapolo commented at 6:15 PM on July 19, 2014:

Does this need the Q_OBJECT macro?

in src/qt/bitcoinamountfield.cpp:None in 9bf54bf9f3 outdated

  25 |  {
  26 |  public:
  27 |      explicit AmountSpinBox(QWidget *parent):
  28 | -        QDoubleSpinBox(parent)
  29 | +        QAbstractSpinBox(parent),
  30 | +        currentUnit(0),

Diapolo commented at 6:18 PM on July 19, 2014:

Could/Should this better be BitcoinUnits::BTC?

cozz commented at 9:04 PM on July 20, 2014: contributor
- Object::connect: No such signal QAbstractSpinBox::valueChanged(QString) in qt/bitcoinamountfield.cpp:201 We need a different implementation of the signal valueChanged now.
- If the field is empty or zero, you can not use the single step feature.
- For example, if you enter 1 mBTC and then press single step down, the result is 1 satoshi. Same for 1 uBTC.
- The alignment of the field-content has changed from left to right. Is this intentional?
laanwj commented at 5:08 AM on July 21, 2014: member

The alignment change to right is intentional, as we always align amounts to the right. Will look at the rest.
laanwj commented at 7:36 AM on July 21, 2014: member
- Changed the event code to send the event to the superclass directly
- Added Q_OBJECT and the necessary .moc boilerplate
If the field is empty or zero, you can not use the single step feature.

Fixed. Stepping up from an empty field is now allowed, and zero amounts are allowed. I moved the check for zero amounts to SendCoinsEntry as that's the only place where zero amounts are invalid.

For example, if you enter 1 mBTC and then press single step down, the result is 1 satoshi. Same for 1 uBTC.

Fixed.

Object::connect: No such signal QAbstractSpinBox::valueChanged(QString) in qt/bitcoinamountfield.cpp:201 We need a different implementation of the signal valueChanged now.

Fixed. I changed the signal to valueChanged(qint64) instead.
cozz commented at 2:47 AM on July 22, 2014: contributor
works for me now, after I made 2 minor changes:
- sendcoinsentry.cpp, line 75 needs to connect to valueChanged(qint64) instead of textChanged()
- the signal is not triggered, if you manually edit the field, only single-step triggers. I simply added an "emit valueChanged(value());" to the event-function in bitcoinamountfield.cpp, line 176.
laanwj commented at 6:43 AM on July 22, 2014: member

Thanks for testing and your suggestions.

I think connecting lineEdit()->textEdited to a signal handler that does emit valueChanged(value()); would be proper way to handle this.
laanwj commented at 7:06 AM on July 22, 2014: member

Or even better, just connect lineEdit()->textEdited to valueChanged. No one is using the passed-in value so it can just as well be valueChanged() without arg.
laanwj commented at 7:34 AM on July 22, 2014: member

OK - should be fixed now.
cozz commented at 1:21 PM on July 22, 2014: contributor

Just tested, looks good now.

qt: Use fixed-point arithmetic in amount spinbox

Fixes various issues and cleans up code

- Fixes issue #4500: Amount widget +/- has floating point rounding artifacts
- Amount box can now be emptied again, without clearing to 0

Also aligns the amount to the right, as in other places.

91cce1732b

qt: Remove unused functions from BitcoinUnits
Remove two functions that are now unused.
2a05101efd
ui: Make sure sendcoinsentry signals only connected once
Move signal connections to constructor where possible.
29eaa31694
laanwj merged this on Jul 23, 2014
laanwj closed this on Jul 23, 2014
laanwj referenced this in commit 7bf8ab9dd3 on Jul 23, 2014
BitcoinPullTester commented at 4:15 PM on July 23, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4556_29eaa316944477af538b75b439a49ee1a9fc2f2a/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#4500 Amount widget +/- has floating point rounding artifacts #5287 [Qt] Fix height of BitcoinAmountField