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.

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

added support for svg stroke width change and viewBox dimension #181

Closed
wants to merge 3 commits into from
Closed

Conversation

NeedNap
Copy link

@NeedNap NeedNap commented Sep 30, 2020

I modified your library in order to implement the following functionalities:

  • added the possibility to get the svg viewBox dimensions reading them from the relative tag
  • added the strokeWidth property to allow the svg stroke change programmatically

I would like to find these new features in the next release of the library.
Thank you for your great job.

@arielelkin
Copy link
Collaborator

Hi @NeedNap, thank you for your PR!

Could you please make sure that this passes our CI? (it currently indicates checks have passed but that's not the case if you look at the CI logs ).

In the modified SVGBezierPath.h methods, could you please make the viewBox parameter optional?

@arielelkin
Copy link
Collaborator

More generally, PRs adding support for SVG attributes shouldn't introduce breaking changes (i.e. people upgrading from the current version to this new version should be able to build without issues – currently they need to modify all their calls to CGPathsFromSVGString, pathsFromSVGString, etc.)

@NeedNap
Copy link
Author

NeedNap commented Sep 30, 2020

You are right: I made these changes quickly for my personal project without thinking about backward compatibility

@NeedNap
Copy link
Author

NeedNap commented Oct 1, 2020

I fixed it by remove the "unused variable 'strViewBox'" warning

Copy link
Collaborator

@arielelkin arielelkin left a comment

Choose a reason for hiding this comment

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

Hey @NeedNap, please let me know your thoughts on my comments 😀

@@ -68,6 +103,20 @@ + (void)resetCache
return paths;
}

+ (NSArray<SVGBezierPath*> *)pathsFromSVGString:(NSString * const)svgString viewBox:(CGRect *)viewBox
Copy link
Collaborator

Choose a reason for hiding this comment

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

why a pointer to a CGRect? This creates the need to pass in an UnsafeMutablePointer<CGRect> in Swift.

Copy link
Author

Choose a reason for hiding this comment

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

I used this pointer to get the viewBox info and put this value in the _svg_viewBoxCache variable. I don't know if this part can be write in a better way :)

* @param viewBox The SVG viewBox tag rect.
*
*/
+ (NSArray<SVGBezierPath*> *)pathsFromSVGAtURL:(NSURL *)aURL viewBox:(CGRect *)viewBox;
Copy link
Collaborator

Choose a reason for hiding this comment

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

what happens if the SVG already contains a viewBox tag?

Copy link
Author

Choose a reason for hiding this comment

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

I don't understand this question. Do you mean if the SVGBezierPath class? This code read the viewBox tag from the SVG file and return to the caller (by the viewBox pointer) these values in a CGRect struct.

Copy link
Collaborator

@arielelkin arielelkin Oct 5, 2020

Choose a reason for hiding this comment

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

Ah ok now I understand the intention of your code and why you pass a pointer to a CGRect (I initially thought you wanted the caller to pass a CGRect as the viewBox to render the SVG in).

If the caller wants to read the viewBox tag in the SVG file, what if we expose this property on SVGBezierPath:

@property(nonatomic, readonly) CGrect viewBox;

This way, no need to pass a pointer.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, you are right, but I use the SVGBezierPath static function to get the viewBox information so I can't store the value in a class property.
Do you have any suggestions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ciao Alessio, sorry for the delay, I'll be able to take a look at this soon.

@arielelkin
Copy link
Collaborator

Alessio, sorry for the long wait.

I've been thinking about exposing viewBox as part of the PocketSVG API. The viewBox attribute defines the scaling and positioning of a graphic within its viewport. So I would expect an SVG renderer like PocketSVG to render an SVG accounting for its viewBox attribute. It shouldn't be the user's responsibility to check what an SVGImageView's or SVGLayer's viewBox is and then manually transform the graphic.

Did you have a similar use case or something else in mind?

In any case, if we're not applying the viewBox attribute to the render we should go for a simpler API. What do you think about adding this method to SVGBezierPath?

+ (CGRect)viewBoxFromSVGAtURL:(NSURL *)aURL;

This would do a simple XML parsing of the SVG string and convert the viewBox attribute into a CGRect (no need for caching it)

Let me know what you think!

@NeedNap
Copy link
Author

NeedNap commented Oct 26, 2020

@arielelkin I think what you propose is the best solution. The only con I see is that the svg file will be parsed two times.
In my use case I need this information to properly size the svg container, otherwise I can't know the exact svg dimensions.

Let me know if you need more explanations.

@arielelkin
Copy link
Collaborator

@NeedNap I found a simple way to parse the SVG just once and save the viewBox attribute, exposing it in SVGLayer and SVGImageView. It'll also show up as any individual SVGBezierPath's viewBox attribute.

Please review if this suits your need:
#185

Could you also please modify this PR (#181) so that it only adds support for svg stroke width change?

@arielelkin
Copy link
Collaborator

@NeedNap I hope you're well!

Are you ok to modify this PR so that it only adds support svg stroke width change?

@fjolnir fjolnir closed this Jul 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants