-
Notifications
You must be signed in to change notification settings - Fork 334
feat:spr plugin #344
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?
feat:spr plugin #344
Conversation
```bash | ||
set -g @dracula-spotify-tui-max-len 30 | ||
``` | ||
|
||
`set -g @dracula-refresh-rate 5` affects this widget | ||
`set remote-g @dracula-refresh-rate 5` affects this widget |
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.
is this change intentional?
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.
yeah, I noticed that the when I place like a refresh rate lke 2, the presentation was faster. Maybe it's an issue with my current installation.
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.
since the refresh rate option is essentially a sleep statement, lower numbers should yield less delay between the refreshes, so what you say makes sense. however, im am a little confused as to why you changed the flag from -g
to remote-g
which i think would be considered illegal syntax. hence the question of whether this is intentional. sorry for having been so unclear
📝 Walkthrough""" WalkthroughThe changes introduce a new "spr" plugin for Spotify integration, updating the documentation and main plugin script to support it. A new Bash script implements the plugin's functionality, including track display and remote control features. Documentation is expanded and refined for clarity, with formatting improvements and detailed instructions for the new plugin. Changes
Sequence Diagram(s)sequenceDiagram
participant Tmux
participant dracula.sh
participant spr.sh
participant spotify_player
Tmux->>dracula.sh: Selects "spr" plugin
dracula.sh->>spr.sh: Executes spr.sh
spr.sh->>spotify_player: Query playback status (get key playback)
spotify_player-->>spr.sh: Returns JSON playback info
spr.sh->>spr.sh: Parse status, format track info
spr.sh->>Tmux: Output track info for display
Note over spr.sh,spotify_player: If remote control enabled
Tmux->>spr.sh: User presses remote key (play/pause, next, back)
spr.sh->>spotify_player: Sends corresponding control command
""" 📜 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: 2
🧹 Nitpick comments (4)
scripts/dracula.sh (1)
275-278
: Quote substitutions in color option parsing
The here-string expansion should be quoted to prevent unintended word splitting when colors include spaces.
Apply this diff:- IFS=' ' read -r -a colors <<<$(get_tmux_option "@dracula-spr-colors" "green dark_gray") + IFS=' ' read -r -a colors <<< "$(get_tmux_option "@dracula-spr-colors" "green dark_gray")"scripts/spr.sh (1)
108-111
: Improve cache timestamp portability
stat -f%c
is BSD-specific and will fail on Linux. Use a portable method to retrieve file modification time:- if [ ! -f "$cache_file" ] || [ $(($(date +%s) - $(stat -f%c "$cache_file"))) -ge "$RATE" ]; then + if [ ! -f "$cache_file" ] || [ $(( $(date +%s) - $(date +%s -r "$cache_file") )) -ge $RATE ]; thendocs/CONFIG.md (2)
639-642
: Clarify remote feature description and correct casing
Rewrite for clarity and consistency with macOS styling:-This contains a remote (experimental) but limits the use of the display widget in MacOS. +This section includes an experimental remote control feature, but it may limit the widget’s display on macOS.
646-649
: Specify code block languages for syntax highlighting
Add the language identifier to all fenced blocks to improve readability and consistency:-``` +```bash set -g @dracula-spr-play-icon "♪ " set -g @dracula-spr-pause-icon "❚❚ " -``` +```bash set -g @dracula-spr-remote true -``` +```bash set -g @dracula-spr-remote-pp "P" set -g @dracula-spr-remote-back "R" set -g @dracula-spr-remote-next "N" -``` +```bashAlso applies to: 658-660, 670-673
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/CONFIG.md
(15 hunks)scripts/dracula.sh
(1 hunks)scripts/spr.sh
(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
scripts/dracula.sh
[warning] 276-276: Quote this to prevent word splitting.
(SC2046)
🪛 markdownlint-cli2 (0.17.2)
docs/CONFIG.md
33-33: Link fragments should be valid
null
(MD051, link-fragments)
327-327: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
539-539: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
616-616: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
693-693: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
715-715: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
717-717: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
759-759: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
765-765: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
775-775: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🪛 LanguageTool
docs/CONFIG.md
[uncategorized] ~219-~219: Possible missing comma found.
Context: .... _make sure to include the compact-alt widget as you won't be able to switch out of n...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~316-~316: It seems likely that a singular genitive (’s) apostrophe is missing.
Context: ...orking directory, where the path of the users home directory will be shortened to ~
...
(AI_HYDRA_LEO_APOSTROPHE_S_XS)
[uncategorized] ~339-~339: It appears that hyphens are missing in the adjective “up-to-date”.
Context: ...` Set symbol to use for when branch is up to date with HEAD ```bash # default is ✓. Avoi...
(UP_TO_DATE_HYPHEN)
[grammar] ~382-~382: The operating system from Apple is written “macOS”.
Context: ... - partial support for apple m-chips on MacOS. - partial support for amd and intel on...
(MAC_OS)
[uncategorized] ~436-~436: It appears that hyphens are missing in the adjective “up-to-date”.
Context: ...` Set symbol to use for when branch is up to date with HEAD ```bash #default is ✓.Avoid ...
(UP_TO_DATE_HYPHEN)
[typographical] ~520-~520: Usually, there’s no comma before “if”.
Context: ...nly display the wifi you're connected to, if it provides internet access!** You can...
(IF_NO_COMMA)
[style] ~522-~522: Consider a more concise word here.
Context: ...** You can use different hosts to ping in order to check for a wifi or wired connection. I...
(IN_ORDER_TO_PREMIUM)
[grammar] ~553-~553: The verb form ‘are’ does not seem to match the subject ‘name’.
Context: ...rface. The most common interfaces name are eth0
for a wired connection and `wlan...
(SUBJECT_VERB_AGREEMENT)
[grammar] ~652-~652: The operating system from Apple is written “macOS”.
Context: ...limits the use of the display widget in MacOS. In order to utilize the remote featur...
(MAC_OS)
[style] ~653-~653: Consider a more concise word here.
Context: ...he use of the display widget in MacOS. In order to utilize the remote feature you need to ...
(IN_ORDER_TO_PREMIUM)
[typographical] ~654-~654: It seems that a comma is missing.
Context: ... MacOS. In order to utilize the remote feature you need to install the [spotify-player...
(IN_ORDER_TO_VB_COMMA)
[grammar] ~677-~677: Probable usage error. Use “and” after ‘both’.
Context: ... combination, both of the local machine as well as when connected via ssh. To output noth...
(BOTH_AS_WELL_AS)
🔇 Additional comments (1)
docs/CONFIG.md (1)
33-33
: TOC entry added correctly
The newspr
plugin entry in the Table of Contents matches the section heading and fragment.
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/CONFIG.md
(15 hunks)scripts/spr.sh
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/spr.sh
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
docs/CONFIG.md
33-33: Link fragments should be valid
null
(MD051, link-fragments)
327-327: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
539-539: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
616-616: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
693-693: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
715-715: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
717-717: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
759-759: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
765-765: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
775-775: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🪛 LanguageTool
docs/CONFIG.md
[uncategorized] ~219-~219: Possible missing comma found.
Context: .... _make sure to include the compact-alt widget as you won't be able to switch out of n...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~316-~316: It seems likely that a singular genitive (’s) apostrophe is missing.
Context: ...orking directory, where the path of the users home directory will be shortened to ~
...
(AI_HYDRA_LEO_APOSTROPHE_S_XS)
[uncategorized] ~339-~339: It appears that hyphens are missing in the adjective “up-to-date”.
Context: ...` Set symbol to use for when branch is up to date with HEAD ```bash # default is ✓. Avoi...
(UP_TO_DATE_HYPHEN)
[grammar] ~382-~382: The operating system from Apple is written “macOS”.
Context: ... - partial support for apple m-chips on MacOS. - partial support for amd and intel on...
(MAC_OS)
[uncategorized] ~436-~436: It appears that hyphens are missing in the adjective “up-to-date”.
Context: ...` Set symbol to use for when branch is up to date with HEAD ```bash #default is ✓.Avoid ...
(UP_TO_DATE_HYPHEN)
[typographical] ~520-~520: Usually, there’s no comma before “if”.
Context: ...nly display the wifi you're connected to, if it provides internet access!** You can...
(IF_NO_COMMA)
[style] ~522-~522: Consider a more concise word here.
Context: ...** You can use different hosts to ping in order to check for a wifi or wired connection. I...
(IN_ORDER_TO_PREMIUM)
[grammar] ~553-~553: The verb form ‘are’ does not seem to match the subject ‘name’.
Context: ...rface. The most common interfaces name are eth0
for a wired connection and `wlan...
(SUBJECT_VERB_AGREEMENT)
[style] ~653-~653: Consider a more concise word here.
Context: ...y limit the widget’s display on macOS. In order to utilize the remote feature you need to ...
(IN_ORDER_TO_PREMIUM)
[typographical] ~654-~654: It seems that a comma is missing.
Context: ... macOS. In order to utilize the remote feature you need to install the [spotify-player...
(IN_ORDER_TO_VB_COMMA)
[grammar] ~677-~677: Probable usage error. Use “and” after ‘both’.
Context: ... combination, both of the local machine as well as when connected via ssh. To output noth...
(BOTH_AS_WELL_AS)
```bash | ||
set -g @dracula-spotify-tui-max-len 30 | ||
``` | ||
|
||
`set -g @dracula-refresh-rate 5` affects this widget | ||
`set remote-g @dracula-refresh-rate 5` affects this widget |
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.
Correct tmux option syntax for refresh rate.
The line currently reads:
set remote-g @dracula-refresh-rate 5
It mixes up the -g
flag. It should be:
set -g @dracula-refresh-rate 5
- set remote-g @dracula-refresh-rate 5
+ set -g @dracula-refresh-rate 5
🤖 Prompt for AI Agents
In docs/CONFIG.md at line 636, the tmux option syntax is incorrect; it uses "set
remote-g" instead of "set -g". Correct the line to "set -g @dracula-refresh-rate
5" to properly apply the global option flag.
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
♻️ Duplicate comments (1)
docs/CONFIG.md (1)
636-636
: Correct tmux Option Syntax for Refresh Rate
This line still usesset remote-g @dracula-refresh-rate 5
, which should beset -g @dracula-refresh-rate 5
.
🧹 Nitpick comments (2)
docs/CONFIG.md (2)
219-219
: Sentence Capitalization
The line starts with a lowercase “to”. Consider capitalizing it:- to set what widgets should be shown in narrow mode, set the following variable... + To set what widgets should be shown in narrow mode, set the following variable...
654-655
: Grammar & Conciseness
Consider simplifying and adding a comma for clarity:- In order to utilize the remote feature you need to install the spotify-player-daemon + To utilize the remote feature, install the spotify-player-daemon
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/CONFIG.md
(15 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/CONFIG.md
[uncategorized] ~219-~219: Possible missing comma found.
Context: .... _make sure to include the compact-alt widget as you won't be able to switch out of n...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~316-~316: It seems likely that a singular genitive (’s) apostrophe is missing.
Context: ...orking directory, where the path of the users home directory will be shortened to ~
...
(AI_HYDRA_LEO_APOSTROPHE_S_XS)
[uncategorized] ~339-~339: It appears that hyphens are missing in the adjective “up-to-date”.
Context: ...` Set symbol to use for when branch is up to date with HEAD ```bash # default is ✓. Avoi...
(UP_TO_DATE_HYPHEN)
[grammar] ~382-~382: The operating system from Apple is written “macOS”.
Context: ... - partial support for apple m-chips on MacOS. - partial support for amd and intel on...
(MAC_OS)
[uncategorized] ~436-~436: It appears that hyphens are missing in the adjective “up-to-date”.
Context: ...` Set symbol to use for when branch is up to date with HEAD ```bash #default is ✓.Avoid ...
(UP_TO_DATE_HYPHEN)
[typographical] ~520-~520: Usually, there’s no comma before “if”.
Context: ...nly display the wifi you're connected to, if it provides internet access!** You can...
(IF_NO_COMMA)
[style] ~522-~522: Consider a more concise word here.
Context: ...** You can use different hosts to ping in order to check for a wifi or wired connection. I...
(IN_ORDER_TO_PREMIUM)
[grammar] ~553-~553: The verb form ‘are’ does not seem to match the subject ‘name’.
Context: ...rface. The most common interfaces name are eth0
for a wired connection and `wlan...
(SUBJECT_VERB_AGREEMENT)
[style] ~653-~653: Consider a more concise word here.
Context: ...y limit the widget’s display on macOS. In order to utilize the remote feature you need to ...
(IN_ORDER_TO_PREMIUM)
[typographical] ~654-~654: It seems that a comma is missing.
Context: ... macOS. In order to utilize the remote feature you need to install the [spotify-player...
(IN_ORDER_TO_VB_COMMA)
[grammar] ~677-~677: Probable usage error. Use “and” after ‘both’.
Context: ... combination, both of the local machine as well as when connected via ssh. To output noth...
(BOTH_AS_WELL_AS)
🪛 markdownlint-cli2 (0.17.2)
docs/CONFIG.md
327-327: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
539-539: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
616-616: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
693-693: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
715-715: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
717-717: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
759-759: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
765-765: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
775-775: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🔇 Additional comments (9)
docs/CONFIG.md (9)
4-4
: Trivial blank line insertions for improved readability.Also applies to: 92-92, 100-100, 126-126, 151-151, 637-637
33-33
: TOC Entry forspr
Plugin Added
The new Table of Contents entry- [spr](#spr---up)
matches the heading’s anchor pattern and is consistent with other plugins.
638-638
: Header forspr
Plugin
The heading### spr - [up](#table-of-contents)
is consistent with other plugin sections.
642-642
: Refresh Rate Note
The noteNOTE: \
set -g @dracula-refresh-rate 5` affects this widget` follows the established pattern.
644-644
: Player Icon Configuration
The instructionTo change player icons:
aligns with other “To change” sections.
646-649
: Play/Pause Icon Snippet
The provided code block correctly shows how to set the play and pause icons.
652-652
: Experimental Remote Feature Note
The notice about the experimental remote control feature is clear and properly formatted.
658-659
: Enable Remote Snippet
The snippetset -g @dracula-spr-remote true
correctly shows how to activate remote control.
669-673
: Remote Keybindings Snippet
The code block for customizing<prefix> + P/R/N
is consistent and clear.
hi, im sorry for the long wait. currently im rarely using macos, so i dont really get to install and test fancy things on it. ill try not to keep you waiting for too long anymore |
The spotify_player tui supports various linux distributions is not only for macos. If you have cargo installed you can install it seemlessly. |
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
♻️ Duplicate comments (1)
docs/CONFIG.md (1)
718-720
: Invalid tmux flagremote-g
; should be-g
remote-g
is not recognised by tmux. Fix the example:-`set remote-g @dracula-refresh-rate 5` affects this widget +`set -g @dracula-refresh-rate 5` affects this widget
🧹 Nitpick comments (2)
scripts/dracula.sh (1)
281-283
: Shellcheck SC2046 – quote the command-substitution or silence intentionallyShellcheck flags the unquoted command substitution.
If you do want the value to be fed toread
as a single string (split afterwards byIFS
), quoting is safe and silences the warning:- IFS=' ' read -r -a colors <<<$(get_tmux_option "@dracula-spr-colors" "green dark_gray") + IFS=' ' read -r -a colors <<<"$(get_tmux_option "@dracula-spr-colors" "green dark_gray")"Alternatively add
# shellcheck disable=SC2046
for consistency with the rest of the file.docs/CONFIG.md (1)
725-758
: Duplicate refresh-rate note – keep only oneThe sentence noting that
@dracula-refresh-rate
affects the widget appears twice (lines 725 and 758). Removing the second keeps the section concise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/CONFIG.md
(3 hunks)scripts/dracula.sh
(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
scripts/dracula.sh (1)
scripts/utils.sh (1)
get_tmux_option
(3-12)
🪛 LanguageTool
docs/CONFIG.md
[style] ~736-~736: Consider a more concise word here.
Context: ...y limit the widget’s display on macOS. In order to utilize the remote feature you need to ...
(IN_ORDER_TO_PREMIUM)
🪛 Shellcheck (0.10.0)
scripts/dracula.sh
[warning] 282-282: Quote this to prevent word splitting.
(SC2046)
is it normal for the tmux session to switch to a blank screen till you press enter when trying to use the remote control feature? besides it's displaying well! formatting options like there used to be with spotify-tui would be cool, but not a must ( |
I just noticed that and will try to solve this issue. is it when you do the 'R' remote command? # Remote Access
if [[ "$REMOTE_ACCESS" == true ]]; then
remoteControl "$PLAY_PAUSE_BUTTON" "$BACK_BUTTON" "$NEXT_BUTTON" "$REMOTE_APP"
fi then add a guard to prevent the commands from leaking to other commands in tmux. my bad :p . if [[ "$REMOTE_ACCESS" == "true" ]]; then
# Remote Control Buttons Customizations
PLAY_PAUSE_BUTTON=$(get_tmux_option "@dracula-mac-player-remote-play-pause" "P")
BACK_BUTTON=$(get_tmux_option "@dracula-mac-player-remote-back" "R")
NEXT_BUTTON=$(get_tmux_option "@dracula-mac-player-remote-next" "N")
remoteControl "$PLAY_PAUSE_BUTTON" "$BACK_BUTTON" "$NEXT_BUTTON" "$REMOTE_APP"
else
tmux set -g @dracula-mac-player-remote-play-pause ""
tmux set -g @dracula-mac-player-remote-back ""
tmux set -g @dracula-mac-player-remote-next ""
fi
for the formatting options, I was finding ways to store the fields separately to the formatting options like in spotify-tui cause spotify-player doesn't provide the formatting options. The data they expose is in json but I'll figure something out. |
so I found out that I was solving another error based on the mac-player where spotify does not have a back track option in applescript. For the terminal going blank issue, I researched and found out the tui commands creates an exit command that leads to tmux showing a blank page. I've added this prefix and also polished up the remote control issues and still working on the formatting options. |
This is based on the creation of spotify-player widget for the spotify-player app based on #336
There is also a remote option but it has some issues with latency and needs the daemon to be installed but limits the refresh rate on the display widget.