-
-
Notifications
You must be signed in to change notification settings - Fork 62
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
Add librespot initial-volume option #276
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThe changes introduce a new configuration option, Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 1
🧹 Outside diff range and nitpick comments (7)
spotify/translations/en.yaml (1)
20-22
: Remove trailing spaces from descriptionThe YAML linter detected trailing spaces at the end of lines.
Apply this diff:
- Initial volume in % from 0-100. - Default for softvol: 50. - For the alsa mixer: the current volume. + Initial volume in % from 0-100. + Default for softvol: 50. + For the alsa mixer: the current volume.🧰 Tools
🪛 yamllint (1.35.1)
[error] 20-20: trailing spaces
(trailing-spaces)
[error] 21-21: trailing spaces
(trailing-spaces)
spotify/translations/it.yaml (1)
18-18
: Translate "Initial Volume" to ItalianFor consistency with other translations in the file, the name should be in Italian.
Apply this diff:
- name: Initial Volume + name: Volume Inizialespotify/translations/hu.yaml (1)
18-18
: Translate the name field to HungarianThe name field "Initial Volume" should be translated to Hungarian to maintain consistency with other configuration options in the file.
spotify/rootfs/etc/services.d/spotifyd/run (1)
18-19
: Add validation for initial-volume configuration valueThe script should validate that the initial-volume value is within the valid range (0-100) before passing it to librespot.
Consider adding validation like this:
# Initial Volume +initial_volume=$(bashio::config 'initial-volume') +if ! [[ "$initial_volume" =~ ^[0-9]+$ ]] || [ "$initial_volume" -lt 0 ] || [ "$initial_volume" -gt 100 ]; then + bashio::log.warning "Invalid initial-volume value: $initial_volume. Using default." + initial_volume=50 +fi -options+=(--initial-volume $(bashio::config 'initial-volume')) +options+=(--initial-volume "$initial_volume")spotify/DOCS.md (3)
78-81
: Enhance documentation for initial-volume optionThe documentation could be more detailed about:
- The behavior differences between softvol and alsa mixer
- When this setting takes effect (startup, crash recovery)
- How to determine which volume backend is in use
Consider expanding the documentation like this:
### Option: `initial-volume` -Initial volume in % from 0-100. Default for softvol: 50. For the alsa mixer: the current volume. +Initial volume in % from 0-100. This setting takes effect when the addon starts or +recovers from a crash. The behavior depends on the volume backend in use: + +- For softvol (default backend): Uses the specified value, defaulting to 50% +- For alsa mixer: Uses the specified value, falling back to the current system volume if not set + +To determine which backend is in use, check the addon logs for "Using softvol" or +"Using alsa mixer" messages during startup.
38-38
: Consider documenting initial-volume as optional in exampleSince this setting has reasonable defaults, consider making it clear in the example that it's optional.
-initial-volume: 50 +initial-volume: 50 # Optional, defaults to 50 for softvol
Translation needed for "Initial Volume" in Hungarian and Italian language files
spotify/translations/hu.yaml
: "Initial Volume" needs to be translated to Hungarianspotify/translations/it.yaml
: "Initial Volume" needs to be translated to ItalianThe description fields are properly translated in both files, but the name field remains in English.
🔗 Analysis chain
Line range hint
17-22
: Verify translations in all language filesLet's ensure this new configuration option is properly translated in all supported language files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if initial-volume is present in all translation files # and if the name field is properly translated (not in English) echo "Checking translation files for initial-volume..." for f in $(fd -e yaml . spotify/translations); do echo "=== $f ===" # Check if initial-volume exists grep -A 3 "initial-volume:" "$f" || echo "Missing initial-volume section" # Check if name is in English grep -A 1 "initial-volume:" "$f" | grep "name: Initial Volume" && echo "WARNING: Name field is in English" doneLength of output: 1584
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
spotify/DOCS.md
(2 hunks)spotify/config.yaml
(1 hunks)spotify/rootfs/etc/services.d/spotifyd/run
(1 hunks)spotify/translations/en.yaml
(1 hunks)spotify/translations/hu.yaml
(1 hunks)spotify/translations/it.yaml
(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
spotify/translations/en.yaml
[error] 20-20: trailing spaces
(trailing-spaces)
[error] 21-21: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
spotify/translations/it.yaml (1)
17-22
: Verify translations in all language files
The initial-volume configuration has been added to English and Italian translations. Let's verify if other language files need to be updated as well.
✅ Verification successful
All translation files include the initial-volume configuration
The verification shows that all YAML files in the spotify/translations
directory contain the initial-volume
configuration, as the grep search returned no files missing this entry. This indicates proper synchronization across all language files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all translation files and check for initial-volume entry
echo "Translation files without initial-volume configuration:"
fd -e yaml -e yml . spotify/translations -x grep -L "initial-volume" {}
Length of output: 194
Proposed Changes
Related Issues
Summary by CodeRabbit
Release Notes
New Features
initial-volume
, allowing users to set the starting volume level (0-100%) for the Spotify Connect add-on.initial-volume
option and its usage.Translations
initial-volume
configuration entry in English, Hungarian, and Italian translations, including name and description details for user clarity.