Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Dec 28, 2020

Currently, strings are stored for what are actually integral (strong) enum types. This is fine, because the strings are only used as-is for the debug log and RPC. However, it complicates using them in the GUI. User facing strings in the GUI should be translated and only string literals can be picked up for translation, not runtime std::strings.

Fix that by removing the std::string members and replace them by strong enum integral types.

@maflcko
Copy link
Member Author

maflcko commented Dec 28, 2020

Picked up from #20778 and #20779 . Credits go to @jonatack

@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 28, 2020

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

Conflicts

No conflicts as of last run.

@practicalswift
Copy link
Contributor

Concept ACK

@DrahtBot DrahtBot mentioned this pull request Dec 28, 2020
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.

ACK 7f126f2da6.

@maflcko maflcko force-pushed the 2012-net branch 4 times, most recently from 7f126f2 to 6e7e053 Compare December 29, 2020 08:05
@ajtowns
Copy link
Contributor

ajtowns commented Dec 30, 2020

Concept ACK.

@SoraKohaku
Copy link

Rebased ok.

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.

Code review ACK 6e7e053.

@maflcko
Copy link
Member Author

maflcko commented Jan 7, 2021

Rebased due to trivial conflict in adjacent line

@bitcoin bitcoin deleted a comment from sheffine Jan 7, 2021
Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

Code review ACK faecb74

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

LGTM
Code review ACK faecb74 🌲

@maflcko maflcko merged commit 9158d6f into bitcoin:master Jan 8, 2021
@maflcko maflcko deleted the 2012-net branch January 8, 2021 14:17
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 8, 2021
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jan 28, 2021
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants