Merge pull request #674 from xkr47/patch-min_icon_size-20191122

Implement #392, take 2

.valgrind.suppressions

@@ -20,10 +20,30 @@
    fun:g_error_new_valist
    fun:g_set_error
    obj:*/librsvg-2.so*
+   fun:rsvg_handle_close
+   obj:*/loaders/libpixbufloader-svg.so
+   fun:gdk_pixbuf_loader_close
+   fun:gdk_pixbuf_get_file_info
+   fun:get_pixbuf_from_file
+   ...
+}
+
+# same as above, but as occurs in CI environment
+{
+   rsvg_error_handle_close2
+   Memcheck:Leak
+   match-leak-kinds: definite
+   fun:malloc
+   fun:g_malloc
+   fun:g_slice_alloc
+   fun:g_error_new_valist
+   fun:g_set_error
+   obj:*/librsvg-2.so*
    obj:*/librsvg-2.so*
    obj:*/loaders/libpixbufloader-svg.so
    obj:*/libgdk_pixbuf-2.0.so*
-   fun:gdk_pixbuf_new_from_file
+   fun:gdk_pixbuf_loader_close
+   fun:gdk_pixbuf_get_file_info
    fun:get_pixbuf_from_file
    ...
 }
@@ -45,7 +65,8 @@
    fun:rsvg_handle_write
    obj:*/loaders/libpixbufloader-svg.so
    obj:*/libgdk_pixbuf-2.0.so*
-   fun:gdk_pixbuf_new_from_file
+   fun:gdk_pixbuf_loader_close
+   fun:gdk_pixbuf_get_file_info
    fun:get_pixbuf_from_file
    ...
 }

config.h

@@ -65,6 +65,7 @@ struct settings defaults = {
 
 .browser = "/usr/bin/firefox",
 
+.min_icon_size = 0,
 .max_icon_size = 0,
 
 /* paths to default icons */

docs/dunst.pod

@@ -384,14 +384,28 @@ ACTIONS below for further details.
 Defines the position of the icon in the notification window. Setting it to off
 disables icons.
 
+=item B<min_icon_size> (default: 0)
+
+Defines the minimum size in pixels for the icons.
+If the icon is larger than or equal to the specified value it won't be affected.
+If it's smaller then it will be scaled up so that the smaller axis is equivalent
+to the specified size.
+
+Set to 0 to disable icon upscaling. (default)
+
+If B<icon_position> is set to off, this setting is ignored.
+
 =item B<max_icon_size> (default: 0)
 
 Defines the maximum size in pixels for the icons.
-If the icon is smaller than the specified value it won't be affected.
+If the icon is smaller than or equal to the specified value it won't be affected.
 If it's larger then it will be scaled down so that the larger axis is equivalent
 to the specified size.
 
-Set to 0 to disable icon scaling. (default)
+Set to 0 to disable icon downscaling. (default)
+
+If both B<min_icon_size> and B<max_icon_size> are enabled, the latter
+gets the last say.
 
 If B<icon_position> is set to off, this setting is ignored.
 

dunstrc

@@ -162,6 +162,11 @@
     # Align icons left/right/off
     icon_position = off
 
+    # Scale small icons up to this size, set to 0 to disable. Helpful
+    # for e.g. small files or high-dpi screens. In case of conflict,
+    # max_icon_size takes precedence over this.
+    min_icon_size = 0
+
     # Scale larger icons down to this size, set to 0 to disable
     max_icon_size = 32
 

src/icon.c

@@ -110,25 +110,56 @@ cairo_surface_t *gdk_pixbuf_to_cairo_surface(GdkPixbuf *pixbuf)
         return icon_surface;
 }
 
-GdkPixbuf *icon_pixbuf_scale(GdkPixbuf *pixbuf)
+/**
+ * Scales the given image dimensions if necessary according to the settings.
+ *
+ * @param w a pointer to the image width, to be modified in-place
+ * @param h a pointer to the image height, to be modified in-place
+ * @return TRUE if the dimensions were updated, FALSE if they were left unchanged
+ */
+static bool icon_size_clamp(int *w, int *h) {
+        int _w = *w, _h = *h;
+        int landscape = _w > _h;
+        int orig_larger = landscape ? _w : _h;
+        double larger = orig_larger;
+        double smaller = landscape ? _h : _w;
+        if (settings.min_icon_size && smaller < settings.min_icon_size) {
+                larger = larger / smaller * settings.min_icon_size;
+                smaller = settings.min_icon_size;
+        }
+        if (settings.max_icon_size && larger > settings.max_icon_size) {
+                smaller = smaller / larger * settings.max_icon_size;
+                larger = settings.max_icon_size;
+        }
+        if ((int) larger != orig_larger) {
+                *w = (int) (landscape ? larger : smaller);
+                *h = (int) (landscape ? smaller : larger);
+                return TRUE;
+        }
+        return FALSE;
+}
+
+/**
+ * Scales the given GdkPixbuf if necessary according to the settings.
+ *
+ * @param pixbuf (nullable) The pixbuf, which may be too big.
+ *                          Takes ownership of the reference.
+ * @return the scaled version of the pixbuf. If scaling wasn't
+ *         necessary, it returns the same pixbuf. Transfers full
+ *         ownership of the reference.
+ */
+static GdkPixbuf *icon_pixbuf_scale(GdkPixbuf *pixbuf)
 {
         ASSERT_OR_RET(pixbuf, NULL);
 
         int w = gdk_pixbuf_get_width(pixbuf);
         int h = gdk_pixbuf_get_height(pixbuf);
-        int larger = w > h ? w : h;
-        if (settings.max_icon_size && larger > settings.max_icon_size) {
-                int scaled_w = settings.max_icon_size;
-                int scaled_h = settings.max_icon_size;
-                if (w >= h)
-                        scaled_h = (settings.max_icon_size * h) / w;
-                else
-                        scaled_w = (settings.max_icon_size * w) / h;
 
+        if (icon_size_clamp(&w, &h)) {
                 GdkPixbuf *scaled = gdk_pixbuf_scale_simple(
                                 pixbuf,
-                                scaled_w,
+                                w,
-                                scaled_h,
+                                h,
                                 GDK_INTERP_BILINEAR);
                 g_object_unref(pixbuf);
                 pixbuf = scaled;
@@ -141,8 +172,19 @@ GdkPixbuf *get_pixbuf_from_file(const char *filename)
 {
         char *path = string_to_path(g_strdup(filename));
         GError *error = NULL;
+        gint w, h;
 
-        GdkPixbuf *pixbuf = gdk_pixbuf_new_from_file(path, &error);
+        if (!gdk_pixbuf_get_file_info (path, &w, &h)) {
+                LOG_W("Failed to load image info for %s", filename);
+                g_free(path);
+                return NULL;
+        }
+        icon_size_clamp(&w, &h);
+        GdkPixbuf *pixbuf = gdk_pixbuf_new_from_file_at_scale(path,
+                                                              w,
+                                                              h,
+                                                              TRUE,
+                                                              &error);
 
         if (error) {
                 LOG_W("%s", error->message);
@@ -320,6 +362,8 @@ GdkPixbuf *icon_get_for_data(GVariant *data, char **id)
         g_free(data_chk);
         g_variant_unref(data_variant);
 
+        pixbuf = icon_pixbuf_scale(pixbuf);
+
         return pixbuf;
 }
 

src/icon.h

@@ -8,18 +8,7 @@
 
 cairo_surface_t *gdk_pixbuf_to_cairo_surface(GdkPixbuf *pixbuf);
 
-/**
+/** Retrieve an icon by its full filepath, scaled according to settings.
- * Scales the given GdkPixbuf if necessary according to the settings.
- *
- * @param pixbuf (nullable) The pixbuf, which may be too big.
- *                          Takes ownership of the reference.
- * @return the scaled version of the pixbuf. If scaling wasn't
- *         necessary, it returns the same pixbuf. Transfers full
- *         ownership of the reference.
- */
-GdkPixbuf *icon_pixbuf_scale(GdkPixbuf *pixbuf);
-
-/** Retrieve an icon by its full filepath.
  *
  * @param filename A string representing a readable file path
  *
@@ -28,7 +17,7 @@ GdkPixbuf *icon_pixbuf_scale(GdkPixbuf *pixbuf);
  */
 GdkPixbuf *get_pixbuf_from_file(const char *filename);
 
-/** Retrieve an icon by its name sent via the notification bus
+/** Retrieve an icon by its name sent via the notification bus, scaled according to settings
  *
  * @param iconname A string describing a `file://` URL, an arbitary filename
  *                 or an icon name, which then gets searched for in the
@@ -39,7 +28,7 @@ GdkPixbuf *get_pixbuf_from_file(const char *filename);
  */
 GdkPixbuf *get_pixbuf_from_icon(const char *iconname);
 
-/** Read an icon from disk and convert it to a GdkPixbuf.
+/** Read an icon from disk and convert it to a GdkPixbuf, scaled according to settings
  *
  * 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.
@@ -54,7 +43,7 @@ GdkPixbuf *get_pixbuf_from_icon(const char *iconname);
  */
 GdkPixbuf *icon_get_for_name(const char *name, char **id);
 
-/** Convert a GVariant like described in GdkPixbuf
+/** Convert a GVariant like described in GdkPixbuf, scaled according to settings
  *
  * 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.

src/notification.c

@@ -252,7 +252,6 @@ void notification_icon_replace_path(struct notification *n, const char *new_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)
@@ -264,7 +263,6 @@ void notification_icon_replace_data(struct notification *n, GVariant *new_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 */

src/settings.c

@@ -423,6 +423,12 @@ void load_settings(char *cmdline_config_path)
                 g_free(c);
         }
 
+        settings.min_icon_size = option_get_int(
+                "global",
+                "min_icon_size", "-min_icon_size", defaults.min_icon_size,
+                "Scale smaller icons up to this size, set to 0 to disable. If max_icon_size also specified, that has the final say."
+        );
+
         settings.max_icon_size = option_get_int(
                 "global",
                 "max_icon_size", "-max_icon_size", defaults.max_icon_size,

src/settings.h

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

test/dbus.c

@@ -6,6 +6,7 @@
 #include <gdk-pixbuf/gdk-pixbuf.h>
 #include <gio/gio.h>
 
+#include "helpers.h"
 #include "queues.h"
 
 extern const char *base;
@@ -252,33 +253,13 @@ bool dbus_notification_fire(struct dbus_notification *n, uint *id)
 
 void dbus_notification_set_raw_image(struct dbus_notification *n_dbus, const char *path)
 {
-        GdkPixbuf *pb = gdk_pixbuf_new_from_file(path, NULL);
+        GVariant *hint = notification_setup_raw_image(path);
-
+        if (!hint)
-        if (!pb)
                 return;
 
-        GVariant *hint_data = g_variant_new_from_data(
-                                G_VARIANT_TYPE("ay"),
-                                gdk_pixbuf_read_pixels(pb),
-                                gdk_pixbuf_get_byte_length(pb),
-                                TRUE,
-                                (GDestroyNotify) g_object_unref,
-                                g_object_ref(pb));
-
-        GVariant *hint = g_variant_new(
-                                "(iiibii@ay)",
-                                gdk_pixbuf_get_width(pb),
-                                gdk_pixbuf_get_height(pb),
-                                gdk_pixbuf_get_rowstride(pb),
-                                gdk_pixbuf_get_has_alpha(pb),
-                                gdk_pixbuf_get_bits_per_sample(pb),
-                                gdk_pixbuf_get_n_channels(pb),
-                                hint_data);
-
         g_hash_table_insert(n_dbus->hints,
                             g_strdup("image-data"),
                             g_variant_ref_sink(hint));
-        g_object_unref(pb);
 }
 
 /////// TESTS

test/helpers.c

@@ -0,0 +1,35 @@
+#include <gdk-pixbuf/gdk-pixbuf.h>
+
+#include "helpers.h"
+
+GVariant *notification_setup_raw_image(const char *path)
+{
+        GdkPixbuf *pb = gdk_pixbuf_new_from_file(path, NULL);
+
+        if (!pb)
+                return NULL;
+
+        GVariant *hint_data = g_variant_new_from_data(
+                                G_VARIANT_TYPE("ay"),
+                                gdk_pixbuf_read_pixels(pb),
+                                gdk_pixbuf_get_byte_length(pb),
+                                TRUE,
+                                (GDestroyNotify) g_object_unref,
+                                g_object_ref(pb));
+
+        GVariant *hint = g_variant_new(
+                                "(iiibii@ay)",
+                                gdk_pixbuf_get_width(pb),
+                                gdk_pixbuf_get_height(pb),
+                                gdk_pixbuf_get_rowstride(pb),
+                                gdk_pixbuf_get_has_alpha(pb),
+                                gdk_pixbuf_get_bits_per_sample(pb),
+                                gdk_pixbuf_get_n_channels(pb),
+                                hint_data);
+
+        g_object_unref(pb);
+
+        return hint;
+}
+
+/* vim: set tabstop=8 shiftwidth=8 expandtab textwidth=0: */

test/helpers.h

@@ -0,0 +1,9 @@
+#ifndef DUNST_TEST_HELPERS_H
+#define DUNST_TEST_HELPERS_H
+
+#include <glib.h>
+
+GVariant *notification_setup_raw_image(const char *path);
+
+#endif
+/* vim: set tabstop=8 shiftwidth=8 expandtab textwidth=0: */

test/icon.c

@@ -113,6 +113,61 @@ TEST test_get_pixbuf_from_icon_fileuri(void)
         PASS();
 }
 
+TEST test_icon_size_clamp_too_small(void)
+{
+        int w = 12, h = 24;
+        bool resized = icon_size_clamp(&w, &h);
+        ASSERT(resized);
+        ASSERT_EQ(w, 16);
+        ASSERT_EQ(h, 32);
+
+        PASS();
+}
+
+TEST test_icon_size_clamp_not_necessary(void)
+{
+        int w = 20, h = 30;
+        bool resized = icon_size_clamp(&w, &h);
+        ASSERT(!resized);
+        ASSERT_EQ(w, 20);
+        ASSERT_EQ(h, 30);
+
+        PASS();
+}
+
+TEST test_icon_size_clamp_too_big(void)
+{
+        int w = 75, h = 150;
+        bool resized = icon_size_clamp(&w, &h);
+        ASSERT(resized);
+        ASSERT_EQ(w, 50);
+        ASSERT_EQ(h, 100);
+
+        PASS();
+}
+
+TEST test_icon_size_clamp_too_small_then_too_big(void)
+{
+        int w = 8, h = 80;
+        bool resized = icon_size_clamp(&w, &h);
+        ASSERT(resized);
+        ASSERT_EQ(w, 10);
+        ASSERT_EQ(h, 100);
+
+        PASS();
+}
+
+TEST test_get_pixbuf_from_icon_both_is_scaled(void)
+{
+        GdkPixbuf *pixbuf = get_pixbuf_from_icon("onlypng");
+        ASSERT(pixbuf);
+        ASSERT_EQ(gdk_pixbuf_get_width(pixbuf), 16);
+        ASSERT_EQ(gdk_pixbuf_get_height(pixbuf), 16);
+        g_clear_pointer(&pixbuf, g_object_unref);
+
+        PASS();
+}
+
 SUITE(suite_icon)
 {
         settings.icon_path = g_strconcat(
@@ -129,6 +184,31 @@ SUITE(suite_icon)
         RUN_TEST(test_get_pixbuf_from_icon_onlypng);
         RUN_TEST(test_get_pixbuf_from_icon_filename);
         RUN_TEST(test_get_pixbuf_from_icon_fileuri);
+        RUN_TEST(test_icon_size_clamp_not_necessary);
+
+        settings.min_icon_size = 16;
+        settings.max_icon_size = 100;
+
+        RUN_TEST(test_get_pixbuf_from_icon_both_is_scaled);
+        RUN_TEST(test_icon_size_clamp_too_small);
+        RUN_TEST(test_icon_size_clamp_not_necessary);
+        RUN_TEST(test_icon_size_clamp_too_big);
+        RUN_TEST(test_icon_size_clamp_too_small_then_too_big);
+
+        settings.min_icon_size = 16;
+        settings.max_icon_size = 0;
+
+        RUN_TEST(test_icon_size_clamp_too_small);
+        RUN_TEST(test_icon_size_clamp_not_necessary);
+
+        settings.min_icon_size = 0;
+        settings.max_icon_size = 100;
+
+        RUN_TEST(test_icon_size_clamp_not_necessary);
+        RUN_TEST(test_icon_size_clamp_too_big);
+
+        settings.min_icon_size = 0;
+        settings.max_icon_size = 0;
 
         g_clear_pointer(&settings.icon_path, g_free);
 }

test/notification.c

@@ -1,5 +1,6 @@
 #include "../src/notification.c"
 #include "greatest.h"
+#include "helpers.h"
 
 #include "../src/option_parser.h"
 #include "../src/settings.h"
@@ -124,6 +125,76 @@ TEST test_notification_referencing(void)
         PASS();
 }
 
+
+static struct notification *notification_load_icon_with_scaling(int min_icon_size, int max_icon_size)
+{
+        struct notification *n = notification_create();
+
+        char *path = g_strconcat(base, "/data/icons/valid.svg", NULL); // 16x16
+
+        GVariant *rawIcon = notification_setup_raw_image(path);
+
+        settings.min_icon_size = min_icon_size;
+        settings.max_icon_size = max_icon_size;
+        notification_icon_replace_data(n, rawIcon);
+        settings.min_icon_size = 0;
+        settings.max_icon_size = 0;
+
+        g_variant_unref(rawIcon);
+        g_free(path);
+
+        return n;
+}
+
+TEST test_notification_icon_scaling_toosmall(void)
+{
+        struct notification *n = notification_load_icon_with_scaling(20, 100);
+
+        ASSERT_EQ(gdk_pixbuf_get_width(n->icon), 20);
+        ASSERT_EQ(gdk_pixbuf_get_height(n->icon), 20);
+
+        notification_unref(n);
+
+        PASS();
+}
+
+
+TEST test_notification_icon_scaling_toolarge(void)
+{
+        struct notification *n = notification_load_icon_with_scaling(5, 10);
+
+        ASSERT_EQ(gdk_pixbuf_get_width(n->icon), 10);
+        ASSERT_EQ(gdk_pixbuf_get_height(n->icon), 10);
+
+        notification_unref(n);
+
+        PASS();
+}
+
+TEST test_notification_icon_scaling_notconfigured(void)
+{
+        struct notification *n = notification_load_icon_with_scaling(0, 0);
+
+        ASSERT_EQ(gdk_pixbuf_get_width(n->icon), 16);
+        ASSERT_EQ(gdk_pixbuf_get_height(n->icon), 16);
+
+        notification_unref(n);
+
+        PASS();
+}
+
+TEST test_notification_icon_scaling_notneeded(void)
+{
+        struct notification *n = notification_load_icon_with_scaling(10, 20);
+
+        ASSERT_EQ(gdk_pixbuf_get_width(n->icon), 16);
+        ASSERT_EQ(gdk_pixbuf_get_height(n->icon), 16);
+
+        notification_unref(n);
+
+        PASS();
+}
+
 TEST test_notification_format_message(struct notification *n, const char *format, const char *exp)
 {
         n->format = format;
@@ -167,6 +238,10 @@ SUITE(suite_notification)
         RUN_TEST(test_notification_is_duplicate);
         RUN_TEST(test_notification_replace_single_field);
         RUN_TEST(test_notification_referencing);
+        RUN_TEST(test_notification_icon_scaling_toosmall);
+        RUN_TEST(test_notification_icon_scaling_toolarge);
+        RUN_TEST(test_notification_icon_scaling_notconfigured);
+        RUN_TEST(test_notification_icon_scaling_notneeded);
 
         // TEST notification_format_message
         struct notification *a = notification_create();