From d412800d3cc931b59152d491ab62b58972c873d2 Mon Sep 17 00:00:00 2001 From: Yee Cheng Chin Date: Fri, 18 Sep 2020 02:05:59 -0700 Subject: [PATCH] Fix duplicate menu items in Window menu Previously MacVim would see a lot of duplicate window menu items like "Enter Full Screen" or "Tile Window to Left of Screen" when the user toggles between two windows. This is because the `setWindowsMenu:` call was injecting these items, but AppKit isn't smart enough to de-duplicate them (unlike the window list at the bottom). Just fix this by making a copy of the main menu before passing it in. This way every time we try to set a main menu (which happens whenever we jump among Vim windows as each Vim can have different menu items), it will be set with a fresh one that doesn't have the injected menu items in it. - This also requires adding a refresh functionality because adding/removing items to the original menu no longer get automatically reflected to the app since it only knows about the copied version. Also, set NSFullScreenMenuItemEverywhere to prevent AppKit from injecting "Enter Full Screen" items. MacVim already has similar menu items to handle that. Also, remove old private API call to `setAppleMenu:`. As far as I could tell this is not useful anymore in recent macOS versions and that line of code was written in 2008. Fix #566, Fix #992 --- src/MacVim/MMAppController.h | 6 +++ src/MacVim/MMAppController.m | 90 ++++++++++++++++++++++-------------- src/MacVim/MMVimController.m | 20 +++++++- 3 files changed, 81 insertions(+), 35 deletions(-) diff --git a/src/MacVim/MMAppController.h b/src/MacVim/MMAppController.h index 9ebd066030..a4a1b94b40 100644 --- a/src/MacVim/MMAppController.h +++ b/src/MacVim/MMAppController.h @@ -22,7 +22,11 @@ NSMutableArray *vimControllers; NSString *openSelectionString; NSMutableDictionary *pidArguments; + NSMenu *defaultMainMenu; + NSMenu *currentMainMenu; + BOOL mainMenuDirty; + NSMenuItem *appMenuItemTemplate; NSMenuItem *recentFilesMenuItem; NSMutableArray *cachedVimControllers; @@ -44,6 +48,8 @@ - (void)removeVimController:(id)controller; - (void)windowControllerWillOpen:(MMWindowController *)windowController; - (void)setMainMenu:(NSMenu *)mainMenu; +- (void)markMainMenuDirty:(NSMenu *)mainMenu; +- (void)refreshMainMenu; - (NSArray *)filterOpenFiles:(NSArray *)filenames; - (BOOL)openFiles:(NSArray *)filenames withArguments:(NSDictionary *)args; diff --git a/src/MacVim/MMAppController.m b/src/MacVim/MMAppController.m index 64aec2133b..28248bb52d 100644 --- a/src/MacVim/MMAppController.m +++ b/src/MacVim/MMAppController.m @@ -299,7 +299,7 @@ - (id)init ASLogCrit(@"Failed to register connection with name '%@'", name); [connection release]; connection = nil; } - + #if !DISABLE_SPARKLE // Sparkle is enabled (this is the default). Initialize it. It will // automatically check for update. @@ -321,6 +321,7 @@ - (void)dealloc [openSelectionString release]; openSelectionString = nil; [recentFilesMenuItem release]; recentFilesMenuItem = nil; [defaultMainMenu release]; defaultMainMenu = nil; + currentMainMenu = nil; [appMenuItemTemplate release]; appMenuItemTemplate = nil; [updater release]; updater = nil; @@ -329,6 +330,11 @@ - (void)dealloc - (void)applicationWillFinishLaunching:(NSNotification *)notification { + // This prevents macOS from injecting "Enter Full Screen" menu item. + // MacVim already has a separate menu item to do that. + // See https://developer.apple.com/library/archive/releasenotes/AppKit/RN-AppKitOlderNotes/index.html#10_11FullScreen + [[NSUserDefaults standardUserDefaults] setBool:NO forKey:@"NSFullScreenMenuItemEverywhere"]; + // Remember the default menu so that it can be restored if the user closes // all editor windows. defaultMainMenu = [[NSApp mainMenu] retain]; @@ -862,7 +868,42 @@ - (void)windowControllerWillOpen:(MMWindowController *)windowController - (void)setMainMenu:(NSMenu *)mainMenu { - if ([NSApp mainMenu] == mainMenu) return; + if (currentMainMenu == mainMenu) { + return; + } + currentMainMenu = mainMenu; + [self refreshMainMenu]; +} + +// Refresh the currently active main menu. This call is necessary when any +// modification was made to the menu, because refreshMainMenu makes a copy of +// the main menu, meaning that modifications to the original menu wouldn't be +// reflected until refreshMainMenu is invoked. +- (void)markMainMenuDirty:(NSMenu *)mainMenu +{ + if (currentMainMenu != mainMenu) { + // The menu being updated is not the currently set menu, so just ignore, + // as this is likely a background Vim window. + return; + } + if (!mainMenuDirty) { + // Mark the main menu as dirty and queue up a refresh. We don't immediately + // execute the refresh so that multiple calls would get batched up in one go. + mainMenuDirty = YES; + [self performSelectorOnMainThread:@selector(refreshMainMenu) withObject:nil waitUntilDone:NO]; + } +} + +- (void)refreshMainMenu +{ + mainMenuDirty = NO; + + // Make a copy of the menu before we pass to AppKit. The main reason is + // that setWindowsMenu: below will inject items like "Tile Window to Left + // of Screen" to the Window menu, and on repeated calls it will keep adding + // the same item over and over again, without resolving for duplicates. Using + // copies help keep the source menu clean. + NSMenu *mainMenu = [currentMainMenu copy]; // If the new menu has a "Recent Files" dummy item, then swap the real item // for the dummy. We are forced to do this since Cocoa initializes the @@ -871,7 +912,7 @@ - (void)setMainMenu:(NSMenu *)mainMenu NSMenu *fileMenu = [mainMenu findFileMenu]; if (recentFilesMenuItem && fileMenu) { int dummyIdx = - [fileMenu indexOfItemWithAction:@selector(recentFilesDummy:)]; + [fileMenu indexOfItemWithAction:@selector(recentFilesDummy:)]; if (dummyIdx >= 0) { NSMenuItem *dummyItem = [[fileMenu itemAtIndex:dummyIdx] retain]; [fileMenu removeItemAtIndex:dummyIdx]; @@ -889,19 +930,9 @@ - (void)setMainMenu:(NSMenu *)mainMenu } } - // Now set the new menu. Notice that we keep one menu for each editor - // window since each editor can have its own set of menus. When swapping - // menus we have to tell Cocoa where the new "MacVim", "Windows", and - // "Services" menu are. - [NSApp setMainMenu:mainMenu]; - - // Setting the "MacVim" (or "Application") menu ensures that it is typeset - // in boldface. (The setAppleMenu: method used to be public but is now - // private so this will have to be considered a bit of a hack!) - NSMenu *appMenu = [mainMenu findApplicationMenu]; - [NSApp performSelector:@selector(setAppleMenu:) withObject:appMenu]; - #if DISABLE_SPARKLE + NSMenu *appMenu = [mainMenu findApplicationMenu]; + // If Sparkle is disabled, we want to remove the "Check for Updates" menu // item since it's no longer useful. NSMenuItem *checkForUpdatesItem = [appMenu itemAtIndex: @@ -909,25 +940,16 @@ - (void)setMainMenu:(NSMenu *)mainMenu checkForUpdatesItem.hidden = true; #endif + // Now set the new menu. Notice that we keep one menu for each editor + // window since each editor can have its own set of menus. When swapping + // menus we have to tell Cocoa where the new "MacVim", "Windows", and + // "Services" menu are. + [NSApp setMainMenu:mainMenu]; + NSMenu *servicesMenu = [mainMenu findServicesMenu]; [NSApp setServicesMenu:servicesMenu]; NSMenu *windowsMenu = [mainMenu findWindowsMenu]; - if (windowsMenu) { - // Cocoa isn't clever enough to get rid of items it has added to the - // "Windows" menu so we have to do it ourselves otherwise there will be - // multiple menu items for each window in the "Windows" menu. - // This code assumes that the only items Cocoa add are ones which - // send off the action makeKeyAndOrderFront:. (Cocoa will not add - // another separator item if the last item on the "Windows" menu - // already is a separator, so we needen't worry about separators.) - int i, count = [windowsMenu numberOfItems]; - for (i = count-1; i >= 0; --i) { - NSMenuItem *item = [windowsMenu itemAtIndex:i]; - if ([item action] == @selector(makeKeyAndOrderFront:)) - [windowsMenu removeItem:item]; - } - } [NSApp setWindowsMenu:windowsMenu]; } @@ -1220,7 +1242,7 @@ - (IBAction)showVimHelp:(id)sender @"-c", @":h gui_mac", @"-c", @":res", nil] workingDirectory:nil]; } - + - (IBAction)checkForUpdates:(id)sender { #if !DISABLE_SPARKLE @@ -1816,7 +1838,7 @@ - (void)handleGetURLEvent:(NSAppleEventDescriptor *)event // Only opens files that already exist. if ([[NSFileManager defaultManager] fileExistsAtPath:filePath]) { NSArray *filenames = [NSArray arrayWithObject:filePath]; - + // Look for the line and column options. NSDictionary *args = nil; NSString *line = [dict objectForKey:@"line"]; @@ -1831,7 +1853,7 @@ - (void)handleGetURLEvent:(NSAppleEventDescriptor *)event args = [NSDictionary dictionaryWithObject:line forKey:@"cursorLine"]; } - + [self openFiles:filenames withArguments:args]; } else { NSAlert *alert = [[NSAlert alloc] init]; @@ -2198,7 +2220,7 @@ - (void)startWatchingVimDir NSString *path = [@"~/.vim" stringByExpandingTildeInPath]; NSArray *pathsToWatch = [NSArray arrayWithObject:path]; - + fsEventStream = FSEventStreamCreate(NULL, &fsEventCallback, NULL, (CFArrayRef)pathsToWatch, kFSEventStreamEventIdSinceNow, MMEventStreamLatency, kFSEventStreamCreateFlagNone); diff --git a/src/MacVim/MMVimController.m b/src/MacVim/MMVimController.m index 4aa629943b..7309a428f7 100644 --- a/src/MacVim/MMVimController.m +++ b/src/MacVim/MMVimController.m @@ -1251,7 +1251,8 @@ - (void)addMenuWithDescriptor:(NSArray *)desc atIndex:(int)idx [item setSubmenu:menu]; NSMenu *parent = [self parentMenuForDescriptor:desc]; - if (!parent && [MMVimController hasPopupPrefix:rootName]) { + const BOOL isPopup = [MMVimController hasPopupPrefix:rootName]; + if (!parent && isPopup) { if ([popupMenuItems count] <= idx) { [popupMenuItems addObject:item]; } else { @@ -1271,6 +1272,8 @@ - (void)addMenuWithDescriptor:(NSArray *)desc atIndex:(int)idx [item release]; [menu release]; + if (!isPopup) + [[MMAppController sharedInstance] markMainMenuDirty:mainMenu]; } - (void)addMenuItemWithDescriptor:(NSArray *)desc @@ -1350,6 +1353,9 @@ - (void)addMenuItemWithDescriptor:(NSArray *)desc } else { [parent insertItem:item atIndex:idx]; } + const BOOL isPopup = [MMVimController hasPopupPrefix:rootName]; + if (!isPopup) + [[MMAppController sharedInstance] markMainMenuDirty:mainMenu]; } - (void)removeMenuItemWithDescriptor:(NSArray *)desc @@ -1405,6 +1411,10 @@ - (void)removeMenuItemWithDescriptor:(NSArray *)desc [[item menu] removeItem:item]; [item release]; + + const BOOL isPopup = [MMVimController hasPopupPrefix:rootName]; + if (!isPopup) + [[MMAppController sharedInstance] markMainMenuDirty:mainMenu]; } - (void)enableMenuItemWithDescriptor:(NSArray *)desc state:(BOOL)on @@ -1439,6 +1449,10 @@ - (void)enableMenuItemWithDescriptor:(NSArray *)desc state:(BOOL)on // but at the same time Vim can set if a menu is enabled whenever it // wants to. [[self menuItemForDescriptor:desc] setTag:on]; + + const BOOL isPopup = [MMVimController hasPopupPrefix:rootName]; + if (!isPopup) + [[MMAppController sharedInstance] markMainMenuDirty:mainMenu]; } - (void)updateMenuItemTooltipWithDescriptor:(NSArray *)desc @@ -1471,6 +1485,10 @@ - (void)updateMenuItemTooltipWithDescriptor:(NSArray *)desc } [[self menuItemForDescriptor:desc] setToolTip:tip]; + + const BOOL isPopup = [MMVimController hasPopupPrefix:rootName]; + if (!isPopup) + [[MMAppController sharedInstance] markMainMenuDirty:mainMenu]; } - (void)addToolbarItemToDictionaryWithLabel:(NSString *)title