-
Notifications
You must be signed in to change notification settings - Fork 335
feat/fix/multi battery display #348
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThe documentation and battery status script were updated to support and clarify handling of multiple batteries. The script now processes arrays of battery data, displays each battery's label, icon, and percentage individually, and introduces a configuration option for custom separators. Documentation was updated to reflect these multi-battery configuration options. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BatteryScript
participant System
User->>BatteryScript: Run battery.sh
BatteryScript->>System: Query battery percentages, statuses, and labels
System-->>BatteryScript: Return percentages, statuses, labels (possibly multiple)
BatteryScript->>BatteryScript: Split data into arrays
loop For each battery
BatteryScript->>BatteryScript: parse_battery_status(percentage, status)
BatteryScript->>User: Output label, icon, percentage (with separator)
end
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (2)
scripts/battery.sh (1)
90-150
:parse_battery_status
recreates the same associative arrays on every callDeclaring the 11-element maps inside the function for every battery is
needless work. Move them to file scope or declare themlocal -g
once, then
index them inside the function. It makes the hot-path loop lighter and easier
to read.docs/CONFIG.md (1)
212-223
: Clarify newline usage in tmux optionsThe example uses an actual line-break inside the quoted string:
set -g @dracula-battery-label "1:\n2:"tmux will store the literal “\n”, not a newline.
To embed real newlines you need either:set -g @dracula-battery-label "1:or escape with
$'...'
:set -g @dracula-battery-label $'1:\n2:'Worth mentioning to prevent confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/CONFIG.md
(1 hunks)scripts/battery.sh
(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
scripts/battery.sh (1)
scripts/utils.sh (1)
get_tmux_option
(3-12)
bat_perc=$(battery_percent) | ||
bat_perc="${bat_perc%\%}" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Percent sign removed only from LAST battery – breaks arithmetic for the others
${bat_perc%\%}
strips the %
at one trailing location.
With multiple batteries (70%\n60%
) the result becomes 70%\n60
, so the first value still carries %
and parse_battery_status
later receives 70%
, triggering “value too great for base” or “invalid arithmetic” errors.
- bat_perc="${bat_perc%\%}"
+# drop “%” from *every* line
+bat_perc=$(tr -d '%' <<<"$bat_perc")
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
bat_perc=$(battery_percent) | |
bat_perc="${bat_perc%\%}" | |
bat_perc=$(battery_percent) | |
bat_perc=$(tr -d '%' <<<"$bat_perc") |
🤖 Prompt for AI Agents
In scripts/battery.sh around lines 174 to 176, the code removes the percent sign
only from the last battery percentage value, causing errors when multiple
batteries are present. Modify the code to remove the percent signs from all
battery percentage values by processing each value individually or using a
method that strips '%' from every entry before further arithmetic operations.
IFS=$'\n' read -rd '' -a percs <<<"$bat_perc" | ||
IFS=$'\n' read -rd '' -a stats <<<"$(get_battery_status)" | ||
IFS=$'\n' read -rd '' -a lbls <<<"$bat_label" | ||
show_bat_label=$(get_tmux_option "@dracula-show-battery-status" false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
read -rd ''
is invalid – arrays stay empty
read
’s -d
expects a single delimiter character; an empty string yields
read: -d: option requires an argument
, so the arrays never populate and the
widget silently prints nothing.
A portable Bash way:
-IFS=$'\n' read -rd '' -a percs <<<"$bat_perc"
-IFS=$'\n' read -rd '' -a stats <<<"$(get_battery_status)"
-IFS=$'\n' read -rd '' -a lbls <<<"$bat_label"
+readarray -t percs <<<"$bat_perc"
+readarray -t stats <<<"$(get_battery_status)"
+readarray -t lbls <<<"$bat_label"
Alternatively, keep read
:
IFS=$'\n' read -r -a percs <<<"$bat_perc"
🤖 Prompt for AI Agents
In scripts/battery.sh around lines 183 to 186, the use of 'read -rd ''' is
invalid because the -d option requires a single delimiter character and an empty
string causes the arrays to remain empty. To fix this, remove the '-d ''' option
and use 'read -r -a' with IFS set to newline to correctly populate the arrays
from the input strings.
for ((i=0; i<num_bats; i++)); do | ||
if [[ i -gt 0 ]]; then | ||
echo -n "$(get_tmux_option "@dracula-battery-separator" "; ")" | ||
fi | ||
echo -n "${lbls[$i]}" | ||
if $show_bat_label; then | ||
echo -n "$(parse_battery_status "${percs[$i]}" "${stats[$i]}") " | ||
fi | ||
echo -n "${percs[$i]}%" | ||
done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Avoid per-iteration tmux calls & off-by-one risk
-
get_tmux_option "@dracula-battery-separator" "; "
is executed for every
battery. Cache once before the loop to halve tmux round-trips. -
num_bats=$(wc -l <<<"$bat_perc")
undercounts when the last line lacks a
trailing newline and overcounts ifbat_perc
was empty.
You already havepercs
; just use its length.
- num_bats=$(wc -l <<< "$bat_perc")
+num_bats=${#percs[@]}
…
- if [[ i -gt 0 ]]; then
- echo -n "$(get_tmux_option "@dracula-battery-separator" "; ")"
+if (( i > 0 )); then
+ echo -n "$bat_sep"
fi
bat_sep=$(get_tmux_option "@dracula-battery-separator" "; ")
🤖 Prompt for AI Agents
In scripts/battery.sh around lines 187 to 196, avoid calling get_tmux_option
inside the loop by caching its result in a variable before the loop starts.
Also, replace the calculation of num_bats using wc -l on bat_perc with using the
length of the percs array to prevent off-by-one errors and incorrect counts when
bat_perc is empty or lacks a trailing newline. Update the loop to use the cached
separator variable and the corrected num_bats value.
support multiple batteries
upgrading the battery widget to display an arbitrary amount of detected batteries.
battery label
the battery label can be removed, set at just the leftmost end, or set for each battery individually.
setting multiple battery labels requires separating them with newlines
battery divider
as soon as there is more than one battery, a divider can be set. with the following being the default setting.
nerdfont battery status
the battery status continues to be displayed as nerdfont glyph for each battery individually, as it was for one battery before.
tested under: