LineChart initialized

[marcrasi]

Thanks for creating this repo and PR! Let's try discussing things in here.

Brad's comments from the email thread apply to this code, so doing some of the stuff he suggested would be a good thing to start with. Do you have any questions about his comments, or do you want some more detailed comments on specific pieces of this code?

Also, could you commit all the third party libraries separately and then have a PR that just adds your code, so that the PR is smaller and easier to browse through? In fact, it might even be valuable to separate your code into a few separate PRs so that we can have separate detailed discussions about each one. This might be a good sequence of PRs:

1. The basic SPM package setup (e.g. Package.swift that builds all the third party libraries and builds a really simple executable that calls one of them).
2. The renderer backends
3. The line plotter
4. Some example usages of the line plotter.

[marcrasi]

I read through this a bit and have a few more high level comments.

Eliminate Vectorizer, just have Renderer

The functions inside VectorizerProtocol seem like a good start -- but could they be functions on a protocol Renderer instead, completely eliminating the need for Vectorizer and VectorizerProtocol? Something like this:

protocol Renderer {
    func drawRect(...)
    ...
}

// Use `struct` instead of `class` for things that are values. (See next section for more discussion about why I think this is a value.)
struct SVGRenderer : Renderer {
    var image: String

    func drawRect(...) {
        image += ...
    }
}

// Rename the C implementation CAGGRenderer, so that you can wrap it in a swift struct that conforms to a protocol.
// Might be a class, if it's a reference to some AGG drawing.
class AGGRenderer : Renderer {
    ... wrap CAGGRenderer ...
}

What is a Renderer?

Think about what precisely a Renderer is, and add doc comments to the protocol Renderer. Possibly even come up with a better name, if Renderer turns out not to capture the concept well.

I think a Renderer is something along the lines of "a value that receives commands from the plotting layer". Notice that this concept does not say anything about what the Renderer does with the commands! This allows implementations complete freedom to do what they want with the commands. e.g. the SVG one can be a string value that appends SVG elements to the string when it receives commands, the AGG one can be a reference to an AGG drawing that draws on the drawing when it receives commands, some OpenGL (or whatever) renderer in the future can be a reference to an OpenGL window that draws on the window when it receives commands.

One concrete consequence of this is that savePlotImage should not be part of the protocol. It constrains Renderers to be something that can actually save an image.

How should the plotting layer call the renderer?

Right now, your LinePlot stores its own renderer.

I suggest deleting the renderer from LinePlot, so that LinePlot only has fields containing data about the actual plot. Having objects that are "just data" is generally good because then you can pass them around, copy them, operate on them, serialize/deserialize them, etc, without having to worry about what renderer is attached to them.

Then to actually render them, they can have a method that accepts a renderer like

func render(to renderer: Renderer) {
  renderer.drawXYZ(...)
  ...
}

This is probably not the best user-facing API. We might not want user to have to keep track of their renderer and pass it in whenever they want to draw something. (Or maybe we do!?) But this is a job for the user-facing API layer, so we can deal with it there!

[KarthikRIyer]

Sorry for the commits. I didn't see your comments before. I've gone through them now. I'll make changes accordingly and open fresh PRs.

[KarthikRIyer]

Is it fine if I close this PR?

[BradLarson]

If you wanted to have a separate pull request for the plot-related functions (in addition to the library pull request), I'd be fine with closing this and starting a new one.

I agree with Marc's comments about the rendering design. Thinking more about it, the renderer should not be a property of the line plot and should instead be passed in to the one core rendering method (render, draw, update, or whatever it's called) and the sub-methods that draw the individual elements. That potentially could be something that would be set globally when a user was working with the library (such as the thread-local Context used in Swift for TensorFlow) and then passed in at render time.

In fact, I'd recommend splitting the core properties and layout logic from the rendering code for the LinePlot into the base class and then an extension that contains just the code that interacts with the renderer. That should make the class layout easier to read.

You may also want to think about grouping parameters for the LinePlot, because there are a lot of individual configuration settings. Maybe the title and its style properties could be separated into a PlotTitle type with good defaults, for example.

While I'm thinking about it, you may also want to make your parameter descriptions a little more verbose, using width instead of w, height instead of h, and so on. My personal preference is to have the function signature be as descriptive as possible, and I don't mind the extra typing.

Also, Swift code usually doesn't have get and set methods, instead relying on get {} set{} or willSet{} / didSet{} inside property definitions (more detail here). Many of these that I see in LinePlot here will probably go away with the migration of the renderer outside of the LinePlot, anyway.