Add desktop systray quit menu item only if it's not already present

[bzdtchk]

Description:

Add desktop systray quit menu item only if it's not already present

Checklist:

• Tests included.
• Lint and formatter run with no errors.
• Tests all pass.

[andydotxyz]

Thanks this seems like a good idea, however let's not duplicate the method to use it in system tray. The quit item is, after all, appended to a Menu in both cases.

[andydotxyz]

This looks like a duplicate of addMissingQuitForMainMenu but with the first parameter different.

Would it not make sense to call one from the other? The Menu is found in menus.Items[0] so it should be a simple call.

[bzdtchk]

Well their item.Action anonymous function is different and addMissingQuitForMainMenu returns the MainMenu object. addMissingQuitForMenu is void.

I'm not really sure it's just a simple call, seems like a big difference to me and I'm not really sure it's possible to make these two methods work together. I might extract the first two if statements to separate private methods but that's about it I guess.

Or am I missing something?

[andydotxyz]

I don't know how, but I haven't managed to explain. The following diff seemed to work for me:

index f46945175..9d7624973 100644
--- a/internal/driver/glfw/menu.go
+++ b/internal/driver/glfw/menu.go
@@ -15,30 +15,6 @@ func buildMenuOverlay(menus *fyne.MainMenu, w *window) fyne.CanvasObject {
 }
 
 func addMissingQuitForMainMenu(menus *fyne.MainMenu, w *window) *fyne.MainMenu {
-       var lastItem *fyne.MenuItem
-       if len(menus.Items[0].Items) > 0 {
-               lastItem = menus.Items[0].Items[len(menus.Items[0].Items)-1]
-               if lastItem.Label == "Quit" {
-                       lastItem.IsQuit = true
-               }
-       }
-       if lastItem == nil || !lastItem.IsQuit { // make sure the first menu always has a quit option
-               quitItem := fyne.NewMenuItem("Quit", nil)
-               quitItem.IsQuit = true
-               menus.Items[0].Items = append(menus.Items[0].Items, fyne.NewMenuItemSeparator(), quitItem)
-       }
-       for _, item := range menus.Items[0].Items {
-               if item.IsQuit && item.Action == nil {
-                       item.Action = func() {
-                               for _, win := range w.driver.AllWindows() {
-                                       if glWin, ok := win.(*window); ok {
-                                               glWin.closed(glWin.view())
-                                       } else {
-                                               win.Close() // for test windows
-                                       }
-                               }
-                       }
-               }
-       }
+       addMissingQuitForMenu(menus.Items[0], w.driver)
        return menus
 }

[andydotxyz]

However make sure that the full quit action is taken - the version you added is not as thorough as the one in the original main menu quit code...

[coveralls]

Coverage: 62.019% (-0.03%) from 62.044% when pulling bbb5ff0 on bzdtchk:feature/systray-quit-menu-item into ec0d80e on fyne-io:develop.

[andydotxyz]

I'm not sure why another review was requested - the duplicate code I tried to help with is still present.

[Jacalz]

Thanks for the contribution. I'm sorry that this got left for such a long time but I will have to close this per the rule that stale PRs can be closed after 6 months. If you want to continue working on this, you are more than welcome to rebase on latest develop and reopen (or open a new one if that's easier).