Add Annotation Support

[WilliamHYZhang]

40

Hi,

This is my new PR for the addition of annotation support following the advice and help of @KarthikRIyer and @karwa.

First, I have created a protocol of type Annotation in PlotStyleHelpers.swift that contains common properties such as the size, color, and location, and also a TextAnnotation of type Annotation that contains the text (specific to TextAnnotation) and the aforementioned properties in Annotation. Please let me know if that was the correct location to put those definitions, thanks!

Next, I updated the GraphLayout.swift file. I added an array of type Annotation, initialized as an empty array. Annotations would be added to this array with the addAnotation mutable function that accepts type Annotation. So, if you want to add an annotation you would call:

lineGraph.addAnnotation(annotation: TextAnnotation(text:"HELLO WORLD", location: Point(0.95, 0.95), color: .black))

as @KarthikRIyer demonstrated in my previous PR.

I also defined a function, drawAnnotations, that is called when drawForeground is called. This function takes in the renderer and iterates through all of the annotations. To make this function compatible with all types of Annotations (currently now it's only TextAnnotation, but going forward annotations such as boxes, arrows, etc.), I have this function check if it is of type xAnnotation (as this stage just TextAnnotation, and if it is I perform an explicit downcast and perform the drawing function for that specific annotation. So, if in the future we want more types of annotations, we would simply define a new type conforming to the protocol Annotation, and add an if statement in the drawAnnotations function to handle that type.

Please let me know what you think!

P.S. @KarthikRIyer In your previous comment you suggested to put drawAnnotation function in the protocol, however would it make more sense to place it in GraphLayout as I did in this PR, as that is where the other draw functions are put? Thanks for your feedback.

EDIT: Now, the drawAnnotation function is a method of Annotation and is called in the drawAnnotations function of GraphLayout.swift for each annotation. Thanks to @odmir for his comment.

[odmir]

Hi! This looks really nice!

I think that what @KarthikRIyer meant is that the Annotation protocol should have a drawAnnotation method requirement. Then types conforming to that protocol, like TextAnnotation, will be required to implement that method. This is a nice thing, new types that want to "be" an Annotation will have to have a drawAnnotation method or the compiler will complain. This is to say, the annotations should know how to draw themselves and the protocol makes sure that they do. Then the drawAnnotations method of the GraphLayout struct should only really need to iterate over all Annotations in the array and directly call drawAnnotation on them (passing the renderer as argument), without needing to test if it is a TextAnnotation or an ArrowAnnotation first.

When eventually a new kind of annotation comes along it only needs to conform to Annotation and implement a drawAnnotation method. You will not need to edit the drawAnnotations method on GraphLayout and it will just work.

Hope this helps!

[WilliamHYZhang]

@odmir I see what @KarthikRIyer means now, thanks so much for clearing it up! I'll fix that up and push a new commit when I'm done. Thank you again!

[KarthikRIyer]

@WilliamHYZhang in addition to the above comments you need to remove the PlotAnnotation type you created earlier.

Also, have you tested your code on an example? You can edit the existing tests in the Tests folder temporarily to see if your feature works as expected, and then when you're done with it you should make a new test for this feature and update the reference images in Tests/Reference. You can run the tests simply bu running swift test.

In the watermark test It'd be nice if the watermark was a bit transparent so instead of using .black as the color you could use Color(0.0, 0.0, 0.0, 0.5).

Take a look at XCTestManifests.swift to see how the tests are made.

[WilliamHYZhang]

Thank you for your feedback! I'll work on implementing these changes today.

[karwa]

A couple of points:

• I would strip the Annotation protocol down to only the drawing function.

  The protocol is only useful for the GraphLayout, which itself doesn't care about the annotation's colour, or even its size or location. The only thing the GraphLayout cares about is that the annotation can draw itself with the renderer that we give it.

  And the function could just be named draw. We know it's an annotation - there's no need to say it again by calling the method drawAnnotation.

• I wonder if we should call the struct Text or Label. Again, do we really need to repeat the fact that this is an annotation? graph.annotations = [TextAnnotation(...), ArrowAnnotation(...)] just seems a bit longer and more verbose than it needs to be.

  graph.annotations = [Text(...), Arrow(...), ...] is much easier to read IMO.

• It would be nice if you could add a test (at least one), so we can all see that it works, and can make sure that nobody accidentally breaks it in the future. Just a simple example showing a chart with a text annotation would be great.

[KarthikRIyer]

Stripping the protocol to just the drawing function makes sense.

I think we could call the struct Text, as it could be used for watermarks also and it could be confused with the plot/axes labels. What do you think? And yeah, simply Text/Arrow is easier to read.

[WilliamHYZhang]

That makes sense, I will make the necessary changes today and add a test as well. Thanks!

[WilliamHYZhang]

Hi,

Thank you all again for the great feedback on this PR.

First, I've cleaned up and refactored the Annotation protocol and the Text struct to reflect the change requests. I agree that it's much cleaner and everything should be working now.

Next, I've added some tests. I created a new folder called Annotation in the SwiftPlotTests directory that will eventually contain all the tests for the types of annotations (now it only contains annotation-text). I tried to mimic the style of other test files/folders when writing these tests. In essence it's just an empty line graph with the text watermark on it.

I added corresponding .png and .svg files to agg and svg renderer types, however unfortunately because I don't have quartz (no MacOs), I couldn't add the reference file for that. Please note that consequently, the test check for quartz is currently commented out. If someone with access to MacOs and quartz could run swift test and add the corresponding outputted file to references that would be much appreciated!

Thanks so much,

William

[KarthikRIyer]

@WilliamHYZhang this looks good. But I'd prefer if the example was complete with a plot. It would be nice to know how the annotation would look with a proper graph. Also could you please make the text bigger? You still need to remove public struct PlotAnnotation{ from PlotStyleHelpers.swift line 23.

[WilliamHYZhang]

@KarthikRIyer, I don't see public struct PlotAnnotation{ anywhere in PlotStyleHelpers.swift

[WilliamHYZhang]

Ok, pushed update for the tests. Thanks for the review!

[KarthikRIyer]

@WilliamHYZhang this looks good. Thanks a lot for the contribution! Looking forward to see your further contributions in GCI.

Merging.