Translate strings for connection/disconnection #2094

Kebap · 2018-10-29T01:37:37Z

Brief overview of PR changes/additions

Including many strings for translation, mostly related to connecting and disconnecting a profile.
Removing strings from translations where they are not needed.
Adjusting code to better grasp strings to translate.

Motivation for adding to Mudlet

Reduce English texts in translated Mudlet versions to mostly only texts used internally.

Other info (issues closed, discussion etc)

As discussed in #1964 and #2010 and Crowdin

vadi2

Thanks a ton!

vadi2

Thanks a ton!

vadi2 · 2018-10-29T06:46:07Z

Looks like you're missing some semicolons: https://travis-ci.org/Mudlet/Mudlet/jobs/447586154#L4494

Kebap · 2018-10-29T07:05:29Z

Fixed. Thanks for the feedback!

vadi2 · 2018-10-29T07:40:04Z

Some more - there is no tr() in main.cpp: https://travis-ci.org/Mudlet/Mudlet/jobs/447649485#L1825

vadi2 · 2018-10-29T08:13:19Z

Getting close - missing some closing brackets and something to concatenate the build number together? https://travis-ci.org/Mudlet/Mudlet/jobs/447660104#L2784

vadi2 · 2018-10-29T09:08:40Z

Another one, need a bracket on "ne"): if (dir == QStringLiteral("ne" || dir == QStringLiteral("northeast")) {

vadi2 · 2018-10-29T09:42:48Z

Getting there - I think msg.append(e.c_str()); now needs to be msg.append(e); now that e is a QString and not a C++ string

vadi2 · 2018-10-29T11:33:16Z

Missing a closing semicolon: https://travis-ci.org/Mudlet/Mudlet/jobs/447726380#L5487

Getting there!

vadi2 · 2018-10-29T13:35:37Z

🎉 nice work

vadi2 · 2018-11-01T08:24:48Z

@Kebap feel free to merge this in, by the way

SlySven

⚠️ I know it is too late for this PR but there are some issues that need fixing amongst the comments I've made here (the use of QString::toLatin1())...

SlySven · 2018-11-17T10:18:03Z

src/TLuaInterpreter.cpp

        if (lua_isstring(pGlobalLua, -1)) {
-            e = "Lua error:";
+            e = tr("Lua error:");


A separate issue that does still need to be fixed - there needs to be a space after the colon in English. Otherwise the appended error message is appended directly to it! (BTW for French there will also be a space before the colon - but then the French are different that way about punctuation and spaces, « bizarre »!)

Noted as Issue: #2124 ...

SlySven · 2018-11-17T10:22:14Z

src/TLuaInterpreter.cpp

-        QString msg = "[ ERROR ] - Cannot find Lua module rex_pcre.\n"
-                      "Some functions may not be available.\n";
-        msg.append(e.c_str());
+        QString msg = tr("[ ERROR ] - Cannot find Lua module rex_pcre.\n"


FTR appending QStrings is less efficient than creating them with all the elements at once, it would have been better here to have changed lines 13501 to 13503 to:

QString msg = tr("[ ERROR ] - Cannot find Lua module rex_pcre.\n" "Some functions may not be available.\n "%1").arg(e);

Noted as a general issue as: #2123

SlySven · 2018-11-17T10:28:55Z

src/ctelnet.cpp

@@ -292,7 +292,7 @@ void cTelnet::connectIt(const QString& address, int port)

    hostName = address;
    hostPort = port;
-    QString server = "[ INFO ]  - Looking up the IP address of server:" + address + ":" + QString::number(port) + " ...";
+    QString server = tr("[ INFO ]  - Looking up the IP address of server:") + address + ":" + QString::number(port) + " ...";


The QString::operator+ is the same as QString::append() {under most circumstances, though there is a workaround that swaps it for QStringBuilder::operator% IIRC which works more efficiently}. It would have been better to merge all the concatenations into a single instantiation using the QString:arg() method to drop in address and port replaceable arguments...

As above noted as: #2123

SlySven · 2018-11-17T10:35:51Z

src/ctelnet.cpp

@@ -305,7 +305,7 @@ void cTelnet::disconnect()

 void cTelnet::handle_socket_signal_error()
 {
-    QString err = "[ ERROR ] - TCP/IP socket ERROR:" % socket.errorString();
+    QString err = tr("[ ERROR ] - TCP/IP socket ERROR:") % socket.errorString();


Whilst this is a valid use of the QStringBuilder::operator% it mandates that the error message is tacked onto the end of the message - it would have been more translatable to use:

QString err = tr("[ ERROR ] - TCP/IP socket ERROR: \"%1\".").arg(socket.errorString());

Notice that there is now a space after the colon which is missing in the original (this is a common feature in the original texts in this PR isn't it! 😝) as well as a terminal .. It also allows the socket.errorString() to be rearranged (as %1) within the body of the translated text should that be necessary/desirable for a particular locale/language.

SlySven · 2018-11-17T10:42:17Z

src/ctelnet.cpp

                if (mpHost->mServerGUI_Package_version != newVersion) {
-                    QString _smsg = tr("<The server wants to upgrade the GUI to new version '%1'. Uninstalling old version '%2'>")
+                    _smsg = tr("[ INFO ] - The server wants to upgrade the GUI to new version '%1'. Uninstalling old version '%2'.")
                                            .arg(QString::number(newVersion), QString::number(mpHost->mServerGUI_Package_version));
                    mpHost->mpConsole->print(_smsg.toLatin1().data());


I'm getting DejaVu all over again. It is not valid to used toLatin1() on a text that contains translations as toLatin1() will nuke any graphemes that are not part of the Latin1 charset which are going to be present in at least some of the translations! If TConsole::print(...) requires a std::string (or const char*) argument then you must use toUtf8() instead...

Logged as #2122

Kebap · 2018-11-18T18:29:09Z

@SlySven Could you please file your (valid) comments and requests for improvement as issues of their own, instead of mere comments on an already merged PR? I know it may seem more handy here, because you can comment code lines specifically, and you found these while traversing there, but still... They will easily get lost here, and can't be adressed anymore. Whereas as seperate issues we can ponder, discuss scope and fix them normally when the time comes, so they can be closed of after.

SlySven · 2018-11-18T22:34:35Z

Okay, have raised three issues that should cover the main points that I noted.

Kebap added 2 commits October 29, 2018 00:12

Update strings' translatability for (dis-)connections

5d50ca5

Translate startup splash screen, command line options

a8c20dd

Kebap requested review from a team as code owners October 29, 2018 01:37

vadi2 approved these changes Oct 29, 2018

View reviewed changes

vadi2 assigned Kebap Oct 29, 2018

Add 2 missing semicolons

d0f45aa

Kebap removed their assignment Oct 29, 2018

replace tr("x") with QCoreApplication::translate("main", "x")

13aa04c

fix typos

72c5625

more typos, also string -> QString

1655cfa

msg.append(e) and not msg.append(e.c_str());

901735d

Add 1 semicolon

14a38c1

Kebap merged commit d1c9e4a into Mudlet:development Nov 1, 2018

Kebap deleted the Translate-TLuaInterpreter branch November 1, 2018 22:32

SlySven reviewed Nov 17, 2018

View reviewed changes

SlySven mentioned this pull request Nov 18, 2018

Ensure that translatable QStrings are not put through a toLatin1() call #2122

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Translate strings for connection/disconnection #2094

Translate strings for connection/disconnection #2094

Kebap commented Oct 29, 2018

vadi2 left a comment

vadi2 left a comment

vadi2 commented Oct 29, 2018

Kebap commented Oct 29, 2018

vadi2 commented Oct 29, 2018

vadi2 commented Oct 29, 2018

vadi2 commented Oct 29, 2018

vadi2 commented Oct 29, 2018

vadi2 commented Oct 29, 2018

vadi2 commented Oct 29, 2018

vadi2 commented Nov 1, 2018

SlySven left a comment

SlySven Nov 17, 2018

SlySven Nov 18, 2018

SlySven Nov 17, 2018

SlySven Nov 18, 2018

SlySven Nov 17, 2018

SlySven Nov 18, 2018

SlySven Nov 17, 2018

SlySven Nov 17, 2018

SlySven Nov 18, 2018

Kebap commented Nov 18, 2018

SlySven commented Nov 18, 2018

Translate strings for connection/disconnection #2094

Translate strings for connection/disconnection #2094

Conversation

Kebap commented Oct 29, 2018

Brief overview of PR changes/additions

Motivation for adding to Mudlet

Other info (issues closed, discussion etc)

vadi2 left a comment

Choose a reason for hiding this comment

vadi2 left a comment

Choose a reason for hiding this comment

vadi2 commented Oct 29, 2018

Kebap commented Oct 29, 2018

vadi2 commented Oct 29, 2018

vadi2 commented Oct 29, 2018

vadi2 commented Oct 29, 2018

vadi2 commented Oct 29, 2018

vadi2 commented Oct 29, 2018

vadi2 commented Oct 29, 2018

vadi2 commented Nov 1, 2018

SlySven left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kebap commented Nov 18, 2018

SlySven commented Nov 18, 2018