Merge pull request #549 from bebehei/threaded-dmenu

Threaded dmenu

Create a refcounting mechanism for notifications and "lock" them while displayed in dmenu.

Fixes #456

src/dbus.c

@@ -137,6 +137,7 @@ static struct notification *dbus_message_to_notification(const gchar *sender, GV
 
         n->actions = g_malloc0(sizeof(struct actions));
         n->dbus_client = g_strdup(sender);
+        n->dbus_valid = true;
 
         {
                 GVariantIter *iter = g_variant_iter_new(parameters);
@@ -282,7 +283,7 @@ static void on_notify(GDBusConnection *connection,
         // The message got discarded
         if (id == 0) {
                 signal_notification_closed(n, 2);
-                notification_free(n);
+                notification_unref(n);
         }
 
         wake_up();
@@ -316,6 +317,12 @@ static void on_get_server_information(GDBusConnection *connection,
 
 void signal_notification_closed(struct notification *n, enum reason reason)
 {
+        if (!n->dbus_valid) {
+                LOG_W("Closing notification '%s' not supported. "
+                      "Notification already closed.", n->summary);
+                return;
+        }
+
         if (reason < REASON_MIN || REASON_MAX < reason) {
                 LOG_W("Closing notification with reason '%d' not supported. "
                       "Closing it with reason '%d'.", reason, REASON_UNDEF);
@@ -337,6 +344,8 @@ void signal_notification_closed(struct notification *n, enum reason reason)
                                       body,
                                       &err);
 
+        n->dbus_valid = false;
+
         if (err) {
                 LOG_W("Unable to close notification: %s", err->message);
                 g_error_free(err);
@@ -346,6 +355,12 @@ void signal_notification_closed(struct notification *n, enum reason reason)
 
 void signal_action_invoked(const struct notification *n, const char *identifier)
 {
+        if (!n->dbus_valid) {
+                LOG_W("Invoking action '%s' not supported. "
+                      "Notification already closed.", identifier);
+                return;
+        }
+
         GVariant *body = g_variant_new("(us)", n->id, identifier);
         GError *err = NULL;
 

src/menu.c

@@ -23,6 +23,12 @@
 static bool is_initialized = false;
 static regex_t cregex;
 
+struct notification_lock {
+        struct notification *n;
+        gint64 timeout;
+};
+static gpointer context_menu_thread(gpointer data);
+
 static int regex_init(void)
 {
         if (is_initialized)
@@ -96,37 +102,45 @@ char *extract_urls(const char *to_match)
  */
 void open_browser(const char *in)
 {
-        char *url = NULL;
+        if (!settings.browser_cmd) {
+                LOG_C("Unable to open browser: No browser command set.");
+                return;
+        }
 
+        char *url, *end;
         // If any, remove leading [ linktext ] from URL
-        const char *end = strstr(in, "] ");
-        if (*in == '[' && end)
+        if (*in == '[' && (end = strstr(in, "] ")))
                 url = g_strdup(end + 2);
         else
                 url = g_strdup(in);
 
-        int browser_pid1 = fork();
+        int argc = 2+g_strv_length(settings.browser_cmd);
+        char **argv = g_malloc_n(argc, sizeof(char*));
 
-        if (browser_pid1) {
-                g_free(url);
-                int status;
-                waitpid(browser_pid1, &status, 0);
-        } else {
-                int browser_pid2 = fork();
-                if (browser_pid2) {
-                        exit(0);
-                } else {
-                        char *browser_cmd = g_strconcat(settings.browser, " ", url, NULL);
-                        char **cmd = g_strsplit(browser_cmd, " ", 0);
-                        execvp(cmd[0], cmd);
-                        // execvp won't return if it's successful
-                        // so, if we're here, it's definitely an error
-                        fprintf(stderr, "Warning: failed to execute '%s': %s\n",
-                                        settings.browser,
-                                        strerror(errno));
-                        exit(EXIT_FAILURE);
-                }
+        memcpy(argv, settings.browser_cmd, argc * sizeof(char*));
+        argv[argc-2] = url;
+        argv[argc-1] = NULL;
+
+        GError *err = NULL;
+        g_spawn_async(NULL,
+                      argv,
+                      NULL,
+                      G_SPAWN_DEFAULT
+                        | G_SPAWN_SEARCH_PATH
+                        | G_SPAWN_STDOUT_TO_DEV_NULL
+                        | G_SPAWN_STDERR_TO_DEV_NULL,
+                      NULL,
+                      NULL,
+                      NULL,
+                      &err);
+
+        if (err) {
+                LOG_C("Cannot spawn browser: %s", err->message);
+                g_error_free(err);
         }
+
+        g_free(argv);
+        g_free(url);
 }
 
 /*
@@ -171,38 +185,128 @@ void invoke_action(const char *action)
         }
 }
 
-/*
- * Dispatch whatever has been returned
- * by the menu.
+/**
+ * Dispatch whatever has been returned by dmenu.
+ * If the given result of dmenu is empty or NULL, nothing will be done.
+ *
+ * @param input The result from dmenu.
  */
 void dispatch_menu_result(const char *input)
 {
+        if (!input)
+                return;
+
         char *in = g_strdup(input);
         g_strstrip(in);
-        if (in[0] == '#') {
+
+        if (in[0] == '#')
                 invoke_action(in + 1);
-        } else {
+        else if (in[0] != '\0')
                 open_browser(in);
-        }
+
         g_free(in);
 }
 
+/** Call dmenu with the specified input. Blocks until dmenu is finished.
+ *
+ * @param dmenu_input The input string to feed into dmenu
+ * @returns the selected string from dmenu
+ */
+char *invoke_dmenu(const char *dmenu_input)
+{
+        if (!settings.dmenu_cmd) {
+                LOG_C("Unable to open dmenu: No dmenu command set.");
+                return NULL;
+        }
+
+        if (!dmenu_input || *dmenu_input == '\0')
+                return NULL;
+
+        gint dunst_to_dmenu;
+        gint dmenu_to_dunst;
+        GError *err = NULL;
+        char buf[1024];
+        char *ret = NULL;
+
+        g_spawn_async_with_pipes(NULL,
+                                 settings.dmenu_cmd,
+                                 NULL,
+                                 G_SPAWN_DEFAULT
+                                   | G_SPAWN_SEARCH_PATH,
+                                 NULL,
+                                 NULL,
+                                 NULL,
+                                 &dunst_to_dmenu,
+                                 &dmenu_to_dunst,
+                                 NULL,
+                                 &err);
+
+        if (err) {
+                LOG_C("Cannot spawn dmenu: %s", err->message);
+                g_error_free(err);
+        } else {
+                size_t wlen = strlen(dmenu_input);
+                if (write(dunst_to_dmenu, dmenu_input, wlen) != wlen) {
+                        LOG_W("Cannot feed dmenu with input: %s", strerror(errno));
+                }
+                close(dunst_to_dmenu);
+
+                ssize_t rlen = read(dmenu_to_dunst, buf, sizeof(buf));
+                close(dmenu_to_dunst);
+
+                if (rlen > 0)
+                        ret = g_strndup(buf, rlen);
+                else
+                        LOG_W("Didn't receive input from dmenu.");
+        }
+
+        return ret;
+}
+
 /*
  * Open the context menu that let's the user
  * select urls/actions/etc
  */
 void context_menu(void)
 {
-        if (!settings.dmenu_cmd) {
-                LOG_C("Unable to open dmenu: No dmenu command set.");
-                return;
+        GError *err = NULL;
+        g_thread_unref(g_thread_try_new("dmenu",
+                                        context_menu_thread,
+                                        NULL,
+                                        &err));
+
+        if (err) {
+                LOG_C("Cannot start thread to call dmenu: %s", err->message);
+                g_error_free(err);
         }
+}
+
+static gpointer context_menu_thread(gpointer data)
+{
         char *dmenu_input = NULL;
+        char *dmenu_output;
+
+        GList *locked_notifications = NULL;
 
         for (const GList *iter = queues_get_displayed(); iter;
              iter = iter->next) {
                 struct notification *n = iter->data;
 
+
+                // Reference and lock the notification if we need it
+                if (n->urls || n->actions) {
+                        notification_ref(n);
+
+                        struct notification_lock *nl =
+                                g_malloc(sizeof(struct notification_lock));
+
+                        nl->n = n;
+                        nl->timeout = n->timeout;
+                        n->timeout = 0;
+
+                        locked_notifications = g_list_prepend(locked_notifications, nl);
+                }
+
                 if (n->urls)
                         dmenu_input = string_append(dmenu_input, n->urls, "\n");
 
@@ -212,65 +316,27 @@ void context_menu(void)
                                           "\n");
         }
 
-        if (!dmenu_input)
-                return;
-
-        char buf[1024] = {0};
-        int child_io[2];
-        int parent_io[2];
-        if (pipe(child_io) != 0) {
-                LOG_W("pipe(): error in child: %s", strerror(errno));
-                g_free(dmenu_input);
-                return;
-        }
-        if (pipe(parent_io) != 0) {
-                LOG_W("pipe(): error in parent: %s", strerror(errno));
-                g_free(dmenu_input);
-                return;
-        }
-        int pid = fork();
-
-        if (pid == 0) {
-                close(child_io[1]);
-                close(parent_io[0]);
-                close(0);
-                if (dup(child_io[0]) == -1) {
-                        LOG_W("dup(): error in child: %s", strerror(errno));
-                        exit(EXIT_FAILURE);
-                }
-                close(1);
-                if (dup(parent_io[1]) == -1) {
-                        LOG_W("dup(): error in parent: %s", strerror(errno));
-                        exit(EXIT_FAILURE);
-                }
-                execvp(settings.dmenu_cmd[0], settings.dmenu_cmd);
-                fprintf(stderr, "Warning: failed to execute '%s': %s\n",
-                                settings.dmenu,
-                                strerror(errno));
-                exit(EXIT_FAILURE);
-        } else {
-                close(child_io[0]);
-                close(parent_io[1]);
-                size_t wlen = strlen(dmenu_input);
-                if (write(child_io[1], dmenu_input, wlen) != wlen) {
-                        LOG_W("write(): error: %s", strerror(errno));
-                }
-                close(child_io[1]);
-
-                size_t len = read(parent_io[0], buf, 1023);
-
-                waitpid(pid, NULL, 0);
-
-                if (len == 0) {
-                        g_free(dmenu_input);
-                        return;
-                }
-        }
-
-        close(parent_io[0]);
-
-        dispatch_menu_result(buf);
+        dmenu_output = invoke_dmenu(dmenu_input);
+        dispatch_menu_result(dmenu_output);
 
         g_free(dmenu_input);
+        g_free(dmenu_output);
+
+        // unref all notifications
+        for (GList *iter = locked_notifications;
+                    iter;
+                    iter = iter->next) {
+
+                struct notification_lock *nl = iter->data;
+                struct notification *n = nl->n;
+
+                n->timeout = nl->timeout;
+
+                g_free(nl);
+                notification_unref(n);
+        }
+        g_list_free(locked_notifications);
+
+        return NULL;
 }
 /* vim: set tabstop=8 shiftwidth=8 expandtab textwidth=0: */

src/notification.c

@@ -42,6 +42,10 @@ const char *enum_to_string_fullscreen(enum behavior_fullscreen in)
         }
 }
 
+struct _notification_private {
+        gint refcount;
+};
+
 /* see notification.h */
 void notification_print(const struct notification *n)
 {
@@ -206,12 +210,35 @@ void rawimage_free(struct raw_image *i)
         g_free(i);
 }
 
+static void notification_private_free(NotificationPrivate *p)
+{
+        g_free(p);
+}
+
 /* see notification.h */
-void notification_free(struct notification *n)
+gint notification_refcount_get(struct notification *n)
+{
+        assert(n->priv->refcount > 0);
+        return g_atomic_int_get(&n->priv->refcount);
+}
+
+/* see notification.h */
+void notification_ref(struct notification *n)
+{
+        assert(n->priv->refcount > 0);
+        g_atomic_int_inc(&n->priv->refcount);
+}
+
+/* see notification.h */
+void notification_unref(struct notification *n)
 {
         if (!n)
                 return;
 
+        assert(n->priv->refcount > 0);
+        if (!g_atomic_int_dec_and_test(&n->priv->refcount))
+                return;
+
         g_free(n->appname);
         g_free(n->summary);
         g_free(n->body);
@@ -228,6 +255,8 @@ void notification_free(struct notification *n)
         actions_free(n->actions);
         rawimage_free(n->raw_icon);
 
+        notification_private_free(n->priv);
+
         g_free(n);
 }
 
@@ -255,11 +284,21 @@ void notification_replace_single_field(char **haystack,
         g_free(input);
 }
 
+static NotificationPrivate *notification_private_create(void)
+{
+        NotificationPrivate *priv = g_malloc0(sizeof(NotificationPrivate));
+        g_atomic_int_set(&priv->refcount, 1);
+
+        return priv;
+}
+
 /* see notification.h */
 struct notification *notification_create(void)
 {
         struct notification *n = g_malloc0(sizeof(struct notification));
 
+        n->priv = notification_private_create();
+
         /* Unparameterized default values */
         n->first_render = true;
         n->markup = settings.markup;
@@ -274,6 +313,7 @@ struct notification *notification_create(void)
         n->progress = -1;
 
         n->script_run = false;
+        n->dbus_valid = false;
 
         n->fullscreen = FS_SHOW;
 

src/notification.h

@@ -42,9 +42,13 @@ struct actions {
         gsize count;
 };
 
+typedef struct _notification_private NotificationPrivate;
+
 struct notification {
+        NotificationPrivate *priv;
         int id;
         char *dbus_client;
+        bool dbus_valid;
 
         char *appname;
         char *summary;
@@ -90,11 +94,23 @@ struct notification {
  *  - the default (if it's not needed to be freed later)
  *  - its undefined representation (NULL, -1)
  *
+ * The reference counter is set to 1.
+ *
  * This function is guaranteed to return a valid pointer.
  * @returns The generated notification
  */
 struct notification *notification_create(void);
 
+/**
+ * Retrieve the current reference count of the notification
+ */
+gint notification_refcount_get(struct notification *n);
+
+/**
+ * Increase the reference counter of the notification.
+ */
+void notification_ref(struct notification *n);
+
 /**
  * Sanitize values of notification, apply all matching rules
  * and generate derived fields.
@@ -118,11 +134,11 @@ void actions_free(struct actions *a);
 void rawimage_free(struct raw_image *i);
 
 /**
- * Free the memory used by the given notification.
+ * Decrease the reference counter of the notification.
  *
- * @param n (nullable): pointer to #notification
+ * If the reference count drops to 0, the object gets freed.
  */
-void notification_free(struct notification *n);
+void notification_unref(struct notification *n);
 
 /**
  * Helper function to compare two given notifications.

src/option_parser.c

@@ -30,7 +30,6 @@ static struct section *new_section(const char *name);
 static struct section *get_section(const char *name);
 static void add_entry(const char *section_name, const char *key, const char *value);
 static const char *get_value(const char *section, const char *key);
-static char *clean_value(const char *value);
 
 static int cmdline_argc;
 static char **cmdline_argv;
@@ -90,7 +89,7 @@ void add_entry(const char *section_name, const char *key, const char *value)
         int len = s->entry_count;
         s->entries = g_realloc(s->entries, sizeof(struct entry) * len);
         s->entries[s->entry_count - 1].key = g_strdup(key);
-        s->entries[s->entry_count - 1].value = clean_value(value);
+        s->entries[s->entry_count - 1].value = string_strip_quotes(value);
 }
 
 const char *get_value(const char *section, const char *key)
@@ -201,21 +200,6 @@ int ini_get_bool(const char *section, const char *key, int def)
         }
 }
 
-char *clean_value(const char *value)
-{
-        char *s;
-
-        if (value[0] == '"')
-                s = g_strdup(value + 1);
-        else
-                s = g_strdup(value);
-
-        if (s[strlen(s) - 1] == '"')
-                s[strlen(s) - 1] = '\0';
-
-        return s;
-}
-
 int load_ini_file(FILE *fp)
 {
         if (!fp)

src/queues.c

@@ -191,7 +191,7 @@ static bool queues_stack_duplicate(struct notification *n)
                                 if ( allqueues[i] == displayed )
                                         n->start = time_monotonic_now();
 
-                                notification_free(orig);
+                                notification_unref(orig);
                                 return true;
                         }
                 }
@@ -218,7 +218,7 @@ bool queues_notification_replace_id(struct notification *new)
                                         notification_run_script(new);
                                 }
 
-                                notification_free(old);
+                                notification_unref(old);
                                 return true;
                         }
                 }
@@ -278,12 +278,12 @@ void queues_history_push(struct notification *n)
         if (!n->history_ignore) {
                 if (settings.history_length > 0 && history->length >= settings.history_length) {
                         struct notification *to_free = g_queue_pop_head(history);
-                        notification_free(to_free);
+                        notification_unref(to_free);
                 }
 
                 g_queue_push_tail(history, n);
         } else {
-                notification_free(n);
+                notification_unref(n);
         }
 }
 
@@ -488,7 +488,7 @@ bool queues_pause_status(void)
 static void teardown_notification(gpointer data)
 {
         struct notification *n = data;
-        notification_free(n);
+        notification_unref(n);
 }
 
 /* see queues.h */

src/settings.c

@@ -448,6 +448,16 @@ void load_settings(char *cmdline_config_path)
                 "path to browser"
         );
 
+        {
+                GError *error = NULL;
+                if (!g_shell_parse_argv(settings.browser, NULL, &settings.browser_cmd, &error)) {
+                        LOG_W("Unable to parse browser command: '%s'."
+                              " URL functionality will be disabled.", error->message);
+                        g_error_free(error);
+                        settings.browser_cmd = NULL;
+                }
+        }
+
         {
                 char *c = option_get_string(
                         "global",

src/settings.h

@@ -75,6 +75,7 @@ struct settings {
         char *dmenu;
         char **dmenu_cmd;
         char *browser;
+        char **browser_cmd;
         enum icon_position icon_position;
         int max_icon_size;
         char *icon_path;

src/utils.c

@@ -97,6 +97,23 @@ char *string_append(char *a, const char *b, const char *sep)
 
 }
 
+/* see utils.h */
+char *string_strip_quotes(const char *value)
+{
+        if (!value)
+                return NULL;
+
+        size_t len = strlen(value);
+        char *s;
+
+        if (value[0] == '"' && value[len-1] == '"')
+                s = g_strndup(value + 1, len-2);
+        else
+                s = g_strdup(value);
+
+        return s;
+}
+
 void string_strip_delimited(char *str, char a, char b)
 {
         int iread=-1, iwrite=0, copen=0;

src/utils.h

@@ -21,6 +21,14 @@ char *string_append(char *a, const char *b, const char *sep);
 /* strip content between two delimiter characters (inplace) */
 void string_strip_delimited(char *str, char a, char b);
 
+/**
+ * Strip quotes from a string. Won't touch inner quotes.
+ *
+ * @param value The string to strip the quotes from
+ * @returns A copy of the string value with the outer quotes removed (if any)
+ */
+char *string_strip_quotes(const char *value);
+
 /* replace tilde and path-specific values with its equivalents */
 char *string_to_path(char *string);
 

test/data/test-ini

@@ -22,6 +22,7 @@
 	simple = A simple string
 	quoted = "A quoted string"
 	quoted_with_quotes = "A string "with quotes""
+	unquoted_with_quotes = A" string with quotes"
 
 [path]
 	expand_tilde    = ~/.path/to/tilde

test/notification.c

@@ -98,29 +98,52 @@ TEST test_notification_replace_single_field(void)
         PASS();
 }
 
+TEST test_notification_referencing(void)
+{
+        struct notification *n = notification_create();
+        ASSERT(notification_refcount_get(n) == 1);
+
+        notification_ref(n);
+        ASSERT(notification_refcount_get(n) == 2);
+
+        notification_unref(n);
+        ASSERT(notification_refcount_get(n) == 1);
+
+        // Now we have to rely on valgrind to test, that
+        // it gets actually freed
+        notification_unref(n);
+
+        PASS();
+}
+
 SUITE(suite_notification)
 {
         cmdline_load(0, NULL);
         load_settings("data/dunstrc.default");
 
         struct notification *a = notification_create();
-        a->appname = "Test";
-        a->summary = "Summary";
-        a->body = "Body";
-        a->icon = "Icon";
+        a->appname = g_strdup("Test");
+        a->summary = g_strdup("Summary");
+        a->body = g_strdup("Body");
+        a->icon = g_strdup("Icon");
         a->urgency = URG_NORM;
 
         struct notification *b = notification_create();
-        memcpy(b, a, sizeof(*b));
+        b->appname = g_strdup("Test");
+        b->summary = g_strdup("Summary");
+        b->body = g_strdup("Body");
+        b->icon = g_strdup("Icon");
+        b->urgency = URG_NORM;
 
         //2 equal notifications to be passed for duplicate checking,
         struct notification *n[2] = {a, b};
 
         RUN_TEST1(test_notification_is_duplicate, (void*) n);
-        g_free(a);
-        g_free(b);
+        notification_unref(a);
+        notification_unref(b);
 
         RUN_TEST(test_notification_replace_single_field);
+        RUN_TEST(test_notification_referencing);
 
         g_clear_pointer(&settings.icon_path, g_free);
 }

test/option_parser.c

@@ -53,6 +53,8 @@ TEST test_ini_get_string(void)
         free(ptr);
         ASSERT_STR_EQ("A string \"with quotes\"", (ptr = ini_get_string(string_section, "quoted_with_quotes", "")));
         free(ptr);
+        ASSERT_STR_EQ("A\" string with quotes\"", (ptr = ini_get_string(string_section, "unquoted_with_quotes", "")));
+        free(ptr);
 
         ASSERT_STR_EQ("default value", (ptr = ini_get_string(string_section, "nonexistent", "default value")));
         free(ptr);

test/utils.c

@@ -104,6 +104,32 @@ TEST test_string_append(void)
         PASS();
 }
 
+TEST test_string_strip_quotes(void)
+{
+        char *exp = string_strip_quotes(NULL);
+        ASSERT_FALSE(exp);
+
+        ASSERT_STR_EQ("NewString", (exp = string_strip_quotes("NewString")));
+        g_free(exp);
+
+        ASSERT_STR_EQ("becomes unquoted", (exp = string_strip_quotes("\"becomes unquoted\"")));
+        g_free(exp);
+
+        ASSERT_STR_EQ("\"stays quoted", (exp = string_strip_quotes("\"stays quoted")));
+        g_free(exp);
+
+        ASSERT_STR_EQ("stays quoted\"", (exp = string_strip_quotes("stays quoted\"")));
+        g_free(exp);
+
+        ASSERT_STR_EQ("stays \"quoted\"", (exp = string_strip_quotes("stays \"quoted\"")));
+        g_free(exp);
+
+        ASSERT_STR_EQ(" \"stays quoted\"", (exp = string_strip_quotes(" \"stays quoted\"")));
+        g_free(exp);
+
+        PASS();
+}
+
 TEST test_string_strip_delimited(void)
 {
         char *text = malloc(128 * sizeof(char));
@@ -178,6 +204,7 @@ SUITE(suite_utils)
         RUN_TEST(test_string_replace_all);
         RUN_TEST(test_string_replace);
         RUN_TEST(test_string_append);
+        RUN_TEST(test_string_strip_quotes);
         RUN_TEST(test_string_strip_delimited);
         RUN_TEST(test_string_to_path);
         RUN_TEST(test_string_to_time);