[Qt] New status bar Unit Display Control and related changes. #4302

gubatron commented at 6:28 AM on June 7, 2014: contributor

New status bar control shows the current Unit of Display. When clicked (left,or right button) it shows a context menu that allows the user to switch the current Unit of Display (BTC, mBTC, uBTC)
Recent Requests and Transaction Table headers are now updated when unit of display is changed, because their "Amount" column now displays the current unit of display.
Takes care of issue #3970 Units in transaction export csv file.
Small refactors for code reuse.
Demo Video https://www.youtube.com/watch?v=wwcr0Yh68go&list=UUG3jF2hgofmLWP0tRPisQAQ

laanwj added the label Wallet on Jun 16, 2014

laanwj commented at 3:26 PM on June 16, 2014: member

This needs some testing!

Diapolo commented at 6:49 AM on June 17, 2014: none

Please order your new includes alphabetically :).

in src/qt/bitcoingui.cpp:None in d8d755a1a6 outdated

1039 | +    setContextMenuPolicy(Qt::CustomContextMenu);
1040 | +    connect(this,SIGNAL(customContextMenuRequested(const QPoint&)),this,SLOT(onDisplayUnitsClicked(const QPoint&)));
1041 | +}
1042 | +
1043 | +/** Lets the control know about the Options Model (and its signals) */
1044 | +void UnitDisplayStatusBarControl::connectOptionsModel(OptionsModel *optionsModel)

Diapolo commented at 6:51 AM on June 17, 2014:

To stay consistent can you please rename this to setOptioneModel().

gubatron commented at 2:37 PM on June 17, 2014:

:+1:

in src/qt/bitcoingui.cpp:None in d8d755a1a6 outdated

1054 | +        onDisplayUnitsChanged(optionsModel->getDisplayUnit());
1055 | +    }
1056 | +}
1057 | +
1058 | +/** When Display Units are changed on OptionsModel it will refresh the display text of the control on the status bar */
1059 | +void UnitDisplayStatusBarControl::onDisplayUnitsChanged(int newUnits)

Diapolo commented at 6:53 AM on June 17, 2014:

For this we mostly use updateDisplayUnit().

gubatron commented at 1:18 PM on June 17, 2014:

can you be more specific which part, the

onDisplayUnitsChanged(optionsModel->getDisplayUnit());

?

will try updateDisplayUnit() there instead and see.

Diapolo commented at 1:24 PM on June 17, 2014:

Just rename that function to onDisplayUnitsChanged() and I'm fine with it.

in src/qt/bitcoingui.cpp:None in d8d755a1a6 outdated

1048 | +        this->optionsModel = optionsModel;
1049 | +
1050 | +        //be aware of a display unit change reported by the OptionsModel object.
1051 | +        connect(optionsModel,SIGNAL(displayUnitChanged(int)),this,SLOT(onDisplayUnitsChanged(int)));
1052 | +
1053 | +        //initialize the display units label with the current value in the model.

Diapolo commented at 7:23 AM on June 17, 2014:

Sorry to be so nitty, but that is my job here ^^, please add a space after // as comments tend to be more readable then.

gubatron commented at 1:16 PM on June 17, 2014:

please be nitty, trying to learn here and do my best

:+1:

in src/qt/bitcoingui.h:None in d8d755a1a6 outdated

   8 | @@ -9,9 +9,14 @@
   9 |  #include "bitcoin-config.h"
  10 |  #endif
  11 |  
  12 | +#include "optionsmodel.h"

Diapolo commented at 7:25 AM on June 17, 2014:

We have this already in the .cpp, try adding just class OptionsModel; after class Notificator; a few lines below.

gubatron commented at 1:10 PM on June 17, 2014:

:+1:

in src/qt/optionsmodel.cpp:None in d8d755a1a6 outdated

 300 | @@ -301,9 +301,8 @@ bool OptionsModel::setData(const QModelIndex & index, const QVariant & value, in
 301 |              break;
 302 |  #endif
 303 |          case DisplayUnit:
 304 | -            nDisplayUnit = value.toInt();
 305 | -            settings.setValue("nDisplayUnit", nDisplayUnit);
 306 | -            emit displayUnitChanged(nDisplayUnit);
 307 | +            setDisplayUnit(value);
 308 | +            return true;

Diapolo commented at 7:27 AM on June 17, 2014:

This is not needed.

gubatron commented at 1:11 PM on June 17, 2014:

just trying not to have repeated code as I think I needed the same code elsewhere. (DRY)

Diapolo commented at 1:24 PM on June 17, 2014:

I'm just talking about the return true;.

gubatron commented at 2:51 AM on June 22, 2014:

oh gotcha, I think I was just trying not to emit dataChanged after the switch statment. will remove it and see if it doesn't cause any unwanted behavior.

in src/qt/optionsmodel.cpp:None in d8d755a1a6 outdated

 349 |  }
 350 |  
 351 | +/** Updates current unit in memory, settings and emits displayUnitChanged(newUnit) signal */
 352 | +void OptionsModel::setDisplayUnit(const QVariant &value)
 353 | +{
 354 | +    QVariant defensiveCopy = QVariant(value);

Diapolo commented at 7:30 AM on June 17, 2014:

Can you explain the need for this copy?

gubatron commented at 1:15 PM on June 17, 2014:

hmm, i guess since it's const already not necessary, trying to remember why, perhaps while testing I thought of the possibility of another signal being fired, just being careful and trying to avoid mutability i guess.

in src/qt/transactionview.cpp:None in d8d755a1a6 outdated

 321 | @@ -322,6 +322,17 @@ void TransactionView::exportClicked()
 322 |      }
 323 |  }
 324 |  
 325 | +QString TransactionView::getAmountColumnTitle()

Diapolo commented at 7:33 AM on June 17, 2014:

This looks pretty similar to QString TransactionTableModel::getAmountTitle(), perhaps you can add a generic function somewhere (e.g. GUIUtil)?

gubatron commented at 1:15 PM on June 17, 2014:

:+1:

laanwj commented at 9:06 AM on June 17, 2014: member

@diapolo Have you also tested the functionality? Although I agree with your nits, I think user experience is more important than small details at this initial stage. Is this what the interface should be?

gubatron commented at 1:19 PM on June 17, 2014: contributor

(I'm currently out of town, I'll be back in like 3 days or so and then I'll be able to push all the fixes, meanwhile please test functionality)

Diapolo commented at 1:26 PM on June 17, 2014: none

@laanwj I didn't yet test the functionality, my first stage for Qt pulls is the code and style always ;). Yes we need to evaluate if the UX this pull adds is something we like/want, I agree.

laanwj commented at 8:17 AM on June 23, 2014: member

Looks like you accidentally got some other merges in here. When you want to bring your tree up to date use git rebase or pull --rebase. This needs some git acrobatics to fix.

gubatron commented at 2:00 PM on June 23, 2014: contributor

got everything into a single rebased commit now. still getting the hang of the whole rebasing and pulling.

[Qt] New status bar Unit Display Control and related changes.

- New status bar control shows the current Unit of Display.
  When clicked (left,or right button) it shows a context menu
  that allows the user to switch the current Unit of Display (BTC, mBTC, uBTC)
- Recent Requests and Transaction Table headers are now updated when
  unit of display is changed, because their "Amount" column now displays the
  current unit of display.
- Takes care of issue #3970 Units in transaction export csv file.
- Small refactors for reusability.
- Demo Video https://www.youtube.com/watch?v=wwcr0Yh68go&list=UUG3jF2hgofmLWP0tRPisQAQ
- changes after Diapolo's feedback. Have not been able to build after last pool, issues with boost on MacOSX, will test on Ubuntu these changes.
- removed return statement on switch
- renamed onDisplayUnitsChanged(int) to updateDisplayUnit(int)
- now getAmountColumnTitle(int unit) takes a simple unit parameter. moved to BitcoinUnits.

8969828d06

in src/qt/guiutil.cpp:None in cbdefcd3ac outdated

 801 | @@ -801,4 +802,14 @@ QString formatServicesStr(uint64_t mask)
 802 |          return QObject::tr("None");
 803 |  }
 804 |  
 805 | +QString getAmountColumnTitle(OptionsModel* optionsModel)

laanwj commented at 10:17 AM on June 25, 2014:

This function should not be in guiutil, I don't like introducing a dependency of guiutil on optionsmodel.

Please move it to BitcoinUnits and make it take a unit ID instead of an options model.

gubatron commented at 10:24 AM on June 25, 2014:

:+1:

in src/qt/transactionview.cpp:None in cbdefcd3ac outdated

 308 | @@ -309,7 +309,7 @@ void TransactionView::exportClicked()
 309 |      writer.addColumn(tr("Type"), TransactionTableModel::Type, Qt::EditRole);
 310 |      writer.addColumn(tr("Label"), 0, TransactionTableModel::LabelRole);
 311 |      writer.addColumn(tr("Address"), 0, TransactionTableModel::AddressRole);
 312 | -    writer.addColumn(tr("Amount"), 0, TransactionTableModel::FormattedAmountRole);
 313 | +    writer.addColumn(GUIUtil::getAmountColumnTitle(model->getOptionsModel()).toUtf8(), 0, TransactionTableModel::FormattedAmountRole);

laanwj commented at 10:19 AM on June 25, 2014:

This .toUtf8() is unnecessary here. utf8 is the standard conversion used everywhere.

gubatron commented at 10:26 AM on June 25, 2014:

:+1: oh, forgot to remove.

I'm getting issues with the "μ" character when exporting to .csv, but I read this is really an issue of excel not handling encoding correctly when it comes to .csv files and the user is supposed to specify the encoding when opening the .csv.

BitcoinPullTester commented at 11:22 PM on June 25, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4302_8969828d069e4e55108618a493749535edc12ec7/ 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.

laanwj commented at 9:13 AM on July 3, 2014: member

+1 on the interface, I think it is very cool and it works as expected in my (limited) testing

laanwj commented at 9:17 AM on July 3, 2014: member

After this we could make the unit selector on individual amount widgets read-only/an informative label only. After all, if it's so easy to switch the unit globaly, it may be unnecessary to be able to change it on a per-amount basis. *Edit: * but no reason to do it in this pull. let's just test and review it and get it merged.

in src/qt/bitcoingui.cpp:None in 8969828d06

  50 | @@ -49,6 +51,8 @@
  51 |  #include <QToolBar>
  52 |  #include <QVBoxLayout>
  53 |  
  54 | +

laanwj commented at 9:28 AM on July 3, 2014:

No need to add extra whitespace here :)

in src/qt/bitcoingui.h:None in 8969828d06

 206 | @@ -198,4 +207,32 @@ private slots:
 207 |      void showProgress(const QString &title, int nProgress);
 208 |  };
 209 |  
 210 | +class UnitDisplayStatusBarControl : public QLabel

laanwj commented at 9:34 AM on July 3, 2014:

Do we want to move this class to a separate .cpp/.h file?

gubatron commented at 3:15 PM on July 3, 2014:

I didn't add a new .cpp/.h as I don't know if that would mean making changes on the build related files (I'm still a noob when it comes to C++ projects)

laanwj commented at 3:46 PM on July 3, 2014:

OK,no problem, can be done later

laanwj merged this on Jul 3, 2014

laanwj closed this on Jul 3, 2014

laanwj referenced this in commit 07b6c2b901 on Jul 3, 2014

Diapolo commented at 2:50 PM on July 4, 2014: none

Nice addition, some small include glitches, which I'm going to cleanup with a small pull :).

gubatron deleted the branch on Jul 5, 2014

wtogami referenced this in commit d4f5c384f3 on Sep 9, 2014

wtogami referenced this in commit 535a7f89f4 on Sep 19, 2014

wtogami referenced this in commit d6b0b267fa on Oct 2, 2014

wtogami referenced this in commit cbe6e7cd18 on Nov 14, 2014

wtogami referenced this in commit 420fb3747c on Dec 23, 2014

MarcoFalke locked this on Sep 8, 2021