fyne-io / fyne

Cross platform GUI toolkit in Go inspired by Material Design
https://fyne.io/
Other
25.02k stars 1.39k forks source link

`SetSystemTrayMenu` doesn't work when run in goroutine #5039

Closed williambrode closed 2 months ago

williambrode commented 3 months ago

Checklist

Describe the bug

I changed my code so it would set the System Tray in a goroutine after another condition was met. The app still showed the system tray icon but right-clicking it did nothing. Moving it back so SetSystemTrayMenu is always run before App.Run() appears to fix the problem. It may be fine that this is a requirement - but its not documented anywhere and it would be good to at least return or log an error if SetSystemTrayMenu doesn't actually work in this case.

How to reproduce

See example code. Run SetSystemTrayMenu in a goroutine while App.Run() is executing.

Screenshots

No response

Example code

fyneApp := app.NewWithID("TestApp")
win := fyneApp.NewWindow("Master")
win.SetMaster()
go func () {
  time.Sleep(1 * time.Second)
  trayMenu := fyne.NewMenu("TestMenu",
            fyne.NewMenuItem("Test", func() {
                win.Show()
                fmt.Println("test"
            })}
  if desk, ok := fyneApp.(desktop.App); ok {
    desk.SetSystemTrayMenu(trayMenu)
  }
}
fyneApp.Run()

Fyne version

2.4.5

Go compiler version

1.22

Operating system and version

Windows 10

Additional Information

No response

andydotxyz commented 3 months ago

App.Run will block until the app exists. You should set up the system tray code before calling the Run() method.

williambrode commented 3 months ago

Hi @andydotxyz! The core of this issue report was the following:

It may be fine that this is a requirement - but its not documented anywhere and it would be good to at least return or log an error if SetSystemTrayMenu doesn't actually work in this case.

I wasn't able to debug the issue I was having and had to binary search changes to find the reason our app tray stopped working. So I'm trying to suggest how to help future devs (some kind of error reporting).

Should I change the description to "No error when setting system tray fails"?

andydotxyz commented 3 months ago

This is not a system tray requirement - it is how the GUI app lifecycle works. Documented in getting started at https://docs.fyne.io/started/apprun.

If the documentation needs to be made clearer then we can do that, but I'm not sure I understand why this is a system tray issue. Just like if you create another window after calling Run that window won't visibly appear because its creation will execute as the app shuts down...

andydotxyz commented 3 months ago

I was having and had to binary search changes to find the reason our app tray stopped working.

Perhaps this was an important point that I overlooked... are you saying that somehow running the app and then setting up a system tray had worked in the past? I cannot understand how this is possible.

williambrode commented 3 months ago

Every instance of a developer doing something incorrect with fyne could be excused as "they should know better". But I suggest that the library itself would be improved if it helped a developer see what they did wrong (through some kind of indication of an error). I've been coding for quite some time and this missed error handling seemed particularly bad to me, so I thought I'd try to help by letting you guys know. Maybe its too low priority to improve in the near future, but at least it might aid another dev who notices their system tray doesn't work and searches the issues for help (as I did).

The use case here is that we have system tray options that only work after login. So I was trying to set the system tray after the user logged in - which had to be after App.run() was called.

andydotxyz commented 3 months ago

We are talking past each other here I think. I don't disagree that we should do everything we can to make this easy to understand. Which is why we spend so long on documentation and examples like the one I linked.

In this case I have no idea what we could possibly do with error handling because there has been no error. The code you are referring to has not run when you are expediting us to provide errors - everything after "Run" happens when the app is shutting down.

As GUI apps are event based or at least have multiple threads of execution the system tray info should be set when your user has logged in as you say. The time this happens is not due to the order of lines inside the main function...

I know I'm not managing to explain this well but I am not sure how else to illustrate the situation.

williambrode commented 3 months ago

Perhaps the confusion here is that you didn't see that the tray app is set in a goroutine, while Run() is still running? I'm not running it after App.Run() returns.

At the very least App.Run() could set a flag saying "I'm currently executing" and SetSystemTrayMenu could simply check that flag and return an error if its already executing - right?

williambrode commented 3 months ago

I updated the title - I see how my original description could be interpreted as a simple code ordering problem.

andydotxyz commented 3 months ago

Thanks for the clarification, I understand now.

I doubt very much that it has anything to do with the co-execution of "App.Run()" and may point to the system tray having to execute initial setup on the main thread. (i.e. not a goroutine)

If that is the case then we could fix internally to do the heavy lifting so you can call it from any thread.

andydotxyz commented 2 months ago

Fixed on develop and will merge to v2.5.1 release