Added scrolling text to Mac player module and cleaned up functions #356
Conversation
Made sure that both modules have scrollable text as an option as exposed several other parameters that the user can use to customize
📝 Walkthrough""" WalkthroughScrolling text functionality was introduced for the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WidgetScript
participant Utils
User->>WidgetScript: Run widget (mac-player/playerctl)
WidgetScript->>WidgetScript: Read tmux config for scroll options
alt Scrolling enabled
WidgetScript->>Utils: Call scroll()/slice_text()
Utils-->>WidgetScript: Return scrolled substring
WidgetScript->>User: Output scrolling text
else Scrolling disabled
WidgetScript->>User: Output truncated text
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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 (
|
Reverted some formatting changes to previous code
![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
♻️ Duplicate comments (2)
docs/CONFIG.md (1)
691-697: Same wording issue as abovePlease apply the same wording fix for the playerctl section.
scripts/mac-player.sh (1)
193-203: Same “newline per frame” issue as in playerctlUse
printf "\r%s " …instead ofecho, otherwise the widget spams new lines.
🧹 Nitpick comments (3)
docs/CONFIG.md (1)
580-586: Grammar / clarity tweak“Lower speeds means faster scroll” → “Lower values mean faster scrolling”.
scripts/playerctl.sh (1)
5-7: Quote path when sourcing utilitiesIf the path contains spaces the current unquoted
sourcefails.-source $current_dir/utils.sh +source "$current_dir/utils.sh"scripts/mac-player.sh (1)
6-6: Quote utils path for robustness-source $current_dir/utils.sh +source "$current_dir/utils.sh"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/CONFIG.md(2 hunks)scripts/mac-player.sh(4 hunks)scripts/playerctl.sh(3 hunks)scripts/utils.sh(1 hunks)
🪛 Shellcheck (0.10.0)
scripts/mac-player.sh
[warning] 255-255: Declare and assign separately to avoid masking return values.
(SC2155)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
scripts/mac-player.sh
[warning] 255-255: Declare and assign separately to avoid masking return values.
(SC2155)
Added a couple fixes suggested by code rabbit
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
scripts/mac-player.sh (2)
227-230: Validate@dracula-mac-player-scroll-speedto prevent invalidsleeparguments
sleepaccepts only non-negative numbers. If a user sets the option to a negative or non-numeric value the script will exit withsleep: invalid time interval. Consider clamping or defaulting:case $SCROLL_SPEED in ''|*[!0-9.]*|-* ) SCROLL_SPEED=0.08 ;; esac
251-256: Avoid SC2155 & fork-less read for cache content
Assigning and declaring in one line hides the exit status and thecatfork is unnecessary.- local str=$(cat "$cache_file") + local str + IFS= read -r str <"$cache_file"Reduces process count and keeps shellcheck quiet.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
scripts/mac-player.sh(4 hunks)scripts/playerctl.sh(3 hunks)scripts/utils.sh(1 hunks)
🪛 Shellcheck (0.10.0)
scripts/mac-player.sh
[warning] 251-251: Declare and assign separately to avoid masking return values.
(SC2155)
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/utils.sh
- scripts/playerctl.sh
🧰 Additional context used
🪛 Shellcheck (0.10.0)
scripts/mac-player.sh
[warning] 251-251: Declare and assign separately to avoid masking return values.
(SC2155)
| @@ -3,8 +3,7 @@ | |||
| export LC_ALL=en_US.UTF-8 | |||
|
|
|||
| current_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | |||
| source "$current_dir/utils.sh" | |||
|
|
|||
| source $current_dir/utils.sh | |||
There was a problem hiding this comment.
Quote the sourced path to avoid breakage on paths containing spaces
Unquoted expansions break if the script is located in a directory with whitespace (e.g. “/Users/al ice/tmux”). Always quote path-like variables when sourcing or executing.
-source $current_dir/utils.sh
+source "$current_dir/utils.sh"📝 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.
| source $current_dir/utils.sh | |
| source "$current_dir/utils.sh" |
🤖 Prompt for AI Agents
In scripts/mac-player.sh at line 6, the sourced path variable $current_dir is
not quoted, which can cause the script to break if the directory path contains
spaces. Fix this by enclosing $current_dir/utils.sh in double quotes when using
source, like source "$current_dir/utils.sh", to ensure the path is correctly
interpreted even with spaces.
| # Scroll the text | ||
| function scroll() { | ||
| local str=$1 | ||
| local width=$2 | ||
| local speed=$3 | ||
|
|
||
| local scrolling_text="" | ||
| local i=0 | ||
| local len=${#str} | ||
|
|
||
| for ((i = 0; i <= len; i++)); do | ||
| scrolling_text=$(slice_text "$str" "$i" "$width") | ||
| printf "\r%s " "$scrolling_text" | ||
|
|
||
| sleep "$speed" | ||
| done | ||
|
|
||
| printf "\r%s " "$scrolling_text" | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
scroll() blocks for len*speed seconds and resets every run – consider a time-based slice instead
The current loop:
- Sleeps
lentimes, so a 60-char title at0.08sstalls the script for almost 5 s – longer than the default refresh rate and likely to spawn overlapping processes. - Restarts from index 0 on every invocation, causing a “jump” rather than seamless scroll.
- Uses
<= len; wheni == lenthe slice starts at the terminating NUL, returning an empty string.
A safer, non-blocking alternative is to compute the start index from the current epoch modulo len, output one slice, and let tmux refresh drive the animation:
-function scroll() {
- local str=$1
- local width=$2
- local speed=$3
-
- local len=${#str}
- for ((i = 0; i < len; i++)); do
- printf "\r%s " "$(slice_text "$str" "$i" "$width")"
- sleep "$speed"
- done
-}
+scroll() {
+ local str=$1 width=$2 speed=$3
+ local len=${#str}
+
+ # Calculate frame based on time so the function prints *once* per call.
+ local frame=$(( $(date +%s%N) / $(printf '1%0.s' {1..${#speed}}) % len ))
+ echo "$(slice_text "$str" "$frame" "$width")"
+}This keeps runtime ~ 1 ms, eliminates overlap and yields smooth, continuous scrolling.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In scripts/mac-player.sh around lines 187 to 206, the scroll() function blocks
execution for len*speed seconds and restarts the scroll from index 0 each time,
causing jumps and potential overlapping processes. To fix this, refactor
scroll() to compute the start index based on the current epoch time modulo the
string length, output only one slice per call, and remove the loop and sleep
calls. This change will make scrolling non-blocking, smooth, and continuous,
relying on external refresh timing to drive the animation.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
scripts/mac-player.sh (2)
6-6: Quote$current_dirwhen sourcing – unresolved from previous review
The path is still unquoted, so the script breaks if its directory contains spaces.-source $current_dir/utils.sh +source "$current_dir/utils.sh"
187-205:scroll()blocks for len × speed seconds and restarts every call – re-implement as a non-blocking, time-based slice
Running a full loop withsleepinside a status-line script stalls tmux, spawns overlapping processes and consumes CPU. It also “jumps” back to index 0 on every refresh.Replace the loop with a single, time-derived frame calculation:
-function scroll() { - local str=$1 - local width=$2 - local speed=$3 - - local scrolling_text="" - local i=0 - local len=${#str} - - for ((i = 0; i <= len; i++)); do - scrolling_text=$(slice_text "$str" "$i" "$width") - printf "\r%s " "$scrolling_text" - sleep "$speed" - done - - printf "\r%s " "$scrolling_text" -} +scroll() { + local str=$1 width=$2 speed=$3 + local len=${#str} + + # Frame index based on current time → smooth & stateless + # Using nanoseconds divided by speed-in-µs to get a rolling counter. + local frame=$(( ( $(date +%s%N) / $(printf '%d' "$(awk 'BEGIN{printf "%d",'"$speed"'*1000000}')") ) % len )) + + printf "%s" "$(slice_text "$str" "$frame" "$width")" +}Benefits: O(1 ms) runtime, no sleeps, no overlap, continuous scroll.
🧹 Nitpick comments (2)
scripts/mac-player.sh (2)
244-247: Quote$cache_fileto survive paths with spaces- sliceTrack "$(cat $cache_file)" "$MAX_LENGTH" >"$cache_file" + sliceTrack "$(cat "$cache_file")" "$MAX_LENGTH" >"$cache_file"
250-252: Separate declaration & command substitution to silence SC2155- local str=$(cat "$cache_file") + local str + str=$(cat "$cache_file")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/mac-player.sh(4 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
scripts/mac-player.sh
[warning] 251-251: Declare and assign separately to avoid masking return values.
(SC2155)
🔇 Additional comments (1)
scripts/mac-player.sh (1)
253-256: Calling the blockingscroll()here will freeze the status line until it completes
Even with a short title the loop lastslen × speedseconds (e.g. 3 s for 40 chars × 0.08 s), exceeding normal refresh intervals. Once the refactor above is applied, this concern disappears.
Initially, I did some work that added a small scrolling text effect to the
playerctlplugin last year (#273). I revisited the plugin and decided to do some clean up on it. I decided to expose some more parameters to users such as defining the width of the plugin and also the scrolling speed.Additionally, I love the new Macos player plugin (#343) and decided to also add a similar scrolling effect for users. I exposed the scrolling speed as a parameter for users that wish to have this option on. I believe that since both of these plugins have very similar functions, there should be a common function that can be used to get the scrolling characters on screen. This function was added to the
utils.shfile.Let me know if you have any questions or if there's something that needs to be fixed up!