fix(modules): Avoid downcast in module constructor

The previous CAST_MOD(Impl) for the action_router constructor was
illegal because `this` is not yet of type Impl (because the subclass
constructor has not run yet).

The action_router now accepts std::function for its callbacks.

Fixes #2519

include/modules/meta/base.hpp

@@ -44,7 +44,6 @@ class config;
 class logger;
 class signal_emitter;
 
-template <typename Impl>
 class action_router;
 // }}}
 
@@ -200,7 +199,7 @@ namespace modules {
     const logger& m_log;
     const config& m_conf;
 
-    unique_ptr<action_router<Impl>> m_router;
+    unique_ptr<action_router> m_router;
 
     mutex m_buildlock;
     mutex m_updatelock;

include/modules/meta/base.inl

@@ -20,18 +20,16 @@ namespace modules {
       , m_bar(bar)
       , m_log(logger::make())
       , m_conf(config::make())
-      // TODO this cast is illegal because 'this' is not yet of type Impl but only of type module<Impl>
-      // Change action router to use lambdas
-      , m_router(make_unique<action_router<Impl>>(CAST_MOD(Impl)))
+      , m_router(make_unique<action_router>())
       , m_name("module/" + name)
       , m_name_raw(name)
       , m_builder(make_unique<builder>(bar))
       , m_formatter(make_unique<module_formatter>(m_conf, m_name))
       , m_handle_events(m_conf.get(m_name, "handle-events", true))
       , m_visible(!m_conf.get(m_name, "hidden", false)) {
-    m_router->register_action(EVENT_MODULE_TOGGLE, &module<Impl>::action_module_toggle);
-    m_router->register_action(EVENT_MODULE_SHOW, &module<Impl>::action_module_show);
-    m_router->register_action(EVENT_MODULE_HIDE, &module<Impl>::action_module_hide);
+    m_router->register_action(EVENT_MODULE_TOGGLE, [this]() { action_module_toggle(); });
+    m_router->register_action(EVENT_MODULE_SHOW, [this]() { action_module_show(); });
+    m_router->register_action(EVENT_MODULE_HIDE, [this]() { action_module_hide(); });
   }
 
   template <typename Impl>

include/utils/action_router.hpp

@@ -9,14 +9,11 @@
 POLYBAR_NS
 
 /**
- * Maps action names to function pointers in this module and invokes them.
+ * Maps action names to lambdas and invokes them.
  *
  * Each module has one instance of this class and uses it to register action.
  * For each action the module has to register the name, whether it can take
- * additional data, and a pointer to the member function implementing that
- * action.
- *
- * Ref: https://isocpp.org/wiki/faq/pointers-to-members
+ * additional data, and a callback that is called whenever the action is triggered.
  *
  * The input() function in the base class uses this for invoking the actions
  * of that module.
@@ -24,51 +21,15 @@ POLYBAR_NS
  * Any module that does not reimplement that function will automatically use
  * this class for action routing.
  */
-template <typename Impl>
 class action_router {
-  typedef void (Impl::*callback)();
-  typedef void (Impl::*callback_data)(const std::string&);
+  using callback = std::function<void(void)>;
+  using callback_data = std::function<void(const std::string&)>;
 
  public:
-  explicit action_router(Impl* This) : m_this(This) {}
-
-  void register_action(const string& name, callback func) {
-    entry e;
-    e.with_data = false;
-    e.without = func;
-    register_entry(name, e);
-  }
-
-  void register_action_with_data(const string& name, callback_data func) {
-    entry e;
-    e.with_data = true;
-    e.with = func;
-    register_entry(name, e);
-  }
-
-  bool has_action(const string& name) {
-    return callbacks.find(name) != callbacks.end();
-  }
-
-  /**
-   * Invokes the given action name on the passed module pointer.
-   *
-   * The action must exist.
-   */
-  void invoke(const string& name, const string& data) {
-    auto it = callbacks.find(name);
-    assert(it != callbacks.end());
-
-    entry e = it->second;
-
-#define CALL_MEMBER_FN(object, ptrToMember) ((object).*(ptrToMember))
-    if (e.with_data) {
-      CALL_MEMBER_FN(*m_this, e.with)(data);
-    } else {
-      CALL_MEMBER_FN(*m_this, e.without)();
-    }
-#undef CALL_MEMBER_FN
-  }
+  void register_action(const string& name, callback func);
+  void register_action_with_data(const string& name, callback_data func);
+  bool has_action(const string& name);
+  void invoke(const string& name, const string& data);
 
  protected:
   struct entry {
@@ -77,18 +38,23 @@ class action_router {
       callback_data with;
     };
     bool with_data;
+
+    entry(callback func);
+    entry(callback_data func);
+    ~entry();
   };
 
-  void register_entry(const string& name, const entry& e) {
+  template <typename F>
+  void register_entry(const string& name, const F& e) {
     if (has_action(name)) {
       throw std::invalid_argument("Tried to register action '" + name + "' twice. THIS IS A BUG!");
     }
-    callbacks[name] = e;
+
+    callbacks.emplace(name, std::move(e));
   }
 
  private:
   std::unordered_map<string, entry> callbacks;
-  Impl* m_this;
 };
 
 POLYBAR_NS_END

src/CMakeLists.txt

@@ -104,6 +104,7 @@ if(BUILD_LIBPOLY)
     ${src_dir}/tags/parser.cpp
 
     ${src_dir}/utils/actions.cpp
+    ${src_dir}/utils/action_router.cpp
     ${src_dir}/utils/bspwm.cpp
     ${src_dir}/utils/color.cpp
     ${src_dir}/utils/command.cpp

src/modules/alsa.cpp

@@ -19,9 +19,9 @@ namespace modules {
 
   alsa_module::alsa_module(const bar_settings& bar, string name_) : event_module<alsa_module>(bar, move(name_)) {
     if (m_handle_events) {
-      m_router->register_action(EVENT_DEC, &alsa_module::action_dec);
-      m_router->register_action(EVENT_INC, &alsa_module::action_inc);
-      m_router->register_action(EVENT_TOGGLE, &alsa_module::action_toggle);
+      m_router->register_action(EVENT_DEC, [this]() { action_dec(); });
+      m_router->register_action(EVENT_INC, [this]() { action_inc(); });
+      m_router->register_action(EVENT_TOGGLE, [this]() { action_toggle(); });
     }
 
     // Load configuration values

src/modules/backlight.cpp

@@ -25,8 +25,8 @@ namespace modules {
 
   backlight_module::backlight_module(const bar_settings& bar, string name_)
       : inotify_module<backlight_module>(bar, move(name_)) {
-    m_router->register_action(EVENT_DEC, &backlight_module::action_dec);
-    m_router->register_action(EVENT_INC, &backlight_module::action_inc);
+    m_router->register_action(EVENT_DEC, [this]() { action_dec(); });
+    m_router->register_action(EVENT_INC, [this]() { action_inc(); });
 
     auto card = m_conf.get(name(), "card");
 

src/modules/bspwm.cpp

@@ -40,9 +40,9 @@ namespace modules {
   template class module<bspwm_module>;
 
   bspwm_module::bspwm_module(const bar_settings& bar, string name_) : event_module<bspwm_module>(bar, move(name_)) {
-    m_router->register_action_with_data(EVENT_FOCUS, &bspwm_module::action_focus);
-    m_router->register_action(EVENT_NEXT, &bspwm_module::action_next);
-    m_router->register_action(EVENT_PREV, &bspwm_module::action_prev);
+    m_router->register_action_with_data(EVENT_FOCUS, [this](const std::string& data) { action_focus(data); });
+    m_router->register_action(EVENT_NEXT, [this]() { action_next(); });
+    m_router->register_action(EVENT_PREV, [this]() { action_prev(); });
 
     auto socket_path = bspwm_util::get_socket_path();
 

src/modules/date.cpp

@@ -13,7 +13,7 @@ namespace modules {
       datetime_stream.imbue(std::locale(m_bar.locale.c_str()));
     }
 
-    m_router->register_action(EVENT_TOGGLE, &date_module::action_toggle);
+    m_router->register_action(EVENT_TOGGLE, [this]() { action_toggle(); });
 
     m_dateformat = m_conf.get(name(), "date", ""s);
     m_dateformat_alt = m_conf.get(name(), "date-alt", ""s);

src/modules/i3.cpp

@@ -13,9 +13,9 @@ namespace modules {
   template class module<i3_module>;
 
   i3_module::i3_module(const bar_settings& bar, string name_) : event_module<i3_module>(bar, move(name_)) {
-    m_router->register_action_with_data(EVENT_FOCUS, &i3_module::action_focus);
-    m_router->register_action(EVENT_NEXT, &i3_module::action_next);
-    m_router->register_action(EVENT_PREV, &i3_module::action_prev);
+    m_router->register_action_with_data(EVENT_FOCUS, [this](const std::string& data) { action_focus(data); });
+    m_router->register_action(EVENT_NEXT, [this]() { action_next(); });
+    m_router->register_action(EVENT_PREV, [this]() { action_prev(); });
 
     auto socket_path = i3ipc::get_socketpath();
 

src/modules/ipc.cpp

@@ -15,7 +15,7 @@ namespace modules {
    * create formatting tags
    */
   ipc_module::ipc_module(const bar_settings& bar, string name_) : module<ipc_module>(bar, move(name_)) {
-    m_router->register_action_with_data(EVENT_SEND, &ipc_module::action_send);
+    m_router->register_action_with_data(EVENT_SEND, [this](const std::string& data) { action_send(data); });
 
     size_t index = 0;
 

src/modules/menu.cpp

@@ -14,9 +14,9 @@ namespace modules {
   menu_module::menu_module(const bar_settings& bar, string name_) : static_module<menu_module>(bar, move(name_)) {
     m_expand_right = m_conf.get(name(), "expand-right", m_expand_right);
 
-    m_router->register_action_with_data(EVENT_OPEN, &menu_module::action_open);
-    m_router->register_action(EVENT_CLOSE, &menu_module::action_close);
-    m_router->register_action_with_data(EVENT_EXEC, &menu_module::action_exec);
+    m_router->register_action_with_data(EVENT_OPEN, [this](const std::string& data) { action_open(data); });
+    m_router->register_action(EVENT_CLOSE, [this]() { action_close(); });
+    m_router->register_action_with_data(EVENT_EXEC, [this](const std::string& data) { action_exec(data); });
 
     string default_format;
     if (m_expand_right) {

src/modules/mpd.cpp

@@ -13,16 +13,16 @@ namespace modules {
   template class module<mpd_module>;
 
   mpd_module::mpd_module(const bar_settings& bar, string name_) : event_module<mpd_module>(bar, move(name_)) {
-    m_router->register_action(EVENT_PLAY, &mpd_module::action_play);
-    m_router->register_action(EVENT_PAUSE, &mpd_module::action_pause);
-    m_router->register_action(EVENT_STOP, &mpd_module::action_stop);
-    m_router->register_action(EVENT_PREV, &mpd_module::action_prev);
-    m_router->register_action(EVENT_NEXT, &mpd_module::action_next);
-    m_router->register_action(EVENT_REPEAT, &mpd_module::action_repeat);
-    m_router->register_action(EVENT_SINGLE, &mpd_module::action_single);
-    m_router->register_action(EVENT_RANDOM, &mpd_module::action_random);
-    m_router->register_action(EVENT_CONSUME, &mpd_module::action_consume);
-    m_router->register_action_with_data(EVENT_SEEK, &mpd_module::action_seek);
+    m_router->register_action(EVENT_PLAY, [this]() { action_play(); });
+    m_router->register_action(EVENT_PAUSE, [this]() { action_pause(); });
+    m_router->register_action(EVENT_STOP, [this]() { action_stop(); });
+    m_router->register_action(EVENT_PREV, [this]() { action_prev(); });
+    m_router->register_action(EVENT_NEXT, [this]() { action_next(); });
+    m_router->register_action(EVENT_REPEAT, [this]() { action_repeat(); });
+    m_router->register_action(EVENT_SINGLE, [this]() { action_single(); });
+    m_router->register_action(EVENT_RANDOM, [this]() { action_random(); });
+    m_router->register_action(EVENT_CONSUME, [this]() { action_consume(); });
+    m_router->register_action_with_data(EVENT_SEEK, [this](const std::string& data) { action_seek(data); });
 
     m_host = m_conf.get(name(), "host", m_host);
     m_port = m_conf.get(name(), "port", m_port);

src/modules/pulseaudio.cpp

@@ -16,9 +16,9 @@ namespace modules {
   pulseaudio_module::pulseaudio_module(const bar_settings& bar, string name_)
       : event_module<pulseaudio_module>(bar, move(name_)) {
     if (m_handle_events) {
-      m_router->register_action(EVENT_DEC, &pulseaudio_module::action_dec);
-      m_router->register_action(EVENT_INC, &pulseaudio_module::action_inc);
-      m_router->register_action(EVENT_TOGGLE, &pulseaudio_module::action_toggle);
+      m_router->register_action(EVENT_DEC, [this]() { action_dec(); });
+      m_router->register_action(EVENT_INC, [this]() { action_inc(); });
+      m_router->register_action(EVENT_TOGGLE, [this]() { action_toggle(); });
     }
 
     // Load configuration values

src/modules/systray.cpp

@@ -16,7 +16,7 @@ namespace modules {
    */
   systray_module::systray_module(const bar_settings& bar, string name_)
       : static_module<systray_module>(bar, move(name_)), m_connection(connection::make()) {
-    m_router->register_action(EVENT_TOGGLE, &systray_module::action_toggle);
+    m_router->register_action(EVENT_TOGGLE, [this]() { action_toggle(); });
 
     // Add formats and elements
     m_formatter->add(DEFAULT_FORMAT, TAG_LABEL_TOGGLE, {TAG_LABEL_TOGGLE, TAG_TRAY_CLIENTS});

src/modules/xbacklight.cpp

@@ -18,8 +18,8 @@ namespace modules {
    */
   xbacklight_module::xbacklight_module(const bar_settings& bar, string name_)
       : static_module<xbacklight_module>(bar, move(name_)), m_connection(connection::make()) {
-    m_router->register_action(EVENT_INC, &xbacklight_module::action_inc);
-    m_router->register_action(EVENT_DEC, &xbacklight_module::action_dec);
+    m_router->register_action(EVENT_INC, [this]() { action_inc(); });
+    m_router->register_action(EVENT_DEC, [this]() { action_dec(); });
 
     auto output = m_conf.get(name(), "output", m_bar.monitor->name);
 

src/modules/xkeyboard.cpp

@@ -24,7 +24,7 @@ namespace modules {
    */
   xkeyboard_module::xkeyboard_module(const bar_settings& bar, string name_)
       : static_module<xkeyboard_module>(bar, move(name_)), m_connection(connection::make()) {
-    m_router->register_action(EVENT_SWITCH, &xkeyboard_module::action_switch);
+    m_router->register_action(EVENT_SWITCH, [this]() { action_switch(); });
 
     // Setup extension
     // clang-format off

src/modules/xworkspaces.cpp

@@ -35,9 +35,9 @@ namespace modules {
    */
   xworkspaces_module::xworkspaces_module(const bar_settings& bar, string name_)
       : static_module<xworkspaces_module>(bar, move(name_)), m_connection(connection::make()) {
-    m_router->register_action_with_data(EVENT_FOCUS, &xworkspaces_module::action_focus);
-    m_router->register_action(EVENT_NEXT, &xworkspaces_module::action_next);
-    m_router->register_action(EVENT_PREV, &xworkspaces_module::action_prev);
+    m_router->register_action_with_data(EVENT_FOCUS, [this](const std::string& data) { action_focus(data); });
+    m_router->register_action(EVENT_NEXT, [this]() { action_next(); });
+    m_router->register_action(EVENT_PREV, [this]() { action_prev(); });
 
     // Load config values
     m_pinworkspaces = m_conf.get(name(), "pin-workspaces", m_pinworkspaces);

src/utils/action_router.cpp

@@ -0,0 +1,45 @@
+#include "utils/action_router.hpp"
+
+POLYBAR_NS
+
+action_router::entry::entry(callback func) : without(func), with_data(false){}
+action_router::entry::entry(callback_data func) : with(func), with_data(true){}
+action_router::entry::~entry() {
+  if (with_data) {
+    with.~function();
+  } else {
+    without.~function();
+  }
+}
+
+void action_router::register_action(const string& name, callback func) {
+  register_entry(name, func);
+}
+
+void action_router::register_action_with_data(const string& name, callback_data func) {
+  register_entry(name, func);
+}
+
+bool action_router::has_action(const string& name) {
+  return callbacks.find(name) != callbacks.end();
+}
+
+/**
+ * Invokes the given action name on the passed module pointer.
+ *
+ * The action must exist.
+ */
+void action_router::invoke(const string& name, const string& data) {
+  auto it = callbacks.find(name);
+  assert(it != callbacks.end());
+
+  auto& e = it->second;
+
+  if (e.with_data) {
+    e.with(data);
+  } else {
+    e.without();
+  }
+}
+
+POLYBAR_NS_END

tests/unit_tests/utils/action_router.cpp

@@ -21,18 +21,18 @@ TEST(ActionRouterTest, CallsCorrectFunctions) {
     EXPECT_CALL(m, action2("foo")).Times(1);
   }
 
-  action_router<MockModule> router(&m);
-  router.register_action("action1", &MockModule::action1);
-  router.register_action_with_data("action2", &MockModule::action2);
+  action_router router;
+  router.register_action("action1", [&]() { m.action1(); });
+  router.register_action_with_data("action2", [&](const std::string& data) { m.action2(data); });
   router.invoke("action1", "");
   router.invoke("action2", "foo");
 }
 
 TEST(ActionRouterTest, HasAction) {
   MockModule m;
-  action_router<MockModule> router(&m);
+  action_router router;
 
-  router.register_action("foo", &MockModule::action1);
+  router.register_action("foo", [&]() { m.action1(); });
 
   EXPECT_TRUE(router.has_action("foo"));
   EXPECT_FALSE(router.has_action("bar"));
@@ -40,9 +40,10 @@ TEST(ActionRouterTest, HasAction) {
 
 TEST(ActionRouterTest, ThrowsOnDuplicate) {
   MockModule m;
-  action_router<MockModule> router(&m);
+  action_router router;
 
-  router.register_action("foo", &MockModule::action1);
-  EXPECT_THROW(router.register_action("foo", &MockModule::action1), std::invalid_argument);
-  EXPECT_THROW(router.register_action_with_data("foo", &MockModule::action2), std::invalid_argument);
+  router.register_action("foo", [&]() { m.action1(); });
+  EXPECT_THROW(router.register_action("foo", [&]() { m.action1(); }), std::invalid_argument);
+  EXPECT_THROW(router.register_action_with_data("foo", [&](const std::string& data) { m.action2(data); }),
+      std::invalid_argument);
 }