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';