dmscripts is too complex
dmscripts has grown in complexity to an unacceptable level. My goal is to simplify the code base by documenting what I think are large areas of concern and also obtaining community feedback. A roadmap should probably be established. I think I could cover most of it if there's more of an effort towards fixing bugs rather than adding new things for me to deal with.
- The boilerplate sucks
- _dm-helper is bloated and confusing
- Configuration sucks
- A lot of the scripts suck
The boilerplate sucks.
Just to kinda get a baseline of how bad the boiler plate is, this is a fresh script generated with dm-template and nothing eles:
- 1.8K in size
- 58 lines
I know LoC isn't everything and not including whitespace and the pound symbols it might not be too bad. but it still is a lot of boilerplate for something that shouldn't need to be that large.
I think a good example is this block here:
_path="$(cd "$(dirname "${BASH_SOURCE[0]}")" && cd "$(dirname "$(readlink "${BASH_SOURCE[0]}" || echo ".")")" && pwd)"
if [[ -f "${_path}/_dm-helper.sh" ]]; then
# shellcheck disable=SC1090,SC1091
source "${_path}/_dm-helper.sh"
else
# shellcheck disable=SC1090
echo "No helper-script found"
fi
this is both longer and more complex than necessary. consider this 1 liner
source ./_dm-helper.sh || source _dm-helper.sh
this checks if _dm-helper is in the local directory and if it isn't it uses the one in $PATH and it sources them:q. which is essentially what the previous one did anyways.
these lines may do better in the helper script if at all possible, most aren't going to use extra flags and if they are then they can just copy the boilerplate.
noOpt=1
# If script is run with '-d', it will use 'dmenu'
# If script is run with '-f', it will use 'fzf'
# If script is run with '-d', it will use 'rofi'
while getopts "dfrh" arg 2>/dev/null; do
case "${arg}" in
d) # shellcheck disable=SC2153
MENU=${DMENU}
[[ "${BASH_SOURCE[0]}" == "${0}" ]] && main ;;
f) # shellcheck disable=SC2153
MENU=${FMENU}
[[ "${BASH_SOURCE[0]}" == "${0}" ]] && main ;;
r) # shellcheck disable=SC2153
MENU=${RMENU}
[[ "${BASH_SOURCE[0]}" == "${0}" ]] && main ;;
h) help ;;
*) err "invalid option:
Type $(basename "$0") -h for help" ;;
esac
noOpt=0
done
# If script is run with NO argument, it will use 'dmenu'
[ $noOpt = 1 ] && MENU=${DMENU} && [[ "${BASH_SOURCE[0]}" == "${0}" ]] && main "$@"
the api should be:
MENU="$(get_menu_program "$@")"
[[ "${BASH_SOURCE[0]}" == "${0}" ]] && main
I think these are the main points of bloat and complexity in the boilerplate.
_dm-helper is bloated and confusing
dm-helper suffers similar problems to the boilerplate itself.
A few areas of shame:
parse_rss() {
echo "$1" | while xmlgetnext ; do
case $TAG in
'entry')
title=''
link=''
published=''
;;
'media:title')
title="$VALUE"
;;
'yt:videoId')
link="$VALUE"
;;
'published')
published="$(date --date="${VALUE}" "+%Y-%m-%d %H:%M")"
;;
'/entry')
echo " ${published} | ${link} | ${title}"
;;
esac
done
}
this function is named incorrectly and is only used in dm-youtube, it should probably be moved there and given a better name. Another approach would be to simply write a true parser but that might be a bit beyond the scope of dmscripts that and it's literally used in 1 script. also probably this should be moved as well but it's probably fine since it's generic enough to work well to parse other xml like formats and it's fairly small and simple.
xmlgetnext () {
local IFS='>'
# we need to mangle backslashes for this to work
# shellcheck disable=SC2162
read -d '<' TAG VALUE
}
this one here is my fault tbh:
############################
# Dislay server checks #
############################
# Boiler code for if you want to do something with display servers
#function() {
# case "$XDG_SESSION_TYPE" in
# 'x11') something with x;;
# 'wayland') something with wayland;;
# *) err "Unknown display server";;
# esac
#}
# Function to copy to clipboard with different tools depending on the display server
cp2cb() {
case "$XDG_SESSION_TYPE" in
'x11') xclip -r -selection clipboard;;
'wayland') wl-copy -n;;
*) err "Unknown display server";;
esac
}
grep-desktop() {
case "$XDG_SESSION_TYPE" in
'x11') grep "Name=" /usr/share/xsessions/*.desktop | cut -d'=' -f2;;
'wayland') grep "Name=" /usr/share/wayland-sessions/*.desktop | cut -d'=' -f2 || grep "Name=" /usr/share/xsessions/*.desktop | grep -i "wayland" | cut -d'=' -f2 | cut -d' ' -f1;;
*) err "Unknown display server";;
esac
}
So if I ever need to patch a bug, I now have 3 places to potentially do it in. The solution is to make a generic function using an api like this:
x_or_wayland "xorg command" "wayland command" "optional fail command before panic" (maybe a -n option for no panic)
then I can make all of these functions wrappers to x_or_wayland:
```bash
cp2cb() {
x_or_wayland "xclip -r -selection clipboard" \
"grep \"Name=\" /usr/share/wayland-sessions/*.desktop | cut -d'=' -f2 || grep \"Name=\" /usr/share/xsessions/*.desktop | grep -i \"wayland\" | cut -d'=' -f2 | cut -d' ' -f1"
these functions are a bit terrible:
get_local_config() {
# Do some subshell magic finding out where the script we are running
# is located and checking if ../config is a dir relative to the script
echo "$(
cd "$(dirname "$(readlink "${BASH_SOURCE[0]}" || echo ".")")./"
if [[ -d "${PWD}/config" ]]; then
echo "${PWD}/config"
fi
)"
}
get_config() {
local _config_files=()
local _local_conf
_local_conf="$(get_local_config)"
# add User config path
_config_files+=( "${HOME}/.config/dmscripts/config" )
# Add git-repo relative config path (if exits)
[[ -f "${_local_conf}/config" ]] && _config_files+=( "${_local_conf}/config" )
# Add global installed config path
_config_files+=( "/etc/dmscripts/config" )
for conf in "${_config_files[@]}"; do
if [[ -f ${conf} ]]; then
echo "${conf}"
return
fi
done
}
# Check if config has updates that should be displayed to the user
check_updated_config() {
local _base_file
local _config_file
_base_file=-1
[[ -f /etc/dmscripts/config ]] && _base_file="/etc/dmscripts/config"
_local_conf="$(get_local_config)"
[[ -f "${_local_conf}/config" ]] && _base_file=${_local_conf}/config
_config_file=$(get_config)
[[ "${_config_file}" == "${_base_file}" ]] && return
_config_file_revision=$(grep "^_revision=" "${_config_file}")
_base_file_revision=$(grep "^_revision=" "${_base_file}")
if [[ ! "${_config_file_revision}" == "${_base_file_revision}" ]] ; then
diff -y "${_config_file}" "${_base_file}" | less
echo "${_config_file} > ${_base_file}"
echo "New revision of the configuration detected, please review and set ${_base_file_revision} in ${_config_file} when done"
fi
}
get_config and get_local_config are confusing. Why are they named similarly, why are they separated. once again I feel like those are a bit too complex for what the job is. This should do the same. It isn't pretty but it works.
[ -f /etc/dmscripts/config ] && source /etc/dmscripts/config
[ -z "$XDG_CONFIG_HOME" ] && [ -f $HOME/.config/dmscripts/config ] && source $HOME/.config/dmscripts/config
[ -n "$XDG_CONFIG_HOME" ] && [ -f $XDG_CONFIG_HOME/dmscripts/config ] && source $XDG_CONFIG_HOME/dmscripts/config
[ -f ../config/config ] && source ../config/config
this also means that the revision checker will have to be rewritten slightly:
[[ -f /etc/dmscripts/config ]] && _base_file="/etc/dmscripts/config"
[[ -f ../config/config ]] && _base_file="../config/config"
[ -z "$XDG_CONFIG_HOME" ] && [ -f $HOME/.config/dmscripts/config ] && _config_file=$HOME/.config/dmscripts/config
[ -n "$XDG_CONFIG_HOME" ] && [ -f $XDG_CONFIG_HOME/dmscripts/config ] && _config_file=$XDG_CONFIG_HOME/dmscripts/config
...
Configuration sucks
Not really much to say here:
- variable names suck and are inconsistent (ie DMENU, PDF_VIEWER, DMBROWSER, logout_locker, sounds_dir, bookman_show_source)
- some things should be more dynamic and not even in the config
- like I think for confedit, we should probably say anything in the config directory is generated automatically, we could even generate pretty names using the path (ie dunst/dunstrc would be "dunst dunstrc", doom/config.org would be "doom config.org" etc.)
- Obviously people should still be able to statically generate them but it should be more dynamic than it is now
- a lot of things could be done in simple variables as opposed to lists and other complex bash stuff.
The scripts suck
These are an amalgamation of all the issues we've seen so far.
- Complexity for basically no reason
- inconsistency (variable names, spacing, etc, dmscripts is in desperate need of a formatter)
- incomplete
- replies too much on flags and on internal knowledge of how the script works
- a lot of the scripts feel like they solve a problem nobody had
there's way too much to go into full detail but this would undeniably take the longest to clean up.
Conclusion and what to think about
My ending note is this, I think dmscripts could be a lot simpler. But it might have to come at the cost of backwards compatibility of the config if we want to go all the way. Essentially, how far should I commit to this? Should backwards compatibility be considered? And are there other areas of dmscripts which are too complex that I am not properly evaluating.