search

fyne-io / systray

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

Catch too early calls on windows and return an error instead of panicking #10

Closed dhaavi closed 2 years ago

dhaavi commented 2 years ago

This PR improves developer experience on Windows.

Currently, when you call a function of the library too early, it just panics.

With this PR, it does a not-perfect - but good enough - check to prevent a panic and return an error instead. This makes it a lot easier for developers to understand what they are doing wrong and help them use this library effectively, instead of opening issues about a panic. ;)

dhaavi commented 2 years ago

Well, the panics happen because trying to do pretty much anything with the library before calling Run* will attempt to use the uninitialized winTray and thus panic on the first nil pointer.

I think the nil check is so cheap that an additional mechanism won't bring any meaningful improvement.

Right now this PR uses an existing field of the winTray for the check. A cleaner and better solution would be to use a separate flag for this. Brb.

dhaavi commented 2 years ago

Ok. This is now improved with using a separate flag, which works much better. BUT: I have also now seen that his only protects the tray item itself. onReady may be called before the menu is initialized, but while I would expect the possibility of panics here, we don't see any in practice. So this seems to be ok for now. It's still a race condition, and this PR is about helping devs fix the obvious wrong things, as normally I would expect this lib to be called sequentially up until calling Run*.

andydotxyz commented 2 years ago

That sounds sensible. Though as a follow-on I ask - have we called onReady too soon, is that the underlying issue and should it be resolved instead?

dhaavi commented 2 years ago

Well, onReady waits for a callback from windows after initialization (without the menu). The callback is winTray.wndProc. We could however make onReady wait for both the callback and init function being done (with the menu).