Making time lapse screenshots using Bash

Question

My experience in bash is... well, around the Bourne shell on AIX. I wrote a semi-complex script in Bash, and I feel that it could be improved in 100 different ways.

The Github project is here: https://github.com/mercmobily/lapser

Questions:

• Does anything stand out in terms of things done horribly wrong, in any way?
• In the script I have a bunch of global variables defined at the top of the file; I then re-set them depending on the profile picked by the user. I set them with set_profile_dirs() which is run once the user has picked her profile. Is this a horrific way to go about it?
• To deal with files with spaces, I did something like convert ... "$SHOTS_CURRENT_DIR"/"$now".jpg . Is this normally enough?
• I remember having problems with IFs if one of the operands were empty. Did I make my IF statements strong enough?
• The part file='ls -d "$SHOTS_CURRENT_DIR"/* | head -1' looks fragile to me, because if there are no files, it returns a "*". Any hints?
• In the program I have something like working_on=`echo $res | cut -d '|' -f 1`. Is this the right way of doing things? Is there a way to get all of the fields at once?
• with delta='expr $new_timestamp - $timestamp'; -- is expr still the way to go?
• When I have a yad command like this res=$(yad, I end up with two results: the standard output and the error code. The ifs underneath those commands are kind of clunky. Is there a better way of checking the results?

The code:

#!/bin/bash
# Global variables
DATA_DIR="$HOME/.lapser"
PROFILE_DIR=''
LABEL_FILE=''
CONFIG_FILE=''
SHOTS_CURRENT_DIR=''
SHOTS_NOT_CONVERTED_DIR=''
SHOTS_CONVERTED_DIR=''
MOVIES_NOT_UPLOADED_DIR=''
MOVIES_UPLOADED_DIR=''
set_profile_dirs(){
 profile=1ドル
 PROFILE_DIR="$DATA_DIR"/profiles/"$profile"
 LABEL_FILE="$PROFILE_DIR"/working_on.txt
 CONFIG_FILE="$PROFILE_DIR"/config.cfg
 LOG_FILE="$PROFILE_DIR"/log.txt
 SHOTS_CURRENT_DIR="$PROFILE_DIR"/current_shots
 SHOTS_NOT_CONVERTED_DIR="$PROFILE_DIR"/shots_archived_not_converted
 SHOTS_CONVERTED_DIR="$PROFILE_DIR"/shots_archived_converted
 MOVIES_NOT_UPLOADED_DIR="$PROFILE_DIR"/movies_not_uploaded
 MOVIES_UPLOADED_DIR="$PROFILE_DIR"/movies_uploaded
 for d in "$DATA_DIR" "$DATA_DIR"/profiles "$DATA_DIR"/profiles/default "$PROFILE_DIR" "$SHOTS_CURRENT_DIR" "$SHOTS_NOT_CONVERTED_DIR" "$SHOTS_CONVERTED_DIR" "$MOVIES_NOT_UPLOADED_DIR" "$MOVIES_UPLOADED_DIR" ;do
 if [ ! -d "$d" ]; then
 mkdir "$d"
 fi
 done
 for f in "$LABEL_FILE" "$LOG_FILE" "$CONFIG_FILE";do
 if [ ! -f "$f" ]; then
 echo -n > "$f"
 fi
 done
}
set_basic_dirs(){
 for d in "$DATA_DIR" "$DATA_DIR"/profiles "$DATA_DIR"/profiles/default;do
 if [ ! -d "$d" ]; then mkdir "$d"; fi
 done
}
yad_message(){
 yad \
 --width=600 \
 --title="Config" \
 --text="1ドル" \
 --button="OK:0" \
 --center
}
first_current_shot(){
 file=`ls -d "$SHOTS_CURRENT_DIR"/* | head -1`
 file=`basename $file`
 file="${file%.*}"
 echo "$file"
}
last_current_shot(){
 file=`ls -d "$SHOTS_CURRENT_DIR"/* | tail -1`
 file=`basename $file`
 file="${file%.*}"
 echo "$file"
}
how_many_current_shots(){
 ret=`find "$SHOTS_CURRENT_DIR" -maxdepth 1 -type f | wc -l`
 echo "$ret"
}
monitor(){
 # DEBUG, PUT BACK TO 15
 #threshold='15';
 threshold='1';
 timestamp=$SECONDS
 while sleep 1; do 
 echo 100
 new_timestamp=$SECONDS
 delta=`expr $new_timestamp - $timestamp`;
 if [ $delta -ge $threshold ];then
 # Prepare timestamp for next cycle
 timestamp=$new_timestamp
 # Get label variables ready
 now=$(date +"%Y-%m-%d_%H.%M.%S")
 label=`cat "$LABEL_FILE"`
 # Make screenshot
 import -resize 800 -window root /tmp/shot.${PID}.jpg
 convert /tmp/shot.${PID}.jpg -gravity Southwest -background black -fill white -splice 0x22 -pointsize 18 -annotate +0+0 "$now: $label" "$SHOTS_CURRENT_DIR"/"$now".jpg
 fi
 done
}
the_program(){
 profile=1ドル
 if [ -z "$profile" ];then
 profile='default';
 fi
 set_profile_dirs $profile
 PID=$$
 # Read the config file
 source "$CONFIG_FILE"
 working_on=`cat $LABEL_FILE`
 while true;do
 ##############
 # MAIN MENU
 ##############
 # Show main dialog, getting $ret and $res
 res=$(yad \
 --width=600 \
 --title="Lapser automatic screenshots - $profile" \
 --text="Press the button to start logging..." \
 --form \
 --field="Working on..." \
 --button="Start capturing:2" \
 --button="Archive:6" \
 --button="Convert to movie:8" \
 --button="Upload:10" \
 --button="Config:4" \
 --button="Cancel:17" \
 --center \
 "$working_on" )
 ret=$?
 # This gets saved regardless (unless ESCAPing or CANCELing)
 if [ $ret -ne 17 -a $ret -ne 252 ];then
 working_on=`echo $res | cut -d '|' -f 1`
 echo "$working_on" > $LABEL_FILE
 fi
 # #1: Start capturing
 if [ $ret -eq 2 -o $ret -eq 0 ];then
 first_shot=$(first_current_shot)
 how_many=$(how_many_current_shots)
 if [ $how_many -eq 0 ];then
 extra_text="This is a first capture after archiving"
 else
 first_shot=$(first_current_shot)
 extra_text="This capture started on $first_shot"
 fi
 d=$(date +"%Y-%m-%d_%H.%M.%S")
 label=`cat "$LABEL_FILE"`
 echo $d - START $label >> $LOG_FILE
 monitor | yad --progress --title="Capturing for $profile" --progress-text="Capturing in progress. $extra_text" --text="Press Cancel to stop capturing" --center
 d=$(date +"%Y-%m-%d_%H.%M.%S")
 echo $d - STOP $label >> $LOG_FILE
 # #4: Config
 elif [ $ret -eq 4 ];then
 res=$(yad \
 --width=600 \
 --title="Config" \
 --text="COnfiguration options" \
 --form \
 --field="User" \
 --field="Password:H" \
 --field="SSH Server" \
 --button="Save:2" \
 --button="Cancel:1" \
 --center \
 "$cfg_user" "$cfg_password" "$cfg_server" )
 ret=$?
 if [ $ret -eq 2 -o $ret -eq 0 ];then
 cfg_user=`echo $res | cut -d '|' -f 1`
 cfg_password=`echo $res | cut -d '|' -f 2`
 cfg_server=`echo $res | cut -d '|' -f 3`
 echo -e "cfg_user='${cfg_user}'\ncfg_password='${cfg_password}'\ncfg_server='${cfg_server}'\n" > $CONFIG_FILE
 source "$CONFIG_FILE"
 yad_message "Configuration saved!"
 fi
 # #6: Archive
 elif [ $ret -eq 6 ];then
 how_many=$(how_many_current_shots)
 if [ $how_many -le 4 ];then
 yad_message "Less than 4 screenshots taken, too early to archive"
 else 
 first_file=$(first_current_shot)
 last_file=$(last_current_shot)
 if [ -z "$last_file" -o -z "$first_file" ];then
 yad_message "Error working out the name of the destination folder!"
 else 
 log=$(cat $LOG_FILE)
 yad \
 --width=600 \
 --height=400 \
 --title="Are you sure?" \
 --text="This will archive the current time lapse.\n\n(From $first_file to $last_file)\n\n" \
 --form \
 --field="Log entries that will be attached to the archive:TXT" \
 --center \
 "$log"
 ret=$?
 if [ $ret -eq 0 ];then
 to="$first_file"_TO_"$last_file" 
 mkdir "$SHOTS_NOT_CONVERTED_DIR"/"$to"
 mv "$SHOTS_CURRENT_DIR"/* "$SHOTS_NOT_CONVERTED_DIR"/"$to"
 cp "$LOG_FILE" "$SHOTS_NOT_CONVERTED_DIR"/"$to"
 > "$LOG_FILE"
 yad_message "Archive created, timestamp: $to"
 fi
 fi
 fi
 # #8: Convert to movie
 elif [ $ret -eq 8 ];then
 list='';
 for f in "$SHOTS_NOT_CONVERTED_DIR"/*;do
 f=`basename $f`
 exc='!'
 list="${list}${exc}${f}"
 done
 pick=$(yad \
 --width=600 \
 --title="Which archive do you want to convert?" \
 --form \
 --field="Pick an archive:CB" \
 --center \
 "$list")
 ret=$?
 if [ $ret -ne 1 -a $ret -ne 252 ];then
 pick=`echo $pick | cut -d '|' -f 1`
 ffmpeg28 -y -framerate 4 -pattern_type glob -i "$SHOTS_NOT_CONVERTED_DIR"/"$pick"/'*.jpg' -b:v 800k /tmp/video.${PID}.mp4
 ret=$?
 if [ "$ret" -ne 0 ];then
 yad_message "Video creation failed"
 else
 # Move the screenshots to the "CONVERTED" directory
 mv "$SHOTS_NOT_CONVERTED_DIR"/"$pick" "$SHOTS_CONVERTED_DIR"
 # Move the freshly generated video over
 mv /tmp/video.${PID}.mp4 "$MOVIES_NOT_UPLOADED_DIR"/"$pick".mp4
 # Copy the log file over, named as the video
 cp "$SHOTS_CONVERTED_DIR"/"$pick"/log.txt "$MOVIES_NOT_UPLOADED_DIR"/"$pick".log
 yad_message "Conversion successful!"
 fi
 fi
 # #8: Upload
 elif [ $ret -eq 10 ];then
 yad \
 --width=600 \
 --title="Are you sure?" \
 --text="This will upload the existing movies to the remote server\nAre you sure?" \
 --center
 ret=$?
 # #4: The end
 elif [ $ret -eq 4 -o $ret -eq 252 ];then
 return
 fi
 done;
 # TODO:
 # ----
 # * Fix cycle, so that first yad calls second one and back to first
 # * Write rsync call to upload/move videos
 # * Check if this command makes better videos smaller
 # convert 0.png -background black -flatten +matte 0_opaque.png
 # http://stackoverflow.com/questions/3561715/using-ffmpeg-to-encode-a-high-quality-video|
} 
set_basic_dirs
while true;do
 list='';
 for f in "$DATA_DIR"/profiles/*;do
 f=`basename $f`
 exc='!'
 list="${list}${exc}${f}"
 done
 list="${list}${exc}Make new profile"
 pick=$(yad \
 --width=600 \
 --title="Which profile do you want to enter?" \
 --form \
 --field="Pick a profile:CB" \
 --center \
 "$list")
 ret=$?
 if [ $ret -ne 1 -a $ret -ne 252 ];then
 pick=`echo $pick | cut -d '|' -f 1`
 if [ "$pick" == 'Make new profile' ];then
 res=$(yad \
 --width=600 \
 --title="Create new profile" \
 --form \
 --field="New profile" \
 --button="Create:2" \
 --button="Cancel:1" \
 --center )
 ret=$?
 if [ $ret -eq 2 -o $ret -eq 0 ];then
 res=`echo $res | cut -d '|' -f 1`
 set_profile_dirs $res 
 fi
 else
 the_program $pick
 fi
 else
 exit 0
 fi
done

Answer 1

Your questions

• Does anything stand out in terms of things done horribly wrong, in any way?

It's nicely written, easy to read. Many things can be improved, but the way you've written it is very easy to review.

• In the script I have a bunch of global variables defined at the top of the file; I then re-set them depending on the profile picked by the user. I set them with set_profile_dirs() which is run once the user has picked her profile. Is this a horrific way to go about it?

It's fine. But instead of functions expecting global variables, it's easier to test when functions take parameters. For example, testing first_current_shot looks basically like this:

SHOTS_CURRENT_DIR=some_dir
first_current_shot
# check output
SHOTS_CURRENT_DIR=some_other_dir
first_current_shot
# check output

If the function was taking parameters, testing would look more like this:

first_current_shot some_dir
# check output
first_current_shot some_other_dir
# check output

One statement less per test. It's not a big problem, but this is why I tend to prefer functions taking parameters rather than expecting global variables.

• To deal with files with spaces, I did something like convert ... "$SHOTS_CURRENT_DIR"/"$now".jpg . Is this normally enough?

You mean, you enclosed them in double-quotes? Yes, that's it. In this example you could have enclosed the whole thing in double quotes, "$SHOTS_CURRENT_DIR/$now.jpg", the result is equivalent.

• I remember having problems with IFs if one of the operands were empty. Did I make my IF statements strong enough?

I guess you're referring to this kind of situation:

path=
if [ -d $path ]; then
 echo path is a directory: $path
else
 echo path is not a directory: $path
fi

This will not work, because $path is empty, and [ -d ] is missing an operand. Changing to [ -d "$path" ] fixes that. If the operand is guaranteed to never be empty, you can omit the double-quotes.

• The part file='ls -d "$SHOTS_CURRENT_DIR"/* | head -1' looks fragile to me, because if there are no files, it returns a "*". Any hints?

You need to add a check in case there are no files.

• In the program I have something like working_on=`echo $res | cut -d '|' -f 1`. Is this the right way of doing things? Is there a way to get all of the fields at once?

You could do something like this:

IFS=$'\n' fields=($(tr '|' '\n' <<< "$res"))

This the | characters to newlines, and puts each line in the fields array.

• with delta='expr $new_timestamp - $timestamp'; -- is expr still the way to go?

In Bash you can use math within ((...)), like this:

((delta = new_timestamp - timestamp))

• When I have a yad command like this res=$(yad, I end up with two results: the standard output and the error code. The ifs underneath those commands are kind of clunky. Is there a better way of checking the results?

I'm not familiar with yad, and I'm not sure what you're asking here.

Creating many directory levels

Instead of this:

 for d in "$DATA_DIR" "$DATA_DIR"/profiles "$DATA_DIR"/profiles/default;do
 if [ ! -d "$d" ]; then mkdir "$d"; fi
 done

You could use the -p flag of mkdir to create all missing parent directories:

mkdir -p "$DATA_DIR"/profiles/default

This works even if the target directory already exists.

Finding the first and last shots

Several improvements are possible to these functions:

first_current_shot(){
 file=`ls -d "$SHOTS_CURRENT_DIR"/* | head -1`
 file=`basename $file`
 file="${file%.*}"
 echo "$file"
}
last_current_shot(){
 file=`ls -d "$SHOTS_CURRENT_DIR"/* | tail -1`
 file=`basename $file`
 file="${file%.*}"
 echo "$file"
}

Issues:

• Instead of backticks, use $(...) for subshells, for example $(ls ...)
• There are many processes here: ls, head, basename. It's good to use as few processes as possible
• The stripping of parent directories and extension is duplicated
• Doesn't handle empty directories

An alternative implementation that treats all these issues:

filename() {
 test -e "1ドル" || return
 file=${1#*/}
 file=${file%.*}
 echo $file
}
first_current_shot() {
 for file in "$SHOTS_CURRENT_DIR"/*; do
 filename "$file"
 return
 done
}
last_current_shot() {
 for file in "$SHOTS_CURRENT_DIR"/*; do
 :
 done
 filename "$file"
}

When the directory is empty, these functions output nothing. Callers must either ensure that the directory is not empty, or check the output.

Use $(...) instead of `...`

Backticks are obsolete, and troublesome when you need to nest sub-shells. Replace all backticks with $(...).

Avoid echo with flags

echo is fine for simple printing like this:

echo some message

But the flags of echo are not portable. So instead of this:

 echo -n > "$f"

This is more portable:

printf > "$f"

But in this particular example, you don't actually need a printing command at all, you can simply do:

> "$f"

Minor simplifications

When an if statement is something simple like this:

if [ ! -f "$f" ]; then
 echo -n > "$f"
fi

This more compact writing style might be easier to read:

[ ! -f "$f" ] && echo -n > "$f"

---

Instead of this:

PROFILE_DIR=''
LABEL_FILE=''
CONFIG_FILE=''

You can write simpler like this:

PROFILE_DIR=
LABEL_FILE=
CONFIG_FILE=

---

Semicolons at the end of line are pointless, remove them, for example here:

 threshold='1';

• Merc: I made a ticket on Lapser pointing to this issue