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

Unified named colors in RichTextLabel #35505

Merged
merged 1 commit into from
Jan 8, 2021

Conversation

dalexeev
Copy link
Member

Now the BB tag [color] uses the color names from the Color class.
Also, the use of the # symbol in an HTML color code has become optional.


Please note that there is backward incompatibility with several old color names.

200124-1

@bojidar-bg
Copy link
Contributor

Maybe, to lessen the compatibility breakage, grey and navy could be added to the list of Colors in Color?

@dalexeev
Copy link
Member Author

Should the grey color be #bfbfbf or #808080? If gray and grey are different, it will be strange. If grey is #bfbfbf, then compatibility will still be broken.

@bojidar-bg
Copy link
Contributor

Well, hm, didn't consider that. Feel free to leave it as is, in this case.

@dalexeev
Copy link
Member Author

I watched https://en.wikipedia.org/wiki/X11_color_names and noticed the 'Alternatives' column in the colors table:
200520-1

I'm not sure if we really need this, but in any case, this is a matter of a separate PR.

@dalexeev

This comment has been minimized.

@dalexeev dalexeev force-pushed the rtl_colors branch 2 times, most recently from b7827a0 to 6ce7fe8 Compare November 16, 2020 16:32
@dalexeev
Copy link
Member Author

Rebased and ready to merge.

Now the BB tag `[color]` uses the color names from the `Color` class.
Also, the use of the `#` symbol in an HTML color code has become optional.
Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

I tested this, throwing everything I could think of at it (3, 4, 6, 8 digit hex colors, with and without #, and named), and it works perfectly.

Additionally, I love the implementation. I believe the idea was to replace ColorN and from HTML with a constructor that accepts a string, and Color::from_string works perfectly for this. Although that can be done in another PR, since it would also involve removing ColorN which would be a big change.

@dalexeev
Copy link
Member Author

dalexeev commented Dec 6, 2020

To be honest, it seems to me too that we have too many color constructors from a string: Color::html, Color::named, and now also Color::from_string. In GDScript, Color(string) uses Color::html.

It seems to me that it would be more appropriate if in GDScript Color(string) would use Color::from_string instead. In this case, it will be possible to use both Color("#ff0000") and Color("red"). Then ColorN("red") won't really be needed.

Also, I had a question if the methods find_named_color, get_named_color_count, get_named_color_name, get_named_color should be made private, but I decided not to touch that. First of all, I wanted to fix the inconsistency in RichTextLabel, and I made changes to the core only when necessary. For other changes, we probably need a separate PR.

@akien-mga
Copy link
Member

akien-mga commented Dec 6, 2020

It seems to me that it would be more appropriate if in GDScript Color(string) would use Color::from_string instead. In this case, it will be possible to use both Color("#ff0000") and Color("red"). Then ColorN("red") won't really be needed.

That's what I planned to implement after a recent discussion with @reduz. But if you want to do it in a follow up PR, that'd be very welcome :)

@akien-mga akien-mga merged commit 9349a55 into godotengine:master Jan 8, 2021
@akien-mga
Copy link
Member

Thanks!

@dalexeev dalexeev deleted the rtl_colors branch January 8, 2021 08:13
@akien-mga
Copy link
Member

Welp, should have rebased before merging as there's some more uses of _get_color_from_string to convert.

akien-mga added a commit to akien-mga/godot that referenced this pull request Jan 8, 2021
Should have rebased before merging.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants