Added tempKeys, & lua hooks for tempKeys/permKeys by SecareLupus · Pull Request #472 · Mudlet/Mudlet

SecareLupus · 2017-03-30T03:08:27Z

I think a file slipped in where I was just cleaning up some indentations, as well. The new API mirrors the existing API for Alias(s), etc.

vadi2 · 2017-03-30T10:15:52Z

Awesome, thanks! Will review this in the next couple of days. We've improved on some code guidelines since the alias code was written, but they haven't been applied to Alias yet so no complaints from me in that regard!

SlySven · 2017-03-30T11:26:23Z

src/KeyUnit.cpp


+bool KeyUnit::killKey( QString & name ) {
+    typedef list<TKey *>::const_iterator I;
+    for( I it = mKeyRootNodeList.begin(); it != mKeyRootNodeList.end(); it++)


Yeah, since we've adopted C++11 you can now use the auto keyword to avoid the typedefs... 😀

SlySven · 2017-03-30T11:31:39Z

src/KeyUnit.cpp

    else return QString("undefined key");
 }

+void KeyUnit::initStats()


You've obviously seen similar code in a different Mudlet "item" class and taken the initiative to replicate the stats reporting stuff. Kudos for doing it, though we may want to rethink this if we get around to doing something like this for all of "Buttons/Menus/Toolbars; Keys, Aliases, Timers, Triggers..." err "Scripts" and "Variables"... 😮

SlySven · 2017-03-30T11:36:44Z

src/TLuaInterpreter.cpp

+    {
+        luaSendText = lua_tostring( L, 1 );
+    }
+    Host * pHost = TLuaInterpreter::luaInterpreterMap[L];


We have some different boiler-plate now that checks for a null pHost and returns an (error) string before produced a lua_error( L ). {Until very recently yours truely 😊 was going for a lua nil+error message response but that is not the pattern I should have used.}

SlySven · 2017-03-30T11:40:21Z

src/TLuaInterpreter.cpp

+    }
+    else
+    {
+        luaSendText = lua_tostring( L, 1 );


In the medium term, we will replace these std::string capturing lines with QString ones and replacing the cargo-cult "luaSendText" with more meaningful local names. In this case I'd suggest using a QString:

itemName = QString::fromUtf8( lua_tostring( L, 1 ) );

SlySven · 2017-03-30T11:43:44Z

src/TLuaInterpreter.cpp

+    string luaSendText="";
+    if( ! lua_isstring( L, 1 ) )
+    {
+        lua_pushstring( L, "killKey: wrong argument type" );


To support the I18n work for Mudlet 4.0 you will see later code in this class containing more informative (and translatable should we go that way) error code such as:

lua_pushstring( L, tr( "killKey: bad argument #1 type (key name as string expected, got %1!)" ) .arg( luaL_typename( L, 1 ) ) .toUtf8().constData() );

While looking into this, I also found on line 3201 the following variant. (I typed it in, as vmware stopped sharing my clipboard. this might not be exactly the line, but more or less)

lua_pushfstring( L, tr( "startLogging: bad argument #1 (turn logging on/off, as boolean expected, got %s)", luaL_typename(L,1) ).toUtf8().constData() );

This syntax seems more clear to me, but I assume there's a good reason to use the .arg() call instead? If so, I can fix that line while I'm at it.

I don't see why can't we use lua_pushfstring() - @SlySven ? I'd prefer it over having to wrap things in .arg().

alright, I've committed one with the .arg syntax, since that's what was suggested when I'd started. I can switch that over pretty easily though, if you'd like.

I am not sure that lua_pushfstring(...) will work here, tr("string") needs .args() to supply the replacement parameters %1..%99 - the only other valid replacement string is (I have recently discovered) a %n which uses the numeric value as a third argument to tr("string","context",integer) to select the right plural/alternative text form to use in conjunction with the actual number positioned where the %n is, some languages have up to three!

E.g. English has “0 rooms”, “1 room”, “2 rooms”
But French has «0 pièce», «1 pièce», «2 pièces»

And leaving raw strings around (Qt assumes Utf8 not ASCII source files but I am not sure about other parts of the process) will flag as issues when we start looking for the bits that have to be translated as opposed to internal stuff that does not - though to be honest we have not had the discussion of how we will get all multi-lingual for Mudlet 4.0 {though I have some strong feelings about part of it - but we will need to colour some of the details in!}

For sure. I'll start a thread on the forums so we can invite others to the discussion.

SlySven · 2017-03-30T12:24:19Z

src/TLuaInterpreter.cpp

+        luaSendText = lua_tostring( L, 1 );
+    }
+    Host * pHost = TLuaInterpreter::luaInterpreterMap[L];
+    QString text(luaSendText.c_str());


As I've suggested a modification to capture the (unicode) QString directly this line can go and the following would become:

lua_pushboolean( L, pHost->getKeyUnit()->killKey( itemName ) );

Hope these suggestions are of use...

SlySven · 2017-03-30T12:25:14Z

src/TLuaInterpreter.cpp

    TLuaInterpreter * pLuaInterpreter = pHost->getLuaInterpreter();
    QString _luaFunction = luaFunction.c_str();
    QString _luaRegex = luaRegex.c_str();
-    int timerID = pLuaInterpreter->startTempAlias( _luaRegex, _luaFunction );


Huh, was that there...! Well spotted.

SlySven · 2017-03-30T12:31:10Z

src/TLuaInterpreter.cpp

+    else if( type == "keybind")
+    {
+        QMap<QString, TKey *>::const_iterator it1 = pHost->getKeyUnit()->mLookupTable.find( name );
+        while( it1 != pHost->getKeyUnit()->mLookupTable.end() && it1.key() == name )


🤔 If, as I suspect, pHost->getKeyUnit()->mLookupTable is a QMap<QString, TKey *> can not the iteration loop be collapsed to:

cnt += pHost->getKeyUnit()->mLookupTable.count( name );

There is an (int)QMap::count(const Key &key) const method after all...

Actually that pHost->getKeyUnit()->mLookupTable may be a QMultiMap<QString, TKey *> as a QMap does not normally support multiple entries with the same key (unless QMap::insertMulti( const Key &key, const T &value ) was used to populate items with the same key. The QMap::const_iterator can handle either type...

As it turns out, exists() doesn't work for keybinds right now. I thought I'd sorted that out, but just tested it, and it fails. So your suggestions may solve the problem I'm running into also.

They did end up being QMultiMaps, but count() still worked as expected.

It seems that KeyUnit is not using mLookupTable in the same way that AliasUnit does, and since exists() relies on mLookupTable, that's the reason I can't get exists() to work with keybinds.

Couple things yet to try, but I should find out why AU and KU are tracking their children in different ways first, because I don't want to accidentally break something.

SlySven · 2017-03-30T12:48:36Z

src/TLuaInterpreter.cpp

+        luaKeyCode = lua_tointeger( L, 3 );
+    }
+
+    int luaModifier;


We may need to document the integer values for this on the Wiki - they are Qt defined constants:

enum Qt::KeyboardModifier
flags Qt::KeyboardModifiers
This enum describes the modifier keys.

Constant Value Description Qt::NoModifier 0x00000000 No modifier key is pressed. Qt::ShiftModifier 0x02000000 A Shift key on the keyboard is pressed. Qt::ControlModifier 0x04000000 A Ctrl key on the keyboard is pressed. Qt::AltModifier 0x08000000 An Alt key on the keyboard is pressed. Qt::MetaModifier 0x10000000 A Meta key on the keyboard is pressed. Qt::KeypadModifier 0x20000000 A keypad button is pressed. Qt::GroupSwitchModifier 0x40000000 X11 only. A Mode_switch key on the keyboard is pressed.

Note that on MacOs that Ctrl is the "Command" or "⌘" key...

SlySven · 2017-03-30T12:51:33Z

src/XMLexport.cpp

@@ -555,8 +555,10 @@ bool XMLexport::writeHost( Host * pHost )
            if( ! (*it) || (*it)->mModuleMember) {


Can include the temp key check here instead as:

if( ! (*it) || (*it)->isTempKey() || (*it)->mModuleMember) {

SlySven

Nothing critically against this PR just some suggested improvements...

SecareLupus · 2017-03-30T15:38:12Z

Thanks for the suggestions, I'll see what I can do about cleaning things up. C++ is not my best language, so I was heavily pulling from code I recognized to be similar, and fell into some pitfalls as a result. Some of the style issues (the auto thing in particular) came because I was basing my code off of the 3.0.0 tag, and then later moved my changes to the development branch.

In any case, I'll attack some of these later today, and see if I can commit something a bit cleaner.

Edit: I was actually curious about style guidelines, as I saw many inconsistencies among similar code sprinkled about. If there is an official style guideline file, I'd be happy to start working on getting some more of the code into compliance with it.

vadi2 · 2017-03-30T16:02:58Z

We need to write an official one up and also sort out our .clang-format. It's a bit word of mouth right now - sorry. It's on the TODO list we're getting through!

vadi2 · 2017-03-31T20:31:38Z

Tagging #628

…nto development

vadi2 · 2017-04-01T16:28:00Z

@SecareLupus tag the PR with 'Ready' when you'd like it reviewed. Thanks for working on this!

…nto development

SecareLupus · 2017-04-04T04:37:04Z

I don't think I can actually tag this pull request. Not sure how to, at least.

SecareLupus · 2017-05-28T17:57:07Z

I couldn't see an easy way to iterate over the names and values of an enum in c++, only the values, so I couldn't find a way to populate the keys in the lua list.

Also, wouldn't the lua execution have the same speed regardless of whether we're reading from a file or building the string from scratch? The only difference in execution time should be the sourcing of the string we're executing in the lua interpereter. With this in mind, wouldn't it be faster to read hard-coded values from a lua script file than to iterated over the keys and values of a list, building a string from them? Between not seeing how to build the string, and my assumption that it would be more programmatically costly to do so, I thought this was the most elegant way to handle the issue.

Unrelatedly, I'm not sure what

"number of arguments passed to the Lua function"

is in reference to.

vadi2 · 2017-05-28T18:02:29Z

Github screwed up the rendering... it was a comment on int n = lua_gettop( L ); //Number of values being passed in lua_State *L.

vadi2 · 2017-05-28T18:04:06Z

Try http://blog.qt.io/blog/2008/10/09/coding-tip-pretty-printing-enum-values/, does that let you get the name as well as the value?

vadi2 · 2017-05-28T18:08:21Z

Resolved some more conflicts that happened again, though it was due to a pretty awesome commit a975a93 which allows coroutines in Mudlet so it's forgivable.

SecareLupus · 2017-05-29T00:06:53Z

That link is using QMetaEnum to access the info. The QMetaEnum object is being passed from a QMetaObject. I'm not sure how to generate a QMetaQbject to do that though, and if we're spending all these cycles iterating through an emum and building a string to pass to the lua interpreter, what's the benefit to building that string on the fly, vs having it prebuilt and in a .lua file? I suppose if Qt::Key changes, it wouldn't need to be updated, but mudlet doesn't really support every item in the Qt::Key enum as it is. KeyUnit::setupKeyNames() only creates an array with a subset of keycodes.

vadi2 · 2017-05-29T04:44:34Z

KeyUnit::setupKeyNames() should in the future! But okay, you're off the hook, we'll leave it be as-is for now.

vadi2 · 2017-05-29T04:45:32Z

I've simplified the loading since we'll be using this method instead, when I gave the advice I was thinking of the autogenerated string.

By the way I think the new C++11 keyword constexpr would help us do the calculation once at compile time so it wouldn't always be a runtime cost - something to keep in mind.

vadi2

Great work! Works perfect:

tempKey(mudlet.key.F8, [[echo'hi']])
tempKey(mudlet.keymodifier.Control, mudlet.key.F8, [[echo'hello']])

As well as the other functions that have been added.

@SlySven over to you to tick!

SlySven

Some clean-up work (with clang-format), some revising for non-translatable lua error messages and elimination of redundant intermediate QString variables in TLuaInterpreter to do I think...

SlySven · 2017-06-03T21:45:15Z

src/KeyUnit.cpp

    return found;
 }

+bool KeyUnit::killKey( QString & name ) {


📝 We now aim to have function opening braces on the next line but it may not yet be the case for all our source code - no need to worry about it though - it will get cleaned up at some stage in the near future.

SlySven · 2017-06-03T21:49:18Z

src/KeyUnit.cpp

@@ -227,10 +266,16 @@ void KeyUnit::addKey(TKey* pT)

 void KeyUnit::removeKey(TKey* pT)


💭 This is functionally very similar to (void) KeyUnit::removeKeyRootNode(TKey*) so that could just call this one and then do the one extra line of code - but that does not matter for this PR,

SlySven · 2017-06-03T21:51:58Z

src/KeyUnit.cpp

+    msg << "Keys current total: " << QString::number(statsKeyTotal) << "\n"
+        << "tempKeys current total: " << QString::number(statsTempKeys) << "\n"
+        << "active Keys: " << QString::number(statsActiveKeys) << "\n";
+        /*<< "active Aliass max this session: " << QString::number(statsActiveAliassMax) << "\n"


If there is not an equivalent TKey related statistic then these comments should not be left here - and if there are then they ought to be edited to be appropriate IMHO - even if left commented out for the moment.

SlySven · 2017-06-03T22:00:59Z

src/TLuaInterpreter.cpp

 int TLuaInterpreter::enableKey( lua_State *L )
 {
-    string luaSendText="";
+    string luaNameID = "";


I believe this should be a QString by now as we pass it as one of those to host.getKeyUnit()->enableKey(...) further down anyhow.

Also there is no need to have a "lua" prefix for the variable name (which has been derived from the generic luaText that has been mindlessly copied and pasted between a whole load of functions) - keyName is what I would have used - but that is just my feeling on the subject! 🙊

SlySven · 2017-06-03T22:23:28Z

src/TLuaInterpreter.cpp

    if( ! lua_isstring( L, 1 ) )
    {
-        lua_pushstring( L, "enableKey: wrong argument type" );
+        lua_pushstring( L, tr( "enableKey: bad argument #1 type (key name as string expected, got %1!)" )


This may be one source of the current re-basing conflicts and we are no longer planning to translate error messages so these THREE new lines should instead be:

lua_pushfstring(L, "enableKey: bad argument #1 type (key name as string expected, got %s!)", luaL_typename(L, 1));

and the following pair of lines are also refactored to avoid a moan from Coverity (or Codacy, can't remember which!) about an unreachable line (the return 1; one - which is never executed as lua_error(L) never returns - as it does a system longjmp() instead):

return lua_error(L);

SlySven · 2017-06-03T22:26:39Z

src/TLuaInterpreter.cpp

    }
    Host& host = getHostFromLua(L);
-    QString text(luaSendText.c_str());
+    QString text(luaNameID.c_str());


This line is now redundant as we already have it as a QString, and the following line uses keyName instead of text! 🤓

This sort of thing also should be done in the remaining changed TLuaInterpreter functions...

SlySven · 2017-06-03T22:33:06Z

src/XMLexport.cpp

            }
-            if ((*it)->mModuleMember) {
-                if (!writeKey(*it)) {
+            if( ! (*it)->isTempKey() && (*it)->mModuleMember ) {


If you have clang-format available and you are using a recent Qt Creator as your IDE you ought to use that "beautifier" on the functions you have edited - that should make the layout more closely match the layout we are trying to adopt throughout our C++ code now - and make the number of lines of things that have changed smaller... 🤓

SlySven · 2017-06-03T22:33:54Z

src/mudlet-lua/lua/KeyCodes.lua

+  ["Meta"] = 0x10000000,
+  ["Keypad"] = 0x20000000,
+  ["GroupSwitch"] = 0x40000000,  
+}


Needs a new-line at the end of the file I think!

Also, if you are adding to the set of files that we need to distribute like this (and its contents could instead be included inside one of the existing ones IMVHO) then please also add this file to the relevant list {the LUA.files one} in the main qmake project file (i.e. /src/src.pro) as well otherwise it may get missed by one of the installers...

SlySven · 2017-06-03T22:45:42Z

src/dlgPackageExporter.cpp

-    QTreeWidgetItem* top = items.first();
-    for (it = tList.begin(); it != tList.end(); it++) {
-        TKey* pChild = *it;
+    list<TKey *>::const_iterator it;


The first six of these nine lines of change seem entirely down to white space differences where the new one does not seem to match our code style - they ought to be restored either by hand or using the Clang-format beautifier on this chunk of code IMHO!

vadi2 · 2017-06-04T04:57:34Z

Ah man, don't wait until the last minute to give all of that! @SecareLupus I'll do the requested changes in the interests of time so we get this in 😁

vadi2 · 2017-06-04T05:20:06Z

OK - I've applied all the changes. They're pretty minor so I assume no other feedback was necessary, will merge this tomorrow unless there are any other unmentioned concerns!

SlySven

...will merge this tomorrow unless there are any other unmentioned concerns!

Well the things about using a QString marked as a candidate for translation as an error message AND use of an unnecessary std::string for something that gets changed to a QString before being passed to the C++ code in TLuaInterpreter::enableKey() also applies equally to the disableKey() one case - and there are similar things with the other TLuaInterpreter functions that get doctored/added here I am afraid... 😞

@vadi2 I'll give it my approval as I can see you will want to clear those things up and get it in.. 😄

vadi2 · 2017-06-04T07:42:06Z

@SecareLupus it's in! You've taken on a pretty hefty chunk and got it done, that's awesome for the first go!!

This'll make it into Mudlet 3.2 in the next couple of days and then you could use it in your scripts 😃 if you'd like to make any more improvements, feel free to come again! There's a ton of things to improve here.

Added tempKeys, & lua hooks for tempKeys/permKeys

f91858d

SlySven reviewed Mar 30, 2017

View reviewed changes

SecareLupus added 8 commits March 31, 2017 16:42

Removed unnecessary typedefs from KeyUnit

f0b3478

Improved error messages on new kebinding functions

4f30400

Improved error messages on new kebinding functions

dbb4b3e

Merge branch 'development' of https://github.com/SecareLupus/Mudlet i…

966abe9

…nto development

Added check for pHost nullity in keybind functions

392a320

luaSendText (keybind) audit and string init

b4f6722

Simplified exists() checks, using count()

52a0c15

Merge branch 'development' into development

da58f33

SecareLupus added 4 commits April 4, 2017 00:22

Minor style fixes

c810ae7

Exists() now works with keybinds

e5bbb0c

Merge branch 'development' of https://github.com/SecareLupus/Mudlet i…

145236e

…nto development

Moved isTempKey check in XMLexport.cpp

c1356fa

vadi2 added 2 commits May 28, 2017 17:49

Merge branch 'development' into development

048a659

Fixed merge error

4dd0ad5

Merge branch 'development' into development

52ef8f0

vadi2 added 2 commits May 29, 2017 06:25

Fix merge error

7b212d8

Simplified key codes loading

1a40807

vadi2 approved these changes May 29, 2017

View reviewed changes

Removed redundant space as result of deleting function

52183fb

vadi2 assigned SlySven and unassigned SecareLupus May 31, 2017

SlySven reviewed Jun 3, 2017

View reviewed changes

vadi2 added 6 commits June 4, 2017 07:03

Applied clang-format over KeyUnit.cpp

79cf983

Applied feedback on enableKey

e7a24bd

Applied clang-format over XMLexport.cpp

f5c635d

Added newline

2b508dc

Added file to src.pro

b3b5748

Re-applied clang-format

dd52f00

SlySven approved these changes Jun 4, 2017

View reviewed changes

vadi2 added 2 commits June 4, 2017 08:50

Improved other functions to be up to the standard

22c0002

Formatting improvements

832c65a

vadi2 merged commit 6773dcd into Mudlet:development Jun 4, 2017

		@@ -555,8 +555,10 @@ bool XMLexport::writeHost( Host * pHost )
		if( ! (it) \|\| (it)->mModuleMember) {

		@@ -227,10 +266,16 @@ void KeyUnit::addKey(TKey* pT)

		void KeyUnit::removeKey(TKey* pT)

Uh oh!

Conversation

SecareLupus commented Mar 30, 2017

Uh oh!

vadi2 commented Mar 30, 2017

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!

SlySven Mar 30, 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!

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!

SlySven Apr 1, 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!

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!

SlySven Mar 30, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SlySven Mar 30, 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!

Choose a reason for hiding this comment

Uh oh!

SlySven left a comment

Choose a reason for hiding this comment

Uh oh!

SecareLupus commented Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vadi2 commented Mar 30, 2017

Uh oh!

vadi2 commented Mar 31, 2017

Uh oh!

vadi2 commented Apr 1, 2017

Uh oh!

SecareLupus commented Apr 4, 2017

Uh oh!

SecareLupus commented May 28, 2017

Uh oh!

vadi2 commented May 28, 2017

Uh oh!

vadi2 commented May 28, 2017

Uh oh!

vadi2 commented May 28, 2017

Uh oh!

SecareLupus commented May 29, 2017

Uh oh!

vadi2 commented May 29, 2017

Uh oh!

vadi2 commented May 29, 2017

Uh oh!

vadi2 left a comment

Choose a reason for hiding this comment

Uh oh!

SlySven left a comment

SlySven Mar 30, 2017 •

edited

Loading

SlySven Apr 1, 2017 •

edited

Loading

SlySven Mar 30, 2017 •

edited

Loading

SlySven Mar 30, 2017 •

edited

Loading

SecareLupus commented Mar 30, 2017 •

edited

Loading

SlySven Jun 3, 2017 •

edited

Loading

SlySven Jun 3, 2017 •

edited

Loading