Skip to content
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

qt, refactor: rpcconsole translatable string fixes and improvements #221

Merged
merged 2 commits into from
Mar 7, 2021
Merged

qt, refactor: rpcconsole translatable string fixes and improvements #221

merged 2 commits into from
Mar 7, 2021

Conversation

jonatack
Copy link
Member

@jonatack jonatack mentioned this pull request Feb 23, 2021
Merged
hebasto
hebasto reviewed Feb 23, 2021
View reviewed changes
Copy link
Member

@hebasto hebasto 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.

src/qt/rpcconsole.cpp Outdated Show resolved Hide resolved
src/qt/rpcconsole.cpp Outdated Show resolved Hide resolved
@jonatack jonatack force-pushed the qt-rpcconsole-translation-improvements branch from e1a10ff to 17edc9a Compare February 23, 2021 18:53
jonatack
jonatack commented Feb 23, 2021
View reviewed changes
src/qt/rpcconsole.h Outdated Show resolved Hide resolved
hebasto
hebasto approved these changes Feb 23, 2021
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK 17edc9a, tested on Linux Mint 20.1 (Qt 5.12.8).

src/qt/rpcconsole.h Outdated Show resolved Hide resolved
@jonatack jonatack force-pushed the qt-rpcconsole-translation-improvements branch from 17edc9a to 52d10ee Compare February 23, 2021 19:16
hebasto
hebasto approved these changes Feb 23, 2021
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

re-ACK 52d10ee

hebasto
hebasto reviewed Feb 24, 2021
View reviewed changes
src/qt/rpcconsole.h Outdated Show resolved Hide resolved
@jonatack jonatack force-pushed the qt-rpcconsole-translation-improvements branch from 52d10ee to 2e2fe50 Compare February 24, 2021 14:38
hebasto
hebasto approved these changes Feb 24, 2021
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

re-ACK 2e2fe50

@jonatack
Copy link
Member Author

Rebased following merge of #226, dropped "Never" from the struct as it is now only used once.

@jonatack jonatack force-pushed the qt-rpcconsole-translation-improvements branch from 2e2fe50 to 3d85e92 Compare February 25, 2021 14:50
hebasto
hebasto approved these changes Feb 25, 2021
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

re-ACK 3d85e92

Talkless
Talkless reviewed Feb 28, 2021
View reviewed changes
Copy link

@Talkless Talkless 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

src/qt/rpcconsole.cpp Outdated Show resolved Hide resolved
src/qt/rpcconsole.h
@@ -136,6 +136,11 @@ public Q_SLOTS:
void cmdRequest(const QString &command, const WalletModel* wallet_model);

private:
struct TranslatedStrings {
Copy link

Choose a reason for hiding this comment

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

Interesting technique. I guess translators will see TranslatedStrings as location, not rpcconsole.h? But I guess that doesn't matter, not much context needed?

Copy link

Choose a reason for hiding this comment

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

I've tried with some small test project, and lupdate/linguist uses parent class as context, so no issues here.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Talkless thank you for checking!

jonatack and others added 2 commits February 28, 2021 19:13
@jonatack
RPCConsole::updateDetailWidget: convert strings to translated strings
0f035c1 
and in the touched lines:

- replace 2 occurrences of `== ""` with `isEmpty()`

- replace an unneeded `+=` with `=`
@jonatack @hebasto
Hoist repeated translated strings to RPCConsole struct members
6242bee 
and add missing braces to the touched conditionals.

Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
@jonatack jonatack force-pushed the qt-rpcconsole-translation-improvements branch from 3d85e92 to 6242bee Compare February 28, 2021 18:19
@jonatack
Copy link
Member Author

Updated with suggestion from @Talkless (thanks!) to replace QLatin1String("/") with QLatin1Char('/')

hebasto
hebasto approved these changes Feb 28, 2021
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

re-ACK 6242bee

Talkless
Talkless approved these changes Mar 1, 2021
View reviewed changes
Copy link

@Talkless Talkless left a comment

Choose a reason for hiding this comment

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

tACK 6242bee, tested on Debian Sid with Qt 5.15.2. I see "Ban for.." translated to my native language as before, "To/From/Yes/No" are not but that's expected, as .ts files are not updated.

jarolrod
jarolrod reviewed Mar 7, 2021
View reviewed changes
Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

ACK 6242bee

Good idea! These strings are/will be super common. This allows them to be used in other places when appropriate.

@maflcko maflcko merged commit 1a4a930 into bitcoin-core:master Mar 7, 2021
@jonatack jonatack deleted the qt-rpcconsole-translation-improvements branch March 7, 2021 18:04
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 8, 2021
Merge bitcoin-core/gui#221: qt, refactor: rpcconsole translatable str…
e23a0a1
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 28, 2022
@apoelstra
Merge 1a4a930 into merged_master (Bitcoin PR bitcoin-core/gui#221)
d09330b
@bitcoin-core bitcoin-core 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.
Reviewers

@jarolrod jarolrod jarolrod left review comments

@Talkless Talkless Talkless approved these changes

@hebasto hebasto hebasto approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jonatack @Talkless @jarolrod @hebasto @maflcko