Add Path and Shape #123
Add Path and Shape #123
Conversation
There was a problem hiding this comment.
Sorry for taking a while to get around to this.
Thanks so much for getting this to where it is, it's looking great. I've just left a few small change requests but they're mostly formatting, naming and documentation (which were all great for the most part).
I'll test this out locally in a bit more depth once I've finished off my assignment due soon. I'm probably gonna try making a graphing example with bar charts, line charts and pie charts to stress test everything.
| /// - Parameters: | ||
| /// - path: The path to be updated. | ||
| /// - source: The source to copy the path from. | ||
| /// - pointsChanged: If `false`, the ``Path/actions`` of the source have not changed. |
There was a problem hiding this comment.
I feel like shapeChanged could be a batter description, cause I can see it being ambiguous from the name of the property alone whether pointsChanged would be true or false when the points are the same but the interpolation has changed. shape might not be the best word but the only good word I can think of is path which is of course confusing in its own right due to SwiftCrossUI's path type including the stroke style.
There was a problem hiding this comment.
Another alternative could be onlyStyleChanged
There was a problem hiding this comment.
What about actionsChanged? Since it's identifying whether the Path.actions property has changed
Sources/SwiftCrossUI/Path.swift
Outdated
| /// - clockwise: `true` if the arc is to be drawn clockwise, `false` if the arc is to | ||
| /// be drawn counter-clockwise. Used to determine whether to draw the larger arc or | ||
| /// the smaller arc identified by the given start and end angles. |
There was a problem hiding this comment.
I feel like referring to the larger arc and the smaller arc is probably a bit confusing because they could be the same size, the clockwise arc could be larger, or the clockwise arc could be smaller. Maybe just;
... Used to determine which of the two possible arcs to draw between the given start and end angles.
| public protocol Shape: View | ||
| where Content == EmptyView { |
| /// a dry run, while the other properties are used to inform the layout engine how big | ||
| /// or small the shape can be. The ``ViewSize/idealSize`` property should not vary with | ||
| /// the `proposal`, and should only depend on the shape's contents. Pass `nil` for the | ||
| /// maximum width/height if the shape has no maximum size (and therefore may occupy |
There was a problem hiding this comment.
Remove the comment comment about occupying the entire screen because I can see it being a bit misleading; a shape with unlimited size may still occupy less than the entire screen if it likes to keep its aspect ratio etc.
|
|
||
| var cgColor: CGColor { | ||
| CGColor( | ||
| red: CGFloat(red), green: CGFloat(green), blue: CGFloat(blue), alpha: CGFloat(alpha)) |
| shapeLayer.fillRule = path.usesEvenOddFillRule ? .evenOdd : .nonZero | ||
|
|
||
| shapeLayer.lineJoin = | ||
| switch path.lineJoinStyle { |
There was a problem hiding this comment.
The CI checks are warning about CGLineJoin and CGLineCap not being frozen enums. It's probably best to handle the @unknown default: case with a printed warning and a sensible default.
There was a problem hiding this comment.
If I add a print statement then this can no longer be a switch expression :/
but yeah, what you said makes sense
| _ collection: WinUI.GeometryCollection, | ||
| lastPoint: Point | ||
| ) -> PathFigure { | ||
| var pathGeo: PathGeometry |
There was a problem hiding this comment.
Name this pathGeometry unless there some reason it can't be called that
| let pathGeo = geometry.getAt(geometry.size - 1) as? PathGeometry, | ||
| pathGeo.figures.size > 0 | ||
| { | ||
| let figure = pathGeo.figures.getAt(pathGeo.figures.size - 1)! |
There was a problem hiding this comment.
Move this into the if condition to avoid the force unwrap
There was a problem hiding this comment.
The comment from ~30 lines earlier also applies here. Should I change both? I don't think the result can ever be nil in practice.
Not currently implemented in GtkBackend or Gtk3Backend, but I may be able to do the former in the near future