Don't duplicate warning strings in GetWarnings #13896

pull Empact wants to merge 1 commits into bitcoin:master from Empact:warning-msg-dup changing 1 files +10 −6

Empact commented at 9:19 PM on August 6, 2018: member

Since strStatusBar only ever holds the desired string, we can use it in the strGUI construction as well.
fanquake added the label Refactoring on Aug 6, 2018

Don't duplicate warning strings in GetWarnings

Since strStatusBar only ever holds the desired string, we can use it in the
strGUI construction as well.

036821cedb

in src/warnings.cpp:50 in 9103ca42d3 outdated

  46 | @@ -47,7 +47,7 @@ std::string GetWarnings(const std::string& strFor)
  47 |  
  48 |      if (!CLIENT_VERSION_IS_RELEASE) {
  49 |          strStatusBar = "This is a pre-release test build - use at your own risk - do not use for mining or merchant applications";
  50 | -        strGUI = _("This is a pre-release test build - use at your own risk - do not use for mining or merchant applications");
  51 | +        strGUI = _(strStatusBar);

MarcoFalke commented at 9:24 PM on August 6, 2018:

This doesn't work with translations, imo

scravy commented at 9:36 PM on August 6, 2018:

couldn't this just be strGUI = strStatusBar? And strStatusBar = _(...)

Empact force-pushed on Aug 6, 2018

in src/warnings.cpp:42 in 036821cedb

  38 | @@ -39,15 +39,19 @@ void SetfLargeWorkInvalidChainFound(bool flag)
  39 |  
  40 |  std::string GetWarnings(const std::string& strFor)
  41 |  {
  42 | +    static const char* PRE_RELEASE_WARNING = "This is a pre-release test build - use at your own risk - do not use for mining or merchant applications";

domob1812 commented at 7:35 AM on August 7, 2018:

Nit: You could make this constexpr, as in:

static constexpr const char PRE_RELEASE_WARNING[] = "...";

domob1812 commented at 7:35 AM on August 7, 2018: contributor

utACK 036821cedbf7c6010d437b467f0c8cc4a30d4659

in src/warnings.cpp:66 in 036821cedb

  62 | @@ -59,13 +63,13 @@ std::string GetWarnings(const std::string& strFor)
  63 |  
  64 |      if (fLargeWorkForkFound)
  65 |      {
  66 | -        strStatusBar = "Warning: The network does not appear to fully agree! Some miners appear to be experiencing issues.";
  67 | -        strGUI += (strGUI.empty() ? "" : uiAlertSeperator) + _("Warning: The network does not appear to fully agree! Some miners appear to be experiencing issues.");
  68 | +        strStatusBar = FORK_WARNING;

scravy commented at 8:00 AM on August 7, 2018:

shouldn't these be also translated? _(...)

ken2812221 commented at 8:18 AM on August 7, 2018: contributor

utACK 036821cedbf7c6010d437b467f0c8cc4a30d4659
laanwj commented at 11:13 AM on August 7, 2018: member

Please do not do this, the gettext script will not be able to pick up text if you don't directly embed it in _()! (I don't think you can get around the duplication - the idea is that the message will be translated for GUI, but not for RPC, maybe we need to add a comment so people won't try this)
Empact commented at 3:59 PM on August 7, 2018: member

@laanwj is there a reason not to translate the strStatusBar string?
MarcoFalke commented at 4:30 PM on August 7, 2018: member

@Empact I guess you wouldn't know the language setting of the client sending the request to the rpc server?
Empact commented at 4:34 PM on August 7, 2018: member

Gotcha! Closing as impractical.
Empact closed this on Aug 7, 2018
Empact deleted the branch on Aug 7, 2018
laanwj commented at 12:05 PM on August 8, 2018: member

@laanwj is there a reason not to translate the strStatusBar string?

RPC is not a user interface but a machine interface, as client-side software might be matching on messages, they should never be translated.

(as a concrete example, at some point this interfered when running the tests against bitcoin-qt on a non-english locale)
DrahtBot locked this on Sep 8, 2021

Contributors

Labels