Gdi+ related errors are now wrapped in a specific exception with more detailed information #502
Conversation
… a more logical message (including reference to documentation)
… the mac (by having the CoreCompat included)
gvheertum
changed the title
|
I just noticed that this also pushed the netcore changes... This one is kinda related to that branch, so once that is merged I think this one can be merge in. |
|
Hm, I'm not sure that it is a good idea to mix these branches... the netcore branch seems to be branched from another test branch I made earlier for the netstandard stuff, and I'm not sure how that merge would work out - there has been the netstandard stuff added, adapted, partly removed again... I would prefer to have a cleaner PR here, otherwise it is difficult to understand the changes. |
|
i can merge this PR into the netcore branch, make a new prerelease and see how it works. |
|
also merged master into the netcore branch, new nuget preview is ready to test: |
|
Thanks @tebjan! I'm still a bit unclear what are the changes of the .NetCore2.2 branch w/r to master (except the automatic git versioning). Shall we merge this change also to master? |
|
it's based on the older netstadard2.0 branch, has the new SDK style project format with automatic nuget build, auto versioning and the netcore build target. if we get positive feedback from @gvheertum about his changes for macOS we can merge back, i'd say. |
|
ah ok - I had abandonded that branch in favor of the changes for .NetCore2.2, and probably should have deleted it, as it has diverged from master. We have to be careful how to merge back these changes to master, but I agree that it would be a good idea to do after the feedback. |
Looks good to me. Did a testrun on MacOs and it seems to work as expected. So you've got my blessing. :) I still have to do some research on how to get it as easy as possible for MacOs to get it compiling, maybe I'll add a platform specific csproj or buildstep somewhere, because currently I have to remove the net35 target from the project file before I can build with the additional CoreCompat package (as long as the net35 is in the frameworks the package will yield errors). But that's something for another time (I still have to get used to VSCode and working with csproj files, because I am a bit too used to the VS 2017/2019 way of working). |
did you use the nuget or built from source? |
Source build, do you want me to test the NuGet (that'll take a bit longer since I don't have MacOs projects/builds that use the Svg package, only been playing with the unittests directly from the repo sources). |
|
@tebjan I found a way to test the NuGet version also (changed the unit-test project to use the NuGet instead of the project reference). That however required me to also put the package include for the CoreCompat in the project file (since that one was previously carried over from the Svg project include). But I now can confirm that both the nuget and the source behave as expected. |
| catch (Exception ex) | ||
| { | ||
| Trace.TraceWarning(ex.Message); | ||
| if(ExceptionCaughtIsGdiPlusRelated(ex)) { throw; } //Gdi+ errors should be rethrown |
There was a problem hiding this comment.
This block is application of styles.
In my understanding, I think that GDI+ is irrelevant in this process.
Is this necessary ?
|
Good point. I was not really sure about that part. There was some error handling there already, which “swallowed” the errors that happened there. I added the check there to be sure that if a gdi+ exception occurred there it would not be ignored.
However, I guess the code won’t even reach that part of there is a gdi+ issue, since I expect it to fail earlier.
But as said, it’s just for additional safety and a bit with regard to consistency. I did not like the fact that exceptions were ignored since that caused odd behavior in some cases. I think leaving it in is not that much of an issue, if so I think that the check can be removed there. There could be a small performance impact if there are many issues in the style parsing (since the check code will run each time to see if it’s a gdi+ error, but I have the feeling that impact will be quite small).
|
|
Thanks for your response.
If this is not a direct solution to this issue, I think that it should be removed. |
|
Fair point. The (doc) processing code is complex enough already. I’ll do some additional validation soon and will remove the redundant code if indeed no gdi+ interaction is to be expected.
I’m also considering doing a different check for the gdi+ compatibility and wire that up in the beginning of the function. I’m considering a simple function with a call to invoke gdi+, which can also be used as a compatibility check for developers before invoking the svg library. I think that’s going to help readability and portability of that specific part. The result of this call can be cached in the svg lib to mitigate possible performance issues.
Will be continued :)
|
Reference Issue
Fixes #501
What does this implement/fix? Explain your changes.
SvgDocument.Open now has some additional checks to see if the error might be related to the gdi+ issues described in #501. We scan the exception for the presence of a certain type of exception and message in the exception. The specific exceptions is re-thrown (while other exceptions are only logged and not thrown) and is handled at the end of the Load function. If we are handling the specific gdi+ exception, we will wrap it in an exception with a bit more detail.
Any other comments?
Added a commented ItemGroup for the CoreCompat package in the Svg.csproj file. This is commented by default, to enable building/running on Mac uncomment the ItemGroup.