cmd/trayscale: support showing "This device: node (IP)"#72
cmd/trayscale: support showing "This device: node (IP)"#72DeedleFake merged 1 commit intoDeedleFake:masterfrom
Conversation
cmd/trayscale/app.go
Outdated
| a.tray.SetOnlineStatus(online) | ||
| } | ||
| a.tray.Update(s) |
There was a problem hiding this comment.
This is a pain point for me. The original path only sets the online status if the connection status has changed.
I'm not sure if this should be the case for all possible status updates.
Either way, most likely this needs to be modified to just send the tsutil.Status to tray for it to handle updates.
There was a problem hiding this comment.
You could combine SetOnlineStatus() and Update() and pass tsutil.Status along with a.online to Update() to allow it to handle all system tray updates internally.
There was a problem hiding this comment.
Combined into Update. I'm not passing a.online along as it doesn't make sense to me if we have the whole Status.
Now we have to consider how to handle these updates not to waste CPU cycles. It will be updating with the rate of polling (5 seconds AFAICS). It's not ideal and the more items we use, the worse it will get.
I'm not yet sure what will be more expensive (provided we don't change polling rate or switch to the bus watcher #62) - holding last state in tray management and diffing on each update or sending updates to systray and hoping it figures refreshes out better than we do.
There was a problem hiding this comment.
The main reason for passing a.online through, though it would have to be done before the previous if, was so that the tray updates could internally check if the status has changed like the if right above the call to Update() does. That way it doesn't have to change the text every time. It's got one extra if than separating the calls, but it lets the update be entirely internal and thus reduces the coupling. In fact, instead of passing a.online directly, you could pass a.online != online and call the argument statusChanged or something.
There was a problem hiding this comment.
OK, I see your point. I added the previous state as an argument in a3363b6.
cmd/trayscale/app.go
Outdated
| a.tray.SetOnlineStatus(online) | ||
| } | ||
| a.tray.Update(s) |
There was a problem hiding this comment.
You could combine SetOnlineStatus() and Update() and pass tsutil.Status along with a.online to Update() to allow it to handle all system tray updates internally.
cmd/trayscale/systray.go
Outdated
| quitItem *systray.MenuItem | ||
| showItem *systray.MenuItem | ||
| quitItem *systray.MenuItem | ||
| nodeInfoItem *systray.MenuItem |
There was a problem hiding this comment.
Nitpick: I'd rename this to something like selfInfoItem or selfNodeItem or something so that it's more obvious which node it's talking about.
Also, while it's not necessary now, it might be nice to copy the local IP into the clipboard when the item is clicked. Ideally, at some point, I'd like to add all known nodes and their IPs to the menu and be able to copy any IP when clicked much like how the Windows client works.
There was a problem hiding this comment.
Renamed to selfNodeItem.
As for displaying other nodes - it makes sense and would be a nice feature. We can definitely move towards listing IPs like:
Main menu:
- Self IPv4
- Self IPV4
- Submenu node X:
- Node X IPv4
- Node X IPv6
- Submenu node Y:
- Node Y IPv4
- Node Y IPv6
Or
Main menu:
- Self IPv4
- Self IPV4
- Submenu other nodes:
- Submenu node X:
- Node X IPv4
- Node X IPv6
- Submenu node Y
- Node Y IPv4
- Node Y IPv6
- Submenu node X:
As for clipboard - seems nice, I have no knowledge on how to handle that yet and how it works with Wayland.
There was a problem hiding this comment.
Gtk takes care of the clipboard. Take a look at how the IP copying buttons work on the peer pages in cmd/trayscale/app.go.
There was a problem hiding this comment.
Yup, I've seen it after commenting.
But this is not the way I would see the tray go.
- Expecting a tray app (that coincidentally has a GTK4 frontend too) to use GTK to manage clipboard is bloody awful.
- Somehow hooking into respective buttons seems impossible too
- There might be a way to use
gdk.Clipboarddirectly. But it's a part of wider GTK too. I guess that's a way out if there's nothing better in the ecosystem.
| systray.SetTitle("Trayscale") | ||
|
|
||
| selfNodeItem := systray.AddMenuItem("", "") | ||
| systray.AddSeparator() |
There was a problem hiding this comment.
It's disappointing that there doesn't seem to be a way to hide the separator, but at least the self item should be shown most of the time.
There was a problem hiding this comment.
Yup.
Since I've been playing around with listing other nodes, I can tell, that systray, dbus or Plasma does not work as expected.
Starting with submenu, if I don't add anything to the submenu, adding things later (on first Update) will not show any items in the submenu. If I add a placeholder, adding new items once will work, but adding new ones or trying to hide/disable them (e.g. when offline) does not work.
Hell, I can't even tell if current Hide() on selfNodeItem works.
My current conclusion: if things will be like this, it might make more sense to reset the menu on each change and redraw everything. With this we might be wasting some resources, but we will be in full control of entire structure at all times.
|
@DeedleFake do you have any outstanding issues to be resolved in this PR? |
|
No, I think it can be merged once the conflicts are fixed. I'd still like to get clipboard integration, but that can be handled later. I might create an internal package to abstract things like that that can be handled by Gtk but aren't really tied to it. |
slagiewka
force-pushed
the
show_this_device_info
branch
from
a3363b6 to
861d6f4
Compare
|
Rebased :) |
2 participants
No description provided.