Code review comment for lp:~bryanfritt/compiz/compiz-decorator_script-edit_1192376

61 +# Set COMPIZ_BIN_PATH if need be
62 +if [ -z "${COMPIZ_BIN_PATH}" ]; then
63 + #COMPIZ_BIN_PATH="/usr/bin/"
64 + # ?set default to the bin path is in the same path as this file, or to do above?
65 + COMPIZ_BIN_PATH=$(cd $(dirname "$0"); pwd)
66 + #echo "${COMPIZ_BIN_PATH}"
67 +fi

This is fine, although the method is a bit strange. Is there anything wrong with:

if [ -z "${COMPIZ_BIN_PATH}" ]; then
    # Set default binary path to the same path as this file
    COMPIZ_BIN_PATH=`dirname "$0"`
fi

There's lots of commented out code in the review version.

115 -if [ -n "$DECORATOR" ]; then
116 - verbose "Starting ${DECORATOR}\n"
117 - exec ${COMPIZ_BIN_PATH}$DECORATOR "$@"
118 -else
119 - verbose "Found no decorator to start\n"
120 - exec $FALLBACKWM $FALLBACKWM_OPTIONS
121 +if [ -z "${XDG_CONFIG_DIRS}" ]; then
122 + test -f "/etc/xdg/compiz/compiz-manager" && . "/etc/xdg/compiz/compiz-manager"
123 +else
124 + test -f "${XDG_CONFIG_DIRS}/compiz/compiz-manager" && . "${XDG_CONFIG_DIRS}/compiz/compiz-manager"
125 +fi

These changes are inconsistent with the coding style. Use 8-wide tabs for two indents and 4-spaces for one indent, or, if the language doesn't support it, use 4-spaces for indents.

159 + # If there isn't a compiz decorator available, try not leave users without decoration
160 + if [ "${DESKTOP_SESSION}" = "kde-plasma" ]; then
161 + FALLBACKWM=`which kwin` # typically "/usr/bin/kwin"
162 + else
163 + FALLBACKWM=`which metacity` # typically "/usr/bin/metacity"
164 + fi
165 + FALLBACKWM_OPTIONS="--replace"
166 + verbose "Couldn't find a decorator to start, running fallback"
167 + verbose "exec ${FALLBACKWM} ${FALLBACKWM_OPTIONS} &"
168 + exec "${FALLBACKWM}" "${FALLBACKWM_OPTIONS}" &

I'm not so sure how I feel about starting another _window manager_ just because a decorator wasn't found, but this code was here before so I don't mind too much.