Provide a smoother menu refresh experience #2853
Conversation
This adds support for refreshing the system tray menu in the same way
| // Refresh will instruct this menu to update its display. | ||
| // | ||
| // Since: 2.2 | ||
| func (m *Menu) Refresh() { | ||
| for _, w := range CurrentApp().Driver().AllWindows() { | ||
| main := w.MainMenu() | ||
| if main != nil { | ||
| for _, menu := range main.Items { | ||
| if menu == m { | ||
| w.SetMainMenu(main) | ||
| break | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if d, ok := CurrentApp().Driver().(systemTrayDriver); ok { | ||
| if m == d.SystemTrayMenu() { | ||
| d.SetSystemTrayMenu(m) | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I’d expect the Set* methods to do nothing if the item to set is already set, i.e. I find the refresh effect unexpected. There should be a RefreshMenu method instead.
Thinking about it, IMO, it would be better if the menu would not know about the potential places where it might be used.
Instead, the sys-tray, the main menu widget and the menu widget should register a refresh callback if their fyne.(Main)Menu is being set. They should also deregister this if the menu is changed, of course.
Thus, the changed (main) menu would simply call all registered callbacks.
There was a problem hiding this comment.
I agree that would be a cleaner implementation - but I cannot figure out how to do that without additional functionality in our top level APIs.
Because fyne.Menu is a raw struct I am not sure how to support the chance callback hooks without adding complexity to the API - and also exposing the possibility that users could disconnect the callback...
If we do find a way to do it more neatly I would be happy to do so, but also I would hope it could sit behind the same clean public API that mimics how CanvasObject works.
Any thoughts on a way to proceed?
There was a problem hiding this comment.
Okay, I thought a little bit about it and didn’t came up with an idea better than an internal registry to decouple the callback handling from the menu. But this is clearly to much complexity. The current approach is not really a blocker. It can be easily refactored if we have a better idea in the future.
However, the usage of Set* for triggering a refresh should be improved like I said in the first paragraph :).
There was a problem hiding this comment.
Thanks. I'm not totally clear about the Set comment - what would your suggested RefreshMenu do different to Menu.Refresh()? I'll try and catch you on Slack so we can disccus
A work in progress, looking for discussion as it develops.
I think the following is required:
Fixes #2852
Checklist: