Add network interface to UI #112

captain-yossarian · 2020-01-12T14:00:09Z

Regarding this #37 issue

imsnif

Hey, sorry it took me a short while to get to this!

This is a good start. I left some comments on the direction. Let's make this happen. :)

imsnif · 2020-01-14T17:28:59Z

src/display/components/total_bandwidth.rs

@@ -20,10 +20,12 @@ impl<'a> TotalBandwidth<'a> {
            } else {
                Color::Green
            };
+            let interface = datalink::interfaces().into_iter().map(|interface|interface.name).collect::<Vec<String>>();


So, we actually already do something similar on the OS layer. Since we have this information already, I think it would be best if we pass it to the UI in order not to have to do the query a second time (and possibly get different results if something has changed on the system while the app was running, for example).

I think the place to pass this information to the UI would be here: https://github.com/imsnif/bandwhich/blob/master/src/main.rs#L117

imsnif · 2020-01-14T17:30:35Z

src/display/components/total_bandwidth.rs


            [Text::styled(
                format!(
-                    " Total Rate Up / Down: {} / {} {}",
+                    " Interfaces {:?} / Total Rate Up / Down: {} / {} {}",


I'm a little concerned that with a lot of interfaces, this line might become rather long.
Since we can only listen on one interface or on all of them, what do you think about specifying either the one interface name, or something like "*" (or ALL?) if there is more than one?

@imsnif It was my mistake to import network interface. Variable interface_name is Option<String> not vector.
So, there is no need to to use "*"
Could you please take a look again?

imsnif

Hey, left some comments.

imsnif · 2020-01-18T00:52:35Z

src/main.rs

@@ -122,7 +122,7 @@ where
    let raw_mode = opts.raw;

    let network_utilization = Arc::new(Mutex::new(Utilization::new()));
-    let ui = Arc::new(Mutex::new(Ui::new(terminal_backend, opts.render_opts)));
+    let ui = Arc::new(Mutex::new(Ui::new(terminal_backend, opts.render_opts, opts.interface)));


If we already have a render_opts, maybe we can make the interface name part of it?

imsnif · 2020-01-18T00:56:33Z

src/display/components/total_bandwidth.rs

            [Text::styled(
                format!(
-                    " Total Rate Up / Down: {} / {} {}",
+                    " Interfaces {} / Total Rate Up / Down: {} / {} {}",
+                    DisplayString(interface),


I'm not sure I understand... why do we need DisplayString here? Why not input the interface name directly?

Also, could we make the display:

if there is a specific interface passed in the options, we will display <interface_name>: Total Rate Up / Down: {} / {} {}

if there is no interface (meaning we're listening on all interfaces), we'll just display the previous string without the interfaces

What do you say?

@imsnif Sure. I've made all changes.
Travic CI build failing every time. Do you know the reason?
Also, I don't know why, but on my machine all tests are failing every time, even on master branch.

As for travis: you can see the output of the snapshot tests (these are essentially tests that run the app under certain situations take a "snapshot" of the terminal screen and see that the output is what they expect - these should be quite readable, so you can look at the travis output linked in this PR and check them out).

As for your local tests: if you tell me more about why they are failing, maybe we can try and debug it?

@imsnif After cargo test command, insta package creates new files.

Complete log
Part of the log with color for readability

P.S. What do you think about my last changes ? Is it ok?
P.S.S. Sorry for taking your time, I'm really want to help, but I have not a lot experience in Rust. I'm only learning.

No worries friend! We all have to start sometime, right? Thanks for taking the time to work on this. :) The snapshots should be part of the repository, so it's odd that the tests are creating them instead. I'd try to look in that direction.
I'll take a look at your changes here and in the other PR in the next few days.

captain-yossarian · 2020-01-19T18:58:52Z

@imsnif Travis CI tests failed bacause snapshots are incompatible

I dont understand why there is interface_name in snapshot. According to my changes there should be either nothing or interface name

imsnif · 2020-01-21T17:45:18Z

Hey @SerhiiBilyk - I think it will be a lot easier for you if you get the tests running locally. I find that they make development easier for me by quickly showing me the effects of my changes in various situations in the app (without having to push to the branch and waiting for travis).

It's very odd that they create new snapshots for you (as you wrote above). Could you maybe try to re-clone the repository somewhere else and just running cargo test? Do they also create the snapshots that way?

captain-yossarian · 2020-01-24T20:02:51Z

@imsnif Hi, sorry for delay.
I've cloned the project to other folder.
Same problem. All tests has been failed and new snapshot created.

Btw, I have Ubuntu 18 and rust 1.40.0

Could you please checkout on my branch and run the tests?
P.S. I've tried cargo insta accept && cargo insta review it partialy helps, but it anyway creates new snapshots

imsnif · 2020-02-11T17:40:08Z

The snapshots are created by the tests only if they are missing in the snapshots folder.

If this happens to you on a fresh clone of the repository, I cannot explain it. This is not reproduced for me (or for our CI) - to the best of my knowledge this only happens to you. I don't have any ideas more clever than "clone the repository and look in the snapshots folder", sorry :/

captain-yossarian · 2020-02-15T14:06:22Z

If somebody is interested to finish this PR, feel free to pick it up, because I have an issues with testing framework on my local machine

imsnif · 2020-09-13T16:33:27Z

Hey, this was closed by mistake and I don't seem to be able to re-open it. Please feel free to re-open or make another PR with this if you wish. My apologies!

captain-yossarian closed this Jan 12, 2020

captain-yossarian reopened this Jan 12, 2020

captain-yossarian added 3 commits January 13, 2020 20:12

Add interface to UI

f5c37f2

Remove unused code

cf82d0a

Add ref

4f387d4

imsnif self-assigned this Jan 14, 2020

imsnif requested changes Jan 14, 2020

View reviewed changes

captain-yossarian added 9 commits January 15, 2020 20:23

Add interface to UI

be4844f

Remove unused code

62423a1

Add ref

21cf0e5

Resolve conflicts

dfb1fcd

Add interface from main

d9cbaca

Remove whitespace

17460c0

Sync

cd27ff1

Sync

7eb1688

Update

69f25bc

imsnif requested changes Jan 18, 2020

View reviewed changes

captain-yossarian added 3 commits January 18, 2020 15:38

Remove string format

121537d

Reduce arguments

6407bea

Add helper

fef7167

captain-yossarian mentioned this pull request Jan 25, 2020

Question: Every time insta creates new snapshots mitsuhiko/insta#95

Closed

captain-yossarian mentioned this pull request Feb 7, 2020

Add custom error handling #104

Merged

imsnif closed this Sep 13, 2020

bjohnnyd mentioned this pull request May 3, 2022

Display the monitored interface and (if available) name [enhancement] #37

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add network interface to UI #112

Add network interface to UI #112

captain-yossarian commented Jan 12, 2020 •

edited

Loading

imsnif left a comment

imsnif Jan 14, 2020

imsnif Jan 14, 2020

captain-yossarian Jan 15, 2020 •

edited

Loading

imsnif left a comment

imsnif Jan 18, 2020

imsnif Jan 18, 2020

captain-yossarian Jan 18, 2020

imsnif Jan 19, 2020

captain-yossarian Jan 19, 2020 •

edited

Loading

imsnif Jan 20, 2020

captain-yossarian commented Jan 19, 2020

imsnif commented Jan 21, 2020

captain-yossarian commented Jan 24, 2020 •

edited

Loading

imsnif commented Feb 11, 2020

captain-yossarian commented Feb 15, 2020

imsnif commented Sep 13, 2020

Add network interface to UI #112

Add network interface to UI #112

Conversation

captain-yossarian commented Jan 12, 2020 • edited Loading

imsnif left a comment

Choose a reason for hiding this comment

imsnif Jan 14, 2020

Choose a reason for hiding this comment

imsnif Jan 14, 2020

Choose a reason for hiding this comment

captain-yossarian Jan 15, 2020 • edited Loading

Choose a reason for hiding this comment

imsnif left a comment

Choose a reason for hiding this comment

imsnif Jan 18, 2020

Choose a reason for hiding this comment

imsnif Jan 18, 2020

Choose a reason for hiding this comment

captain-yossarian Jan 18, 2020

Choose a reason for hiding this comment

imsnif Jan 19, 2020

Choose a reason for hiding this comment

captain-yossarian Jan 19, 2020 • edited Loading

Choose a reason for hiding this comment

imsnif Jan 20, 2020

Choose a reason for hiding this comment

captain-yossarian commented Jan 19, 2020

imsnif commented Jan 21, 2020

captain-yossarian commented Jan 24, 2020 • edited Loading

imsnif commented Feb 11, 2020

captain-yossarian commented Feb 15, 2020

imsnif commented Sep 13, 2020

captain-yossarian commented Jan 12, 2020 •

edited

Loading

captain-yossarian Jan 15, 2020 •

edited

Loading

captain-yossarian Jan 19, 2020 •

edited

Loading

captain-yossarian commented Jan 24, 2020 •

edited

Loading