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

internal/tray: support displaying connection status #68

Merged
merged 1 commit into from
Jul 9, 2024

Conversation

slagiewka
Copy link
Contributor

@slagiewka slagiewka commented Jun 24, 2023

This adds a simple tray menu section that changes according to connection status and resembles the behaviour of the original client tray (macOS).

I found the current tray menu pretty lacklustre, at least compared to the official client (however, this is old design). In order to familiarise myself with the project, I decided to go with this simple info first.

Let me know if this is the direction you'd like to see to project go towards.

@DeedleFake
Copy link
Owner

DeedleFake commented Jun 26, 2023

Looks good to me, especially with that TODO which I completely agree with. I've taken a stab a few times at building a system to manage the tray menu but haven't gotten too far yet. The systray package has some strange API choices, so I've also started a refactor of that package that I'm hoping to get upstreamed at some point.

@DeedleFake
Copy link
Owner

DeedleFake commented Jun 26, 2023

My one concern is whether or not this is actually necessary. The tray icon itself already changes to indicate connection status, so having an indicator in the menu as well doesn't really seem necessary to me. Are there environments in which the menu is accessible but the icon isn't necessarily visible? I'm mostly using this in GNOME (with an extension) and KDE, so the icon is always visible at a glance unless something's fullscreen or something.

@slagiewka
Copy link
Contributor Author

My one concern is whether or not this is actually necessary. The tray icon itself already changes to indicate connection status, so having an indicator in the menu as well doesn't really seem necessary to me.

No, this doesn't seem to be needed all that much. And current design of the official clients has moved to just having the quick toggle in this place:
Screenshot 2023-06-24 at 15 04 43

And that makes sense, since there's already the status icon (Tailscale and Trayscale) and status toggle in GUI (Trayscale). I agree that it's extraneous. For me it was a good start though to start the development around things in the tray. It's the most common data the app has and updates regularly.

Are there environments in which the menu is accessible but the icon isn't necessarily visible? I'm mostly using this in GNOME (with an extension) and KDE, so the icon is always visible at a glance unless something's fullscreen or something.

I'm also using both of the above. And I also can't think of a scenario where the tray icon is not visible but the tray menu is still (somehow) accessible. Even GNOME without the tray extension only shows the actual window (if present).

To sum up, I've already added new functionality in my local dev:

  • a quick connection toggle that resides with the connection status (see below, not a checkbox yet)
  • an item with User Name ([email protected])

Which proves that we definitely have most of the data we would need to provide more insight at a glance or even quick toggles without going to the actual UI. Kind of against GNOME philosophy, but in line with Tailscale itself.

connection_toggle

@@ -222,6 +225,7 @@ func (a *App) update(s tsutil.Status) {
a.online = online
a.notify(online) // TODO: Notify on startup if not connected?
systray.SetIcon(statusIcon(online))
a.connectionStatusMenuItem.SetTitle(connectionStatusText(online))
Copy link
Owner

Choose a reason for hiding this comment

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

If the user has the tray icon disabled, a.connectionStatusMenuItem could be nil at this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow, didn't cross my mind to disable tray icon for... TRAYscale 😄 I will work on ensuring we do no-ops for unitialised items sooner then 😉

Copy link
Owner

Choose a reason for hiding this comment

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

Wow, didn't cross my mind to disable tray icon for... TRAYscale

Ha ha, yeah. Trayscale actually didn't have a tray icon at all for a while because most implementations used Gtk3 behind the scenes, which didn't work when used with a Gtk4 app, and the Fyne fork, which was created to get rid of the Gtk3 requirement, had a number of bugs at the time that made it infeasible, including one that made changing the icon to display the status not work.

@Jacalz
Copy link
Contributor

Jacalz commented Jul 5, 2024

What's the status on this? It would be a very useful addition to the menu :)

@slagiewka slagiewka changed the title cmd/trayscale: support displaying connection status in menu internal/tray: support displaying connection status Jul 5, 2024
@slagiewka slagiewka requested a review from DeedleFake July 5, 2024 20:49
@slagiewka
Copy link
Contributor Author

Updated to reflect current tray setup.

@DeedleFake
Copy link
Owner

It's been a little while. I'll take another look at this on Sunday and see.

@DeedleFake
Copy link
Owner

I took another look at this and am again unsure of the purpose considering the icon change to indicate the status. What I'm currently thinking is that I'll merge this and then change it to be a toggle that can be clicked to start/stop Tailscale without having to open the window for it, i.e. an item that is not disabled and is labeled "Disconnect" or "Connect" where appropriate. Thoughts?

@Jacalz
Copy link
Contributor

Jacalz commented Jul 9, 2024

Sounds good to me :)

@DeedleFake DeedleFake merged commit 5b60334 into DeedleFake:master Jul 9, 2024
0 of 3 checks passed
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.

3 participants