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 support for rgb and indexed colors #193

Merged
merged 7 commits into from
Nov 7, 2021
Merged

Conversation

uttarayan21
Copy link
Member

Added support for rgb and indexed colors by using cusom de/serialization of the tui::style::Color struct.

Source here https://git.uttarayan.me/uttarayan/color-parser-tui. I will publish to crates.io after a bit more testing.

Copy link
Member

@grtcdr grtcdr left a comment

Choose a reason for hiding this comment

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

LGTM.

(Renaming theme::theme to theme::base is a very welcome change)

@grtcdr
Copy link
Member

grtcdr commented Nov 4, 2021

Your crate sorta makes theme::color::MacchinaColor obsolete, should we remove ours?

@uttarayan21
Copy link
Member Author

Hmm I haven't really looked at MacchinaColor closely I just Renamed MacchinaColor to Color and fixed the compiler errors, and it just worked.

@uttarayan21
Copy link
Member Author

Actually now that I took a look it seems that MacchinaColor was a wrapper enum to use Color with clap. Now that it's not required, we can probably remove it.

@grtcdr
Copy link
Member

grtcdr commented Nov 4, 2021

Hmm I haven't really looked at MacchinaColor closely I just Renamed MacchinaColor to Color and fixed the compiler errors, and it just worked.

It implements Deserializer and Serializer, that's about it. And your crate implements that as well, so how should we proceed?

@uttarayan21
Copy link
Member Author

There's a make_random_color function closure in main that needs to be manually rewritten I guess. Other than that I can remove it.

@grtcdr
Copy link
Member

grtcdr commented Nov 4, 2021

I've been playing around with strum and strum_macros which provide EnumString (providing from_str()) and EnumIter (providing iter()). The issue with using arg_enum! is it just doesn't allow enumerations that use constructors, which we need for Rgb(u8, u8, u8). For Enum::variants() we can use Enum::iter().collect().

@grtcdr
Copy link
Member

grtcdr commented Nov 4, 2021

Before I forget, this should probably be merged into v6.

@uttarayan21
Copy link
Member Author

uttarayan21 commented Nov 4, 2021

Removed MacchinaColor. Updated color-parser-tui with a module optional which adds support for Option<Color>

Copy link
Member

@grtcdr grtcdr left a comment

Choose a reason for hiding this comment

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

Are we ready to merge to the v6 branch?

I'd rather bundle up our new features into one major release this time.

@uttarayan21
Copy link
Member Author

It's core logic is done. Ill just need to clean up the code a bit, add better docs, and add a few more tests.

I'll probably publish it on saturday after all is done.

@grtcdr
Copy link
Member

grtcdr commented Nov 4, 2021

Take your time, v6 is probably a long way from being ready.

@uttarayan21
Copy link
Member Author

Is the name of the crate alright ?. I was thinking to name it similar to ansi-to-tui, and make it hex-to-tui or color-to-tui.

Any suggestions ?

@grtcdr
Copy link
Member

grtcdr commented Nov 6, 2021

If I were you, I'd name it the way I name most of my crates, in Esperanto. So "koloro" in this case.

@uttarayan21
Copy link
Member Author

I mean if I made a standalone crate I might consider it. But this is just a helper crate of sorts, so I think it might be better to name it descriptive.

@grtcdr
Copy link
Member

grtcdr commented Nov 6, 2021

Then color-to-tui it is. I like that.

(as it does more than just hexadecimal colors)

@grtcdr
Copy link
Member

grtcdr commented Nov 7, 2021

Ready to merge into v6?

@uttarayan21
Copy link
Member Author

Yes. Let me know if something needs to be changed.

@grtcdr
Copy link
Member

grtcdr commented Nov 7, 2021

As far as I can see, everything looks great :)

@grtcdr grtcdr changed the base branch from main to v6 November 7, 2021 06:26
Also published to crates.io
@uttarayan21
Copy link
Member Author

Rebased to v6 branch.

@uttarayan21
Copy link
Member Author

I think we need to show the parsing errors to the users. I can't figure out what is wrong with Hydrogen.toml

@grtcdr
Copy link
Member

grtcdr commented Nov 7, 2021

We really do, in the meantime, can you show me the file?

@uttarayan21
Copy link
Member Author

# for a complete reference, visit
# https://github.com/Macchina-CLI/macchina/wiki/Theme-Documentation

spacing = 2
padding = 0
hide_ascii = false
hide_bar_delimiters = true
prefer_small_ascii = false
separator = ">"
key_color = "#ff6135"
separator_color = "#0ac365"
palette = "Light"

[bar]
glyph = "ߋ"
symbol_open = '['
symbol_close = ']'
visible = true

[box]
title = "aurc"
visible = true

[box.inner_margin]
x = 1
y = 0

[custom_ascii]
# color = "Green"
path = "~/.config/macchina/ascii/archlinux.ascii"

[randomize]
key_color = false
separator_color = false

[keys]
host = "Host"
kernel = "Kernel"
battery = "Battery"
os = "OS"
de = "DE"
wm = "WM"
distro = "Distro"
terminal = "Terminal"
shell = "Shell"
packages = "Packages"
uptime = "Uptime"
memory = "Memory"
machine = "Machine"
local_ip = "Local IP"
backlight = "Brightness"
resolution = "Resolution"
cpu_load = "CPU Load"
cpu = "CPU"

Its this one.
Meanwhile I'll try make another pr to show the parsing error

@grtcdr
Copy link
Member

grtcdr commented Nov 7, 2021

The theme parses and shows up correctly when building the usual macchina/main, but it fails on this specific branch/fork, did we deprecate something and not notice?

EDIT: Nope, no drastic changes that would cause something like this to happen.

Meanwhile I'll try make another pr to show the parsing error

That would help us out a lot.

@uttarayan21
Copy link
Member Author

Oh I forgot to mention, I figured it out.
There were no #[serde(default)] in Block and Bar structs so It failed to derive with missing values.

@grtcdr
Copy link
Member

grtcdr commented Nov 7, 2021

Good catch :)

@uttarayan21
Copy link
Member Author

I actually hadn't pushed the #[serde(default)] changes. I'll merge them in #208

@grtcdr
Copy link
Member

grtcdr commented Nov 7, 2021

I actually hadn't pushed the #[serde(default)] changes. I'll merge them in #208

Woops, alright :)

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

Successfully merging this pull request may close these issues.

None yet

2 participants