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

Use StyledStrings for REPL prompt styling #51887

Draft
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

caleb-allen
Copy link

@caleb-allen caleb-allen commented Oct 26, 2023

DRAFT/WIP - somewhat functional, code is messy and not quite ready for review.
Needs to be run with --compiled-modules=no until everything is rebased and up-to-date

This PR uses the recently-merged StyledStrings (#49583) features into the REPL prompt styling code. This is my first PR to the Julia codebase, go easy on me!

TODO:

Things to look out for

  • Since the prompt string is no longer restricted to String instances only, I chose to modify instances of String with Union{String,AnnotatedString}. Is this the right approach? Is this better or worse than widening it to AbstractString? I'm not familiar with how the compiler might handle these differently, or how this is approached within this codebase.

Relevant PRs
#36689
#46474

@brenhinkeller brenhinkeller added the REPL Julia's REPL (Read Eval Print Loop) label Oct 27, 2023
@caleb-allen caleb-allen force-pushed the caleb-allen/repl-styling-with-annotated-strings branch from b4b1265 to 294af95 Compare October 28, 2023 18:35
tecosaur and others added 13 commits November 2, 2023 02:39
This gets us two commits of interest:
- Replace within-module eval with hygienic eval: which makes it possible
  to include StyledStrings in the sysimage without running into
  precompile errors.
- Load the JULIA_*_COLOR env vars for compat: which mirrors the current
  behaviour to the relevant faces.
By loading the StyledStrings stdlib in REPL, we load the privateered
print/show methods for the Annotated{String,Char} types defined there.

This is nice to have, because it means that styled annotated strings can
be constructed in Base and elsewhere without loading the StyledStrings
stdlib, but they will be displayed as intended in the REPL.
@caleb-allen caleb-allen force-pushed the caleb-allen/repl-styling-with-annotated-strings branch from 0de266a to 51309a6 Compare November 2, 2023 17:54
trylock(s.refresh_lock) || return
try
og_prompt = s.p.prompt
beep_prompt = styled"{$beep_face:$(prompt_string(og_prompt))}"
Copy link
Author

Choose a reason for hiding this comment

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

@tecosaur : do you have feedback on this approach, or guidance for a better one? For context, the problem here is that the prompt string needs to be styled with the repl_prompt_beep face for a short period (to indicate a non-action to the user, like when pressing backspace at the beginning of a prompt)

I initially tried to use withfaces, but realized that doing so would require overriding all prompt faces (:repl_prompt_*), and doing so would:

  1. make future modifications slightly more fragile and
  2. exclude user-defined REPL modes.

Thus, my solution here is to create a new styled string which wraps the prompt string in a new style, $beep_face, and then restores the original prompt afterward.

Copy link
Contributor

@tecosaur tecosaur Feb 4, 2024

Choose a reason for hiding this comment

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

Faces applied later take priority, so applying repl_prompt_face around should do the trick nicely I think 🙂

image

I'm curious though, is there any particular reason why beep_face is a variable and you don't just use repl_prompt_beep?

Copy link
Author

@caleb-allen caleb-allen Feb 4, 2024

Choose a reason for hiding this comment

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

Great, thank you for the direction 👍

is there any particular reason why beep_face is a variable and you don't just use repl_prompt_beep

I think my original intent was to maintain the pre-existing logic for the case when color was disabled. Essentially what was removed here:

https://github.com/JuliaLang/julia/pull/51887/files#diff-730f90e0bf9ca018477a1d4a52e0d7398af2bb8cf9e401568fd86690eec81bb7L499

Perhaps a variable like this isn't needed at all, now that StyledStrings is used? In other words, the original variable existed so that the terminal's color state could be carried through to the point of printing the beep control sequences (or not), but is no longer necessary (I think?) because it is precisely the problem which StyledStrings solves. Does that sound right @tecosaur?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think so 😀.

If we're interpreting this correctly, a chunk of this code was originally needed because of the lack of composable styling, but that's a feature that StyledStrings has OOTB.

That said, there's also the complication that it seems like beep_colors is designed so that it could hypothetically cycle through a rainbow of colours? This seems rather strange though. Similarly, there are other aspects that just don't make sense to me, like beep_use_current. I don't understand when you'd want it to be false, and I went through all 104 results for that symbol in GitHub's site-wide search, and couldn't come across any code that set it to anything but true 😕.

The first (of two) commit I could find with it was 1767963 (6 years ago), and that's not introducing it but replacing a global const BEEP_USE_CURRENT = Ref(true). That const itself came from 8c2fc37 just a few months earlier, and the PR that introduces it has this for explanation:

I like it sober, so it blinks once by default, alternating with :light_black color (can be customized).

I think with the benefit of hindsight I'm inclined to view the addition of five variables to control the behaviour of the prompt "beep" as an enthusiastic addition which isn't retrospectively worth the complication it adds, given I haven't heard of anyone customising it previously, nor have I been able to find any examples by searching GitHub.

@rfourquet perhaps you might have something to add here?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we're able to reduce the number of beep customisations, we could potentially reduce the main logic to just something like this (untested):

try
    orig_prompt = s.p.prompt
    prompt_str = prompt_string(orig_prompt)
    isbeep = false
    while s.beeping > 0.0
        s.p.prompt = if (isbeep = !isbeep)
            styled"{repl_beep:$prompt_str}"
        else prompt_str end
        refresh_multi_line(s, beeping=true)
        sleep(blink)
        s.beeping -= blink
    end
    s.p.prompt = orig_prompt
    refresh_multi_line(s, beeping=true)
    s.beeping = 0.0
finally
    unlock(s.refresh_lock)
end

Comment on lines 103 to +107
out_stream::IO
buf::IOBuffer
end

TerminalBuffer(terminal::TextTerminal) = TerminalBuffer(terminal, IOBuffer())
Copy link
Member

Choose a reason for hiding this comment

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

seems like this is writing to the wrong stream now, based on expected/past/current usage?

Suggested change
out_stream::IO
buf::IOBuffer
end
TerminalBuffer(terminal::TextTerminal) = TerminalBuffer(terminal, IOBuffer())
out_stream::IOBuffer
ctx::IO
end
TerminalBuffer(terminal::UnixTerminal) = TerminalBuffer(IOBuffer(), terminal)

(aside, seems like this type some slightly unnecessary alternative to using a TTYTerminal(devnull, IOContext(IOBuffer(), terminal.out_stream), devnull) and deleting UnixTerminal and TextTerminal, but eh, whatever, that is a pre-existing duplication)

Comment on lines +170 to +175
Base.in(key_value::Pair, t::TerminalBuffer) = in(key_value, t.out_stream)
Base.haskey(t::TerminalBuffer, key) = haskey(t.out_stream, key)
Base.getindex(t::TerminalBuffer, key) = getindex(t.out_stream, key)
Base.get(t::TerminalBuffer, key, default) = get(t.out_stream, key, default)

Base.peek(t::TerminalBuffer, ::Type{T}) where {T} = error("Not implemented")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Base.in(key_value::Pair, t::TerminalBuffer) = in(key_value, t.out_stream)
Base.haskey(t::TerminalBuffer, key) = haskey(t.out_stream, key)
Base.getindex(t::TerminalBuffer, key) = getindex(t.out_stream, key)
Base.get(t::TerminalBuffer, key, default) = get(t.out_stream, key, default)
Base.peek(t::TerminalBuffer, ::Type{T}) where {T} = error("Not implemented")
Base.in(key_value::Pair, t::TerminalBuffer) = in(key_value, t.ctx)
Base.haskey(t::TerminalBuffer, key) = haskey(t.ctx, key)
Base.getindex(t::TerminalBuffer, key) = getindex(t.ctx, key)
Base.get(t::TerminalBuffer, key, default) = get(t.ctx, key, default)

@vtjnash
Copy link
Member

vtjnash commented Feb 5, 2024

Since the prompt string is no longer restricted to String instances only, I chose to modify instances of String with Union{String,AnnotatedString}. Is this the right approach? Is this better or worse than widening it to AbstractString? I'm not familiar with how the compiler might handle these differently, or how this is approached within this codebase.

Armchair commentary there is that if the field depends on specific implementation details relevant only to String (and now AnnotatedString) then this small union makes sense. But otherwise, Union{AbstractString, Function} seems like a more useful level of abstraction there.

@caleb-allen
Copy link
Author

I'm hoping to revisit this next week to push it through

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
REPL Julia's REPL (Read Eval Print Loop)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants