-
Notifications
You must be signed in to change notification settings - Fork 337
feat: Hide battery widget on Desktop PCs #353
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
feat: Hide battery widget on Desktop PCs #353
Conversation
📝 WalkthroughWalkthroughA new configuration option, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Tmux
participant BatteryScript
User->>Tmux: Loads Dracula theme
Tmux->>BatteryScript: Executes battery.sh
BatteryScript->>BatteryScript: Retrieve battery percentage
BatteryScript->>BatteryScript: Check @dracula-battery-hide-on-desktop option
alt No battery and hide-on-desktop is true
BatteryScript-->>Tmux: Output empty string
else
BatteryScript->>BatteryScript: Continue with normal battery display logic
BatteryScript-->>Tmux: Output battery status
end
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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: 0
🧹 Nitpick comments (2)
docs/CONFIG.md (1)
222-227
: Unify wording & boolean‐option style with surrounding docsThe preceding paragraphs use the wording “set foo false to hide” (e.g.
@dracula-no-battery-label false
).
To avoid confusion, consider re-using that style here (“set … true to hide”) or mention explicitly how this option interacts with@dracula-no-battery-label
.
This keeps the documentation consistent and lowers the cognitive load for users skimming the file.scripts/battery.sh (1)
175-181
: Treat@dracula-battery-hide-on-desktop
as a real boolean
get_tmux_option
already returns a bare word that can be used as a shell boolean (the pattern used for@dracula-show-battery-status
). Re-using that approach avoids a string comparison and keeps the code symmetric:- hide_on_desktop="$(get_tmux_option "@dracula-battery-hide-on-desktop" "false")" - # If no battery percent, don't show anything - if [ -z "$bat_perc" ] && [ "$hide_on_desktop" = "true" ]; then + hide_on_desktop=$(get_tmux_option "@dracula-battery-hide-on-desktop" false) + # If no battery percent and the feature flag is enabled, hide the widget completely + if $hide_on_desktop && [ -z "$bat_perc" ]; then echo "" return fiBenefits
• Removes a subshell and a quoted string comparison.
• Aligns with the existing boolean-option idiom used a few lines above for@dracula-show-battery-status
.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/CONFIG.md
(1 hunks)scripts/battery.sh
(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
scripts/battery.sh (1)
scripts/utils.sh (1)
get_tmux_option
(3-12)
Nice catch. Thanks. |
hi, we actually already had a solution for that: set -g @dracula-no-battery-label false though i must admit that it wasn't documented. |
@Theoreticallyhugo I apologize. I missed that. Feel free to revert and add the appropriate documentation. Also @lfreixial , I apologize to you as well. |
@Theoreticallyhugo I saw that one but I wasn't 100% sure what it was meant to do as when I tried using it the battery still appeared and when I manually run the ls: cannot access '/sys/class/power_supply/*': No such file or directory
♥ Perhaps there is bug somewhere causing it to not disappear with |
@ethancedwards8 It's no worries at all! No need to apologize! |
Checks to see if there is no battery and if
@dracula-battery-hide-on-desktop"
is true it will hide the battery widget.Useful for when a config is shared between a Laptop and a Desktop and only have the battery hidden on the Desktop