From 95a107dfe24602c8fe7a14b4a0b724813f3fd22b Mon Sep 17 00:00:00 2001 From: Nuru Date: Wed, 1 May 2024 23:03:32 -0700 Subject: [PATCH] Dark mode caching fix (#934) --- rootfs/etc/profile.d/_07-term-mode.sh | 22 +++-- rootfs/etc/profile.d/_10-colors.sh | 111 ++++++++++++++++++++++---- rootfs/etc/profile.d/prompt.sh | 2 +- rootfs/etc/profile.d/terraform.sh | 2 +- 4 files changed, 114 insertions(+), 23 deletions(-) diff --git a/rootfs/etc/profile.d/_07-term-mode.sh b/rootfs/etc/profile.d/_07-term-mode.sh index 5f974c9d..12a064f2 100644 --- a/rootfs/etc/profile.d/_07-term-mode.sh +++ b/rootfs/etc/profile.d/_07-term-mode.sh @@ -16,6 +16,7 @@ # Normally this function produces no output, but with -b, it outputs "true" or "false", # with -bb it outputs "true", "false", or "unknown". (Otherwise, unknown assume light mode.) +# With -m it outputs "dark" or "light", with -mm it outputs "dark", "light", or "unknown". # and always returns true. With -l it outputs integer luminance values for foreground # and background colors. With -ll it outputs labels on the luminance values as well. function _is_term_dark_mode() { @@ -35,9 +36,11 @@ function _is_term_dark_mode() { echo $(tput setaf 1)* TRACE: "Terminal did not respond to OSC 10 and 11 queries.$(tput sgr0)" >&2 fi # If we cannot determine the color scheme, we assume light mode for historical reasons. - if [[ "$*" =~ -b ]]; then - if [[ "$*" =~ -bb ]]; then + if [[ "$*" =~ -b ]] || [[ "$*" =~ -m ]]; then + if [[ "$*" =~ -bb ]] || [[ "$*" =~ -mm ]]; then echo "unknown" + elif [[ "$*" =~ -m ]]; then + echo "light" else echo "false" fi @@ -65,17 +68,22 @@ function _is_term_dark_mode() { fi # If the background luminance is less than the foreground luminance, we are in dark mode. if ((bg_lum < fg_lum)); then - if [[ "$*" =~ -b ]]; then + if [[ "$*" =~ -m ]]; then + echo "dark" + elif [[ "$*" =~ -b ]]; then echo "true" fi return 0 fi - # If we cannot determine the color scheme, we assume light mode for historical reasons. - if [[ "$*" =~ -b ]]; then + # Not in dark mode, must be in light mode. + if [[ "$*" =~ -m ]]; then + echo "light" + + elif [[ "$*" =~ -b ]]; then echo "false" - return 0 # when returning text, always return success + else + return 1 fi - return 1 } # Converting RGB to luminance is a lot more complex than summing the values. diff --git a/rootfs/etc/profile.d/_10-colors.sh b/rootfs/etc/profile.d/_10-colors.sh index 3b435a09..cebf9f62 100755 --- a/rootfs/etc/profile.d/_10-colors.sh +++ b/rootfs/etc/profile.d/_10-colors.sh @@ -10,16 +10,26 @@ # The main change is that it uses the terminal's default colors for foreground and background, # whereas the previous version "reset" the color by setting it to black, which fails in dark mode. -function update_terminal_mode() { - local dark_mode=$(_is_term_dark_mode -b) - if [[ ! -v _geodesic_tput_cache ]] || [[ "${_geodesic_tput_cache[dark_mode]}" != "$dark_mode" ]]; then - _geodesic_tput_cache_init +function update-terminal-mode() { + local new_mode="$1" + case $new_mode in + dark | light) ;; + + "") + new_mode=$(_is_term_dark_mode -m) + ;; + + *) + echo "Usage: update-terminal-mode [dark|light]" >&2 + return 1 + ;; + esac + + # See comments in _geodesic_color() below for why we test ${_geodesic_tput_cache@a} + if [[ ${_geodesic_tput_cache@a} != "A" ]] || [[ "${_geodesic_tput_cache[dark_mode]}" != "$new_mode" ]]; then + _geodesic_tput_cache_init "$1" else - local mode="light" - if [[ $dark_mode == "true" ]]; then - mode="dark" - fi - echo "Not updating terminal mode from $mode to $mode" + echo "Not updating terminal mode from $new_mode to $new_mode" fi } @@ -27,6 +37,15 @@ function update_terminal_mode() { function _geodesic_tput_cache_init() { declare -g -A _geodesic_tput_cache + # If we are here, we have lost the terminal mode settings. + # If we are not in a subshell, we are fixing them now. + # However, if we are in a subshell, we cannot fix them in the main shell + # from here, so we need to tell the user to run the command to fix them. + if [[ $BASH_SUBSHELL != 0 ]]; then + printf "\n* Terminal mode settings have been lost.\n" >&2 + printf "* Please run: update-terminal-mode \n\n" >&2 + fi + local color_off=$(tput op) # reset foreground and background colors to defaults local bold=$(tput bold) local bold_off @@ -52,7 +71,23 @@ function _geodesic_tput_cache_init() { [white]=$(tput setaf 7) ) - if _is_term_dark_mode; then + local new_mode="$1" + case $new_mode in + dark | light) ;; + + "") + new_mode=$(_is_term_dark_mode -m) + ;; + + *) + echo "Usage: _geodesic_tput_cache_init [dark|light]" >&2 + # Proceed with automatic detection to avoid + # repeated reinitializations. + new_mode=$(_is_term_dark_mode -m) + ;; + esac + + if [[ $new_mode == "dark" ]]; then _geodesic_tput_cache[black]=$(tput setaf 7) # swap black and white _geodesic_tput_cache[white]=$(tput setaf 0) # 0 is ANSI black, 7 is ANSI white _geodesic_tput_cache[blue]=${_geodesic_tput_cache[cyan]} # blue is too dark, use cyan instead @@ -87,7 +122,7 @@ function _geodesic_tput_cache_init() { # Save the terminal type so we can invalidate the cache if it changes _geodesic_tput_cache[TERM]="$TERM" - _geodesic_tput_cache[dark_mode]="$(_is_term_dark_mode -b)" + _geodesic_tput_cache[dark_mode]="$new_mode" } # Colorize text using ANSI escape codes. @@ -95,9 +130,57 @@ function _geodesic_tput_cache_init() { # `style` is defined by the keys of the associative array _geodesic_tput_cache set up above. # Not intended to be called directly. Use the named style functions below. function _geodesic_color() { - # The -v test is to see if the variable is set. - # It is required because the associative array syntax does not work with unset variables. - if [[ ! -v _geodesic_tput_cache ]] || [[ "${_geodesic_tput_cache[TERM]}" != "$TERM" ]]; then + ################################################################################################ + # + # Today's bash lesson regarding arrays, associative arrays, and the -v test. + # + # It is remarkably hard to safely test weather a variable has been declared as an associative array. + # + ### Background on indexing arrays in bash + # + # In bash, the expression `var[subscript]` has, unfortunately, two very different meanings, + # depending on whether `var` has been declared as an associative array or not. If `var` has + # NOT been declared an associative arry, then `subscript` is treated as an arithmetic expression. + # Within an expression, shell variables may be referenced by name without using the parameter + # expansion syntax, meaning `subscript` evaluates to `$subscript`, and the value of the variable + # `$subscript` is treated as an arithmetic expression (subject to recursive expansion), which is expected + # to yield a number. On the other hand, if `var` has been declared as an associative array, then + # `subscript` is treated as a string and is used to look up the value associated with that key, + # with no further interpretation of the string. + # + # This means that if `ary` has not been declared as an associative array, the expression + # `$ary[TERM]` causes $TERM to be evaluated as an arithmetic expression, which, through dumb luck, + # fails if $TERM is "xterm-256color" because the subscript expression is expanded like this: + # $TERM -> xterm-256color # the value of $TERM + # $xterm - 256color -> 0 - 256color # $xterm is not set, so it is treated as 0 + # ERROR converting "256color" to an integer: value too great for base + # + # If `TERM` had been set to `xterm-256` then the expression would have successfully evaluated to -256. + # + ### Testing to see if a variable has been declared + # + # In bash, the normal way to test if a variable is set is to use the -v test, as in [[ -v varname ]]. + # (Unlike the -z test, -v distinguishes between unset variables and variables set to the empty string.) + # However, with arrays, -v tests if the given index is set. If `ary` is an array, associative or not, + # [[ -v ary ]] is treated as [[ -v ary[0] ]] and is only true of that specific index (zero) has been assigned + # a value. So for our purposes, we cannot use -v to test if `_geodesic_tput_cache` has been declared + # but not initialized unless we do something like ensure that `_geodesic_tput_cache[0]` is always set. + # While it would be relatively easy to ensure that `_geodesic_tput_cache[0]` is always set, it would + # leave a very mistaken impression of what the [[ -v _geodesic_tput_cache ]] test is actually doing + # and why it works. Since this is open source code, we want to set a better example. + # + ### Testing to see if a variable has been declared as an associative array + # + # Starting with Bash version 5, parameter operators are available, in the form of ${parameter@operator}. + # The "a" operator expands the expression into a string consisting of flag values representing parameter’s attributes. + # For an associative array, the "a" operator expands to "A". For a plain array, it expands to "a". + # Note that other attributes are possible, too. For example, an exported non-associative array expands to "ax". + # This gives us a better solution. We can test if _geodesic_tput_cache has been declared as an associative + # array by using [[ ${_geodesic_tput_cache@a} == "A" ]]. By putting this test first, we can short-circuit + # the evaluation of `${_geodesic_tput_cache[TERM]}` and avoid the arithmetic evaluation error. + # + + if [[ ${_geodesic_tput_cache@a} != "A" ]] || [[ "${_geodesic_tput_cache[TERM]}" != "$TERM" ]]; then _geodesic_tput_cache_init fi diff --git a/rootfs/etc/profile.d/prompt.sh b/rootfs/etc/profile.d/prompt.sh index 1f66f2f2..3081ec5d 100755 --- a/rootfs/etc/profile.d/prompt.sh +++ b/rootfs/etc/profile.d/prompt.sh @@ -149,7 +149,7 @@ function geodesic_prompt() { fi dir_prompt+=$'${GEODESIC_PROMPT_GLYPHS-${BLACK_RIGHTWARDS_ARROWHEAD} }' - update_terraform_prompt + _update_terraform_prompt local old_kube_ps1_prefix="$KUBE_PS1_PREFIX" KUBE_PS1_PREFIX="(" local tf_prompt diff --git a/rootfs/etc/profile.d/terraform.sh b/rootfs/etc/profile.d/terraform.sh index dee0faa9..5a9c3f30 100755 --- a/rootfs/etc/profile.d/terraform.sh +++ b/rootfs/etc/profile.d/terraform.sh @@ -1,4 +1,4 @@ -function update_terraform_prompt() { +function _update_terraform_prompt() { if [[ ${GEODESIC_TF_PROMPT_ENABLED:-false} == "true" ]]; then # Test if there are any files with names ending in ".tf" if compgen -G '*.tf' >/dev/null; then