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 color option for img bbcode tag in RichTextLabel to tint images #39113

Merged
merged 1 commit into from
Jun 3, 2020

Conversation

pouleyKetchoupp
Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp commented May 28, 2020

edit: changed to add a color option in img tag instead of a new tag img-color.

Allows RichTextLabel to display an image with tint using a new option color in img tag.

Allows RichTextLabel to display an image with tint using a new bbcode tag img-color.

It works the same way as the color tag, but only affects images. It was added as a separate tag to keep the expected behavior that color only affects the font color and images are not affected by color changes by default.

bbcode = Text with icon [img width=16]res://icon.png[/img]

bbcode = Text with icon[img width=16 color=#00FF00]res://icon.png[/img]

@akien-mga
Copy link
Member

Maybe this could be an optional parameter in [img] instead of a new tag?

@Calinou
Copy link
Member

Calinou commented May 28, 2020

If breaking compatibility isn't an issue, we could make [color] affect images instead. I guess the concern was to have something cherry-pickable to the 3.2 branch, so we could have a separate PR with a dedicated tag for that branch.

@groud
Copy link
Member

groud commented May 28, 2020

we could make [color] affect images instead.

I think this would be annoying to have to close a color tag just to have your icon to be displayed with the original colors. As it is kind of a corner case, I think I like @akien-mga's solution better.

@pouleyKetchoupp
Copy link
Contributor Author

Thanks all for the feedback!

@Calinou If possible, I would rather not break compatibility and keep the two branches in sync, but my main concern is that I think intuitively you would not expect the font color to affect images too.

I agree the idea proposed by @akien-mga to have an optional parameter instead of a dedicated tag seems like a good compromise.

The syntax for images with color could be:
[img color=<color>]{path}[/img]
[img=<width> color=<color>]{path}[/img]
[img=<width>x<height> color=<color>]{path}[/img]
And it would be just a Color parameter added to image items, so the code changes would be much simpler than what I did so far.

@akien-mga
Copy link
Member

akien-mga commented May 28, 2020

I'd say the size parameter should probably be made explicit too as [img=<width>] is a bit weird currently. So:

[img size=<width> color=<color>]{path}[/img]
[img size=<width>x<height> color=<color>]{path}[/img]

Edit: Well looking at https://docs.godotengine.org/en/stable/tutorials/gui/bbcode_in_richtextlabel.html currently we do have various tags that use [tag=<value>], so I guess [img=<width>] made sense in this context... but what bothers me is that <width> is not a proper value for img, I'd expect more something like [img=<path> size=<width>]...

I think there's an opportunity for cleaning up the BBcode tags and make them more consistent and standard for 4.0.

But in the meantime if we want a simple change that doesn't break compat, just adding the optional color parameter and keeping [img=<width>] makes sense (then it should be compatible with 3.2).

@dalexeev
Copy link
Member

dalexeev commented May 28, 2020

Maybe [tint={color}][img]{path}[/img][/tint] or [modulate={color}][img]{path}[/img][/modulate]?

This is UNIX BBCode way! 😄

@pouleyKetchoupp
Copy link
Contributor Author

@akien-mga Yes, I would rather keep this PR simple for now and have more involved syntax changes separated.

About changing the syntax:
The way image path & size is handled seems to follow some standard bbcode syntax:
https://www.bbcode.org/reference.php

So if we want to keep our syntax consistent with this reference we could add this version as an alternative to set the size:
[img width=<width> height=<height>]{path}[/img]

That said, we could use a custom syntax, but I don't know if it should be called bbcode if it's too different from the original version, to make sure it's not misleading to users.

@Calinou Calinou added this to the 4.0 milestone May 28, 2020
@akien-mga
Copy link
Member

I think we should aim to stay BBcode compatible as far as possible yeah, even though we do have extra tags that BBcode doesn't have (so it's more like "Godot Extended BBcode").

@pouleyKetchoupp pouleyKetchoupp changed the title Add img-color bbcode tag in RichTextLabel to tint images Add color option for img bbcode tag in RichTextLabel to tint images May 29, 2020
@pouleyKetchoupp
Copy link
Contributor Author

Just pushed the changed to make color an option in img tag instead of a new tag.

Since I've added easier-to-use code to check for options in tags, I've also added width & height options to img as an alternative and updated the code handling built-in effects to use it too.

@pouleyKetchoupp
Copy link
Contributor Author

I've pushed a little update on comments styling.

@akien-mga akien-mga merged commit 901832e into godotengine:master Jun 3, 2020
@akien-mga
Copy link
Member

Thanks!

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