Dietpi banner upgrades#8172
Conversation
|
Outstanding items I know of:
|
MichaIng
left a comment
There was a problem hiding this comment.
A great extension! But there is some cleanup/polishing needed to reduce the overhead (in code an UI), and make it work for expected use cases.
| 'Word-wrap lines on small screens' | ||
| 'Kernel' | ||
| 'Network Usage' | ||
| 'Disk Usage' |
There was a problem hiding this comment.
- dynamic disk space usage is independent of the pre-existing disk space entries (what do we do with the existing options?)
I suggest we merge "Disk usage (RootFS)" and "Disk usage (userdata)" into the new single option. Otherwise it is confusing to have 3 different options all showing some disk usage, and hence also 3 code sections to keep in sync. Maybe / and /mnt/dietpi_userdata (if a mount point) can be dedicated fixed quick-select options in a parent menu. Or, maybe simpler, all findmnt -no SOURCE --real mount points could be part of that menu. So when adding a disk, there is a menu showing all mount points to select + custom, and custom shows the inputbox? The fixed mount points can still be treated like a glob when printing the banner, of course.
There was a problem hiding this comment.
So a dynamic checkbox array that has findmnt -no SOURCE --real options listed, plus a checkbox to then enter custom matching? And the first array item is always /mnt/dietpi_userdata.
There was a problem hiding this comment.
Right, that would be my suggestion. Little correction: findmnt -nro TARGET --real, to list them by mount point rather than block device, and without the tree view.
The hardcoded /mnt/dietpi_userdata wouldn't even be needed then, as its true mount point would show up in the list already, if it is a dedicated mount. And / shows up at the top, which is good as well.
…y to accept multiple commands. Added systemd and fail2ban status. Added network usage by namespace. Added disk usage by pattern matching. Added G_TRUNCATE_MID.
…lors inside custom commands
…UR saves just the relevant indexes. added some shellcheck bypasses and documentation.
…ead tests from network data usage. Get_Fail2Ban_Status is cleaner without the complicated sudo handling.
* restore dash-compatibility of Get_Cert_Status_Raw * cleanup Get_Cert_Status: `sudo -n` returns the exit code "1" if sudo permissions are missing, the "no cert found" error code would hence need to swap. However, there is already the exit code 0 handling based on the "No certificate found" output, so make us of that instead. If Get_Cert_Status_Raw runs at all, exit code 0 is now always returned, hence forge banner line based on its output. In case of another exit code, it must have come from a sudo failure, hence print the error about NOPASSWD sudo. * Move `Print_*` functions below `Get_*` functions, and rename `Get_*` functions which are supposed to print the formatted output to console directly to `Print_*` * Remove trailing COLOUR_RESET where not required * Simplify Get_Fail2Ban_Status: process `fail2ban-client` output with a single awk to avoid the large amount of external command calls, and print it directly. * create variables as local ones where appropriate * Simplify Print_Network_Usage: The `/sys/class/net/*/statistics` is always world-readable (unless the kernel is manipulated). Skip related checks, and always read it directly if no namespace is used, using the cheaper builtin `read` instead of external `cat` call. Tailore Get_WAN_IP. Parse simpler `ip -br` output instead of `ip -o`. Print curl errors as is. They should be short with `-sSf`, and leaving them untouched helps with debugging.
| # Separator line | ||
| (( ${aENABLED[useful_commands]} || ${aENABLED[motd]} || ${aENABLED[print_credits]} )) && echo -e "$CLI_LINE" |
There was a problem hiding this comment.
This one seems to be doubled. Especially if the MOTD is disabled, but useful commands or credits are enabled, there will be two lines one after another. If you want to separate the MOTD from the potential prior multi-line outputs, it could be done inside the if (( ${aENABLED[motd]} )) instead.
There was a problem hiding this comment.
Ah, was this probably meant to be:
(( ${aENABLED[network_usage]} || ${aENABLED[disk_usage]} || ${aENABLED[custom_commands]} )) && echo -e "$CLI_LINE"So to print a separator line if there was a subheader output above, which do print a separator line before their multi-line output?
and align new item descriptions
| then | ||
| echo -e "${aCOLOUR[alert]}Custom command $i failed ($status): $output" | ||
| else | ||
| echo -e "$output" |
There was a problem hiding this comment.
I do not like that users need to enter all the formatting stuff manually. I suggest the following:
- Turn
aCUSTOM_COMMANDSinto an associative array, using the subtitle/name as index. - In the menu, replace "Add/Edit/Remove" with
Add/name1/name2/.... So the current custom commands are shown in the menu with their name that is used as array index. Selecting a current command opens the inputbox to edit it. An empty command removes the entry. "Add" requires to enter the command first, and the name afterwards. - We use
Print_Item_State <name> <output>, hence take care of the formatting.
If someone really wants to use a custom formatting, it is still possible to prefix the command with echo -e '\r';, still much shorter than all the color codes needed otherwise.
There was a problem hiding this comment.
My thinking was that a custom command would not have to fit the common formatting, but I am okay with either way. I feel having to name each command is excessive, but that is needed to use Print_Item_State and it's format.
This current version also breaks using custom colors/etc. within the command output, which I think is an important function.
e.g. the alert format for a system condition that is important to the user
I do like an empty inputbox removing the command
There was a problem hiding this comment.
I feel having to name each command is excessive
Currently, if one wants to have some informational title, it needs to be printed (and in case formatted manually) as part of the command. So the amount of things one needs to type is usually not much different when requiring a name. The other benefit is the better overview in the menu, when active custom commands are visible without entering the "Edit" or "Remove" menu first. Of course numeric names would do as well, like using the current numeric index for the menu entry as well as for Print_Item_State "Command $i" "$output".
This current version also breaks using custom colors/etc. within the command output, which I think is an important function.
The current versions does, indeed. The new version, even with Print_Item_State, would still allow to enter color codes manually, but it wouldn't be needed anymore for the base formatting to match the other banner output lines.
Or we allow both: When leaving the command name/title empty, it is added as numeric index to the array, which triggers the output to be printed directly without Print_Item_State. If it is given a name, Print_Item_State is used for formatting.
|
Just as an overview, the things I'd still want to do before merging. But will do so before DietPi v10.6 release (which will be delayed anyway because of a spontaneous holiday trip consuming some time this week):
|
* remove redundant carriage return: Print_Subheader isn't used anywhere inline. * remove obsolete comment about GeoIP removal, since this info is seen as benefitical addition to the IP address * remove doubled newline: dietpi-print_large has an empty line included already. Adding a second empty line makes the gap too big IMO
Whoos, dietpi-print_large does NOT include an empty line. It just appears as if it did, if the hostname has not hanging character, like no "g", "q" etc. Those however make use of the last line, in which case the next banner entry looks too close.
…being unfriendly.
I changed it to `echo -e` since the used `%s` format codes printed color codes literally. However, this can be solved with `%b` as well, keeping the min-length format codes of `printf`. Raised the "IP (iface)" min length to 23 characters, matchig complete IPv4 address + classic WiFi interface name.
|
I went through the open points you mentioned at the top. Regarding clearing the terminal in VS Code: See |
Enhancing dietpi-banner.