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

Support conversion to a StyledStrings.SimpleColor #293

Merged
merged 1 commit into from
Oct 30, 2023

Conversation

tecosaur
Copy link
Contributor

This enables easy interoperability with the Face constructor of StyledStrings.

With this, it's easy to do fun things like:
image

Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

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

LGTM, with one small tweak.

Project.toml Outdated Show resolved Hide resolved
This enables easy interoperability with the Face constructor of
StyledStrings.
@timholy timholy merged commit 808a60f into JuliaGraphics:master Oct 30, 2023
@timholy
Copy link
Member

timholy commented Oct 30, 2023

Thanks!

stillyslalom pushed a commit to stillyslalom/ColorTypes.jl that referenced this pull request Feb 6, 2024
This enables easy interoperability with the Face constructor of
StyledStrings.
@kimikage
Copy link
Collaborator

kimikage commented Apr 3, 2024

I have no intention to oppose this PR, but I think the color parsing this kind of conversion should be in Colors.jl.
I saw a possible discussion regarding this on Colors.jl.
JuliaGraphics/Colors.jl#514
JuliaGraphics/Colors.jl#473

@tecosaur
Copy link
Contributor Author

tecosaur commented Apr 3, 2024

There's no colour parsing going on though?

@kimikage
Copy link
Collaborator

kimikage commented Apr 3, 2024

I used "parsing" in an abstract sense. Sorry for the confusion.
This is a kind of "notation" conversion, not a color space or color model conversion.
Considering that Colors.jl is responsible for conversion from more basic string types, I personally think that conversion to types that do not come under Colorant should be outside of ColorTypes.jl.

johnnychen94 pushed a commit that referenced this pull request Apr 6, 2024
This enables easy interoperability with the Face constructor of
StyledStrings.
johnnychen94 added a commit that referenced this pull request Apr 6, 2024
@johnnychen94
Copy link
Member

johnnychen94 commented Apr 6, 2024

Keeping ColorTypes as small as possible is a good comment; I've seen people thinking 0.2s loading time is too large (#270)

From the engineering and maintenance-ship part, if some codes already sit here and there's no "obvious" impact on the user experiences, there's no need to take the risk for the re-factoring unless someone is willing to respond to whatever happens during the transferring. Since this PR has only been merged and has not yet been released, I'm okay with either ColorTypes or Colors if anyone wants to make the change.

The real problem with this PR is that StyledStrings is a stdlib package in Julia 1.11+, and this makes it impossible for ColorTypes to register a new version if we want to maintain backward compatibility (5eea76d#comments).

@tecosaur I'm not sure what the best solution to this is. Maybe StyledStrings should be registered in the General as well?

@tecosaur
Copy link
Contributor Author

tecosaur commented Apr 6, 2024

@kimikage the way I see it, support for using Colorants in StyledStrings should either be in the package that defines Colorant (this one, ColorTypes), or StyledStrings.

Doing it in StyledStrings isn't really an option, since it's a stdlib, and since the required code is pretty minimal I'm not sure there's enough of a "cost" to having it here to justify worrying about this any more than the basic thought process I've just mentioned.


@johnnychen94 Keeping ColorTypes small and focused is indeed good, but given the simplicity of this specialisation, I'd be amazed to hear if this has any appreciable effect on codeimage size, or load time. Seperately, thinking of a "where does this functionality best sit", other than the usual "not doing type piracy is good", Colors.jl seems a lot more complex than this functionality is.

The real problem with this PR is that StyledStrings is a stdlib package in Julia 1.11+, and this makes it impossible for ColorTypes to register a new version if we want to maintain backward compatibility.

This shouldn't be a problem for much longer. StyledStrings has a julia1-compat branch that's going to be registered in General as soon as the version that's going to come with 1.11 and the implementation of the Base.Abstract{String,Char,IOBuffer} types is settled. See JuliaLang/StyledStrings.jl#5.

I think we're down to a single blocker in that regard, and that's waiting for SubString{AnnotatedString} equality to be worked out: JuliaLang/julia#53042.

@kimikage
Copy link
Collaborator

kimikage commented Apr 6, 2024

I still think Colors.jl is a more appropriate place than ColorTypes.jl (of course than StyledStrings.jl too), but is there another option, e.g. via a more unambiguous type such as Tuple?
In that case, it becomes difficult to use color models other than RGB. Note, however, that the actual implementation of the color conversions is in Colors.jl.

@tecosaur
Copy link
Contributor Author

tecosaur commented Apr 7, 2024

is there another option, e.g. via a more unambiguous type such as Tuple?

I can't say I actually know what you mean by this.

@kimikage
Copy link
Collaborator

kimikage commented Apr 7, 2024

Here is the code for the concept (which is by no means practical)

# ColorTypes.jl side
using ColorTypes
Base.Tuple(c::Colorant{T, 3}) where {T} = (comp1(c), comp2(c), comp3(c))::NTuple{3, T}

(cf. PR #260)

# StyledStrings.jl side
using StyledStrings
Base.convert(::Type{StyledStrings.SimpleColor}, c) = StyledStrings.SimpleColor((round.(Int, Tuple(c) .* 255))...)
Base.convert(::Type{StyledStrings.SimpleColor}, c::StyledStrings.SimpleColor) = c
julia> col = RGB(1, 0.5, 0)
RGB{Float64}(1.0,0.5,0.0)

julia> styled"{(fg=$col):colored}"
"colored"

@tecosaur
Copy link
Contributor Author

tecosaur commented Apr 7, 2024

Hmmm, this seems built on too many assumptions for me to feel comfortable with it at a glance.

@kimikage
Copy link
Collaborator

kimikage commented Apr 8, 2024

I understand your concern. However, are you making any assumptions?

julia> using ColorTypes, StyledStrings

julia> hsv = HSV(300, 0.4, 0.5)
HSV{Float64}(300.0,0.4,0.5)

julia> styled"{(fg=$hsv):Color conversion is implemented in Colors.jl}"
ERROR: No conversion of HSV{Float64}(300.0,0.4,0.5) to RGB24 has been defined

If this PR is for package developers other than stdlib, they should know what type of colors they are dealing with.
If colors are given from an external source, such as an end user or another package, the package should require Colors.jl to avoid such errors.

@kimikage
Copy link
Collaborator

kimikage commented Apr 8, 2024

Also, if in the future julia users request support for the X11 or CSS color specifications, should we add another extension to Colors.jl as well? Of course, having extensions is not so bad in itself.

@kimikage
Copy link
Collaborator

kimikage commented Apr 8, 2024

Since this PR has already been merged, this discussion is somewhat ”closed" in terms of discoverability in GitHub.
If the discussion is to continue, it should be on a new issue or Discourse.

@tecosaur
Copy link
Contributor Author

tecosaur commented Apr 8, 2024

If the discussion is to continue, it should be on a new issue or Discourse.

Frankly, I'm not sure there's much value in continuing it 😅. I still think #293 (comment) sums up the situation well.

I understand your concern. However, are you making any assumptions?

To me, this is behaving exactly as expected, with ColorTypes.jl and StyledString.jl loaded, we see that a non-RGB colour can be used with StyledStrings if conversion methods are implemented. Package authors would need Colors.jl if they wanted to convert colour spaces before, and the situation is unchanged.

Also, if in the future julia users request support for the X11 or CSS color specifications, should we add another extension to Colors.jl as well? Of course, having extensions is not so bad in itself.

I don't really follow, are you thinking of a future where somehow X11/CSS named colour specifications get integrated with the Julia stdlib via some dedicated type? This seems like rather extreme speculation.

@kimikage
Copy link
Collaborator

kimikage commented Apr 8, 2024

JuliaLang/StyledStrings.jl#33 is not the issue you filed?
Of course, we can still extend StyledStringsExt to support alpha!

That's all! Thank you for listening to my gripe.

(BTW, wouldn't it be better if the extension name was globally unique?
Edit: to distinguish from the future ColorsStyledStringsExt? 😝)

@tecosaur
Copy link
Contributor Author

tecosaur commented Apr 8, 2024

JuliaLang/StyledStrings.jl#33 is not the issue you filed?

Yep, it just (1) wouldn't need any changes to StyledStringsExt (SimpleColor is public API) (2) likely won't involve any changes to SimpleColor anyway (going with the method of adding an alpha channel to Face).

That's all! Thank you for listening to my gripe.

I don't mind a bit of griping 🙂

BTW, wouldn't it be better if the extension name was globally unique?

I don't see why...

@kimikage
Copy link
Collaborator

xref: #299 (comment)

This PR on its own causes problems with "Project.toml", so I labeled it breaking. (cf. #302)

@tecosaur
Copy link
Contributor Author

tecosaur commented May 6, 2024

I don't think this is breaking?

@kimikage
Copy link
Collaborator

kimikage commented May 6, 2024

If you have any objections, please comment at #299.

@kimikage kimikage removed the breaking label May 6, 2024
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

Successfully merging this pull request may close these issues.

4 participants