Update the renderers with dedicated atomic rendering methods in order to make rendering overrides easier to implement.

[4np]

Context

Often when implementing charting features, there is a requirement to implement certain UI changes. The more simple changes can often be accomplished by subclassing a particular renderer and overriding some of its methods, but more elaborate changes can be hard to accomplish.

While many methods are marked with open visibility and can theoretically be overloaded, in practice this is not as straightforward as most of those methods either call other methods or use variables that have private or internal visibility.

Consider the use case where you want to fill a bar chart with a pattern instead of a color. In order to do so, you would need to subclass the BarChartRenderer and override drawDataSet(context:, dataSet:, index:) (or drawHighlighted(context:, indices:) to update the highlight rendering).

There are a couple of challenges with this approach:

• drawDataSet(context:, dataSet:, index:) performs quite some calculations you really do not want to replicate and want to keep 'in the charting framework' (separation of concerns).
• these methods call a number of other methods that have internal or private visibility, which you cannot call from your subclassed renderer (for example prepareBuffer(dataSet:, index:) or BarLineScatterCandleBubbleRenderer.isInBoundsX(entry:, dataSet:)), which in turn requires you to duplicate those implementation as well.
• similarly these methods use a number of variables that have internal or private visibility (e.g. _barShadowRectBuffer, _buffers, etcetera), again requiring unnecessary code duplications.
• you want to focus on the rendering override, instead of re-implementing a lot of code in your subclassed renderer that, in essence, does not really matter for what you aim to accomplish.
• because of all the code duplication from the base class, up-taking a new Charts version is cumbersome and requires diffing duplicate logic for changes.

Solution

This Pull Request lifts the rendering logic out of the methods into their own dedicated methods with open visibility. This way it becomes really easy to create a custom renderer that implements custom rendering behavior, without the need of having to duplicate complex logic or re-implement private / internal methods and/or variables. It allows you to focus on what matters, while keeping your subclassed renderer focussed and simple.

Example: Bar Chart Pattern Fill.

This is an example of creating a custom bar chart renderer that will fill bars with a circular pattern:

/// Custom bar chart renderer that fills bars with a circle based pattern.
class PatternFilledBarChartRenderer: BarChartRenderer {
    override func renderFill(with color: UIColor, for rect: CGRect, in context: CGContext,  dataSet: IBarChartDataSet) {
        let patternSize = CGSize(width: 10, height: 10)
        let patternCallback: CGPatternDrawPatternCallback = { (pointer, context) -> Void in
            context.addArc(center: CGPoint(x: 5, y: 5), radius: 5, startAngle: 0, endAngle: CGFloat(2.0 * .pi), clockwise: true)
            context.fillPath()
        }
        var callbacks = CGPatternCallbacks(version: 0, drawPattern: patternCallback, releaseInfo: nil)
        let pattern = CGPattern(info: nil,
                                bounds: CGRect(origin: CGPoint(x: 0, y: 0), size: patternSize),
                                matrix: .identity,
                                xStep: patternSize.width,
                                yStep: patternSize.height,
                                tiling: .constantSpacing,
                                isColored: false,
                                callbacks: &callbacks)
        let patternColorSpace = CGColorSpace(patternBaseSpace: CGColorSpaceCreateDeviceRGB())
        let colorComponents = UnsafeMutablePointer<CGFloat>.allocate(capacity: 4)
        color.getRed(&colorComponents[0], green: &colorComponents[1], blue: &colorComponents[2], alpha: &colorComponents[3])

        defer {
            colorComponents.deinitialize(count: 4)
            colorComponents.deallocate()
        }

        context.saveGState()
        context.setFillColorSpace(patternColorSpace!) //swiftlint:disable:this force_unwrapping
        context.setFillPattern(pattern!, colorComponents: colorComponents) //swiftlint:disable:this force_unwrapping
        context.fill(rect)
        context.restoreGState()
    }
}

Implementing this rendering override can now be done by overriding a single method in a straightforward manner, whereas before you needed to copy over pretty much the complete implementation of BarChartRenderer.swift (and more) while making small alterations to accomplish the same.

Screenshot:

Lastly

It would be greatly appreciated if this could make it into a newly drafted release in the not so distant future 🙏🏻 😉

[codecov-io]

Codecov Report

Merging #4297 into master will increase coverage by 0.02%. The diff coverage is 48.61%.

@@            Coverage Diff             @@
##           master    #4297      +/-   ##
==========================================
+ Coverage    41.8%   41.83%   +0.02%     
==========================================
  Files         124      124              
  Lines       14117    14173      +56     
==========================================
+ Hits         5902     5929      +27     
- Misses       8215     8244      +29

Impacted Files	Coverage Δ
Source/Charts/Renderers/LineRadarRenderer.swift	8% <ø> (+0.3%)	:arrow_up:
Source/Charts/Renderers/BubbleChartRenderer.swift	0% <0%> (ø)	:arrow_up:
Source/Charts/Renderers/PieChartRenderer.swift	69.59% <100%> (+0.3%)	:arrow_up:
Source/Charts/Renderers/LegendRenderer.swift	60.04% <25%> (-1.23%)	:arrow_down:
Source/Charts/Renderers/BarChartRenderer.swift	74.13% <48.71%> (-0.49%)	:arrow_down:
Source/Charts/Renderers/LineChartRenderer.swift	66.82% <78.26%> (+0.71%)	:arrow_up:
...s/Data/Implementations/Standard/ChartDataSet.swift	40.14% <0%> (+0.35%)	:arrow_up:

---

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 847e822...d3e82c3. Read the comment docs.

[jjatie]

I'm all for this, but there has been resistance to adding more function calls in the past. I believe the justification for this hasn't been valid since about Swift 3, but as a result @danielgindi will have to decide.

[4np]

Thank you for your feedback and thoughts @jjatie 👍

[4np]

@danielgindi @liuxuan30 @jjatie : do you have any thoughts about adding these override points for rendering logic?

[4np]

@danielgindi @liuxuan30 : I updated the PR to the latest revision from master.

Perform these three steps to the demo application in order to reproduce the example in my screenshots below:

• add the code below to the bottom of BarChartViewController.swift
• change @IBOutlet var chartView: BarChartView! to @IBOutlet var chartView: PatternFilledBarChartView!
• change the view's class from BarChartView to PatternFilledBarChartView in BarChartViewController.xib

/// Custom bar chart view that fills bars with a circular pattern.
class PatternFilledBarChartView: BarChartView {
    override init(frame: CGRect) {
        super.init(frame: frame)
        updateRenderer()
    }
    
    required init?(coder aDecoder: NSCoder) {
        super.init(coder: aDecoder)
        updateRenderer()
    }
    
    private func updateRenderer() {
        let chartAnimator = renderer?.animator ?? Animator()
        renderer = PatternFilledBarChartRenderer(dataProvider: self,
                                                 animator: chartAnimator,
                                                 viewPortHandler: viewPortHandler)
    }
}

/// Custom bar chart renderer that fills bars with a circular pattern.
class PatternFilledBarChartRenderer: BarChartRenderer {
    override func renderFill(with color: NSUIColor, for rect: CGRect, in context: CGContext, dataSet: BarChartDataSetProtocol, entry: BarChartDataEntry?) {
        let patternSize = CGSize(width: 10, height: 10)
        let patternCallback: CGPatternDrawPatternCallback = { (pointer, context) -> Void in
            context.addArc(center: CGPoint(x: 5, y: 5), radius: 5, startAngle: 0, endAngle: CGFloat(2.0 * .pi), clockwise: true)
            context.fillPath()
        }
        var callbacks = CGPatternCallbacks(version: 0, drawPattern: patternCallback, releaseInfo: nil)
        let pattern = CGPattern(info: nil,
                                bounds: CGRect(origin: CGPoint(x: 0, y: 0), size: patternSize),
                                matrix: .identity,
                                xStep: patternSize.width,
                                yStep: patternSize.height,
                                tiling: .constantSpacing,
                                isColored: false,
                                callbacks: &callbacks)
        let patternColorSpace = CGColorSpace(patternBaseSpace: CGColorSpaceCreateDeviceRGB())
        let colorComponents = UnsafeMutablePointer<CGFloat>.allocate(capacity: 4)
        color.getRed(&colorComponents[0], green: &colorComponents[1], blue: &colorComponents[2], alpha: &colorComponents[3])

        defer {
            colorComponents.deinitialize(count: 4)
            colorComponents.deallocate()
        }

        context.saveGState()
        context.setFillColorSpace(patternColorSpace!) //swiftlint:disable:this force_unwrapping
        context.setFillPattern(pattern!, colorComponents: colorComponents) //swiftlint:disable:this force_unwrapping
        context.fill(rect)
        context.restoreGState()
    }
}

Screenshots

[4np]

~Added a fix for #3701 that will take the axis font into account when determining the size of the labels.~

Reverted in 7e1f6a0 as it caused the tests to fail and it deviated from the focussed PR.

[4np]

@danielgindi @liuxuan30 @jjatie : just a quick look at open issues and PRs (see for example these ☝🏻), it looks like this PR will solve quite a number of them. Rather than extending the charting library with specific logic for very specific use cases, this PR allows people to create their custom renderers and override rendering to fit their specific needs.

[bivant]

The only issue I see here is a performance question. Does it change with this implementation and how much if yes. Other than that this is a perfect solution in many-many cases. I would say it should be done for all renderers (or should have such option) because recently had to modify xAxis one to match designer idea of the beauty :) https://github.com/danielgindi/Charts/pull/3754

[4np]

@bivant : It shouldn't really, as it is only introducing dedicated open overridable render methods rather than doing the rendering inline nested inside private methods. It does not add additional complexity or computational overhead. It depends, of course, on what you do yourself inside a custom renderer, but it should make customizations much easier to accomplish. The example I gave above with the circular fill pattern is still very speedy and feels no different from the default implementation.

[4np]

@danielgindi @liuxuan30 @jjatie: anybody?

[4np]

@danielgindi @liuxuan30 @jjatie: bump...

[scinfu]

When this will be merged?

[4np]

@scinfu see #4569

[liuxuan30]

@jjatie what you think for this PR now?

[liuxuan30]

I had a quick look and it seems great, if we don't consider the android part; unfortunately we need to think about iOS and android interface again :(

[jjatie]

I think that as Charts is today, the implementation makes sense. Two notes:

1. It seems heavy to subclass the chart in the example. Why not just set the renderer on a BarChartView?
2. It feels strange to have to drill down the existing renderer to pull out its animator. Looking at our implementation, it looks like we can just pass in the animator directly from the chart which feels better.

If we agree that we should move forward with this, we need to audit for inline-ability. We also need to ensure all DataRenderer types are updated to reflect this model. It is also a good time to audit the naming of a lot of these methods and their parameters. (I would say render*() should be all renamed draw*() to be more in line with UIKit convention and to be more accurate)

[4np]

@liuxuan30 : In essence the public Charts API does not change, it merely adds ways to more easily customize rendering behavior by introducing a couple of focussed open methods. Perhaps MPAndroidCharts can take inspiration of this change and implement it in there as well? 🤷‍♂️ MPAndroidChart can still accomplish the same thing without having these focussed rendering methods, it will just be more work. It will also help you to get rid of a lot of the Issues and PRs that could be just be solved by overriding these renderers.

@jjatie : Yes true, you don't need to subclass BarChartView and just use the PatternFilledBarChartRenderer in the example, I just thought it would just be a good example class for people to understand. If the animator is available in the chart itself that would be even better, although I think it wasn't (publicly) available in the chart itself? Not sure though.

May I suggest that this perhaps does not merge to danielgindi:master but that we rather change the PR base to a feature branch you create off master? Then perhaps this PR can be merged to that feature branch allowing you to make the changes you want to make (for example render > draw or perhaps some of the method signatures) before it makes it to master? Or do you want me to make some changes on my end?

ps. @inlinable could be interesting performance wise, but I'm not sure that would work for such public overridable methods? Perhaps only when you're absolutely certain these remain stable.

[liuxuan30]

@danielgindi please take a look at this PR

[danielgindi]

I think this is something that I would actually want to port Android too. Will try to dedicate some time soon

[kelvinofula]

Quick question, how do I apply the same logic to achieve a piechart with rounded corners? I've been really trying it for a long time without success. Any help will really be much appreciated. Thanks a lot.

[liuxuan30]

@kelvinofula do not hijack.

@jjatie I think we reach an agreement with @danielgindi that, we could go ahead to make some iOS features and refactorings.

[AppCodeZip]

Recently I have been working on a project, to which I have added a chart library and display a beautiful rounded bar chart in swift. We can achieve this functionality easily. Please check this demo.

[4np]

@AppCodeZip : Do not hijack this Pull Request, your example is exactly opposite of what this PR is about.

[liuxuan30]

@jjatie I'm sorry to say that, I got a layoff and find a new job now, so I would be super busy for the rest of the year, tons of pressure from corp projects.. I might be missing for a few months (and already)

If you think the PR is good to go, probably you want to go ahead without my review.

[4np]

Ouch @liuxuan30, I am sorry to hear that. I can imagine your mind is not with this repository and pull request. Best of luck with your job hunt. Perhaps iOS Dev Jobs might be useful to you?

[liuxuan30]

@4np haha thank you. I think my new job is lovely and challenging, especialy there are 6 months trial period for me, so I have to spend all my time on that. I really wanted to spend more time on Charts but recently I guess I couldn't afford.

So I have to stay quiet for some time. I trust @jjatie and @danielgindi could take care of some PRs.

[4np]

@pmairoldi @liuxuan30 @jjatie @danielgindi

Could you please take another look at this PR? :) Right now we have to maintain a fork to make it easier to override rendering, but it also makes uptaking latest changes more difficult. It would be great to have this merge into the repository so we can get rid of the fork.

Like mentioned above, it might solve a lot of other issues or, at the very least, make it easier for other developers to implement rendering changes.

[417-72KI]

@4np @pmairoldi @liuxuan30 @jjatie @danielgindi Any updates?

[4np]

@pmairoldi @liuxuan30 @jjatie @danielgindi

This PR has been open for close to 3 years. I would still like to see this PR getting merged though. Any updates?