Add bilingual_str type #16362

hebasto · 2019-07-09T18:01:18Z

This PR adds the bilingual_str struct and a strprintf overload:

Lines 1066 to 1067 in 0626b8c

    
           /** Format arguments and return the string or write to given std::ostream (see tinyformat::format doc for details) */ 
        
           #define strprintf tfm::format

Both new features allow bitcoin code to easily send dual translated and non-translated messages to the GUI and the logging framework.

This PR is only a refactoring (has been split off the #16224 (see: #16224)) and does not change behavior.

hebasto · 2019-07-09T18:02:53Z

GH does not render comment address correctly:
https://github.com/bitcoin/bitcoin/pull/16224/#issuecomment-509701396

maflcko · 2019-07-09T18:48:56Z

ACK 4e1eda7, the diff seems verbose but I think it makes sense
to be more explicit where translated strings are passed in.
Currently we have issues where translated strings end up in the debug log.

Also, this is a requirement for future fixes such as #16224

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 4e1eda717e, the diff seems verbose but I think it makes sense
to be more explicit where translated strings are passed in.
Currently we have issues where translated strings end up in the debug log.

Also, this is a requirement for future fixes such as #16224
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgAxgv7B8gVJW+NBhgvYFjV7GzTcEDz0+R5pW0pW+8JXt5bYm4l9YkmBZVq/kk+
Pf/Wr2Vv91OoFIAiAu8iObedp35PzybgeZ4Dc9HlidhOHZvqxOrjNf+lINNyh+ZZ
ibv0VVcNQTeffMf6+zQoHazuyJPtNarJHmfPQQPBuYPbniW8DpDcRtamMIBxg0sd
2FMLWtgt214boyf0aNUTmQUjxqwtafuqSpOri1VVMc458lIAKma8tdQUo9U/ryfP
+F8ev8SuK+ZJF3DRqah5mCO6+5Cd5vv0e/wsVemL6DBW11IuTOdW8WCFQoTIQINL
ayT1m4fbe9wdKh1r/j4Hla8qGjrtPSfgoEj+wfghACLP2MVZxZwqraLz2tH4NFQ4
8zVtjR7Ms1PVSSNFGEoEWU7ouYdX/Jgz/KDp/MxpeFMwzTNZkBWrP5An1AwuJyyt
5VR16+80UJfF5qnOXpO3tt7t7CQzB194ZBJVLneTVUye0Q/87xW8Y5MjCDn5QBVD
TtPRZurp
=g2TS
-----END PGP SIGNATURE-----

Timestamp of file with hash a12ab7776476c20805e88823e532aa26d34082ee0510d1f17344e7869974c62a -

DrahtBot · 2019-07-09T19:40:33Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16443 (refactor: have CCoins* data managed under CChainState by jamesob)
#16442 (Serve BIP 157 compact filters by jimpo)
#16426 (Reverse cs_main, cs_wallet lock order and reduce cs_main locking by ariard)
#16341 (WIP: Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101)
#16265 (Update check disk space in bitcoind by asood123)
#16248 (Make whitebind/whitelist permissions more flexible by NicolasDorier)
#16003 (init: an incorrect amount of file descriptors is requested, and a different amount is also asserted by tryphe)
#15946 (Allow maintaining the blockfilterindex when using prune by jonasschnelli)
#15861 (Restore warning for individual unknown version bits, as well as unknown version schemas by luke-jr)
#15761 (Replace -upgradewallet startup option with upgradewallet RPC by achow101)
#15606 ([experimental] UTXO snapshots by jamesob)
#15112 (build: Optionally enable -Wzero-as-null-pointer-constant by Empact)
#13751 (Utils and libraries: Drops the boost/algorithm/string/split.hpp dependency by l2a5b1)
#10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)
#8994 (Testchains: Introduce custom chain whose constructor... by jtimon)

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.

hebasto · 2019-07-10T15:15:01Z

Rebased.

hebasto · 2019-07-18T16:37:20Z

Rebased.

hebasto · 2019-07-23T17:19:04Z

Rebased.

ryanofsky

Thank for a making new PR to separate refactoring and behavior changes!

Conditional utACK 3b7348f if this PR is fixed to have a descriptive title and OP. For title I'd suggest, "Add bilingual_str class". In the description, I'd say something like, "This PR adds a bilingual_str class and a strprintf overload that allows bitcoin code to easily send dual translated and non-translated messages to the GUI and the logging framework. This PR is only a refactoring, and does not change behavior."

As I commented on the other PR, I think this PR and its followup would be simpler if the _ function signature was changed to return bilingual_str in the same commit that changed ThreadSafeMessageBox, InitError, InitWarning, and similar functions to accept bilingual_str arguments. This would avoid the churn of having to add .translated suffixes and then remove them later. It would mean replacing the scripted diff commit with a manual commit, but I think the manual commit should be smaller, and honestly I'd consider it an advantage not to have to decipher the sed / bash code anyway. But this is just my suggested approach, I'm also fine with the current approach.

DrahtBot · 2019-07-24T00:43:26Z

Needs rebase

hebasto · 2019-07-24T12:27:06Z

@ryanofsky Thank you for your review.

For title I'd suggest, "Add bilingual_str class". In the description, I'd say something like, "This PR adds a bilingual_str class and a strprintf overload that allows bitcoin code to easily send dual translated and non-translated messages to the GUI and the logging framework. This PR is only a refactoring, and does not change behavior."

Done.

... I'm also fine with the current approach.

Let it be ;)

ryanofsky

utACK 375d909. Only changes since last review are rebase and updating the PR title & description (thanks!). There currently a lint error making travis fail, but this looks good otherwise.

One thing is I would probably remove "gui:" prefix from PR title because that even though this is refactoring that doesn't change behavior and is meant to help the gui, the prefix could mislead reviewers to thinking the PR doesn't touch non-gui code.

This is a prerequisite for introducing bilingual error messages. Note: #includes are arranged by clang-format-diff.py script.

-BEGIN VERIFY SCRIPT- sed -i 's/inline std::string _(const char\* psz)/inline bilingual_str _(const char\* psz)/' src/util/translation.h sed -i 's/return G_TRANSLATION_FUN ? (G_TRANSLATION_FUN)(psz) : psz;/return bilingual_str{psz, G_TRANSLATION_FUN ? (G_TRANSLATION_FUN)(psz) : psz};/' src/util/translation.h sed -i 's/\b_("$[^"]\|\\"$*")/&.translated/g' $(git grep --files-with-matches '\b_("' src) echo Hard cases - multiline strings. sed -i 's/"Visit %s for further information about the software.")/&.translated/g' src/init.cpp sed -i "s/\"Only rebuild the block database if you are sure that your computer's date and time are correct\")/&.translated/g" src/init.cpp sed -i 's/" restore from a backup.")/&.translated/g' src/wallet/db.cpp sed -i 's/" or address book entries might be missing or incorrect.")/&.translated/g' src/wallet/wallet.cpp echo Special case. sed -i 's/_(COPYRIGHT_HOLDERS)/&.translated/' src/util/system.cpp test/lint/lint-format-strings.py -END VERIFY SCRIPT-

hebasto · 2019-07-24T13:46:13Z

There currently a lint error making travis fail, but this looks good otherwise.

Fixed.

One thing is I would probably remove "gui:" prefix from PR title because that even though this is refactoring that doesn't change behavior and is meant to help the gui, the prefix could mislead reviewers to thinking the PR doesn't touch non-gui code.

Fair point. Done.

ryanofsky

utACK 753f7cc. Only change since last review is fixing lint error (double includes)

maflcko · 2019-07-24T18:06:49Z

ACK 753f7cc

Show signature and timestamp

Signature:

Timestamp of file with hash ca358d398db24f0840fdb656fc63a356463d01b9f675517cbd06d4b4e20071c3 -

753f7cc scripted-diff: Make translation bilingual (Hennadii Stepanov) 7c45e14 Add bilingual message type (Hennadii Stepanov) 0b86e51 Refactor out translation.h (Hennadii Stepanov) Pull request description: This PR adds the `bilingual_str` struct and a `strprintf` overload: https://github.com/bitcoin/bitcoin/blob/0626b8cbdf0aa971500eb5613c7ab4096c496966/src/tinyformat.h#L1066-L1067 Both new features allow bitcoin code to easily send dual translated and non-translated messages to the GUI and the logging framework. This PR is only a refactoring (has been split off the bitcoin#16224 (see: bitcoin#16224)) and does not change behavior. ACKs for top commit: MarcoFalke: ACK 753f7cc ryanofsky: utACK 753f7cc. Only change since last review is fixing lint error (double includes) Tree-SHA512: 52b0654421d558e4775c0484d78be26319fe3db5118af9b0a9bdfbdaad53a3704f527a5d5aba1013a64560b9b6a0c3c4cf0a6782e49aa731e18d99de95220385

Summary: This is a partial backport of Core [[bitcoin/bitcoin#16362 | PR16362]] : bitcoin/bitcoin@7c45e14 Depends on D6048 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D6050

Summary: -BEGIN VERIFY SCRIPT- sed -i 's/inline std::string _(const char\* psz)/inline bilingual_str _(const char\* psz)/' src/util/translation.h sed -i 's/return G_TRANSLATION_FUN ? (G_TRANSLATION_FUN)(psz) : psz;/return bilingual_str{psz, G_TRANSLATION_FUN ? (G_TRANSLATION_FUN)(psz) : psz};/' src/util/translation.h sed -i 's/\b_("$[^"]\|\\"$*")/&.translated/g' $(git grep --files-with-matches '\b_("' src) echo Hard cases - multiline strings. sed -i 's/"Visit %s for further information about the software.")/&.translated/g' src/init.cpp sed -i "s/\"Only rebuild the block database if you are sure that your computer's date and time are correct\")/&.translated/g" src/init.cpp sed -i 's/" restore from a backup.")/&.translated/g' src/wallet/db.cpp sed -i 's/" or address book entries might be missing or incorrect.")/&.translated/g' src/wallet/wallet.cpp echo Special case. sed -i 's/_(COPYRIGHT_HOLDERS)/&.translated/' src/util/system.cpp test/lint/lint-format-strings.py -END VERIFY SCRIPT- This is a partial backport of Core [[bitcoin/bitcoin#16362 | PR16362]] : bitcoin/bitcoin@753f7cc Depends on D6050 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D6052

hebasto mentioned this pull request Jul 9, 2019

gui: Bilingual GUI error messages #16224

Merged

maflcko added this to the 0.19.0 milestone Jul 9, 2019

fanquake added the GUI label Jul 9, 2019

DrahtBot added the Needs rebase label Jul 9, 2019

hebasto force-pushed the 20190709-bilingual-part-one branch from 4e1eda7 to 29b28c7 Compare July 10, 2019 15:14

DrahtBot added Needs rebase and removed Needs rebase labels Jul 10, 2019

hebasto force-pushed the 20190709-bilingual-part-one branch from 29b28c7 to d2a5927 Compare July 18, 2019 15:48

DrahtBot added Needs rebase and removed Needs rebase labels Jul 18, 2019

hebasto force-pushed the 20190709-bilingual-part-one branch from d2a5927 to 3b7348f Compare July 23, 2019 17:18

DrahtBot removed the Needs rebase label Jul 23, 2019

ryanofsky approved these changes Jul 23, 2019

View reviewed changes

DrahtBot added the Needs rebase label Jul 24, 2019

hebasto changed the title ~~gui: Bilingual translation~~ gui: Add bilingual_str type Jul 24, 2019

hebasto force-pushed the 20190709-bilingual-part-one branch from 3b7348f to 375d909 Compare July 24, 2019 13:21

ryanofsky approved these changes Jul 24, 2019

View reviewed changes

hebasto added 3 commits July 24, 2019 16:32

Refactor out translation.h

0b86e51

This is a prerequisite for introducing bilingual error messages. Note: #includes are arranged by clang-format-diff.py script.

Add bilingual message type

7c45e14

hebasto force-pushed the 20190709-bilingual-part-one branch from 375d909 to 753f7cc Compare July 24, 2019 13:33

hebasto changed the title ~~gui: Add bilingual_str type~~ Add bilingual_str type Jul 24, 2019

fanquake added Utils/log/libs and removed Needs rebase GUI labels Jul 24, 2019

ryanofsky approved these changes Jul 24, 2019

View reviewed changes

maflcko merged commit 753f7cc into bitcoin:master Jul 24, 2019

hebasto deleted the 20190709-bilingual-part-one branch July 26, 2019 19:15

hebasto mentioned this pull request Feb 1, 2020

"PSBT Operations" dialog #18027

Merged

ryanofsky mentioned this pull request Apr 27, 2020

Add fee_est tool for debugging fee estimation code #10443

Closed

kwvg added a commit to kwvg/dash that referenced this pull request Nov 6, 2021

merge bitcoin#16362: Add bilingual_str type

c7da18e

kwvg added a commit to kwvg/dash that referenced this pull request Nov 30, 2021

merge bitcoin#16362: Add bilingual_str type

91e74d0

kwvg added a commit to kwvg/dash that referenced this pull request Nov 30, 2021

merge bitcoin#16362: Add bilingual_str type

42e5bfe

kwvg added a commit to kwvg/dash that referenced this pull request Nov 30, 2021

merge bitcoin#16362: Add bilingual_str type

3bd97cc

kwvg added a commit to kwvg/dash that referenced this pull request Nov 30, 2021

merge bitcoin#16362: Add bilingual_str type

dbb322a

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add bilingual_str type #16362

Add bilingual_str type #16362

Uh oh!

hebasto commented Jul 9, 2019 •

edited

Loading

Uh oh!

hebasto commented Jul 9, 2019

Uh oh!

maflcko commented Jul 9, 2019

Uh oh!

DrahtBot commented Jul 9, 2019 •

edited

Loading

Uh oh!

hebasto commented Jul 10, 2019

Uh oh!

hebasto commented Jul 18, 2019

Uh oh!

hebasto commented Jul 23, 2019

Uh oh!

ryanofsky left a comment

Uh oh!

DrahtBot commented Jul 24, 2019

Uh oh!

hebasto commented Jul 24, 2019

Uh oh!

ryanofsky left a comment •

edited

Loading

Uh oh!

hebasto commented Jul 24, 2019

Uh oh!

ryanofsky left a comment

Uh oh!

maflcko commented Jul 24, 2019

Uh oh!

Uh oh!

	/** Format arguments and return the string or write to given std::ostream (see tinyformat::format doc for details) */
	#define strprintf tfm::format

Add bilingual_str type #16362

Add bilingual_str type #16362

Uh oh!

Conversation

hebasto commented Jul 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Jul 9, 2019

Uh oh!

maflcko commented Jul 9, 2019

Uh oh!

DrahtBot commented Jul 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

hebasto commented Jul 10, 2019

Uh oh!

hebasto commented Jul 18, 2019

Uh oh!

hebasto commented Jul 23, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jul 24, 2019

Uh oh!

hebasto commented Jul 24, 2019

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jul 24, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jul 24, 2019

Uh oh!

Uh oh!

hebasto commented Jul 9, 2019 •

edited

Loading

DrahtBot commented Jul 9, 2019 •

edited

Loading

ryanofsky left a comment •

edited

Loading