Skip to content

Fix gradient crash on non-RGB NSColor and add regression test #69

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
probonopd wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix gradient crash on non-RGB NSColor and add regression test #69

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 38 additions & 33 deletions Source/cairo/CairoGState.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1584,10 +1584,26 @@ - (void) drawGradient: (NSGradient*)gradient
[gradient getColor: &color
location: &location
atIndex: i];
red = [color redComponent];
green = [color greenComponent];
blue = [color blueComponent];
alpha = [color alphaComponent];
/* Ensure color is in an RGB-compatible colorspace to avoid
-redComponent/-greenComponent/-blueComponent raising
for non-RGB colors (patterns, color lists, etc.). Try
device RGB first, then calibrated RGB, and finally
fall back to a safe opaque black if conversion fails. */
NSColor *rgbColor = [color colorUsingColorSpaceName: NSDeviceRGBColorSpace];
if (!rgbColor) rgbColor = [color colorUsingColorSpaceName: NSCalibratedRGBColorSpace];
if (!rgbColor) rgbColor = [NSColor colorWithCalibratedWhite:0.0 alpha:1.0];

@try {
red = [rgbColor redComponent];
green = [rgbColor greenComponent];
blue = [rgbColor blueComponent];
alpha = [rgbColor alphaComponent];
} @catch (NSException *ex) {
/* Last-resort safe defaults */
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Silent error handling may hide issues: When color conversion fails and the catch block assigns default black color values (red = green = blue = 0.0, alpha = 1.0), no warning or log message is generated. This makes debugging difficult if unexpected color conversions occur. Consider adding an NSLog statement in the catch block to help diagnose color conversion failures, similar to other error logging patterns in this file (e.g., line 160, 518, 567).

Suggested change
/* Last-resort safe defaults */
/* Last-resort safe defaults */
NSLog(@"[CairoGState] Failed to convert color %@ to RGB: %@. Using default black.",
color, ex);

Copilot uses AI. Check for mistakes.
red = green = blue = 0.0;
alpha = 1.0;
}
Comment on lines +1587 to +1605
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code duplication: The color normalization logic (lines 1587-1605) is duplicated in the second gradient method (lines 1653-1666). This code should be extracted into a helper method to avoid duplication and ensure both gradient drawing methods handle color conversion consistently. This would improve maintainability and reduce the risk of divergence if the logic needs to be updated in the future.

Copilot uses AI. Check for mistakes.

cairo_pattern_add_color_stop_rgba(cpattern, location,
red, green, blue, alpha);
}
Expand Down Expand Up @@ -1629,39 +1645,28 @@ - (void) drawGradient: (NSGradient*)gradient
double green;
double blue;
double alpha;
NSString *colorSpaceName;


[gradient getColor: &color
location: &location
atIndex: i];

colorSpaceName = [color colorSpaceName];
if([NSCalibratedRGBColorSpace isEqualToString: colorSpaceName] ||
[NSDeviceRGBColorSpace isEqualToString: colorSpaceName])
{
red = [color redComponent];
green = [color greenComponent];
blue = [color blueComponent];
alpha = [color alphaComponent];
cairo_pattern_add_color_stop_rgba(cpattern, location,
red, green, blue, alpha);
}
else if([NSCalibratedWhiteColorSpace isEqualToString: colorSpaceName] ||
[NSDeviceWhiteColorSpace isEqualToString: colorSpaceName] ||
[NSCalibratedBlackColorSpace isEqualToString: colorSpaceName] ||
[NSDeviceBlackColorSpace isEqualToString: colorSpaceName])
{
red = [color whiteComponent];
green = [color whiteComponent];
blue = [color whiteComponent];
alpha = [color alphaComponent];
cairo_pattern_add_color_stop_rgba(cpattern, location,
red, green, blue, alpha);
}
else
{
NSLog(@"Cannot draw gradient for %@",colorSpaceName);
}
/* Normalize to RGB-compatible color before extracting components */
NSColor *rgbColor = [color colorUsingColorSpaceName: NSDeviceRGBColorSpace];
if (!rgbColor) rgbColor = [color colorUsingColorSpaceName: NSCalibratedRGBColorSpace];
if (!rgbColor) rgbColor = [NSColor colorWithCalibratedWhite:0.0 alpha:1.0];

@try {
red = [rgbColor redComponent];
green = [rgbColor greenComponent];
blue = [rgbColor blueComponent];
alpha = [rgbColor alphaComponent];
} @catch (NSException *ex) {
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Silent error handling may hide issues: When color conversion fails and the catch block assigns default black color values, no warning or log message is generated. This makes debugging difficult if unexpected color conversions occur. Consider adding an NSLog statement in the catch block to help diagnose color conversion failures, similar to other error logging patterns in this file (e.g., line 160, 518, 567).

Suggested change
} @catch (NSException *ex) {
} @catch (NSException *ex) {
NSLog(@"CairoGState: Failed to convert color %@ to RGB components for gradient stop at location %f: %@ - %@",
rgbColor,
location,
[ex name],
[ex reason]);

Copilot uses AI. Check for mistakes.
red = green = blue = 0.0;
alpha = 1.0;
}

cairo_pattern_add_color_stop_rgba(cpattern, location,
red, green, blue, alpha);
}

cairo_save(_ct);
Expand Down
9 changes: 9 additions & 0 deletions Tests/gradient/GNUmakefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
include $(GNUSTEP_MAKEFILES)/common.make
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing copyright header: The GNUmakefile lacks a copyright and license header. Based on the convention seen in Tools/GNUmakefile, makefiles in this repository typically include a copyright header with LGPL license information. This should be added for consistency with the rest of the codebase.

Copilot uses AI. Check for mistakes.

TOOL_NAME = testgradient

testgradient_OBJC_FILES = gradient_test.m

testgradient_TOOL_LIBS = -lgnustep-gui -lgnustep-base

include $(GNUSTEP_MAKEFILES)/tool.make
31 changes: 31 additions & 0 deletions Tests/gradient/gradient_test.m
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
#import <Foundation/Foundation.h>
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing copyright header: The test file lacks a copyright and license header. Based on the convention seen throughout this repository (e.g., Tools/font_cacher.m, Source files), source files typically include a copyright header with LGPL license information. This should be added for consistency with the rest of the codebase.

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discrepancy with PR description: The PR description mentions "Modified: gershwin-components/Assistants/AssistantFramework/GSAssistantWindow.m (readded guard)" as one of the changed files. However, this file is not included in the pull request. Either the file should be included or the PR description should be updated to accurately reflect the files being changed.

Copilot uses AI. Check for mistakes.
#import <AppKit/AppKit.h>

int main (int argc, const char * argv[]) {
NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];

// Create a small pattern image and make a pattern color (non-RGB)
NSImage *pat = [[NSImage alloc] initWithSize:NSMakeSize(4,4)];
[pat lockFocus];
[[NSColor colorWithCalibratedRed:1.0 green:0.0 blue:0.0 alpha:1.0] setFill];
NSRectFill(NSMakeRect(0,0,4,4));
[pat unlockFocus];

NSColor *pattern = [NSColor colorWithPatternImage:pat];

NSGradient *g = [[NSGradient alloc] initWithStartingColor:pattern endingColor:[NSColor colorWithCalibratedWhite:1.0 alpha:1.0]];

NSImage *img = [[NSImage alloc] initWithSize:NSMakeSize(100,100)];
Comment on lines +8 to +18
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Memory leak: The test creates several objects (pat, pattern, g, img) using alloc/init but never releases them. While the autorelease pool will drain at the end, these objects should be explicitly released before draining the pool to follow proper memory management practices. This is especially important for a test that's meant to verify correct behavior without side effects.

Copilot uses AI. Check for mistakes.
@try {
[img lockFocus];
[g drawInRect:NSMakeRect(0,0,100,100) angle:90.0];
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incomplete test coverage: The test only exercises the drawInRect:angle: method (which calls the fromPoint:toPoint: variant internally). It should also test the radial gradient method (fromCenter:radius:toCenter:radius:) to ensure both code paths in CairoGState.m are covered, since both were modified to handle non-RGB colors.

Copilot uses AI. Check for mistakes.
[img unlockFocus];
Comment on lines +18 to +22
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test creates an NSImage and calls lockFocus/unlockFocus on it, but this may fail or not work correctly in a headless/non-GUI environment where no display connection is available. The test should either check if the application can be initialized properly, or handle the case where lockFocus might fail. Consider adding a check or using NSGraphicsContext directly to ensure the test can run in various environments.

Copilot uses AI. Check for mistakes.
NSLog(@"OK: gradient drawn without exception");
} @catch (NSException *ex) {
NSLog(@"FAIL: exception: %@", ex);
return 1;
}

[pool drain];
return 0;
}
Loading