Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add colors utilities #121

Merged
merged 5 commits into from
Dec 6, 2023
Merged

Add colors utilities #121

merged 5 commits into from
Dec 6, 2023

Conversation

Athozus
Copy link
Member

@Athozus Athozus commented Nov 5, 2023

This is a rework of mail colors. It is a prerequisite for #102, as the addition of it will add many colors. So, it has the following changes :

  • Remove colors from init.lua
  • Add an utility file /util/colors.lua
  • Only one global function mail.get_color() which returns a color from mixed base colors. It overpasses the mix if there is only one color given.
  • Local function get_base_color(). The inheritor of init.lua. It uses shortcuts of one letter to be simplified and notably for the use of mail.get_color(). Of course, comments to tell what is what.
  • Local functions to convert from hex to rgb and vice-versa.
  • Local function to mix rgb colors.
  • EDIT : and, of course, it replaces all the previous occurrences of mail.colors

An example of use :

  • Old : mail.colors.imp_add_sel
  • New : mail.get_color("ias"), mail.get_color("sai") ...

You can note that the new utility is independent from order.

NB : I wrote the whole code in less than 1 hour, so the review should be quick =)

Local function get_base_color(), conversions hex <=> rgb, rgb color mixer, and global function get_color()
@Athozus Athozus added the Enhancement New feature or request label Nov 5, 2023
@Athozus Athozus added this to the 1.4.0 milestone Nov 5, 2023
Use a single if statement for each property and concatenate to displayed_color then execute mail.get_color(displayed_color) instead of making many combined if statements
Could break the code, the hex convert to rgb always run on 6-chars
@S-S-X
Copy link
Member

S-S-X commented Nov 7, 2023

Didn't yet read all of it but here's what I think about current colors.lua and shortcuts:

I'd suggest either direct math or caching for colors.lua. Also I feel that single letter shortcuts aren't really improvement but instead makes it harder to follow what is actually happening as you have to think about meaning of those codes.

If you like you could implement it with + operator (and others if you like fancy) too so that you can mix colors like colors.header + colors.highlight - colors.blue

@BuckarooBanzay
Copy link
Member

Also I feel that single letter shortcuts aren't really improvement but instead makes it harder to follow what is actually happening as you have to think about meaning of those codes.

☝️

The color.lua code is a bit overkill imo but if you think it is needed 🤷

@Niklp09
Copy link
Member

Niklp09 commented Nov 8, 2023

imo better code: (should be more performant too)

local colors = {
    ["h"] = "colorstr"
}

function mail.get_color(style)
    return color[style]
end

Instead of one-letter symbols, it now supports tables of identifiers or single strings
@Athozus
Copy link
Member Author

Athozus commented Dec 2, 2023

Sorry for being late, I reworked the utility using the following syntax :

mail.get_color("header")
mail.get_color({"selected", "important"})

Note that in the utility itself, the table as suggested by @Niklp09 is called generic_colors to not break colors arguments of some functions.

Hope this one will be ok for you 👍

@Athozus Athozus merged commit 3bad371 into master Dec 6, 2023
@Athozus Athozus deleted the improved_colors branch April 15, 2024 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants