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 tooltip indicator #199

Merged
merged 24 commits into from
Dec 22, 2020
Merged

Add tooltip indicator #199

merged 24 commits into from
Dec 22, 2020

Conversation

pongloongyeat
Copy link
Member

@pongloongyeat pongloongyeat commented Nov 30, 2020

PR to close #197.

Screenshot from 2020-12-01 03-34-54

Todo:

  • Probe WiFi name
  • Probe VPN name
  • Fix Connecting to "" issue

src/Indicator.vala Outdated Show resolved Hide resolved
src/Indicator.vala Outdated Show resolved Hide resolved
@pongloongyeat
Copy link
Member Author

Works as expected for now
Screenshot from 2020-12-02 02-14-41

Note to self for tomorrow morning: Network name not present when connecting to a network (Connecting to "").
Screenshot from 2020-12-02 02-16-06

@pongloongyeat
Copy link
Member Author

pongloongyeat commented Dec 2, 2020

hey @cassidyjames, just wondering how we would display the tooltip for a wired connection? Currently, it shows Connected to "Wired" and if you have multiple wired connections i.e. ethernet and USB tethering then it would show which it is connected to.

usb0

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.
@cassidyjames
Copy link
Contributor

cassidyjames commented Dec 2, 2020

For no connection name, we could just say Connected to wireless network without the quotes, I think. Similarly with wired: Connected to wired network. I like the name showing for USB tethering, that's actually pretty useful!

For multiple connections, I think we have two options: name whatever the icon itself is indicating, or name both, i.e. Connected to wired network and “Wi-Fi Network Name”

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.

@pongloongyeat
Copy link
Member Author

pongloongyeat commented Dec 2, 2020

@cassidyjames How about VPN connections? Is there a specific way we can show this to the user? Perhaps something like Connected to wired network with "<VPN_NAME>"? I feel like this might overcrowd the tooltip with quotation marks when we're connecting to a specific wifi i.e. Connected to "thamlinchan2.4Ghz@unifi" with "NordVPN".

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.

@pongloongyeat
Copy link
Member Author

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:

  • I'm accessing wifi_box and other_box in the popover widget to know which is the currently active connection. This is done by making them public and I'm not sure if that has any consequences.
  • Currently, if both wired and wifi connections are active, the tooltip shows only the wired connection (this is in line with how the icon is shown).
  • If there is only one active wired connection, it shows Connected to wired connection. If there are multiple sources, it shows Connected to "<YOUR_ETHERNET_DEVICE>" i.e,

image

  • I don't think it should show the ethernet device name. It should be showing the name of the network it is connected to but this is what the popover widget shows and I've left it as is. It would look inconsistent if we show the network name in the tooltip and the ethernet device name in the popover widget. I'll file an issue later on this.

@pongloongyeat pongloongyeat marked this pull request as ready for review December 4, 2020 09:01
Copy link
Contributor

@cassidyjames cassidyjames left a 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.

@cassidyjames cassidyjames requested a review from a team December 15, 2020 20:03
src/Indicator.vala Outdated Show resolved Hide resolved
src/Indicator.vala Show resolved Hide resolved
src/Indicator.vala Outdated Show resolved Hide resolved
src/Indicator.vala Outdated Show resolved Hide resolved
src/Widgets/EtherInterface.vala Outdated Show resolved Hide resolved
src/Widgets/PopoverWidget.vala Outdated Show resolved Hide resolved
src/Widgets/WifiInterface.vala Outdated Show resolved Hide resolved
src/Indicator.vala Outdated Show resolved Hide resolved
@cassidyjames cassidyjames requested a review from a team December 17, 2020 22:22
Copy link
Member

@danirabbit danirabbit left a 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

src/Indicator.vala Outdated Show resolved Hide resolved
src/Indicator.vala Outdated Show resolved Hide resolved
src/Indicator.vala Outdated Show resolved Hide resolved
src/Widgets/EtherInterface.vala Outdated Show resolved Hide resolved
src/Widgets/WifiInterface.vala Show resolved Hide resolved
Copy link
Member

@danirabbit danirabbit left a 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 😅

src/Indicator.vala Outdated Show resolved Hide resolved
src/Indicator.vala Outdated Show resolved Hide resolved
Copy link
Member

@danirabbit danirabbit left a 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 :)

@danirabbit danirabbit merged commit 0688f59 into elementary:master Dec 22, 2020
@andirsun
Copy link
Contributor

andirsun commented Jan 2, 2021

Tooltip indicator show connected to (null)
Screenshot from 2021-01-02 10-50-44

@pongloongyeat
Copy link
Member Author

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 :/

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.

Add tooltip to indicator icon
5 participants