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

Show interface names #340

Merged
merged 5 commits into from
Dec 8, 2023
Merged

Show interface names #340

merged 5 commits into from
Dec 8, 2023

Conversation

ilyes-ced
Copy link
Contributor

fixing old commits

solves issue #37 to some extent
unfortunately i wasn't able to figure out how to add the network interface type along side it's name

@ilyes-ced
Copy link
Contributor Author

tests keeps failing because the ui is changed, the change is only made to the header (title of page)
i need some guidance fixing those test as i am unfamiliar with them and i don't want to mess them up

comparison

old ui

old_ui_title

new ui all interfaces (no -i flag)

new_ui_with_all_interfaces_title

new ui with 1 interface (-i flag)

new_ui_with_an_interface_title

src/display/components/header_details.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/display/ui_state.rs Outdated Show resolved Hide resolved
@cyqsimon
Copy link
Collaborator

cyqsimon commented Dec 6, 2023

tests keeps failing because the ui is changed, the change is only made to the header (title of page)
i need some guidance fixing those test as i am unfamiliar with them and i don't want to mess them up

We use insta for most of our tests. I recommend installing the companion tool cargo-insta, then you can use cargo insta test and cargo insta review to update the snapshots.

P.S. and don't worry if there are spontaneous test failures in CI for Windows/MacOS. These are known issues and are being worked on. As long as CI passes for Ubuntu, you should be good to go.

@cyqsimon cyqsimon changed the title added interface names (reposted) Show interface names Dec 6, 2023
@cyqsimon
Copy link
Collaborator

cyqsimon commented Dec 8, 2023

Looks alright now. There are a few small things I'l touch up and then we'll merge.

Comment on lines 185 to 187
pub fn update_interface_name(&mut self, interface_name: Option<String>) {
self.state.interface_name = interface_name;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This kind of trivial setter method is very "Java-esque" and creates a lot of (frankly unnecessary) boilerplate code. We typically don't do this in Rust.

src/main.rs Outdated
@@ -155,6 +155,7 @@ where
let mut ui = ui.lock().unwrap();
let paused = paused.load(Ordering::SeqCst);
let ui_offset = ui_offset.load(Ordering::SeqCst);
ui.update_interface_name(opts.interface.clone());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't need to run in the main loop.

@cyqsimon
Copy link
Collaborator

cyqsimon commented Dec 8, 2023

Changelog detection in CI is not working due to an unhandled edge case, which was fixed in #342. Merging.

@cyqsimon cyqsimon merged commit 5648be6 into imsnif:main Dec 8, 2023
10 of 11 checks passed
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.

2 participants