From 058eb2f7f6281641521d2fd6ea68f41fc5d7c477 Mon Sep 17 00:00:00 2001 From: Yee Cheng Chin Date: Sun, 28 Feb 2021 23:52:12 -0800 Subject: [PATCH] Code clean up / robustification and additional logging for render Clean up code per clang analyzer. Make sure to initialize structs properly and clean up potential memory leaks. When drawing wide characters, clean up the other cell to be empty just to be clean. When updating fonts for a MacVim window, if it's not the main window, make sure to not update the shared font manager as it's previously a minor bug where it would do that. Add additional debug logging for renderer to help debug potential CoreText renderer issues. See #1164. --- src/MacVim/MMAppController.m | 4 +- src/MacVim/MMCoreTextView.m | 66 ++++++++++++++++++++++++++------- src/MacVim/MMVimController.m | 9 +++-- src/MacVim/MMWindowController.h | 1 - src/MacVim/MMWindowController.m | 10 ++++- 5 files changed, 70 insertions(+), 20 deletions(-) diff --git a/src/MacVim/MMAppController.m b/src/MacVim/MMAppController.m index 5c8253264c..8a70dec11f 100644 --- a/src/MacVim/MMAppController.m +++ b/src/MacVim/MMAppController.m @@ -1775,9 +1775,9 @@ fsEventCallback(ConstFSEventStreamRef streamRef, [alert setMessageText:NSLocalizedString(@"Multiple files not found", @"File not found dialog, title")]; text = [NSString stringWithFormat:NSLocalizedString( - @"Could not open file with name %@, and %d other files.", + @"Could not open file with name %@, and %u other files.", @"File not found dialog, text"), - firstMissingFile, count-[files count]-1]; + firstMissingFile, (unsigned int)(count-[files count]-1)]; } [alert setInformativeText:text]; diff --git a/src/MacVim/MMCoreTextView.m b/src/MacVim/MMCoreTextView.m index 7d2906c304..cca0790330 100644 --- a/src/MacVim/MMCoreTextView.m +++ b/src/MacVim/MMCoreTextView.m @@ -176,7 +176,7 @@ static GridCell* grid_cell(Grid *grid, int row, int col) { // with `:set nofu`. If that gets fixed, then delete this workaround. static GridCell* grid_cell_safe(Grid *grid, int row, int col) { if (row >= grid->rows || col >= grid->cols) { - static GridCell scratch_cell; + static GridCell scratch_cell = {}; return &scratch_cell; } return grid_cell(grid, row, col); @@ -377,6 +377,7 @@ static void grid_free(Grid *grid) { - (void)setFont:(NSFont *)newFont { if (!newFont) { + ASLogInfo(@"Trying to set null font"); return; } if (!forceRefreshFont) { @@ -414,7 +415,9 @@ static void grid_free(Grid *grid) { CTFontDescriptorRef desc = CTFontDescriptorCreateWithNameAndSize((CFStringRef)[newFont fontName], pt); CTFontRef fontRef = CTFontCreateWithFontDescriptor(desc, pt, NULL); CFRelease(desc); - + if (!fontRef) { + ASLogInfo(@"CTFontCreateWithFontDescriptor failed (preserveLineHeight == false, fontName: %@), pt: %f", [newFont fontName], pt); + } font = (NSFont*)fontRef; } else { font = [newFont retain]; @@ -439,7 +442,10 @@ static void grid_free(Grid *grid) { if (!newFont) { // Use the normal font as the wide font (note that the normal font may // very well include wide characters.) - if (font) [self setWideFont:font]; + if (font) { + [self setWideFont:font]; + return; + } } else if (newFont != fontWide) { [fontWide release]; fontWide = [newFont retain]; @@ -730,14 +736,22 @@ static void grid_free(Grid *grid) { NSGraphicsContext *context = [NSGraphicsContext currentContext]; CGContextRef ctx = context.CGContext; [context setShouldAntialias:antialias]; - CGContextSetFillColorSpace(ctx, CGColorSpaceCreateWithName(kCGColorSpaceSRGB)); + { + CGColorSpaceRef colorSpace = CGColorSpaceCreateWithName(kCGColorSpaceSRGB); + if (colorSpace) { + CGContextSetFillColorSpace(ctx, colorSpace); + CGColorSpaceRelease(colorSpace); + } else { + ASLogInfo(@"Could not create sRGB color space"); + } + } CGContextSetTextMatrix(ctx, CGAffineTransformIdentity); CGContextSetTextDrawingMode(ctx, kCGTextFill); CGContextSetFontSize(ctx, [font pointSize]); CGContextSetShouldSmoothFonts(ctx, YES); CGContextSetBlendMode(ctx, kCGBlendModeCopy); - int originalSmoothingStyle; + int originalSmoothingStyle = 0; if (thinStrokes) { originalSmoothingStyle = CGContextGetFontSmoothingStyle(ctx); CGContextSetFontSmoothingStyle(ctx, fontSmoothingStyleLight); @@ -757,8 +771,8 @@ static void grid_free(Grid *grid) { __block NSMutableString *lineString = nil; __block CGFloat lineStringStart = 0; - __block CFRange lineStringRange; - __block GridCell lastStringCell; + __block CFRange lineStringRange = {}; + __block GridCell lastStringCell = {}; void (^flushLineString)() = ^{ if (!lineString.length) return; @@ -774,8 +788,14 @@ static void grid_free(Grid *grid) { CGContextSetFillColor(ctx, COMPONENTS(lastStringCell.fg)); CGContextSetTextPosition(ctx, lineStringStart, rowRect.origin.y + fontDescent); CGContextSetBlendMode(ctx, kCGBlendModeNormal); + + const NSUInteger lineStringLength = lineString.length; CTLineRef line = [self lineForCharacterString:lineString textFlags:lastStringCell.textFlags]; - for (id obj in (NSArray*)CTLineGetGlyphRuns(line)) { + NSArray* glyphRuns = (NSArray*)CTLineGetGlyphRuns(line); + if ([glyphRuns count] == 0) { + ASLogDebug(@"CTLineGetGlyphRuns no glyphs for: %@", lineString); + } + for (id obj in glyphRuns) { CTRunRef run = (CTRunRef)obj; CFIndex glyphCount = CTRunGetGlyphCount(run); CFIndex indices[glyphCount]; @@ -783,11 +803,20 @@ static void grid_free(Grid *grid) { CGGlyph glyphs[glyphCount]; CTRunGetStringIndices(run, CFRangeMake(0, 0), indices); CTRunGetGlyphs(run, CFRangeMake(0, 0), glyphs); - for (CFIndex i = 0; i < glyphCount; i++) + for (CFIndex i = 0; i < glyphCount; i++) { + if (indices[i] >= lineStringLength) { + ASLogDebug(@"Invalid glyph pos index: %ld, len: %lu", (long)indices[i], (unsigned long)lineStringLength); + continue; + } positions[i] = positionsByIndex[indices[i]]; + } CTFontRef font = CFDictionaryGetValue(CTRunGetAttributes(run), kCTFontAttributeName); + if (!font) { + ASLogDebug(@"Null font for rendering. glyphCount: %ld", (long)glyphCount); + } CTFontDrawGlyphs(font, glyphs, positions, glyphCount, ctx); } + CGContextSetBlendMode(ctx, kCGBlendModeCopy); [lineString deleteCharactersInRange:NSMakeRange(0, lineString.length)]; }; @@ -867,8 +896,9 @@ static void grid_free(Grid *grid) { [lineString release]; CGContextRestoreGState(ctx); } - if (thinStrokes) + if (thinStrokes) { CGContextSetFontSmoothingStyle(ctx, originalSmoothingStyle); + } } - (void)performBatchDrawWithData:(NSData *)data @@ -1094,6 +1124,7 @@ static void grid_free(Grid *grid) { fontRef = [NSFontManager.sharedFontManager convertFont:fontRef toHaveTrait:NSFontItalicTrait]; if (textFlags & DRAW_BOLD) fontRef = [NSFontManager.sharedFontManager convertFont:fontRef toHaveTrait:NSFontBoldTrait]; + fontVariants[cacheFlags] = fontRef; } return fontRef; @@ -1104,8 +1135,9 @@ static void grid_free(Grid *grid) { int cacheFlags = flags & (DRAW_WIDE | DRAW_ITALIC | DRAW_BOLD); NSNumber *key = @(cacheFlags); NSCache *strCache = characterLines[key]; - if (!strCache) + if (!strCache){ strCache = characterLines[key] = [[[NSCache alloc] init] autorelease]; + } CTLineRef line = (CTLineRef)[strCache objectForKey:string]; if (!line) { NSAttributedString *attrString = [[NSAttributedString alloc] @@ -1476,12 +1508,13 @@ static int ReadDrawCmd(const void **bytesRef, struct DrawCmd *drawCmd) withFlags:(int)flags foregroundColor:(int)fg backgroundColor:(int)bg specialColor:(int)sp { - BOOL wide = flags & DRAW_WIDE ? YES : NO; + const BOOL wide = flags & DRAW_WIDE ? YES : NO; __block size_t cells_filled = 0; [string enumerateSubstringsInRange:NSMakeRange(0, string.length) options:NSStringEnumerationByComposedCharacterSequences usingBlock:^(NSString *substring, NSRange substringRange, NSRange enclosingRange, BOOL *stop) { - GridCell *cell = grid_cell_safe(&grid, row, col + cells_filled++ * (wide ? 2 : 1)); + const int curCol = col + cells_filled++ * (wide ? 2 : 1); + GridCell *cell = grid_cell_safe(&grid, row, curCol); GridCellInsertionPoint insertionPoint = {0}; if (flags & DRAW_TRANSP) insertionPoint = cell->insertionPoint; @@ -1502,6 +1535,13 @@ static int ReadDrawCmd(const void **bytesRef, struct DrawCmd *drawCmd) .insertionPoint = insertionPoint, .string = characterString, }; + + if (wide) { + // Also clear the next cell just to be tidy (even though this cell would be skipped during rendering) + const GridCell clearCell = { .bg = defaultBackgroundColor.argbInt }; + GridCell *nextCell = grid_cell_safe(&grid, row, curCol + 1); + *nextCell = clearCell; + } }]; [self setNeedsDisplayFromRow:row column:col diff --git a/src/MacVim/MMVimController.m b/src/MacVim/MMVimController.m index 6ae432acac..8a1f425b5e 100644 --- a/src/MacVim/MMVimController.m +++ b/src/MacVim/MMVimController.m @@ -88,8 +88,6 @@ static BOOL isUnsafeMessage(int msgid); @property (readonly) NSMutableDictionary *itemDict; @property (readonly) NSMutableArray *itemOrder; -- (id)initWithVimController:(MMVimController *)controller; - @end @interface MMTouchBarItemInfo : NSObject; @@ -828,6 +826,7 @@ static BOOL isUnsafeMessage(int msgid); // This should only happen if the system default font has changed // name since MacVim was compiled in which case we fall back on // using the user fixed width font. + ASLogInfo(@"Failed to load font '%@' / %f", name, size); font = [NSFont userFixedPitchFontOfSize:size]; } @@ -1634,7 +1633,7 @@ static BOOL isUnsafeMessage(int msgid); touchbarItem = item; } - MMTouchBarItemInfo *touchbarItemInfo = [[MMTouchBarItemInfo alloc] initWithItem:touchbarItem label:touchbarLabel]; + MMTouchBarItemInfo *touchbarItemInfo = [[[MMTouchBarItemInfo alloc] initWithItem:touchbarItem label:touchbarLabel] autorelease]; if (submenu) { [touchbarItemInfo makeChildTouchBar]; } @@ -2009,6 +2008,10 @@ static BOOL isUnsafeMessage(int msgid); - (id)init { + if (!(self = [super init])) { + return nil; + } + _touchbar = [[NSTouchBar alloc] init]; _itemDict = [[NSMutableDictionary alloc] init]; diff --git a/src/MacVim/MMWindowController.h b/src/MacVim/MMWindowController.h index 97376801e2..b5c10beb29 100644 --- a/src/MacVim/MMWindowController.h +++ b/src/MacVim/MMWindowController.h @@ -75,7 +75,6 @@ - (void)setScrollbarThumbValue:(float)val proportion:(float)prop identifier:(int32_t)ident; -- (unsigned int)calculateStyleMask; - (void)setBackgroundOption:(int)dark; - (void)refreshApperanceMode; diff --git a/src/MacVim/MMWindowController.m b/src/MacVim/MMWindowController.m index afc1da9730..4d8e25e434 100644 --- a/src/MacVim/MMWindowController.m +++ b/src/MacVim/MMWindowController.m @@ -692,7 +692,13 @@ - (void)setFont:(NSFont *)font { - [[NSFontManager sharedFontManager] setSelectedFont:font isMultiple:NO]; + const NSWindow* mainWindow = [NSApp mainWindow]; + if (mainWindow && (mainWindow == decoratedWindow || mainWindow == fullScreenWindow)) { + // Update the shared font manager with the new font, but only if this is the main window, + // as the font manager is shared among all the windows. + [[NSFontManager sharedFontManager] setSelectedFont:font isMultiple:NO]; + } + [[vimView textView] setFont:font]; [self updateResizeConstraints]; shouldMaximizeWindow = YES; @@ -1159,6 +1165,7 @@ [[MMAppController sharedInstance] setMainMenu:[vimController mainMenu]]; if ([vimView textView]) { + // Update the shared font manager to always be set to the font of the main window. NSFontManager *fm = [NSFontManager sharedFontManager]; [fm setSelectedFont:[[vimView textView] font] isMultiple:NO]; } @@ -1247,6 +1254,7 @@ - (void)windowDidChangeBackingProperties:(NSNotification *)notification { + ASLogDebug(@""); [vimController sendMessage:BackingPropertiesChangedMsgID data:nil]; }