Merge pull request #1330 from bynect/change-lengths

Refactor length parsing and make offset use the (N,N) syntax

docs/dunst.5.pod

@@ -141,7 +141,9 @@ notification being off screen.
 
 Examples:
         origin = top-right
-        offset = 10x300 # a margin of 10 pixels from the right and 300 pixels from the top
+        offset = (10, 300) # a margin of 10 pixels from the right and 300 pixels from the top
+
+For backwards compatibility the syntax NxN is also accepted.
 
 =item B<scale> (default: 0, X11 only)
 

dunstrc

@@ -33,7 +33,7 @@
     origin = top-right
 
     # Offset from the origin
-    offset = 10x50
+    offset = (10, 50)
 
     # Scale factor. It is auto-detected if value is 0.
     scale = 0

src/draw.c

@@ -202,7 +202,7 @@ static void layout_setup(struct colored_layout *cl, int width, int height, doubl
         int icon_width = cl->icon ? get_icon_width(cl->icon, scale) + horizontal_padding : 0;
         int text_width = width - 2 * settings.h_padding - (cl->n->icon_position == ICON_TOP ? 0 : icon_width);
         int progress_bar_height = have_progress_bar(cl) ? settings.progress_bar_height + settings.padding : 0;
-        int max_text_height = MAX(0, settings.height - progress_bar_height - 2 * settings.padding);
+        int max_text_height = MAX(0, settings.height.max - progress_bar_height - 2 * settings.padding);
         layout_setup_pango(cl->l, text_width, max_text_height, cl->n->word_wrap, cl->n->ellipsize, cl->n->alignment);
 }
 
@@ -219,7 +219,7 @@ static void free_colored_layout(void *data)
 static struct dimensions calculate_notification_dimensions(struct colored_layout *cl, double scale)
 {
         struct dimensions dim = { 0 };
-        layout_setup(cl, settings.width.max, settings.height, scale);
+        layout_setup(cl, settings.width.max, settings.height.max, scale);
 
         int horizontal_padding = get_horizontal_text_icon_padding(cl->n);
         int icon_width = cl->icon? get_icon_width(cl->icon, scale) + horizontal_padding : 0;
@@ -245,7 +245,7 @@ static struct dimensions calculate_notification_dimensions(struct colored_layout
         dim.h += progress_bar_height;
         dim.w = dim.text_width + icon_width + 2 * settings.h_padding;
 
-        dim.h = MIN(settings.height, dim.h + settings.padding * 2);
+        dim.h = MIN(settings.height.max, dim.h + settings.padding * 2);
         dim.w = MAX(settings.width.min, dim.w);
         if (have_progress_bar(cl))
                 dim.w = MAX(settings.progress_bar_min_width, dim.w);
@@ -709,7 +709,7 @@ static void render_content(cairo_t *c, struct colored_layout *cl, int width, dou
 {
         // Redo layout setup, while knowing the width. This is to make
         // alignment work correctly
-        layout_setup(cl, width, settings.height, scale);
+        layout_setup(cl, width, settings.height.max, scale);
 
         const int h = layout_get_height(cl, scale);
         LOG_D("Layout height %i", h);
@@ -852,7 +852,7 @@ static struct dimensions layout_render(cairo_surface_t *srf,
         get_text_size(cl->l, NULL, &h_text, scale);
 
         int bg_width = 0;
-        int bg_height = MIN(settings.height, (2 * settings.padding) + cl_h);
+        int bg_height = MIN(settings.height.max, (2 * settings.padding) + cl_h);
 
         cairo_surface_t *content = render_background(srf, cl, cl_next, dim.y, dim.w, bg_height, dim.corner_radius, corners, &bg_width, scale);
         cairo_t *c = cairo_create(content);
@@ -866,10 +866,10 @@ static struct dimensions layout_render(cairo_surface_t *srf,
         if (corners & (C_BOT | _C_LAST))
                 dim.y += settings.frame_width;
 
-        if ((2 * settings.padding + cl_h) < settings.height)
+        if ((2 * settings.padding + cl_h) < settings.height.max)
                 dim.y += cl_h + 2 * settings.padding;
         else
-                dim.y += settings.height;
+                dim.y += settings.height.max;
 
         if (settings.gap_size)
                 dim.y += settings.gap_size;

src/option_parser.c

@@ -131,15 +131,13 @@ int string_parse_list(const void *data, const char *s, void *ret) {
         switch (type) {
                 case MOUSE_LIST:
                         arr = string_to_array(s, ",");
-                        success = string_parse_enum_list(&mouse_action_enum_data,
-                                        arr, ret);
+                        success = string_parse_enum_list(&mouse_action_enum_data, arr, ret);
                         break;
                 case OFFSET_LIST:
                         arr = string_to_array(s, "x");
                         int len = string_array_length(arr);
                         if (len != 2) {
                                 success = false;
-                                LOG_W("Offset has two values, separated by an 'x'");
                                 break;
                         }
                         int *int_arr = NULL;
@@ -301,72 +299,46 @@ int get_setting_id(const char *key, const char *section) {
         return -1;
 }
 
-// TODO simplify this function
+// NOTE: We do minimal checks in this function so the values should be sanitized somewhere else
 int string_parse_length(void *ret_in, const char *s) {
         struct length *ret = (struct length*) ret_in;
-        int val = 0;
         char *s_stripped = string_strip_brackets(s);
+
+        // single int without brackets
         if (!s_stripped) {
-                // single int without brackets
+                int val = 0;
                 bool success = safe_string_to_int(&val, s);
-                if (success && val > 0) {
-                        // single int
-                        ret->min = val;
-                        ret->max = val;
-                        return true;
-                }
-                if (val <= 0) {
-                        LOG_W("A length should be a positive value");
+                if (!success) {
+                        LOG_W("Specify either a single value or two comma-separated values between parentheses");
+                        return false;
                 }
-                return false;
-        }
 
+                ret->min = val;
+                ret->max = val;
+                return true;
+        }
 
         char **s_arr = string_to_array(s_stripped, ",");
-        int len = string_array_length(s_arr);
+        g_free(s_stripped);
 
-        if (len <= 1) {
-                LOG_W("Please specify a minimum and maximum value or a single value without brackets");
-                g_strfreev(s_arr);
-                g_free(s_stripped);
-                return false;
-        }
-        if (len > 2) {
+        int len = string_array_length(s_arr);
+        if (len != 2) {
                 g_strfreev(s_arr);
-                g_free(s_stripped);
-                LOG_W("Too many values in array. A length should be only one or two values");
+                LOG_W("Specify either a single value or two comma-separated values between parentheses");
                 return false;
         }
 
         int *int_arr = NULL;
         bool success = string_parse_int_list(s_arr, &int_arr, true);
-        if (!success) {
-                g_strfreev(s_arr);
-                g_free(s_stripped);
-                return false;
-        }
+        g_strfreev(s_arr);
 
-        if (int_arr[0] == -1)
-                int_arr[0] = 0;
+        if (!success)
+                return false;
 
-        if (int_arr[1] == -1)
-                int_arr[1] = INT_MAX;
-
-        if (int_arr[0] < 0 || int_arr[1] < 0) {
-                LOG_W("A lengths should be positive");
-                success = false;
-        } else if (int_arr[0] > int_arr[1]) {
-                LOG_W("The minimum value should be less than the maximum value. (%i > %i)",
-                                int_arr[0], int_arr[1]);
-                success = false;
-        } else {
-                ret->min = int_arr[0];
-                ret->max = int_arr[1];
-        }
+        ret->min = int_arr[0] == -1 ? INT_MIN : int_arr[0];
+        ret->max = int_arr[1] == -1 ? INT_MAX : int_arr[1];
 
         g_free(int_arr);
-        g_strfreev(s_arr);
-        g_free(s_stripped);
         return success;
 }
 
@@ -428,6 +400,11 @@ bool set_from_string(void *target, struct setting setting, const char *value) {
                         LOG_D("list type %i", GPOINTER_TO_INT(setting.parser_data));
                         return string_parse_list(setting.parser_data, value, target);
                 case TYPE_LENGTH:
+                        // Keep compatibility with old offset syntax
+                        if (STR_EQ(setting.name, "offset") && string_parse_list(GINT_TO_POINTER(OFFSET_LIST), value, target)) {
+                                LOG_I("Using legacy offset syntax NxN, you should switch to the new syntax (N, N)");
+                                return true;
+                        }
                         return string_parse_length(target, value);
                 case TYPE_COLOR:
                         return string_parse_color(value, target);

src/settings.c

@@ -202,16 +202,38 @@ void check_and_correct_settings(struct settings *s) {
                         DIE("setting progress_bar_max_width is smaller than progress_bar_min_width");
                 }
                 if (s->progress_bar_min_width > s->width.max) {
-                        LOG_W("Progress bar min width is greater than the max width of the notification.");
+                        LOG_W("Progress bar min width is greater than the max width of the notification");
                 }
                 int progress_bar_max_corner_radius = (s->progress_bar_height / 2);
                 if (s->progress_bar_corner_radius > progress_bar_max_corner_radius) {
                         settings.progress_bar_corner_radius = progress_bar_max_corner_radius;
-                        LOG_W("Progress bar corner radius clamped to half of progress bar height (%i).",
+                        LOG_W("Progress bar corner radius clamped to half of progress bar height (%i)",
                                 progress_bar_max_corner_radius);
                 }
         }
 
+        // check lengths
+        if (s->width.min == INT_MIN) {
+                s->width.min = 0;
+        }
+        if (s->width.min < 0 || s->width.max < 0) {
+                DIE("setting width does not support negative values");
+        }
+        if (s->width.min > s->width.max) {
+                DIE("setting width min (%i) is always greather than max (%i)", s->width.min, s->width.max);
+        }
+
+        if (s->height.min < 0 || s->height.max < 0) {
+                DIE("setting height does not support negative values");
+        }
+        if (s->height.min != s->height.max) {
+                LOG_W("Dynamic height is not yet supported");
+        }
+
+        if (s->offset.x == INT_MIN || s->offset.y == INT_MAX) {
+                DIE("setting offset needs both horizontal and vertical values");
+        }
+
         // TODO Implement this with icon sizes as rules
 
         // restrict the icon size to a reasonable limit if we have a fixed width.

src/settings.h

@@ -173,8 +173,8 @@ struct settings {
         enum zwlr_layer_shell_v1_layer layer;
         enum origin_values origin;
         struct length width;
-        int height;
-        struct position offset;
+        struct length height;
+        struct position offset; // NOTE: we rely on the fact that lenght and position are similar
         int notification_limit;
         int gap_size;
 };

src/settings_data.h

@@ -1529,7 +1529,7 @@ static const struct setting allowed_settings[] = {
                 .name = "height",
                 .section = "global",
                 .description = "The maximum height of a single notification, excluding the frame.",
-                .type = TYPE_INT,
+                .type = TYPE_LENGTH,
                 .default_value = "300",
                 .value = &settings.height,
                 .parser = NULL,
@@ -1539,11 +1539,11 @@ static const struct setting allowed_settings[] = {
                 .name = "offset",
                 .section = "global",
                 .description = "The offset of the notification from the origin.",
-                .type = TYPE_LIST,
-                .default_value = "10x50",
+                .type = TYPE_LENGTH,
+                .default_value = "(10, 50)",
                 .value = &settings.offset,
                 .parser = NULL,
-                .parser_data = GINT_TO_POINTER(OFFSET_LIST),
+                .parser_data = NULL,
         },
         {
                 .name = "notification_limit",

test/draw.c

@@ -70,23 +70,23 @@ GSList *get_dummy_layouts(GSList *notifications)
         return layouts;
 }
 
-int get_small_max_height(void)
+struct length get_small_max_height(void)
 {
         // to keep test calculations simpler, set max height small to
         // only test cases where height is not dynamically determined
         // by notification content
         // future tests targeting dynamic sizing logic could be added
         // to address this limitation
-        int small_max_height = 10;
-        return small_max_height;
+		struct length height = { 0, 10 };
+		return height;
 }
 
 int get_expected_dimension_height(int layout_count)
 {
         // assumes settings.height == notification height, see get_small_max_height
         int separator_height = (layout_count - 1) * settings.separator_height;
         int total_gap_size = (layout_count - 1) * settings.gap_size;
-        int height = settings.height * layout_count;
+        int height = settings.height.max * layout_count;
         int frame_width_total_height;
         int expected_height;
         if(settings.gap_size) {
@@ -102,7 +102,7 @@ int get_expected_dimension_height(int layout_count)
 int get_expected_dimension_y_offset(int layout_count)
 {
         // assumes settings.height == notification height, see get_small_max_height
-        int expected_y = layout_count * settings.height;
+        int expected_y = layout_count * settings.height.max;
         if(settings.gap_size) {
                 expected_y += (layout_count * (2 * settings.frame_width));
                 expected_y += (layout_count * settings.gap_size);
@@ -155,7 +155,7 @@ TEST test_layout_from_notification_no_icon(void)
 
 TEST test_calculate_dimensions_height_no_gaps(void)
 {
-        int original_height = settings.height;
+        struct length original_height = settings.height;
         bool orginal_gap_size = settings.gap_size;
         settings.height = get_small_max_height();
         settings.gap_size = 10;
@@ -201,7 +201,7 @@ TEST test_calculate_dimensions_height_no_gaps(void)
 
 TEST test_calculate_dimensions_height_gaps(void)
 {
-        int original_height = settings.height;
+        struct length original_height = settings.height;
         bool orginal_gap_size = settings.gap_size;
         settings.height = get_small_max_height();
         settings.gap_size = 10;
@@ -247,7 +247,7 @@ TEST test_calculate_dimensions_height_gaps(void)
 
 TEST test_layout_render_no_gaps(void)
 {
-        int original_height = settings.height;
+        struct length original_height = settings.height;
         bool orginal_gap_size = settings.gap_size;
         settings.height = get_small_max_height();
         settings.gap_size = 0;
@@ -291,7 +291,7 @@ TEST test_layout_render_no_gaps(void)
 
 TEST test_layout_render_gaps(void)
 {
-        int original_height = settings.height;
+        struct length original_height = settings.height;
         bool orginal_gap_size = settings.gap_size;
         settings.height = get_small_max_height();
         settings.gap_size = 10;