Charts for ResearchKit (Issue #92) - #178

Hey @coxy1989, this looks like a great PR, congrats.

I've been drafting a ResearchKit Coding Style guide. It might be a good idea to make sure your PR conforms to it, as there are some divergencies between the ResearchKit and AppCore styles.

(Specially, the opening brackets on the same line item.)

Do note that the guide is a work in progress: and any comments are welcome (via Issue #174 or the mailing list).

Wow, that was quick! Thanks!

I'll review in detail tomorrow. Thanks

@rsanchezsaez to review.

btw this is not expected to go into v1.1, if we take this it'll be after the branch.

I have not started reviewing the code yet, but there seems to be a problem in the pie chart layout. The bottom percentage label overlaps the legend. @coxy1989: Could you look into that?

It happens on all devices. See an iPhone 6 screenshot below.

I'm in the middle of reviewing the code, expect more comments soon. A head start: we'll need to add appledoc comments to all the new headers, it'd be a good idea to start thinking about them.

oops sorry didn't see you were up to this before I pushed that change. Will take a look at your comments now.

I'm internally debating on what the names for the main chart classes should be.

First, there's the debate between chart vs graph. Semantically, all graphs are charts but not all charts are graphs. A pie chart is certainly not a graph, so I think it was a good call to rename APCPieGraph to ORKPieChart.

Regarding ORKBaseGraph, ORKDiscreteGraph and ORKLineGraph, I'm weakly in favour of renaming all those to ORK*Chart, to denote that they are "ResearchKit chart objects". I think I'd also drop the Base prefix from the superclass (which denotes the fact that it's an abstract class, but we can document that instead).

So I think I'd go for ORKBidimensionalChart, with its ORKDiscreteChart and ORKLineChart subclasses (or maybe ORKBidimensionalGraphChart, with its ORKDiscreteGraphChart and ORKLineGraphChart if we want to be more specific at the cost of verbosity).

@jwe-apple, @coxy1989: What are your thoughts on this?

Please also note that there is a chance for greater code reuse between the discrete and line charts. I've been noticing a good amount of duplicated code which could be moved to the superclass.

Let's not jump the gun on actually making these naming changes. I don't have time to review right now, but this needs at least a couple other reviews.

Ok, let's leave the class naming for later. I'll delve into reviewing the actual code.

I have finally finished reworking and refactoring the chart code to the point I think is ready to merge into master. I submitted a new PR against @coxy1989's branch. The changeset is huge, so a peer review would quite helpful. Sorry again for the delay!

An overview of some of the changes:

• The main chart classes (ORKPieChartView; and ORKGraphView with its ORKDiscreteGraphView and ORKLineGraphView subclasses) now operate in the same way regarding layout and animation.
• For performance, I have decoupled graphic-element-CALayers creation from their layout. Layer creation happens when the dataSource changes (as that is what informs the number of CALayer needed). Layer layout generally happens in -layoutSubviews. When a property changes that needs layout of some elements only, the relevant layers are laid out (instead of indiscriminately re-layouting everything).
• All three views respond to -traitCollectionDidChange: and trigger their own layout on rotation, without having to manually do so externally.
• All three views can by animated when first appearing on screen by calling animateWithDuration: from the owning view controller's -viewWillAppear:.
• Another performance improvement: ORKGraphView reuses the layer.contents for the single points (that was a huge performance boost when rotating the device).
• ORKPieChartView was sliced into pie, legend and titleText views. This simplifies logic and allows to use constraints for a simple layout on the parent view. The legend and titleText views calculate their own intrinsic content size, and the pie takes the remaining space.
• Added dynamic contentSizeCategory text support to all ORKGraphView text, and removed needless axisTitleFont property. Also renamed some properties to be more UIKit-like.
• Made sure all custom properties propagate and work as intended. If you change a property when the graph is displayed, it re-layouts and redraws immediately if needed.
• Simplified the ORKCatalog example to make it more visually appealing. Added a more involved ORKTest example/test. The ORKTest example changes most of the graph properties at staged delays to make sure the live updating works.
• Minor headerdoc comment improvements.
• Merged latest master for easy merging.

All in all, I think performance is better than it was before. Rotation of ORKPieChartView seems completely smooth on an iPhone 6. However, rotation to landscape of the ORKGraphViews is a bit janky. This is because they have a big amount of graphic sublayers (mostly the lines, as the point layers reuse memory now, as mentioned above). I couldn't figure a way using less layers, because the line layers completely depend on the plotted data.

Also, it's worth considering if we should rename ORKGraphView and its subclasses to something ending in chart, for example, ORK*GraphChartView or ORK*BidimensionalChartView for coherence with ORKPieChartView (in my mind, a graph is a subtype of chart, and a pie is another subtype of chart).

Some things we could do as follow-on issues:

• Add a delegate method to optionally provide colors to secondary graphs on multi-plot graphs (currently, the main plot is painted of the view's tintColor, and all the secondary plots are painted of referenceLineColor).
• Add a delegate method to allow to select which graph to scrub over on multi-plot graphs (currently, it always scrubs over the first graph).
• Add support for graphs with a large number of points, by setting an interval for the xAxis labels and reference lines (currently the xAxis gets very crowded if you add lots of points).

If you agree this are worthwhile improvements, I can add follow-on issues tomorrow.

Also, @coxy1989, thanks a lot for your previous work! It was a pleasure working with your code. For example, the scrubber animation works beautifully, I didn't had to change any of that. The rotation performance and animation issues were mostly inherited from from the AppCore architecture and implementation.

And the randomized colors-ORKPieChart example is very nice. I moved that to ORKTest (on ORKCatalog I left a very simple pie chart with the original colors).

@rsanchezsaez Thanks for the great work. Tested on iPhone 6 plus, the rotation is much more smooth.

Agree, the class names should end with ChartView.
Also I agree on the three following issues.

@coxy1989 Thanks your previous work too!
Can you merge @rsanchezsaez's PR ASAP, so we can review it here?

I have added a few more commits with last minute fixes, and the final refactor to ORK*GraphChartView.

A note: performance also depends on the number of plotted graph points: it decreases quickly as you add points. For a fair comparison, try with the ORKTest example, as it has more points than the ORKCatalog one.

And the follow-on issues: #379.

Thanks Chris. Typos fixed.

Thanks @coxy1989 and @rsanchezsaez.
It looks good after my brief review. Accepting it.