Make banner size depend on terminal size #51811
Open
+120
−81
Conversation
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
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
tecosaur
force-pushed
the
banner-to-repl
branch
from
75c6468
to
97eb067
Compare
|
If it's needed to get into 1.11 (just to avoid the conflict with |
|
If I have three differently-sized terminals open, I think I would rather not want to see three slightly different banners. In OSCAR we just have the usual banner and a one-line banner. How was the braille dots banner produced? That one looks very nice. |
I suspect that people here are over-estimating how much they'll see different banners in practice. If you're curious/suspicious, I'd encourage you to check out this branch 🙂. For context, here's a haphazard tiling of terminals I put together using the current state of this PR:
|
60 tasks
KristofferC
added a commit
that referenced
this pull request
Mar 17, 2024
Backported PRs: - [x] #39071 <!-- Add a lazy `logrange` function and `LogRange` type --> - [x] #51802 <!-- Allow AnnotatedStrings in log messages --> - [x] #53369 <!-- Orthogonalize re-indexing for FastSubArrays --> - [x] #48050 <!-- improve `--heap-size-hint` arg handling --> - [x] #53482 <!-- add IR encoding for EnterNode --> - [x] #53499 <!-- Avoid compiler warning about redefining jl_globalref_t --> - [x] #53507 <!-- update staled `Core.Compiler.Effects` documentation --> - [x] #53408 <!-- task splitting: change additive accumulation to multiplicative --> - [x] #53523 <!-- add back an alias for `check_top_bit` --> - [x] #53377 <!-- add _readdirx for returning more object info gathered during dir scan --> - [x] #53525 <!-- fix InteractiveUtils call in Base.runtests on failure --> - [x] #53540 <!-- use more efficient `_readdirx` for tab completion --> - [x] #53545 <!-- use `_readdirx` for `walkdir` --> - [x] #53551 <!-- revert "Add @create_log_macro for making custom styled logging macros (#52196)" --> - [x] #53554 <!-- Always return a value in 1-d circshift! of abstractarray.jl --> - [x] #53424 <!-- yet more atomics & cache-line fixes on work-stealing queue --> - [x] #53571 <!-- Update Documenter to v1.3 for inventory writing --> - [x] #53403 <!-- Move parallel precompilation to Base --> - [x] #53589 <!-- add back `unsafe_convert` to pointer for arrays --> - [x] #53596 <!-- build: remove extra .a file --> - [x] #53606 <!-- fix error path in `precompilepkgs` --> - [x] #53004 <!-- Unexport with, at_with, and ScopedValue from Base --> - [x] #53629 <!-- typo fix in scoped values docs --> - [x] #53630 <!-- sroa: Fix incorrect scope counting --> - [x] #53598 <!-- Use Base parallel precompilation to build stdlibs --> - [x] #53649 <!-- precompilepkgs: package in boths deps and weakdeps are in fact only weak --> - [x] #53671 <!-- Fix bootstrap Base precompile in cross compile configuration --> - [x] #52125 <!-- Load Pkg if not already to reinstate missing package add prompt --> - [x] #53602 <!-- Handle zero on arrays of unions of number types and missings --> - [x] #53516 <!-- permit NamedTuple{<:Any, Union{}} to be created --> - [x] #53643 <!-- Bump CSL to 1.1.1 to fix libgomp bug --> - [x] #53679 <!-- move precompile workload back from Base --> - [x] #53663 <!-- add isassigned methods for reinterpretarray --> - [x] #53662 <!-- [REPL] fix incorrectly cleared line after completions accepted --> - [x] #53611 <!-- Linalg: matprod_dest for Diagonal and adjvec --> - [x] #53659 <!-- fix #52025, re-allow all implicit pointer casts in cconvert for Array --> - [x] #53631 <!-- LAPACK: validate input parameters to throw informative errors --> - [x] #53628 <!-- Make some improvements to the Scoped Values documentation. --> - [x] #53655 <!-- Change tbaa of ptr_phi to tbaa_value --> - [x] #53391 <!-- Default to the medium code model in x86 linux --> - [x] #53699 <!-- Move `isexecutable, isreadable, iswritable` to `filesystem.jl` --> - [x] #41232 <!-- Fix linear indexing for ReshapedArray if the parent has offset axes --> - [x] #53527 <!-- Enable analyzegc checks for try catch and fix found issues --> - [x] #52092 - [x] #53682 <!-- Increase build precompilation --> - [x] #53720 - [x] #53553 <!-- typeintersect: fix `UnionAll` unaliasing bug caused by innervars. --> Contains multiple commits, manual intervention needed: - [ ] #53305 <!-- Propagate inbounds in isassigned with CartesianIndex indices --> Non-merged PRs with backport label: - [ ] #53736 <!-- fix literal-pow to return the right type when the base is -1 --> - [ ] #53707 <!-- Make ScopedValue public --> - [ ] #53696 <!-- add invokelatest to on_done callback in bracketed paste --> - [ ] #53660 <!-- put Logging back in default sysimage --> - [ ] #53509 <!-- revert moving "creating packages" from Pkg.jl --> - [ ] #53452 <!-- RFC: allow Tuple{Union{}}, returning Union{} --> - [ ] #53402 <!-- Add `jl_getaffinity` and `jl_setaffinity` --> - [ ] #52694 <!-- Reinstate similar for AbstractQ for backward compatibility --> - [ ] #51928 <!-- Styled markdown, with a few tweaks --> - [ ] #51816 <!-- User-themable stacktraces --> - [ ] #51811 <!-- Make banner size depend on terminal size --> - [ ] #51479 <!-- prevent code loading from lookin in the versioned environment when building Julia -->
59 tasks
LilithHafner
removed
the
backport 1.11
|
Triage: Because this has sat around for so long, let's leave it to 1.12. If we use names for the maximum sizes (e.g. "tiny", "small", "narrow"), then it's not breaking any more. Triage also generally likes the look of this as of the 4-variant version. |
|
Yeah, we can have options to suppress this as well, but I think this will be a much nicer default experience. I expect the tiny banner to be extremely rare, and when it gets used any banner would look terrible anyway. The normal and narrow banners look essentially the same stylistically, the narrow one is just a cleverly "wrapped" version of the normal one. The short one does look different but I suspect will also be pretty rare. |
|
I think the update I've just pushed does the rename suggested by Triage, I've just been unable to test since with the current |
tecosaur
force-pushed
the
banner-to-repl
branch
3 times, most recently
from
1441d5c
to
010f62f
Compare
|
Is this working again for you? |
|
Yep, the issue I mentioned was solved just be rebasing on a newer |
|
Oh, I've also just switched the internal number-name (now: 0=off, 1=largest, ..., 4=smallest) association to be a bit more sensible, thought I might as well toss that in while fixing up the tests. I've also just noticed that the "invalid argument" message needs to be updated, which I've just done. I think this should be good to go at this stage :) |
fingolfin
reviewed
Jul 22, 2024
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
|
If somebody's willing to approve and merge this, I'll muster up the willpower to resolve the latest merge conflict. |
|
I've already approved this, but of course it'd be nice if some more people gave it a thumbs up |
|
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. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.