Skip to content

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.

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.

@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