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

Fix net block hiding issue #897

Merged
merged 4 commits into from
Oct 23, 2020
Merged

Conversation

GladOSkar
Copy link
Contributor

@GladOSkar GladOSkar commented Oct 22, 2020

Fix net block issue where both hide_inactive and hide_missing had to be enabled to ever hide the block on either case. (#891)

This PR implements the functionality as i would expect it:

  • active = !exists || !up
  • show full block if active
  • hide block if inactive && hide_inactive || !exists && hide_missing
  • otherwise, show only icon

I tested all cases and it works as described.

Does that sound correct to you, @ammgws @mvdan ?

be enabled to ever hide the block on either case
@@ -855,6 +857,8 @@ impl Block for Net {
let is_up = self.device.is_up()?;
if !exists || !is_up {
self.active = false;
self.exists = exists;
Copy link
Collaborator

@ammgws ammgws Oct 23, 2020

Choose a reason for hiding this comment

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

Maybe better rewritten as:

let self.exists = self.device.exists()?;
let self.active = self.device.is_up()?;
if !self.exists || !self.active {
...

?

Copy link
Contributor Author

@GladOSkar GladOSkar Oct 23, 2020

Choose a reason for hiding this comment

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

Alright, i did it a little differently than you suggested because it's even cleaner to clarify that active always implies exists

Copy link
Collaborator

@ammgws ammgws left a comment

Choose a reason for hiding this comment

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

LGTM, now hide_missing actually does what it was supposed to do.

Can you update the docs to be more clear? For example, clarifying that "missing" means the device does not exist on the system.

@GladOSkar
Copy link
Contributor Author

Alright i updated the block docs and rewrote the logic there a bit

@GladOSkar
Copy link
Contributor Author

No idea why the CI check is failing :/

@ammgws ammgws linked an issue Oct 23, 2020 that may be closed by this pull request
@ammgws
Copy link
Collaborator

ammgws commented Oct 23, 2020

Just need to run rustfmt, but otherwise it builds fine locally.

Thanks!

@ammgws ammgws merged commit ab2d3e4 into greshake:master Oct 23, 2020
@GladOSkar GladOSkar deleted the fix-net-hiding branch October 23, 2020 10:36
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.

net: support switching between ethernet and wireless
2 participants