Instruct shell how to treat unset variables
set -o nounset
In this script, there should not be possible to encounter an unbound variable, so setting this will prove useful for debugging.
Redirection style
9 test "$1" -eq "$1" 2>/dev/null
in contrast with:
23 if test -x /usr/bin/tput && tput setaf 1 > /dev/null 2>&1
It should be unified either with, or without a space.
Constants should be moved to the top
61 lightshot_key="Print"
62 lightshot_name="Lightshot"
It is obvious, that everyone might not want to use Print, so this really has to be moved to the top in order for each user to quickly customize the keyboard combination for the print screen.
Those constants should be commented and clarified
In order for the users to quickly know, how to define key combinations, there should be some example added.
Consider replacing echo
with printf
28 echo "$bold$red$2 Exit code = $1.$nocolor" 1>&2
29 else
30 echo "$2 Exit code = $1." 1>&2
Basically, it's a portability (and reliability) issue. Read more about this topic in this answer.
Problematic code
I notice now, there is a new line inside these strings:
70 is_number "$lightshot_pid" || print_error_and_exit 7 "lightshot_pid: The argument is not a number!\\nLightshot is most probably not running."
78 is_number "$lightshot_wnd" || print_error_and_exit 8 "lightshot_wnd: The argument is not a number!\\nLightshot is most probably not running."
Which according to Shellcheck.net:
echo
won't expand escape sequences
Read more about this topic in this article.
This code, as it is, works though, in spite of this error. Tested in bash
version 4.3.48(1)-release and POSIX dash
version 0.5.8-2.1ubuntu2.
Better variable naming convention
These are of little value to a foreign script reader:
61 lightshot_key="Print"
62 lightshot_name="Lightshot"
There are only two constants. Why not to give them some real meaning, this would further enhance the script reader's experience.
The variables:
67 lightshot_pid
75 lightshot_wnd
could use some love too.
Tell the user what arguments are (not) expected
50 test "$#" -gt 0 && print_error_and_exit 1 "$0: You have passed some unexpected argument(s) to the script!"
Let's add there something like:
No arguments expected!
Problematic space in the comments (block separators)
12 # ------------------------------------------------------------------------------
There should be no space between #
and -
, because imagine, that you have a larger project than this, having these block separators will prevent you from searching for long arguments beginning (--
).
Re-consider using smileys in the output and using grammar shortcuts
44 command -v "$1" > /dev/null 2>&1 || print_error_and_exit 6 "check_for_prerequisite(): I require $1 but it's not installed :-("
It may look a little unprofessional. Hard to say without knowing the people reading your script, though. (It might be totally OK for personal usage.) This script will likely be spread across the internet, so better not use that. Also, avoid using grammar shortcuts like:
it's
xdotool
: documenting all passed arguments would be greatly appreciated by the script readers (and the order of arguments in lightshot_wnd
could be better a little)
75 lightshot_wnd=$(xdotool search --limit 1 --all --pid "$lightshot_pid" --name "$lightshot_name")
Re-writen code would look as follows
#!/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: do not change this one, it is a case-sensitive name of the Lightshot process
lightshot_process_name="Lightshot"
#------------------------------------------------------------------------------
is_number()
{
# check if exactly one argument has been passed
test "$#" -eq 1 || print_error_and_exit 5 "is_number(): There has not been passed exactly one argument!"
# check if the argument is an integer
test "$1" -eq "$1" 2> /dev/null
}
#------------------------------------------------------------------------------
print_error_and_exit()
{
# check if exactly two arguments have been passed
test "$#" -eq 2 || print_error_and_exit 3 "print_error_and_exit(): There have not been passed exactly two arguments!"
# check if the first argument is a number
is_number "$1" || print_error_and_exit 4 "print_error_and_exit(): The argument #1 is not a number!"
# check if we have color support
if test -x /usr/bin/tput && tput setaf 1 > /dev/null 2>&1
then
bold=$(tput bold)
red=$(tput setaf 1)
nocolor=$(tput sgr0)
printf "%s%s%s Exit code = %s.%s\\n" "$bold" "$red" "$2" "$1" "$nocolor" 1>&2
else
printf "%s Exit code = %s.\\n" "$2" "$1" 1>&2
fi
exit "$1"
}
#------------------------------------------------------------------------------
check_for_prerequisite()
{
# check if exactly one argument has been passed
test "$#" -eq 1 || print_error_and_exit 2 "check_for_prerequisite(): There has not been passed exactly one argument!"
# check if the argument is a program which is installed
command -v "$1" > /dev/null 2>&1 || print_error_and_exit 6 "check_for_prerequisite(): I require $1 but it is not installed!"
}
#------------------------------------------------------------------------------
# check if no arguments have been passed to the script
test "$#" -gt 0 && print_error_and_exit 1 "$0: You have passed some unexpected argument(s) to the script. No arguments expected!"
#------------------------------------------------------------------------------
# check for prerequisites
check_for_prerequisite "pgrep"
check_for_prerequisite "xdotool"
#------------------------------------------------------------------------------
# get the Lightshot process id
lightshot_process_id=$(pgrep "$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! Lightshot 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! Lightshot 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"