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

DietPi-Banner | Word wrap for small terminals #5820

Open
wants to merge 16 commits into
base: dev
Choose a base branch
from

Conversation

mtekman
Copy link
Contributor

@mtekman mtekman commented Oct 20, 2022

This PR implements word wrapping functionality in the DietPi-Banner, which helps small terminals format the text better.

Screenshots

105 Columns
dietpi_tput_105

33 Columns
dietpi_tput_33

@MichaIng MichaIng added this to the v8.10 milestone Oct 20, 2022
@MichaIng MichaIng added the Visual 🌹 Issues affecting only the beauty of menus and output, but not the functionality label Oct 20, 2022
@MichaIng
Copy link
Owner

Many thanks. Just to assure I understand the logic correctly:

  • If a line is too long, it first splits left and right side of the colon.
  • If each side is still too long, it splits at words.
  • It adds 3 spaces on each additional/split line to make it look part of the first.

Generally I'm fine with it, looks indeed much better. But let's try to achieve it with a little less external command calls to enhance performance, e.g.:

  • Obtain string length via ${#variable}
  • Replace parts of strings via ${variable//pattern/replace}
  • Not sure if there is a pure bash-internals way to split at words 🤔.

shellcheck also has some minor complains (only optional tests for best practice).

@mtekman
Copy link
Contributor Author

mtekman commented Oct 21, 2022

Hello, thanks for the review!

So the logic is:

  1. Split the line in two parts at inception:
    • "description": a title with whitespace that exists before the first colon
    • "result": the long text that is usually the text result of some other function call
  2. If the description is too long:
    • fold the description, but left-pad only the 2nd line onwards
  3. If the result is too long, or, the description+result is too long:
    • add a newline, then fold the result with left-padding
  4. Otherwise, print the line as-is.

Thanks for the bash tips! I will try to reduce the outside call overhead, and run shellcheck.


Also: I love this project, it's stable, works flawlessly out of the box, and has greatly enriched my life.

However, I've been reading on HN a lot about unwanted PR's from keen contributors who mean well, but ultimately destabilize the code base. If you think this PR is a little bit overkill, or might lead to more headaches in the future, then please don't feel the pressure to merge it. A quick: "thanks, but we like it how it is" is genuinely okay!

@MichaIng MichaIng modified the milestones: v8.10, v8.11 Oct 22, 2022
@mtekman mtekman force-pushed the banner_width branch 2 times, most recently from 6b16425 to ac54ef4 Compare October 31, 2022 14:36
@mtekman
Copy link
Contributor Author

mtekman commented Oct 31, 2022

I'll do some benchmarks later via strace -o trace -c -Ttt ./<script>, just to see how much overhead the last few commits incur. I want to test:

  • bash word-wrap vs fold
  • bash substitutions vs sed

initial tests via time seem to suggest no major time increase, but I want to see how it affects RAM/CPU given the limited Pi resources

@mtekman
Copy link
Contributor Author

mtekman commented Nov 1, 2022

dietpiall_edit

Okay, so I've made a few changes:

  • The "title" text (bold text before the ':') is word wrapped neatly by the terminal width
  • The "description" text (normal text after the ':') is word wrapped into the space to the right of the title text and left-padded with spaces.
  • Left-padding is done via bash substitutions not sed
  • Word wrapping was implemented in bash but could not handle whitespaces as good as fold. I tested via the time and strace command the difference between bash word wrapping and fold word wrapping and saw barely any change in performance.
  • Word wrapping can be enabled in the customization menu (by default on).
  • Word wrapping is about 2.5x slower than without, though we're talking about a difference of 0.15 vs 0.35 seconds.
  • If I disable word wrapping in the PR and compare to the original, I see a difference of 0.13 vs 0.15 seconds (old vs new).

@MichaIng
Copy link
Owner

Roughly doubled execution time on RPi 2 (excluding any download):

2022-11-13 13:15:42 root@micha:~# time for i in {1..10}; do /boot/dietpi/func/dietpi-banner 1; done > /dev/null

real    0m3.134s
user    0m1.876s
sys     0m1.631s
2022-11-13 13:15:49 root@micha:~# time for i in {1..10}; do /boot/dietpi/func/dietpi-banner2 1; done > /dev/null

real    0m6.670s
user    0m4.004s
sys     0m3.149s

There is still room for improvement:

  • Strip control codes without dedicated function, just with generic approach: stripped=${original//\\e[[0-9]*([;0-9])m}
    I can add this, if you agree (and I'm not overlooking something).
  • It does not handle multi-line strings properly, e.g. adds an additional space and unintended spacing to MOTD. We could remove line breaks in MOTD server-side and leave this to the banner, which would solve the issue, at least with this feature enabled.
  • I'm not sure whether I like the in case large indentation until colon, compared to fixed 3 spaces. This can make space for actual text very small, leading to multiple short lines. Thanks to hyphen and different colours, it is clear enough that the line is continuation of the above line? @Joulinar @StephanStS any opinion on this? Basically here vs here.

But many thanks for your work on this, I'll definitely merge it for v8.11 release.

@mtekman
Copy link
Contributor Author

mtekman commented Nov 13, 2022

@MichaIng - ah thanks for testing on the RPi2, is there any significant performance hit when ${aENABLED[16]}==0 ?

Strip control codes without dedicated function, ..... I can add this, if you agree (and I'm not overlooking something).

I recall trying this approach but somehow decided that exact matches would be more performant (somehow). Please do feel free to edit, a simple regex makes far more sense in retrospect!

It does not handle multi-line strings properly, e.g. adds an additional space and unintended spacing to MOTD. We could remove line breaks in MOTD server-side and leave this to the banner, which would solve the issue, at least with this feature enabled.

Damn. The pure bash implementation I tried had the same problem, so I thought using a dedicated tool like fmt or fold would solve it cleaner.

If the issue is just the MOTD text, then editing the MOTD makes sense to me.

I'm not sure whether I like the in case large indentation until colon, compared to fixed 3 spaces.

Hah! I thought it looked ugly with 3 indents, and didn't save much linewise. It was much faster though, I remember

@StephanStS
Copy link
Collaborator

I'm not sure whether I like the in case large indentation until colon, compared to fixed 3 spaces.

I like the colon wide indentation: This helps to read it better on narrow terminals. On typical wide terminals this seems to have no difference in the output. It looks more "table style".

@MichaIng
Copy link
Owner

Little performance enhancement with generic control code stripping (dietpi-banner2 is the version at time of writing, dietpi-banner3 with local enhancements):

# time for i in {1..10}; do ./dietpi-banner2 1; done > /dev/null

real    0m6.497s
user    0m3.922s
sys     0m3.055s
# time for i in {1..10}; do ./dietpi-banner2 1; done > /dev/null

real    0m6.728s
user    0m3.843s
sys     0m3.391s
# time for i in {1..10}; do ./dietpi-banner3 1; done > /dev/null

real    0m5.633s
user    0m2.950s
sys     0m3.169s
# time for i in {1..10}; do ./dietpi-banner3 1; done > /dev/null

real    0m5.731s
user    0m2.903s
sys     0m3.325s

Further enhancements (dietpi-banner is the original one):

# time ./dietpi-banner 1 > /dev/null

real    0m0.315s
user    0m0.200s
sys     0m0.155s
# time ./dietpi-banner 1 > /dev/null

real    0m0.302s
user    0m0.201s
sys     0m0.138s
# time ./dietpi-banner2 1 > /dev/null

real    0m0.669s
user    0m0.427s
sys     0m0.290s
# time ./dietpi-banner2 1 > /dev/null

real    0m0.669s
user    0m0.430s
sys     0m0.289s
# time ./dietpi-banner3 1 > /dev/null

real    0m0.400s
user    0m0.204s
sys     0m0.242s
# time ./dietpi-banner3 1 > /dev/null

real    0m0.401s
user    0m0.261s
sys     0m0.183s

Without line breaks enabled:

# time ./dietpi-banner 1 > /dev/null

real    0m0.302s
user    0m0.202s
sys     0m0.145s
# time ./dietpi-banner 1 > /dev/null

real    0m0.334s
user    0m0.176s
sys     0m0.197s
# time ./dietpi-banner2 1 > /dev/null

real    0m0.395s
user    0m0.225s
sys     0m0.223s
# time ./dietpi-banner2 1 > /dev/null

real    0m0.419s
user    0m0.214s
sys     0m0.251s
# time ./dietpi-banner3 1 > /dev/null

real    0m0.340s
user    0m0.172s
sys     0m0.205s
# time ./dietpi-banner3 1 > /dev/null

real    0m0.330s
user    0m0.247s
sys     0m0.125s

Space wrapping with word wrapping fallback like fold does, is very complex with pure shell syntax, at least I cannot think of a simple way. So this is best we can do, and +25% execution time is acceptable. It takes more time (up to doubled), when titles and/or descriptions are actually wrapped, but this is where the benefit takes effect as well, so my aim is that wide-term time is not increased.

But there are still some logical and visual issues:

  1. Wrapped title and descriptions are generated from the colour-stripped versions, hence colour codes are lost, e.g. for the hyphen, the colon in all cases, but also within the outputs.
  2. For titles, bold white is re-added as hardcoded colour (as well for hyphen and colon), but it is e.g. not correct for credits.
  3. For descriptions it is completely lost, e.g. the DietPi/APT update and reboot notifications are reverted to default colour.
  4. If colour codes are contained as raw control characters, e.g. when passed through echo -e already, of if an external command prints coloured text, those are not stripped, but added to length estimation, which leads to earlier than required line break, which can be seen in above After output, for CPU temperature and VPN status. It is furthermore possible that line break is done in the middle of such control code, which breaks colours or leads to various other unexpected behaviour, of the part before of after the break is still a valid control code.
  5. If the title with colon takes exactly the terminal width, it is not folded, hence no line break added, hence the description starts without padding at the new line.
  6. If the title with colon + 1 space takes exactly the terminal width, fold gets a width of 0 and hence prints the error fold: invalid number of columns: ‘0’: Numerical result out of range.
  7. Since also the titles are space-wrapped, the estimation of the last line's length is incorrect, which leads to wrong description indentation.
  8. The 3 space indentation for title wraps is added after fold, hence the title can be 3 characters too long, leading to unintended additional line breaks.

Some can be seen here:

padding

I'm not yet sure how to solve these things properly. A fold which ignores \e[ and raw control codes automatically, so that they don't need to be stripped, would be great, else we introduce a lot of potential inconsistencies. Spacing can be solved, at least a way that one line is never too long but may be only too short (if raw control codes are contained).

Finally, for this to work correctly, I think the whole banner output needs to be aligned to follow strict rules, i.e. consistent title and description colours, or adding the colour codes after the line breaking has been done.

@mtekman
Copy link
Contributor Author

mtekman commented Nov 16, 2022

The last local enhancement on your dietpi-banner3 really seems to be the winner here .

As for the folding artefacts you mention, maybe we really do need a custom word wrapping tool that can handle colour-codes and emoticons? I found this implementation dreamed up in awk, but for performance, one could write a small one in C.

That would of course create the extra headache of deploying, and maintaining a tool outside the main package manager.

One other improvement I thought about was adding a third argument to the Print_Line function, which tells us how the title should be styled. At the moment this is embedded in the title itself and then stripped out (and then later re-added with a generic style) by the function, but we could make this more dynamic.

Maybe even a 4th parameter, for the desired number of left-padding could be useful.

@MichaIng MichaIng modified the milestones: v8.11, v8.12 Nov 19, 2022
@MichaIng MichaIng modified the milestones: v8.12, v8.13 Dec 18, 2022
@MichaIng MichaIng modified the milestones: v8.13, v8.14 Jan 15, 2023
@MichaIng MichaIng modified the milestones: v8.14, v8.15 Feb 11, 2023
@MichaIng MichaIng changed the title Word wrap in dietpi-banner for small terminals DietPi-Banner | Word wrap for small terminals Feb 19, 2023
@MichaIng MichaIng modified the milestones: v8.15, v8.16 Mar 11, 2023
@MichaIng MichaIng modified the milestones: v8.16, v8.17 Apr 11, 2023
@mtekman
Copy link
Contributor Author

mtekman commented Apr 27, 2023

During a brief attempt to patch fold to detect unicode emoji's and colour, I stumbled across this tiny little word-wrap tool called par, which appears to have capabilities to support multibyte characters like emojis.

It can also be told to ignore some symbols (hopefully the \e[ colour syntax) but the syntax is very dense and I haven't really understood how to use it yet.

A quick test shows that par formats coloured and emoji text at least as good, or slightly better than fold and fmt.

text="This is \e[1;33mthe weird sentence\e[m ✅ that \e[0;41mtries\e[m to \e[4;42mvisually\e[m ⛏️  align\e[m ⌚ words to a (⛰️  ⛵) column width of 34 chr"; 
echo -e "$text" | /usr/bin/par 34f

@MichaIng
Copy link
Owner

It's a Debian package as well, here the man page: https://manpages.debian.org/par

@MichaIng MichaIng removed this from the v8.17 milestone May 6, 2023
@mtekman
Copy link
Contributor Author

mtekman commented Jul 18, 2023

It's moving forward: https://gitlab.com/mtekman/print_line

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Visual 🌹 Issues affecting only the beauty of menus and output, but not the functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants