Dark mode caching fix (#934)

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.

rootfs/etc/profile.d/_10-colors.sh

@@ -10,23 +10,42 @@
 # 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
 }
 
 # We call `tput` several times for every prompt, and it can add up, so we cache the results.
 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,17 +122,65 @@ 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.
 # Usage: _geodesic_color style text...
 # `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
 

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

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