-
Notifications
You must be signed in to change notification settings - Fork 0
DTS changes #1
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
base: main
Are you sure you want to change the base?
DTS changes #1
Conversation
| * `TUI_CONFIG_FILE` in `tui-lib.sh` is unused | ||
| * We are loading YAML config with `yq`, converting it to JSON and then using | ||
| `jq` to parse it. Use YAML directly. | ||
| * Related to previous point, functions for parsing this config are unreadable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Parsing YAML should be easy to change and does not impact the general architecture.
| * We are loading YAML config with `yq`, converting it to JSON and then using | ||
| `jq` to parse it. Use YAML directly. | ||
| * Related to previous point, functions for parsing this config are unreadable | ||
| * Callbacks have to be full path to file, can't use commands |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we are gathering some requirements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we consider this a requirement to be able to use either command or script as a callback? Or maybe also a function?
Or maybe having too much flexibility and options would lead us to convoluted solutions, and executing each action as another script (even if it uses only one comand) is fine? Or we should decide on one of these two:
- script as a callback
- command as a callback (command in the end can call another script, so maybe this one is more flexible, and in the end it's basically 2 in 1).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would consider using arguments for the commands in the config files as well.
| * callback can't have arguments | ||
| * conditions can only be variables | ||
| * `tui_handle_input` | ||
| - `# Hidden option: Q to quit (useful for testing)` - I am very apprehensive |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was on my request to quickly shutdown when testing demo and have the same options as current DTS. Can easily be dropped.
| * Callbacks have to be full path to file, can't use commands | ||
| * Why are we exporting functions at least ones that likely shouldn't be used by | ||
| backend like `tui_stop` | ||
| * We should split `tui-lib.sh` into lib that should be used by scripts to print |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of the same.
Don't wait for key press when using footer options Signed-off-by: Michał Iwanicki <michal.iwanicki@3mdeb.com>
Signed-off-by: Michał Iwanicki <michal.iwanicki@3mdeb.com>
Signed-off-by: Michał Iwanicki <michal.iwanicki@3mdeb.com>
| `jq` to parse it. Use YAML directly. | ||
| * Related to previous point, functions for parsing this config are unreadable | ||
| * Callbacks have to be full path to file, can't use commands | ||
| * Why are we exporting functions at least ones that likely shouldn't be used by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once we have clear expectations on what is used and how, it can/should be limited.
| backend like `tui_stop` | ||
| * We should split `tui-lib.sh` into lib that should be used by scripts to print | ||
| something, and core lib used to run menu. | ||
| * Add input parsing in menu to disallow multiline strings (or deal with them |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you give an example here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
diff --git a/examples/demo.sh b/examples/demo.sh
index 1920b5d429b6..3ecf767a8d3a 100755
--- a/examples/demo.sh
+++ b/examples/demo.sh
@@ -15,3 +15,6 @@ export BASEBOARD_INFO="Emulation QEMU x86 q35/ich9"
export CPU_INFO="Intel Core Processor (Skylake)"
-export RAM_INFO="Not Specified"
+export RAM_INFO="RAM1: 2 GB
+RAM2: 2GB
+RAM3: None
+RAM4: None"
export BIOS_INFO="3mdeb Dasharo (coreboot+UEFI) v0.2.1-rc1"************************************************************
** HARDWARE INFORMATION
************************************************************
** System Inf.: Emulation QEMU x86 q35/ich9
** Baseboard Inf.: Emulation QEMU x86 q35/ich9
** CPU Inf.: Intel Core Processor (Skylake)
** RAM Virtual: RAM1: 2 GB
RAM2: 2GB
RAM3: None
RAM4: None
************************************************************
Same with strings that are too long, they'll either move outside of border or wrap around
| something, and core lib used to run menu. | ||
| * Add input parsing in menu to disallow multiline strings (or deal with them | ||
| correctly) | ||
| * Add right border (and handle too long strings) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add right border
You mean * on the right as well?
(and handle too long strings)
Handle how? Wrap? Prevent?
| * Add input parsing in menu to disallow multiline strings (or deal with them | ||
| correctly) | ||
| * Add right border (and handle too long strings) | ||
| * callback can't have arguments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we probably need option 2 from: https://github.com/3mdeb/tui-sh/pull/1/files#r2471258244 and also arguments handling?
| correctly) | ||
| * Add right border (and handle too long strings) | ||
| * callback can't have arguments | ||
| * conditions can only be variables |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really want to put more complex shell expressions in the YAML? Can you give some examples?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return value of a command
| - I thought this function was to be used by other scripts, but it shouldn't | ||
| as this function is to handle menu input. We really need to separate core | ||
| functions, internal to the UI. | ||
| * Missing various input, prompt, choice, etc. functions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was not the point of this lib at this point. The point was main menu drawing. These functions could be added to this second library. Do you have a complete list of the input/prompt/choice primitives we would need based on your integration experiment?
| as this function is to handle menu input. We really need to separate core | ||
| functions, internal to the UI. | ||
| * Missing various input, prompt, choice, etc. functions | ||
| * We can only print lines (with newline at the end), if we want to print |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we phrase it differently: what function is missing, or what kind of change is needed here?
| print on screen and functions that should be used to e.g. pass strings | ||
| between functions, variables etc. In the future, we could make sure that | ||
| we only show text that is printed via those screen printing functions | ||
| * Find better way to do submenus - in DTS menu was generated dynamically so I |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any ideas here? Does it mean static YAML is not well fit for us? Or maybe the menu creation logic in DTS is overcomplicated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To enter submenu we have to create separate script that will call tui_run. Not sure if that's how we want to do it
| * Find better way to do submenus - in DTS menu was generated dynamically so I | ||
| created temporary `yaml` and ran `tui_run <path_to_new_config>"` | ||
| * `Enter an option:|` cursor is too close (add space) `Enter an option: |` | ||
| * Allow using commands instead of shell variables? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is the same as: https://github.com/3mdeb/tui-sh/pull/1/files#r2471267205 I guess
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that was related to information sections. E.g. something like:
https://github.com/iwanicki92/tui-test/blob/ae7759d64196d01f0e7ee9f3a97630eca60daaf3/config.yml#L18
We call the function, format the output (e.g. get only first line, trim if too long, e.t.c.) and print on screen
| created temporary `yaml` and ran `tui_run <path_to_new_config>"` | ||
| * `Enter an option:|` cursor is too close (add space) `Enter an option: |` | ||
| * Allow using commands instead of shell variables? | ||
| * Missing utility functions related to e.g. asking for user choice e.g.: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same as: https://github.com/3mdeb/tui-sh/pull/1/files#r2471269337 ? But I appreciate how much more concrete this one is.
| * Allow using commands instead of shell variables? | ||
| * Missing utility functions related to e.g. asking for user choice e.g.: | ||
| <https://github.com/Dasharo/dts-scripts/blob/7b43513360816fc2171161b39c2a4bc79f88f487/include/dts-functions.sh#L1921> | ||
| * Feature: we could force usage of TUI printing functions to print anything on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as 10 lines above?
| * Feature: we could force usage of TUI printing functions to print anything on | ||
| screen. Usage of `echo` wouldn't print anything, and could be used to pass | ||
| strings between functions in shell script. | ||
| * DTS related: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to convert these into any specific tasks to be done on the DTS side?
DaniilKl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor notes.
| readonly TUI_NORMAL='\033[0m' | ||
| readonly TUI_RED='\033[0;31m' | ||
| readonly TUI_GREEN='\033[0;32m' | ||
| readonly TUI_YELLOW='\033[0;33m' | ||
| readonly TUI_BLUE='\033[0;36m' # Cyan, used for borders (matches DTS BLUE) | ||
| TUI_NORMAL='\033[0m' | ||
| TUI_RED='\033[0;31m' | ||
| TUI_GREEN='\033[0;32m' | ||
| TUI_YELLOW='\033[0;33m' | ||
| TUI_BLUE='\033[0;36m' # Cyan, used for borders (matches DTS BLUE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why have you deleted readonly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warnings when sourcing this file multiple times
| TUI_BLUE='\033[0;36m' # Cyan, used for borders (matches DTS BLUE) | ||
|
|
||
| # Terminal width configuration | ||
| readonly TUI_MAX_WIDTH=60 # Maximum width for borders and footer wrapping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why have you deleted readonly?
| declare -A TUI_PRE_RENDER_CALLBACKS=() | ||
| declare -A TUI_POST_RENDER_CALLBACKS=() | ||
| # used to call callbacks in the same order as they were registered | ||
| TUI_PRE_RENDER_CALLBACKS_ORDER=() | ||
| TUI_POST_RENDER_CALLBACKS_ORDER=() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initializing arrays with empty values two times, why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mistake
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wasn't a mistake, those are different arrays
TUI_PRE_RENDER_CALLBACKS- associativeTUI_PRE_RENDER_CALLBACKS_ORDER- normal array, to keep callback insertion order
| local answer | ||
| echo -n "${prompt}: " >&2 | ||
| read -r answer | ||
| echo "${answer}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the answer is being echo'ed?`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it can be saved to variable (echoed to stdout). User facing output is outputted to stderr (same as read does when echoing what you are writing)
| * We are loading YAML config with `yq`, converting it to JSON and then using | ||
| `jq` to parse it. Use YAML directly. | ||
| * Related to previous point, functions for parsing this config are unreadable | ||
| * Callbacks have to be full path to file, can't use commands |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would consider using arguments for the commands in the config files as well.
Related: Dasharo/dts-scripts#121