module: Implement action router (#2336)

* module: Implement proof of concept action router

Action implementation inside module becomes much cleaner because each
module just registers action names together with a callback (pointer to
member function) and the action router does the rest.

* Make input function final

This forces all modules to use the action router

* modules: Catch exceptions in action handlers

* Use action router for all modules

* Use action_ prefix for function names

The mpd module's 'stop' action overwrote the base module's stop function
which caused difficult to debug behavior.

To prevent this in the future we now prefix each function that is
responsible for an action with 'action_'

* Cleanup

* actions: Throw exception when re-registering action

Action names are unique inside modules. Unfortunately there is no way to
ensure this statically, the next best thing is to crash the module and
let the user know that this is a bug.

* Formatting

* actions: Ignore data for actions without data

This is the same behavior as before.

* action_router: Write tests

include/modules/alsa.hpp

@@ -36,7 +36,15 @@ namespace modules {
     static constexpr auto EVENT_TOGGLE = "toggle";
 
    protected:
-    bool input(const string& action, const string& data);
+    void action_inc();
+    void action_dec();
+    void action_toggle();
+
+    void change_volume(int interval);
+
+    void action_epilogue(const vector<mixer_t>& mixers);
+
+    vector<mixer_t> get_mixers();
 
    private:
     static constexpr auto FORMAT_VOLUME = "format-volume";

include/modules/backlight.hpp

@@ -32,7 +32,10 @@ namespace modules {
     static constexpr const char* EVENT_DEC = "dec";
 
    protected:
-    bool input(const string& action, const string& data);
+    void action_inc();
+    void action_dec();
+
+    void change_value(int value_mod);
 
    private:
     static constexpr auto TAG_LABEL = "<label>";

include/modules/bspwm.hpp

@@ -54,7 +54,12 @@ namespace modules {
     static constexpr auto EVENT_PREV = "prev";
 
    protected:
-    bool input(const string& action, const string& data);
+    void action_focus(const string& data);
+    void action_next();
+    void action_prev();
+
+    void focus_direction(bool next);
+    void send_command(const string& payload_cmd, const string& log_info);
 
    private:
     bool handle_status(string& data);

include/modules/date.hpp

@@ -21,7 +21,7 @@ namespace modules {
     static constexpr auto EVENT_TOGGLE = "toggle";
 
    protected:
-    bool input(const string& action, const string& data);
+    void action_toggle();
 
    private:
     static constexpr auto TAG_LABEL = "<label>";

include/modules/i3.hpp

@@ -58,7 +58,11 @@ namespace modules {
     static constexpr auto EVENT_PREV = "prev";
 
    protected:
-    bool input(const string& action, const string& data);
+    void action_focus(const string& ws);
+    void action_next();
+    void action_prev();
+
+    void focus_direction(bool next);
 
    private:
     static string make_workspace_command(const string& workspace);

include/modules/menu.hpp

@@ -29,7 +29,9 @@ namespace modules {
     static constexpr auto EVENT_EXEC = "exec";
 
    protected:
-    bool input(const string& action, const string& data);
+    void action_open(const string& data);
+    void action_close();
+    void action_exec(const string& item);
 
    private:
     static constexpr auto TAG_LABEL_TOGGLE = "<label-toggle>";

include/modules/meta/base.hpp

@@ -42,9 +42,12 @@ class config;
 class logger;
 class signal_emitter;
 
+template <typename Impl>
+class action_router;
 // }}}
 
 namespace modules {
+
   using namespace drawtypes;
 
   DEFINE_ERROR(module_error);
@@ -156,7 +159,7 @@ namespace modules {
     void teardown();
     string contents();
 
-    bool input(const string& action, const string& data);
+    bool input(const string& action, const string& data) final;
 
    protected:
     void broadcast();
@@ -174,6 +177,8 @@ namespace modules {
     const logger& m_log;
     const config& m_conf;
 
+    unique_ptr<action_router<Impl>> m_router;
+
     mutex m_buildlock;
     mutex m_updatelock;
     mutex m_sleeplock;

include/modules/meta/base.inl

@@ -1,13 +1,17 @@
+#include <cassert>
+
 #include "components/builder.hpp"
 #include "components/config.hpp"
 #include "components/logger.hpp"
 #include "events/signal.hpp"
 #include "events/signal_emitter.hpp"
 #include "modules/meta/base.hpp"
+#include "utils/action_router.hpp"
 
 POLYBAR_NS
 
 namespace modules {
+
   // module<Impl> public {{{
 
   template <typename Impl>
@@ -16,6 +20,7 @@ namespace modules {
       , m_bar(bar)
       , m_log(logger::make())
       , m_conf(config::make())
+      , m_router(make_unique<action_router<Impl>>(CAST_MOD(Impl)))
       , m_name("module/" + name)
       , m_name_raw(name)
       , m_builder(make_unique<builder>(bar))
@@ -117,11 +122,19 @@ namespace modules {
   }
 
   template <typename Impl>
-  bool module<Impl>::input(const string&, const string&) {
+  bool module<Impl>::input(const string& name, const string& data) {
-    // By default a module doesn't support inputs
+    if (!m_router->has_action(name)) {
       return false;
     }
 
+    try {
+      m_router->invoke(name, data);
+    } catch (const exception& err) {
+      m_log.err("%s: Failed to handle command '%s' with data '%s' (%s)", this->name(), name, data, err.what());
+    }
+    return true;
+  }
+
   // }}}
   // module<Impl> protected {{{
 

include/modules/mpd.hpp

@@ -37,8 +37,17 @@ namespace modules {
     static constexpr const char* EVENT_CONSUME = "consume";
     static constexpr const char* EVENT_SEEK = "seek";
 
-   protected:
+   private:
-    bool input(const string& action, const string& data);
+    void action_play();
+    void action_pause();
+    void action_stop();
+    void action_prev();
+    void action_next();
+    void action_repeat();
+    void action_single();
+    void action_random();
+    void action_consume();
+    void action_seek(const string& data);
 
    private:
     static constexpr const char* FORMAT_ONLINE{"format-online"};

include/modules/pulseaudio.hpp

@@ -29,7 +29,9 @@ namespace modules {
     static constexpr auto EVENT_TOGGLE = "toggle";
 
    protected:
-    bool input(const string& action, const string& data);
+    void action_inc();
+    void action_dec();
+    void action_toggle();
 
    private:
     static constexpr auto FORMAT_VOLUME = "format-volume";

include/modules/systray.hpp

@@ -24,10 +24,9 @@ namespace modules {
     static constexpr auto EVENT_TOGGLE = "toggle";
 
    protected:
-    bool input(const string& action, const string& data);
+    void action_toggle();
 
    private:
-
     static constexpr const char* TAG_LABEL_TOGGLE{"<label-toggle>"};
     static constexpr const char* TAG_TRAY_CLIENTS{"<tray-clients>"};
 

include/modules/xbacklight.hpp

@@ -37,7 +37,11 @@ namespace modules {
 
    protected:
     void handle(const evt::randr_notify& evt);
-    bool input(const string& action, const string& data);
+
+    void action_inc();
+    void action_dec();
+
+    void change_value(int value_mod);
 
    private:
     static constexpr const char* TAG_LABEL{"<label>"};

include/modules/xkeyboard.hpp

@@ -38,7 +38,7 @@ namespace modules {
     void handle(const evt::xkb_state_notify& evt);
     void handle(const evt::xkb_indicator_state_notify& evt);
 
-    bool input(const string& action, const string& data);
+    void action_switch();
 
    private:
     static constexpr const char* TAG_LABEL_LAYOUT{"<label-layout>"};

include/modules/xworkspaces.hpp

@@ -72,7 +72,12 @@ namespace modules {
     void rebuild_desktops();
     void rebuild_desktop_states();
 
-    bool input(const string& action, const string& data);
+    void action_focus(const string& data);
+    void action_next();
+    void action_prev();
+
+    void focus_direction(bool next);
+    void focus_desktop(unsigned new_desktop);
 
    private:
     static vector<string> get_desktop_names();

include/utils/action_router.hpp

@@ -0,0 +1,94 @@
+#pragma once
+
+#include <cassert>
+#include <stdexcept>
+#include <unordered_map>
+
+#include "common.hpp"
+
+POLYBAR_NS
+
+/**
+ * Maps action names to function pointers in this module 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
+ *
+ * The input() function in the base class uses this for invoking the actions
+ * of that module.
+ *
+ * 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&);
+
+ 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
+  }
+
+ protected:
+  struct entry {
+    union {
+      callback without;
+      callback_data with;
+    };
+    bool with_data;
+  };
+
+  void register_entry(const string& name, const entry& e) {
+    if (has_action(name)) {
+      throw std::invalid_argument("Tried to register action '" + name + "' twice. THIS IS A BUG!");
+    }
+    callbacks[name] = e;
+  }
+
+ private:
+  std::unordered_map<string, entry> callbacks;
+  Impl* m_this;
+};
+
+POLYBAR_NS_END

src/components/controller.cpp

@@ -472,8 +472,6 @@ bool controller::try_forward_legacy_action(const string& cmd) {
       for (auto&& module : m_modules) {
         if (module->type() == type) {
           auto module_name = module->name_raw();
-          // TODO make this message more descriptive and maybe link to some documentation
-          // TODO use route to string methods to print action name that should be used.
           if (data.empty()) {
             m_log.warn("The action '%s' is deprecated, use '#%s.%s' instead!", cmd, module_name, action);
           } else {
@@ -546,7 +544,8 @@ void controller::switch_module_visibility(string module_name_raw, int visible) {
     return;
   }
 
-  m_log.err("controller: Module '%s' not found for visibility change (state=%s)", module_name_raw, visible ? "shown" : "hidden");
+  m_log.err("controller: Module '%s' not found for visibility change (state=%s)", module_name_raw,
+      visible ? "shown" : "hidden");
 }
 
 /**
@@ -694,8 +693,6 @@ bool controller::process_update(bool force) {
  * Creates module instances for all the modules in the given alignment block
  */
 size_t controller::setup_modules(alignment align) {
-  size_t count{0};
-
   string key;
 
   switch (align) {
@@ -739,13 +736,12 @@ size_t controller::setup_modules(alignment align) {
 
       m_modules.push_back(module);
       m_blocks[align].push_back(module);
-      count++;
+    } catch (const std::exception& err) {
-    } catch (const runtime_error& err) {
       m_log.err("Disabling module \"%s\" (reason: %s)", module_name, err.what());
     }
   }
 
-  return count;
+  return m_modules.size();
 }
 
 /**

src/modules/alsa.cpp

@@ -1,15 +1,14 @@
 #include "modules/alsa.hpp"
+
 #include "adapters/alsa/control.hpp"
 #include "adapters/alsa/generic.hpp"
 #include "adapters/alsa/mixer.hpp"
 #include "drawtypes/label.hpp"
 #include "drawtypes/progressbar.hpp"
 #include "drawtypes/ramp.hpp"
-#include "utils/math.hpp"
-
 #include "modules/meta/base.inl"
-
 #include "settings.hpp"
+#include "utils/math.hpp"
 
 POLYBAR_NS
 
@@ -19,6 +18,12 @@ namespace modules {
   template class module<alsa_module>;
 
   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);
+    }
+
     // Load configuration values
     m_mapped = m_conf.get(name(), "mapped", m_mapped);
     m_interval = m_conf.get(name(), "interval", m_interval);
@@ -218,14 +223,44 @@ namespace modules {
     return true;
   }
 
-  bool alsa_module::input(const string& action, const string&) {
+  void alsa_module::action_inc() {
-    if (!m_handle_events) {
+    change_volume(m_interval);
-      return false;
-    } else if (!m_mixer[mixer::MASTER]) {
-      return false;
   }
 
-    try {
+  void alsa_module::action_dec() {
+    change_volume(-m_interval);
+  }
+
+  void alsa_module::action_toggle() {
+    if (!m_mixer[mixer::MASTER]) {
+      return;
+    }
+    auto mixers = get_mixers();
+    for (auto&& mixer : mixers) {
+      mixer->set_mute(m_muted || mixers[0]->is_muted());
+    }
+  }
+
+  void alsa_module::change_volume(int interval) {
+    if (!m_mixer[mixer::MASTER]) {
+      return;
+    }
+    auto mixers = get_mixers();
+    for (auto&& mixer : mixers) {
+      m_mapped ? mixer->set_normalized_volume(math_util::cap<float>(mixer->get_normalized_volume() + interval, 0, 100))
+               : mixer->set_volume(math_util::cap<float>(mixer->get_volume() + interval, 0, 100));
+    }
+  }
+
+  void action_epilogue(const vector<mixer_t>& mixers) {
+    for (auto&& mixer : mixers) {
+      if (mixer->wait(0)) {
+        mixer->process_events();
+      }
+    }
+  }
+
+  vector<mixer_t> alsa_module::get_mixers() {
     vector<mixer_t> mixers;
     bool headphones{m_headphones};
 
@@ -242,35 +277,8 @@ namespace modules {
           string{m_mixer[mixer::SPEAKER]->get_name()}, string{m_mixer[mixer::SPEAKER]->get_sound_card()}));
     }
 
-      if (action == EVENT_TOGGLE) {
+    return mixers;
-        for (auto&& mixer : mixers) {
-          mixer->set_mute(m_muted || mixers[0]->is_muted());
   }
-      } else if (action == EVENT_INC) {
+}  // namespace modules
-        for (auto&& mixer : mixers) {
-          m_mapped ? mixer->set_normalized_volume(math_util::cap<float>(mixer->get_normalized_volume() + m_interval, 0, 100))
-                   : mixer->set_volume(math_util::cap<float>(mixer->get_volume() + m_interval, 0, 100));
-        }
-      } else if (action == EVENT_DEC) {
-        for (auto&& mixer : mixers) {
-          m_mapped ? mixer->set_normalized_volume(math_util::cap<float>(mixer->get_normalized_volume() - m_interval, 0, 100))
-                   : mixer->set_volume(math_util::cap<float>(mixer->get_volume() - m_interval, 0, 100));
-        }
-      } else {
-        return false;
-      }
-
-      for (auto&& mixer : mixers) {
-        if (mixer->wait(0)) {
-          mixer->process_events();
-        }
-      }
-    } catch (const exception& err) {
-      m_log.err("%s: Failed to handle command (%s)", name(), err.what());
-    }
-
-    return true;
-  }
-}
 
 POLYBAR_NS_END

src/modules/backlight.cpp

@@ -25,6 +25,9 @@ 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);
+
     auto card = m_conf.get(name(), "card");
 
     // Get flag to check if we should add scroll handlers for changing value
@@ -113,18 +116,16 @@ namespace modules {
     return true;
   }
 
-  bool backlight_module::input(const string& action, const string&) {
+  void backlight_module::action_inc() {
-    double value_mod{0.0};
+    change_value(5);
-
-    if (action == EVENT_INC) {
-      value_mod = 5.0;
-    } else if (action == EVENT_DEC) {
-      value_mod = -5.0;
-    } else {
-      return false;
   }
 
-    m_log.info("%s: Changing value by %f%", name(), value_mod);
+  void backlight_module::action_dec() {
+    change_value(-5);
+  }
+
+  void backlight_module::change_value(int value_mod) {
+    m_log.info("%s: Changing value by %d%", name(), value_mod);
 
     try {
       int rounded = math_util::cap<double>(m_percentage + value_mod, 0.0, 100.0) + 0.5;
@@ -136,8 +137,6 @@ namespace modules {
           "configuration. Please read the module documentation.\n(reason: %s)",
           name(), err.what());
     }
-
-    return true;
   }
 }  // namespace modules
 

src/modules/bspwm.cpp

@@ -41,6 +41,10 @@ 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);
+
     auto socket_path = bspwm_util::get_socket_path();
 
     if (!file_util::exists(socket_path)) {
@@ -445,20 +449,7 @@ namespace modules {
     return false;
   }
 
-  bool bspwm_module::input(const string& action, const string& data) {
+  void bspwm_module::action_focus(const string& data) {
-    auto send_command = [this](string payload_cmd, string log_info) {
-      try {
-        auto ipc = bspwm_util::make_connection();
-        auto payload = bspwm_util::make_payload(payload_cmd);
-        m_log.info("%s: %s", name(), log_info);
-        ipc->send(payload->data, payload->len, 0);
-        ipc->disconnect();
-      } catch (const system_error& err) {
-        m_log.err("%s: %s", name(), err.what());
-      }
-    };
-
-    if (action == EVENT_FOCUS) {
     size_t separator{string_util::find_nth(data, 0, "+", 1)};
     size_t monitor_n{std::strtoul(data.substr(0, separator).c_str(), nullptr, 10)};
     string workspace_n{data.substr(separator + 1)};
@@ -469,20 +460,17 @@ namespace modules {
     } else {
       m_log.err("%s: Invalid monitor index in command: %s", name(), data);
     }
-
+  }
-      return true;
+  void bspwm_module::action_next() {
+    focus_direction(true);
   }
 
-    string scrolldir;
+  void bspwm_module::action_prev() {
-
+    focus_direction(false);
-    if (action == EVENT_NEXT) {
-      scrolldir = "next";
-    } else if (action == EVENT_PREV) {
-      scrolldir = "prev";
-    } else {
-      return false;
   }
 
+  void bspwm_module::focus_direction(bool next) {
+    string scrolldir = next ? "next" : "prev";
     string modifier;
 
     if (m_pinworkspaces) {
@@ -496,8 +484,14 @@ namespace modules {
     }
 
     send_command("desktop -f " + scrolldir + modifier, "Sending desktop " + scrolldir + " command to ipc handler");
+  }
 
-    return true;
+  void bspwm_module::send_command(const string& payload_cmd, const string& log_info) {
+    auto ipc = bspwm_util::make_connection();
+    auto payload = bspwm_util::make_payload(payload_cmd);
+    m_log.info("%s: %s", name(), log_info);
+    ipc->send(payload->data, payload->len, 0);
+    ipc->disconnect();
   }
 }  // namespace modules
 

src/modules/date.cpp

@@ -13,6 +13,8 @@ namespace modules {
       datetime_stream.imbue(std::locale(m_bar.locale.c_str()));
     }
 
+    m_router->register_action(EVENT_TOGGLE, &date_module::action_toggle);
+
     m_dateformat = m_conf.get(name(), "date", ""s);
     m_dateformat_alt = m_conf.get(name(), "date-alt", ""s);
     m_timeformat = m_conf.get(name(), "time", ""s);
@@ -83,13 +85,9 @@ namespace modules {
     return true;
   }
 
-  bool date_module::input(const string& action, const string&) {
+  void date_module::action_toggle() {
-    if (action != EVENT_TOGGLE) {
-      return false;
-    }
     m_toggled = !m_toggled;
     wakeup();
-    return true;
   }
 }  // namespace modules
 

src/modules/i3.cpp

@@ -14,6 +14,10 @@ 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);
+
     auto socket_path = i3ipc::get_socketpath();
 
     if (!file_util::exists(socket_path)) {
@@ -217,28 +221,29 @@ namespace modules {
     return true;
   }
 
-  bool i3_module::input(const string& action, const string& data) {
+  void i3_module::action_focus(const string& ws) {
-    try {
     const i3_util::connection_t conn{};
-
-      if (action == EVENT_FOCUS) {
     m_log.info("%s: Sending workspace focus command to ipc handler", name());
-        conn.send_command(make_workspace_command(data));
+    conn.send_command(make_workspace_command(ws));
-        return true;
   }
 
-      if (action != EVENT_NEXT && action != EVENT_PREV) {
+  void i3_module::action_next() {
-        return false;
+    focus_direction(true);
   }
 
-      bool next = action == EVENT_NEXT;
+  void i3_module::action_prev() {
+    focus_direction(false);
+  }
+
+  void i3_module::focus_direction(bool next) {
+    const i3_util::connection_t conn{};
 
     auto workspaces = i3_util::workspaces(conn, m_bar.monitor->name);
     auto current_ws = std::find_if(workspaces.begin(), workspaces.end(), [](auto ws) { return ws->visible; });
 
     if (current_ws == workspaces.end()) {
       m_log.warn("%s: Current workspace not found", name());
-        return false;
+      return;
     }
 
     if (next && (m_wrap || std::next(current_ws) != workspaces.end())) {
@@ -256,12 +261,6 @@ namespace modules {
       m_log.info("%s: Sending workspace prev_on_output command to ipc handler", name());
       conn.send_command("workspace prev_on_output");
     }
-
-    } catch (const exception& err) {
-      m_log.err("%s: %s", name(), err.what());
-    }
-
-    return true;
   }
 
   string i3_module::make_workspace_command(const string& workspace) {

src/modules/menu.cpp

@@ -15,8 +15,12 @@ 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);
+
     string default_format;
-    if(m_expand_right) {
+    if (m_expand_right) {
       default_format += TAG_LABEL_TOGGLE;
       default_format += TAG_MENU;
     } else {
@@ -24,7 +28,6 @@ namespace modules {
       default_format += TAG_LABEL_TOGGLE;
     }
 
-
     m_formatter->add(DEFAULT_FORMAT, default_format, {TAG_LABEL_TOGGLE, TAG_MENU});
 
     if (m_formatter->has(TAG_LABEL_TOGGLE)) {
@@ -71,7 +74,7 @@ namespace modules {
       builder->action(mousebtn::LEFT, *this, EVENT_CLOSE, "", m_labelclose);
     } else if (tag == TAG_MENU && m_level > -1) {
       auto spacing = m_formatter->get(get_format())->spacing;
-      //Insert separator after menu-toggle and before menu-items for expand-right=true
+      // Insert separator after menu-toggle and before menu-items for expand-right=true
       if (m_expand_right && *m_labelseparator) {
         builder->node(m_labelseparator);
         builder->space(spacing);
@@ -87,7 +90,7 @@ namespace modules {
             builder->node(m_labelseparator);
             builder->space(spacing);
           }
-        //Insert separator after last menu-item and before menu-toggle for expand-right=false
+          // Insert separator after last menu-item and before menu-toggle for expand-right=false
         } else if (!m_expand_right && *m_labelseparator) {
           builder->space(spacing);
           builder->node(m_labelseparator);
@@ -99,21 +102,40 @@ namespace modules {
     return true;
   }
 
-  bool menu_module::input(const string& action, const string& data) {
+  void menu_module::action_open(const string& data) {
-    if (action == EVENT_EXEC) {
+    string level = data.empty() ? "0" : data;
-      auto sep = data.find("-");
+    int level_num = m_level = std::strtol(level.c_str(), nullptr, 10);
+    m_log.info("%s: Opening menu level '%i'", name(), static_cast<int>(level_num));
 
-      if (sep == data.npos) {
+    if (static_cast<size_t>(level_num) >= m_levels.size()) {
-        m_log.err("%s: Malformed data for exec action (data: '%s')", name(), data);
+      m_log.warn("%s: Cannot open unexisting menu level '%s'", name(), level);
-        return false;
+      m_level = -1;
+    } else {
+      m_level = level_num;
+    }
+    broadcast();
   }
 
-      auto level = std::strtoul(data.substr(0, sep).c_str(), nullptr, 10);
+  void menu_module::action_close() {
-      auto item = std::strtoul(data.substr(sep + 1).c_str(), nullptr, 10);
+    m_log.info("%s: Closing menu tree", name());
+    if (m_level != -1) {
+      m_level = -1;
+      broadcast();
+    }
+  }
+
+  void menu_module::action_exec(const string& element) {
+    auto sep = element.find("-");
+
+    if (sep == element.npos) {
+      m_log.err("%s: Malformed data for exec action (data: '%s')", name(), element);
+    }
+
+    auto level = std::strtoul(element.substr(0, sep).c_str(), nullptr, 10);
+    auto item = std::strtoul(element.substr(sep + 1).c_str(), nullptr, 10);
 
     if (level >= m_levels.size() || item >= m_levels[level]->items.size()) {
-        m_log.err("%s: menu-exec-%d-%d doesn't exist (data: '%s')", name(), level, item, data);
+      m_log.err("%s: menu-exec-%d-%d doesn't exist (data: '%s')", name(), level, item, element);
-        return false;
     }
 
     string exec = m_levels[level]->items[item]->exec;
@@ -125,34 +147,11 @@ namespace modules {
      * This stops the menu from closing, if the exec action comes from an
      * external source
      */
-      if (m_level == (int) level) {
+    if (m_level == (int)level) {
       m_level = -1;
       broadcast();
     }
-
-    } else if (action == EVENT_OPEN) {
-      auto level = data;
-
-      if (level.empty()) {
-        level = "0";
   }
-      m_level = std::strtol(level.c_str(), nullptr, 10);
+}  // namespace modules
-      m_log.info("%s: Opening menu level '%i'", name(), static_cast<int>(m_level));
-
-      if (static_cast<size_t>(m_level) >= m_levels.size()) {
-        m_log.warn("%s: Cannot open unexisting menu level '%s'", name(), level);
-        m_level = -1;
-      }
-    } else if (action == EVENT_CLOSE) {
-      m_log.info("%s: Closing menu tree", name());
-      m_level = -1;
-    } else {
-      return false;
-    }
-
-    broadcast();
-    return true;
-  }
-}
 
 POLYBAR_NS_END

src/modules/mpd.cpp

@@ -14,6 +14,17 @@ 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_host = m_conf.get(name(), "host", m_host);
     m_port = m_conf.get(name(), "port", m_port);
     m_pass = m_conf.get(name(), "password", m_pass);
@@ -350,41 +361,82 @@ namespace modules {
     return true;
   }
 
-  bool mpd_module::input(const string& action, const string& data) {
+/**
-    m_log.info("%s: event: %s", name(), action);
+ * Small macro to create a temporary mpd connection for the action handlers.
+ *
+ * We have to create a separate mpd instance because actions run in the
+ * controller thread and the `m_mpd` pointer is used in the module thread.
+ */
+#define MPD_CONNECT()                                                            \
+  auto mpd = factory_util::unique<mpdconnection>(m_log, m_host, m_port, m_pass); \
+  mpd->connect();                                                                \
+  auto status = mpd->get_status()
 
-    try {
+  void mpd_module::action_play() {
-      auto mpd = factory_util::unique<mpdconnection>(m_log, m_host, m_port, m_pass);
+    MPD_CONNECT();
-      mpd->connect();
+    if (!status->match_state(mpdstate::PLAYING)) {
-
-      auto status = mpd->get_status();
-
-      bool is_playing = status->match_state(mpdstate::PLAYING);
-      bool is_paused = status->match_state(mpdstate::PAUSED);
-      bool is_stopped = status->match_state(mpdstate::STOPPED);
-
-      if (action == EVENT_PLAY && !is_playing) {
       mpd->play();
-      } else if (action == EVENT_PAUSE && !is_paused) {
+    }
+  }
+
+  void mpd_module::action_pause() {
+    MPD_CONNECT();
+
+    if (!status->match_state(mpdstate::PAUSED)) {
       mpd->pause(true);
-      } else if (action == EVENT_STOP && !is_stopped) {
+    }
+  }
+
+  void mpd_module::action_stop() {
+    MPD_CONNECT();
+
+    if (!status->match_state(mpdstate::STOPPED)) {
       mpd->stop();
-      } else if (action == EVENT_PREV && !is_stopped) {
+    }
+  }
+
+  void mpd_module::action_prev() {
+    MPD_CONNECT();
+
+    if (!status->match_state(mpdstate::STOPPED)) {
       mpd->prev();
-      } else if (action == EVENT_NEXT && !is_stopped) {
+    }
+  }
+
+  void mpd_module::action_next() {
+    MPD_CONNECT();
+
+    if (!status->match_state(mpdstate::STOPPED)) {
       mpd->next();
-      } else if (action == EVENT_SINGLE) {
+    }
-        mpd->set_single(!status->single());
+  }
-      } else if (action == EVENT_REPEAT) {
+
+  void mpd_module::action_repeat() {
+    MPD_CONNECT();
     mpd->set_repeat(!status->repeat());
-      } else if (action == EVENT_RANDOM) {
+  }
+
+  void mpd_module::action_single() {
+    MPD_CONNECT();
+    mpd->set_single(!status->single());
+  }
+
+  void mpd_module::action_random() {
+    MPD_CONNECT();
     mpd->set_random(!status->random());
-      } else if (action == EVENT_CONSUME) {
+  }
+
+  void mpd_module::action_consume() {
+    MPD_CONNECT();
     mpd->set_consume(!status->consume());
-      } else if (action == EVENT_SEEK) {
+  }
+
+  void mpd_module::action_seek(const string& data) {
+    MPD_CONNECT();
+
     int percentage = 0;
     if (data.empty()) {
-          return false;
+      return;
     } else if (data[0] == '+') {
       percentage = status->get_elapsed_percentage() + std::strtol(data.substr(1).c_str(), nullptr, 10);
     } else if (data[0] == '-') {
@@ -393,16 +445,9 @@ namespace modules {
       percentage = std::strtol(data.c_str(), nullptr, 10);
     }
     mpd->seek(status->get_songid(), status->get_seek_position(percentage));
-      } else {
-        return false;
-      }
-    } catch (const mpd_exception& err) {
-      m_log.err("%s: %s", name(), err.what());
-      m_mpd.reset();
   }
 
-    return true;
+#undef MPD_CONNECT
-  }
 }  // namespace modules
 
 POLYBAR_NS_END

src/modules/pulseaudio.cpp

@@ -15,6 +15,12 @@ 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);
+    }
+
     // Load configuration values
     m_interval = m_conf.get(name(), "interval", m_interval);
 
@@ -142,29 +148,16 @@ namespace modules {
     return true;
   }
 
-  bool pulseaudio_module::input(const string& action, const string&) {
+  void pulseaudio_module::action_inc() {
-    if (!m_handle_events) {
-      return false;
-    }
-
-    try {
-      if (m_pulseaudio && !m_pulseaudio->get_name().empty()) {
-        if (action == EVENT_TOGGLE) {
-          m_pulseaudio->toggle_mute();
-        } else if (action == EVENT_INC) {
-          // cap above 100 (~150)?
     m_pulseaudio->inc_volume(m_interval);
-        } else if (action == EVENT_DEC) {
-          m_pulseaudio->inc_volume(-m_interval);
-        } else {
-          return false;
-        }
-      }
-    } catch (const exception& err) {
-      m_log.err("%s: Failed to handle command (%s)", name(), err.what());
   }
 
-    return true;
+  void pulseaudio_module::action_dec() {
+    m_pulseaudio->inc_volume(-m_interval);
+  }
+
+  void pulseaudio_module::action_toggle() {
+    m_pulseaudio->toggle_mute();
   }
 }  // namespace modules
 

src/modules/systray.cpp

@@ -1,11 +1,11 @@
 #if DEBUG
 #include "modules/systray.hpp"
+
 #include "drawtypes/label.hpp"
+#include "modules/meta/base.inl"
 #include "x11/connection.hpp"
 #include "x11/tray_manager.hpp"
 
-#include "modules/meta/base.inl"
-
 POLYBAR_NS
 
 namespace modules {
@@ -16,6 +16,8 @@ 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);
+
     // Add formats and elements
     m_formatter->add(DEFAULT_FORMAT, TAG_LABEL_TOGGLE, {TAG_LABEL_TOGGLE, TAG_TRAY_CLIENTS});
 
@@ -53,17 +55,11 @@ namespace modules {
   /**
    * Handle input event
    */
-  bool systray_module::input(const string& action, const string&) {
+  void systray_module::action_toggle() {
-    if (action.find(EVENT_TOGGLE) != 0) {
-      return false;
-    }
-
     m_hidden = !m_hidden;
     broadcast();
-
-    return true;
   }
-}
+}  // namespace modules
 
 POLYBAR_NS_END
 #endif

src/modules/xbacklight.cpp

@@ -1,13 +1,13 @@
 #include "modules/xbacklight.hpp"
+
 #include "drawtypes/label.hpp"
 #include "drawtypes/progressbar.hpp"
 #include "drawtypes/ramp.hpp"
+#include "modules/meta/base.inl"
 #include "utils/math.hpp"
 #include "x11/connection.hpp"
 #include "x11/winspec.hpp"
 
-#include "modules/meta/base.inl"
-
 POLYBAR_NS
 
 namespace modules {
@@ -18,6 +18,9 @@ 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);
+
     auto output = m_conf.get(name(), "output", m_bar.monitor->name);
 
     auto monitors = randr_util::get_monitors(m_connection, m_connection.root(), bar.monitor_strict, false);
@@ -144,34 +147,23 @@ namespace modules {
     return true;
   }
 
-  /**
+  void xbacklight_module::action_inc() {
-   * Process scroll events by changing backlight value
+    change_value(5);
-   */
-  bool xbacklight_module::input(const string& action, const string&) {
-    double value_mod{0.0};
-
-    if (action == EVENT_INC) {
-      value_mod = 5.0;
-      m_log.info("%s: Increasing value by %i%", name(), value_mod);
-    } else if (action == EVENT_DEC) {
-      value_mod = -5.0;
-      m_log.info("%s: Decreasing value by %i%", name(), -value_mod);
-    } else {
-      return false;
   }
 
-    try {
+  void xbacklight_module::action_dec() {
+    change_value(-5);
+  }
+
+  void xbacklight_module::change_value(int value_mod) {
+    m_log.info("%s: Changing value by %i%", name(), value_mod);
     int rounded = math_util::cap<double>(m_percentage + value_mod, 0.0, 100.0) + 0.5;
+
     const int values[1]{math_util::percentage_to_value<int>(rounded, m_output->backlight.max)};
 
     m_connection.change_output_property_checked(
         m_output->output, m_output->backlight.atom, XCB_ATOM_INTEGER, 32, XCB_PROP_MODE_REPLACE, 1, values);
-    } catch (const exception& err) {
-      m_log.err("%s: %s", name(), err.what());
   }
-
+}  // namespace modules
-    return true;
-  }
-}
 
 POLYBAR_NS_END

src/modules/xkeyboard.cpp

@@ -25,6 +25,8 @@ 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);
+
     // Setup extension
     // clang-format off
     m_connection.xkb().select_events_checked(XCB_XKB_ID_USE_CORE_KBD,
@@ -208,11 +210,7 @@ namespace modules {
   /**
    * Handle input command
    */
-  bool xkeyboard_module::input(const string& action, const string&) {
+  void xkeyboard_module::action_switch() {
-    if (action != EVENT_SWITCH) {
-      return false;
-    }
-
     size_t current_group = m_keyboard->current() + 1;
 
     if (current_group >= m_keyboard->size()) {
@@ -224,8 +222,6 @@ namespace modules {
     m_connection.flush();
 
     update();
-
-    return true;
   }
 
   /**

src/modules/xworkspaces.cpp

@@ -35,6 +35,10 @@ 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);
+
     // Load config values
     m_pinworkspaces = m_conf.get(name(), "pin-workspaces", m_pinworkspaces);
     m_click = m_conf.get(name(), "enable-click", m_click);
@@ -357,12 +361,21 @@ namespace modules {
     }
   }
 
-  /**
+  void xworkspaces_module::action_focus(const string& data) {
-   * Handle user input event
-   */
-  bool xworkspaces_module::input(const string& action, const string& data) {
     std::lock_guard<std::mutex> lock(m_workspace_mutex);
+    focus_desktop(std::strtoul(data.c_str(), nullptr, 10));
+  }
 
+  void xworkspaces_module::action_next() {
+    focus_direction(true);
+  }
+
+  void xworkspaces_module::action_prev() {
+    focus_direction(false);
+  }
+
+  void xworkspaces_module::focus_direction(bool next) {
+    std::lock_guard<std::mutex> lock(m_workspace_mutex);
     vector<unsigned int> indexes;
     for (auto&& viewport : m_viewports) {
       for (auto&& desktop : viewport->desktops) {
@@ -370,29 +383,28 @@ namespace modules {
       }
     }
 
-    std::sort(indexes.begin(), indexes.end());
+    unsigned new_desktop;
-
-    unsigned int new_desktop{0};
     unsigned int current_desktop{ewmh_util::get_current_desktop()};
 
-    if (action == EVENT_FOCUS) {
+    if (next) {
-      new_desktop = std::strtoul(data.c_str(), nullptr, 10);
-    } else if (action == EVENT_NEXT) {
       new_desktop = math_util::min<unsigned int>(indexes.back(), current_desktop + 1);
       new_desktop = new_desktop == current_desktop ? indexes.front() : new_desktop;
-    } else if (action == EVENT_PREV) {
+    } else {
       new_desktop = math_util::max<unsigned int>(indexes.front(), current_desktop - 1);
       new_desktop = new_desktop == current_desktop ? indexes.back() : new_desktop;
     }
 
+    focus_desktop(new_desktop);
+  }
+
+  void xworkspaces_module::focus_desktop(unsigned new_desktop) {
+    unsigned int current_desktop{ewmh_util::get_current_desktop()};
     if (new_desktop != current_desktop) {
       m_log.info("%s: Requesting change to desktop #%u", name(), new_desktop);
       ewmh_util::change_current_desktop(new_desktop);
     } else {
       m_log.info("%s: Ignoring change to current desktop", name());
     }
-
-    return true;
   }
 }  // namespace modules
 

tests/CMakeLists.txt

@@ -47,6 +47,7 @@ function(add_unit_test source_file)
 endfunction()
 
 add_unit_test(utils/actions)
+add_unit_test(utils/action_router)
 add_unit_test(utils/color)
 add_unit_test(utils/command)
 add_unit_test(utils/math)

tests/unit_tests/utils/action_router.cpp

@@ -0,0 +1,48 @@
+#include "utils/action_router.hpp"
+
+#include "common/test.hpp"
+#include "gmock/gmock.h"
+
+using namespace polybar;
+using ::testing::InSequence;
+
+class MockModule {
+ public:
+  MOCK_METHOD(void, action1, ());
+  MOCK_METHOD(void, action2, (const string&));
+};
+
+TEST(ActionRouterTest, CallsCorrectFunctions) {
+  MockModule m;
+
+  {
+    InSequence seq;
+    EXPECT_CALL(m, action1()).Times(1);
+    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);
+  router.invoke("action1", "");
+  router.invoke("action2", "foo");
+}
+
+TEST(ActionRouterTest, HasAction) {
+  MockModule m;
+  action_router<MockModule> router(&m);
+
+  router.register_action("foo", &MockModule::action1);
+
+  EXPECT_TRUE(router.has_action("foo"));
+  EXPECT_FALSE(router.has_action("bar"));
+}
+
+TEST(ActionRouterTest, ThrowsOnDuplicate) {
+  MockModule m;
+  action_router<MockModule> router(&m);
+
+  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);
+}