EdgeTX / edgetx

EdgeTX is the cutting edge open source firmware for your R/C radio
https://edgetx.org
GNU General Public License v2.0
1.61k stars 339 forks source link

Lua mixer scripts can't be deleted (mixer-script line popup menu) #1318

Open wimalopaan opened 2 years ago

wimalopaan commented 2 years ago

1) the name Reset of the entry is misleading, it should read Delete line

2) the closure erases a local copy of the ScriptData because it captures that per value.

https://github.com/EdgeTX/edgetx/blob/d37b3e40518af42b6e54b3bda1b432a8f0431427/radio/src/gui/colorlcd/model_mixer_scripts.cpp#L259-L269

Fix using a pointer copy:

   ScriptData* sp = &sd;
    button->setPressHandler([=]() -> uint8_t {
      Menu* menu = new Menu(window);
      menu->addLine(STR_EDIT, [=]() { editLine(window, idx); });

      if (runtimeData != nullptr) {
        menu->addLine(STR_RESET, [=]() {
          memset((void*)sp, 0, sizeof(sd));
          // TODO: anything else? reload scripts???
          storageDirty(EE_MODEL);
          rebuild(window, idx);
        });
        return 0;
      }
      return 0;
    });

But: I see this pattern all over the place (capture by value), which make me wonder what the reason is, maybe because there are different threads involved?

So, I don't know if a captue by ref would be ok?

eshifri commented 2 years ago

Can you please provide description of the problem? (Please notice, that your code does exactly the same as original: sp==&sd)

pfeerick commented 2 years ago

There's some discussion on Discord in #dev. But basically, as mentioned the Reset context menu entry is really a delete function, and is currently ineffective.

raphaelcoeffic commented 2 years ago

There's some discussion on Discord in #dev. But basically, as mentioned the Reset context menu entry is really a delete function, and is currently ineffective.

Sounds like we should do something about it.

wimalopaan commented 2 years ago

Can you please provide description of the problem? (Please notice, that your code does exactly the same as original: sp==&sd)

The code I posted above does a different thing as the original (and works as expected). This the capture of the lambda is by-value, the original closure deletes a copy of the ScriptData, which effectively does nothing. The modification passes the pointer (not the to be deleted object) by-value, and therefor really zeroes the array-element.

instead of STR_RESET it should be STR_DELETE.

wimalopaan commented 2 years ago

Is incorcoparetd in #1317 now

wimalopaan commented 2 years ago

Maybe #1340 is of interest also.

philmoz commented 1 year ago

Can this be closed?