Enhance: get and set state of push-down-buttons (button love part1) by SlySven · Pull Request #5227 · Mudlet/Mudlet

SlySven · 2021-05-13T21:51:49Z

Brief overview of PR changes/additions

Provide a means to get and set the state of push down buttons (the latter without executing the associate command or script). It requires adding members to the TAction class to keep track of the EAction or TFlipButton instance that is the implementation of the button on a TEasyButton or TToolBar respectively. As it happens this showed that the previous code to regenerate them was creating new buttons each time but was not deleting the existing ones that were getting replaced.

Also:

made some members of the TAtion class (the name, script and both commands) private as otherwise there is no point in the setters and getters also provided!
simplified the existing getButtonState() {that only works within the script for a button) to remove an unneeded intermediate local (int) value.

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

Motivation for adding to Mudlet

It is something has been requested in the past (I think recently on Discord).

Other info (issues closed, discussion etc)

Further improvements to buttons/menus/toolbars are desirable and I can foresee some of them.

Release post highlight

Allow Lua scripts to get the state of click-down (toggle-able) buttons outside of the script for said button and allow for them to also set that state.

…art 1) Provide a means to get and set the state of push down buttons (the latter without executing the associate command or script). It requires adding members to the `TAction` class to keep track of the `EAction` or `TFlipButton` instance that is the implimentation of the button on a `TEasyButton` or `TToolBar` respectively. As it happens this showed that the previous code to regenerate them was creating new buttons each time but was not deleting the existing ones that were getting replaced. Also: * made some members of the `TAtion` class (the name, script and both commands) private as otherwise there is no point in the setters and getters also provided! * simpliedied the existing `getButtonState()` {that only works within the script for a button) to remove an unneeded intermediate local (`int`) value. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

The original comment was wrong as well - using a `Q_ASSERT_X` would have broken things for a floatable/dockable toolbar type button (what `TAction::mpFButton` will point at)...! Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

add-deployment-links · 2021-05-13T21:51:50Z

Hey there! Thanks for helping Mudlet improve. 🌟

Test versions

You can directly test the changes here:

No need to install anything - just unzip and run.
Let us know if it works well, and if it doesn't, please give details.

github-actions · 2021-05-13T21:59:57Z

clang-tidy review says "All clean, LGTM! 👍"

src/TLuaInterpreter.cpp

Note that for the getter this is an intermediate stage as is should be possibble to combine it with the existing `getButtonState()` function that takes NO arguments but only works within the Lua script for a push-down button. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

This simpilifies the UI so that `getButtonState()` works the same as before but if an button's ID number or "name" is supplied as an argument the state of the corresponding button (as a boolean though, NOT the existing and, TBH, odd, integer value 1 or 2, that the original function produces) is returned. In the event that the matching item is NOT a push-down button, or that it does not match an existing button then a 'nil' + (run-time) error message is returned instead. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

keneanung

One suggestion for me, but otherwise I like this.

keneanung · 2021-06-24T19:54:33Z

src/TLuaInterpreter.cpp

+    auto argType = lua_type(L, 1);
+    TAction* pItem = nullptr;
+    if (argType == LUA_TNUMBER) {
+        int id = lua_tonumber(L, 1);
+        if (id < 0) {
+            return warnArgumentValue(L, __func__, QStringLiteral("item Id (%1) invalid, it must be equal or greater than zero").arg(id).toUtf8().constData());
+        }
+        pItem = host.getActionUnit()->getAction(id);
+        if (!pItem) {
+            return warnArgumentValue(L, __func__, QStringLiteral("no button item with Id %1 found").arg(id).toUtf8().constData());
+        }
+        if (!pItem->isPushDownButton()) {
+            return warnArgumentValue(L, __func__, QStringLiteral("item Id with %1 is not a push-down button").arg(id).toUtf8().constData());
+        }
+    }
+
+    if (argType == LUA_TSTRING) {
+        QString name = lua_tostring(L, 1);
+        if (name.isEmpty()) {
+            return warnArgumentValue(L, __func__, "item name must not be an empty string");
+        }
+        pItem = host.getActionUnit()->findAction(name);
+        if (!pItem) {
+            return warnArgumentValue(L, __func__, QStringLiteral("no button item with name '%1' found").arg(name).toUtf8().constData());
+        }
+        if (!pItem->isPushDownButton()) {
+            return warnArgumentValue(L, __func__, QStringLiteral("item with name '%1' is not a push-down button").arg(name).toUtf8().constData());
+        }
+    }


This whole part seems to be pretty much the same as in the set function. I would suggest putting this into a function that's used by both.

TBH if the code was to be used in more than these two cases I'd go along with that idea but the additional complexity of a further function call for just this pair means I think it is a borderline decision and I'd prefer not to do that currently...

🤔

In the medium term I'd like to have a set of introspection (?) functions that can get, and for some set, the common elements of all the mudlet item types (alias, button/menu/toolbars, keybindings, scripts, timers, triggers) i.e. "name", "command", "ID", "active" {based on both it's own and its ancestors' state} and possibly some further details... as such it may then be useful to factor out item identification based on EITHER a numeric ID OR a string name basis. At that point this would be useful to revisit.

To me, this is not a borderline decision. It has strings and quite a bit of logic that should stay the same. And the easiest way for that is adding a function.

Kebap

setButtonState is missing in Area 51

set the state of push down buttons (without executing the associate command or script).

Is that really always desired? Maybe a toggle would make sense. Not sure if this should even be default. Then again, I hardly used buttons myself.

Kebap · 2021-06-25T08:12:54Z

src/TLuaInterpreter.cpp

+            return warnArgumentValue(L, __func__, QStringLiteral("no button item with Id %1 found").arg(id).toUtf8().constData());
+        }
+        if (!pItem->isPushDownButton()) {
+            return warnArgumentValue(L, __func__, QStringLiteral("item Id with %1 is not a push-down button").arg(id).toUtf8().constData());


Suggested change

return warnArgumentValue(L, __func__, QStringLiteral("item Id with %1 is not a push-down button").arg(id).toUtf8().constData());

return warnArgumentValue(L, __func__, QStringLiteral("item with Id %1 is not a push-down button").arg(id).toUtf8().constData());

we discussed earlier if we go for "id" or "Id" or "ID" and can't remember the consensus reached.. but I feel like "Id" is the worst of those ☹️

Um, one reference might be: #2605 - and it has not been closed...

Tomayto, Tomaahtoe ....

Ah, I'll revise it to ID and hope that'll silence the critics... 🤞

Kebap · 2021-06-25T08:14:01Z

src/TLuaInterpreter.cpp

+
+    if (!pItem) {
+        // we'll get here if the (first) argument is NOT usable:
+        lua_pushfstring(L, "%s: bad argument #1 type (ID as number or name as string expected, got %s!)",


See there is an "ID", let's use that consistently, shall we?

Okay, in the scheme of things I can't be ~~arsed~~bothered to fight this...

SlySven · 2021-07-03T19:26:52Z

set the state of push down buttons (without executing the associate command or script).

Is that really always desired? Maybe a toggle would make sense. Not sure if this should even be default. Then again, I hardly used buttons myself.

When configuring a UI that uses buttons/menus/toolbars it is pretty much essential to be able to set the state of push-down buttons to a required initial or to reflect something that is happening in-game independently of whatever script/command is associated with that button. A bit of refactoring to make the Lua script a simple function call to a script defined elsewhere (in a script element I guess) would be enough to reproduce clicking the button whereas only providing a function to simulate toggling the button state AND executing the script / send the command is actually a much less flexible way of doing things - and would not give the same scope for usage that the setter function herein gives...

As suggested by peer-review. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

@keneanung

…e(...) This was strongly requested by @keneanung though my preference was against it for just two instances - as developing an API that would work with the `warnArgumentValue(...)` function took a bit of thought and I had to work with handling a `TAction**` (pointer to a pointer to a `TAction`) as a function argument! Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

github-actions

clang-tidy made some suggestions

src/TLuaInterpreter.cpp

keneanung

Thank you very much for the rework 😄 I have a niggle that I'd love to see a change for. But it won't break my heart if you're adamant here 😉

keneanung · 2021-07-17T18:42:13Z

src/TLuaInterpreter.cpp

+//   it also resets the pItem to be a nullptr
+// does NOT return (normally) if the index item on the stack is not a number or
+//   a string
+int TLuaInterpreter::getTActionFromIdOrName(lua_State* L, TAction** pItem, const int index, const char* func)


I know this isn't atypical for C++, but I prefer using function arguments as input instead of output. This is also mentioned in Clean Code. Have you considered returning multiple values via https://www.geeksforgeeks.org/returning-multiple-values-from-a-function-using-tuple-and-pair-in-c/?

Agree - and we have many examples of this, check for example:

std::pair<bool, QString> changeModuleSync(const QString& enableModuleName, const QLatin1String &value); std::pair<bool, QString> getModuleSync(const QString& moduleName);

Make it a std::pair with the original return value of `TLuaInterpreter::getTActionFromIdOrName(...)` instead of passing it as a non-const reference argument. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

vadi2

Thanks for adding this requested feature in!

SlySven · 2021-07-19T19:41:04Z

📚 documentation done, see:

~~getButtonState(...)~~ - pulled from main Wiki
~~setButtonState(...)~~ - pulled from main Wiki
getButtonState(...) - relocated to Area 51
setButtonState(...) - relocated to Area 51

vadi2 · 2021-07-20T06:39:16Z

Looks good! But it should be in Area 51 as it's not available yet. People naturally get confused when we list releases in docs that aren't out yet

Kebap · 2021-07-20T08:07:31Z

Please honor Area 51 template, e.g. don't hide version info below example section

SlySven · 2021-07-20T18:57:48Z

Looks good! But it should be in Area 51 as it's not available yet. People naturally get confused when we list releases in docs that aren't out yet

Ah, I was thinking that it being in the PTB would be justification for it making it into the main documentation.

Please honor Area 51 template, e.g. don't hide version info below example section

🤔 I was trying to avoid contaminating the auto-completion ~~extraction process~~ scraper.

Anyhow I have now relocated them, in the meantime, to Area 51 and perhaps someone else could reshape the items so as to fit better...

Kebap · 2021-09-20T09:54:03Z

perhaps someone else could reshape the items so as to fit better...

someone else did

Kebap · 2021-09-20T10:30:10Z

📚 documentation done, see:

* ~**[getButtonState(...)](https://wiki.mudlet.org/w/Manual:Mudlet_Object_Functions#getButtonState)**~ - pulled from main Wiki

* ~**[setButtonState(...)](https://wiki.mudlet.org/w/Manual:Mudlet_Object_Functions#setButtonState)**~ - pulled from main Wiki

* **[getButtonState(...)](https://wiki.mudlet.org/w/Area_51#getButtonState)** - relocated to Area 51

* **[setButtonState(...)](https://wiki.mudlet.org/w/Area_51#setButtonState)** - relocated to Area 51

published again for upcoming release

SlySven added 2 commits May 13, 2021 22:34

SlySven added enhancement needs documentation This pull request changes things the players would use/see and thus needs an update in the manual labels May 13, 2021

SlySven added this to the 4.12.0 milestone May 13, 2021

SlySven assigned vadi2 May 13, 2021

SlySven requested a review from a team as a code owner May 13, 2021 21:51

SlySven requested review from a team May 13, 2021 21:51

vadi2 reviewed May 24, 2021

View reviewed changes

src/TLuaInterpreter.cpp Outdated Show resolved Hide resolved

SlySven added 2 commits May 30, 2021 15:10

SlySven modified the milestones: 4.12.0, 4.13.0 May 30, 2021

keneanung reviewed Jun 24, 2021

View reviewed changes

Kebap changed the title ~~Enhance: (button love part1) add push down button state getters and setters~~ Enhance: get and set state of push-down-buttons (button love part1) Jun 25, 2021

Kebap reviewed Jun 25, 2021

View reviewed changes

SlySven added 2 commits July 3, 2021 20:32

Revise: used ID consistently in two functions added/modified in this PR

f72f656

As suggested by peer-review. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

github-actions bot reviewed Jul 14, 2021

View reviewed changes

src/TLuaInterpreter.cpp Show resolved Hide resolved

src/TLuaInterpreter.cpp Show resolved Hide resolved

keneanung approved these changes Jul 17, 2021

View reviewed changes

Revise: use a different method to return the pointer to a TAction

2f97f34

Make it a std::pair with the original return value of `TLuaInterpreter::getTActionFromIdOrName(...)` instead of passing it as a non-const reference argument. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

vadi2 approved these changes Jul 19, 2021

View reviewed changes

SlySven merged commit 0c348f6 into Mudlet:development Jul 19, 2021

SlySven deleted the Enhance_buttonLovePart1_addPushDownButtonStateGettersAndSetters branch July 22, 2021 21:35

Kebap removed the needs documentation This pull request changes things the players would use/see and thus needs an update in the manual label Sep 20, 2021

SlySven mentioned this pull request Oct 5, 2024

Mudlet's editor for the buttons widget should have an option to execute code. #7455

Open

	return warnArgumentValue(L, __func__, QStringLiteral("item Id with %1 is not a push-down button").arg(id).toUtf8().constData());
	return warnArgumentValue(L, __func__, QStringLiteral("item with Id %1 is not a push-down button").arg(id).toUtf8().constData());

Uh oh!

Conversation

SlySven commented May 13, 2021

Brief overview of PR changes/additions

Motivation for adding to Mudlet

Other info (issues closed, discussion etc)

Release post highlight

Uh oh!

add-deployment-links bot commented May 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test versions

Uh oh!

github-actions bot commented May 13, 2021

Uh oh!

Uh oh!

keneanung 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!

SlySven Jul 3, 2021 • 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!

Kebap left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SlySven commented Jul 3, 2021

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

keneanung 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!

SlySven commented Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vadi2 commented Jul 20, 2021

Uh oh!

Kebap commented Jul 20, 2021

Uh oh!

SlySven commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kebap commented Sep 20, 2021

Uh oh!

Kebap commented Sep 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

add-deployment-links bot commented May 13, 2021 •

edited

Loading

SlySven Jul 3, 2021 •

edited

Loading

Kebap left a comment •

edited

Loading

SlySven commented Jul 19, 2021 •

edited

Loading

SlySven commented Jul 20, 2021 •

edited

Loading