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

withfaces doesn't apply to constructed StyledStrings #87

Open
topolarity opened this issue Sep 4, 2024 · 3 comments · May be fixed by #99
Open

withfaces doesn't apply to constructed StyledStrings #87

topolarity opened this issue Sep 4, 2024 · 3 comments · May be fixed by #99

Comments

@topolarity
Copy link
Member

topolarity commented Sep 4, 2024

I'd have expected styling to be applied at string construction, so that:

const alert_msg = withfaces(:banner => Face(foreground = :red)) do
    styled"{banner:text}"
end
println(alert_msg)

prints "text" in red. Instead, it prints without styling.

IMO this is a subtle but important difference between what I'd expect to be a StyledString vs. an AnnotatedString.

If you want to carry around style information with your string (I almost always do), I have to shuttle around the Faces I need to the presentation layer somehow - which is especially unclear how to do if I wanted to do AbstractString-style operations on this in a way that preserves styling.

@tecosaur
Copy link
Collaborator

tecosaur commented Sep 4, 2024

Mmmm, from this I take it that some of the nuances with the system could be better explained in the docs. I can see how one might imagine withfaces to remap named faces seen within it. This reminds me of an extra utility that has come to my mind: a function like freezefaces(::AnnotatedString) which resolves all named faces into standalone Face objects.

Let's add this to the discussion we've lined up. Just so it's out of Slack, I'll also reproduce the list of benefits/annoyances of the current system I wrote and you suggested edits to.

Key points I like with the current system:

  • Straightforward user-customisation of all named faces
  • Composability across packages
  • The ability to remap faces in certain contexts
  • Being able to weakly depend on other package's faces
  • Concise usage

Key points of annoyance with the current system:

  • Global mutable state
  • Requires __init__-y stuff
  • Uses a TOML parser

@topolarity
Copy link
Member Author

Sounds good to me!

FWIW I would naively have expected annotated"..." to behave like in the OP but styled"..." to resolve eagerly, and then for styled(::AnnotatedString) to behave in the way you describe for freezefaces

@topolarity
Copy link
Member Author

topolarity commented Sep 4, 2024

And here's a gist sketching the "palette" idea that I brought up on Slack: https://gist.github.com/topolarity/f4e422c70441a002af88510750788048

The main idea here is to go from a single-level namespace to a two-level namespace, w/ more explicit inheritance - The goal is to make it easier to answer questions like:

  • "Should I be prefixing all my Face names?"
    • not necessary (without palettes, it's almost certainly best practice - it's the emacs tradition for example)
  • "How can I reasonably expect this to display?"
    • a Palette gives a place to document the "semantic" meaning of a Face, esp. for customizations (e.g. light v. dark assumptions, or "whole theme" assumptions like :title_bold should be readable inside of :title, :emphasis can never be used in a :title, etc.). For picky users that just want reliable styling without theming/customizations - they can opt-out via mutable = false and/or providing no toml_name
  • "What is everything I need to customize to provide a 'theme'?"
    • the set of Faces in a Palette is fixed, just enumerate its contents

topolarity added a commit that referenced this issue Oct 15, 2024
This is step #1 to eliminating our type-piracy problems

StyledStrings needs to put some type that it owns into its AnnotatedStrings
so that we have a right to hook into the display logic (and the display
logic can know which copy of StyledStrings to delegate to.

It is also a semantic change to how constructing StyledStrings
behaves, but overall I think it's a lot more intuitive to have a
StyledString actually compute its style information at construction

Resolves #87
topolarity added a commit that referenced this issue Oct 15, 2024
This is step 1 to eliminating our type-piracy problems

StyledStrings needs to put some type that it owns into its AnnotatedStrings
so that we have a right to hook into the display logic (and the display
logic can know which copy of StyledStrings to delegate to.

It is also a semantic change to how constructing StyledStrings
behaves, but overall I think it's a lot more intuitive to have a
StyledString actually compute its style information at construction

Resolves #87
topolarity added a commit that referenced this issue Oct 15, 2024
This is step 1 to eliminating our type-piracy problems

StyledStrings needs to put some type that it owns into its AnnotatedStrings
so that we have a right to hook into the display logic (and the display
logic can know which copy of StyledStrings to delegate to).

It is also a semantic change to how constructing StyledStrings
behaves, but overall I think it's a lot more intuitive to have a
StyledString actually compute its style information at construction

Resolves #87
@topolarity topolarity linked a pull request Oct 15, 2024 that will close this issue
tecosaur pushed a commit that referenced this issue Oct 16, 2024
This is step 1 to eliminating our type-piracy problems

StyledStrings needs to put some type that it owns into its AnnotatedStrings
so that we have a right to hook into the display logic (and the display
logic can know which copy of StyledStrings to delegate to).

It is also a semantic change to how constructing StyledStrings
behaves, but overall I think it's a lot more intuitive to have a
StyledString actually compute its style information at construction

Resolves #87
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 a pull request may close this issue.

2 participants