Skip to content

Conversation

@macpijan
Copy link
Contributor

@macpijan macpijan commented Oct 1, 2025

No description provided.

Signed-off-by: Maciej Pijanowski <[email protected]>
Signed-off-by: Maciej Pijanowski <[email protected]>
Copy link
Contributor

@DaniilKl DaniilKl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reviewed only code that is important from the architecture point of view.

Comment on lines +245 to +278
# ui_action_exit
# Exit the UI loop
ui_action_exit() {
ui_exit 0
}

# ui_action_reboot
# Reboot the system (requires confirmation)
ui_action_reboot() {
if ui_confirm "Are you sure you want to reboot?"; then
ui_print_info "Rebooting system..."
sync
reboot
fi
}

# ui_action_poweroff
# Power off the system (requires confirmation)
ui_action_poweroff() {
if ui_confirm "Are you sure you want to power off?"; then
ui_print_info "Powering off system..."
sync
poweroff
fi
}

# ui_action_shell
# Launch a shell
ui_action_shell() {
ui_clear_screen
echo "Entering shell. Type 'exit' or press Ctrl+D to return to menu."
echo
bash || /bin/sh
}
Copy link
Contributor

@DaniilKl DaniilKl Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure whether UI that is a library and should only modify what is being displayed and redirect commands to backend that should do these commands. It would rather redirect them to backend so backend can execute any hardware or firmware -specific things before, for example, rebooting.

Generally it would be better not to add any code that modify hardware or firmware states that are not related to GUI.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it is a minor architectural note. Can be addressed later

@@ -0,0 +1,329 @@
#!/usr/bin/env bash
Copy link
Contributor

@DaniilKl DaniilKl Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently the library has four functions that can use fallbacks to draw the specific parts of DTS main menu, I have added threads regarding these functions:

After analyzing the architecture depply I dissagree with my conclusion "I do not like that we delegate drawing header to some other function." from all the threads. Right now I think the library should use fallbacks that are defined by backend because the library provides primitives to build UI and should not know how the backend arranges these primitives to display the infomration it wants.

But after checking the library I have not found some of the needed primitives. The missing primitives forces the backend to use echo in the fallbacks and we want to ommit this. For example consider this fallback from the examples:

dts_custom_header() {
  local _os_version
  _os_version=$(grep "VERSION_ID" "${OS_VERSION_FILE}" 2>/dev/null | cut -d "=" -f 2- | tr -d '"' || echo "unknown")

  printf "\ec"
  echo -e "${NORMAL}\n Dasharo Tools Suite Script ${_os_version} ${NORMAL}"
  echo -e "${NORMAL} (c) Dasharo <[email protected]> ${NORMAL}"
  echo -e "${NORMAL} Report issues at: https://github.com/Dasharo/dasharo-issues ${NORMAL}"
}

This is an example DTS header fallback that will print smth like this:

 Dasharo Tools Suite Script 2.7.1 
 (c) Dasharo <[email protected]> 
 Report issues at: https://github.com/Dasharo/dasharo-issues

But I do not want to use the echo but want to use the function the UI library will provide. This way we will make all backends adhere to the general menu format we define by library and only change the structure for their needs. This will help to stay in line with the OSFV parsers for all projects that will use this IU library.

The example above could be covered by one primitive that can be called, for example, a simple line consists of two properties:

  1. It is a text line.
  2. It does not have any formatting (hence 'simple').

Then the fallback could look like this:

dts_custom_header() {
  local _os_version
  _os_version=$(grep "VERSION_ID" "${OS_VERSION_FILE}" 2>/dev/null | cut -d "=" -f 2- | tr -d '"' || echo "unknown")

  ui_print_simple_line "Dasharo Tools Suite Script ${_os_version}"
  ui_print_simple_line "(c) Dasharo <[email protected]>"\
  ui_print_simple_line "Report issues at: https://github.com/Dasharo/dasharo-issues"
}

To list the other primitives that the UI should provide to construct the menu we can use current DTS menu as an example:

 Dasharo Tools Suite Script 2.7.1 
 (c) Dasharo <[email protected]> 
 Report issues at: https://github.com/Dasharo/dasharo-issues 
*********************************************************
**                HARDWARE INFORMATION 
*********************************************************
**    System Inf.: Emulation QEMU x86 q35/ich9
** Baseboard Inf.: Emulation QEMU x86 q35/ich9
**       CPU Inf.: Intel Core Processor (Skylake)
**    RAM Virtual: Not Specified
*********************************************************
**                FIRMWARE INFORMATION 
*********************************************************
** BIOS Inf.: 3mdeb Dasharo (coreboot+UEFI) v0.2.1-rc1
*********************************************************
**                DPP credentials 
*********************************************************
**      Email: ***************
**   Password: ***************
*********************************************************
**     1) Dasharo HCL report
**     2) Update Dasharo Firmware
**     4) Edit your DPP keys
**     6) Transition Dasharo Firmware
**     7) Fuse platform
*********************************************************
R to reboot  P to poweroff  S to enter shell  
K to launch SSH server  L to enable sending DTS logs 
C to display DPP credentials 

Enter an option:

Where:

*********************************************************

Is a separator that consists of properties separator character and length that could be configured. This primitive is already handled by function ui_draw_separator() (I have not checked if the implementation is correct).

Then we have:

**                HARDWARE INFORMATION 

That could be called a header line, that is line, has a frame constructed of the separator character (* in this case),
has some predefined header formatting and the header text. The header text can be configured when adding the line, but other properties are predefined during the menu initialization. So printing the header lines should look like this ui_print_header_line "HARDWARE INFORMATION"

Then we have lines like:

**    System Inf.: Emulation QEMU x86 q35/ich9

That we can call an information line, that is line, that presents information (in this case Emulation QEMU x86 q35/ich9) for some property (in this case System Inf.), that is centered, separated by : and has the frame. So the call could look like ui_print_information_line 'System Inf.' 'Emulation QEMU x86 q35/ich9' with all of the fomatting predefined.

Then we have the option line primitive, for example:

**     1) Dasharo HCL report

Then the footer option primitive, for example:

R to reboot

These as I can see are handled by functions ui_render_menu and ui_render_default_footer respectfully.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something like this 28fc9ea ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, smth like this. We can go even further by adding some more formatting into the primitives functions. E.g. to center the text in header line automatically.

@@ -0,0 +1,329 @@
#!/usr/bin/env bash

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently we have 3 types of output that a user can see:

  1. The output we want the user to see.
  2. The output we do not want the user to see.
  3. The output we want user to see when a certain condition is met (e.g. the debug logs should be seen only when turned on).

The third one might be questionable.

The example of a command that produces the 1 output:

echo -ne "${UI_THEME[PROMPT]}\nEnter an option:${UI_COLORS[NORMAL]} "

The example of a command that produces the 2 output:

cbfstool <args>

Both these commands are printing their STDOUT (and STDERR as well) to the default file descriptor defined by the console Linux kernel command line parameter.

In DTS we typically redirect the 2 output to /dev/null. But there are cases when we do not, for example if we forget or some tool will unexpectedly start to throw smth. to STDOUT. These cases appear in the backends and are not controlled by frontend, hence they could ruin the UI experience.

The idea is to print all the output from the UI libraries not to the file descriptor defined by the console Linux kernel command line parameter, but to some other. And then display the UI from this descriptor. This way we will not care whether some tool unexpectedly throus smth to the file descriptor defined by the console Linux kernel command line parameter. But this may bring up some TTY redirection issues.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a @m-iwanicki idea.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something from logging function could be used:

start_logging() {
local file="$DTS_LOG_FILE"
if [ -z "$LOGGING" ]; then
LOGGING="true"
if [ -z "$DESC" ]; then
# Create file descriptor that echoes back anything written to it
# and also logs that line to file with added timestamp prefix
exec {DESC}> >(tee >(stdbuf -i0 -oL -eL ts "[%T]: " >>"$file"))
fi
exec 1>&$DESC
fi
}

# SPDX-License-Identifier: Apache-2.0

# ui-core.sh - Core UI engine and main loop for bash-ui-lib

Copy link
Contributor

@DaniilKl DaniilKl Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently we have all global (not all but many actually) DTS variables in one bash env.. Bringing another library with the configuration stored in global bash variables will mess upp the environment further. For example some backend can overwrite the UI configuration variable without calling to the specific UI function, hence messing up UI configuration.

The idea is to make the UI not a simple library that will consists of the functions that can be called from backend like:

source ui-render.sh

ui_print_info "Hello world!"

Where the ui_print_info uses global variables that are visible by the backend code as well. But to make the UI a bash script (or a set of bash scripts) twith a small library consisting of simple functions that will:

  1. Provide the ability of modifying UI static configuration that is stored, for example, in YAML file.
  2. Provide the ability to communicate the dynamic variables via a defined set of variables (this set of variables already exists in DTS) to the UI. For example:
source ui-helper-functions.sh

# Function that changes static configuration in YAML
configure_print_color yellow


declare -g info
info="Hello world!"
# The function that redirects some message via global variable without saving it to YAML or using sockets or any other additional technologies by providing the name of the dynamic variable to the UI script.
ui_command.sh ui_print_info info

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a @m-iwanicki idea.

Copy link
Contributor

@m-iwanicki m-iwanicki Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are my ideas, heavily reworded (or rather it's your idea based on my input.)

Signed-off-by: Maciej Pijanowski <[email protected]>
@macpijan macpijan closed this Oct 29, 2025
@macpijan macpijan deleted the ui-lib branch October 29, 2025 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants