From mboxrd@z Thu Jan  1 00:00:00 1970
From: Michael Tremer <michael.tremer@ipfire.org>
To: network@lists.ipfire.org
Subject: Re: [PATCH v2 2/5] color: add colors to zone and ports
Date: Mon, 19 Jun 2017 12:19:07 +0100
Message-ID: <1497871147.21214.99.camel@ipfire.org>
In-Reply-To: <1497543533-5925-2-git-send-email-jonatan.schlag@ipfire.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============3138334767386307651=="
List-Id: <network.lists.ipfire.org>

--===============3138334767386307651==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

Hi,

there is a problem with the color_cli() function:

On Thu, 2017-06-15 at 18:18 +0200, Jonatan Schlag wrote:
> Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
> ---
> =C2=A0src/functions/functions.colors | 192
> +++++++++++++++++++++++++++++++++++++++++
> =C2=A0src/functions/functions.ports=C2=A0=C2=A0|=C2=A0=C2=A0=C2=A08 ++
> =C2=A0src/functions/functions.zone=C2=A0=C2=A0=C2=A0|=C2=A0=C2=A0=C2=A08 ++
> =C2=A03 files changed, 208 insertions(+)
>=20
> diff --git a/src/functions/functions.colors b/src/functions/functions.colors
> index 8d7193c..433ce78 100644
> --- a/src/functions/functions.colors
> +++ b/src/functions/functions.colors
> @@ -73,3 +73,195 @@ MSG_STP_DISCARDING=3D"${CLR_RED_BG}${CLR_WHITE_B} DISCA=
RDING
> ${CLR_RESET}"
> =C2=A0MSG_STP_LEARNING=3D"${CLR_YELLOW_BG}${CLR_WHITE_B}=C2=A0=C2=A0LEARNIN=
G=C2=A0=C2=A0=C2=A0${CLR_RESET}"
> =C2=A0MSG_STP_LISTENING=3D"${CLR_YELLOW_BG}${CLR_WHITE_B}=C2=A0=C2=A0LISTEN=
ING=C2=A0=C2=A0${CLR_RESET}"
> =C2=A0MSG_STP_BLOCKING=3D"${CLR_RED_BG}${CLR_WHITE_B}=C2=A0=C2=A0BLOCKING=
=C2=A0=C2=A0=C2=A0${CLR_RESET}"
> +
> +color_cli() {
> +	#Is the cli function to parse the options submitted by a user.
> +	local type=3D${1}
> +	local name=3D${2}
> +	local action=3D${3}
> +	shift 3

Here you shift the arguments...

> +
> +	case ${action} in
> +		set)
> +			local color=3D${4}

And here you access the fourth one which has been shifted to ${1} by calling
shift 3 earlier.

So either drop the shift (preferred), or change the argument to ${1} here.

> +			# Check if we get to many arguments
> +			shift 1

This is shift is also unnecessary. You could just check for > 1 below.

> +			if [ $# -gt 0 ]; then
> +				error "Too many arguments: $@"
> +				return ${EXIT_ERROR}
> +			fi
> +			color_set ${type} ${name} ${@}
> +			;;
> +		reset)
> +			# We set the color to white.
> +			# Check if we get to many arguments
> +			shift
> +			if [ $# -gt 0 ]; then

Same as above.

> +				error "Too many arguments: $@"
> +				return ${EXIT_ERROR}
> +			fi
> +			color_set ${type} ${name} "ffffff"
> +			;;
> +		*)
> +			error "Invalid argument: ${action}"
> +			;;
> +	esac
> +
> +}
> +
> +color_set() {
> +	#Write a given color into the color config file of a zone or port.
> +	assert [ $# -eq 3 ]
> +
> +	local type=3D${1}
> +	local name=3D${2}
> +	local COLOR=3D${3}
> +	# Check if we get to many arguments
> +	# Check if the color code is valid
> +	if ! color_hex_is_valid ${COLOR}; then
> +		error "Hexadecimal color code '${COLOR}' is not valid"
> +		return ${EXIT_ERROR}
> +	fi
> +
> +	local file=3D$(color_format_filename ${type} ${name})
> +	settings_write ${file} COLOR
> +}
> +
> +color_read() {
> +	#Read a color out of color config file of a zone or port.
> +	#If this is unsuccessful we use white.
> +	local type=3D${1}
> +	local name=3D${2}
> +
> +	local file=3D$(color_format_filename ${type} ${name})
> +
> +	local COLOR
> +
> +	if ! settings_read ${file} COLOR; then
> +		COLOR=3D"ffffff"
> +	fi
> +
> +	print "${COLOR}"
> +}
> +
> +color_format_filename() {
> +	#Formats the color config file name.
> +	local type=3D${1}
> +	local name=3D${2}
> +	case ${type} in
> +		zone)
> +			echo "$(zone_dir ${name})/color"
> +			;;
> +		port)
> +			echo "$(port_dir ${name})/color"
> +			;;
> +	esac
> +}
> +
> +color_hex_is_valid() {
> +	#Check if a color hex is valid.
> +	[[ ${1} =3D~ ^[0-9a-fA-F]{6}$ ]]
> +}
> +
> +color_hex2rgb() {
> +	#Converts a color hex into rgb values.
> +	local hex=3D${1}
> +
> +	assert [ ${#hex} -eq 6 ]
> +
> +	for (( i =3D 0; i < 6; i +=3D 2 )); do
> +		hex2dec ${hex:${i}:2}
> +	done | tr '\n' ' '
> +
> +	print # newline
> +}
> +
> +_find_nearest_rgb_value() {
> +	# For the calculation of the xterm value the rgb values must be:
> +	# 0; 95; 135; 175; 215; 255;
> +	# this function find the closest value of these 6 numbers for a give
> rgb number
> +	local rgb=3D${1}
> +
> +	local best_value
> +	local best_value_index
> +
> +	local values=3D( 0 95 135 175 215 255 )
> +	local result
> +	local i=3D0
> +
> +	local value
> +	for value in ${values[@]}; do
> +		result=3D$(( ${value} - ${rgb} ))
> +		result=3D$(abs ${result})
> +
> +		if [ -z ${best_value} ]; then
> +			best_value=3D${result}
> +			best_value_index=3D${i}
> +
> +		# In the first iteration best_value is empty and so set to
> ${result}
> +		# two lines above. So if statement must use -le because in
> the first iteration
> +		# is the best_value eqal to result
> +		elif [ ${result} -le ${best_value} ]; then
> +			best_value=3D${result}
> +			best_value_index=3D${i}
> +		fi
> +
> +		(( i++ ))
> +	done
> +
> +	echo "${best_value_index}"
> +}
> +
> +color_rgb2shell() {
> +	#Converts a rgb value triple into an xterm color code.
> +	assert [ $# -eq 3 ]
> +
> +	local red=3D${1}
> +	local green=3D${2}
> +	local blue=3D${3}
> +
> +	local color
> +	for color in red green blue; do
> +		printf -v "${color}" $(_find_nearest_rgb_value ${!color})
> +	done
> +
> +	print $(( 16 + 36 * ${red} + 6 * ${green} + ${blue} ))
> +}
> +
> +color_set_shell() {
> +	#Set the shell color which unfourtunately does not work for putty.
> +	local where=3D${1}
> +	local color=3D${2}
> +
> +	local prefix
> +	case "${where}" in
> +		fg)
> +			prefix=3D"\e[38"
> +			;;
> +		bg)
> +			prefix=3D"\e[48"
> +			;;
> +	esac
> +
> +	# Convert color from hex to RGB
> +	local red green blue
> +	read red green blue <<< $(color_hex2rgb ${color})
> +
> +	# Set standard shell color
> +	local shell_color=3D$(color_rgb2shell ${red} ${green} ${blue})
> +	printf "${prefix};5;${shell_color}m"
> +
> +	# For shells that support it, we will try to set the RGB color code
> +	case "${TERM}" in
> +		putty*)
> +			# PuTTY is a piece of garbage and does not know
> +			# how to handle colors at all although it has nice
> +			# checkboxes to enable them, but they actually make
> +			# things even worse. So no colors for you Windows
> +			# users.
> +			;;
> +		*)
> +			printf "${prefix};2;${red};${green};${blue}m"
> +			;;
> +	esac
> +}
> diff --git a/src/functions/functions.ports b/src/functions/functions.ports
> index c6e45d0..94fc68b 100644
> --- a/src/functions/functions.ports
> +++ b/src/functions/functions.ports
> @@ -422,3 +422,11 @@ ports_lowest_address() {
> =C2=A0port_identify() {
> =C2=A0	device_identify $@
> =C2=A0}
> +
> +port_get_color() {
> +	# This function return the color of a port
> +	assert [ $# -eq 1 ]
> +
> +	local name=3D${1}
> +	echo $(color_read "port" ${name})

You could just call "color_read port ${name}" here without the echo and the
$(...) since color_read already prints the result. This would save us calling=
 a
subshell and would also make port_get_color return the exit code of color_rea=
d.

> +}
> diff --git a/src/functions/functions.zone b/src/functions/functions.zone
> index 88c81a8..68d4ab6 100644
> --- a/src/functions/functions.zone
> +++ b/src/functions/functions.zone
> @@ -1200,3 +1200,11 @@ zone_port_settings_remove() {
> =C2=A0	local path=3D"$(zone_dir "${zone}")/ports/${port}"
> =C2=A0	settings_remove "${path}"
> =C2=A0}
> +
> +zone_get_color() {
> +	# This function return the color of a zone
> +	assert [ $# -eq 1 ]
> +
> +	local name=3D${1}
> +	echo $(color_read "zone" ${name})
> +}

Likewise.

--===============3138334767386307651==
Content-Type: application/pgp-signature
Content-Transfer-Encoding: base64
Content-Disposition: attachment; filename="signature.asc"
MIME-Version: 1.0

LS0tLS1CRUdJTiBQR1AgU0lHTkFUVVJFLS0tLS0KVmVyc2lvbjogR251UEcgdjIKCmlRSWNCQUFC
Q2dBR0JRSlpSN01zQUFvSkVJQjU4UDl2a0FrSGZoMFAvMFdsUjNNbUcxd0tXWExzMWQ3a3A0R1QK
S3NqWDJCOTl3eFdNenVFU1lhR3c4YjkrOUREdXdPeVp0ZUtaN0E3YTJwc3RqUk5qSUVvWDBFMVZV
b3AvQkVrdApIMFpncm04QTN2RUFBbVRUMEZaaHBkSFV0RThmbGFqdFZSUFNsNW9JQWxnTXJCVm9x
bDJ6MHh6cWJOVisrKzlFClRFRjk5QXBsTzg1S0RreXpid3RydzlRRFZVSU43M3RlNUZ2Szh0ZkhU
TVR4YmxOQzFJdjdpU2h2cGMvSDAyVzQKK0J1QWZGR2UvVkxTMXRYQ24wbEJIak82TFdjeTV2d2ph
ZGZkWjBhanl4TUV2bDJHek50ZnRScXViUjVDRTlBQgo4RkZBd3JtREhLUURvVW5NbGFHN2ZIOTJ2
eW85WFZQb3A1TGlnMks1Um52UTByd2ZDeHpDbzNHZXFMVS9mbjhKCkxjWkliWnJiNDF1di96SzZN
M0Eya2ViNExwR3N0akJ3RGN5ZkFCdWpHZmZOclpLcHA0SzdheVV0c1dUVTVJZFQKclZXOTN0b2xS
SDl2VjB3K1dmT0JUbS9NR1p6ZjdBOWY1VlhhZmIrYjgyZittVHRnenpsSndtQzNJamJRMEpBLwpw
QlNRMllHeEVPM05CNlRNOURiYXN0ZXhiWmloZ1BlTTdLcmNPVjNTQUN3c3B6alRVSGxWSXZMQ3Za
VkJoalR4CkhreVhKNEtFTjJqQ3RiUGpza3M3MjZUMkxsMVhWMkdKVkU1aWZQRGE1bVZhUkZsT3cz
dk52bGZiRTRndHp6b1cKVTRpSGRwRHJEQ0t1THgxVTRPZ1ExNjByWGRMSmYyZHZtc3RDU2wwM2pT
eStBWGFjOTgwUXM2d245eFY5NTdVOApxdGJ0VTUwK1NITHNEN1dIRGF5NQo9RzVrdAotLS0tLUVO
RCBQR1AgU0lHTkFUVVJFLS0tLS0K

--===============3138334767386307651==--