Store colors in a struct #1306

bynect · 2024-03-04T21:52:00Z

I changed every usage of colors from a string to a color struct. Colors are now parsed only once in the option_parser instead of every single draw call.

Note: Hopefully I haven't missed anything. I would be super grateful if someone could do some extra testing on their own if they encounter anything strange

bynect · 2024-03-04T21:52:10Z

Still working on the tests

bynect · 2024-03-04T23:56:44Z

Fixes what we talked about in #1286

bynect · 2024-03-05T17:18:11Z

Note that invalid colors sent to dbus are ignored

fwsmit · 2024-03-08T15:35:15Z

Yeah, looks good!

zappolowski · 2024-03-08T16:10:41Z

src/option_parser.c

+#define UINT_MAX_N(bits) ((1 << bits) - 1)
+
+// Parse a #RRGGBB[AA] string
+int string_parse_color(const void *data, const char *s, void *ret)


Suggested change

int string_parse_color(const void *data, const char *s, void *ret)

int string_parse_color(const void *data, const char *s, struct color *color)

Shouldn't the target always be a struct color *? Then we should be able to get rid of line 205 as well.

Yes, I was trying to use TYPE_CUSTOM before but then I changed

zappolowski · 2024-03-08T16:13:44Z

src/draw.h

+};
+
+#define COLOR_UNINIT { -1, -1, -1, -1 }
+#define COLOR_VALID(c) ((c).r >= 0 && (c).g >= 0 && (c).b >= 0 && (c).a >= 0)


Should we check c.x > 1 here as well (instead of just when trying to print it)? Cairo colors have to be in the [0, 1] range and thus COLOR_VALID should enforce this everywhere.

My idea was that either a color was valid or it was initialized with all -1. But yes, maybe an extra check for correctness is fine

zappolowski · 2024-03-08T16:22:09Z

src/option_parser.c

+// Parse a #RRGGBB[AA] string
+int string_parse_color(const void *data, const char *s, void *ret)
+{
+        if (STR_EMPTY(s) || *s != '#') {


I know it was basically this way already before your changes, but dunst.5 states, that named colors like "Yellow" are supported, which doesn't seem to be the case anymore (I don't know since when). Either it is removed from the documentation (breaking change, should be added - retroactively - to CHANGELOG.md) or it is possible to somehow get it back to work (starting point could be this list).

Mmm, didn't know about that. I moved the parsing code from draw.c. maybe we can add color names in another pr.

Did you think about embedding an array with all the info from that file?

zappolowski · 2024-03-08T16:30:29Z

src/draw.h

+
+#define COLOR_UNINIT { -1, -1, -1, -1 }
+#define COLOR_VALID(c) ((c).r >= 0 && (c).g >= 0 && (c).b >= 0 && (c).a >= 0)
+#define COLOR_SAME(c1, c2) ((c1).r == (c2).r && (c1).g == (c2).g && (c1).b == (c2).b && (c1).a == (c2).a)


This one is just used in tests. I think, it should be moved to test/helpers.h.

zappolowski · 2024-03-08T16:32:07Z

src/notification.c

+        if (!COLOR_VALID(n->colors.fg)) n->colors.fg = defcolors.fg;
+        if (!COLOR_VALID(n->colors.bg)) n->colors.bg = defcolors.bg;
+        if (!COLOR_VALID(n->colors.highlight)) n->colors.highlight = defcolors.highlight;
+        if (!COLOR_VALID(n->colors.frame)) n->colors.frame = defcolors.frame;


You're a bit inconsistent on your style (see src/rules.c). Sometimes you make single statement if a oneliner and sometimes it's put on two lines. Personally, I prefer to have the two line variant.

Right. I try to minimize the use of lines, so if both the condition and statements are really short I put them in a single line (especially if there are a lot chained like here). I will try to be more consistent in the future

zappolowski · 2024-03-08T16:44:39Z

src/settings.c

@@ -160,12 +160,16 @@ void settings_init(void) {
        }
 }

+// NOTE: This function is probably outdated and no longer relevant
+//       Since it does not even fully display rule information we
+//       may want to remove it or change it


I'm fine with removing it ... it basically just echoes the user input. The only use case could be to find typos in the configuration, but a) is this just printed at DEBUG level and b) is there already a WARNING issued, whenever an unknown key is encountered.

The main problem is that it prints only some values, so it doesn't really help that much

bynect · 2024-03-09T09:27:56Z

I will do some fixes and merge it then 👍

zappolowski · 2024-03-09T09:56:13Z

I will do some fixes and merge it then 👍

I wouldn't merge as long as CI doesn't work properly. I also accidentally merged a branch yesterday - but luckily, this was just update of a pod file and thus the chance of breakage is rather low.

bynect · 2024-03-09T11:01:27Z

Right, ci is still down 😬

zappolowski · 2024-04-05T09:07:53Z

@bynect As I've merged the new pipeline you could rebase this branch and then hopefully we can go forward merging it.

codecov-commenter · 2024-04-06T20:07:17Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 83.91608% with 23 lines in your changes missing coverage. Please review.

Project coverage is 65.30%. Comparing base (15b1e8c) to head (6740fb0).
Report is 179 commits behind head on master.

Files with missing lines	Patch %	Lines
src/draw.c	53.57%	13 Missing ⚠️
src/notification.c	69.23%	4 Missing ⚠️
src/option_parser.c	92.85%	3 Missing ⚠️
src/dbus.c	75.00%	2 Missing ⚠️
src/rules.c	87.50%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1306      +/-   ##
==========================================
+ Coverage   65.06%   65.30%   +0.23%     
==========================================
  Files          48       48              
  Lines        8173     8215      +42     
==========================================
+ Hits         5318     5365      +47     
+ Misses       2855     2850       -5

Flag	Coverage Δ
unittests	`65.30% <83.91%> (+0.23%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bynect · 2024-04-06T20:42:54Z

@zappolowski should I merge it now?

zappolowski · 2024-04-07T16:48:05Z

Yes, just go ahead and merge it.

bynect force-pushed the cache-colors branch 2 times, most recently from cf83285 to 3ad3886 Compare March 4, 2024 23:44

bynect requested review from fwsmit and zappolowski March 4, 2024 23:56

zappolowski reviewed Mar 8, 2024

View reviewed changes

bynect added 7 commits April 6, 2024 22:04

Store colors in a struct instead of strings

d0d2f43

Properly init notification colors

e3110d0

Print color information again

0397a5e

Add color parser type

57c7262

Fix sepcolor testing

e50ebc1

Update test_dbus_notify_colors

15f18f9

Add tests for parsing color

4881128

bynect force-pushed the cache-colors branch from 2b36fa5 to 52a813c Compare April 6, 2024 20:05

bynect force-pushed the cache-colors branch from 52a813c to f145c42 Compare April 6, 2024 20:09

Add checks and refactor

6740fb0

bynect force-pushed the cache-colors branch from f145c42 to 6740fb0 Compare April 6, 2024 20:10

zappolowski approved these changes Apr 7, 2024

View reviewed changes

bynect merged commit 6d2296b into dunst-project:master Apr 7, 2024
19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store colors in a struct #1306

Store colors in a struct #1306

bynect commented Mar 4, 2024 •

edited

Loading

bynect commented Mar 4, 2024

bynect commented Mar 4, 2024

bynect commented Mar 5, 2024

fwsmit commented Mar 8, 2024

zappolowski Mar 8, 2024

bynect Mar 9, 2024

zappolowski Mar 8, 2024

bynect Mar 9, 2024

zappolowski Mar 8, 2024

bynect Mar 9, 2024

zappolowski Mar 8, 2024

zappolowski Mar 8, 2024

bynect Mar 9, 2024

zappolowski Mar 8, 2024

bynect Mar 9, 2024

bynect commented Mar 9, 2024

zappolowski commented Mar 9, 2024

bynect commented Mar 9, 2024

zappolowski commented Apr 5, 2024

codecov-commenter commented Apr 6, 2024 •

edited

Loading

bynect commented Apr 6, 2024

zappolowski commented Apr 7, 2024

	int string_parse_color(const void data, const char s, void *ret)
	int string_parse_color(const void data, const char s, struct color *color)

Store colors in a struct #1306

Store colors in a struct #1306

Conversation

bynect commented Mar 4, 2024 • edited Loading

bynect commented Mar 4, 2024

bynect commented Mar 4, 2024

bynect commented Mar 5, 2024

fwsmit commented Mar 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bynect commented Mar 9, 2024

zappolowski commented Mar 9, 2024

bynect commented Mar 9, 2024

zappolowski commented Apr 5, 2024

codecov-commenter commented Apr 6, 2024 • edited Loading

Codecov Report

bynect commented Apr 6, 2024

zappolowski commented Apr 7, 2024

bynect commented Mar 4, 2024 •

edited

Loading

codecov-commenter commented Apr 6, 2024 •

edited

Loading