Rebased revise ensure sys event names not translated#934
Conversation
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. As it is concerned with using QLatin1String wrappers around things involving TEvent instances I have extended the re-coding to the qDebug() code provided in TEvent.h to produce a formatted and useful dump of a TEvent instance should it be passed to the Qt Debug system methods. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
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!) In the light of revisions to our code style choices - I also adopt those in the areas I have modified. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
We recently added nil and boolean type argument support to the TEvent infrastructure however it was missed from being added to labels. This will do that and also bring the coding styles recently decided upon to those Lua functions. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
ahmedcharles
left a comment
There was a problem hiding this comment.
I'm fine with the first two commits but the third commit has some translation stuff and I'm a bit confused. It seems like we decided not to do that, but perhaps I'm mistaken?
src/TLuaInterpreter.cpp
Outdated
|
|
||
| QString labelName; | ||
| if (! lua_isstring(L, 1)) { | ||
| lua_pushstring(L, tr("setLabelClickCallback: bad argument #1 type (label name as string expected, got %1!)") |
There was a problem hiding this comment.
Your commit message doesn't mention translation?
There was a problem hiding this comment.
I'm not totally on-board with not coding for it at this point and was retaining the use of the methodology that I started using some (years ?) ago, should it become necessary to rip it all out it can be done but I don't want to do that until I am certain that it is totally irrelevant/harmful.
There was a problem hiding this comment.
I'm certainly not asking you to remove any that are already in development.
I'm not saying you can't add these.
But it seems like we agreed not to translate the Mudlet API errors. These are the Mudlet API errors. Adding more because you're not on-board seems to run counter to working together, doesn't it?
There was a problem hiding this comment.
It's going counter to the feedback we've received from international users and #904, so I agree it's something we've gotta reach a conclusion on.
|
If you want, submit a PR with the first two alone and I'll approve. |
…Translated Neede to take on-board changes in error message creation (removal of ability to translate them; change away from QString and related classes to produce them).
Changes made in the series of commits making up this pull-request will clash with other changes in TLuaInterpreter in a separate but now merged pull-request initiated after this one was started. By merging in the other one and revising the changes in the third of the original commits in this it will now be possible to merge the result back in to the development branch without conflicts. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
|
Would you mind submitting the first two commits as a separate PR? There's no way to get this to have three commits using the web ui at this point. |
|
@vadi2 @ahmedcharles do I have to re-base and do another (or more than one) PR again? 😢 |
|
Maybe @ahmedcharles could help you sort it out. I'm happy with the code and can put up with the imperfect history. Not an expert in Git so doing any complex history rewriting besides commit amending is beyond me. |
|
I think I will take another stab at it and split the bits up... |
|
Right I have three commits onto the current (at the time of writing but the goal-posts move around rapidly nowadays 😉) development that do what was intended. As I see it I could force an overwrite to my Repo which will rewrite the history here but will look clean when welded into the branch. Or I can close this one as well and put them out as a third PR. I am minded to go the second route. |
|
I just want it in, this is dragging on.
…On Fri, 28 Apr 2017 10:51 pm Stephen Lyons, ***@***.***> wrote:
Right I have three commits onto the current (at the time of writing but
the goal-posts move around rapidly nowadays 😉) *development* that do
what was intended. As I see it I could *force* an overwrite to my Repo
which will rewrite the history here but will look clean when welded into
the branch. Or I can close this one as well and put them out as a third PR.
I am minded to go the second route.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#934 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAGxjGavDPz-Gj611LXX3z61gUT18Fytks5r0lFLgaJpZM4NCZLP>
.
|
These three commits are the cleaned up series of PR #911 organised as was wanted. For the full gory details I have to refer you to that PR which I am about to close in favour of this one; apart from some spaces {and possibly some
return lua_error(L);s now used to quieten a coverity(?) warning} the net effect should be the same.I won't ask @vadi2 to review it again - but I'll mention you to keep you in the loop. 😄