-
-
Notifications
You must be signed in to change notification settings - Fork 14
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
base: master
Are you sure you want to change the base?
Show an indicator icon for each active connection #201
Conversation
Due to missing hardware I can't test all possible configurations. |
There was a problem hiding this 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.
There was a problem hiding this 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.
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. |
@jeremypw thanks for your detailed review. Could you please take a look at my changes again? 😅️ |
Trying latest version ... |
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. |
} | ||
|
||
construct { | ||
image = new Gtk.Image.from_icon_name (_icon_name, Gtk.IconSize.LARGE_TOOLBAR); |
There was a problem hiding this comment.
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.
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 :) |
Closes #200