ryanofsky
commented at 3:02 pm on June 17, 2019:
member
EDIT: I didn’t realize when I wrote this there was lots of previous discussion already about showing untranslated messages in #15340.
Concept ACK, especially for the code cleanup making error handling more consistent. Some thoughts:
I’m not sure it would be better to show same message twice in error dialogs, than to only show the translated message and something like “More detailed debug information can be found in debug.log,” or maybe just the translated message and a “View debug log…” button, to reduce the amount of clutter. I’d hope that in most cases translated error messages would be clear enough to help users solve problems, and only more rarely would users need untranslated strings for to report a bug or look things up.
Using _("") syntax for translated strings is pretty common and well understood, but _("", true) for bilingual strings seems harder to distinguish and figure out. Might worth considering other syntaxes _b(""), bilingual(""), translate("") or even ""_translate with user literal syntax. Other option would be not having any special syntax and just adding normal functions that return literal and formatted bilingual_str objects.
PR looks good in its current form though, and it would be good to get opinions from other reviewers.
MarcoFalke
commented at 3:42 pm on June 17, 2019:
member
Is there even a point in having two different translation functions long term? Shouldn’t all non-gui code use the one that returns a bilingual_str?
ryanofsky
commented at 3:58 pm on June 17, 2019:
member
Is there even a point in having two different translation functions long term?
Good point. I don’t think there is. So another alternative could be to update the _ function to return both translated and untranslated strings, and not have any new syntax at all.
DrahtBot
commented at 6:37 pm on June 17, 2019:
member
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.
DrahtBot added the label
Needs rebase
on Jun 18, 2019
hebasto force-pushed
on Jun 19, 2019
hebasto
commented at 3:53 pm on June 19, 2019:
member
Rebased.
DrahtBot removed the label
Needs rebase
on Jun 19, 2019
DrahtBot added the label
Needs rebase
on Jun 25, 2019
in
src/util/translation.h:26
in
81baf61a15outdated
Using _("") syntax for translated strings is pretty common and well understood, but _("", true) for bilingual strings seems harder to distinguish and figure out. Might worth considering other syntaxes _b(""), bilingual(""), translate("") or even ""_translate with user literal syntax.
IIUC, this will require appropriate changes for the share/qt/extract_strings_qt.py script, right?
DrahtBot removed the label
Needs rebase
on Jun 25, 2019
in
src/util/translation.h:37
in
1855f54d3coutdated
32+
33+/**
34+ * Translation function.
35+ * If no translation function is set, simply return the input.
36+ */
37+inline std::string _(const char* psz)
IIUC, your suggestion implies some kind of type conversion (from bilingual_str to std::string) for arguments of some functions and in assignment statements?
If you don’t need the original string, it can be dropped with _().translated, no? I understand that the changes are more disruptive, but it seems clearer than the unused optional argument.
It could probably be done as a scripted-diff.
DrahtBot added the label
Needs rebase
on Jun 27, 2019
hebasto force-pushed
on Jun 28, 2019
hebasto force-pushed
on Jun 28, 2019
DrahtBot removed the label
Needs rebase
on Jun 28, 2019
hebasto
commented at 6:31 pm on June 28, 2019:
member
The function with signature InitError(const std::string&) has been removed intentionally to prevent emerging of untranslated error messages in the future. All such cases should be implemented in the code explicitly.
promag
commented at 3:25 pm on July 5, 2019:
member
Agree with @ryanofsky, I think original text could be set with QMessageBox::setDetailedText and BitcoinGUI::message could be extended to accept that parameter.
hebasto
commented at 3:47 pm on July 5, 2019:
member
Agree with @ryanofsky, I think original text could be set with QMessageBox::setDetailedText and BitcoinGUI::message could be extended to accept that parameter.
Do you think the Show Details... button on an error message window is expressive enough for a user? Maybe QMessageBox::informativeText?
Anyway, could this improvement be implemented with a successive PR?
hebasto force-pushed
on Jul 5, 2019
promag
commented at 8:47 pm on July 5, 2019:
member
I don’t think so, the idea is to allow access to the original message while not displaying both.
hebasto
commented at 8:54 pm on July 5, 2019:
member
I think it’d be nice if InitWarning took a bilingual string. It would make InitWarning more consistent with InitError, and give us the ability to easily log untranslated warnings. But this could be implemented in a separate PR, since it would probably complicate this one.
I realize you are not changing behavior, but is it intentional that only the first part of the translated string is actually translated, and the second half is not? I’m not sure this makes very much sense, so it might be helpful to have a TODO comment for cleaning this up later.
It would be good to add some comment here explaining this. If it’s intentional never to translate the abortnode message it’d be good to write that. Or if it would be better to fix this later, there could be a TODO comment here saying AbortNode should take a bilingual string, or whatever the followup would be.
This change as a whole seems to be adding a lot of new translated strings. But I wonder if it would be better to be more conservative and not add new translations here, instead saving that part for a followup PR and selectively leaving less common errors untranslated. One way to do this might be with a DoNotTranslate function
Nit: don’t forget to actually use Untranslated here.
ryanofsky approved
ryanofsky
commented at 8:26 pm on July 15, 2019:
member
utACK32cf78af45c662617eaf91bb3bb07e27151a3a95. I skimmed all the commits, but found it most easy just to review the final consolidated diff, because some of intermediate commits update the same lines more than once.
This change seems acceptable in its current state, but my preference would be to have a new refactoring-only PR that introduces bilingual string instances without changing behavior. The refactoring-only PR could:
Introduce the bilingual_str class
Change _ to return bilingual strings, and change MessageBox and InitError (and maybe AbortNode and InitWarning) to accept bilingual strings
Add DoNotTranslate (see below) calls wherever needed to maintain existing behavior and avoid breaking compilation.
After that PR, this PR altering the GUI and adding new translations could be smaller and easier to understand.
DrahtBot added the label
Needs rebase
on Jul 16, 2019
hebasto force-pushed
on Jul 18, 2019
hebasto
commented at 4:41 pm on July 18, 2019:
member
Rebased.
DrahtBot removed the label
Needs rebase
on Jul 18, 2019
hebasto
commented at 6:19 pm on July 18, 2019:
member
The refactoring-only PR could:
…
2. Change _ to return bilingual strings, and change MessageBox and InitError (and maybe AbortNode and InitWarning) to accept bilingual strings
Such commit could be very complicated. Currently, this PR introduces bilingual abilities to the related functions (ThreadSafeMessageBox, InitError) commit by commit.
Could we move on? Would you mind looking into #16362 too?
hebasto force-pushed
on Jul 24, 2019
hebasto
commented at 1:59 pm on July 24, 2019:
member
Rebased.
fanquake removed the label
Needs rebase
on Jul 24, 2019
hebasto force-pushed
on Jul 24, 2019
hebasto
commented at 3:30 pm on July 24, 2019:
member
@MarcoFalke
Rebasing on top of #16366 caused a bilingual representation of some messages: diff.
I suppose some of them should be untranslated intentionally. If so, let me know to make changes, please?
ryanofsky
commented at 4:48 pm on July 24, 2019:
member
I suppose some of them should be untranslated intentionally. If so, let me know to make changes, please?
There could be other opinions, but I’d recommend replacing _ with DoNotTranslate here to avoid refactoring and changing behavior in the same PR. (Sorry for sounding like a broken record on mixing refactoring and behavior changes, but I think if we want to add new translations, this should happen in a small separate standalone PR adding new translations, not mixed in with everything else here).
MarcoFalke referenced this in commit
d960d5ca99
on Jul 24, 2019
translations are only for the gui, this should never be added here
I haven’t looked at the context for this comment yet, but in general translations are desirable when writing error messages to stderr, right? My understanding is we only want to avoid translating very technical messages, rare messages, and messages sent to debug.log.
translations are only for the gui, this should never be added here
That is correct. But without #include <util/translation.h> the linker fires an error for the last but one commit:
0 CXXLD bitcoind
1libbitcoin_server.a(libbitcoin_server_a-init.o): In function `_(char const*)':2/home/hebasto/github/bitcoin/src/./util/translation.h:39: undefined reference to `G_TRANSLATION_FUN[abi:cxx11]'
in
src/qt/bitcoin.cpp:458
in
af608a0cf0outdated
458@@ -459,7 +459,7 @@ int GuiMain(int argc, char* argv[])
459 SetupUIArgs();
460 std::string error;
461 if (!node->parseParameters(argc, argv, error)) {
462- node->initError(strprintf("Error parsing command line arguments: %s\n", error));
463+ node->initError(strprintf(_("Error parsing command line arguments: %s\n"), error));
464 // Create a message box, because the gui has neither been created nor has subscribed to core signals
465 QMessageBox::critical(nullptr, PACKAGE_NAME,
466 // message can not be translated because translations have not been initialized
Here and the two other instances: The The gui is not yet subscribed to core signals and can’t pick up the translation, which is why it is manually translated below.
This translated title, QObject::tr("Original message:"), is intended for non-english interface.
The message itself is not translated, obviously.
That was an initial idea, but I’m open for changes ;)
MarcoFalke
commented at 6:06 pm on October 7, 2019:
yes, what is the point of showing the translation and the original message. If the user understood the original message (in English), what is the point of showing the translation?
MarcoFalke
commented at 6:24 pm on October 7, 2019:
in commit aa21c807af55f94867b11bab5e2e9a8160a3fc9d
style-question: Could this be called Untranslated, so that arbitrary strings can be passed in (even string that are impossible to translate, such as error strings returned from third party libraries).
hebasto
commented at 7:03 pm on October 7, 2019:
member
@MarcoFalke
Thank you for your review. Your comments have been addressed.
jonasschnelli
commented at 7:37 am on October 10, 2019:
contributor
Tested and looks good.
Would it make sense to also make the strings in qt/bitcoin.cpp translate the new way?
Things like "Specified data directory \"%s\" does not exist" or "Error reading configuration file: %s\n"
hebasto
commented at 8:44 am on October 10, 2019:
member
Would it make sense to also make the strings in qt/bitcoin.cpp translate the new way?
Things like "Specified data directory \"%s\" does not exist" or "Error reading configuration file: %s\n"
These strings are not in qt/bitcoin.cpp but in bitcoind.cpp ;)
More details in #16366 by @MarcoFalke
DrahtBot added the label
Needs rebase
on Oct 15, 2019
hebasto force-pushed
on Oct 16, 2019
hebasto
commented at 4:27 pm on October 16, 2019:
member
DrahtBot removed the label
Needs rebase
on Nov 6, 2019
in
src/httpserver.cpp:179
in
ffed46515doutdated
175@@ -175,7 +176,7 @@ static bool InitHTTPAllowList()
176 LookupSubNet(strAllow.c_str(), subnet);
177 if (!subnet.IsValid()) {
178 uiInterface.ThreadSafeMessageBox(
179- strprintf("Invalid -rpcallowip subnet specification: %s. Valid are a single IP (e.g. 1.2.3.4), a network/netmask (e.g. 1.2.3.4/255.255.255.0) or a network/CIDR (e.g. 1.2.3.4/24).", strAllow),
180+ strprintf(_("Invalid -rpcallowip subnet specification: %s. Valid are a single IP (e.g. 1.2.3.4), a network/netmask (e.g. 1.2.3.4/255.255.255.0) or a network/CIDR (e.g. 1.2.3.4/24)."), strAllow),
Sjors
commented at 2:36 pm on November 20, 2019:
member
The end result ffed46515d556c0dbc8535548d0a485a62306106 looks good, lightly tested on macOS.
But the individual commits are too confusing atm, they would be easier to review if you:
the first commit 05b20f6 as is
introduce Untranslated earlier, with just {return original}
the next commit moves .translated to ThreadSafeMessageBox
a commit where you introduce newly translated strings _(
a commit that drops the redundant return false from calls to InitError (although otherwise unrelated)
(maybe a few more things, to keep 7 focussed)
switch to bilingual_str
I don’t mind if this PR adds or removes a few translations, if that makes things easier. I also suspect there’s more places where we can add Untranslated, but this could be done in a followup.
laanwj
commented at 2:54 pm on November 20, 2019:
member
ACKffed46515d556c0dbc8535548d0a485a62306106
I like that this has a by-effect of introducing a type for GUI-facing messages versus internal ones. An explicit Untranslated instead of having bilingual_str implicitly constructed is a good idea. I also like that you put the translation-related stuff in a separate header.
DrahtBot added the label
Needs rebase
on Nov 20, 2019
hebasto force-pushed
on Nov 21, 2019
hebasto
commented at 6:18 pm on November 21, 2019:
member
But the individual commits are too confusing atm, they would be easier to review if you …
Sorry, but I’d prefer to leave the commit order as is for the following reasons: (a) it helps me to keep this PR rebased; (b) other reviewers have ACKed already.
The intrinsic logic of each commit is: a functional change plus other changes to compile successfully.
I also suspect there’s more places where we can add Untranslated, but this could be done in a followup.
Totally agree with you.
DrahtBot removed the label
Needs rebase
on Nov 21, 2019
DrahtBot added the label
Needs rebase
on Jan 8, 2020
hebasto force-pushed
on Jan 8, 2020
hebasto
commented at 5:19 pm on January 8, 2020:
member
MarcoFalke
commented at 4:00 pm on May 8, 2020:
member
ACK18bd83b1fee2eb47ed4ad05c91f2d6cc311fc9ad 🐦
Thanks for finally making the debug log of international users readable. I
found it painful to translate the debug log in bug reports back to English.
I checked that no new translations are added, but did not run the GUI.
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2 3ACK18bd83b1fee2eb47ed4ad05c91f2d6cc311fc9ad 🐦
4 5Thanks for finally making the debug log of international users readable. I
6found it painful to translate the debug log in bug reports back to English.
7 8I checked that no new translations are added, but did not run the GUI.
9-----BEGIN PGP SIGNATURE-----
1011iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
12pUiI8wwAnm6ab4DxH44gY+MYdYKbbQe+Qayc2wuMw1wvpNfYdJDZ2xK2N5ZWCIoc
13grA4eVeluZggMf30d8pKP+HFofQVugYFyTB/lG/9PnSl//WAgezClqkxnW1YE0pm
14xfVfKC1f7RBGswfcm+Rrq7m0LfzWEjoGgCCbcjQm86kAP+Mt8phmjQRNc43FycFJ
15dLO7DFZMiqjSIhLi4bbSqiynUXuTY2tMbjv4e23gHWKFcnvndBE3QjfirsRlcp4f
16Ly5lvEFCABOdnMQ0K8/9bSn2XqHwWyZxXJ+Yj1twkCchcjmcnHTfAmTJXs0Vahqd
17vapzAw2X0dk6x8JWZcu+am7rFSzZu+tAHWnDC1DgjgsnDZMMTI4rjPbtBP9X01ka
18C6SjRDt91NZdm2x+jXrHKEhmQdQ3FDVU2hGb6g3cjxe/RJAyxZ3W5ZTTYqvQXOjh
19992J7pQOtQjQHovjFWyQ4WSxlTXPd7PFhZ1NyOJE2UCJD8rdUtav2nxzbh0q3SX2
20Seu4nsii
21=xXpH
22-----END PGP SIGNATURE-----
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2024-12-18 06:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
