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.

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

wallet: Avoid translating RPC errors #18699

Merged
merged 4 commits into from
May 4, 2020

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Apr 18, 2020

Common errors and warnings should be translated when displayed in the
GUI, but not translated when displayed elsewhere. The wallet method
CreateWalletFromFile does not know its caller, so this commit changes it
to return a bilingual_str to the caller.

Fixes #17072

@maflcko
Copy link
Member Author

maflcko commented Apr 18, 2020

Can be tested with:

XDG_SESSION_TYPE=""  QT_QPA_PLATFORM=minimal LC_ALL=de_DE.UTF-8 BITCOIND=bitcoin-qt ./test/functional/wallet_basic.py 

The gui popups should be translated, but the json rpc errors should not.

@hebasto
Copy link
Member

hebasto commented Apr 18, 2020

Concept ACK.

@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 18, 2020

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.

@maflcko
Copy link
Member Author

maflcko commented Apr 28, 2020

Ok, I marked everything that wasn't translated before as Untranslated(). Should be easier to review now.

MarcoFalke added 4 commits May 1, 2020 07:39
Common errors and warnings should be translated when displayed in the
GUI, but not translated when displayed elsewhere. The wallet method
CreateWalletFromFile does not know its caller, so this commit changes it
to return a bilingual_str to the caller.
Also, mark feebumper bilingual_str as Untranslated

They are technical and have previously not been translated either.
It is questionable whether they can even appear in the GUI.
If the potential translation strings are translated in the future,
trailing whitespace is going to make translation effort harder.
@maflcko
Copy link
Member Author

maflcko commented May 1, 2020

Rebased

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 fa2cce4

Ok, I marked everything that wasn't translated before as Untranslated().

Verified by code reviewing that no new translations added.

@maflcko
Copy link
Member Author

maflcko commented May 2, 2020

Can also be verified with (cd src && make translate) && git diff before and after (diff of diff should be empty). :)

@laanwj
Copy link
Member

laanwj commented May 4, 2020

ACK fa2cce4, checked that no new translation messages are added compared to master.

@laanwj laanwj merged commit 23c926d into bitcoin:master May 4, 2020
@maflcko maflcko deleted the 1908-walletErrorSegfault branch May 4, 2020 14:39
Comment on lines +28 to +33
/** Mark a bilingual_str as untranslated */
inline static bilingual_str Untranslated(std::string original) { return {original, original}; }
/** Unary operator to return the original */
inline static std::string OpOriginal(const bilingual_str& b) { return b.original; }
/** Unary operator to return the translation */
inline static std::string OpTranslated(const bilingual_str& b) { return b.translated; }
Copy link
Member

Choose a reason for hiding this comment

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

@MarcoFalke What is the rationale to declare these functions with static?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is a leftover. They only need to be inline to avoid linker errors. static might have no effect.

Copy link
Member Author

Choose a reason for hiding this comment

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

Feel free to remove in your pull

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 4, 2020
fa2cce4 wallet: Remove trailing whitespace from potential translation strings (MarcoFalke)
fa59cc1 wallet: Report full error message in wallettool (MarcoFalke)
fae7776 wallet: Avoid translating RPC errors when creating txs (MarcoFalke)
fae51a5 wallet: Avoid translating RPC errors when loading wallets (MarcoFalke)

Pull request description:

  Common errors and warnings should be translated when displayed in the
  GUI, but not translated when displayed elsewhere. The wallet method
  `CreateWalletFromFile` does not know its caller, so this commit changes it
  to return a `bilingual_str` to the caller.

  Fixes bitcoin#17072

ACKs for top commit:
  laanwj:
    ACK fa2cce4, checked that no new translation messages are added compared to master.
  hebasto:
    ACK fa2cce4

Tree-SHA512: c6a943ae9c3689ea3c48c20d26de6e4970de0257a1f1eec57a2bded67a4af9dcc5c45b2d64659d6fb4c4bc4d8103e28483ea3d14bb850df8db0ff9e8e5c77ee2
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 1, 2020
Summary:
Common errors and warnings should be translated when displayed in the
GUI, but not translated when displayed elsewhere. The wallet method
CreateWalletFromFile does not know its caller, so this commit changes it
to return a bilingual_str to the caller.

This is a partial backport of Core [[bitcoin/bitcoin#18699 | PR18699]] : bitcoin/bitcoin@fae51a5

Depends on D7699

Test Plan:
  ninja all check-all

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D7701
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 1, 2020
Summary:
Also, mark feebumper bilingual_str as Untranslated

They are technical and have previously not been translated either.
It is questionable whether they can even appear in the GUI.

This is a partial backport of Cor [[bitcoin/bitcoin#18699 | PR18699]] : bitcoin/bitcoin@fae7776

Test Plan:
  ninja all check

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D7703
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 1, 2020
Summary:
this is a partial backport of Core [[bitcoin/bitcoin#18699 | PR18699]]

Depends on D7703 and D7706

Test Plan:
  ninja all check-all

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D7707
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 1, 2020
Summary:
If the potential translation strings are translated in the future,
trailing whitespace is going to make translation effort harder.

This is the final part of Core [[bitcoin/bitcoin#18699 | PR18699]] : bitcoin/bitcoin@fa2cce4

Depends on D7707

Test Plan:
  ninja all check-all

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D7708
@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.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gui: RPC responses should not be translated
6 participants