search

regolith-linux / regolith-i3xrocks-config

Regolith customization of i3xrocks.
Other
29 stars 49 forks source link

Add support for multiple network interface stats #119

Open aacero opened 3 years ago

aacero commented 3 years ago

Some of my machines have multiple default interfaces and this caused the net-traffic script to not work. This pull request adds support for multiple interfaces and is backward compatible with the original script.

kgilmer commented 3 years ago

Hi @aacero thanks for your contribution! The code looks very clean. I ran it locally and didn't find any issues, although I don't have multiple active network interfaces.

It is common in this part of Regolith (status indicators) that a seemingly benign change causes regressions for users with various configurations we didn't consider. Unfortunately due to the nature of i3blocks these errors can result in big problems like spamming syslog or consuming 100% of a cpu core. Unrelated to your PR, but generally I have low confidence that any indicator shell script changes won't result in some regressions for some users.

Given this I propose that in the short term we rename this such that the existing script and your updates are available independently for users. Those wishing for network status for multiple interfaces would install another package, say i3xrocks-net-traffic-multi-nic or something. It would follow that rather than modification to net-traffic this would be a new script, net-traffic-multi-nic (or whatever is the better name).

aacero commented 3 years ago

@kgilmer -- renaming sounds good to me

kgilmer commented 3 years ago

Great, can you update the PR such that your changes are in new file as described? Then I'll merge and add a i3xrocks config script and package it for testing.

kgilmer commented 3 years ago

If you rebase from master the test regression should disappear @aacero

aacero commented 3 years ago

Will do. I currently don't have a way to test so it might be a week or two before I resubmit a PR

On Sun, Jul 4, 2021, 11:20 Ken Gilmer @.***> wrote:

If you rebase from master the test regression should disappear @aacero https://github.com/aacero

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/regolith-linux/regolith-i3xrocks-config/pull/119#issuecomment-873610638, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFSDKETQ5ELHL3JSH3IUQDTWB34TANCNFSM426VZ2DQ .