From 1a59599388724b983e61be309dd61a75e0689be2 Mon Sep 17 00:00:00 2001 From: patrick96 Date: Sun, 3 Oct 2021 02:57:49 +0200 Subject: [PATCH] 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 | 3 +- include/modules/meta/base.inl | 10 ++-- include/utils/action_router.hpp | 66 ++++++------------------ src/CMakeLists.txt | 1 + src/modules/alsa.cpp | 6 +-- src/modules/backlight.cpp | 4 +- src/modules/bspwm.cpp | 6 +-- src/modules/date.cpp | 2 +- src/modules/i3.cpp | 6 +-- src/modules/ipc.cpp | 2 +- src/modules/menu.cpp | 6 +-- src/modules/mpd.cpp | 20 +++---- src/modules/pulseaudio.cpp | 6 +-- src/modules/systray.cpp | 2 +- src/modules/xbacklight.cpp | 4 +- src/modules/xkeyboard.cpp | 2 +- src/modules/xworkspaces.cpp | 6 +-- src/utils/action_router.cpp | 45 ++++++++++++++++ tests/unit_tests/utils/action_router.cpp | 19 +++---- 19 files changed, 113 insertions(+), 103 deletions(-) create mode 100644 src/utils/action_router.cpp diff --git a/include/modules/meta/base.hpp b/include/modules/meta/base.hpp index 5fc75e88..941525da 100644 --- a/include/modules/meta/base.hpp +++ b/include/modules/meta/base.hpp @@ -44,7 +44,6 @@ class config; class logger; class signal_emitter; -template class action_router; // }}} @@ -200,7 +199,7 @@ namespace modules { const logger& m_log; const config& m_conf; - unique_ptr> m_router; + unique_ptr m_router; mutex m_buildlock; mutex m_updatelock; diff --git a/include/modules/meta/base.inl b/include/modules/meta/base.inl index 182a2ab2..219a35b1 100644 --- a/include/modules/meta/base.inl +++ b/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 - // Change action router to use lambdas - , m_router(make_unique>(CAST_MOD(Impl))) + , m_router(make_unique()) , m_name("module/" + name) , m_name_raw(name) , m_builder(make_unique(bar)) , m_formatter(make_unique(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::action_module_toggle); - m_router->register_action(EVENT_MODULE_SHOW, &module::action_module_show); - m_router->register_action(EVENT_MODULE_HIDE, &module::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 diff --git a/include/utils/action_router.hpp b/include/utils/action_router.hpp index 2f0aa4f0..38a3daa7 100644 --- a/include/utils/action_router.hpp +++ b/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 class action_router { - typedef void (Impl::*callback)(); - typedef void (Impl::*callback_data)(const std::string&); + using callback = std::function; + using callback_data = std::function; 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 + 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 callbacks; - Impl* m_this; }; POLYBAR_NS_END diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 0a50ecf6..c671ce2c 100644 --- a/src/CMakeLists.txt +++ b/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 diff --git a/src/modules/alsa.cpp b/src/modules/alsa.cpp index 7a886ba4..143787c6 100644 --- a/src/modules/alsa.cpp +++ b/src/modules/alsa.cpp @@ -19,9 +19,9 @@ namespace modules { alsa_module::alsa_module(const bar_settings& bar, string name_) : event_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 diff --git a/src/modules/backlight.cpp b/src/modules/backlight.cpp index 4119536b..19d86859 100644 --- a/src/modules/backlight.cpp +++ b/src/modules/backlight.cpp @@ -25,8 +25,8 @@ namespace modules { backlight_module::backlight_module(const bar_settings& bar, string name_) : inotify_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"); diff --git a/src/modules/bspwm.cpp b/src/modules/bspwm.cpp index c0a7228f..3257aed8 100644 --- a/src/modules/bspwm.cpp +++ b/src/modules/bspwm.cpp @@ -40,9 +40,9 @@ namespace modules { template class module; bspwm_module::bspwm_module(const bar_settings& bar, string name_) : event_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(); diff --git a/src/modules/date.cpp b/src/modules/date.cpp index 3d9c1836..7d243195 100644 --- a/src/modules/date.cpp +++ b/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); diff --git a/src/modules/i3.cpp b/src/modules/i3.cpp index d66ce117..94aee0eb 100644 --- a/src/modules/i3.cpp +++ b/src/modules/i3.cpp @@ -13,9 +13,9 @@ namespace modules { template class module; i3_module::i3_module(const bar_settings& bar, string name_) : event_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(); diff --git a/src/modules/ipc.cpp b/src/modules/ipc.cpp index 53b325e3..0e2a0995 100644 --- a/src/modules/ipc.cpp +++ b/src/modules/ipc.cpp @@ -15,7 +15,7 @@ namespace modules { * create formatting tags */ ipc_module::ipc_module(const bar_settings& bar, string name_) : 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; diff --git a/src/modules/menu.cpp b/src/modules/menu.cpp index 69d4c30b..c8fa6184 100644 --- a/src/modules/menu.cpp +++ b/src/modules/menu.cpp @@ -14,9 +14,9 @@ namespace modules { menu_module::menu_module(const bar_settings& bar, string name_) : static_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) { diff --git a/src/modules/mpd.cpp b/src/modules/mpd.cpp index db664e4d..c2658be7 100644 --- a/src/modules/mpd.cpp +++ b/src/modules/mpd.cpp @@ -13,16 +13,16 @@ namespace modules { template class module; mpd_module::mpd_module(const bar_settings& bar, string name_) : event_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); diff --git a/src/modules/pulseaudio.cpp b/src/modules/pulseaudio.cpp index 29b6286c..1eebb4b2 100644 --- a/src/modules/pulseaudio.cpp +++ b/src/modules/pulseaudio.cpp @@ -16,9 +16,9 @@ namespace modules { pulseaudio_module::pulseaudio_module(const bar_settings& bar, string name_) : event_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 diff --git a/src/modules/systray.cpp b/src/modules/systray.cpp index ae1cd348..e8c69188 100644 --- a/src/modules/systray.cpp +++ b/src/modules/systray.cpp @@ -16,7 +16,7 @@ namespace modules { */ systray_module::systray_module(const bar_settings& bar, string name_) : static_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}); diff --git a/src/modules/xbacklight.cpp b/src/modules/xbacklight.cpp index a84c38a4..d0ec3794 100644 --- a/src/modules/xbacklight.cpp +++ b/src/modules/xbacklight.cpp @@ -18,8 +18,8 @@ namespace modules { */ xbacklight_module::xbacklight_module(const bar_settings& bar, string name_) : static_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); diff --git a/src/modules/xkeyboard.cpp b/src/modules/xkeyboard.cpp index bae29913..c07941ed 100644 --- a/src/modules/xkeyboard.cpp +++ b/src/modules/xkeyboard.cpp @@ -24,7 +24,7 @@ namespace modules { */ xkeyboard_module::xkeyboard_module(const bar_settings& bar, string name_) : static_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 diff --git a/src/modules/xworkspaces.cpp b/src/modules/xworkspaces.cpp index bbebc466..b2acc2ea 100644 --- a/src/modules/xworkspaces.cpp +++ b/src/modules/xworkspaces.cpp @@ -35,9 +35,9 @@ namespace modules { */ xworkspaces_module::xworkspaces_module(const bar_settings& bar, string name_) : static_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); diff --git a/src/utils/action_router.cpp b/src/utils/action_router.cpp new file mode 100644 index 00000000..3a0118e1 --- /dev/null +++ b/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 diff --git a/tests/unit_tests/utils/action_router.cpp b/tests/unit_tests/utils/action_router.cpp index 21d043c6..656143ed 100644 --- a/tests/unit_tests/utils/action_router.cpp +++ b/tests/unit_tests/utils/action_router.cpp @@ -21,18 +21,18 @@ TEST(ActionRouterTest, CallsCorrectFunctions) { EXPECT_CALL(m, action2("foo")).Times(1); } - action_router 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 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 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); }