search

fyne-io / systray

a cross platfrom Go library to place an icon and menu in the notification area
Apache License 2.0
223 stars 41 forks source link

Handle missing or restarted systray on linux more gracefully #52

Closed bdwalton closed 1 year ago

bdwalton commented 1 year ago

In the current design, any app using this systray library needs to be started after the process that exposes org.kde.StatusNotifierWatcher. If the app owning that name is absent of restarts after initial setup, the systray display is broken for the duration of the process lifetime. It would be nice to detect this condition and gracefully reconnect. WDYT?

andydotxyz commented 1 year ago

That would be excellent yes!

bdwalton commented 1 year ago

Ok. I'll sketch out a change here and share.

bdwalton commented 1 year ago

This is what I came up with as a rough cut. Thoughts before I do a pull request: https://github.com/fyne-io/systray/compare/master...bdwalton:systray:master

andydotxyz commented 1 year ago

Looks like a good job. I'm not sure why you have the signal and a timer, but it's a good change. I wouldn't use the quitChan to exit, as it is used elsewhere so you might steal the event from the main quit loop.

bdwalton commented 1 year ago

The signal is a dbus signal and listens for ownership changes of the org.kde.StatusNotifierWatcher name. Those would happen if the systray process is restarted. The timer could likely go, but is meant to handle the case where the systray isn't available at initial startup. Ideally the signal handling would cover that but there could be a race condition between the systray client and the systray server. Also if there are other errors regsitering not due to the systray being absent during the attempt (transient communication error or something), the timer could repair things.

WRT the close channel. Nobody every writes to that channel. It's simply closed to flag shutdown, so that should see the read here succeed in a way that won't harm other users of it. https://go.dev/play/p/PYsSFzKtEdA as an example of this.

andydotxyz commented 1 year ago

Thanks for this, I see you are right about the quit channel. But if the timer is there just to heal some unknown problems that could occur maybe leave it out if possible and then if an issue is raised it could be considered again? Simplest fixes are always the easiest to land :).

bdwalton commented 1 year ago

Sent a pull request with the timer stuff removed. PTAL.

bdwalton commented 1 year ago

Ping?

bdwalton commented 1 year ago

LMK if the pull request is no good or doesn't meet the coding standards. Happy to rework or rethink it.

Jacalz commented 1 year ago

We will review when we have time. There is no need to stress things. Most of us are volunteering to contribute to this project in our free time

andydotxyz commented 1 year ago

If you check out our contributor guidelines you can see we ask folk to leave a week before asking for any feedback - also do it on the PR not on the image, that way the right item appears in the notification feed.

From my perspective I just needed to get to a Linux computer to test, which I have now managed to do. Thanks so much for your contribution.

bdwalton commented 1 year ago

Ack, thanks. I'll go read those guidelines as I'd missed them. Thanks for being patient with me. :)