[Qt] Use CURRENCY_UNIT for BitcoinUnits. #7894

pull domob1812 wants to merge 1 commits into bitcoin:master from domob1812:currency-unit changing 1 files +5 −3

domob1812 commented at 6:25 PM on April 16, 2016: contributor

Instead of hardcoding the string "BTC" in bitcoinunits.cpp, make use of the already existing constant CURRENCY_UNIT of amount.h.

[Qt] Use CURRENCY_UNIT for BitcoinUnits.

Instead of hardcoding the string "BTC" in bitcoinunits.cpp, make use of
the already existing constant CURRENCY_UNIT of amount.h.

cf56ea7094

jonasschnelli added the label GUI on Apr 17, 2016
jonasschnelli commented at 7:44 AM on April 17, 2016: contributor

Yes. Why not. utACK cf56ea709490cc6ac5b7d7896e843fdc3e909342
MarcoFalke commented at 9:03 AM on April 17, 2016: member

utACK
in src/qt/bitcoinunits.cpp:None in cf56ea7094
```
  43 |      switch(unit)
  44 |      {
  45 | -    case BTC: return QString("BTC");
  46 | -    case mBTC: return QString("mBTC");
  47 | -    case uBTC: return QString::fromUtf8("μBTC");
  48 | +    case BTC: return QString("%1").arg(baseUnit);
```
paveljanik commented at 9:10 AM on April 17, 2016:

Is %1 etc. needed for "BTC"/baseUnit here? Isn't QString(baseUnit) enough here?

domob1812 commented at 9:18 AM on April 17, 2016:

Actually just baseUnit alone would be enough. I chose the current style because it is in my opinion more readable, since all three cases look the same. But since this is a matter of taste, I can as well change the code if everyone thinks that I should just return baseUnit here.

paveljanik commented at 9:24 AM on April 17, 2016:

I see this was the goal, but you must have ::fromUtf8 in the micro case anyway...
laanwj commented at 10:18 AM on April 18, 2016: member

I'm not convinced that this doesn't just obscure things. The idea of BitcoinUnits was already to be able to change the units there easily. Hiding the actual unit names behind a layer of splicing together strings is not very elegant. (the idea is that different coins may have different units, there's no need for them to be mXTC etc)
domob1812 commented at 11:49 AM on April 18, 2016: contributor

Ok, but what is then the point of having CURRENCY_UNIT in amount.h, if not to reduce the number of times where the string is written?
laanwj commented at 12:16 PM on April 18, 2016: member

See it like this: bitcoinunits is the GUI's equivalent of the core's amount.h

In the future we want to decrease the coupling between both, this doesn't really help that.
domob1812 commented at 12:39 PM on April 18, 2016: contributor

I still believe that it makes sense to have the constant duplicated in as few places as possible, but I can also understand this point of view. So if you prefer to keep this separation, feel free to close the PR.
laanwj commented at 8:25 AM on April 19, 2016: member

Ok, closing this then...
laanwj closed this on Apr 19, 2016
domob1812 deleted the branch on Apr 19, 2016
laanwj commented at 6:49 AM on April 20, 2016: member

Another thing to consider here: BITCOIN SIGN has been assigned the unicode codepoint U+20BF. In the future we likely want to use that in the UI instead (or along with) the three-letter code. There will always be GUI-specific component to currency "visualization".
DrahtBot locked this on Sep 8, 2021

Contributors

Labels