3
\$\begingroup\$

I would like to ask for a final review of my Lightshot print screen Linux handler POSIX shell script.

When I started writing it, it was a one-liner (shell snippet), but then I decided to grow it, and learn during that time while at it. It's been a wonderful journey, but all has to reach an end. And so this will be my last ever review of this script.

With your help I hope, that is. I attach the whole code as it is now before publishing.

PS: Give yourself time with reviewing it. The more notes I get, the better the final release could be. There is no deadline or anything alike. Thank you all and good hunting!

In case you need anything from description up to usage of this small script, look at the project on GitHub (direct link). I gave it full details there.


#!/bin/sh

#--------------------------------------------------------------------------------
#--                    Lightshot print screen Linux handler                    --
#--                     Git: https://burian.work/lightshot                     --
#--                             Code: POSIX shell                              --
#--                                License: MIT                                --
#--                                Version: 9.1                                --
#--                          Released on: 2025-Oct-01                          --
#--                      E-mail: [email protected]                       --
#--                    Copyright 2018-2025 Vlastimil Burian                    --
#--------------------------------------------------------------------------------

# The hotkey variable is on top for easy set-up; overridable with -k switch.
# Run this script with -h for complete help to this script with an example.
# Just run this script to test out, it will print out if Lightshot was found.
# Set this to the same hotkey which you have set up in your Lightshot options.
# For the left control key and the print screen key put in Control_L+Print.
hotkey=Print

# Treat unset variables as errors when substituting.
# Clarification: this means that accessing any unassigned variable
# produces immediate termination of the script with an error printed.
set -o nounset

# (bool) Function to test a variable for an integer.
is_int()
{
    case "${1#[+-]}" in
        (*[!0123456789]*) return 1 ;;
        ('')              return 1 ;;
        (*)               return 0 ;;
    esac
}

# (bool) Function to test a variable for an unsigned integer.
is_uint()
{
    case "$1" in
        ([+-]*) return 1 ;;
    esac

    is_int "$1"
}

# (bool) Function to test a variable for a "safe" exit code:
# "Safe" exit code must be an unsigned integer from the range of <0;125> which is considered
# the safe range, the remaining range of <126;255> is reserved or otherwise not recommended.
is_safe_exit()
{
    is_uint "$1" && [ "$1" -le 125 ]
}

# (string) Function to detect basic variable types passed to my functions:
# empty, integer, file (with specific info), the rest is considered string.
dump_args()
{
    # we do not write anything for 0 arguments
    [ "$#" -eq 0 ] && return 1
    exec >&2
    i=1
    printf '%s\n' \
        "dump_args()" \
        "$# arguments inspected:"
    while [ "$#" -gt 0 ]; do
        printf "[%d]: '%s' " "$i" "$1"
        if [ -z "$1" ]; then
            printf '%s' "(empty)"
        elif is_int "$1"; then
            printf '%s' "(integer)"
        elif [ -e "$1" ]; then
            printf '%s' "(file: $( file -b "$1" ))"
        else
            printf '%s' "(string)"
        fi
        printf '\n'
        shift
        i=$(( i + 1 ))
    done
}

# (string) Function to print an error message.
error()
{
    printf >&2 '%s\n' \
        "Error heading: $1" \
        "Error message: $2"
}

# (string) Function to print an error message and exit right away.
error_out()
{
    if ! { [ "$#" -eq 2 ] && [ -n "$1" ] && [ -n "$2" ]; } &&
       ! { [ "$#" -eq 3 ] && [ -n "$1" ] && [ -n "$2" ] && [ "$3" -ge 1 ] && is_safe_exit "$3"; } then
        dump_args "$@"
        # shellcheck disable=SC2016
        printf >&2 '%s\n' \
            'error_out() input check' \
            'Wrong number, type or empty arguments have been passed to the function.' \
            'Expected the following strings: $1 = error heading, $2 = error message.' \
            'Optionally, accepting a number: $3 = exit code; if not given 1 is used.' \
            'Acceptable exit code range: anywhere between <1;125>; 1 is the default.'
        exit 1
    fi
    error "$1" "$2"
    exit "${3:-1}"
}

if [ "$( id -u )" -eq 0 ]; then
    error_out 'is_user_root()' 'Running this script with root privileges is not recommended! Expected an ordinary user.'
fi

dependencies()
{
    if [ "$#" -eq 0 ]; then
        error_out 'dependencies() input check' 'No arguments passed to the function! At least one program/package name pair expected.'
    fi
    if [ "$(( $# % 2 ))" -ne 0 ]; then
        dump_args "$@"
        error_out 'dependencies() input check' 'Odd number of arguments has been passed to the function. Program and package names in pairs expected.'
    fi

    dependency_missing=false

    while [ "$#" -gt 0 ]; do
        if ! command -v "$1" >/dev/null 2>&1; then
            dependency_missing=true
            error 'dependencies(): unmet required dependency' "This script requires '$1' program but it is not available on this system! On Ubuntu/Debian OS, just install the '$2' package; other systems may differ."
        fi
        shift 2
    done

    if "$dependency_missing"; then
        exit 1
    fi
}

dependencies \
    pgrep      procps \
    xdotool    xdotool

usage()
{
# Original logic:
#    if ! { [ "$#" -eq 1 ] && is_safe_exit "$1"; } then

    if [ "$#" -ne 1 ] || ! is_safe_exit "$1"; then
        dump_args "$@"
        # shellcheck disable=SC2016
        error_out 'usage() input check' 'Number required: $1 = exit code from the script.'
    fi

    [ "$1" -ne 0 ] && exec >&2

cat <<EOF
Project: Lightshot print screen Linux handler
Version: 9.1 (2025-Oct-01)
GitHub : https://burian.work/lightshot
--------------------------------------------------------------------
Description: This script works with XDOTOOL to trigger Print Screen
key combination in Lightshot application installed on Linux in Wine.
The script, however, won't launch Lightshot for you due to variable
installation locations which make it impossible. You need to run it!
License is MIT. The code can be further enhanced by others provided
that any new copies have my copyright notice included in every case.
--------------------------------------------------------------------
Usage in terminal: ./$( basename "$0" ) [-k HotKey]

    -k HotKey: Optional switch requiring one argument,
        which is the print screen hotkey combination.
        For the left Control key and the Print Screen key
        that would be -k Control_L+Print (as an example).

    -h: Show this help.

Usage in desktop environment:

    You need to find Keyboard shortcuts, and re-map Print to run
    this script, an example from Linux Mint is placed on GitHub.

Author:
    Copyright 2018-2025
    Vlastimil Burian
    [email protected]
EOF

    exit "$1"
}

while getopts ':hk:' option; do

    case "$option" in

        k)
            hotkey=$OPTARG
            shift 2
        ;;

        h)
            usage 0
        ;;

        *)
            dump_args "$@"
            usage 1
        ;;

    esac

done


# This was simulated and IDK how to prevent erroring out without catching it...
# But it does seem to work properly if some expected Key combination is passed.
#./lightshot_print_screen -k -k

# Possible problem under my question here, suggesting including the command below, investigate:
# https://unix.stackexchange.com/a/778450/126755
#shift $(( OPTIND - 1 ))


if [ "$#" -ne 0 ]; then
    dump_args "$@"
    usage 2              # Changed from 1 to 2 for debugging the "problem" above
fi

get_process_id()
{
    if [ "$#" -ne 1 ] || [ -z "$1" ]; then

# Original logic:
#    if ! { [ "$#" -eq 1 ] && [ -n "$1" ]; } then

        dump_args "$@"
        # shellcheck disable=SC2016
        error_out 'get_process_id() input check' 'String required: $1 = a process name to find id to.'
    fi
    # -o, --oldest: least recently started
    pgrep -o "$1"
}

get_window_id()
{

# Original logic:
#    if ! { [ "$#" -eq 2 ] && is_uint "$1" && [ -n "$2" ]; } then

    if [ "$#" -ne 2 ] || ! is_uint "$1" || [ -z "$2" ]; then
        dump_args "$@"
        error_out 'get_window_id() input check' 'Two valid arguments were not passed to the function! Expected a process id (unsigned integer) and a window name (string).'
    fi
    #--all  : Require all conditions to be met.
    #--sync : Wait until a search result is found.
    #--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.
    if ! xdotool search --all --sync --limit 1 --pid "$1" --name "$2"; then
        error_out 'get_window_id()' "No window matching process id '$1' and window name '$2' has been found."
    fi
}

# The Lightshot file name is case-sensitive.
lightshot_process_id=$( get_process_id Lightshot.exe )

if ! is_uint "$lightshot_process_id"; then
    # shellcheck disable=SC2016
    error_out 'is_uint $lightshot_process_id' 'Lightshot process is not running. Launch Lightshot app first, please.'
fi

# The Lightshot window name is case-insensitive.
lightshot_window_id=$( get_window_id "$lightshot_process_id" Lightshot )

if ! is_uint "$lightshot_window_id"; then
    # shellcheck disable=SC2016
    error_out 'is_uint $lightshot_window_id' 'Lightshot process was found (is running) but its window was not found for some reason / get_window_id() returned an empty value.'
fi

if xdotool key --window "$lightshot_window_id" "$hotkey"; then
    printf '%s\n' \
        "Success, the hotkey = $hotkey was successfully sent to Lightshot window!" \
        'If the Lightshot overlay did not appear, you likely sent a wrong hotkey.' \
        'In such a case, please adjust the hotkey in Lightshot tray options menu.'
else
    error_out "xdotool key --window $lightshot_window_id $hotkey" 'The above send-hotkey command encountered an unknown error.'
fi
\$\endgroup\$
6
  • 1
    \$\begingroup\$ This is abnormally high-quality code -- it's vanishingly rare that I see a shell script written this well. \$\endgroup\$ Commented Oct 1 at 19:58
  • \$\begingroup\$ @CharlesDuffy Thank you! Good morning to you sir. \$\endgroup\$ Commented Oct 2 at 5:10
  • \$\begingroup\$ Will you take documentation writing/grammar critiques? Because, "Run this script with -h for complete help to this script with an example." is clunky and has one too many "this script"s in it. (The fact that it's immediately followed by, "Just run this script..." definitely does not help.) \$\endgroup\$ Commented Oct 2 at 11:02
  • \$\begingroup\$ @FeRD Of course. Just put down any recommendation, and I will think about ALL notes/advise. \$\endgroup\$ Commented Oct 2 at 11:07
  • 1
    \$\begingroup\$ @oguzismail Hello. Thank you. Will look at it. Thanks! Update: I agree, Done. \$\endgroup\$ Commented Oct 11 at 9:02

2 Answers 2

4
+100
\$\begingroup\$

Looks pretty good generally, and Shellcheck is happy. I've only skimmed it so far and not seen much to criticise.


I don't think is_safe_exit requires the is_uint test. A non-integer value will fail the [ -le with an error message, so we could simply test

is_safe_exit()
{
    [ 0 -le "$1" ] 2>/dev/null && [ "$1" -le 125 ]
}

get_process_id gets the most recently started Lightshot instance. This might be valid for a single-user box but could be risky for a general-purpose computer serving multiple X terminals - we might have a long wait in xdotool if we pick an instance that's connected to a different X server.

\$\endgroup\$
4
  • \$\begingroup\$ I saw that the exit status is correct. I was referring to the choice of output stream. I.e. lightshot -h >usage.text. \$\endgroup\$ Commented Oct 1 at 9:52
  • 1
    \$\begingroup\$ Oh yes, I did miss that the redirection is conditional. I've never seen that before; the usual pattern is to choose redirection where the function is invoked (e.g. usage 0 and usage 1 >&2). \$\endgroup\$ Commented Oct 1 at 10:38
  • \$\begingroup\$ Sorry, I have been too busy up until now. What do you suggest instead of pgrep -o ... please Toby? \$\endgroup\$ Commented Oct 2 at 19:03
  • \$\begingroup\$ Would you be able to expand your answer? I'm taking all notes on just about everything. There is now also a bounty worth a 100 points, not that you need it... Thanks in advance, if you decide to make time. Cheers \$\endgroup\$ Commented Oct 4 at 6:50
4
\$\begingroup\$

Here are some general coding style suggestions.

Comments

The code is well-documented with comments.

However, commented-out code should be deleted to reduce clutter. For example:

# Original logic:
#    if ! { [ "$#" -eq 1 ] && is_safe_exit "$1"; } then

# Original logic:
#    if ! { [ "$#" -eq 1 ] && [ -n "$1" ]; } then

It looks like you left notes to yourself in the code to help during development, but they should now be deleted.

Comments are intended to explain the code, but the following seem to cast doubt on proper operation:

# This was simulated and IDK how to prevent erroring out without catching it...
# But it does seem to work properly if some expected Key combination is passed.
#./lightshot_print_screen -k -k

# Possible problem under my question here, suggesting including the command below, investigate:
# https://unix.stackexchange.com/a/778450/126755
#shift $(( OPTIND - 1 ))

I suggest deleting those comments as well. You can keep track of potential issues in another file in your repo instead.

Layout

The code layout is excellent. It would be better to shorten some of the very long lines, such as:

error_out 'get_window_id() input check' 'Two valid arguments were not passed to the function! Expected a process id (unsigned integer) and a window name (string).'

You used line continuation successfully in other areas of your code.

My imperfect editor

It is a limitation of the editor in which I am viewing your code, but the apostrophe in the here-doc trashes syntax highlighting thereafter. When I change "won't":

The script, however, won't launch Lightshot for you due to variable

to "will not":

The script, however, will not launch Lightshot for you due to variable

proper highlighting returns.

There is absolutely nothing wrong with your code as-is. I just mentioned this in case others have editors with quirks as well.

\$\endgroup\$
3
  • \$\begingroup\$ Today, I started changing the logic, one remains, and the rest I commented out in case I made an error to be able to revert tomorrow. The -k -k argument I doubt will be executed, well ever... Why would someone... But since I have made couple simulations, I found it, and until I decide what to do with it, it will be there. The will not I might change to, thank you. Cheers \$\endgroup\$ Commented Oct 1 at 12:09
  • \$\begingroup\$ I've thought about the won't issue with my apostrophe and will not won. Thank you. These days are crazy, but I hope during the weekend I may close this question. Cheers \$\endgroup\$ Commented Oct 2 at 19:16
  • \$\begingroup\$ Would you be able to expand your answer? I'm taking all notes on just about everything. There is now also a bounty worth a 100 points, not that you need it... Thanks in advance, if you decide to make time. Cheers \$\endgroup\$ Commented Oct 4 at 6:50

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.