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

Mo' colors, mo' problems (extended color support?) #14

Open
tehprofessor opened this issue Feb 22, 2020 · 3 comments
Open

Mo' colors, mo' problems (extended color support?) #14

tehprofessor opened this issue Feb 22, 2020 · 3 comments

Comments

@tehprofessor
Copy link

Howdy!

First off I want to say thanks for all your hard work on Ratatouille and ExTermbox! They're both really awesome and useful packages.

Now onto my issue 😈

I was wondering if we this could be relaxed to accept any valid color in the extended color palette instead of only the eight, definitely supported everywhere but limited, colors:

@valid_color_codes Constants.colors() |> Map.values()
@valid_attribute_codes Constants.attributes() |> Map.values()
def to_terminal_color(code)
when is_integer(code) and code in @valid_color_codes do
code
end

I noticed ExTermbox doesn't have any problem with accepting valid integers outside of the eight named colors, but because of that validation Ratatouille does not. I'm imagining something like this would remedy it:

diff --git a/lib/ratatouille/renderer/attributes.ex b/lib/ratatouille/renderer/attributes.ex
index ee1f26b..18403c3 100644
--- a/lib/ratatouille/renderer/attributes.ex
+++ b/lib/ratatouille/renderer/attributes.ex
@@ -6,10 +6,11 @@ defmodule Ratatouille.Renderer.Attributes do
   alias Ratatouille.Constants

   @valid_color_codes Constants.colors() |> Map.values()
+  @valid_color_codes_extended 0x11..0xe8
   @valid_attribute_codes Constants.attributes() |> Map.values()

   def to_terminal_color(code)
-      when is_integer(code) and code in @valid_color_codes do
+  when is_integer(code) and code in @valid_color_codes or @valid_color_codes_extended do
     code
   end

I'd be happy to push a PR if you're interested in this approach, or if you can give some guidance on an approach you'd like (or even why this is all a terrrrrrible idea).

Thank you for your time.

@ndreynolds
Copy link
Owner

Hi @tehprofessor! Sorry for the delay here! Yep, I think this was just an oversight. I haven't needed to venture outside of the 8 colors yet myself in my projects. Your proposed changes to loosen the validations look very reasonable. Happy to merge that if you push up a PR.

Please let me know if you run into any other limitations like this. This sort of feedback is really helpful :)

@qhwa
Copy link

qhwa commented Apr 24, 2020

Hi @tehprofessor , do you have any progress on your PR mentioned above? I just did some little attempts to remove the validations but with no luck. I'm thinking about diving deeper.

@tehprofessor
Copy link
Author

tehprofessor commented Apr 24, 2020

@ndreynolds Sorry the delay on my part! Ha 😄 life comes at you fast. I'll get those changes pushed this weekend, thank you for the reply.

@qhwa removing the validation causes a lot of other breakage if I remember correctly. The above diff was the simplest way I found to accomplish adding the extended colors... 🤘

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

No branches or pull requests

3 participants