Use StyledStrings for REPL prompt styling

[caleb-allen]

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:

• Clean up code, remove comments/notes
• Investigate flashing REPL
• Rebase onto Update and load styledstrings #51869
• Revise PR description with context and details

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

[caleb-allen]

@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.

[tecosaur]

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

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

[caleb-allen]

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?

[tecosaur]

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?

[tecosaur]

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

[vtjnash]

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)

[vtjnash]

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]

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.