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

Add NetworkManager module #518

Merged
merged 1 commit into from
Aug 4, 2024

Conversation

Zedfrigg
Copy link
Contributor

@Zedfrigg Zedfrigg commented Mar 30, 2024

Implements a module displaying (very) basic network connectivity from NetworkManager.

First step towards resolving #148.

Copy link
Owner

@JakeStanger JakeStanger left a comment

Choose a reason for hiding this comment

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

Thanks for your hard work on this so far. There's a few changes needed here, but generally the code looks solid & this should be a good foundation for the module.

If you can also resolve the formatting issue, and squash commits down into one please that'd be great.

Any questions let me know.

@Zedfrigg
Copy link
Contributor Author

@JakeStanger let me know if you think the code is ready to merge in its current state :)

Copy link
Owner

@JakeStanger JakeStanger left a comment

Choose a reason for hiding this comment

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

It's getting there generally, but there's still a few things pending & I'm still not 100% sure about the signals crate - if you can get back to my Qs pls just to clear up some potential concerns.

I need to give this a proper test too, so I'll try and do that as soon as I can.

Copy link
Owner

@JakeStanger JakeStanger left a comment

Choose a reason for hiding this comment

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

Thanks for the work on the refactor, and apologies I haven't inputted a lot on this one for a while.

A couple of very small changes now, but outside of those the code lgtm now. I'll give this some tests and report back.

Is there anything else you want to add at the PR at this stage, or do you reckon we're go for the initial version?

@JakeStanger
Copy link
Owner

I'll give this some tests and report back.

Looks like it's all working as intended. I've tested Ethernet and Wifi and toggling connections in both cases causes instant updates.

@Zedfrigg
Copy link
Contributor Author

Zedfrigg commented Jun 7, 2024

Is there anything else you want to add at the PR at this stage, or do you reckon we're go for the initial version?

No as far as I'm concerned we're good to go!

@JakeStanger
Copy link
Owner

Sorry for delay on this (again). There's a merge conflict outstanding - if you can resolve that I'll add the docs and merge :)

@Zedfrigg
Copy link
Contributor Author

Merged and re-tested 👍

The devShell CI workflow is failing, but I don't think it's related to this change?

@JakeStanger JakeStanger force-pushed the feat/networkmanager branch from 638cf12 to 6d0fe4c Compare August 4, 2024 11:55
Copy link
Owner

@JakeStanger JakeStanger left a comment

Choose a reason for hiding this comment

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

Apologies for the huge delay on this, and thanks again for all your work. I've finally got round to putting the finishing touches on it.

FYI, I've made some basic refactors and renamed the module/feature flag to network_manager.

I'll merge this in a few mins and it'll be available in the git build.

@JakeStanger JakeStanger merged commit 578293c into JakeStanger:master Aug 4, 2024
8 checks passed
@Zedfrigg Zedfrigg deleted the feat/networkmanager branch August 4, 2024 15:06
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