Skip to content
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

Added a Network widget #1292

Merged
merged 1 commit into from
Apr 8, 2022
Merged

Added a Network widget #1292

merged 1 commit into from
Apr 8, 2022

Conversation

seidnerj
Copy link
Contributor

It displays sent & received megabytes per second as well as a graph of the overall throughput per second.

@seidnerj
Copy link
Contributor Author

seidnerj commented Feb 14, 2022

I fixed the linting warnings, can try to re-run now.

@seidnerj
Copy link
Contributor Author

Hmmm, fetching the network stats fails on Windows due to a timeout for some reason, can't figure out why - any help would be appreciated.

@seidnerj
Copy link
Contributor Author

I tried replicating the issue that shows up in the windows tests using a Windows 11 VM but could not replicate it. Could someone help?

@seidnerj
Copy link
Contributor Author

Okay, fixed. All tests have passed, see here: https://github.com/seidnerj/homebridge-config-ui-x/actions/runs/1855056483

@seidnerj
Copy link
Contributor Author

Sorry for the all the mess, next time I'm going to work off of a feature branch and only merge to the fork's master when done.

At any rate, what exists right now in the fork's master branch works and passes all the tests.

Would appreciate merging this pull request.

Thanks!

@seidnerj
Copy link
Contributor Author

@oznu, can you please check this?

Thanks in advance

@oznu
Copy link
Member

oznu commented Feb 21, 2022

I'll review this soon. Quite busy with life at the moment sorry.

@seidnerj
Copy link
Contributor Author

seidnerj commented Feb 21, 2022 via email

@seidnerj
Copy link
Contributor Author

Is there any way I can help getting this merged? Thanks.

@oznu
Copy link
Member

oznu commented Apr 7, 2022

Thanks for this, looks good, however a few things:

  1. I think this should be an optional widget, the default layout should remain unchanged
  2. the network utilisation tracking should not occur server side if the widget is not enabled - I can see certain low powered devices not liking the additional calls to check network activity every few seconds. Maybe only start the interval to check network activity when requested for the first time?

@seidnerj
Copy link
Contributor Author

seidnerj commented Apr 7, 2022

I've removed the network widget from the default layout. Regarding your second point, am happy to do that but I'm not sure how - could you please point me to a similar behavior in the existing code or provide some other guidance? Thanks in advance

@oznu
Copy link
Member

oznu commented Apr 7, 2022

@seidnerj if you don't mind rebasing to the current master to resolve the merge conflict, I'll merge this in and fix up that part before doing a release.

If that works for you?

@seidnerj
Copy link
Contributor Author

seidnerj commented Apr 7, 2022 via email

…ond as well as a graph of the total throughput per second
@seidnerj
Copy link
Contributor Author

seidnerj commented Apr 7, 2022

Done. Thanks!

@oznu oznu changed the base branch from master to network-widget April 8, 2022 10:11
@oznu oznu merged commit f3190a4 into homebridge:network-widget Apr 8, 2022
@seidnerj
Copy link
Contributor Author

seidnerj commented Apr 8, 2022

Strange issue with the tests. I've run the e2e tests locally and it finished successfully. I also launched and used the server successfully. Any idea what's the issue?

@oznu
Copy link
Member

oznu commented Apr 8, 2022

Made a few changes if you would like to take a look 😄

4.43.0-test.7
https://github.com/oznu/homebridge-config-ui-x/wiki/How-To-Test-Upcoming-Changes
https://github.com/oznu/homebridge-config-ui-x/tree/network-widget

  1. Increased default width of the widget.
  2. Changed default position in mobile layout.
  3. Changed units from MB/s to Mb/s - most users i think still measure their internet speed in Mb/s.
  4. Polling will only start the first time the get network info method is called.

@seidnerj
Copy link
Contributor Author

seidnerj commented Apr 8, 2022

Looks great! Thanks a lot!

@oznu
Copy link
Member

oznu commented Apr 8, 2022

On second thoughts, maybe we should only poll for network activity when the client needs it. There is little point having 10 seconds worth of history before the page loads.

That will mean we aren't burning CPU cycles every second only to have the data discarded in 10 seconds.

@seidnerj
Copy link
Contributor Author

seidnerj commented Apr 8, 2022

Agreed. I guess this can be said about other stats as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants