Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
hebasto wants to merge 4 commits into from
Closed

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

hebasto wants to merge 4 commits into from

Conversation

@hebasto
Copy link
Member

@hebasto hebasto commented Jan 5, 2020 •
edited
Loading

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:

promag
promag reviewed Jan 5, 2020
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK, and change LGTM.

@practicalswift
Copy link
Contributor

practicalswift commented Jan 5, 2020

Concept ACK: structured is better than unstructured :)

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 6, 2020 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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
Copy link
Contributor

elichai commented Jan 6, 2020

Concept ACK. will review later this week

@jonasschnelli
Copy link
Contributor

jonasschnelli commented Jan 9, 2020

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

@Empact
Copy link
Contributor

Empact commented Jan 13, 2020

Concept ACK

@hebasto hebasto force-pushed the 20200105-scoped-enums branch from cf95074 to 22be3d6 Compare January 27, 2020 17:57
@hebasto
Copy link
Member Author

hebasto commented Jan 27, 2020

Rebased after #17453 has been merged.

This was referenced Feb 11, 2020
Merged
Merged
Merged
Merged
Merged
This was referenced Feb 21, 2020
Merged
Closed
Draft
This was referenced Mar 2, 2020
Closed
Merged
@hebasto hebasto force-pushed the 20200105-scoped-enums branch from 22be3d6 to 4ead3f4 Compare March 24, 2020 16:33
@hebasto
Copy link
Member Author

hebasto commented Mar 24, 2020

Rebased 22be3d6 -> 4ead3f4 due to the conflict with #18278.

jb55
jb55 reviewed Apr 2, 2020
View reviewed changes
@hebasto hebasto force-pushed the 20200105-scoped-enums branch from 4ead3f4 to cca2ebd Compare April 11, 2020 12:48
@hebasto
Copy link
Member Author

hebasto commented Apr 11, 2020

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

@DrahtBot DrahtBot mentioned this pull request Jun 5, 2020
Merged
@maflcko
Copy link
Member

maflcko commented Jun 16, 2020

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.

@DrahtBot DrahtBot mentioned this pull request Jun 17, 2020
Merged
@hebasto hebasto mentioned this pull request Jun 18, 2020
Merged
@hebasto
Copy link
Member Author

hebasto commented Jun 18, 2020

@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: bitcoin-core/gui#3

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

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

maflcko pushed a commit to bitcoin-core/gui that referenced this pull request Jun 18, 2020
Merge #3: scripted-diff: Make SeparatorStyle a scoped enum
0865a88 
25f3554 scripted-diff: Make SeparatorStyle a scoped enum (Hennadii Stepanov)

Pull request description:

  This PR is [split](bitcoin/bitcoin#17877 (comment)) from bitcoin/bitcoin#17877 and makes `BitcoinUnits::SeparatorStyle` a scoped enum.

ACKs for top commit:
  MarcoFalke:
    review ACK 25f3554 🚐

Tree-SHA512: 578f1340a476cf79faa109a83815d3c75e26d9c18873e653d7624b52428ccb2677293116db0a60ae14c949d63b64988fc5a39c7184c2352b87b00e8ddaaaf474
@hebasto hebasto force-pushed the 20200105-scoped-enums branch from cbd498e to 1a348c8 Compare June 18, 2020 17:21
@hebasto
Copy link
Member Author

hebasto commented Jun 18, 2020

Rebased cbd498e -> 1a348c8 (pr17877.10 -> pr17877.11) due to the merging of bitcoin-core/gui#3.

@hebasto hebasto force-pushed the 20200105-scoped-enums branch from 1a348c8 to 72819bd Compare July 10, 2020 07:58
@hebasto
Copy link
Member Author

hebasto commented Jul 10, 2020

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

@hebasto hebasto force-pushed the 20200105-scoped-enums branch from 72819bd to 1575ffb Compare July 10, 2020 09:09
@hebasto
Copy link
Member Author

hebasto commented Jul 10, 2020

Updated 72819bd -> 1575ffb (pr17877.12 -> pr17877.13, diff):

@DrahtBot DrahtBot mentioned this pull request Jul 10, 2020
Draft
@hebasto hebasto changed the title qt, refactor: Make enums in BitcoinUnits class scoped qt, refactor: Make BitcoinUnits::Unit a scoped enum Jul 15, 2020
@hebasto hebasto force-pushed the 20200105-scoped-enums branch from 1575ffb to 243205f Compare August 13, 2020 13:49
@hebasto
Copy link
Member Author

hebasto commented Aug 13, 2020

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

@fanquake
Copy link
Member

fanquake commented Aug 14, 2020

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

@fanquake fanquake closed this Aug 14, 2020
@hebasto hebasto mentioned this pull request Aug 14, 2020
Closed
@hebasto
Copy link
Member Author

hebasto commented Aug 14, 2020

Moved to bitcoin-core/gui#60.

@hebasto hebasto deleted the 20200105-scoped-enums branch August 14, 2020 10:58
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@jb55 jb55 jb55 left review comments

@promag promag promag left review comments

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

GUI Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@hebasto @practicalswift @DrahtBot @elichai @jonasschnelli @Empact @maflcko @fanquake @jb55 @promag