Closed tocsoft closed 3 years ago
Merging #126 (45c2a26) into master (c29e5de) will decrease coverage by
6.00%
. The diff coverage is82.66%
.
@@ Coverage Diff @@
## master #126 +/- ##
==========================================
- Coverage 71.05% 65.05% -6.01%
==========================================
Files 86 86
Lines 5472 5488 +16
Branches 1119 1121 +2
==========================================
- Hits 3888 3570 -318
- Misses 1359 1695 +336
+ Partials 225 223 -2
Flag | Coverage Δ | |
---|---|---|
unittests | 65.05% <82.66%> (-6.01%) |
:arrow_down: |
Flags with carried forward coverage won't be shown. Click here to find out more.
Impacted Files | Coverage Δ | |
---|---|---|
.../Processing/Extensions/ClearRectangleExtensions.cs | 100.00% <ø> (ø) |
|
...wing/Processing/Extensions/DrawBezierExtensions.cs | 100.00% <ø> (ø) |
|
...rawing/Processing/Extensions/DrawLineExtensions.cs | 100.00% <ø> (ø) |
|
...ing/Processing/Extensions/FillPolygonExtensions.cs | 100.00% <ø> (ø) |
|
...g/Processing/Extensions/FillRectangleExtensions.cs | 100.00% <ø> (ø) |
|
...rocessing/ShapeGraphicOptionsDefaultsExtensions.cs | 100.00% <ø> (ø) |
|
src/ImageSharp.Drawing/Processing/ShapeOptions.cs | 100.00% <ø> (ø) |
|
src/ImageSharp.Drawing/Processing/TextOptions.cs | 100.00% <ø> (ø) |
|
...rawing/Processing/TextOptionsDefaultsExtensions.cs | 100.00% <ø> (ø) |
|
...ssing/Processors/Text/DrawTextProcessor{TPixel}.cs | 75.35% <57.89%> (-22.65%) |
:arrow_down: |
... and 42 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update c29e5de...45c2a26. Read the comment docs.
@tocsoft Are there any issues you see with adding the property upstream? I.E do you think people will try to use it for non related operations?
I would love to see the duplication between TextGraphicsOptions
and ShapeGraphicsOptions
being removed, but I don't think adding .Transform
to the common GraphicsOptions
is a good idea unless we want to make all the processors in the core library to support it. Otherwise it would be confusing to users (there is a Transform
property but it's not being applied ...).
This would mean that for example .Resize(...)
also applies an arbitrary free transform, and .Transform(...)
has an additional transform etc. This is not the way our API-s were evolving in the core library so far, and it's just too big change to even consider at this point IMO.
We should rather do an API simplification right within ImageSharp.Drawing
.
ShapeGraphicsOptions
, TextGraphicsOptions
, ShapeOptions
, TextOptions
... it's just too confusing.
How about something like this:
- public class ShapeGraphicsOptions { ... }
- public class TextGraphicsOptions { ... }
+public class DrawingOptions
+{
+ public ShapeOptions ShapeOptions { get; set; }
+ public TextOptions ShapeOptions { get; set; }
+ public Matrix3x2 Transform { get; set; }
+ public GraphicsOptions GraphicsOptions { get; set; }
+}
Then instead of doing .SetShapeOptions(x => x.Transform = ...)
or .SetShapeOptions(x => x.Transform = ...)
, we can have a SetTransform(Matrix3x2)
extension method.
Prerequisites
Description
Adds a
Transform
property to bothShapeOptions
andTextOptions
(which can be registered globally).This allows for usages using this syntax (for shapes):
or for text
Current limitations on this pattern is that it uses 2 separate properties... Ideally we would add the
Transform
property toGraphicsOptions
(which is available for both shapes and text processors) but that class lives in the main ImageSharp repo so would require a change over there.. if we are happy to make the change over there then we should do that first then update this PR to set it centrally.resolves #125