-
Notifications
You must be signed in to change notification settings - Fork 317
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 support for displaying network bandwidth usage in macOS #113
Conversation
showing fractions is noise and just clutters up the status bar
set download/upload to use fixed number of characters - up to 4. This stops the status bar annoyingly jerking left to right as bandwidth usage fluctuates. If bandwidth goes above 9999kb/s, then an extra character spaces will be used, and the status bar will move, but this should be relatively rare compared with going from 0 -> 00 etc
At start up, it would take $INTERVAL time till network bandwith is displayed, meaning when it does display after a second, everything moves around. Just display 0kB/s download/upload as a placeholder till then. Do this by defaulting totals to 0, and moving display to the top of the loop, rather than at the end.
change option names, and write them to correct new spelling option
Let me know if you want this squashed and/or have the separate fixes split out into different PRs |
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.
I really like your changes. Could you add some line in the INSTALL.md
regarding the config for linux users?
total_download_kbps=$(echo "scale=0; $total_download_bps / 1024" | bc) | ||
total_upload_kbps=$(echo "scale=0; $total_upload_bps / 1024" | bc) |
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.
bc
in not install by default on linux, at least on my distro. What about using
total_download_kbps=$(expr $total_download_bps / 1024)
total_upload_kbps=$(expr $total_upload_bps / 1024)
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.
Yup, we want to avoid using utilities that aren't installed by default. That includes bc
upload_bytes() { | ||
case $(uname -s) in | ||
Linux) | ||
cat "/sys/class/net/$network_name/statistics/rx_bytes" |
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.
This should be tx_bytes
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.
Whats the difference between the two?
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.
rx is for received bytes, tx is for transmitted
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.
I see, makes sense.
total_upload_kbps=0 | ||
while true | ||
do | ||
echo "$(printf "%8skB/s • %8skB/s" "↓$total_download_kbps" "↑$total_upload_kbps")" |
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.
echo "$(printf "↓%8skB/s • ↑%8skB/s" "$total_download_kbps" "$total_upload_kbps")"
Would this be more readable? (Just personal preference)
@@ -0,0 +1,58 @@ | |||
#!/usr/bin/env bash | |||
|
|||
INTERVAL="1" # update interval in seconds |
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.
This should follow the standard that the other files follow. See the gpu_usage.sh file for a good example.
|
||
INTERVAL="1" # update interval in seconds | ||
|
||
network_name=$(tmux show-option -gqv "@dracula-network-bandwidth") |
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.
This option needs documentation in INSTALL.md
cat "/sys/class/net/$network_name/statistics/rx_bytes" | ||
;; | ||
Darwin) | ||
netstat -I "$network_name" -b | tail -n 1 | awk '{print $10}' |
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.
Would it make sense to provide the -n
flag as well?
I have issues currently locally with netstat hanging if I skip it, which is because (I believe, correct me if I'm wrong) it's trying to resolve addresses but we don't care about that in this case. For reference, from the man page:
-n Show network addresses as numbers (normally netstat interprets addresses and attempts to
display them symbolically). This option may be used with any of the display formats.
but having the line as netstat -nbI "$network_name" | tail -n1 | awk '{print $10}'
works fine for me
@madlep any updates? |
@ethancedwards8 hey there. Got busy with work things, and forgot about this sorry! Suggestions look good. Will work them in and update the PR. |
I completely understand, not a problem, thanks for the work! |
With the merging of #123 , this PR will have to be refactored a bit. But overall, MacOS support would be a huge plus. |
Description of Changes
Major change is added support for displaying network bandwidth usage when running in macOS
While I was working on it, spotted a couple of other things to fix up
printf
so status bar didn't bounce around as the download/upload values changedtmux display-message
doesn't do anything until a session is actually up and running.Testing
Have run this on macOS, and works fine, however haven't been able to test on Linux. It should be ok as the only Linux specific code is kept the same.
Pic of plugin running on macOS