Skip to content
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

Support PlatformColor #1561

Merged
merged 2 commits into from
Feb 23, 2022
+33 −16
Merged

Support PlatformColor #1561

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2312b50
Support PlatformColor
oblador Apr 1, 2021
2a8a0d0
Merge branch 'develop' into platform-colors
oblador Oct 22, 2021
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
8 changes: 7 additions & 1 deletion android/src/main/java/com/horcrux/svg/RenderableView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import com.facebook.react.bridge.ReactContext;
import com.facebook.react.bridge.ReadableArray;
import com.facebook.react.bridge.ReadableType;
import com.facebook.react.bridge.ColorPropConverter;
import com.facebook.react.uimanager.PointerEvents;
import com.facebook.react.uimanager.annotations.ReactProp;

Expand Down Expand Up @@ -474,7 +475,12 @@ private void setupPaint(Paint paint, float opacity, ReadableArray colors) {
switch (colorType) {
case 0:
if (colors.size() == 2) {
int color = colors.getInt(1);
int color;
if (colors.getType(1) == ReadableType.Map) {
color = ColorPropConverter.getColor(colors.getMap(1), getContext());
} else {
color = colors.getInt(1);
}
int alpha = color >>> 24;
int combined = Math.round((float)alpha * opacity);
paint.setColor(combined << 24 | (color & 0x00ffffff));
Expand Down
19 changes: 10 additions & 9 deletions apple/Brushes/RNSVGSolidColorBrush.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,48 +13,49 @@

@implementation RNSVGSolidColorBrush
{
CGColorRef _color;
UIColor *_color;
Copy link
Member

Choose a reason for hiding this comment

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

I believe we should use RNSVGColor so it compiles on macOS.

}

- (instancetype)initWithArray:(NSArray<RNSVGLength *> *)array
{
if ((self = [super initWithArray:array])) {
_color = CGColorRetain([RCTConvert RNSVGCGColor:array offset:1]);
_color = [RCTConvert RNSVGUIColor:array offset:1];
}
return self;
}

- (instancetype)initWithNumber:(NSNumber *)number
{
if ((self = [super init])) {
_color = CGColorRetain([RCTConvert CGColor:number]);
_color = [RCTConvert UIColor:number];
}
return self;
}

- (void)dealloc
{
CGColorRelease(_color);
_color = nil;
}

- (CGColorRef)getColorWithOpacity:(CGFloat)opacity
{
return CGColorCreateCopyWithAlpha(_color, opacity * CGColorGetAlpha(_color));
CGColorRef baseColor = _color.CGColor;
CGColorRef color = CGColorCreateCopyWithAlpha(baseColor, opacity * CGColorGetAlpha(baseColor));
CGColorRelease(baseColor);
Copy link
Member

Choose a reason for hiding this comment

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

We should not release it since it will release CGColor prop of _color, so trying to use _color.CGColor again will lead to crash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you sure about that? Why would releasing _baseColor affect _color?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, now I see, good catch 👍

return color;
}

- (BOOL)applyFillColor:(CGContextRef)context opacity:(CGFloat)opacity
{
CGColorRef color = CGColorCreateCopyWithAlpha(_color, opacity * CGColorGetAlpha(_color));
CGColorRef color = [self getColorWithOpacity:opacity];
CGContextSetFillColorWithColor(context, color);
CGColorRelease(color);
return YES;
}

- (BOOL)applyStrokeColor:(CGContextRef)context opacity:(CGFloat)opacity
{
CGColorRef color = CGColorCreateCopyWithAlpha(_color, opacity * CGColorGetAlpha(_color));
CGColorRef color = [self getColorWithOpacity:opacity];
CGContextSetStrokeColorWithColor(context, color);
CGColorRelease(color);
return YES;
}

Expand Down
2 changes: 1 addition & 1 deletion apple/Utils/RCTConvert+RNSVG.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
+ (RNSVGBrush *)RNSVGBrush:(id)json;
+ (RNSVGPathParser *)RNSVGCGPath:(NSString *)d;
+ (CGRect)RNSVGCGRect:(id)json offset:(NSUInteger)offset;
+ (CGColorRef)RNSVGCGColor:(id)json offset:(NSUInteger)offset;
+ (UIColor *)RNSVGUIColor:(id)json offset:(NSUInteger)offset;
+ (CGGradientRef)RNSVGCGGradient:(id)json;

@end
6 changes: 3 additions & 3 deletions apple/Utils/RCTConvert+RNSVG.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,17 +130,17 @@ + (CGRect)RNSVGCGRect:(id)json offset:(NSUInteger)offset
};
}

+ (CGColorRef)RNSVGCGColor:(id)json offset:(NSUInteger)offset
+ (UIColor *)RNSVGUIColor:(id)json offset:(NSUInteger)offset
{
NSArray *arr = [self NSArray:json];
if (arr.count == offset + 1) {
return [self CGColor:[arr objectAtIndex:offset]];
return [self UIColor:[arr objectAtIndex:offset]];
}
if (arr.count < offset + 4) {
RCTLogError(@"Too few elements in array (expected at least %zd): %@", (ssize_t)(4 + offset), arr);
return nil;
}
return [self CGColor:[arr subarrayWithRange:(NSRange){offset, 4}]];
return [self UIColor:[arr subarrayWithRange:(NSRange){offset, 4}]];
}

+ (CGGradientRef)RNSVGCGGradient:(id)json
Expand Down
4 changes: 2 additions & 2 deletions src/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as React from 'react';
import * as ReactNative from 'react-native';
import { GestureResponderEvent } from 'react-native';
import { GestureResponderEvent, OpaqueColorValue } from 'react-native';
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this just be "ColorValue"?


// Common props
export type NumberProp = string | number;
Expand Down Expand Up @@ -101,7 +101,7 @@ export type rgbaArray = ReadonlyArray<number>;
// int32ARGBColor = 0xaarrggbb
export type int32ARGBColor = number;

export type Color = int32ARGBColor | rgbaArray | string;
export type Color = int32ARGBColor | rgbaArray | OpaqueColorValue | string;

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we just replace the "Color" type altogether with "ColorValue" from react native? It should represent the same set of types of colors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not the same, from the type it's string | OpaqueColorValue so we could replace the string with this one, but not the whole type. I prefer to use OpaqueColorValue here as I'm not sure what types will be added to ColorValue in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was under the impression that ColorValue is just the standard Color type in React Native 0.63+, and that OpaqueColorValue will include all the extra types of colors that can be passed to native. In react-native-macos (which we recently added support for in react-native-svg), I extended ColorValue/OpaqueColorValue to include a new API ColorWithSystemEffectMacOS (microsoft/react-native-macos#751).

But also, I thought the idea of OpaqueColorValue is that we do not need to know the specifics of the type, as long as it gets passed over the bridge to RCTConvert UIColor:] (or whatever the equivalent on Android is), React Native will make sure it's a valid native color. Is there a reason we need to know the specifics of the types of colors now? I was under the impression we could abstract most of that away to React Native and would need less processing on this repo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can find the definition here:

export type ColorValue = string | OpaqueColorValue;

So it's not the same, and it can't be replaced as the way this library deals with colors is quite custom. I'm not the author of this library, but i guess it's partly due to color inheritance. If it was not dealing with colors in a custom way then this PR wouldn't be necessary.

export interface FillProps {
fill?: Color;
Expand Down
10 changes: 10 additions & 0 deletions src/lib/extract/extractBrush.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,16 @@ export default function extractBrush(color?: Color) {
return int32ARGBColor;
}

// iOS PlatformColor
if ('semantic' in color) {
Copy link
Member

Choose a reason for hiding this comment

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

I think here we should also add a check for if color is an object, as suggested here by @gaodeng : https://github.com/oblador/react-native-svg/pull/1/files#diff-955398674a5278517eeeb9b5daca466f8e891b408d79757c3afbfa58a552a324R78

return [0, color];
}

// Android PlatformColor
if ('resource_paths' in color) {
return [0, color];
}
Comment on lines +43 to +51
Copy link
Contributor

Choose a reason for hiding this comment

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

At least on iOS, "semantic" isn't enough, because a color might be a DynamicColor instead. Could we instead do something like a type check? if (typeof color === ColorValue) ....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DynamicColorIOS isn't supported in this PR, and I don't really see that API surviving long term as it's iOS only.

Copy link
Contributor

Choose a reason for hiding this comment

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

Surviving in react-native, or surviving in this repo? I don't think it's going anywhere as long as PlatformColor exists. We also have a react-native-macos equivalent DynacmicColorMacOS that won't go anywhere as long as DynamicColor exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I meant in react-native. DynamicColorIOS predates PlaformColor, and while it's not able to fully replace it just yet, I don't see that API as useful without cross platform support. I guess is possible to add however.


console.warn(`"${color}" is not a valid color or brush`);
return null;
}