-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
caleb-allen
wants to merge
16
commits into
JuliaLang:master
Choose a base branch
from
caleb-allen:caleb-allen/repl-styling-with-annotated-strings
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
621e646
Update StyledStrings version
tecosaur 887c3ee
Load StyledStrings in REPL
tecosaur 40af7f8
start poking around
caleb-allen 8820b92
yanking prefixes and suffixes
caleb-allen 045d5e8
prompt and prompt prefix stuff
caleb-allen 1039f60
exploring :color for TerminalBuffer
caleb-allen 498cfa8
Note to self
caleb-allen a30278a
Use properties of original IO for `TerminalBuffer`
caleb-allen 212a1b1
Clean up excess color parameters
caleb-allen 36b2a21
Re-implement `beep` with StyledStrings
caleb-allen 7631901
Remove notes file
caleb-allen 30545b7
Clean up some mess
caleb-allen 51309a6
remove duplicate statements from merge
caleb-allen dfbf552
Remove comments
caleb-allen 25f50f6
Remove some whitespace
caleb-allen 50d4973
Merge branch 'master' into caleb-allen/repl-styling-with-annotated-st…
caleb-allen File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
1 change: 1 addition & 0 deletions
1
deps/checksums/StyledStrings-c49ae82f2b176556944929ade88a4fecf1556ccb.tar.gz/md5
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
f4e49cee0253a6d00f1d057cad2d373a |
1 change: 1 addition & 0 deletions
1
deps/checksums/StyledStrings-c49ae82f2b176556944929ade88a4fecf1556ccb.tar.gz/sha512
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
487c8c9f0138710165e207dc198376417b312a4f5d91a8616c5093938a2b314afd2a4fb72ea687a9cdc7ddfac246dde793a5c2e60ce465851f7c83194ae6f7d8 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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: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.There was a problem hiding this comment.
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 🙂I'm curious though, is there any particular reason why
beep_face
is a variable and you don't just userepl_prompt_beep
?There was a problem hiding this comment.
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 👍
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?
There was a problem hiding this comment.
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, likebeep_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 buttrue
😕.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 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?
There was a problem hiding this comment.
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):