refactor: Make BitcoinUnits::Unit a scoped enum by hebasto · Pull Request #60 · bitcoin-core/gui

hebasto · 2020-08-14T10:52:41Z

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.

hebasto · 2020-08-14T10:57:01Z

bitcoin/bitcoin#17877 has Concept ACKs from:

@promag @jonasschnelli Mind reviewing this PR?

hebasto · 2020-08-26T17:54:10Z

Rebased 243205f -> b109ae6 (pr60.01 -> pr60.02) due to the conflict with #35.

Sjors · 2020-08-28T12:26:21Z

tACK b109ae6

I just noticed one practical downside of this separate repo; having to add another remote: git remote add hebasto-gui git@github.com:hebasto/gui.git

promag · 2020-11-20T09:37:29Z

@Sjors I only have git@github.com:bitcoin-core/gui.git and my gui fork.

maflcko · 2020-11-20T09:56:06Z

I manually call git fetch https://github.com/hebasto/gui/ 200814-unit, which works without a remote

elichai · 2020-11-22T10:07:49Z

FWIW, I'm like @promag and using @jonatack's scripts to fetch all PRs:

[remote "upstream"]
        fetch = +refs/pull/*/head:refs/remotes/upstream/pr/*

then you can just git checkout upstream/pr/60.

hebasto · 2021-01-21T20:35:13Z

Rebased b109ae6 -> 7e636c7 (pr60.02 -> pr60.03) due to the conflict with #176.

jonatack · 2021-03-26T15:13:54Z

Concept ACK

promag

Code review ACK 7e636c7 with some comments.

Will test and play around.

src/qt/bitcoinunits.cpp

promag · 2021-04-19T20:22:23Z

src/qt/optionsmodel.cpp


    // Display
-    if (!settings.contains("nDisplayUnit"))
-        settings.setValue("nDisplayUnit", BitcoinUnits::BTC);


77edf1b

Maybe it should migrate the old setting key if it exists?

Will it be dead code after a while?

And? For instance bitcoin/bitcoin#20966 also tries to use old data.

@promag
I was thinking about your suggestion, and I failed to find a safe and short (about 1..2 lines of code) solution.
OTOH, adjusting this option requires only two clicks.

Therefore, I'm going to leave it as is, leaving a room for improvements in follow ups.

src/qt/optionsmodel.cpp

hebasto · 2021-04-20T19:47:02Z

Updated 7e636c7 -> cb08a1c (pr60.03 -> pr60.04, diff):

addressed most of @promag's comments

hebasto · 2021-05-02T13:32:56Z

Updated cb08a1c -> 68baad3 (pr60.04 -> pr60.05):

rebased due to the conflict with Revamp context menus #263
option renamed s/display_unit/DisplayBitcoinUnit/ to be in line with other recently added options, see doc: Naming convention for new QSettings setting names #295

This change improves type safety.

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

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

hebasto · 2021-05-29T12:47:13Z

Rebased 68baad3 -> 8eb1a34 (pr60.05 -> pr60.06) due to the conflict with #275.

jb55 · 2021-06-22T14:33:44Z

Code Review ACK 8eb1a34

looks like a reasonable cleanup

note: I found this PR with a git script that emails you when someone touches a part of the codebase you've previously touched (via git-contacts and git-blame). I recommend using this trick to find potential reviewers: 😉

(for commit in $(git log --format=%H origin/master..FEATUREBRANCH); do git-contacts ${commit}^-; done) | sort | uniq -c | sort -nr
      5 Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
      4 Wladimir J. van der Laan <laanwj@gmail.com>
      2 William Casarin <jb55@jb55.com>
      2 Russell Yanofsky <russ@yanofsky.org>
      1 João Barbosa <joao.paulo.barbosa@gmail.com>
      1 GreatSock <greatsock@protonmail.com>

DrahtBot · 2021-10-05T08:09:18Z

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

jarolrod · 2021-11-02T05:10:16Z

cc @hebasto, a rebase is needed :)

DrahtBot · 2022-02-22T12:09:32Z

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

jb55 · 2022-02-22T17:31:31Z

I have a rebased version here: master...jb55:units-scoped-enum

I can submit a PR if you want me to take this over @hebasto

hebasto · 2022-02-22T17:37:23Z

I have a rebased version here: master...jb55:units-scoped-enum

I can submit a PR if you want me to take this over @hebasto

@jb55

Thank you! I'll appreciate if you do that.

hebasto · 2022-02-22T17:46:20Z

Closing in favor of #556.

0e5dedb qt/wallettests: sort includes (William Casarin) 0554251 qt: Skip displayUnitChanged signal if unit is not actually changed (Hennadii Stepanov) ffbc2fe qt, refactor: Remove default cases for scoped enum (Hennadii Stepanov) 152d5ba qt, refactor: Remove BitcoinUnits::valid function (Hennadii Stepanov) aa23960 qt, refactor: Make BitcoinUnits::Unit a scoped enum (Hennadii Stepanov) 75832fd qt: Use QVariant instead of int for BitcoinUnit in QSettings (Hennadii Stepanov) Pull request description: This is a rebased version of #60 Since Qt 5.5 there are [means](https://doc.qt.io/qt-5/qobject.html#Q_ENUM) to register an enum type with the meta-object system (such enum still lacks an ability to interact with [QSettings::setValue()](https://doc.qt.io/qt-5/qsettings.html#setValue) and [QSettings::value()](https://doc.qt.io/qt-5/qsettings.html#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. ACKs for top commit: jonatack: ACK 0e5dedb, 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 promag: Code review ACK 0e5dedb Tree-SHA512: 39ec0d7e4f0b9b25be287888121a8db6b282339674e37ec3a3554da63a9e22d6fe079e8310ca289b2a0356a19b3c7e55afa17d09dd34e0f222177f603bb053a3

hebasto mentioned this pull request Aug 14, 2020

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

Closed

DrahtBot added the Needs rebase label Aug 26, 2020

maflcko added the Refactoring label Aug 26, 2020

hebasto force-pushed the 200814-unit branch from 243205f to b109ae6 Compare August 26, 2020 17:45

DrahtBot removed the Needs rebase label Aug 26, 2020

jonasschnelli added this to the 0.22.0 milestone Nov 11, 2020

DrahtBot added the Needs rebase label Jan 21, 2021

hebasto force-pushed the 200814-unit branch from b109ae6 to 7e636c7 Compare January 21, 2021 20:34

DrahtBot removed the Needs rebase label Jan 21, 2021

promag reviewed Apr 19, 2021

View reviewed changes

hebasto force-pushed the 200814-unit branch from 7e636c7 to cb08a1c Compare April 20, 2021 19:42

DrahtBot added the Needs rebase label Apr 20, 2021

hebasto mentioned this pull request Apr 25, 2021

doc: Naming convention for new QSettings setting names #295

Open

hebasto force-pushed the 200814-unit branch from cb08a1c to 68baad3 Compare May 2, 2021 13:27

DrahtBot removed the Needs rebase label May 2, 2021

DrahtBot added the Needs rebase label May 28, 2021

hebasto added 4 commits May 29, 2021 15:43

qt: Use QVariant instead of int for BitcoinUnit in QSettings

f07c8c5

This change improves type safety.

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

a11cfe9

qt, refactor: Remove BitcoinUnits::valid function

7a82cdc

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

qt, refactor: Remove default cases for scoped enum

16e7d3f

qt: Skip displayUnitChanged signal if unit is not actually changed

8eb1a34

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

hebasto force-pushed the 200814-unit branch from 68baad3 to 8eb1a34 Compare May 29, 2021 12:44

DrahtBot removed the Needs rebase label May 29, 2021

hebasto removed this from the 22.0 milestone Jun 14, 2021

DrahtBot added the Needs rebase label Oct 5, 2021

jb55 mentioned this pull request Feb 22, 2022

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

Merged

hebasto closed this Feb 22, 2022

bitcoin-core locked and limited conversation to collaborators Feb 22, 2023

Comments

Conversation

hebasto commented Aug 14, 2020

Uh oh!

hebasto commented Aug 14, 2020

Uh oh!

hebasto commented Aug 26, 2020

Uh oh!

Sjors commented Aug 28, 2020

Uh oh!

promag commented Nov 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Nov 20, 2020

Uh oh!

elichai commented Nov 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Jan 21, 2021

Uh oh!

jonatack commented Mar 26, 2021

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

promag Apr 19, 2021

Choose a reason for hiding this comment

Uh oh!

hebasto Apr 20, 2021

Choose a reason for hiding this comment

Uh oh!

promag Apr 20, 2021

Choose a reason for hiding this comment

Uh oh!

hebasto May 2, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto commented Apr 20, 2021

Uh oh!

hebasto commented May 2, 2021

Uh oh!

hebasto commented May 29, 2021

Uh oh!

jb55 commented Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Oct 5, 2021

Uh oh!

jarolrod commented Nov 2, 2021

Uh oh!

DrahtBot commented Feb 22, 2022

Uh oh!

jb55 commented Feb 22, 2022

Uh oh!

hebasto commented Feb 22, 2022

Uh oh!

hebasto commented Feb 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

promag commented Nov 20, 2020 •

edited

Loading

elichai commented Nov 22, 2020 •

edited

Loading

jb55 commented Jun 22, 2021 •

edited

Loading