Revise: clean up some TEvent related stuff by SlySven · Pull Request #911 · Mudlet/Mudlet

SlySven · 2017-04-16T23:11:47Z

Ensure all system generated events have event names that are NOT subjected to translation. However as I have recently learned that QLatin1String is a better choice in most circumstances as opposed to QStringLiteral I have changed to use them instead.

Rename some TEvent instances where our convention on naming suggested the data structure reference was a pointed to a TEvent rather than the TEvent itself (i.e. use of pE - at some stage in the past we were using pointers!)

Revamp TLuaInterpreter::setLabelClickCallback, setLabelReleaseCallback, setLabelOnEnter and setLabelOnLeave to handle event arguments of boolean and nil types - as we have expanded the range for TEvents everywhere else. Also now returns a boolean true on success to find the given label or nil and an "error message" on various run-time failures - including not finding the label.

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

Ensure all system generated events have event names that are NOT subjected to translation. However as I have recently learned that QLatin1String is a better choice in most circumstances as opposed to QStringLiteral I have changed to use them instead. Rename some TEvent instances where our convention on naming suggested the data structure reference was a pointed to a TEvent rather than the TEvent itself (i.e. use of pE - at some stage in the past we were using pointers!) Revamp TLuaInterpreter::setLabelClickCallback, setLabelReleaseCallback, setLabelOnEnter and setLabelOnLeave to handle event arguments of boolean and nil types - as we have expanded the range for TEvents everywhere else. Also now returns a boolean true on success to find the given label or nil and an error message on various run-time failures - including not finding the label. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

SlySven · 2017-04-16T23:16:17Z

Having been made aware in #900 that system events should not have names that are subjected to translation made me want to go through the rest of them and fix the same point. Whilst doing this I became aware that the four label related events in TLuaInterpreter did not have support for nil or bool type arguments that all the rest of the TEvent infrastructure can handle. It was also clear that they were ripe for overhaul in terms of error handling/reporting so I have done them over here!

I have taken on board the preference to use QLatin1String as well and adopted that here.

ahmedcharles

Perhaps we could split this into commits which:

rename variables
use QLatin1String/QStringBuilder
the changes in TLuaInterpreter (since I'm not quite sure what they do, so could use more explanation).

Conflicts resolved in: src/TEvent.h Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

Somehow I missed a small bit of the diff output markings when I used meld. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

ahmedcharles · 2017-04-17T03:05:46Z

src/TEvent.h

                case ARGUMENT_TYPE_BOOLEAN:
-                    result.append( QLatin1String("[") % QString::number(i) % QLatin1String("{bool}") % (event.mArgumentList.at( i ).toInt()?QLatin1String("true"):QLatin1String("false")) % QLatin1String("]") );
+                    result.append(QLatin1String("[") % QString::number(i) % QLatin1String("{bool}") % (event.mArgumentList.at(i).toInt() ? QLatin1String("true") : QLatin1String("false")) % QLatin1String("]"));
                    break;


This probably doesn't build? :)

ahmedcharles · 2017-04-17T03:06:30Z

src/TEvent.h

@@ -52,44 +52,44 @@ inline QDebug& operator<<(QDebug& debug, const TEvent& event)
    const int typeCount = event.mArgumentTypeList.count();
    int i = 0;
    QString result = QLatin1String("TEvent(");


This is supposed to be } else { from the merge conflict.

… skip] Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

vadi2

Hey I don't think it worked, here was my test case:

local width, height = getMainWindowSize()
createLabel("messageBox",(width/2)-300,(height/2)-100,250,150,1)
resizeWindow("messageBox",500,70)
moveWindow("messageBox", (width/2)-300,(height/2)-100 )
setBackgroundColor("messageBox", 255, 204, 0, 200)
echo("messageBox", [[<p style="font-size:35px"><b><center><font color="red">You are under attack!</font></center></b></p>]] )
showWindow("messageBox")

-- you can also make it react to clicks!
mynamespace = {
  messageBoxClicked = function(...)
    echo("hey you've clicked the box!\n")
    display{...}
  end
}
 
setLabelClickCallback("messageBox", "mynamespace.messageBoxClicked", 1,2,3.14, true)

Results of display normally:

 display{1,2,3.14, true}
{
  1,
  2,
  3.14,
  true
}

Results from the callback:

hey you've clicked the box!
{
  1,
  2,
  0
}

vadi2 · 2017-04-17T04:54:01Z

src/TLuaInterpreter.cpp


    QString localFileName = downloadMap.value( reply );
-    TEvent e;
+    TEvent event;


👍 meanigful variable names never hurt anybody. Glad to see this improvement to lower the cognitive load.

I had to choose mudletEvent in a couple of places because there was already a Qt event in the same code. 😆

vadi2 · 2017-04-17T04:54:49Z

src/TLuaInterpreter.cpp

-            e.mArgumentTypeList << ARGUMENT_TYPE_STRING;
-            e.mArgumentList << tr( "unableToOpenLocalFileForWriting", "This string might not need to be translated!" );
-            e.mArgumentTypeList << ARGUMENT_TYPE_STRING;
+            event.mArgumentList << QStringLiteral("sysDownloadError");


I'm sure you had a good reason - but how come this is QStringLiteral here when there's no UTF-8? QLatin1String was used above for the event name okay.

Curious, that seems to be one that escaped...!

🚧 I'll go and nail the right thing in there right away. 🚧

Sorted now...

vadi2 · 2017-04-17T05:33:43Z

src/TLuaInterpreter.cpp

+
+    QString labelName;
+    if( ! lua_isstring( L, 1 ) ) {
+        lua_pushstring( L, tr( "setLabelClickCallback: bad argument #1 type (label name as string expected, got %1!)" )


If we translate error messages, people in those languages are doing to be at a disavantage because everyone else who doesn't understand their language won't be able to help them. This does not seem like a win for me to put non-English users at a disadvantage like this.

Thoughts @keneanung?

Well we do have some established "patterns" for the text before the (...) - and, even I do not now anticipate translating the function names so from a "support" point of view some key word recognition (for "value" and "type" along side the argument number) will probably go a long way to determining the sort of issues a user is having. Of course, the users who do work out from an error message (in their own language) what the problem is, they will most likely not be the ones seeking help from others!

With the "Show Lua errors in console" option these will show up in the main screen will they not?

The Mudlet enthusiast who rolls their own Mudlet from source will have access to the .ts files as part of the source code bundle so it will be possible for those interested in getting into cross-language support to fire up Qt Linguist and cross-check what other language users will see for each error message - in the same manner as coders can dive into Qt Creator and poke around in the source code.

As for not understanding the language putting off those who want to help - will we demand all Forum posts to be in English from the people originally seeking help. It would be nice from a support point of view but I'd be surprised if it works out like that...!

Tagging @ftpd, Polish Mudlet user - what're your thoughts on translating API error messages?

@ecam85 as well

My opinion is that there are Mudlet users and Mudlet developers (obviously, one is a subset of the other). Mudlet developers, like all developers, probably have to know enough English to write code. Lua isn't translated and will give English error messages for the same types of errors that are happening here and Lua is Brazilian... Are you suggesting that we should show users error messages in multiple languages at the same time? Or are you suggesting we should translate all Lua messages instead?

I'd leave error message in English as well, simply because it makes it easier to search for them.

You can also translate them, but that should be a different setting from the main GUI, because some may want to keep their main GUI in their native language and use English errors for easier searching.

Sadly I think that users could - initially be seeing mixed language message at the same time, for example I turned on showing Lua error messages to the main console and tried to get some errors:

As I see it, in the above the various strings "centerview: ..." would be in the user's language; actually so could the "[ XXXX ]" ones I think.

To be fair the function names are going to stick to being English with the current consensus of opinion and the Lua ones, like the fragment that says:

.... ":1: unexpected symbol near
'<eof>'>

will be in English as that comes from Lua and not from Mudlet - however from the user (not developer) point of view do they want to see everything in English or is it reasonable to at least give Mudlet related problems (which are going to be related to OUR project, and likely only to this project) in their language even if general/common Lua issues (which many more groups of people, using many languages around the 🌏 could/can have) get reported in English only?

From your screenshot, the messages at the top about the lua api probably shouldn't be there, but they shouldn't be translated because they are messages for developers. If you aren't going to write code, then you don't really care if the sqlite3 module is loaded or not.

The green messages about the connection should get translated because they are in fact, user messages that the user wants to see.

Everything after that is a developer running lua code and all of those should be in English.

I'm starting to be concerned that we're getting too much into a guessing game of what we think the users want and don't want. Let's put out a Mudlet that can actually work with other languages first and translate the GUI because those are obvious desired features, and leave the rest - such as enablement of API translation - for later, just like it is on the roadmap. Once we get Mudlet into the hands of non-English users, we'll see given mass feedback if it's necessary.

tl:dr; lets not clutter (and slow down with unneeded hash lookups) mudlet with tr everywhere right now except GUI and we'll revisit this in the future.

I'll try to make this my last comment on this sub-topic, but the whole idea about hash lookups was for them to be fast or so I thought! It is easier for me as a coder to formulate as much of text stuff that might need to be translated as I develop it and then comment it as something that might not need it rather than have to revisit it later and insert the coding than - it is stronger in my mind in the first case and should something later prove to not be required to go through translation the, now somewhat depreciated, QStringLiteral will work as a temporary means to remove such items fairly simply until a better but more convoluted QLatin1String form can be made!

vadi2 · 2017-04-17T05:35:12Z

src/TLuaInterpreter.cpp

-    {
-        luaName = lua_tostring( L, 2 );
+    } else {
+        labelName = QString::fromUtf8( lua_tostring( L, 1 ) );


Just checking - is this with the merged #881 applied? I think the idea is that we start running it on the code we change now, yeah?

I'm trying to adopt the new style to reduce work going forward - trying to put a space between the if and the following (for example. I think it might help when something I change is in an area that subsequently gets clang-formatted in a different PR between me creating the branch with my change on and when it get approved for merging...

vadi2 · 2017-04-17T05:55:04Z

On splitting the commits - I agree this should have been 3 separate PRs, but don't worry about redoing the work you've already done now. Just something for the future.

Somehow I forgot four instances in TLuaInterpreter::slot_replyFinished! Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

SlySven · 2017-04-17T15:24:43Z

Vadim wrote:

Hey I don't think it worked, here was my test case:

It works for me, using code that also includes a release:

I get this:

SlySven · 2017-04-17T15:36:41Z

It is possible now to pass nil arguments in these handlers but it can be tricky to recover them from a varadic handler function; when they are a value in a table they are hard to distinguish from the absence of the key with that value and turning the arguments from such a function into a table is one way that is used to process varadic arguments!

📚 this explained it to me in more detail...

vadi2 · 2017-04-17T15:39:14Z

You're right, it works fine, I used the wrong branch.

vadi2 · 2017-04-17T15:40:04Z

Just awaiting feedback from international users on translating error messages - so we have these things patten down as we move forward with i18n.

vadi2

All of the feedback we've been getting so far from international users was that code and anything related should stay in English (on our forums, in other PRs) and so on, so I think the tr() on error messages is superflous. Nonetheless want to get this progressing along so I'll approve the PR, it does what it needs to do.

SlySven · 2017-04-19T16:02:58Z

@ahmedcharles do you feel the urge to turn that red cross into a green tick?

ahmedcharles · 2017-04-19T19:35:19Z

My feedback was at a high level:

Perhaps we could split this into commits which:

rename variables
use QLatin1String/QStringBuilder
the changes in TLuaInterpreter (since I'm not quite sure what they do, so could use more explanation).

That hasn't happened and there's no explanation as to why it hasn't (unless I missed it).

SlySven · 2017-04-20T00:22:26Z

Closing in favour of the cleaned-up/rebased #934 which should be the same end result but look nicer in the commit history...

SlySven added the 1 - Ready label Apr 16, 2017

SlySven self-assigned this Apr 16, 2017

SlySven requested review from ahmedcharles and vadi2 April 16, 2017 23:11

SlySven mentioned this pull request Apr 16, 2017

Replace QStringLiteral with QLatin1String where UTF-8 is not needed #907

Closed

ahmedcharles suggested changes Apr 17, 2017

View reviewed changes

SlySven added 2 commits April 17, 2017 03:52

Merge branch 'development' into Revise_ensureSysEventNamesNotTranslated

deb31f6

Conflicts resolved in: src/TEvent.h Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

BugFix: remove remnant markup from manual merge resolution

028bf82

Somehow I missed a small bit of the diff output markings when I used meld. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

ahmedcharles reviewed Apr 17, 2017

View reviewed changes

Tweak: missed layout issue when manually resolving merge conflict [ci…

f441668

… skip] Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

vadi2 requested changes Apr 17, 2017

View reviewed changes

Fixup: convert some previously missed QStringLiteral's to QLatin1String

469f278

Somehow I forgot four instances in TLuaInterpreter::slot_replyFinished! Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

vadi2 approved these changes Apr 19, 2017

View reviewed changes

SlySven mentioned this pull request Apr 20, 2017

Rebased revise ensure sys event names not translated #934

Merged

SlySven closed this Apr 20, 2017

SlySven deleted the Revise_ensureSysEventNamesNotTranslated branch June 22, 2020 18:54

Uh oh!

Conversation

SlySven commented Apr 16, 2017

Uh oh!

SlySven commented Apr 16, 2017

Uh oh!

ahmedcharles left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadi2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadi2 commented Apr 17, 2017

Uh oh!

SlySven commented Apr 17, 2017

Uh oh!

SlySven commented Apr 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vadi2 commented Apr 17, 2017

Uh oh!

vadi2 commented Apr 17, 2017

Uh oh!

vadi2 left a comment

Choose a reason for hiding this comment

Uh oh!

SlySven commented Apr 19, 2017

Uh oh!

ahmedcharles commented Apr 19, 2017

Uh oh!

SlySven commented Apr 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

SlySven commented Apr 17, 2017 •

edited

Loading