feat/fix/multi battery display #348
Conversation
📝 WalkthroughWalkthroughDocs and the battery script were updated to support multi-battery labeling: docs describe newline-separated labels and a configurable separator; the script was refactored to parse multiple batteries, map percent+state to icons, and emit per-battery label/icon/percentage outputs. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BatteryScript
participant System
User->>BatteryScript: invoke battery.sh
BatteryScript->>System: read battery percentages, statuses, labels
System-->>BatteryScript: return lists
BatteryScript->>BatteryScript: split into arrays
loop per battery
BatteryScript->>BatteryScript: parse_battery_status(percentage, state)
BatteryScript-->>User: output label (optional), icon, percentage
BatteryScript-->>BatteryScript: emit separator if not last
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
scripts/battery.sh (1)
90-150:parse_battery_statusrecreates 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 -gonce, 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)
| 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.
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.
🛠️ 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_percwas 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"
fibat_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.
Theoreticallyhugo
force-pushed
the
fix/battery-status
branch
from
e3f927b to
4564d62
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/CONFIG.md (2)
226-228: Clarify how to hide the widget when no battery is presentThe example mixes two knobs: turning off the regular battery label and hiding the widget when there’s no battery. If the intent is “hide when no battery,” setting only
@dracula-no-battery-label falseshould be sufficient; disabling@dracula-battery-labelalso suppresses labels even when a battery exists, which may not be intended.Consider simplifying the example to only set the no-battery label, or add a sentence explaining when to use each setting.
Proposed edit to show the minimal setting for hiding on no-battery:
- set -g @dracula-battery-label false set -g @dracula-no-battery-label falseFollow-up:
- Please confirm the script logic hides the widget when
@dracula-no-battery-labelisfalseregardless of@dracula-battery-label, and that@dracula-battery-label falseisn’t required here. If it is required, add a one-line rationale above the block.
236-246: Grammar and clarity; define “front” precisely; state default separatorMinor language fixes and clearer wording help avoid ambiguity. Also, explicitly document the default separator.
Apply this edit:
-in case you have multiple batteries: +If you have multiple batteries: -the default battery label is only displayed in the very front. -you can specify multiple battery labels by splitting them with `\n` like so: +The default battery label is only displayed on the left-most (first) battery. +You can specify multiple battery labels by separating them with the literal `\n` (backslash-n), not an actual newline, like so: ```bash -set -g @dracula-battery-label "1:\n2:" +set -g @dracula-battery-label '1:\n2:'-additionally you can specify the separator between each battery like so:
+Additionally, you can specify the separator shown between batteries (default:"; ") like so:set -g @dracula-battery-separator "; "Notes: - Using single quotes around the label avoids any accidental interpolation of backslash sequences. - If users may supply fewer labels than detected batteries, please document how the remainder are labeled (e.g., omitted, repeated last label, or fall back to default).Please confirm:
1) The parser expects the literal two-character sequence "\n" in the option value (not an embedded newline).
2) Behavior when the number of labels != number of batteries (fewer or more labels than batteries).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
docs/CONFIG.md(2 hunks)scripts/battery.sh(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/battery.sh
🧰 Additional context used
🪛 LanguageTool
docs/CONFIG.md
[grammar] ~238-~238: There might be a mistake here.
Context: ...bel is only displayed in the very front. you can specify multiple battery labels ...
(QB_NEW_EN)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/CONFIG.md (1)
235-245: Polish grammar, clarify semantics, and document the default separator.Small editorial changes improve clarity and align terminology with the PR summary (“leftmost battery”). Also, explicitly call out that
\nis the literal 2-character sequence and surface the default separator to reduce ambiguity.Apply this diff:
-in case you have multiple batteries: +If you have multiple batteries: -the default battery label is only displayed in the very front. -you can specify multiple battery labels by splitting them with `\n` like so: +The default battery label is shown only on the leftmost battery. +You can specify multiple battery labels by separating them with the literal `\n` sequence, like so: ```bash set -g @dracula-battery-label "1:\n2:"-additionally you can specify the separator between each battery like so:
+Additionally, you can specify the separator between battery entries (default: "; ") like so:set -g @dracula-battery-separator "; "</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** **💡 Knowledge Base configuration:** - MCP integration is disabled by default for public repositories - Jira integration is disabled by default for public repositories - Linear integration is disabled by default for public repositories You can enable these settings in your CodeRabbit configuration. <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 4564d62a4d93285c64dad8c6ff22f390e8e1dfee and f35a171815cf01fcfca956677ebc9c084fe1f8cd. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `docs/CONFIG.md` (1 hunks) * `scripts/battery.sh` (3 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * scripts/battery.sh </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 LanguageTool</summary> <details> <summary>docs/CONFIG.md</summary> [grammar] ~237-~237: There might be a mistake here. Context: ...bel is only displayed in the very front. you can specify multiple battery labels ... (QB_NEW_EN) </details> </details> </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| you can specify multiple battery labels by splitting them with `\n` like so: | ||
| ```bash | ||
| # default is false | ||
| set -g @dracula-battery-hide-on-desktop true | ||
| set -g @dracula-battery-label "1:\n2:" | ||
| ``` |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Clarify number-of-labels vs. number-of-batteries behavior.
Please document what happens when the number of configured labels doesn’t match the number of detected batteries (e.g., do extra labels get ignored? do missing labels default to empty or reuse the first/last?). This helps users set expectations.
Run this script to locate the relevant logic in scripts/battery.sh and confirm the behavior so we can add a precise sentence to the docs:
🏁 Script executed:
#!/bin/bash
# Explore how labels and separators are read and applied.
echo "1) How is @dracula-battery-label read?"
rg -n -A3 -B3 '@dracula-battery-label' scripts/battery.sh
echo -e "\n2) Look for splitting on the literal \\n:"
rg -n -A2 -B2 '\\\\n' scripts/battery.sh
echo -e "\n3) How is @dracula-battery-separator read and applied?"
rg -n -A3 -B3 '@dracula-battery-separator' scripts/battery.sh
echo -e "\n4) Check logic around mapping labels to multiple batteries:"
rg -n -A4 -B4 -e 'label' -e 'labels' -e 'separator' -e 'map' -e 'join' scripts/battery.shLength of output: 3356
Document battery-label count behavior and newline semantics
Verified in scripts/battery.sh: labels are split on actual newline chars into an array and mapped one-to-one to detected batteries. Extra labels are ignored; missing labels produce empty (no) label; the separator is printed between battery entries. Default label is "♥" (setting the option to false yields an empty label).
Files/locations to reference:
- scripts/battery.sh
- bat_label read & default/false handling: line ~163 (bat_label=$(get_tmux_option "@dracula-battery-label" "♥"))
- splitting into array: line ~183 (IFS=$'\n' read -rd '' -a lbls <<<"$bat_label")
- mapping loop & separator: lines ~184–195 (num_bats, loop, separator at ~188, echo -n "${lbls[$i]}" at ~190)
- docs/CONFIG.md — update near lines 238–241 (the multi-label example)
Suggested one-line addition for docs/CONFIG.md (insert after the example):
Note: labels are split on actual newline characters and matched one-to-one to detected batteries — extra labels are ignored, and missing labels are left empty (no label). The @dracula-battery-separator is inserted between battery entries. The default label is "♥" (or empty if you set the option to false).
🤖 Prompt for AI Agents
In docs/CONFIG.md around lines 238–241, update the multi-label battery example
by adding a single-line note after the example stating that labels are split on
actual newline characters and matched one-to-one to detected batteries (extra
labels are ignored, missing labels produce an empty label), that
@dracula-battery-separator is inserted between battery entries, and that the
default label is "♥" (or empty if the option is set to false); base the wording
on scripts/battery.sh behavior (bat_label default/false handling ~line 163, IFS
split ~line 183, mapping loop & separator ~lines 184–195).
feat/fix/multi battery display
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: