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

hebasto commented at 9:01 pm on January 5, 2020: member

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::SeparatorStyle a scoped enum (done in https://github.com/bitcoin-core/gui/pull/3)
makes BitcoinUnits::Unit a scoped enum (typedef BitcoinUnits::Unit BitcoinUnit;)
cleans BitcoinUnits class and its functions
does not change behavior

fanquake added the label GUI on Jan 5, 2020

fanquake added the label Refactoring on Jan 5, 2020

in src/qt/bitcoinunits.cpp:35 in cf9507442f outdated

47+    case Unit::BTC: return QString("BTC");
48+    case Unit::mBTC: return QString("mBTC");
49+    case Unit::uBTC: return QString::fromUtf8("µBTC (bits)");
50+    case Unit::SAT: return QString("Satoshi (sat)");
51+    } // no default case, so the compiler can warn about missing cases
52+    assert(false);

promag commented at 9:23 pm on January 5, 2020:

In Qt there’s Q_UNREACHABLE().

hebasto commented at 10:12 pm on January 5, 2020:

Correct. But I’d prefer to stick to our Developer Notes ;) When C++ will provide the standard way to handle such cases, it will be easier to implement the appropriate change.

Or did I miss something important why Q_UNREACHABLE() should be preferable in Qt code?

promag commented at 10:21 pm on January 5, 2020:

No preference!

promag commented at 9:29 pm on January 5, 2020: member

Concept ACK, and change LGTM.

practicalswift commented at 11:02 pm on January 5, 2020: contributor

Concept ACK: structured is better than unstructured :)

DrahtBot commented at 3:03 am on January 6, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15936 (Unify bitcoin-qt and bitcoind persistent settings by ryanofsky)

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.

elichai commented at 4:02 pm on January 6, 2020: contributor

Concept ACK. will review later this week

jonasschnelli commented at 5:25 am on January 9, 2020: contributor

Concept ACK. Flew over the code (cf9507442fbec9252f557ed0de8237b631f89d39),… looks good. Needs testing.

Empact commented at 7:37 pm on January 13, 2020: member

Concept ACK

DrahtBot added the label Needs rebase on Jan 27, 2020

hebasto force-pushed on Jan 27, 2020

hebasto commented at 5:58 pm on January 27, 2020: member

Rebased after #17453 has been merged.

DrahtBot removed the label Needs rebase on Jan 27, 2020

DrahtBot added the label Needs rebase on Mar 23, 2020

hebasto force-pushed on Mar 24, 2020

hebasto commented at 4:35 pm on March 24, 2020: member

Rebased 22be3d60cab774ffd2f8a287f522176b5a0abdfe -> 4ead3f47c35b40382007fd0eee2b01c29581d1e0 due to the conflict with #18278.

DrahtBot removed the label Needs rebase on Mar 24, 2020

in src/qt/transactiontablemodel.h:104 in 1b46d50d4f outdated

100@@ -101,7 +101,7 @@ class TransactionTableModel : public QAbstractTableModel
101     QString formatTxDate(const TransactionRecord *wtx) const;
102     QString formatTxType(const TransactionRecord *wtx) const;
103     QString formatTxToAddress(const TransactionRecord *wtx, bool tooltip) const;
104-    QString formatTxAmount(const TransactionRecord *wtx, bool showUnconfirmed=true, BitcoinUnits::SeparatorStyle separators=BitcoinUnits::separatorStandard) const;
105+    QString formatTxAmount(const TransactionRecord* wtx, bool showUnconfirmed = true, BitcoinUnits::SeparatorStyle separators = BitcoinUnits::SeparatorStyle::STANDARD) const;

jb55 commented at 6:35 pm on April 2, 2020:

was const TransactionRecord *wtx -> const TransactionRecord* wtx intentional? looks inconsistent wrt surrounding code

hebasto commented at 6:40 pm on April 2, 2020:

clang-format-diff.py was applied.

DrahtBot added the label Needs rebase on Apr 10, 2020

hebasto force-pushed on Apr 11, 2020

hebasto commented at 12:52 pm on April 11, 2020: member

Rebased 4ead3f47c35b40382007fd0eee2b01c29581d1e0 -> cca2ebdaf7f9af26a3381273f79efd9fb95ed1a9 (pr17877.03 -> pr17877.04) due to the conflict with #17905.

DrahtBot removed the label Needs rebase on Apr 11, 2020

DrahtBot added the label Needs rebase on Apr 27, 2020

hebasto force-pushed on Apr 29, 2020

hebasto commented at 5:44 pm on April 29, 2020: member

Rebased cca2ebdaf7f9af26a3381273f79efd9fb95ed1a9 -> a2edcd60a614053ec642f21cedf4858b650c726b (pr17877.04 -> pr17877.05) due to the conflict with #16528.

DrahtBot removed the label Needs rebase on Apr 29, 2020

DrahtBot added the label Needs rebase on May 29, 2020

hebasto force-pushed on May 29, 2020

hebasto commented at 10:35 am on May 29, 2020: member

Rebased a2edcd60a614053ec642f21cedf4858b650c726b -> 61c84eab0c5451b2a19b51047c493b45d4562eff (pr17877.05 -> pr17877.06) due to the conflict with #16432.

DrahtBot removed the label Needs rebase on May 29, 2020

MarcoFalke commented at 4:10 pm on May 30, 2020: member

Can’t this be done with a scripted diff like in fa5997bd6f?

hebasto force-pushed on May 31, 2020

hebasto commented at 6:19 am on May 31, 2020: member

Updated 61c84eab0c5451b2a19b51047c493b45d4562eff -> 53bf585eb918f1674febcb0d2b7ece635d2cc767 (pr17877.06 -> pr17877.07, diff):

addressed @MarcoFalke’s comment:

Can’t this be done with a scripted diff like in fa5997b?

The SeparatorStyle enum has been refactored in a scripted diff. Skipped for the Unit enum as that changes touch a way different things.

in src/qt/bitcoinamountfield.cpp:329 in 10082d283d outdated

325@@ -326,14 +326,14 @@ void BitcoinAmountField::unitChanged(int idx)
326     unit->setToolTip(unit->itemData(idx, Qt::ToolTipRole).toString());
327 
328     // Determine new unit ID
329-    int newUnit = unit->itemData(idx, BitcoinUnits::UnitRole).toInt();
330+    BitcoinUnit newUnit = unit->itemData(idx, BitcoinUnits::UnitRole).value<BitcoinUnit>();

MarcoFalke commented at 12:13 pm on May 31, 2020:

Why is this safe to do in qt? Any integer can be converted to BitcoinUnit, at least in C++.

hebasto commented at 4:23 pm on May 31, 2020:

I believe, no int to BitcoinUnit conversion here. QComboBox::itemData() does a kind of mapping from int index to QVariant.

Anyway, going to make this code more safer.

hebasto commented at 7:14 pm on May 31, 2020:

Updated.

in src/qt/optionsmodel.cpp:74 in 10082d283d outdated

67@@ -68,9 +68,10 @@ void OptionsModel::Init(bool resetSettings)
68     fMinimizeOnClose = settings.value("fMinimizeOnClose").toBool();
69 
70     // Display
71+    // As BitcoinUnit does not provide its own stream operators, we cast it to int and back.
72     if (!settings.contains("nDisplayUnit"))
73-        settings.setValue("nDisplayUnit", BitcoinUnits::BTC);
74-    nDisplayUnit = settings.value("nDisplayUnit").toInt();
75+        settings.setValue("nDisplayUnit", static_cast<int>(BitcoinUnit::BTC));
76+    nDisplayUnit = static_cast<BitcoinUnit>(settings.value("nDisplayUnit").toInt());

MarcoFalke commented at 12:16 pm on May 31, 2020:

Same here

hebasto commented at 7:15 pm on May 31, 2020:

See #17877 (comment)

in src/qt/bitcoinunits.h:63 in 2d0c28d869 outdated

57@@ -59,8 +58,6 @@ class BitcoinUnits: public QAbstractListModel
58 
59     //! Get list of units, for drop-down box
60     static QList<Unit> availableUnits();
61-    //! Is unit ID valid?
62-    static bool valid(Unit unit);

MarcoFalke commented at 12:17 pm on May 31, 2020:

Same

hebasto commented at 7:15 pm on May 31, 2020:

See #17877 (comment)

in src/qt/bitcoinunits.cpp:49 in 53bf585eb9 outdated

31+    switch (unit) {
32     case Unit::BTC: return QString("BTC");
33     case Unit::mBTC: return QString("mBTC");
34     case Unit::uBTC: return QString::fromUtf8("µBTC (bits)");
35     case Unit::SAT: return QString("Satoshi (sat)");
36-    default: return QString("???");

MarcoFalke commented at 12:18 pm on May 31, 2020:

Same

hebasto commented at 7:15 pm on May 31, 2020:

See #17877 (comment)

MarcoFalke commented at 12:18 pm on May 31, 2020: member

ACK f8cd12dfd39104d875a16ea0226b4be1450e77f4

hebasto force-pushed on May 31, 2020

hebasto commented at 7:08 pm on May 31, 2020: member

Updated 53bf585eb918f1674febcb0d2b7ece635d2cc767 -> 25df40fcadaf5b284e94765e25213eb224b33a25 (pr17877.07 -> pr17877.08, diff):

addressed @MarcoFalke’s comment:

Why is this safe to do in qt? Any integer can be converted to BitcoinUnit, at least in C++.

For more type safety I suggest in a follow-up to implement the operator<<() and operator>>() for BitcoinUnit type, and use QVariant instances to set and read values from the QSettings object directly.

MarcoFalke commented at 9:36 pm on May 31, 2020: member

Still ACK on the first commit, but I don’t feel comfortable about the rest. Previously a corrupt settings file (or a file from a previous version) would recover with a gui glitch. Now it crashes the node.

hebasto force-pushed on Jun 1, 2020

hebasto commented at 1:44 pm on June 1, 2020: member

Updated 25df40fcadaf5b284e94765e25213eb224b33a25 -> 949669431de3ecc41d16e58d323dad741588bf79 (pr17877.08 -> pr17877.09, diff):

added “qt: Use QVariant instead of int for BitcoinUnit in QSettings” commit to address @MarcoFalke’s comment:

Still ACK on the first commit, but I don’t feel comfortable about the rest. Previously a corrupt settings file (or a file from a previous version) would recover with a gui glitch.

Now it crashes the node.

Mind providing the Bitcoin-Qt.conf file and steps to reproduce a crash?

MarcoFalke commented at 2:04 pm on June 1, 2020: member

Mind providing the Bitcoin-Qt.conf file and steps to reproduce a crash?

Sorry I suspected that this crashes the node based on reading the code, I haven’t actually tested or verified that.

MarcoFalke commented at 2:07 pm on June 1, 2020: member

And with the new code, FromQuint8 seems to deal with the issue.

hebasto commented at 2:09 pm on June 1, 2020: member

This variant is backward compatible as it uses a new key “display_unit” in QSettings file.

DrahtBot added the label Needs rebase on Jun 2, 2020

hebasto force-pushed on Jun 4, 2020

hebasto commented at 7:23 am on June 4, 2020: member

Rebased 949669431de3ecc41d16e58d323dad741588bf79 -> cbd498e593498d3b28c0c72032f1e62a2f9ef2df (pr17877.09 -> pr17877.10) due to the conflict with #19104.

DrahtBot removed the label Needs rebase on Jun 4, 2020

MarcoFalke commented at 1:07 pm on June 16, 2020: member

Maybe split out the scripted-diff into a separate pull request: https://github.com/bitcoin-core/gui/pulls

It seems that it can be reviewed by anyone familiar with C++. As opposed to the latter commits, which require specific gui knowledge.

hebasto commented at 12:49 pm on June 18, 2020: member

@MarcoFalke

Maybe split out the scripted-diff into a separate pull request: https://github.com/bitcoin-core/gui/pulls

It seems that it can be reviewed by anyone familiar with C++.

Done: https://github.com/bitcoin-core/gui/pull/3

As opposed to the latter commits, which require specific gui knowledge.

Should they be also moved to https://github.com/bitcoin-core/gui ?

MarcoFalke referenced this in commit 0865a8881d on Jun 18, 2020

hebasto force-pushed on Jun 18, 2020

hebasto commented at 5:24 pm on June 18, 2020: member

Rebased cbd498e593498d3b28c0c72032f1e62a2f9ef2df -> 1a348c8ceddcb38ba33caf3ceade419c7590c879 (pr17877.10 -> pr17877.11) due to the merging of https://github.com/bitcoin-core/gui/pull/3.

DrahtBot added the label Needs rebase on Jul 9, 2020

hebasto force-pushed on Jul 10, 2020

hebasto commented at 8:00 am on July 10, 2020: member

Rebased 1a348c8ceddcb38ba33caf3ceade419c7590c879 -> 72819bd15cb8c77fc6e1162c046060522b3ce6e5 (pr17877.11 -> pr17877.12) due to the conflict with #19314.

hebasto force-pushed on Jul 10, 2020

hebasto commented at 9:11 am on July 10, 2020: member

Updated 72819bd15cb8c77fc6e1162c046060522b3ce6e5 -> 1575ffbbe42dd6c78b131dffe8cb00f636141cc1 (pr17877.12 -> pr17877.13, diff):

fixed the conflict with #18027

DrahtBot removed the label Needs rebase on Jul 10, 2020

hebasto renamed this:
~~qt, refactor: Make enums in BitcoinUnits class scoped~~
qt, refactor: Make BitcoinUnits::Unit a scoped enum
on Jul 15, 2020

DrahtBot added the label Needs rebase on Aug 13, 2020

qt: Use QVariant instead of int for BitcoinUnit in QSettings

This change improves type safety.

5372deb4a7

refactor: Make BitcoinUnits::Unit a scoped enum 8d9817b114

refactor: Remove BitcoinUnits::valid() function

Since BitcoinUnits::Unit became a scoped enum, BitcoinUnits::valid()
gets needless.

86357f1851

refactor: Remove default cases for scoped enum 243205f85b

hebasto force-pushed on Aug 13, 2020

hebasto commented at 1:51 pm on August 13, 2020: member

Rebased 1575ffbbe42dd6c78b131dffe8cb00f636141cc1 -> 243205f85b8b10e4f43865557cdd8ea3aa71c380 (pr17877.13 -> pr17877.14) due to the conflict with #19011.

DrahtBot removed the label Needs rebase on Aug 13, 2020

fanquake commented at 6:47 am on August 14, 2020: member

Lots of Concept ACKs, but not really any review in 6 months. Lets try and kick-start this over on the GUI repo.

fanquake closed this on Aug 14, 2020

hebasto commented at 10:57 am on August 14, 2020: member

Moved to https://github.com/bitcoin-core/gui/pull/60.

hebasto deleted the branch on Aug 14, 2020

DrahtBot locked this on Feb 15, 2022

qt, refactor: Make BitcoinUnits::Unit a scoped enum #17877

Conflicts