MMTabline: Use cached images for buttons, fix memory leaks and misc issues

ychin · ychin · commit 558c2e936a4e · 2025-01-28T23:49:10.000-08:00
MMHoverButton was a bit inefficient in its image management. It always
made a new template image which then had to be converted into two other
images (image and alternateImage) for every button. Cache at least the
template images with weak references so we don't have to keep generating
new ones. For now, allow setImage: to create new images but they could
be changed to also just use cached images as well.

Also, fix memory leaks in the tabs codebase due to improper closure
usage in blocks. They were subtly capturing the self pointer which led
to the tab line and hover buttons never getting destroyed. Fix to make
sure we never accidentally capture self and try to capture as little as
possible.

Another leak happens in the usage of the local event monitor that we use
to intercept scroll wheel events. The API contract mandates that we
remove the monitor which the code never does. Make sure we do that, and
fix up the logic of the event interceptor to be more resilient and works
better with third-party software (which could inject horizontal scroll
events without holding down the Shift key).
diff --git a/src/MacVim/MMTabline/MMHoverButton.h b/src/MacVim/MMTabline/MMHoverButton.h
@@ -6,6 +6,14 @@
 
 @property (nonatomic, retain) NSColor *fgColor;
 
-+ (NSImage *)imageNamed:(NSString *)name;
+typedef enum : NSUInteger {
+    MMHoverButtonImageAddTab = 0,
+    MMHoverButtonImageCloseTab,
+    MMHoverButtonImageScrollLeft,
+    MMHoverButtonImageScrollRight,
+    MMHoverButtonImageCount
+} MMHoverButtonImage;
+
++ (NSImage *)imageFromType:(MMHoverButtonImage)imageType;
 
 @end
diff --git a/src/MacVim/MMTabline/MMHoverButton.m b/src/MacVim/MMTabline/MMHoverButton.m
@@ -6,41 +6,64 @@ @implementation MMHoverButton
     NSBox *_circle;
 }
 
-+ (NSImage *)imageNamed:(NSString *)name
++ (NSImage *)imageFromType:(MMHoverButtonImage)imageType
 {
-    CGFloat size = [name isEqualToString:@"CloseTabButton"] ? 15 : 17;
-    return [NSImage imageWithSize:NSMakeSize(size, size) flipped:NO drawingHandler:^BOOL(NSRect dstRect) {
-        NSBezierPath *p = [NSBezierPath new];
-        if ([name isEqualToString:@"AddTabButton"]) {
+    if (imageType >= MMHoverButtonImageCount)
+        return nil;
+
+    CGFloat size = imageType == MMHoverButtonImageCloseTab ? 15 : 17;
+
+    static __weak NSImage *imageCache[MMHoverButtonImageCount] = { nil };
+    if (imageCache[imageType] != nil)
+        return imageCache[imageType];
+
+    BOOL (^drawFuncs[MMHoverButtonImageCount])(NSRect) = {
+        // AddTab
+        ^BOOL(NSRect dstRect) {
+            NSBezierPath *p = [NSBezierPath new];
             [p moveToPoint:NSMakePoint( 8.5,  4.5)];
             [p lineToPoint:NSMakePoint( 8.5, 12.5)];
             [p moveToPoint:NSMakePoint( 4.5,  8.5)];
             [p lineToPoint:NSMakePoint(12.5,  8.5)];
             [p setLineWidth:1.2];
             [p stroke];
-        }
-        else if ([name isEqualToString:@"CloseTabButton"]) {
+            return YES;
+        },
+        // CloseTab
+        ^BOOL(NSRect dstRect) {
+            NSBezierPath *p = [NSBezierPath new];
             [p moveToPoint:NSMakePoint( 4.5,  4.5)];
             [p lineToPoint:NSMakePoint(10.5, 10.5)];
             [p moveToPoint:NSMakePoint( 4.5, 10.5)];
             [p lineToPoint:NSMakePoint(10.5,  4.5)];
             [p setLineWidth:1.2];
             [p stroke];
-        }
-        else if ([name isEqualToString:@"ScrollLeftButton"]) {
+            return YES;
+        },
+        // ScrollLeft
+        ^BOOL(NSRect dstRect) {
+            NSBezierPath *p = [NSBezierPath new];
             [p moveToPoint:NSMakePoint( 5.0,  8.5)];
             [p lineToPoint:NSMakePoint(10.0,  4.5)];
             [p lineToPoint:NSMakePoint(10.0, 12.5)];
             [p fill];
-        }
-        else if ([name isEqualToString:@"ScrollRightButton"]) {
+            return YES;
+        },
+        // ScrollRight
+        ^BOOL(NSRect dstRect) {
+            NSBezierPath *p = [NSBezierPath new];
             [p moveToPoint:NSMakePoint(12.0,  8.5)];
             [p lineToPoint:NSMakePoint( 7.0,  4.5)];
             [p lineToPoint:NSMakePoint( 7.0, 12.5)];
             [p fill];
+            return YES;
         }
-        return YES;
-    }];
+    };
+    NSImage *img = [NSImage imageWithSize:NSMakeSize(size, size)
+                                  flipped:NO
+                           drawingHandler:drawFuncs[imageType]];
+    imageCache[imageType] = img;
+    return img;
 }
 
 - (instancetype)initWithFrame:(NSRect)frameRect
@@ -70,22 +93,28 @@ - (void)setFgColor:(NSColor *)color
     self.image = super.image;
 }
 
-- (void)setImage:(NSImage *)image
+- (void)setImage:(NSImage *)imageTemplate
 {
-    _circle.cornerRadius = image.size.width / 2.0;
+    _circle.cornerRadius = imageTemplate.size.width / 2.0;
     NSColor *fillColor = self.fgColor ?: NSColor.controlTextColor;
-    super.image = [NSImage imageWithSize:image.size flipped:NO drawingHandler:^BOOL(NSRect dstRect) {
-        [image drawInRect:dstRect];
+    NSImage *image = [NSImage imageWithSize:imageTemplate.size
+                                    flipped:NO
+                             drawingHandler:^BOOL(NSRect dstRect) {
+        [imageTemplate drawInRect:dstRect];
         [fillColor set];
         NSRectFillUsingOperation(dstRect, NSCompositingOperationSourceAtop);
         return YES;
     }];
-    self.alternateImage = [NSImage imageWithSize:image.size flipped:NO drawingHandler:^BOOL(NSRect dstRect) {
+    NSImage *alternateImage = [NSImage imageWithSize:imageTemplate.size
+                                             flipped:NO
+                                      drawingHandler:^BOOL(NSRect dstRect) {
         [[fillColor colorWithAlphaComponent:0.2] set];
         [[NSBezierPath bezierPathWithOvalInRect:dstRect] fill];
-        [super.image drawInRect:dstRect];
+        [image drawInRect:dstRect];
         return YES;
     }];
+    super.image = image;
+    self.alternateImage = alternateImage;
 }
 
 - (void)setEnabled:(BOOL)enabled
diff --git a/src/MacVim/MMTabline/MMTab.m b/src/MacVim/MMTabline/MMTab.m
@@ -39,7 +39,7 @@ - (instancetype)initWithFrame:(NSRect)frameRect tabline:(MMTabline *)tabline
         _tabline = tabline;
         
         _closeButton = [MMHoverButton new];
-        _closeButton.image = [MMHoverButton imageNamed:@"CloseTabButton"];
+        _closeButton.image = [MMHoverButton imageFromType:MMHoverButtonImageCloseTab];
         _closeButton.target = self;
         _closeButton.action = @selector(closeTab:);
         _closeButton.translatesAutoresizingMaskIntoConstraints = NO;
diff --git a/src/MacVim/MMTabline/MMTabline.m b/src/MacVim/MMTabline/MMTabline.m
@@ -13,9 +13,9 @@
 const CGFloat TabOverlap      = 6;
 const CGFloat ScrollOneTabAllowance = 0.25; // If we are showing 75+% of the tab, consider it to be fully shown when deciding whether to scroll to next tab.
 
-static MMHoverButton* MakeHoverButton(MMTabline *tabline, NSString *imageName, NSString *tooltip, SEL action, BOOL continuous) {
+static MMHoverButton* MakeHoverButton(MMTabline *tabline, MMHoverButtonImage imageType, NSString *tooltip, SEL action, BOOL continuous) {
     MMHoverButton *button = [MMHoverButton new];
-    button.image = [MMHoverButton imageNamed:imageName];
+    button.image = [MMHoverButton imageFromType:imageType];
     button.translatesAutoresizingMaskIntoConstraints = NO;
     button.target = tabline;
     button.action = action;
@@ -81,9 +81,9 @@ - (instancetype)initWithFrame:(NSRect)frameRect
         _scrollView.documentView = _tabsContainer;
         [self addSubview:_scrollView];
 
-        _addTabButton = MakeHoverButton(self, @"AddTabButton", @"New Tab (⌘T)", @selector(addTabAtEnd), NO);
-        _leftScrollButton = MakeHoverButton(self, @"ScrollLeftButton", @"Scroll Tabs", @selector(scrollLeftOneTab), YES);
-        _rightScrollButton = MakeHoverButton(self, @"ScrollRightButton", @"Scroll Tabs", @selector(scrollRightOneTab), YES);
+        _addTabButton = MakeHoverButton(self, MMHoverButtonImageAddTab, @"New Tab (⌘T)", @selector(addTabAtEnd), NO);
+        _leftScrollButton = MakeHoverButton(self, MMHoverButtonImageScrollLeft, @"Scroll Tabs", @selector(scrollLeftOneTab), YES);
+        _rightScrollButton = MakeHoverButton(self, MMHoverButtonImageScrollRight, @"Scroll Tabs", @selector(scrollRightOneTab), YES);
 
         [self addConstraints:[NSLayoutConstraint constraintsWithVisualFormat:@"H:[_leftScrollButton][_rightScrollButton]-5-[_scrollView]-5-[_addTabButton]" options:NSLayoutFormatAlignAllCenterY metrics:nil views:NSDictionaryOfVariableBindings(_scrollView, _leftScrollButton, _rightScrollButton, _addTabButton)]];
         [self addConstraints:[NSLayoutConstraint constraintsWithVisualFormat:@"V:|[_scrollView]|" options:0 metrics:nil views:@{@"_scrollView":_scrollView}]];
@@ -96,29 +96,8 @@ - (instancetype)initWithFrame:(NSRect)frameRect
         
         [[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(didScroll:) name:NSViewBoundsDidChangeNotification object:_scrollView.contentView];
 
-        // Monitor for scroll wheel events so we can scroll the tabline
-        // horizontally without the user having to hold down SHIFT.
-        _scrollWheelEventMonitor = [NSEvent addLocalMonitorForEventsMatchingMask:NSEventMaskScrollWheel handler:^NSEvent * _Nullable(NSEvent * _Nonnull event) {
-            NSPoint location = [_scrollView convertPoint:event.locationInWindow fromView:nil];
-            // We want events:
-            //   where the mouse is over the _scrollView
-            //   and where the user is not modifying it with the SHIFT key
-            //   and initiated by the scroll wheel and not the trackpad
-            if ([_scrollView mouse:location inRect:_scrollView.bounds]
-                && !event.modifierFlags
-                && !event.hasPreciseScrollingDeltas)
-            {
-                // Create a new scroll wheel event based on the original,
-                // but set the new deltaX to the original's deltaY.
-                // stackoverflow.com/a/38991946/111418
-                CGEventRef cgEvent = CGEventCreateCopy(event.CGEvent);
-                CGEventSetIntegerValueField(cgEvent, kCGScrollWheelEventDeltaAxis2, event.scrollingDeltaY);
-                NSEvent *newEvent = [NSEvent eventWithCGEvent:cgEvent];
-                CFRelease(cgEvent);
-                return newEvent;
-            }
-            return event;
-        }];
+        [self addScrollWheelMonitor];
+
     }
     return self;
 }
@@ -141,6 +120,32 @@ - (void)viewDidChangeEffectiveAppearance
     for (MMTab *tab in _tabs) tab.state = tab.state;
 }
 
+- (void)viewDidHide
+{
+    if (_scrollWheelEventMonitor != nil) {
+        [NSEvent removeMonitor:_scrollWheelEventMonitor];
+        _scrollWheelEventMonitor = nil;
+    }
+    [super viewDidHide];
+}
+
+- (void)viewDidUnhide
+{
+    [self addScrollWheelMonitor];
+    [super viewDidUnhide];
+}
+
+- (void)dealloc
+{
+    if (_scrollWheelEventMonitor != nil) {
+        [NSEvent removeMonitor:_scrollWheelEventMonitor];
+        _scrollWheelEventMonitor = nil;
+    }
+
+    // This is not necessary after macOS 10.11, but there's no harm in doing so
+    [[NSNotificationCenter defaultCenter] removeObserver:self];
+}
+
 #pragma mark - Accessors
 
 - (NSInteger)numberOfTabs
@@ -546,6 +551,50 @@ - (TabWidth)tabWidthForTabs:(NSInteger)numTabs
     return (TabWidth){tabWidth, availableWidthForTabs - tabWidth * numTabs};
 }
 
+/// Install a scroll wheel event monitor so that we can convert vertical scroll
+/// wheel events to horizontal ones, so that the user doesn't have to hold down
+/// SHIFT key while scrolling.
+///
+/// Caller *has* to call `removeMonitor:` on `_scrollWheelEventMonitor`
+/// afterwards.
+- (void)addScrollWheelMonitor
+{
+    // We have to use a local event monitor because we are not allowed to
+    // override NSScrollView's scrollWheel: method. If we do so we will lose
+    // macOS responsive scrolling. See:
+    // https://developer.apple.com/library/archive/releasenotes/AppKit/RN-AppKitOlderNotes/index.html#10_9Scrolling
+    if (_scrollWheelEventMonitor != nil)
+        return;
+    __weak NSScrollView *scrollView_weak = _scrollView;
+    __weak __typeof__(self) self_weak = self;
+    _scrollWheelEventMonitor = [NSEvent addLocalMonitorForEventsMatchingMask:NSEventMaskScrollWheel handler:^NSEvent * _Nullable(NSEvent * _Nonnull event) {
+        // We want an event:
+        //   - that actually belongs to this window
+        //   - initiated by the scroll wheel and not the trackpad
+        //   - is a vertical scroll event (if this is a horizontal scroll event
+        //     either via holding SHIFT or third-party software we just let it
+        //     through)
+        //   - where the mouse is over the scroll view
+        if (event.window == self_weak.window
+            && !event.hasPreciseScrollingDeltas
+            && (event.scrollingDeltaX == 0 && event.scrollingDeltaY != 0)
+            && [scrollView_weak mouse:[scrollView_weak convertPoint:event.locationInWindow fromView:nil]
+                               inRect:scrollView_weak.bounds])
+        {
+            // Create a new scroll wheel event based on the original,
+            // but set the new deltaX to the original's deltaY.
+            // stackoverflow.com/a/38991946/111418
+            CGEventRef cgEvent = CGEventCreateCopy(event.CGEvent);
+            CGEventSetIntegerValueField(cgEvent, kCGScrollWheelEventDeltaAxis1, 0);
+            CGEventSetIntegerValueField(cgEvent, kCGScrollWheelEventDeltaAxis2, event.scrollingDeltaY);
+            NSEvent *newEvent = [NSEvent eventWithCGEvent:cgEvent];
+            CFRelease(cgEvent);
+            return newEvent;
+        }
+        return event;
+    }];
+}
+
 - (void)fixupCloseButtons
 {
     if (_tabs.count == 1) {
