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 an indicator icon for each active connection #201

Draft
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

meisenzahl
Copy link
Member

Closes #200

@meisenzahl
Copy link
Member Author

meisenzahl commented Dec 6, 2020

Due to missing hardware I can't test all possible configurations.

@meisenzahl meisenzahl marked this pull request as ready for review December 6, 2020 11:58
@cassidyjames cassidyjames requested a review from a team December 6, 2020 20:25
Copy link
Collaborator

@jeremypw jeremypw left a comment

Choose a reason for hiding this comment

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

A couple of coding comments. I try testing this with some faked connections.

src/Widgets/DisplayWidget.vala Outdated Show resolved Hide resolved
src/Widgets/DisplayWidget.vala Show resolved Hide resolved
Copy link
Collaborator

@jeremypw jeremypw left a comment

Choose a reason for hiding this comment

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

You have not implemented a handler for "Network.State.DISCONNECTED_WIRED" so the wired indicator does not disappear if you unplug the cable and you still have a wireless network connection.

src/Utils.vala Outdated Show resolved Hide resolved
@jeremypw
Copy link
Collaborator

Another issue that required some thought - there is only one tooltip and only one Network.State at any moment so you get, for example a message about the wireless connection when hovering over the wire connection icon. It will require some refactoring to get a tooltip relevant to the icon being hovered.

@meisenzahl
Copy link
Member Author

@jeremypw thanks for your detailed review. Could you please take a look at my changes again? 😅️

@jeremypw
Copy link
Collaborator

Trying latest version ...

@jeremypw
Copy link
Collaborator

There is still a problem detecting disconnection/disabling wireless and wired networks (until restart the session).

If you start with a wireless connection and a wired connection (plugged in) then you get two icons as expected.
Unplugging the wired connection does not remove the wired icon
Switching off the wireless connection with the popover does not remove the wireless icon
Switching off the wired connection does remove the wired icon.

}

construct {
image = new Gtk.Image.from_icon_name (_icon_name, Gtk.IconSize.LARGE_TOOLBAR);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You probably need to add tooltips to the images themselves rather than the overall display widget.

@danirabbit
Copy link
Member

Gonna move this to "draft" since there's been no movement since April. Please feel free to reopen when this is ready for review again :)

@danirabbit danirabbit marked this pull request as draft June 15, 2021 21:43
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.

Show an indicator icon for each active connection
3 participants