m5stack / M5Core2

M5Core2 Arduino Library
MIT License
267 stars 117 forks source link

Button doesn't work with STL containers like std::vector<Button> #113

Open aseanwatson opened 2 years ago

aseanwatson commented 2 years ago

Describe the bug

The C++ compiler implicitly generates a copy constructor Button::Button(const Button& button). However, this doesn't register the new Button in Button::instances or copy the event handlers for the old button in M5Buttons::_eventHandlers.

The result is that you have buttons drawn on the screen, but which don't do anything.

To reproduce

Run this code:

#include <Arduino.h>
#include <M5Core2.h>
#include <vector>

std::vector<Button> Buttons;

void ButtonHandler(Event& event)
{
  Serial.printf("Handler: %s %s\r\n",
    event.button->label(),
    event.typeName());
}

struct ButtonInfo
{
  int16_t x;
  int16_t y;
  const char* label;
} ButtonInfos[] =
{
  { 0, 0, "b1" },
  { 1, 0, "b2" },
  { 2, 0, "b3" },
  { 0, 1, "b4" },
  { 1, 1, "b5" },
  { 2, 1, "b6" },
};

void setup()
{
  M5.begin();
  for (ButtonInfo& bi: ButtonInfos)
  {
    const int16_t margin = 5;
    const int16_t size = 80;
    Buttons.emplace_back(margin + bi.x * (margin + size), margin + bi.y * (margin + size), size, size);
    Button& b = Buttons.back();
    b.setLabel(bi.label);
    b.on = { BLACK, WHITE, WHITE };
    b.off = { DARKGREY, WHITE, WHITE };
    b.addHandler(ButtonHandler);
    b.draw();
  }

#ifdef WORKAROUND
  for (Button& b : Buttons)
  {
    b.delHandlers();
    int i = b.instanceIndex();
    if (i >= 0)
    {
      Button::instances.erase(Button::instances.begin() + i);
    }

    Button::instances.push_back(&b);
    b.addHandler(ButtonHandler);
  }
#endif
}

void loop()
{
  M5.update();
}

Expect: All 6 buttons work. Actual: Not all but buttons work (it seems that the last two work).

Note: You can get things to work by #define WORKAROUND 1, but it's not very obvious.

Expected behavior

At a bare minimum, make the implicit methods deleted so they don't compile with things that expect them (but then not work).

class Button : public Zone {
// ...
  Button(const Button&) = delete;
  operator =(const Button&) = delete;
};

It'd be better if you:

  1. Implement the copy constructor and have it maintain Button::instances.
  2. Rather than have a std::vector<EventHandler> _eventHandlers; in M5Buttons, put it in Button (and Gesture). Theplaces which access _eventHandlers know the Button* (or Gesture*)

Screenshots

No response

Environment

Additional context

No response

Issue checklist