-
-
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
Add tooltip indicator #199
Add tooltip indicator #199
Conversation
Since this is called on on_state_changed ()
Returns the active network name
Use ellipses instead of periods
hey @cassidyjames, just wondering how we would display the tooltip for a wired connection? Currently, it shows Also, I'm assuming that if the user is connected to both wired AND wifi, the tooltip for wired connection takes precedence. |
Scrapped get_active_ap in favour of getting the name in update_active_ap instead. Results in less computations as well.
For no connection name, we could just say For multiple connections, I think we have two options: name whatever the icon itself is indicating, or name both, i.e. Or I guess a third option: actually just show an additional icon for additional networks… which I kind of like better, but is outside the scope of this work; I have filed that as a separate issue (#200). Showing whatever the icon is indicating is probably the easiest/simplest approach. |
@cassidyjames How about VPN connections? Is there a specific way we can show this to the user? Perhaps something like I can't tell what the icon behaviour is like as I don't have a VPN. Is there a way I could spoof a VPN connection on elementary? Edit: Please ignore the fact that commits are not in sync with the messages in this PR. My network time was messed up. |
Marking this as ready to review since this mostly works except for VPN. Since I'm practically new to Vala, feel free to critique anything you see fit. Some things to note:
|
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.
UX-wise this seems to work for me. I'd like a review from @elementary/desktop-developers though.
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.
Hey thanks for working on this! I left a few comments where I think things can be simplified a bit more
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.
Oops, my bad on those suggestions. Updated with code I actually tested 😅
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.
Works as expected and code makes sense to me! Thanks for iterating so quickly on this. Nice work :)
Mine is working fine. Could you mention the steps to reproduce this? Also, I think it may be better to file this as a separate issue since this PR has already been merged :/ |
PR to close #197.
Todo: