I18n: clear out translatable Lua error messsages by SlySven · Pull Request #941 · Mudlet/Mudlet

SlySven · 2017-04-21T09:24:02Z

Since it has been decided that the Mudlet Lua API should not have the capability to report errors in using the API in a translated form the use of the Qt tr(...) function to mark such QStrings is not wanted. This commit switches those usages to use the QStringLiteral(...) wrapper instead. As the strings concerned are each unique the known downsides of using this "non"-translatable QString wrapper should not be significant and the code "template" for the two things are very similar {if there are no second or second & third arguments in the tr(...) case} - the editing to switch to QLatin1Strings in the same location would be much more intensive.

Where the (formerly) translated QString is then passed to the Lua system there are some instances where QString::toUtf8() must be used instead of QString::toLatin1() - these are generally the cases where a variable included in the resulting QString can contain non-ASCII characters which is any user defined/named item or {mainly on Windows platform} involving file-system objects as that can also have non-ASCII characters.

There may be a few long text lines that I have manually inserted new-lines into to try and prevent the resulting text from exceeding around 80 characters when shown on the main profile console.

There was a typo in one error message for TLuaInterpreter::downloadFile as it described an issue for argument 1 when it should have said 2. Also in three functions a QString "windowName" is set to the null QString when the QString::clear()is a little more descriptive of what is happening.

Signed-off-by: Stephen Lyons slysven@virginmedia.com

Since it has been decided that the Mudlet Lua API should not have the capability to report errors in using the API in a translated form the use of the Qt tr(...) function to mark such QStrings is not wanted. This commit switches those usages to use the QStringLiteral(...) wrapper instead. As the strings concerned are each unique the known downsides of using this "non"-translatable QString wrapper should not be significant and the code "template" for the two things are very similar {if there are no second or second & third arguments in the tr(...) case} - the editing to switch to QLatin1Strings in the same location would be much more intensive. Where the (formerly) translated QString is then passed to the Lua system there are some instances where QString::toUtf8() must be used instead of QString::toLatin1() - these are generally the cases where a variable included in the resulting QString can contain non-ASCII characters which is any user defined/named item or {mainly on Windows platform} involving file-system objects as that can also have non-ASCII characters. There may be a few long text lines that I have manually inserted new-lines into to try and prevent the resulting text from exceeding around 80 characters when shown on the main profile console. There was a typo in one error message for TLuaInterpreter::downloadFile as it described an issue for argument 1 when it should have said 2. Also in three functions a QString "windowName" is set to the null `QString` when the `QString::clear()`is a little more descriptive of what is happening. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

vadi2

👌 awesome. Trying your hand at the source code smashing too, hey?

SlySven · 2017-04-21T16:18:35Z

It's a mixed feeling I have for this 😐 - I still think there is a middle way where a user having a problem with a Mudlet API feature could get help in their native language whilst using current English API functions/constants/variables even though the Lua core + libraries which are used by (some large multiplier) more people will also be producing English error messages but which will also have a wider support base amongst non-English users.

Maybe it will get revisited for 5.0 when we get many more non-English users from Mudlet 4.0? 😉

Anyhow this is a big, repetitive PR and I'll see if @ahmedcharles has any input (spotted any clangers?) before hitting the green button.

vadi2 · 2017-04-21T16:26:31Z

Happy to revisit it if circumstances change for sure.

ahmedcharles · 2017-04-22T02:18:07Z

src/TLuaInterpreter.cpp

-        lua_pushstring( L, tr( "raiseEvent: NULL Host pointer - something is wrong!" )
-                        .toUtf8().constData() );
+        lua_pushstring(L, QStringLiteral("raiseEvent: NULL Host pointer - something is wrong!")
+                       .toLatin1().constData());


Why are we using Qt types here at all? lua_pushstring(L, "raiseEvent: NULL Host pointer - something is wrong!"); will work exactly the same way in this case, so using Qt types just makes things more complex for no benefit.

I think that makes sense, because we don't want a QString for Lua, and we're not dealing with UTF-8 here.

😊 Humm, yes, that does seem to be the case for a fair proportion of the strings, gonna have to review what I dun!

ahmedcharles · 2017-04-22T02:22:46Z

src/TLuaInterpreter.cpp

-                            .arg( luaL_typename( L, i ) )
-                            .toUtf8().constData() );
+            lua_pushstring(L, QStringLiteral("raiseGlobalEvent: bad argument type #%1 (boolean, number, string or nil\n"
+                                             "expected, got a %2!)")


Similarly, here, I'd rather use: https://www.lua.org/manual/5.3/manual.html#lua_pushfstring

lua_pushstring(L, "raiseGlobalEvent: bad argument type #%d (boolean, number, string or nil expected, got a %s!)", i, luaL_typename(L, i))

ahmedcharles

I apparently still suck at doing reviews instead of individual comments. :P

Since we are not intending to translate the Mudlet Lua API error messages we can avoid instantiating QStrings in many places where Latin1 text is injected straight into the Lua sub-system. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

SlySven · 2017-04-22T16:32:54Z

📖 Drat! Didn't read the Lua manual enough, used %i as the lua integer placement parameter in the lua_printfstring(\<format string>, replacement values...) when I should have used %d - got to go and replace them all...! 😊

SlySven · 2017-04-22T16:53:10Z

@ahmedcharles Do you like this better (can approve it) now?

ahmedcharles · 2017-04-22T23:04:10Z

src/TLuaInterpreter.cpp

-                            .arg( i )
-                            .arg( luaL_typename( L, i ) )
-                            .toUtf8().constData() );
+            lua_pushfstring(L, "raiseEvent: bad argument #%d type (string, number, boolean, or nil\n"


I guess I'm not sure why we are hardwrapping an error message like this, but I suppose it's not changed, so no need to fix it.

☹️ Agreed it is not ideal and I would like to revisit that at some point...

SlySven added the 1 - Ready label Apr 21, 2017

SlySven self-assigned this Apr 21, 2017

SlySven requested review from ahmedcharles and vadi2 April 21, 2017 09:24

SlySven mentioned this pull request Apr 21, 2017

Added internalisation do's and don't #927

Merged

vadi2 approved these changes Apr 21, 2017

View reviewed changes

This was referenced Apr 21, 2017

I18n: ensure Lua echo and feedTriggers commands pass Utf-8 #939

Merged

Added saveProfile() #936

Merged

ahmedcharles reviewed Apr 22, 2017

View reviewed changes

ahmedcharles suggested changes Apr 22, 2017

View reviewed changes

vadi2 approved these changes Apr 22, 2017

View reviewed changes

Revise: fixup use of %i in lua_pushfstring(...) when it should be %d

ac2c68d

SlySven mentioned this pull request Apr 22, 2017

centerview on non-existant map causes crash to desktop #945

Closed

ahmedcharles approved these changes Apr 22, 2017

View reviewed changes

SlySven merged commit 014b668 into Mudlet:development Apr 23, 2017

SlySven deleted the I18b_clearOutTranslateableLuaErrorMesssages branch April 23, 2017 00:59

SlySven restored the I18b_clearOutTranslateableLuaErrorMesssages branch June 22, 2020 17:59

SlySven deleted the I18b_clearOutTranslateableLuaErrorMesssages branch June 22, 2020 18:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

I18n: clear out translatable Lua error messsages#941

I18n: clear out translatable Lua error messsages#941
SlySven merged 3 commits intoMudlet:developmentfrom
SlySven:I18b_clearOutTranslateableLuaErrorMesssages

SlySven commented Apr 21, 2017

Uh oh!

vadi2 left a comment

Uh oh!

SlySven commented Apr 21, 2017

Uh oh!

vadi2 commented Apr 21, 2017

Uh oh!

ahmedcharles Apr 22, 2017

Uh oh!

vadi2 Apr 22, 2017

Uh oh!

SlySven Apr 22, 2017

Uh oh!

ahmedcharles Apr 22, 2017 •

edited

Loading

Uh oh!

ahmedcharles left a comment

Uh oh!

SlySven commented Apr 22, 2017

Uh oh!

SlySven commented Apr 22, 2017

Uh oh!

ahmedcharles Apr 22, 2017

Uh oh!

SlySven Apr 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

SlySven commented Apr 21, 2017

Uh oh!

vadi2 left a comment

Choose a reason for hiding this comment

Uh oh!

SlySven commented Apr 21, 2017

Uh oh!

vadi2 commented Apr 21, 2017

Uh oh!

ahmedcharles Apr 22, 2017

Choose a reason for hiding this comment

Uh oh!

vadi2 Apr 22, 2017

Choose a reason for hiding this comment

Uh oh!

SlySven Apr 22, 2017

Choose a reason for hiding this comment

Uh oh!

ahmedcharles Apr 22, 2017 • 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!

ahmedcharles left a comment

Choose a reason for hiding this comment

Uh oh!

SlySven commented Apr 22, 2017

Uh oh!

SlySven commented Apr 22, 2017

Uh oh!

ahmedcharles Apr 22, 2017

Choose a reason for hiding this comment

Uh oh!

SlySven Apr 23, 2017

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ahmedcharles Apr 22, 2017 •

edited

Loading