Skip to content

Commit

Permalink
Fix duplicate menu items in Window menu
Browse files Browse the repository at this point in the history
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 macvim-dev#566, Fix macvim-dev#992
  • Loading branch information
ychin committed Sep 20, 2020
1 parent 8b25779 commit d412800
Show file tree
Hide file tree
Showing 3 changed files with 81 additions and 35 deletions.
6 changes: 6 additions & 0 deletions src/MacVim/MMAppController.h
Expand Up @@ -22,7 +22,11 @@
NSMutableArray *vimControllers;
NSString *openSelectionString;
NSMutableDictionary *pidArguments;

NSMenu *defaultMainMenu;
NSMenu *currentMainMenu;
BOOL mainMenuDirty;

NSMenuItem *appMenuItemTemplate;
NSMenuItem *recentFilesMenuItem;
NSMutableArray *cachedVimControllers;
Expand All @@ -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;

Expand Down
90 changes: 56 additions & 34 deletions src/MacVim/MMAppController.m
Expand Up @@ -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.
Expand All @@ -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;

Expand All @@ -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];
Expand Down Expand Up @@ -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
Expand All @@ -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];
Expand All @@ -889,45 +930,26 @@ - (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:
[appMenu indexOfItemWithAction:@selector(checkForUpdates:)]];
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];
}

Expand Down Expand Up @@ -1220,7 +1242,7 @@ - (IBAction)showVimHelp:(id)sender
@"-c", @":h gui_mac", @"-c", @":res", nil]
workingDirectory:nil];
}

- (IBAction)checkForUpdates:(id)sender
{
#if !DISABLE_SPARKLE
Expand Down Expand Up @@ -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"];
Expand All @@ -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];
Expand Down Expand Up @@ -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);
Expand Down
20 changes: 19 additions & 1 deletion src/MacVim/MMVimController.m
Expand Up @@ -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 {
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit d412800

Please sign in to comment.