Gradients #1286
Gradients #1286
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1286 +/- ##
==========================================
- Coverage 64.48% 64.35% -0.14%
==========================================
Files 48 48
Lines 7918 7956 +38
==========================================
+ Hits 5106 5120 +14
- Misses 2812 2836 +24
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@fwsmit gladly the ci passed, so let me know what do you think 👍🏻 |
|
What do you mean with delta correction? |
Good idea. I was thinking of adding the possibility to provide a list of xy points for the gradient breaks. But just an angle is also fine. Let me know if I should add gradient support for other color. For the delta correction is something that's done for the foreground but I don't think it's needed |
| char *end, *str = strv[i]; | ||
| if (STR_EMPTY(str)) continue; | ||
|
|
||
| uint_fast32_t val = strtoul(str+1, &end, 16); |
There was a problem hiding this comment.
Why uint_fast32_t ... shouldn't it be unsigned long?
Are you using str+1 here to skip the initial #? This should be done, iff it is actually a #.
How about handling of named colors like Yellow? Maybe I'm missing something here, but I couldn't find, where this is handled.
There was a problem hiding this comment.
Again, i am following the previous behavior (but I don't like it either)
| case 6: g.cs[g.length++] = hex_to_color((val << 8) | 0xFF, 2); break; | ||
| case 4: g.cs[g.length++] = hex_to_color(val, 1); break; | ||
| case 8: g.cs[g.length++] = hex_to_color(val, 2); break; | ||
| } |
There was a problem hiding this comment.
There should be a warning, when an incorrectly formatted color was found here.
| g.cs = g_malloc0(g_strv_length(strv) * sizeof(struct color)); | ||
| for (int i = 0; strv[i] != NULL; i++) { | ||
| char *end, *str = strv[i]; | ||
| if (STR_EMPTY(str)) continue; |
There was a problem hiding this comment.
Just for understanding: this would mean an empty element would be allowed and yield a opaque black?
There was a problem hiding this comment.
I am following the previous behavior
| printf("\thighlight: %s\n", n->colors.highlight); | ||
| printf("\tframe: %s\n", n->colors.frame); | ||
| printf("\thighlight: "); | ||
| for (int i = 0; n->colors.highlight[i]; i++) printf("%s ", n->colors.highlight[i]); |
There was a problem hiding this comment.
Nit pick: this would leave a trailing white space and moving the line break to the following entry seems a bit fragile (when lines are moved around). How about something like
gchar *highlight = g_strjoinv(" ", n->colors.highlight);
printf("\thighlight: %s\n", highlight);
g_free(highlight);There was a problem hiding this comment.
Fair enough, I wasn't satisfied with that in the first place
|
@fwsmit @zappolowski I don't particularly like the way dunst decodes color strings at every draw call, since it makes for more difficult validation and it is plain inefficient. I was thinking of storing in the config directly the color struct and parse once in the option_parser. Is there something that prevents me from doing that (like protocols) or can I go ahead? Obviously I would close this pr and make it again later (and better) to store gradients in the config Also I would remove the fact that invalid colors are black and just error out on parsing |
|
I had a similar idea in mind, but TBH I didn't have the time yet to fully dig through all the code and thus cannot foresee what the implications are. From quickly skimming through the Desktop Notification Specification I could not find anything which prevents this (e.g. sending color information along with the actual notification data). Having said that, I think parsing should be done once when reading the configuration and not on every notification being created. So, go for it, but maybe do this refactoring first and then (on top of that) add the new gradient feature. |
Yes that was the plan, and it will make the whole feature easier |
|
I will rework this completely. At the moment my idea is to store a cairo_pattern_t in the gradient struct |
This is the implementation of #1273.
At the moment I only implemented the gradient for
highlight, but it can be easily applied also tobackground,foregroundetc.I am using a cairo linear pattern which allows for any number of colors so the user is free.
TODO:
Example