5
\$\begingroup\$

This question is a direct follow-up #1 of my previous question:

Lightshot Print Screen key linux handler


There were too many errors which I didn't see at the moment and I tried hard to fix them up now. I would like someone else (than myself) to actually review the code this time.


#!/bin/sh

# treat unset variables as an error when substituting
set -o nounset

#------------------------------------------------------------------------------

# global constants for an easy set-up
#
# lightshot_printscreen_hotkey: set this to the same hotkey which you have set up in Lightshot
#                               example: for left control and print screen key -> type Control_L+Print
lightshot_printscreen_hotkey="Print"

# lightshot_process_name: no need to change this one; it is a case-sensitive name of the Lightshot process
lightshot_process_name="Lightshot"

#------------------------------------------------------------------------------

print_error_and_exit()
# expected arguments:
# $1 = exit code
# $2 = error origin; usually function name
# $3 = error message
{
    # colors definitions
    bold=$(tput bold)
    red=$(tput setaf 1)
    yellow=$(tput setaf 3)
    nocolor=$(tput sgr0)
    bold_red="$bold$red"
    bold_yellow="$bold$yellow"

    # check if exactly 3 arguments have been passed
    # if not, print out an internal error without colors
    if [ "$#" -ne 3 ]
    then
        printf "print_error_and_exit() internal error\\n\\n\\tThere has been passed a wrong number of arguments (%b)! Expected 3:\\n\\t\\t1 - exit code\\n\\t\\t2 - error origin\\n\\t\\t3 - error message\\n\\nexit code = 2\\n" "$#" 1>&2
        exit 2
    fi

    # check if the first argument is a number
    # if not, print out an internal error without colors
    if ! [ "$1" -eq "$1" ] 2> /dev/null
    then
        printf "print_error_and_exit() internal error\\n\\n\\tThere has been passed the first argument as not a number (%b)!\\n\\tExpected an exit code.\\n\\nexit code = 2\\n" "$1" 1>&2
        exit 2
    fi

    # check if we have color support
    if [ -x /usr/bin/tput ] && tput setaf 1 > /dev/null 2>&1
    then
        # here we do have color support, so we highlight the error origin and the exit code
        printf "%b%b()\\n\\n\\t%b%b%b\\n\\nexit code = %b%b\\n" "$bold_yellow" "$2" "$nocolor" "$3" "$bold_red" "$1" "$nocolor" 1>&2
        exit "$1"
    else
        printf "%b()\\n\\n\\t%b\\n\\nexit code = %b\\n" "$2" "$3" "$1" 1>&2
        exit "$1"
    fi
}

#------------------------------------------------------------------------------

# expected arguments to the script: none
# check if no arguments have been passed to the script
[ "$#" -gt 0 ] && print_error_and_exit 1 "$0" "You have passed $# unexpected argument(s) to the script!\\n\\tNo arguments expected."

#------------------------------------------------------------------------------

check_for_prerequisite()
# expected arguments:
# $1 = program name
{
    # check if exactly one argument has been passed
    [ "$#" -eq 1 ] || print_error_and_exit 3 "check_for_prerequisite" "There has not been passed exactly one argument!\\n\\tA program name expected."

    # check if the argument is a program which is installed
    command -v "$1" > /dev/null 2>&1 || print_error_and_exit 4 "check_for_prerequisite" "I require $1 but it is not installed! Please install it."
}

#------------------------------------------------------------------------------

# check for prerequisites
check_for_prerequisite "pgrep"
check_for_prerequisite "xdotool"

#------------------------------------------------------------------------------

get_process_id_using_process_name()
# expected arguments:
# $1 = process name
{
    # check if exactly one argument has been passed
    [ "$#" -eq 1 ] || print_error_and_exit 5 "get_process_id_using_process_name" "There has not been passed exactly one argument!\\n\\tA process name expected."

    # try to get the process id using the process name
    pgrep "$1"
}

#------------------------------------------------------------------------------

is_number()
# expected arguments:
# $1 = any variable
{
    # check if exactly one argument has been passed
    [ "$#" -eq 1 ] || print_error_and_exit 6 "is_number" "There has not been passed exactly one argument!\\n\\tOne variable to test expected."

    # check if the argument is an integer number 
    [ "$1" -eq "$1" ] 2> /dev/null
}

#------------------------------------------------------------------------------

# try to get the Lightshot process id
lightshot_process_id=$(get_process_id_using_process_name "$lightshot_process_name")

# test if a process id has been successfully acquired
is_number "$lightshot_process_id" || print_error_and_exit 7 "lightshot_process_id" "The argument is not a number!\\n\\tLightshot is most probably not running."

#------------------------------------------------------------------------------

# get the window id from the Lightshot process id
    #--all   : Require that all conditions be met.
    #--limit : Stop searching after finding N matching windows.
    #--pid   : Match windows that belong to a specific process id.
    #--name  : Match against the window name. This is the same string that is displayed in the window titlebar.
lightshot_window_id=$(xdotool search --all --limit 1 --pid "$lightshot_process_id" --name "$lightshot_process_name")

# test if a window id has been successfully acquired
is_number "$lightshot_window_id" || print_error_and_exit 8 "lightshot_window_id" "The argument is not a number!\\n\\tLightshot is most probably not running."

#------------------------------------------------------------------------------

# simulate the above pre-defined print screen hotkey press on the Lightshot window
xdotool key --window "$lightshot_window_id" "$lightshot_printscreen_hotkey"
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Long lines are hard to read

This is hard to read:

printf "print_error_and_exit() internal error\\n\\n\\tThere has been passed a wrong number of arguments (%b)! Expected 3:\\n\\t\\t1 - exit code\\n\\t\\t2 - error origin\\n\\t\\t3 - error message\\n\\nexit code = 2\\n" "$#" 1>&2

It would be better to split this to multiple echo statements. When you do that, write in a way to not use any flags of echo, which are not portable. The result will be easier to read, and POSIX compliant.

When printing multiple lines, sometimes it's easier to use a here-document.

To redirect the output of multiple commands, you can group them within { ... }:

{
    echo "foo"
    echo "bar"
} >/dev/null 2>&1

Checking if a command exists

The recommended way to check if a command exists seems to be using command -v name. Using -x /usr/bin/tput imposes a stronger constraint by requiring the program at a specific location. That seems unnecessary.

Contradiction

print_error_and_exit uses tput to compute colors, and later checks if tput is available. That doesn't make sense.

Avoid unnecessary execution

The print_error_and_exit computes colors by executing tput, but then it may exit before actually using those values. So the tput calls should be moved down, to execute only when you know for sure that the results will be useful.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.