Make ChartHighlighter a protocol, publicly accessible

[iheart2code]

Relates to #357

[pmairoldi]

Pretty sweet stuff man 👍. Could this be used from any graph type and not just BarLineChartViewBase?

[iheart2code]

Yes, it can. Although I had some difficulty figuring out how to get a downcast version of the chart into some of the classes. See the TODO here: https://github.com/danielgindi/ios-charts/pull/567/files#diff-ac32fb888c389b4e17eca7e5ac42579dR20

I am assuming that the solution involves generics somehow. Never been very strong on generics.

[pmairoldi]

Something like this could possibly work

public protocol ChartHighlighter {
    public typealias ChartViewType
    weak var chart: ChartViewType? { get set }
}

[danielgindi]

I'm going to hold this a bit, as Phil and I are working on some changes that make the library more interface-oriented, mainly to allow querying for data (instead of pushing an array of entries).
We're trying to keep it 100% backwards compatible, but things could happen.

Anyway, I'll try to add this into the mix before we merge to master!

[iheart2code]

No worries. Good luck with your work. I'm excited to see what you've got in store.

[danielgindi]

Don't you think that the use of an extension in this case is a little bit problematic?

I mean if we want to have an interface with some base functionality implemented, we should probably have an "abstract" base class...

What will happen if an implementation has a function with the same name as one implemented in the extension?

[danielgindi]

@iheart2code Actually I don't get why not make the original class' properties public..?

[pmairoldi]

defining a protocol and then defining an extension of that protocol is the same as an abstract base class. Like can be seen below.

protocol CoolType {
    func superFunction() -> String
}

extension CoolType {
    func superFunction() -> String {
        return "blue"
    }
}

struct AType: CoolType {

}

struct BType: CoolType {

    func superFunction() -> String {
        return "red"
    }
}

let a = AType().superFunction()
let b = BType().superFunction()

The output looks like so:

[danielgindi]

@petester42 thanks for explaining!

Is there an advantage of having this as an abstract class and not just making those properties public?
Because as I see it, if the class conforms to a protocol, then its methods are public anyway as those are protocol methods. Which means that just making the properties public in the first place will achieve the same desired effect.

[danielgindi]

I just want to be finished with this one correctly on the iOS and Android side, before merging in dynamic-preparations branch which introduces interfaces between current dataset classes and their implementations, and easy Realm.io integration.

[pmairoldi]

The advantage is that you have a flatter hierarchy instead of more nested one with subclass of a subclass etc.

You can also set the access control to protocol methods like so:

public struct BType: CoolType {

    internal func superFunction() -> String {
        return "red"
    }
}

that function would only be public inside the library which for what this is trying to accomplish probably doesn't make sense but you can do it.

[danielgindi]

Remember that the library is also used from ObjC, so if anyone tries to call a protocol method from ObjC, it will let him...

Also that protocol should actually be exposed to ObjC, which is not the case right now.

Anyway, I still don't see why we need the additional hierarchy if we just want to make things public!

[pmairoldi]

The point would be that a user would not have to subclass something from the library in order set their own chart highlighter. Subclassing add a level of complexity since you never know if you need to call super or not (unless the function is properly annotated). Having it be a protocol makes it more flexible for a user since it is more straight forward. Implement these methods and it will work.

[danielgindi]

Well in such a case we would have to rename the protocol methods to specify the highlighter context, as one can (and often do) implement multiple protocols in one class.

@PhilJay what do you say about this? Basically this means making ChartHighlighter an interface, moving the implementation to DefaultChartHighlighter.

The thing that I mostly don't like about this is that this would mean a major inconsistency API with the Android side.
As an "extension" is something that only iOS allows, and so all current Highlighters would have to subclass DefaultChartHighlighter, while on the iOS side they just implement the base interface and get the functionality by default.
The iOS implementation is like a combination of an interface and an abstract class...

@petester42 I now get why these changes were implemented in the PR, but as you probably know the biggest strength of the framework is being cross-platform :-) We just need to carefully consider this before implementing.

[pmairoldi]

For sure, from the issues I know a bunch of people use this with objc and of course the API consistency with android is a priority.

Without having actually gone through it I would say that this should probably be rejected based on the things you mentioned. I don't know about protocol extensions in objc but I would assume they don't work.

[danielgindi]

@petester42 Oh no, I haven't event thought about protocol extensions in ObjC... No they don't work. That's an issue on iOS even before considering Android.

So sadly this PR is going to be rejected... @iheart2code do you want to modify the PR to just make things public, make another PR, or just leave that to us?