From b666fc76a6de9d4eb24ca9a1e458736b1ffee974 Mon Sep 17 00:00:00 2001 From: Ian Fan Date: Wed, 2 Jan 2019 22:31:05 +0000 Subject: [PATCH 1/3] swaybar: free the right item during tray destruction Also added a comment to make more obvious the reason for comparing sni->status[0] == 'N' --- swaybar/tray/item.c | 4 ++-- swaybar/tray/tray.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/swaybar/tray/item.c b/swaybar/tray/item.c index 41cacd168..130496ecc 100644 --- a/swaybar/tray/item.c +++ b/swaybar/tray/item.c @@ -19,7 +19,7 @@ // TODO menu static bool sni_ready(struct swaybar_sni *sni) { - return sni->status && (sni->status[0] == 'N' ? + return sni->status && (sni->status[0] == 'N' ? // NeedsAttention sni->attention_icon_name || sni->attention_icon_pixmap : sni->icon_name || sni->icon_pixmap); } @@ -396,7 +396,7 @@ uint32_t render_sni(cairo_t *cairo, struct swaybar_output *output, double *x, icon_size = actual_size < ideal_size ? actual_size*(ideal_size/actual_size) : ideal_size; icon = cairo_image_surface_scale(sni->icon, icon_size, icon_size); - } else { // draw a sad face + } else { // draw a :( icon_size = ideal_size*0.8; icon = cairo_image_surface_create(CAIRO_FORMAT_ARGB32, icon_size, icon_size); cairo_t *cairo_icon = cairo_create(icon); diff --git a/swaybar/tray/tray.c b/swaybar/tray/tray.c index acc300afd..0c3517cb7 100644 --- a/swaybar/tray/tray.c +++ b/swaybar/tray/tray.c @@ -79,7 +79,7 @@ void destroy_tray(struct swaybar_tray *tray) { finish_host(&tray->host_xdg); finish_host(&tray->host_kde); for (int i = 0; i < tray->items->length; ++i) { - destroy_sni(tray->items->items[0]); + destroy_sni(tray->items->items[i]); } list_free(tray->items); destroy_watcher(tray->watcher_xdg); From d093c3ac55649e9a30504cfeaff506329ffd6ec7 Mon Sep 17 00:00:00 2001 From: Ian Fan Date: Fri, 4 Jan 2019 11:57:18 +0000 Subject: [PATCH 2/3] swaybar: handle SNI signals better This fixes a crash caused by callbacks not matching the right sender, and frees old values later, before they are re-assigned. --- include/swaybar/tray/item.h | 4 ++ swaybar/tray/item.c | 94 ++++++++++++++++++++++++------------- 2 files changed, 65 insertions(+), 33 deletions(-) diff --git a/include/swaybar/tray/item.h b/include/swaybar/tray/item.h index 9bba79515..4238bdb8e 100644 --- a/include/swaybar/tray/item.h +++ b/include/swaybar/tray/item.h @@ -35,6 +35,10 @@ struct swaybar_sni { bool item_is_menu; char *menu; char *icon_theme_path; // non-standard KDE property + + sd_bus_slot *new_icon_slot; + sd_bus_slot *new_attention_icon_slot; + sd_bus_slot *new_status_slot; }; struct swaybar_sni *create_sni(char *id, struct swaybar_tray *tray); diff --git a/swaybar/tray/item.c b/swaybar/tray/item.c index 130496ecc..44fd876c8 100644 --- a/swaybar/tray/item.c +++ b/swaybar/tray/item.c @@ -78,6 +78,7 @@ static int read_pixmap(sd_bus_message *msg, struct swaybar_sni *sni, sd_bus_message_exit_container(msg); } + list_free_items_and_destroy(*dest); *dest = pixmaps; return ret; @@ -121,6 +122,10 @@ static int get_property_callback(sd_bus_message *msg, void *data, goto cleanup; } } else { + if (*type == 's' || *type == 'o') { + free(*(char **)dest); + } + ret = sd_bus_message_read(msg, type, dest); if (ret < 0) { wlr_log(WLR_DEBUG, "Failed to read property %s: %s", prop, @@ -156,55 +161,73 @@ static void sni_get_property_async(struct swaybar_sni *sni, const char *prop, } } +/* + * There is a quirk in sd-bus that in some systems, it is unable to get the + * well-known names on the bus, so it cannot identify if an incoming signal, + * which uses the sender's unique name, actually matches the callback's matching + * sender if the callback uses a well-known name, in which case it just calls + * the callback and hopes for the best, resulting in false positives. In the + * case of NewIcon & NewAttentionIcon, this doesn't affect anything, but it + * means that for NewStatus, if the SNI does not definitely match the sender, + * then the safe thing to do is to query the status independently. + * This function returns 1 if the SNI definitely matches the signal sender, + * which is returned by the calling function to indicate that signal matching + * can stop since it has already found the required callback, otherwise, it + * returns 0, which allows matching to continue. + */ +static int sni_check_msg_sender(struct swaybar_sni *sni, sd_bus_message *msg, + const char *signal) { + bool has_well_known_names = + sd_bus_creds_get_mask(sd_bus_message_get_creds(msg)) & SD_BUS_CREDS_WELL_KNOWN_NAMES; + if (sni->service[0] == ':' || has_well_known_names) { + wlr_log(WLR_DEBUG, "%s has new %s", sni->watcher_id, signal); + return 1; + } else { + wlr_log(WLR_DEBUG, "%s may have new %s", sni->watcher_id, signal); + return 0; + } +} + static int handle_new_icon(sd_bus_message *msg, void *data, sd_bus_error *error) { struct swaybar_sni *sni = data; - wlr_log(WLR_DEBUG, "%s has new IconName", sni->watcher_id); - - free(sni->icon_name); - sni->icon_name = NULL; sni_get_property_async(sni, "IconName", "s", &sni->icon_name); - - list_free_items_and_destroy(sni->icon_pixmap); - sni->icon_pixmap = NULL; sni_get_property_async(sni, "IconPixmap", NULL, &sni->icon_pixmap); - - return 0; + return sni_check_msg_sender(sni, msg, "icon"); } static int handle_new_attention_icon(sd_bus_message *msg, void *data, sd_bus_error *error) { struct swaybar_sni *sni = data; - wlr_log(WLR_DEBUG, "%s has new AttentionIconName", sni->watcher_id); - - free(sni->attention_icon_name); - sni->attention_icon_name = NULL; sni_get_property_async(sni, "AttentionIconName", "s", &sni->attention_icon_name); - - list_free_items_and_destroy(sni->attention_icon_pixmap); - sni->attention_icon_pixmap = NULL; sni_get_property_async(sni, "AttentionIconPixmap", NULL, &sni->attention_icon_pixmap); - - return 0; + return sni_check_msg_sender(sni, msg, "attention icon"); } static int handle_new_status(sd_bus_message *msg, void *data, sd_bus_error *error) { - char *status; - int ret = sd_bus_message_read(msg, "s", &status); - if (ret < 0) { - wlr_log(WLR_DEBUG, "Failed to read new status message: %s", strerror(-ret)); + struct swaybar_sni *sni = data; + int ret = sni_check_msg_sender(sni, msg, "status"); + if (ret == 1) { + char *status; + int r = sd_bus_message_read(msg, "s", &status); + if (r < 0) { + wlr_log(WLR_ERROR, "Failed to read new status message: %s", strerror(-ret)); + ret = r; + } else { + free(sni->status); + sni->status = strdup(status); + wlr_log(WLR_DEBUG, "%s has new status: %s", sni->watcher_id, status); + set_sni_dirty(sni); + } } else { - struct swaybar_sni *sni = data; - free(sni->status); - sni->status = strdup(status); - wlr_log(WLR_DEBUG, "%s has new Status '%s'", sni->watcher_id, status); - set_sni_dirty(sni); + sni_get_property_async(sni, "Status", "s", &sni->status); } + return ret; } -static void sni_match_signal(struct swaybar_sni *sni, char *signal, - sd_bus_message_handler_t callback) { - int ret = sd_bus_match_signal(sni->tray->bus, NULL, sni->service, sni->path, +static void sni_match_signal(struct swaybar_sni *sni, sd_bus_slot **slot, + char *signal, sd_bus_message_handler_t callback) { + int ret = sd_bus_match_signal(sni->tray->bus, slot, sni->service, sni->path, sni->interface, signal, callback, sni); if (ret < 0) { wlr_log(WLR_DEBUG, "Failed to subscribe to signal %s: %s", signal, @@ -241,9 +264,10 @@ struct swaybar_sni *create_sni(char *id, struct swaybar_tray *tray) { sni_get_property_async(sni, "ItemIsMenu", "b", &sni->item_is_menu); sni_get_property_async(sni, "Menu", "o", &sni->menu); - sni_match_signal(sni, "NewIcon", handle_new_icon); - sni_match_signal(sni, "NewAttentionIcon", handle_new_attention_icon); - sni_match_signal(sni, "NewStatus", handle_new_status); + sni_match_signal(sni, &sni->new_icon_slot, "NewIcon", handle_new_icon); + sni_match_signal(sni, &sni->new_attention_icon_slot, "NewAttentionIcon", + handle_new_attention_icon); + sni_match_signal(sni, &sni->new_status_slot, "NewStatus", handle_new_status); return sni; } @@ -253,6 +277,10 @@ void destroy_sni(struct swaybar_sni *sni) { return; } + sd_bus_slot_unref(sni->new_icon_slot); + sd_bus_slot_unref(sni->new_attention_icon_slot); + sd_bus_slot_unref(sni->new_status_slot); + free(sni->watcher_id); free(sni->service); free(sni->path); From f87628e2abd98348a218de581ca93271a99d1591 Mon Sep 17 00:00:00 2001 From: Ian Fan Date: Fri, 4 Jan 2019 11:57:31 +0000 Subject: [PATCH 3/3] swaybar: improve tray logging --- swaybar/main.c | 2 +- swaybar/tray/host.c | 4 ++-- swaybar/tray/item.c | 57 +++++++++++++++++++++++---------------------- 3 files changed, 32 insertions(+), 31 deletions(-) diff --git a/swaybar/main.c b/swaybar/main.c index 06238c629..fa99b1bad 100644 --- a/swaybar/main.c +++ b/swaybar/main.c @@ -76,7 +76,7 @@ int main(int argc, char **argv) { if (debug) { wlr_log_init(WLR_DEBUG, NULL); } else { - wlr_log_init(WLR_ERROR, NULL); + wlr_log_init(WLR_INFO, NULL); } if (!swaybar.id) { diff --git a/swaybar/tray/host.c b/swaybar/tray/host.c index 30339fece..cc8dd188d 100644 --- a/swaybar/tray/host.c +++ b/swaybar/tray/host.c @@ -21,7 +21,7 @@ static int cmp_sni_id(const void *item, const void *cmp_to) { static void add_sni(struct swaybar_tray *tray, char *id) { int idx = list_seq_find(tray->items, cmp_sni_id, id); if (idx == -1) { - wlr_log(WLR_DEBUG, "Registering Status Notifier Item '%s'", id); + wlr_log(WLR_INFO, "Registering Status Notifier Item '%s'", id); struct swaybar_sni *sni = create_sni(id, tray); if (sni) { list_add(tray->items, sni); @@ -54,7 +54,7 @@ static int handle_sni_unregistered(sd_bus_message *msg, void *data, struct swaybar_tray *tray = data; int idx = list_seq_find(tray->items, cmp_sni_id, id); if (idx != -1) { - wlr_log(WLR_DEBUG, "Unregistering Status Notifier Item '%s'", id); + wlr_log(WLR_INFO, "Unregistering Status Notifier Item '%s'", id); destroy_sni(tray->items->items[idx]); list_del(tray->items, idx); set_bar_dirty(tray->bar); diff --git a/swaybar/tray/item.c b/swaybar/tray/item.c index 44fd876c8..0833dcb9a 100644 --- a/swaybar/tray/item.c +++ b/swaybar/tray/item.c @@ -35,11 +35,12 @@ static int read_pixmap(sd_bus_message *msg, struct swaybar_sni *sni, const char *prop, list_t **dest) { int ret = sd_bus_message_enter_container(msg, 'a', "(iiay)"); if (ret < 0) { - wlr_log(WLR_DEBUG, "Failed to read property %s: %s", prop, strerror(-ret)); + wlr_log(WLR_ERROR, "%s %s: %s", sni->watcher_id, prop, strerror(-ret)); return ret; } if (sd_bus_message_at_end(msg, 0)) { + wlr_log(WLR_DEBUG, "%s %s no. of icons = 0", sni->watcher_id, prop); return ret; } @@ -51,14 +52,14 @@ static int read_pixmap(sd_bus_message *msg, struct swaybar_sni *sni, while (!sd_bus_message_at_end(msg, 0)) { ret = sd_bus_message_enter_container(msg, 'r', "iiay"); if (ret < 0) { - wlr_log(WLR_DEBUG, "Failed to read property %s: %s", prop, strerror(-ret)); + wlr_log(WLR_ERROR, "%s %s: %s", sni->watcher_id, prop, strerror(-ret)); goto error; } int size; ret = sd_bus_message_read(msg, "ii", NULL, &size); if (ret < 0) { - wlr_log(WLR_DEBUG, "Failed to read property %s: %s", prop, strerror(-ret)); + wlr_log(WLR_ERROR, "%s %s: %s", sni->watcher_id, prop, strerror(-ret)); goto error; } @@ -66,7 +67,7 @@ static int read_pixmap(sd_bus_message *msg, struct swaybar_sni *sni, size_t npixels; ret = sd_bus_message_read_array(msg, 'y', &pixels, &npixels); if (ret < 0) { - wlr_log(WLR_DEBUG, "Failed to read property %s: %s", prop, strerror(-ret)); + wlr_log(WLR_ERROR, "%s %s: %s", sni->watcher_id, prop, strerror(-ret)); goto error; } @@ -80,6 +81,8 @@ static int read_pixmap(sd_bus_message *msg, struct swaybar_sni *sni, } list_free_items_and_destroy(*dest); *dest = pixmaps; + wlr_log(WLR_DEBUG, "%s %s no. of icons = %d", sni->watcher_id, prop, + pixmaps->length); return ret; error: @@ -104,15 +107,15 @@ static int get_property_callback(sd_bus_message *msg, void *data, int ret; if (sd_bus_message_is_method_error(msg, NULL)) { - sd_bus_error err = *sd_bus_message_get_error(msg); - wlr_log(WLR_DEBUG, "Failed to get property %s: %s", prop, err.message); - ret = -sd_bus_error_get_errno(&err); + wlr_log(WLR_ERROR, "%s %s: %s", sni->watcher_id, prop, + sd_bus_message_get_error(msg)->message); + ret = sd_bus_message_get_errno(msg); goto cleanup; } ret = sd_bus_message_enter_container(msg, 'v', type); if (ret < 0) { - wlr_log(WLR_DEBUG, "Failed to read property %s: %s", prop, strerror(-ret)); + wlr_log(WLR_ERROR, "%s %s: %s", sni->watcher_id, prop, strerror(-ret)); goto cleanup; } @@ -128,12 +131,17 @@ static int get_property_callback(sd_bus_message *msg, void *data, ret = sd_bus_message_read(msg, type, dest); if (ret < 0) { - wlr_log(WLR_DEBUG, "Failed to read property %s: %s", prop, - strerror(-ret)); + wlr_log(WLR_ERROR, "%s %s: %s", sni->watcher_id, prop, strerror(-ret)); goto cleanup; - } else if (*type == 's' || *type == 'o') { + } + + if (*type == 's' || *type == 'o') { char **str = dest; *str = strdup(*str); + wlr_log(WLR_DEBUG, "%s %s = '%s'", sni->watcher_id, prop, *str); + } else if (*type == 'b') { + wlr_log(WLR_DEBUG, "%s %s = %s", sni->watcher_id, prop, + *(bool *)dest ? "true" : "false"); } } @@ -157,7 +165,7 @@ static void sni_get_property_async(struct swaybar_sni *sni, const char *prop, sni->path, "org.freedesktop.DBus.Properties", "Get", get_property_callback, data, "ss", sni->interface, prop); if (ret < 0) { - wlr_log(WLR_DEBUG, "Failed to get property %s: %s", prop, strerror(-ret)); + wlr_log(WLR_ERROR, "%s %s: %s", sni->watcher_id, prop, strerror(-ret)); } } @@ -210,12 +218,12 @@ static int handle_new_status(sd_bus_message *msg, void *data, sd_bus_error *erro char *status; int r = sd_bus_message_read(msg, "s", &status); if (r < 0) { - wlr_log(WLR_ERROR, "Failed to read new status message: %s", strerror(-ret)); + wlr_log(WLR_ERROR, "%s new status error: %s", sni->watcher_id, strerror(-ret)); ret = r; } else { free(sni->status); sni->status = strdup(status); - wlr_log(WLR_DEBUG, "%s has new status: %s", sni->watcher_id, status); + wlr_log(WLR_DEBUG, "%s has new status = '%s'", sni->watcher_id, status); set_sni_dirty(sni); } } else { @@ -230,7 +238,7 @@ static void sni_match_signal(struct swaybar_sni *sni, sd_bus_slot **slot, int ret = sd_bus_match_signal(sni->tray->bus, slot, sni->service, sni->path, sni->interface, signal, callback, sni); if (ret < 0) { - wlr_log(WLR_DEBUG, "Failed to subscribe to signal %s: %s", signal, + wlr_log(WLR_ERROR, "Failed to subscribe to signal %s: %s", signal, strerror(-ret)); } } @@ -322,18 +330,11 @@ static void handle_click(struct swaybar_sni *sni, int x, int y, char *orientation = (dir = 'U' || dir == 'D') ? "vertical" : "horizontal"; int sign = (dir == 'U' || dir == 'L') ? -1 : 1; - int ret = sd_bus_call_method_async(sni->tray->bus, NULL, sni->service, - sni->path, sni->interface, "Scroll", NULL, NULL, "is", - delta*sign, orientation); - if (ret < 0) { - wlr_log(WLR_DEBUG, "Failed to scroll on SNI: %s", strerror(-ret)); - } + sd_bus_call_method_async(sni->tray->bus, NULL, sni->service, sni->path, + sni->interface, "Scroll", NULL, NULL, "is", delta*sign, orientation); } else { - int ret = sd_bus_call_method_async(sni->tray->bus, NULL, sni->service, - sni->path, sni->interface, method, NULL, NULL, "ii", x, y); - if (ret < 0) { - wlr_log(WLR_DEBUG, "Failed to click on SNI: %s", strerror(-ret)); - } + sd_bus_call_method_async(sni->tray->bus, NULL, sni->service, sni->path, + sni->interface, method, NULL, NULL, "ii", x, y); } } @@ -345,7 +346,7 @@ static int cmp_sni_id(const void *item, const void *cmp_to) { static enum hotspot_event_handling icon_hotspot_callback( struct swaybar_output *output, struct swaybar_hotspot *hotspot, int x, int y, enum x11_button button, void *data) { - wlr_log(WLR_DEBUG, "Clicked on Status Notifier Item '%s'", (char *)data); + wlr_log(WLR_DEBUG, "Clicked on %s", (char *)data); struct swaybar_tray *tray = output->bar->tray; int idx = list_seq_find(tray->items, cmp_sni_id, data); @@ -359,7 +360,7 @@ static enum hotspot_event_handling icon_hotspot_callback( int global_y = output->output_y + (top_bar ? config->gaps.top + y: (int) output->output_height - config->gaps.bottom - y); - wlr_log(WLR_DEBUG, "Guessing click at (%d, %d)", global_x, global_y); + wlr_log(WLR_DEBUG, "Guessing click position at (%d, %d)", global_x, global_y); handle_click(sni, global_x, global_y, button, 1); // TODO get delta from event return HOTSPOT_IGNORE; } else {