Make banner size depend on terminal size #51811
Merged
+134
−81
Conversation
This file contains hidden or 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
tecosaur
added
REPL
|
Some relevant discussion #25587 (comment) |
|
A long time ago i made a package that replaced the default banner with an adaptative one https://github.com/longemen3000/BetterBanner.jl . In my particular implementation, The banner graphics stay the same, but I start adding line breaks to some text present in the banner |
Interesting. The rounded
I've seen this 🙂. While that does (mostly) solve the matter of narrower terminals, it doesn't help with how (IMO at least) it looks a bit silly when the banner takes up most of the terminal. For instance, here's me opening a squat terminal with this PR:
The braille dots approach is interesting though, and I think your screenshots don't do it justice.
|
tecosaur
force-pushed
the
banner-to-repl
branch
from
f1ec0a4 to
cfc3230
Compare
|
Ok, I've solved problem (2), and it's a classic coding-when-you-should-have-been-asleep thing. I had |
|
Bikeshedding: I like the braille dots "Julia", and dislike how Julia is printed in variant 4 and 5. Better to not show it at all in this case. |
|
Julia 5/4 are for slightly but not extremely squat/narrow terminals. 
If we remove 4, we'd jump straight to 2, which I think is too aggressively shrunk. 
|
tecosaur
force-pushed
the
banner-to-repl
branch
from
cfc3230 to
2f51baa
Compare
KristofferC
reviewed
Oct 23, 2023
|
Some opinions:
|
|
Thanks for the thoughts Kristoffer.
|
|
How about merging this with version 5 as the default, and having one other option: julia --banner=short (and defaulting to if very narrow). I like stuff moved out of Base, and not too concerned about bloating REPL, so could go either way on having many options, but it seems it can also go into a (already existing) package. I also like the like the braille dots "Julia", maybe even more, I just have concerns about not all terminals supporting? If not such could/should be the new default. We could even merge such versions since for master/1.11 to begin with, to see if people object/it doesn't work. |
|
Regarding precompiling, with The extra time (benchmarking The trace-compile output |
|
That 750 should be 0. #51557 |
|
Ooof, well at least it's not a problem I'm accidently introducing here. |
|
Ok, I'm about to push a much more conservative take, that only has four variants.
(1) and (2) should look pretty familiar, it's just the current banner with:
(3) and (4) are only used by default in very extreme situations, namely TTYs of 1-3 rows (where even scrolling the big banner feels silly).
|
tecosaur
force-pushed
the
banner-to-repl
branch
from
2f51baa to
e3ece93
Compare
tecosaur
force-pushed
the
banner-to-repl
branch
from
e3ece93 to
6f80a22
Compare
|
Now resolved. |
|
The "Rotated", with the logo on top, banner works with/respects small horizontal space, but I would also like to limit vertical space, why I suggested the new version 5 as the default or (same-sized) with braille dots. I don't have the use-case of narrow/tiled windows, by I think I at least would want to go straight to the "last-resort one-liner", and be annoyed by getting a double vertical spaced one, why I suggested only 2 is enough. You can merge as is, or with such a change, since it does't affect me much. I think I would personally opt-into the tiny one always. It's better than turning it off, since it can be helpful to know which version you're using, at least for power-users (and most other likely would not opt-into a non-default, or know how). I note python3 has only a 2-line banner... |
So would I, but it seems that larger changes are more controversial, so I'd like to leave that to a subsequent PR/discussion. This slimmed-down version doesn't really add any new banners, other than the "rotated" form, but does solve the banner-reflow issue, so I'm inclined to not let perfect be the enemy of better, and settle for that improvement for now. |
|
I really like the braille dots! |
|
Yea, the braille dots are really cool. I'd like to explore them further. |
|
I think the moving banner to REPL part of this is ready to merge. If you split it out into a separate PR we can merge that now and make reviewing this PR easier. Using multiple well-factored commits is helpful, but it would be even better to do two separate PRs so that discussion and CI can be separate. As for the second commit, having both (3) and (4) seems a bit excessive. Specifically, I don't like that the distinction between a TTY 3-4 lines tall vs a TTY 0-2 lines tall results in a semantic change in which information is displayed. Also, an objection to (4), the one line fallback, all other banners display the branch name. The only other way to get the branch name is imo is good but is missing important information. I think we should only omit the |
tecosaur
force-pushed
the
banner-to-repl
branch
2 times, most recently
from
7d1267e to
9d984f2
Compare
|
Maybe I should be more willing to merge my own PRs, but somehow the idea of doing so feels wrong to me. I've got a strong ingrained feeling that in a project like this I should almost never merge my own PRs, and it's hard to shake. |
|
Bump and backport 1.12? |
|
Probably not backport now that we've had the feature freeze. Anyway, I still feel uncomfortable with merging my own PRs, so this is ultimately waiting for somebody else who's willing to review + merge (at which point I'll find the time to resolve the latest merge conflict). |
|
@fingolfin approved way before the feature freeze. This is a nice little feature, so I think we should just merge it (but I have no power here; to do that). alpha1 seems days away, would be good to get it in, because, it's a feature we can live without, and plenty of time to just drop it if problematic. This shouldn't slow down startup much, at least not without a REPL (my issue I just opened), and I can't think of many objections to this, even if measurably slower, but not noticeable. I would test this if I could...: |
|
I like this. Let's do it. Before/after feature freeze doesn't really matter—it will eventually be in a release. This is definitely not worth a backport, please stop suggesting that minor features that you happen to like get backported. |
tecosaur
force-pushed
the
banner-to-repl
branch
from
0f4d783 to
1f62965
Compare
|
I've resolved the merge conflicts and confirmed it works, but checking perf it seems like there's now a substantial of non-precompiled code that's run, increasing the REPL latency by a noticeable amount.
|
|
I see the |
|
I haven't given this a lot of attention since (I generally don't tend to have much success trying to reduce precompilation, even with a large amount of time/effort), but I have made some changes that should help (e.g. removing broadcasting, as mentioned). To be clear, in my eyes, the only blocker is the latency hit from methods that aren't precompiled/are re-compiled. I'm also going to cross my fingers and hope that there might be compiler/precompilation changes that may help with:
With StyledStrings being used more in the REPL too in PRs like #59778 and #59819 that will/seem likely to be part of 1.13, I think it would be nice to get this work in too, to form a larger batch of REPL-related improvements in 1.13. |
|
After some help from @vchuravy investigating why precompilation isn't working as well as it used to, the jump in latency that's now holding up merging has been speculatively connected to c31710a. At the very least, wrapping the REPL precompilation workload in After #59850, here's what remains: #= 4.9 ms =# precompile(Tuple{typeof(Base.get), Base.Dict{Tuple{Symbol, Any}, Int64}, Tuple{Symbol, Symbol}, Int64})
#= 20.3 ms =# precompile(Tuple{typeof(Base.setindex!), Base.Dict{Tuple{Symbol, Any}, Int64}, Int64, Tuple{Symbol, Symbol}})
#= 3.0 ms =# precompile(Tuple{typeof(Base.get), Base.Dict{Tuple{Symbol, Any}, Int64}, Tuple{Symbol, StyledStrings.Face}, Int64})
#= 28.8 ms =# precompile(Tuple{typeof(Base.setindex!), Base.Dict{Tuple{Symbol, Any}, Int64}, Int64, Tuple{Symbol, StyledStrings.Face}})
#= 2.0 ms =# precompile(Tuple{typeof(Base.all), Tuple{Bool, Bool}})
#= 10.7 ms =# precompile(Tuple{typeof(Base.get), Base.Dict{Tuple{Symbol, Any}, Int64}, Tuple{Symbol, String}, Int64})
#= 18.8 ms =# precompile(Tuple{typeof(Base.setindex!), Base.Dict{Tuple{Symbol, Any}, Int64}, Int64, Tuple{Symbol, String}})
#= 2.9 ms =# precompile(Tuple{typeof(Base.hashindex), Tuple{Symbol, String}, Int64})
#= 1.6 ms =# precompile(Tuple{typeof(Base.isempty), Base.Dict{String, Any}})At the very least, I think this now gives us enough information to indicate that the precompilation problems lie outside this PR. Given this, I'm inclined to suggest that this should be merged for 1.13. |
vchuravy
added a commit
that referenced
this pull request
Oct 16, 2025
REPL precompile scripts runs a workload and might thus encounter code in other standard libraries that needs to be precompiled. Before #54899 we had a bespoke variant of PrecompileTools.jl. PrecompileTools was fixed with #57828 so we can now re-instate the support in REPL. Noticed by @tecosaur, while looking at #51811
|
Now #59850 has been merged, the latency hit is ~90ms. Not great, but not nearly as bad as ~400ms. Given this, is it best to:
Thoughts? |
KristofferC
pushed a commit
that referenced
this pull request
Oct 16, 2025
REPL precompile scripts runs a workload and might thus encounter code in other standard libraries that needs to be precompiled. Before #54899 we had a bespoke variant of PrecompileTools.jl. PrecompileTools was fixed with #57828 so we can now re-instate the support in REPL. Noticed by @tecosaur, while looking at #51811 (cherry picked from commit ecfec85)
|
I'll also note that there is exactly one test failure that's responsible for the |
tecosaur
force-pushed
the
banner-to-repl
branch
from
1f62965 to
2ee9aef
Compare
|
I figure it's probably best to go with the approach of including the According to hyperfine, REPL latency is actually faster slightly than current after this PR (I'm guessing due to the manual precompilation): This may be affected by whether Results: |
We also spritz it up a bit using the new StyledStrings library, namely: - colouring the Help and Pkg key prompts - making the docs link a terminal link, and Pkg a link to the Pkg docs - using box drawing characters for the dividing line - making branch status more colourful - making the official version text more subdued With these change the four banners (from smallest to largest) are: 1. A one-liner, for extreme circumstances (new) 2. The short banner 3. The large banner, with the description stacked vertically (new) 4. The large banner, with the description stacked horizontally Because precompilation is a little flakey at the moment, I add a bunch of precompile statements to prevent latency from regressing.
tecosaur
force-pushed
the
banner-to-repl
branch
from
2ee9aef to
1ab0e32
Compare
|
With the latency concerns addressed, it seems sensible to (finally) merge this. Poking Stefan he's of a similar mind. |
|
Some comments, questions: 
|
|
First impressions.. 
(nit. the PR title should probably have mentioned that it changes the formatting also) |
|
This broke a tests on macOS: https://buildkite.com/julialang/julia-master/builds/51664#019a1eb8-3af0-4847-9dda-dc89eaead358/812-1578 It's failing everwhere |
|
I have some comments on the styling (short version: I'm happy to tweak it), but the CI failure is more pressing. I'm not quite sure what to make of it. It's only on This PR also passed CI, including Because of different precompile issues in #59958, also That has: but includes this PR and the history PR (why 1 instead of 15 precompiles?). There we see I'm not sure what to make of this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
14 participants
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.
This PR is the result of three thoughts:
versions.jlis a weird place for the banner function to live, when we haveREPLThe result of these three thoughts is a set of six banner sizes, selected based on
displaysizeto ensure that this never happens:The variants below are outdated, see this comment for more up-to-date screenshots: #51811 (comment)
There are two issues I'm currently facing that I'd appreciate help with:
evalinStyledStrings/src/stylemacro.jltoCore.eval(__module__during precompile, even if the code is pure. Furthermore, I find myself needing to change the clause on line 669 tostate.interpolated[] || Base.generating_output(). This seems a little dodgy, and so I'd appreciate thoughts.For some reason, with this PR the banner doesn't appear on startingjulia. Yet, as seen by the screenshot above invokingREPL.bannerdoes work. I have confirmed thatREPL.banneris indeed called, and so find myself at a loss here.