Compare raw icons by their checksums

Currently, we just skipped the notification comparison, if the
notification had a raw icon attached. This is a bit counterintuitive.

Calculating a checksum of the raw icon's data is the solution.

For that we cache the pixel buffer and introduce a field, which saves
the current icon's id. The icon_id may be a path or a hash.
So you can compare two notifications by their icon_id field regardless
of their icon type by their icon_id field.

src/dbus.c

@@ -86,8 +86,6 @@ struct dbus_method {
                         GVariant *parameters, \
                         GDBusMethodInvocation *invocation)
 
-static struct raw_image *get_raw_image_from_data_hint(GVariant *icon_data);
-
 int cmp_methods(const void *vkey, const void *velem)
 {
         const char *key = (const char*)vkey;
@@ -241,7 +239,7 @@ static struct notification *dbus_message_to_notification(const gchar *sender, GV
         if (!dict_value)
                 dict_value = g_variant_lookup_value(hints, "icon_data", G_VARIANT_TYPE("(iiibiiay)"));
         if (dict_value) {
-                n->raw_icon = get_raw_image_from_data_hint(dict_value);
+                notification_icon_replace_data(n, dict_value);
                 g_variant_unref(dict_value);
         }
 
@@ -577,42 +575,6 @@ static void dbus_cb_name_lost(GDBusConnection *connection,
         exit(1);
 }
 
-static struct raw_image *get_raw_image_from_data_hint(GVariant *icon_data)
-{
-        struct raw_image *image = g_malloc(sizeof(struct raw_image));
-        GVariant *data_variant;
-        gsize expected_len;
-
-        g_variant_get(icon_data,
-                      "(iiibii@ay)",
-                      &image->width,
-                      &image->height,
-                      &image->rowstride,
-                      &image->has_alpha,
-                      &image->bits_per_sample,
-                      &image->n_channels,
-                      &data_variant);
-
-        expected_len = (image->height - 1) * image->rowstride + image->width
-                * ((image->n_channels * image->bits_per_sample + 7) / 8);
-
-        if (expected_len != g_variant_get_size (data_variant)) {
-                LOG_W("Expected image data to be of length %" G_GSIZE_FORMAT
-                      " but got a length of %" G_GSIZE_FORMAT,
-                      expected_len,
-                      g_variant_get_size(data_variant));
-                g_free(image);
-                g_variant_unref(data_variant);
-                return NULL;
-        }
-
-        image->data = (guchar *) g_memdup(g_variant_get_data(data_variant),
-                                          g_variant_get_size(data_variant));
-        g_variant_unref(data_variant);
-
-        return image;
-}
-
 int dbus_init(void)
 {
         guint owner_id;

src/icon.c

@@ -205,19 +205,120 @@ GdkPixbuf *get_pixbuf_from_icon(const char *iconname)
         return pixbuf;
 }
 
-GdkPixbuf *get_pixbuf_from_raw_image(const struct raw_image *raw_image)
+GdkPixbuf *icon_get_for_name(const char *name, char **id)
 {
-        GdkPixbuf *pixbuf = NULL;
+        ASSERT_OR_RET(name, NULL);
+        ASSERT_OR_RET(id, NULL);
 
-        pixbuf = gdk_pixbuf_new_from_data(raw_image->data,
+        GdkPixbuf *pb = get_pixbuf_from_icon(name);
+        if (pb)
+                *id = g_strdup(name);
+        return pb;
+}
+
+GdkPixbuf *icon_get_for_data(GVariant *data, char **id)
+{
+        ASSERT_OR_RET(data, NULL);
+        ASSERT_OR_RET(id, NULL);
+
+        if (!STR_EQ("(iiibiiay)", g_variant_get_type_string(data))) {
+                LOG_W("Invalid data for pixbuf given.");
+                return NULL;
+        }
+
+        /* The raw image is a big array of char data.
+         *
+         * The image is serialised rowwise pixel by pixel. The rows are aligned
+         * by a spacer full of garbage. The overall data length of data + garbage
+         * is called the rowstride.
+         *
+         * Mind the missing spacer at the last row.
+         *
+         * len:     |<--------------rowstride---------------->|
+         * len:     |<-width*pixelstride->|
+         * row 1:   |   data for row 1    | spacer of garbage |
+         * row 2:   |   data for row 2    | spacer of garbage |
+         *          |         .           | spacer of garbage |
+         *          |         .           | spacer of garbage |
+         *          |         .           | spacer of garbage |
+         * row n-1: |   data for row n-1  | spacer of garbage |
+         * row n:   |   data for row n    |
+         */
+
+        GdkPixbuf *pixbuf = NULL;
+        GVariant *data_variant = NULL;
+        unsigned char *data_pb;
+
+        gsize len_expected;
+        gsize len_actual;
+        gsize pixelstride;
+
+        int width;
+        int height;
+        int rowstride;
+        int has_alpha;
+        int bits_per_sample;
+        int n_channels;
+
+        g_variant_get(data,
+                      "(iiibii@ay)",
+                      &width,
+                      &height,
+                      &rowstride,
+                      &has_alpha,
+                      &bits_per_sample,
+                      &n_channels,
+                      &data_variant);
+
+        // note: (A+7)/8 rounds up A to the next byte boundary
+        pixelstride = (n_channels * bits_per_sample + 7)/8;
+        len_expected = (height - 1) * rowstride + width * pixelstride;
+        len_actual = g_variant_get_size(data_variant);
+
+        if (len_actual != len_expected) {
+                LOG_W("Expected image data to be of length %" G_GSIZE_FORMAT
+                      " but got a length of %" G_GSIZE_FORMAT,
+                      len_expected,
+                      len_actual);
+                g_variant_unref(data_variant);
+                return NULL;
+        }
+
+        data_pb = (guchar *) g_memdup(g_variant_get_data(data_variant), len_actual);
+
+        pixbuf = gdk_pixbuf_new_from_data(data_pb,
                                           GDK_COLORSPACE_RGB,
-                                          raw_image->has_alpha,
+                                          has_alpha,
-                                          raw_image->bits_per_sample,
+                                          bits_per_sample,
-                                          raw_image->width,
+                                          width,
-                                          raw_image->height,
+                                          height,
-                                          raw_image->rowstride,
+                                          rowstride,
-                                          NULL,
+                                          (GdkPixbufDestroyNotify) g_free,
-                                          NULL);
+                                          data_pb);
+        if (!pixbuf) {
+                /* Dear user, I'm sorry, I'd like to give you a more specific
+                 * error message. But sadly, I can't */
+                LOG_W("Cannot serialise raw icon data into pixbuf.");
+                return NULL;
+        }
+
+        /* To calculate a checksum of the current image, we have to remove
+         * all excess spacers, so that our checksummed memory only contains
+         * real data. */
+        size_t data_chk_len = pixelstride * width * height;
+        unsigned char *data_chk = g_malloc(data_chk_len);
+        size_t rowstride_short = pixelstride * width;
+
+        for (int i = 0; i < height; i++) {
+                memcpy(data_chk + (i*rowstride_short),
+                       data_pb  + (i*rowstride),
+                       rowstride_short);
+        }
+
+        *id = g_compute_checksum_for_data(G_CHECKSUM_MD5, data_chk, data_chk_len);
+
+        g_free(data_chk);
+        g_variant_unref(data_variant);
 
         return pixbuf;
 }

src/icon.h

@@ -37,9 +37,34 @@ GdkPixbuf *get_pixbuf_from_file(const char *filename);
  */
 GdkPixbuf *get_pixbuf_from_icon(const char *iconname);
 
-/** Convert a struct raw_image to a `GdkPixbuf`
+/** Read an icon from disk and convert it to a GdkPixbuf.
+ *
+ * The returned id will be a unique identifier. To check if two given
+ * GdkPixbufs are equal, it's sufficient to just compare the id strings.
+ *
+ * @param name A string describing and icon. May be a full path, a file path or
+ *             just a simple name. If it's a name without a slash, the icon will
+ *             get searched in the folders of the icon_path setting.
+ * @param id   (necessary) A unique identifier of the returned pixbuf. Only filled,
+ *             if the return value is non-NULL.
+ * @return     a pixbuf representing name's image.
+ *             If an invalid path given, it will return NULL.
  */
-GdkPixbuf *get_pixbuf_from_raw_image(const struct raw_image *raw_image);
+GdkPixbuf *icon_get_for_name(const char *name, char **id);
+
+/** Convert a GVariant like described in GdkPixbuf
+ *
+ * The returned id will be a unique identifier. To check if two given
+ * GdkPixbufs are equal, it's sufficient to just compare the id strings.
+ *
+ * @param data A GVariant in the format "(iiibii@ay)" filled with values
+ *             like described in the notification spec.
+ * @param id   (necessary) A unique identifier of the returned pixbuf.
+ *             Only filled, if the return value is non-NULL.
+ * @return     a pixbuf representing name's image.
+ *             If an invalid GVariant is passed, it will return NULL.
+ */
+GdkPixbuf *icon_get_for_data(GVariant *data, char **id);
 
 #endif
 /* vim: set tabstop=8 shiftwidth=8 expandtab textwidth=0: */

src/notification.c

@@ -24,7 +24,6 @@
 #include "settings.h"
 #include "utils.h"
 
-static void notification_update_icon(struct notification *n);
 static void notification_extract_urls(struct notification *n);
 static void notification_format_message(struct notification *n);
 
@@ -55,7 +54,8 @@ void notification_print(const struct notification *n)
         printf("\tsummary: '%s'\n", n->summary);
         printf("\tbody: '%s'\n", n->body);
         printf("\ticon: '%s'\n", n->iconname);
-        printf("\traw_icon set: %s\n", (n->raw_icon ? "true" : "false"));
+        printf("\traw_icon set: %s\n", (n->icon_id && !STR_EQ(n->iconname, n->icon_id)) ? "true" : "false");
+        printf("\ticon_id: '%s'\n", n->icon_id);
         printf("\tcategory: %s\n", n->category);
         printf("\ttimeout: %ld\n", n->timeout/1000);
         printf("\turgency: %s\n", notification_urgency_to_string(n->urgency));
@@ -175,29 +175,15 @@ int notification_cmp_data(const void *va, const void *vb, void *data)
         return notification_cmp(a, b);
 }
 
-int notification_is_duplicate(const struct notification *a, const struct notification *b)
+bool notification_is_duplicate(const struct notification *a, const struct notification *b)
 {
-        //Comparing raw icons is not supported, assume they are not identical
-        if (settings.icon_position != ICON_OFF
-                && (a->raw_icon || b->raw_icon))
-                return false;
-
         return STR_EQ(a->appname, b->appname)
             && STR_EQ(a->summary, b->summary)
             && STR_EQ(a->body, b->body)
-            && (settings.icon_position != ICON_OFF ? STR_EQ(a->iconname, b->iconname) : 1)
+            && (settings.icon_position != ICON_OFF ? STR_EQ(a->icon_id, b->icon_id) : 1)
             && a->urgency == b->urgency;
 }
 
-/* see notification.h */
-void rawimage_free(struct raw_image *i)
-{
-        ASSERT_OR_RET(i,);
-
-        g_free(i->data);
-        g_free(i);
-}
-
 static void notification_private_free(NotificationPrivate *p)
 {
         g_free(p);
@@ -241,13 +227,44 @@ void notification_unref(struct notification *n)
         g_free(n->stack_tag);
 
         g_hash_table_unref(n->actions);
-        rawimage_free(n->raw_icon);
+
+        if (n->icon)
+                g_object_unref(n->icon);
+        g_free(n->icon_id);
 
         notification_private_free(n->priv);
 
         g_free(n);
 }
 
+void notification_icon_replace_path(struct notification *n, const char *new_icon)
+{
+        ASSERT_OR_RET(n,);
+        ASSERT_OR_RET(new_icon,);
+        ASSERT_OR_RET(n->iconname != new_icon,);
+
+        g_free(n->iconname);
+        n->iconname = g_strdup(new_icon);
+
+        g_clear_object(&n->icon);
+        g_clear_pointer(&n->icon_id, g_free);
+
+        n->icon = icon_get_for_name(new_icon, &n->icon_id);
+        n->icon = icon_pixbuf_scale(n->icon);
+}
+
+void notification_icon_replace_data(struct notification *n, GVariant *new_icon)
+{
+        ASSERT_OR_RET(n,);
+        ASSERT_OR_RET(new_icon,);
+
+        g_clear_object(&n->icon);
+        g_clear_pointer(&n->icon_id, g_free);
+
+        n->icon = icon_get_for_data(new_icon, &n->icon_id);
+        n->icon = icon_pixbuf_scale(n->icon);
+}
+
 /* see notification.h */
 void notification_replace_single_field(char **haystack,
                                        char **needle,
@@ -332,8 +349,13 @@ void notification_init(struct notification *n)
         /* Icon handling */
         if (STR_EMPTY(n->iconname))
                 g_clear_pointer(&n->iconname, g_free);
-        if (!n->raw_icon && !n->iconname)
+        if (!n->icon && n->iconname) {
-                n->iconname = g_strdup(settings.icons[n->urgency]);
+                char *icon = g_strdup(n->iconname);
+                notification_icon_replace_path(n, icon);
+                g_free(icon);
+        }
+        if (!n->icon && !n->iconname)
+                notification_icon_replace_path(n, settings.icons[n->urgency]);
 
         /* Color hints */
         struct notification_colors defcolors;
@@ -365,25 +387,10 @@ void notification_init(struct notification *n)
         rule_apply_all(n);
 
         /* UPDATE derived fields */
-        notification_update_icon(n);
         notification_extract_urls(n);
         notification_format_message(n);
 }
 
-static void notification_update_icon(struct notification *n)
-{
-        g_return_if_fail(n);
-
-        g_clear_object(&n->icon);
-
-        if (n->raw_icon)
-                n->icon = get_pixbuf_from_raw_image(n->raw_icon);
-        else if (n->iconname)
-                n->icon = get_pixbuf_from_icon(n->iconname);
-
-        n->icon = icon_pixbuf_scale(n->icon);
-}
-
 static void notification_format_message(struct notification *n)
 {
         g_clear_pointer(&n->msg, g_free);

src/notification.h

@@ -27,16 +27,6 @@ enum urgency {
         URG_MAX = 2,   /**< Maximum value, useful for boundary checking */
 };
 
-struct raw_image {
-        int width;
-        int height;
-        int rowstride;
-        int has_alpha;
-        int bits_per_sample;
-        int n_channels;
-        unsigned char *data;
-};
-
 typedef struct _notification_private NotificationPrivate;
 
 struct notification_colors {
@@ -57,9 +47,11 @@ struct notification {
         char *category;
         enum urgency urgency;
 
-        GdkPixbuf *icon;
+        GdkPixbuf *icon;         /**< The raw cached icon data used to draw */
-        char *iconname;          /**< plain icon information (may be a path or just a name) */
+        char *icon_id;           /**< plain icon information, which acts as the pixbuf's id, which is saved in .icon
-        struct raw_image *raw_icon;  /**< passed icon data of notification, takes precedence over icon */
+                                      May be a hash for a raw icon or a name/path for a regular app icon. */
+        char *iconname;          /**< plain icon information (may be a path or just a name)
+                                      Use this to compare the icon name with rules.*/
 
         gint64 start;      /**< begin of current display */
         gint64 timestamp;  /**< arrival time */
@@ -123,13 +115,6 @@ void notification_ref(struct notification *n);
  */
 void notification_init(struct notification *n);
 
-/**
- * Free a #raw_image
- *
- * @param i (nullable): pointer to #raw_image
- */
-void rawimage_free(struct raw_image *i);
-
 /**
  * Decrease the reference counter of the notification.
  *
@@ -148,7 +133,26 @@ int notification_cmp(const struct notification *a, const struct notification *b)
  */
 int notification_cmp_data(const void *va, const void *vb, void *data);
 
-int notification_is_duplicate(const struct notification *a, const struct notification *b);
+bool notification_is_duplicate(const struct notification *a, const struct notification *b);
+
+/**Replace the current notification's icon with the icon specified by path.
+ *
+ * Removes the reference for the previous icon automatically and will also free the
+ * iconname field. So passing n->iconname as new_icon is invalid.
+ *
+ * @param n the notification to replace the icon
+ * @param new_icon The path of the new icon. May be an absolute path or an icon name.
+ */
+void notification_icon_replace_path(struct notification *n, const char *new_icon);
+
+/**Replace the current notification's icon with the raw icon given in the GVariant.
+ *
+ * Removes the reference for the previous icon automatically.
+ *
+ * @param n the notification to replace the icon
+ * @param new_icon The icon's data. Has to be in the format of the notification spec.
+ */
+void notification_icon_replace_data(struct notification *n, GVariant *new_icon);
 
 /**
  * Run the script associated with the

src/rules.c

@@ -26,11 +26,8 @@ void rule_apply(struct rule *r, struct notification *n)
                 n->transient = r->set_transient;
         if (r->markup != MARKUP_NULL)
                 n->markup = r->markup;
-        if (r->new_icon) {
+        if (r->new_icon)
-                g_free(n->iconname);
+                notification_icon_replace_path(n, r->new_icon);
-                n->iconname = g_strdup(r->new_icon);
-                g_clear_pointer(&n->raw_icon, rawimage_free);
-        }
         if (r->fg) {
                 g_free(n->colors.fg);
                 n->colors.fg = g_strdup(r->fg);

test/dbus.c

@@ -618,7 +618,8 @@ TEST test_hint_raw_image(void)
         ASSERT_EQ(queues_length_waiting(), len+1);
         n = queues_debug_find_notification_by_id(id);
 
-        ASSERT(n->raw_icon);
+        ASSERT(n->icon);
+        ASSERT(!STR_EQ(n->icon_id, n_dbus->app_icon));
 
         dbus_notification_free(n_dbus);
         g_free(path);

test/notification.c

@@ -26,6 +26,7 @@ TEST test_notification_is_duplicate(void)
         a->summary = g_strdup("Summary");
         a->body = g_strdup("Body");
         a->iconname = g_strdup("Icon");
+        a->icon_id = g_strdup("Icon");
         a->urgency = URG_NORM;
 
         struct notification *b = notification_create();
@@ -33,8 +34,11 @@ TEST test_notification_is_duplicate(void)
         b->summary = g_strdup("Summary");
         b->body = g_strdup("Body");
         b->iconname = g_strdup("Icon");
+        b->icon_id = g_strdup("Icon");
         b->urgency = URG_NORM;
 
+        ASSERT(notification_is_duplicate(a, b));
+
         CHECK_CALL(test_notification_is_duplicate_field(&(b->appname), a, b));
         CHECK_CALL(test_notification_is_duplicate_field(&(b->summary), a, b));
         CHECK_CALL(test_notification_is_duplicate_field(&(b->body), a, b));
@@ -47,21 +51,17 @@ TEST test_notification_is_duplicate(void)
         settings.icon_position = ICON_OFF;
         ASSERT(notification_is_duplicate(a, b));
         //Setting pointer to a random value since we are checking for null
-        b->raw_icon = (struct raw_image*)0xff;
+        char *icon_id = b->icon_id;
-        ASSERT(notification_is_duplicate(a, b));
+        b->icon_id = "false";
-        b->raw_icon = NULL;
+        ASSERTm("Icons have to get ignored for duplicate check when icons are off",
+                notification_is_duplicate(a, b));
+        b->icon_id = icon_id;
 
         settings.icon_position = ICON_LEFT;
-        CHECK_CALL(test_notification_is_duplicate_field(&(b->iconname), a, b));
+        CHECK_CALL(test_notification_is_duplicate_field(&(b->icon_id), a, b));
-        b->raw_icon = (struct raw_image*)0xff;
-        ASSERT_FALSE(notification_is_duplicate(a, b));
-        b->raw_icon = NULL;
 
         settings.icon_position = ICON_RIGHT;
-        CHECK_CALL(test_notification_is_duplicate_field(&(b->iconname), a, b));
+        CHECK_CALL(test_notification_is_duplicate_field(&(b->icon_id), a, b));
-        b->raw_icon = (struct raw_image*)0xff;
-        ASSERT_FALSE(notification_is_duplicate(a, b));
-        b->raw_icon = NULL;
 
         settings.icon_position = icon_setting_tmp;