Conversation
In preparation for performing a form of interning with `Face`s to make
"faces refer to faces" instead of using symbols, we want to end up with
a single pointer that's passed around for each Face. This can be done by
making `Face` a thin const mutable struct wrapper around an underlying
face type (`_Face`).
We also want to differentiate between "no value provided" and "attribute
explicitly unset". For this purpose we introduce special values that act
as hard and soft nothing-likes. This could be done with two singletons,
and expanding each field to be a `Union{hardnothing, softnothing,
<original type>}` but with the current compiler this produces worse
memory layout/assembly than if we introduce special values and go to the
effort of handling them appropriately, and so that's what I've done.
If we can replace this with union fields and two singletons and not have
to bother with the rest, with the compiler doing just as well as the
manually written code, I'll be overjoyed and happily delete this work.
1340ce0 to
586f3bf
Compare
| # Hook into the AnnotatedDisplay invalidation barrier | ||
|
|
There was a problem hiding this comment.
I guess the change below, where it puts the Face into the type, is the core part of this PR for avoiding the type piracy issue, right?
If so, maybe point that out explicitly here?
| # Hook into the AnnotatedDisplay invalidation barrier | |
| # Hook into the AnnotatedDisplay invalidation barrier. | |
| # Note that to avoid type piracy with Julia Base, it is crucial that we write | |
| # AnnotatedString{<:Any, >:Face} instead of AnnotatedString below, | |
| # and similar for AnnotatedChar. This is because `Face` is "our" type, while | |
| # AnnotatedString and AnnotatedChar belong to Base. |
There was a problem hiding this comment.
Face in the type does do that, but I need to check with Cody/someone compiler-y whether:
>:Faceworks, since that technically encompassesAnnotatedString{<:Any, Any}- There's a way to remove this code and keep styled strings in log messages working
There was a problem hiding this comment.
This is still (a more advanced form of) type-piracy, because it has no pathway to implement this for other types.
If I have my own type Foo and then follow your lead, defining:
Base.AnnotatedDisplay.ansi_write(f::F, io::IO, s::Union{<:AnnotatedString{<:Any, >:Foo}, <:SubString{<:AnnotatedString{<:Any, >:Foo}}}) where {F <: Function} =
_ansi_writer(f, io, s)This creates an ambiguous dispatch for AnnotatedString{String, Union{Foo, Face}}.
It would be consistent to:
- Implement this only for
AnnotatedString{<:Any, Face}(which has no ambiguity / delegation issue), but that will break underAnnotatedStringpromotion. - Implement this purely compositionally (i.e. never dispatch on
AnnotatedString{..., Face}, but that implementation is likely to be significantly more complex)
There was a problem hiding this comment.
FWIW this is also why topolarity/julia@db7e607 opted to "disable" non-concrete types, to avoid being forced to make all the display code fully compositional.
Another approach would be to have separate D (display) and V (value) type parameters (AnnotatedString{S,V,D}) or delegate the printing behavior to an enclosing type (e.g. a StyledString, perhaps?), so that display behavior can be non-compositional while value accumulation remains compositional.
| @@ -0,0 +1,497 @@ | |||
| # This file is a part of Julia. License is MIT: https://julialang.org/license | |||
There was a problem hiding this comment.
I've stopped my review at this point and did not look at anything "below". If there is a specific part where you'd like someone to have a look at, feel free to point it out to me, and I might be able to do it, but there's a lot of new code below and I need to get back to other things.
|
I neglected to add my overall assessment: overall this seems like a good PR in the right direction. Take that with a grain of salt in that I am new to the codebase, obviously @tecosaur is much more qualified than me; but what I mean is that as a relative outsider, the approach taken here seems sensible and "intuitive" enough. There are a few rough corners perhaps were e.g. things like |
| Base.convert(::Type{SimpleColor}, rgb::UInt32) = SimpleColor(rgb) | ||
|
|
||
| function Base.convert(::Type{SimpleColor}, namedcolor::Symbol) | ||
| Base.depwarn("Creating a SimpleColor from a face name Symbol is deprecated as of v1.14. Use faces directly instead, such as from `face\"colourname\"`", :convert) |
There was a problem hiding this comment.
One more point: these deprecation warnings will be a PITA for anyone who enables depwarn=error in their CI (e.g. we do that in Oscar, otherwise it is too easy to miss deprecation warnings which are after all hidden by default when working locally).
It is also makes migration harder if one wants to keep compatibility with both the old and the new system. Two ways to mitigate this come to mind (not mutually exclusive):
- when documenting the deprecation, give concrete examples for code that works on both old and new system w/o deprecations. Say
(Alas this can only be used after this version of StyledString is merged, so can't easily be
@static if VERSION >= 1.14.0-DEV.12345 ... else ... end
tested beforehand) - Delay the deprecation warning, and e.g print it only starting with Julia 1.15-DEV
| Base.depwarn("Creating a SimpleColor from a face name Symbol is deprecated as of v1.14. Use faces directly instead, such as from `face\"colourname\"`", :convert) | |
| @static if VERSION >= 1.15-DEV | |
| Base.depwarn("Creating a SimpleColor from a face name Symbol is deprecated as of v1.14. Use faces directly instead, such as from `face\"colourname\"`", :convert) | |
| end |
There was a problem hiding this comment.
Hmmm, this is a good point. Maybe commenting out the depwarn lines for now and enabling them in 1.15 might be the best course of action.
586f3bf to
907be3c
Compare
42e6d5f to
1e0f0d3
Compare
1e0f0d3 to
e69484e
Compare
Thanks to the `Base.Filesystem.uripath` function introduced in Julia 1.13 (technically private API, but should be fine).
e69484e to
5224fa0
Compare
These changes give
Faces primacy, replacing the use ofSymbols to name faces. This change is capitalised on to address several long-standing pain points with face/style management in StyledStrings.The supplants #99 and #110, and perhaps also #100.
This closes #87, and should also help with #61.
Motivation
The current system has several issues:
AnnotatedStringprintingAnyvalue type in annotationsREPL_History_search_unselected)styled"{bluue:text}"renders unstyled)Faces andSymbols throughout the APISolution
Parameterised annotation values:
AnnotatedString{S}becomesAnnotatedString{S, V}, allowing dispatch on the annotation value type without piracy (see: JuliaLang/julia#60527).Face primacy: Named faces are now referenced by
Faceobjects rather thanSymbols. Theface""macro provides convenient access (face"red"instead of:red). Faces are hybrid-interned: created at compile time, registered at runtime.Palettes: Three new macros for module-scoped face definitions:
@defpalette!- define a set of named faces@registerpalette!- register faces at runtime (in__init__)@usepalettes!- import faces from other modulesFace remapping:
remapfacessubstitutes faces at string construction time, complementing the existingwithfaceswhich operates at display time.Example
Breaking changes
None. The old
Symbol-based API remains supported with deprecation warnings.