refactor: Make BitcoinUnits::Unit a scoped enum

jb55 commented at 5:45 pm on February 22, 2022: contributor

This is a rebased version of #60

Since Qt 5.5 there are means to register an enum type with the meta-object system (such enum still lacks an ability to interact with QSettings::setValue() and QSettings::value() without defined stream operators).

In order to reduce global namespace polluting and to force strong type checking, this PR makes BitcoinUnits::Unit a scoped enum (typedef BitcoinUnits::Unit BitcoinUnit;).

No behavior change.

jb55 commented at 5:46 pm on February 22, 2022: contributor

Previous invalidated ACKS: @Sjors tACK @jonatack Concept ACK @promag Code Review ACK @jb55 Code Review ACK

in src/qt/optionsmodel.cpp:82 in 15294b976b outdated

80+    }
81+    QVariant unit = settings.value("DisplayBitcoinUnit");
82+    if (unit.isValid()) {
83+        m_display_bitcoin_unit = unit.value<BitcoinUnit>();
84+    } else {
85+        m_display_bitcoin_unit = BitcoinUnit::BTC;

promag commented at 6:27 pm on February 22, 2022:

15294b976bb92f8b6183e9c0e3103d647830016b

And we keep the invalid value in settings? This also has the default value repeated. How about:

0const auto unit = settings.value("DisplayBitcoinUnit");
1if (unit.canConvert<BitcoinUnit>()) {
2    m_display_bitcoin_unit = unit.value<BitcoinUnit>();
3} else {
4    m_display_bitcoin_unit = BitcoinUnit::BTC
5    settings.setValue("DisplayBitcoinUnit", QVariant::fromValue(m_display_bitcoin_unit));
6}

jb55 commented at 9:39 pm on February 22, 2022:

good idea, done!

promag approved

promag commented at 6:36 pm on February 22, 2022: contributor

Code review ACK 3466e7fede103359ac4cda8c65370d73ffe5ca30

jb55 force-pushed on Feb 22, 2022

qt: Use QVariant instead of int for BitcoinUnit in QSettings

This change improves type safety.

75832fdc37

qt, refactor: Make BitcoinUnits::Unit a scoped enum aa23960fdf

qt, refactor: Remove BitcoinUnits::valid function

Since BitcoinUnits::Unit became a scoped enum, BitcoinUnits::valid
function is no longer needed.

152d5bad50

qt, refactor: Remove default cases for scoped enum ffbc2fe459

qt: Skip displayUnitChanged signal if unit is not actually changed

Co-authored-by: João Barbosa <joao.paulo.barbosa@gmail.com>

0554251d66

qt/wallettests: sort includes

I split this out from an earlier commit to make it a bit less noisy.

Signed-off-by: William Casarin <jb55@jb55.com>

0e5dedbc9e

in src/qt/optionsmodel.cpp:84 in 9b018ca26d outdated

82+    if (unit.canConvert<BitcoinUnit>()) {
83+        m_display_bitcoin_unit = unit.value<BitcoinUnit>();
84+    } else {
85+        m_display_bitcoin_unit = BitcoinUnit::BTC;
86+
87+        // If for whatever reason we have an invalid DisplayBitcoinUnit setting,

promag commented at 9:40 pm on February 22, 2022:

9b018ca26db5c9ea4d3abdb432a9016607a5c6b8

I think this comment is unnecessary. Besides, this runs on the first app execution.

jb55 commented at 9:51 pm on February 22, 2022:

okies, done.

jb55 force-pushed on Feb 22, 2022

promag approved

promag commented at 10:17 pm on February 22, 2022: contributor

Code review ACK 0e5dedbc9eb54105ab9b0c4ce1f57afa55bcb5b6

DrahtBot commented at 9:00 am on February 24, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#435 (Move third-party tx URL setting from Display to Wallet options tab by jarolrod)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

hebasto added the label Refactoring on Mar 10, 2022

in src/qt/bitcoinunits.h:114 in 0e5dedbc9e

110+    QList<Unit> unitlist;
111 };
112 typedef BitcoinUnits::Unit BitcoinUnit;
113 
114+QDataStream& operator<<(QDataStream& out, const BitcoinUnit& unit);
115+QDataStream& operator>>(QDataStream& in, BitcoinUnit& unit);

jonatack commented at 10:00 pm on April 13, 2022:

75832fdc37ea3fe9cf515bd1946e220fe07a440b

0QDataStream& operator>>(const QDataStream& in, BitcoinUnit& unit);

hebasto commented at 9:46 am on April 15, 2022:

I think it is ok as is. See https://doc.qt.io/qt-5.15/qdatastream.html#reading-and-writing-other-qt-classes.

jonatack commented at 9:50 am on April 15, 2022:

Sure but there’s no reason for an in-param to be passed by reference instead of by reference to const, see https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#f16-for-in-parameters-pass-cheaply-copied-types-by-value-and-others-by-reference-to-const. No big deal either way but I think it’s clearer to show that it’s an in-param that won’t be mutated.

in src/qt/optionsmodel.cpp:78 in 0e5dedbc9e

76-        settings.setValue("nDisplayUnit", BitcoinUnits::BTC);
77-    nDisplayUnit = settings.value("nDisplayUnit").toInt();
78+    if (!settings.contains("DisplayBitcoinUnit")) {
79+        settings.setValue("DisplayBitcoinUnit", QVariant::fromValue(BitcoinUnit::BTC));
80+    }
81+    QVariant unit = settings.value("DisplayBitcoinUnit");

jonatack commented at 10:01 pm on April 13, 2022:

75832fdc37ea3fe9cf515bd1946e220fe07a440b this is just memoizing the value, so why not

0    const QVariant& unit = settings.value("DisplayBitcoinUnit");

jonatack commented at 10:48 pm on April 13, 2022: contributor

WIP review, looked at the first commit 75832fdc37ea3fe9cf515bd1946e220fe07a440b rebased to current master, noting my initial thoughts/questions here so far, continuing tomorrow

jonatack commented at 9:50 am on April 14, 2022: contributor

ACK 0e5dedbc9eb54105ab9b0c4ce1f57afa55bcb5b6, review and debug build of each commit after rebase on current master, lightly tested running the GUI, changing units a few times, and verifying persistence after restarting

Happy to re-ACK if update for the comments above.

laanwj commented at 5:22 pm on April 14, 2022: member

Concept ACK

hebasto merged this on Apr 15, 2022

hebasto closed this on Apr 15, 2022

sidhujag referenced this in commit a85635f459 on Apr 18, 2022

bitcoin-core locked this on Apr 15, 2023

refactor: Make BitcoinUnits::Unit a scoped enum #556

Conflicts