Skip to content

Rgba support#717

Merged
tsipinakis merged 11 commits intodunst-project:masterfrom
nick87720z:rgba-support
May 28, 2020
Merged

Rgba support#717
tsipinakis merged 11 commits intodunst-project:masterfrom
nick87720z:rgba-support

Conversation

@nick87720z
Copy link
Contributor

Besides core rgba support I tried to implement support for arbitrary hex color string length (though limited by maximum unsigned length for given machine), as well as rgba() and rgb() formats, using scanf.

What's also interesting, adjacent non-overlaping areas are better to be combined in additive mode, so that fading out pixels at bounds mix correctly. I'm not sure though, if cairo uses alpha channel correctly (must be summed too, not like how it does with multiply op), but at least it removes artifacts between frame and background, visible when both are equally contrast with background.

Btw, imho colors are better to be parsed once on theme loading instead of on each draw().

@codecov-commenter
Copy link

codecov-commenter commented May 23, 2020

Codecov Report

Merging #717 into master will decrease coverage by 0.16%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #717      +/-   ##
==========================================
- Coverage   65.91%   65.74%   -0.17%     
==========================================
  Files          29       29              
  Lines        5102     5115      +13     
==========================================
  Hits         3363     3363              
- Misses       1739     1752      +13     
Flag Coverage Δ
#unittests 65.74% <0.00%> (-0.17%) ⬇️
Impacted Files Coverage Δ
src/draw.c 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8134179...12bea04. Read the comment docs.

@tsipinakis
Copy link
Member

tsipinakis commented May 24, 2020

The comment I posed in #713 still applies here. I don't see a reason to support so many different formats. #RRGGBBAA is widely used and and I don't think adding more provides any sort of useful functionality. In fact, the opposite, it unnecessarily increases code complexity and adds more things that may break without anyone realising. 'Less is more' as they say.

The docs indeed did claim #RGB support, but given that no-one has complained thus far that it doesn't work, my reaction would be to fix the docs rather than implement it.

Probably I was too precatious, not relying to common rule about
unsigned subtraction to minus.
@nick87720z
Copy link
Contributor Author

Hm, let assume FP format and hex string generic parsers are too complex.
But as for #RGB, it looks two times more readable than #RRGGBB. At least I started to use it where is possible :] .

I could undo back to switch(len) variant. It is still necessary to differ between #RRGGBBAA and #RRGGBB, unless you are going to break compatibility - with original code RRGGBB will be GGBBAA :) . IMHO, there is nothing complex in hex_to_color() taking digits-per-channel parameter.

@nick87720z
Copy link
Contributor Author

nick87720z commented May 28, 2020

ping? At least one (my) vote for #RGB/#RGBA hex code support :)
Or you want it in same form as pre-rgba, with exception that it now expects full 32-bit value?

Copy link
Member

@tsipinakis tsipinakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excuse the delay.

I could undo back to switch(len) variant.

That's what I had in mind. After thinking it over I'm fine with merging #RGB support given it requires minimal changes to the parsing.
However the other variant was too long and complex in my opinion which is why I said better to keep it simple.

All in all, LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants