Script module Q - vars outside of functions?

[sleve_mcdichael]

I am working on a script module and I find myself repeating a lot of the same vars over and over again in my different functions. Is it okay to define them once outside of any function and then call them from within?

Example:

rp_module_id="mymodule"
rp_module_flags="!all rpi"
dir1="path/to/dir1"
file2="path/to/file2.ext"

function install_mymodule() {
    mkUserDir "$dir1"
}

function configure_mymodule() {
    mv "$file2" "$dir1"
}

...instead of:

rp_module_id="mymodule"
rp_module_flags="!all rpi"
 
function install_mymodule() {
    local dir1="path/to/dir1"
    mkUserDir "$dir1"
}
 
function configure_mymodule() {
    local dir1="path/to/dir1"
    local file2="path/to/file2.ext"
    mv "$file2" "$dir1"
}

...if yes, what do I do with the initially-undefined var used in "for" loops, i.e.:

local file
for file in "$file1" "file2"; do

Would I,

1: still declare a local file inside each function where I need to use it?

2: do an empty file="" (or file=()?) outside the functions with the other vars?

3: just wing it and do for file in without declaring it anywhere, first?

[BuZz]

@sleve_mcdichael for retropie modules we dont allow global vars apart from the module configuration ones.

[BuZz]

If you need to share paths between functions you can deduplicate this by having a function which outputs the path.

[sleve_mcdichael]

@BuZz said in Script module Q - vars outside of functions?:

If you need to share paths between functions you can deduplicate this by having a function which outputs the path.

...and then I'm calling that function from within the other function right? Something like:

function _getpath1_mymodule() {
    echo "$path/to/file"
}
 
function _getpath2_mymodule() {
    echo "/path/to/$other/file"
}
 
function_a_mymodule() {
path1="$(_getpath1_mymodule)"
path2="$(_getpath2_mymodule)"
}
 
function_b_mymodule() {
path1="$(_getpath1_mymodule)"
path2="$(_getpath2_mymodule)"
}

...this doesn't seem to save any lines, in fact it uses even more lines since I'm now defining the paths once in each function and once each in their own individual function. I guess the benefit of this method is, if I change the path later, I only have to change it in one place instead of in each function separately.

Is that all roughly correct?

[BuZz]

@sleve_mcdichael yes. Have a look at existing scriptmodules as some use this already.

[sleve_mcdichael]

@BuZz thanks, I see something like this in e.g. moonlight.sh.

Do you have any thoughts or comments on the method I use here in function enable_ vs the method in disable_ and install_ (or, on anything else)? I did it this way in enable_ because I also need to process the files individually, while the other two do them all as a group in the "for" loop:

#!/usr/bin/env bash
 
# This file is part of The RetroPie Project
#
# The RetroPie Project is the legal property of its developers, whose names are
# too numerous to list here. Please refer to the COPYRIGHT.md file distributed with this source.
#
# See the LICENSE.md file at the top-level directory of this distribution and
# at https://raw.githubusercontent.com/RetroPie/RetroPie-Setup/master/LICENSE.md
#
 
rp_module_id="bgm123"
rp_module_desc="Straightforward background music player using mpg123"
rp_module_help="Place your MP3 files in $datadir/bgm"
rp_module_section="exp"
rp_module_flags="!all rpi"
 
function _autostart_bgm123() {
    echo "$configdir/all/autostart.sh"
}
 
function _bashrc_bgm123() {
    echo "$home/.bashrc"
}
 
function _onstart_bgm123() {
    echo "$configdir/all/runcommand-onstart.sh"
}
 
function _onend_bgm123() {
    echo "$configdir/all/runcommand-onend.sh"
}
 
function depends_bgm123() {
    getDepends mpg123
}
 
function install_bin_bgm123() {
    local scriptname="bgm_vol_fade.sh"
    local files=(
        "$(_autostart_bgm123)"
        "$(_bashrc_bgm123)"
        "$(_onstart_bgm123)"
        "$(_onend_bgm123)"
    )
    local file
 
    cp "$md_data/$scriptname" "$md_inst"
    chmod +x "$md_inst/$scriptname"
 
    for file in "${files[@]}"; do
        # preserve original file versions
        if [[ -f "$file" && ! -f "$file.old.bgm123" ]]; then
            cp "$file" "$file.old.bgm123"
            chown $user:$user "$file.old.bgm123"
        fi
    done
}
 
function disable_bgm123() {
    local files=(
        "$(_autostart_bgm123)"
        "$(_bashrc_bgm123)"
        "$(_onstart_bgm123)"
        "$(_onend_bgm123)"
    )
    local file
 
    # kill player now since .bashrc won't do it later
    vcgencmd force_audio hdmi 0 >/dev/null
    pgrep mpg123 >/dev/null && pkill mpg123
 
    for file in "${files[@]}"; do
        if [[ -f "$file" ]]; then
            # backup file and attempt to remove any existing bgm config
            cp -f "$file" "$file.bak"
            chown $user:$user "$file.bak"
            sed -i '/#bgm/d' "$file"
 
            # if file is now empty, remove it
            [[ ! -s "$file" ]] && rm -f "$file"
        fi
    done
}
 
function remove_bgm123() {
    disable_bgm123
    remove_share_samba "bgm"
    restart_samba
}
 
function enable_bgm123() {
    local fadescript="$md_inst/bgm_vol_fade.sh"
    local autostart="$(_autostart_bgm123)"
    local bashrc="$(_bashrc_bgm123)"
    local onstart="$(_onstart_bgm123)"
    local onend="$(_onend_bgm123)"
    local file
 
    disable_bgm123
 
    for file in "$autostart" "$bashrc" "$onstart" "$onend"; do
        touch "$file"
        chown $user:$user "$file"
    done
 
    echo -e "$(echo -e 'while pgrep omxplayer >/dev/null; do sleep 1; done #bgm123\n(vcgencmd force_audio hdmi 1 >/dev/null; sleep 8; mpg123 -Z "'$datadir'/bgm/"*.[mM][pP]3 >/dev/null 2>&1) & #bgm123'; cat $autostart)" > "$autostart"
    echo -e '[[ "$(tty)" == "/dev/tty1" ]] && (vcgencmd force_audio hdmi 0 >/dev/null; pkill mpg123) #bgm123' >> "$bashrc"
    echo -e '"'"$fadescript"'" -STOP & #bgm123' >> "$onstart"
    echo -e '(sleep 1; "'"$fadescript"'" -CONT) & #bgm123' >> "$onend"
}
 
function configure_bgm123() {
    [[ "$md_mode" == "remove" ]] && return
 
    local share="$datadir/bgm"
 
    mkUserDir "$share"
    add_share_samba "bgm" "$share"
    restart_samba
 
    enable_bgm123
}
 
function play_pause_bgm123() {
    if pgrep mpg123 >/dev/null; then
        su $user -c "$md_inst/bgm_vol_fade.sh &"
    else
        su $user -c "(vcgencmd force_audio hdmi 1 >/dev/null; sleep 1; mpg123 -Z $datadir/bgm/*.[mM][pP]3 >/dev/null 2>&1) &"
    fi
}
 
function next_track_bgm123() {
    pgrep mpg123 >/dev/null && pkill mpg123
    su $user -c "(vcgencmd force_audio hdmi 1 >/dev/null; sleep 1; mpg123 -Z $datadir/bgm/*.[mM][pP]3 >/dev/null 2>&1) &"
}
 
function gui_bgm123() {
    local cmd=(dialog --backtitle "$__backtitle" --cancel-label "Back" --menu "Choose an option." 22 86 16)
    while true; do
        local enabled=0
        grep '#bgm123' "$configdir/all/autostart.sh" >/dev/null && enabled=1
        local options=()
        if [[ "$enabled" -eq 1 ]]; then
            if pgrep emulationstatio >/dev/null; then
                options+=(
                    P "Play / pause"
                    N "Next track"
                )
            fi
            options+=(
                1 "Enable background music (currently: Enabled)"
            )
        else
            options+=(
                1 "Enable background music (currently: Disabled)"
            )
        fi
        local choice=$("${cmd[@]}" "${options[@]}" 2>&1 >/dev/tty)
        if [[ -n "$choice" ]]; then
            case "$choice" in
                P)
                    play_pause_bgm123
                    ;;
                N)
                    next_track_bgm123
                    ;;
                1)
                    if [[ "$enabled" -eq 1 ]]; then
                        disable_bgm123
                        printMsgs "dialog" "Background music disabled."
                    else
                        enable_bgm123
                        printMsgs "dialog" "Background music enabled."
                    fi
                    ;;
            esac
        else
            break
        fi
    done
}

Edit:

@sleve_mcdichael said in Script module Q - vars outside of functions?:

    local autostart="$(_autostart_bgm123)"
    local bashrc="$(_bashrc_bgm123)"
    local onstart="$(_onstart_bgm123)"
    local onend="$(_onend_bgm123)"

I am seeing in the style guide that I should not do this when the variable is assigned by command substitution and that declaration and assignment should be in separate lines:

    local autostart
    autostart="$(_autostart_bgm123)"
    local bashrc
    bashrc="$(_bashrc_bgm123)"
    local onstart
    onstart="$(_onstart_bgm123)"
    local onend
    onend="$(_onend_bgm123)"
    local file
 
    for file in "$autostart" "$bashrc" "$onstart" "$onend"; do