-
Notifications
You must be signed in to change notification settings - Fork 475
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
Split drawing code into separate files #934
Split drawing code into separate files #934
Conversation
Yes, I think this is a good idea. As far as I can see, you didn't change any code so far, just moved out the drawing code into partial classes. As this is a bit difficult to review, you may want to add a note if you do change anything else. |
Ok will add PR comments if something is changed. |
@mrbean-bremen @H1Gdev Added notes to PR description. |
I think it's not readable in IDE... I think the following will be helpful. |
I think next step would be moving all drawing code into separate project. |
Yes, exactly. |
It seems that the PR related to this will make a big change toward |
Yes, I also thought that we should make a release before these changes, good point. |
This PR is ready for final review. Moving to separate project is more complex task and requires some interfaces and code refactoring with breaking changes so I would do that work is separate PR. |
I'll see if I can review this tomorrow, and also make a release of the current version (bit knocked out today after the Booster). |
I made a release based on the current version (really have to understand how this works with Nerdbank.GitVersioning, I couldn't get it to work correctly and worked around it), but didn't get the time for a review (again). Though from a cursory glance it looks as if nothing is really changed apart from the restructuring (which is what I would expect). Maybe @H1Gdev can have a look, and I will see if I can merge this finally later tomorrow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can see, this all looks good. Anyway, the compiler is your friend in this case - thanks for this time-consuming piece of work.
#if !NO_SDC | ||
public static RectangleF GetRectangle(this SvgRectangle r) | ||
{ | ||
return new RectangleF(r.X, r.Y, r.Width, r.Height); | ||
} | ||
|
||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be factored out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would leave that when we split into separate project, there are more such issues to solve
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I think we can merge this, thanks!
…ONTRIBUTING.md Generators Nuget README.md Samples Source Tests doc docfx.json index.md license.txt Split all classes classes with drawing code BuildProcessTemplates CONTRIBUTING.md Generators Nuget README.md Samples Source Tests doc docfx.json index.md license.txt Put NO_SDC if/def around drawing interfaces (ISvgClipable, IGraphicsProvider, ISvgRenderer, IFontDefn) BuildProcessTemplates CONTRIBUTING.md Generators Nuget README.md Samples Source Tests doc docfx.json index.md license.txt Put NO_SDC if/def around drawing only classes (GdiFontDefn, SvgFontDefn) BuildProcessTemplates CONTRIBUTING.md Generators Nuget README.md Samples Source Tests doc docfx.json index.md license.txt Move implicit operator float to NO_SDC if/def BuildProcessTemplates CONTRIBUTING.md Generators Nuget README.md Samples Source Tests doc docfx.json index.md license.txt Move GetRectangle method into NO_SDC if/def
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was out on the weekends and couldn't review.
Unnecessary code remains due to the separation. I think these removals are necessary to clarify the dependencies.
Please consider in next PR.
|
||
namespace Svg | ||
{ | ||
public partial class SvgDocument : SvgFragment, ITypeDescriptorContext |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think not need ITypeDescriptorContext
.
Reference Issue
Part of #590
Continuation of #931
What does this implement/fix? Explain your changes.
Splits of all drawing code inside NO_SDC if/def into separate files. This should be easier to read and maintain also make next step closer for #590
Any other comments?
Notable changes from master:
Interface specific to System.Drawing renderer:
Classes specific to System.Drawing renderer: