BugFix: multiple issues with TKey creation from lua script commands by SlySven · Pull Request #1069 · Mudlet/Mudlet

SlySven · 2017-06-04T23:24:02Z

Bug fixes for issue(s) raised in: #1068

in TLuaInterpreter::permKey(...) corrected luaL_typename(...) calls to refer to correct argument instead of hard-coded 1.
in TLuaInterpreter::permKey(...) and tempKey(...) corrected tests for strings for optional third and required fourth to be the number ones as that is what we want them to be interpreted as; edited the type error message to match; revised the error message to the keyModifier argument to refer to it as optional as it is!
in TLuaInterpreter::tempKey(...) forced the error message generation to no longer be subject to potential translation efforts following the decision already implemented elsewhere in the class NOT to I18n these texts.
in TLuaInterpreter::permKey(...) and tempKey(...) added missing argument number from error messages for cases where the number is a run-time variable rather than a compile time constant.
in TLuaInterpreter::permKey(...) corrected the swap of keyModifier and keyCode in the call to TLuaIntepreter::startPermKey(...)
in TLuaInterpreter::tempKey(...) refactored the luaScript variable to be a QString all the way through and to explicitly retrieve it from the Lua sub-system as UTF-8 text with a QString::fromUtf8(...) call whereas the previous std::string::c_str() as an argument to a plain QString() constructor will fail in the future if/when the QT_NO_CAST_FROM_ASCII macro is #define-d.
As a separate issue I found that the code that checks for TKeys being matched on user text input was defective as it only ran the first match that was found instead of running all - this needed a revision to (bool) KeyUnit::processDataStream(int, int) ...
I have run clang-format over the code that I have touched.

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

Bug fixes for issue(s) raised in: Mudlet#1068 * in TLuaInterpreter::permKey(...) corrected luaL_typename(...) calls to refer to correct argument instead of hard-coded 1. * in TLuaInterpreter::permKey(...) and tempKey(...) corrected tests for strings for optional third and required fourth to be the number ones as that is what we want them to be interpreted as; edited the type error message to match; revised the error message to the keyModifier argument to refere to it as optional as it is! * in TLuaInterpreter::tempKey(...) forced the error message generation to no longer be subject to potential translation efforts following the decision already implemented elsewhere in the class NOT to I18n these texts. * in TLuaInterpreter::permKey(...) and tempKey(...) added missing argument number from error messages for cases where the number is a run-time variable rather than a compile time constant. * in TLuaInterpreter::permKey(...) corrected the swap of keyModifier and keyCode in the call to TLuaIntepreter::startPermKey(...) * in TLuaInterpreter::tempKey(...) refactored the luaScript variable to be a QString all the way through and to explicitly retrieve it from the Lua sub-system as UTF-8 text with a QString::fromUtf8(...) call whereas the previous std::string::c_str() as an argument to a plain QString() constructor will fail in the future if/when the QT_NO_CAST_FROM_ASCII macro is #define-d. * As a separate issue I found that the code that checks for TKeys being matched on user text input was defective as it only ran the first match that was found instead of running all - this needed a revision to (bool) KeyUnit::processDataStream(int, int) ... * I have run clang-format over the code that I have touched. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

vadi2 · 2017-06-05T04:28:29Z

src/KeyUnit.cpp


 bool KeyUnit::processDataStream(int key, int modifier)
 {
+    bool isAtLeastOneMatch = false;


Sorry - but changing the core behaviour of how keybindings work is not in scope of #1068 at all nor do I want to have a discussion on changing the behaviour on the last day PRs can go into a release. Could you move this to a separate PR for the next release?

vadi2 · 2017-06-05T04:30:54Z

src/TLuaInterpreter.cpp

-    int n = lua_gettop( L );
-    int i = 3;
-
+    quint8 argIndex = 0;


C++11 adds a type for this, so we have no need to use the Qt workaround - this stuff is minor so I'll just submit a commit to your branch in the interests of saving time, let me know if you disagree!

vadi2 · 2017-06-05T04:43:57Z

This works now 😁

permKey("my key", "", mudlet.keymodifier.Control, mudlet.key.F8, [[echo("hey this thing actually works!\n")]])

So does this:

lua permKey("my key", "", mudlet.key.F8, [[echo("aaaa this thing actually works!\n")]])

This does not 😞

lua permKey("my key", "", nil, mudlet.key.F8, [[echo("hey this thing actually works!\n")]])
[  LUA  ] - Object<run lua code> Function<Alias6>
            <permKey: bad argument #3 type (key modifier as number is optional, got nil!)>

If it's optional and you give it nil... it should work!

It's far too late in the release schedule to be changing this - let's discuss it next time

vadi2

I've applied the changes I'd like to see so we can get this in quicker instead of doing the usual back and forth that goes on for days - merge if you're okay with the changes.

SlySven · 2017-06-05T14:25:28Z

If it's optional and you give it nil... it should work!

To my mind optional means "you do not have to put anything there", putting in a nil is something, even if it is nothing 😖 - remember the test to determine the situation is based on lua_gettop(...) and I think that still counts a nil {unless I guess it is at the end of a list of values} in that count.

SlySven · 2017-06-05T14:40:50Z

src/TLuaInterpreter.cpp

 int TLuaInterpreter::permKey( lua_State *L )
 {
-    quint8 argIndex = 0;
+    uint_fast8_t argIndex = 0;


It wasn't intended as a "work_around" it was just that I anticipated the maximum number of bits and was willing to be explicit about it instead of just having the generic int and the Qt types are well defined (and will likely now be typedefed to the longer typename of C++1x auto-magically anyhow)...

As it happened I was going to change the keyModifier and keyCode to be quint64 rather than intas I remember that the Qt Clang-analyser plugin does not like assigning the value of lua_tointeger(...)to it but that would be disruptive as it means changing some reference passing of arguments to other functions.

(I have the analyser switched off currently as it is a CPU hog but it gets upset because lua_interpreter(...) returns a long rather than an int on some/all platforms and that is iffy on Windows IIRC...)

SlySven · 2017-06-05T14:53:08Z

src/KeyUnit.cpp

    }

-    return isAtLeastOneMatch;
+    return false;


Hang on, the Wiki currently says:

Note: Mudlet by design allows duplicate names - so calling permKey [or tempkey] with the same name [code and modifier] will keep creating new keys. You can check if a key already exists with the exists() function.

It does not say "but only the first one of any code and modifier combination of either permanent or temporary Keys" will work and the remainder will be ignored...! So if you create a key of either type and drop in buggy lua code and then correct it and call the same command again you will have two instances and only the first, buggy one is going to be (and fail to) run. That is not conforming to "principle of least surprise" IMHO.

That is an edge case scenario. Principle of least surprise is when you press a button and don't get 50 keys going off :)

As someone who did enter lua permKey("my key", "", mudlet.keymodifier.Control, mudlet.key.F8, [[echo("hey this thing actually works!\n")]]) several times with different function strings to test it I was surprised when I didn't get several Keys going off - especially as they all showed in the Editor (after refreshing the contents).

Not sure what the prioritization is but if a script puts up a tempKey and I then enter one in the editor with the same key+modifier combination will my (effectively a permKey) ever run? IIRC the temporary ones do not show but are given priority (I could easily be rong on that I know) but that must surely be confusing even if it is a corner case... ☹️

SlySven · 2017-06-05T14:59:07Z

If we are going to roll out (as a "Squash and Merge" I think) this PR out as it is now and defer the "only the first Key+Modifier will ever be run" bug-fix then I think we should mention it as something in the Wiki as a defect that was/will be fixed in Release 3.2++

vadi2 · 2017-06-05T16:40:34Z

putting in a nil is something

You're confusing nil with NULL 😉

nil is not NULL, nil is nothing in Lua.

vadi2 · 2017-06-05T16:43:52Z

Changing the core behaviour of how keybindings have worked in Mudlet for the last 8 years will umm we can discuss it but lets hold off on calling it a "bug" for now please! We also have about zero reports from users that it's a problem, unlike the 300+ bugs we have sitting in the queue already.

vadi2 · 2017-06-05T16:45:27Z

\o/ thanks for solving the bugs on such a prompt notice! The functions look so shiny now too, conforming to the best standards :)

SlySven added 1 - Ready labels Jun 4, 2017

SlySven added this to the 3.2 milestone Jun 4, 2017

SlySven self-assigned this Jun 4, 2017

SlySven requested a review from vadi2 June 4, 2017 23:24

vadi2 requested changes Jun 5, 2017

View reviewed changes

vadi2 added 5 commits June 5, 2017 06:47

Don't have to use Qt workaround - C++11 now defines 8bit int

d54e274

Fixed casing

a61b5c4

Fix nil not being recognised as missing optional

213629b

Fix typo

2e45ac3

Undid core keybinding behaviour to go with the first match only

8b52858

It's far too late in the release schedule to be changing this - let's discuss it next time

vadi2 approved these changes Jun 5, 2017

View reviewed changes

SlySven commented Jun 5, 2017

View reviewed changes

vadi2 merged commit a0850c3 into Mudlet:development Jun 5, 2017

SlySven deleted the BugFix_permKeyBuggy branch June 5, 2017 18:47

Uh oh!

Conversation

SlySven commented Jun 4, 2017

Uh oh!

vadi2 Jun 5, 2017

Choose a reason for hiding this comment

Uh oh!

vadi2 Jun 5, 2017

Choose a reason for hiding this comment

Uh oh!

vadi2 commented Jun 5, 2017

Uh oh!

vadi2 left a comment

Choose a reason for hiding this comment

Uh oh!

SlySven commented Jun 5, 2017

Uh oh!

SlySven Jun 5, 2017

Choose a reason for hiding this comment

Uh oh!

SlySven Jun 5, 2017

Choose a reason for hiding this comment

Uh oh!

vadi2 Jun 5, 2017

Choose a reason for hiding this comment

Uh oh!

SlySven Jun 5, 2017

Choose a reason for hiding this comment

Uh oh!

SlySven commented Jun 5, 2017

Uh oh!

vadi2 commented Jun 5, 2017

Uh oh!

vadi2 commented Jun 5, 2017

Uh oh!

vadi2 commented Jun 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants