Add new beautiful.colorful module #2276
Conversation
Quick first comment: That page does not have any license statement, so must be considered "all rights reserved", thus GPL incompatible. |
Ah drats. That's a shame. :( |
|
They do mention something related to licensing here:
Maybe you can ask them for details. |
|
Here's the replies I got:
|
|
Granted the formula for rounding isn't exactly copyrighted, it's just math. I figured I'd be honest though and say I found that specific code on the wiki. 🙁 |
lib/gears/math.lua
Outdated
| -- @tparam[opt=1] number bracket 10s place to round to | ||
| function gmath.roundf(v, bracket) | ||
| -- From lua-users.org SimpleRound | ||
| local function sign(v) return (v >= 0 and 1) or -1 end |
There was a problem hiding this comment.
The sign function is used just once and thus does not need to be a function.
Also, unit tests would be nice (also for the other functions in this PR)
There was a problem hiding this comment.
I am planning on adding tests, I just hadn't gotten to it yet. Wanted to see if everyone agreed these were useful enough to add.
There was a problem hiding this comment.
And true, I can yank out sign and inline it. Unless we want to add it as a gears.math function.
lib/gears/math.lua
Outdated
| -- @class function | ||
| -- @name roundf | ||
| -- @tparam number v | ||
| -- @tparam[opt=1] number bracket 10s place to round to |
There was a problem hiding this comment.
So.... this is supposed to be 1, 10, 100, 0.001(?), or what?
I am not sure, but I feel like 0, 1, 2, 3 is more common for this (meaning: x'd place after the comma)
There was a problem hiding this comment.
Yes, you can round to any place you want. Rounding to 1 is default. If we change to 0, 1, 2, 3, etc. then we add an exponent operation in (which isn't slow, but just an extra op we can avoid).
lib/gears/math.lua
Outdated
| function gmath.roundf(v, bracket) | ||
| -- From lua-users.org SimpleRound | ||
| local function sign(v) return (v >= 0 and 1) or -1 end | ||
| bracket = bracket or 1 |
There was a problem hiding this comment.
Is the default of 1 sensible? In that case this just rounds away from zero (while gears.math.round rounds towards positive infinity).
Which raises the question if gears.math.round is wrong and should be fixed. C99's round() rounds away from zero as well and this behaviour would allow to implement this function as gears.math.round(v / bracket) * bracket.
There was a problem hiding this comment.
The default 1 is to round to the nearest whole number, which is what round does essentially, but this gives the extra argument for floating point rounding (and higher rounding to 10s and 1000s and such).
Codecov Report
@@ Coverage Diff @@
## master #2276 +/- ##
=========================================
+ Coverage 84.68% 85.4% +0.72%
=========================================
Files 496 518 +22
Lines 33768 35834 +2066
=========================================
+ Hits 28595 30603 +2008
- Misses 5173 5231 +58
|
|
@psychon what do we do with this? |
|
I forgot about this one! If there's anything you need me to change let me know. |
Apparently, everybody forgot about this one ;). Fact is, none of us had much time in H2 2018. |
lib/gears/color.lua
Outdated
| -- @param b Ending color point | ||
| -- @treturn table 4 values representing color in RGBA format (each of them in | ||
| -- [0, 1] range) or nil if input is incorrect. | ||
| function color.interpolate(val, a, b) |
There was a problem hiding this comment.
i would name it mix (as for example in scss)
Veratil
force-pushed
the
new-gears-funcs
branch
from
7dd846f to
c3ffbcc
Compare
|
@actionless Alright! I've updated the |
themes/gtk/theme.lua
Outdated
| ratio = ratio or 50 | ||
| return darker(color, is_dark(color) and -ratio or ratio) | ||
| ratio = (ratio or 50) / 100 | ||
| return gcolor.darken(color, is_dark(color) and ratio or 0) |
There was a problem hiding this comment.
it was before -ratio or ratio but after your change ratio or 0
There was a problem hiding this comment.
Ah, I was thinking if it was dark then don't do anything, I guess if it's dark then the original darker made it lighter?
There was a problem hiding this comment.
as you can see in implementation of local darker(), it takes negative value to make color lighter
themes/xresources/theme.lua
Outdated
| theme = theme_assets.recolor_titlebar( | ||
| theme, theme.fg_normal, "normal" | ||
| ) | ||
| theme = theme_assets.recolor_titlebar( | ||
| theme, darker(theme.fg_normal, -60), "normal", "hover" | ||
| theme, gcolor.darken(theme.fg_normal, 0.6), "normal", "hover" |
There was a problem hiding this comment.
here and next i see you swapped direction from making lighter color into making darker one
There was a problem hiding this comment.
gears.color.darken takes a positive percentage to reduce by, not a negative (just like scss does).
There was a problem hiding this comment.
Percentage-wise, if we take -60 and convert it to a percent of the 0-255 range, we'd get 0.235, so I could replace it with that?
There was a problem hiding this comment.
in that case please replace it to lighten(60/255)
themes/xresources/theme.lua
Outdated
| theme = theme_assets.recolor_titlebar( | ||
| theme, theme.fg_normal, "normal" | ||
| ) | ||
| theme = theme_assets.recolor_titlebar( | ||
| theme, darker(theme.fg_normal, -60), "normal", "hover" | ||
| theme, gcolor.lighten(theme.fg_normal, (60 / 255)), "normal", "hover" |
There was a problem hiding this comment.
also, lighten returns array of RGBA channels, while everywhere it's expected in a different format
given this particular example, theme_assets.recolor_titlebar is passing color value to gears.color.recolor_image which is passing at the end to gears.color.create_pattern
There was a problem hiding this comment.
D'oh! You're right. It was obviously a late night for me. Have to stringify the values returned.
spec/gears/color_spec.lua
Outdated
| describe("mix 100%", function() | ||
| for _,col in ipairs(colors) do | ||
| it(col[1], function() | ||
| assert.is.same(col[1] .. "ff", color.stringify({color.mix(col[1], col[5], 1)}, true, 8)) |
There was a problem hiding this comment.
does this repetition of stringify every time looks fine for you? i would suggest having color.hex_mix , hex_darker and other similar proxies or find some other solution to have more usable output from those new functions
There was a problem hiding this comment.
That's not a bad idea.
I was actually thinking about creating a complete Color object which we could use similar to how SCSS is put together. I noticed in the GTK helper that the color was accessible through gtk_color.red, .green, .blue, and .alpha. That is my idea for a good object including the additional HSL code. Ideally I wanted to be able to do something like color:lighten(0.25) and do all the magic behind the scenes, afterwards doing something like color:str() to get the stringify'd output.
There was a problem hiding this comment.
i like the idea with object, @Elv13 do you have any opinion on this?
There was a problem hiding this comment.
So, similar to gears.matrix, I guess.
There was a problem hiding this comment.
btw i think :hex() would be better than :str() since it's more explicit
There was a problem hiding this comment.
Ideally everything will be done within the Color object itself.
I sort of have it set up like this right now (we can rename output functions later):
Color = (require 'gears.color').Color
c = Color.new("#ffcc99")
print(c:as_str()) -- prints "#ffcc99"
print(c:as_hex()) -- prints "4291598847"
print(c.red) -- prints 1
print(c.green) -- prints 0.8
print(c.blue) -- prints 0.6
print(c.hue) -- prints 30.0
print(c.saturation) -- prints 1.0
print(c.lightness) -- prints 0.8
print(c.opacity) -- prints 1
c2 = Color.new("#99ccff")
print(c1:mix(c2)) -- defaults to 0.5 mix, half and half; prints #ccccccff
print(c1:mix(c2, 0)) -- prints #ffcc99ff
print(c1:mix(c2, 1)) -- prints #99ccffffYou can do a function on the object itself and it will return a new color (unless we want to make something like c1:mix(c2) destroy the old color). So we'd have color:lighten, color:darken, etc.
The other PR I'll make (unless I just wipe this one and rename it) will essentially replace this one if we want to go that route.
There was a problem hiding this comment.
i would suggest implementing object right in this PR
especially because release is soon and if we merge one API and next will replace it with another API that will mean deprecating old API and providing backward compatibility
There was a problem hiding this comment.
and regarding naming, you could just google "hex colors" and observe what it's common name of "#0088ff" representation
There was a problem hiding this comment.
other than points above : i really like color object idea and appreciate the time you investing into making it happen 👍
There was a problem hiding this comment.
Alright! Sounds good! I'll do it here. :)
Veratil
changed the title
|
I have a few more things I want to add to the Color.String parsing (you can see the commented out lines in |
|
Oh yeah, I'll fix up the spacing. I'll make everything lowercase, too. I was trying to think of a good way to differentiate the |
|
Good question. |
|
|
That may work.
I feel like
Any other ideas? |
Veratil
changed the title
|
i think using the same name for module and its submodule will end up very confusing when playing with it |
You're probably right. I wonder if we reorganize a bit and put the current EDIT: Dunno why I hit comment early... then we rename |
| -- @return A new color object, or nil if invalid | ||
| -- @usage local c = colorful.color.from.hex3(0xffcc99) | ||
| function color.from.hex3(hex) | ||
| return color.from.hex4(lshift(hex, 8) + 0xff) |
There was a problem hiding this comment.
Uhm... this comment is on a random place, but what happens when I specify a color with more than 50% red as a hex number with Lua 5.3 on a 32 bit system? In this case, the number would be stored in an int and we get an integer overflow. I'm not quite sure what would happen next, but I would guess "not the right thing".
I'm not quite sure what to do about this. The only option that I see is: hex3 (in the current implementation) and hex4 (in general) cannot work everywhere.
(Sigh, I'd really like to stay outside of the realm of worrying about integer overflows...)
There was a problem hiding this comment.
If you passed in 0x80ffff then that's 24 bits right? 0x80ffff << 8 == 0x80ffff00, 0x80ffff00 + 0xff == 0x80ffffff. I don't see overflow?
Unless I'm forgetting something, I think that's right?
There was a problem hiding this comment.
Those 24 bits get turned into color.from.hex4(0x80ffffff). Gdb says that's -2130706433 (oh! not only 32 bit Linux has 32 bit int, but 64 bit has, too!). So, the overflow will be in the call to lshift.
There was a problem hiding this comment.
If an integer overflow happens, does it throw a signal that awesome catches or what? How does Lua handle that?
If Lua ignores and just returns the negative number, then it should be okay since the bits are all we're looking at and those should be okay. We're not worried about the whole number itself as we do more bit shifting afterwards to get the values.
There was a problem hiding this comment.
There is no signal. Also, I am not aware of any guarantees that Lua makes. In C, integer overflows are undefined behaviour, meaning that anything can happen when one occurs. There are examples "out there in the internet" were the compiler optimised away some security check due to an integer overflow, i.e. opened up an exploitable security issue. Since Lua is implemented in C, I guess that this also means that integer overflows are undefined behaviour, but there is way less opportunity for the compiler to wreck havoc here...
Also, are you sure that we are okay since this code just looks at the bit? Shifting right by r bits is implemented by division through 2^r. This does not work for negative numbers since the result will still be negative. I vaguely remember some comment in this code that assumed that shifting right results in zeros in the high bits...
I cannot say that anything is definitely wrong, but I am just feeling uneasy about these bit operations.
There was a problem hiding this comment.
I think you're right. The division will keep the high bits.
If we keep this (which I would like to do) then we should put a large warning sign for these functions then when using on 32bit systems.
There was a problem hiding this comment.
And the reason it uses division is because that's how Lua 5.2 defined the operation in their docs.
There was a problem hiding this comment.
The "only on 32 bit systems" was wrong by me, sorry.
https://en.wikipedia.org/wiki/64-bit_computing#64-bit_data_models
int is 32 bit basically everywhere.
However... I was also wrong in another point. Lua 5.3 seems to default to using long long for integer values, not int. According to Wikipedia again, this has at least 64 bits, so all I was saying is moot. Sorry for the noise.
https://en.wikipedia.org/wiki/C_data_types
There was a problem hiding this comment.
Looking through the source code for Lua 5.1, it looks like the default type for LUA_NUMBER is double. Do you think we're safe with that?
There was a problem hiding this comment.
Everything but Lua 5.3 uses double. If I'm not mis-reading Wikipedia, a double has a 52 bit mantissa. That should be quite enough. :-)
|
|
||
| --- Update the RGB values after editing any of the HSL values | ||
| function color:rgb_update() | ||
| self.red, self.green, self.blue = hsl_to_rgb(self.hue, self.saturation, self.lightness) |
There was a problem hiding this comment.
Wait, what? People are meant to just do object.red = 42 and to then call hsl_update? This is not documented anywhere. How about having this immutable instead and no modifications allowed? I think that would be a better idea (e.g. a caller passes in a color object, you modify it for internal reasons and now the caller also has another color; also, all the other functions like mix return new objects).
My suggestion would be to remove these two functions and forbid changing the color after it was created (e.g. by just having the docs say so).
There was a problem hiding this comment.
I stuck this in there thinking about the ability to change the object directly. I wanted to have it update automagically, but I couldn't figure out the metamethods to do it right. I was hoping to get feedback on it. I wasn't sure if we want to keep the color immutable or allow changes (which would update the other colorspace fields automagically).
| local red = clamp(1 - self.red, 0, 1) * 255 | ||
| local green = clamp(1 - self.green, 0, 1) * 255 | ||
| local blue = clamp(1 - self.blue, 0, 1) * 255 | ||
| return color.from.rgba(red, green, blue, self.alpha * 255) |
There was a problem hiding this comment.
Why the clamp? If this actually changes anything, then the passed-in color was invalid. All the color-creation functions seem to be checking for that and clamping happens there.
There was a problem hiding this comment.
So this was in case somehow the red/green/blue values were updated to be outside [0,1]. Since the fields can be edited directly (right now), this was to prevent invalid colors from being created.
| + lshift(round(self.green * 0xFF), 16) | ||
| + lshift(round(self.blue * 0xFF), 8) | ||
| + round(self.alpha * 0xFF) | ||
| ) |
There was a problem hiding this comment.
This is also one of the functions which could have problems with 32 bit signed integers... I'm not sure.
There was a problem hiding this comment.
You might be right.
| function colorful.mix(color1, color2, amount) | ||
| local c1 = color.new(color1) | ||
| local c2 = color.new(color2) | ||
| if c1 == nil or c2 == nil then return nil end |
There was a problem hiding this comment.
Would it make sense to have this cause errors instead of returning nil? nils might mean that some background color is just silently ignored, which is a bit more confusing than "I get an error with a traceback".
There was a problem hiding this comment.
To me I'd like it to just return nil as it shows something went wrong and the user can catch that and use a fallback or error on their own if it's important enough. But if you think error'ing is better here then that's fine.
|
Bump for attention. Not sure why Travis never worked. |
|
From a quick look (assuming I already took a better look before): Looks basically fine to me. 👍 Could you rebase this on master to get codecov to work (so one can look at the coverage of this) and to incorporate the "Travis rename"? |
There was a problem hiding this comment.
Well, sorry for the unacceptable delays. It finally reach the top of the "next thing to do before we release v4.4" stack now that the notification stuff is finally on track.
I started working on an updated version and will submit a pull request to this pull request when I am done (yes, GitHub supports that). Given the number of changes, I might as well do it instead of just complaining an induce another million review cycles. So far, the following changes are being implemented:
- Add
:to_in front of the methods that return a new color object to clarify that they are immutable. - Remove the redundant functions that have identical methods. Lua already support the
mymodule.method(self)syntax, adding wrappers on top just to do it dupllicates the API. So I mergedbeautiful.colorfulandbeautiful.colorful.colornamespace. This is a against some comments above, but the duplication was a problem. - Add a
beautiful.colorfultemplate to thetests/examplesframework - Remove the hardcoded examples from the public doc, unit test them, add images and insert them back into the doc.
- Enforce the line length convention used elsewhere in the codebase.
- Fix the documentation so it renders properly with both markdown backends, otherwise some users who compile Awesome will suffer a broken doc generation [2]. That include limiting the method summary to 78 chars, enforcing the mandatory
.at the end of the method/function/param/return lines and making sure there is always en empty line before (and after) code block starting with 4 spaces. - Change the method parameter names to be more explicit [1]
- Add documentation for the properties of the color object which are mentioned in the example but never formally documented.
- Add an empty line before comments, before
if, before loops and after blocks of variables initialization to make the code a bit faster to read/parse/glance. - Make the spacing between the multi-variable declaration consistent (there's a mix of
r,g,bandr, g, bstyle) - Unify the styling of
#rrggbbvs.#RRGGBBin the documentation and make sure all of them have their ` quote marks.
Otherwise I am fine with the design and code of the module. My own blind.pattern (link) and blind.pixmap (link) have nearly identical daisy-chained immutable design. It works very well for such task. If I could eventually find the time to modernize and complete my old module, along with beautiful.colorful, it would make an awesome theme API!
[1] (yes, the API has hundreds of c for client, s for screen and t for tag, but that's because of naming conflicts/shadowing with the global capi)
[2] There's no "real" way to enforce this on the CI given some HTML is generated, but is rendering unreadable/invisible garbage.
|
I through this would be merged long ago. 😮 What is the status? |
From a quick look: I asked for a rebase to master half a year ago and then Emmanual wrote
So, this PR is now waiting on either Emmanual or @Veratil to do something (but I guess it is mostly blocked by Emmanual and @Veratil cannot do much). |
Don't know how far you are with this, @Elv13 but I have a very basic one that's working for me... I can PR whichever but just don't want all the effort to go to waste. |
Please do so, I am ashamed of how much time I cannot find to finish this. Right now I have literally no time at all. I am already way, way late on this and this PR is a very nice contribution to AwesomeWM. |
Let me know if you can't make a PR against my fork and I'll try to adjust permissions. I've been pretty busy with new job, and unfortunately not using Awesome since I can't get it to play well with XFCE at work, and just don't have the time to work on it yet. |
Awesome, I'll try to do a PR to the branch later this week, @Veratil. |
Add two new gears.color functions:interpolatestringifyAdd one new gears.math function:roundf*~~ sign~~
I've used an interpolation function for a while now to calculate colors where a pattern isn't really viable. I figured this might be useful in gears.color as well. Additionally, I convert those colors to strings for markup, so I figured a stringify function would be nice to have as well.I useroundffor my vicious.widgets.net to show a rounded value to 1 decimal point, I pulled it from lua-users.org's SimpleRound. I figured it would be useful in gears.math alongsideround.Updated PR description