Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Module is not extensible #357

Closed
iheart2code opened this issue Sep 11, 2015 · 30 comments
Closed

Module is not extensible #357

iheart2code opened this issue Sep 11, 2015 · 30 comments

Comments

@iheart2code
Copy link

I do not have a suggested solution for this issue, but the Swift module is not nearly as extensible as the Java counterpart. In Java, you can create a subclass of a chart renderer and take advantage of internal/protected methods & properties to create a custom chart renderer. This does not appear to be the case with the Swift module.

I was trying to create a bar chart with rounded corners on the bars. I needed to subclass BarChartRenderer, but was unable to do so without moving the ios-charts source code into my app's module.

Is there a way to accomplish a similar level of API-parity with the Java counterpart while at the same time allowing for extensibility? Perhaps take advantage of protocols or something.

@danielgindi
Copy link
Collaborator

danielgindi commented Sep 12, 2015 via email

@pmairoldi
Copy link
Collaborator

I would suggest maybe writing an extension to BarChartRenderer and override the methods you want to change. Maybe worth a try.

@martinnormark
Copy link

You cannot override methods from extensions:

Extensions can add new functionality to a type, but they cannot override existing functionality.
Source: Docs, first note

@danielgindi
Copy link
Collaborator

@martinnormark it is not an extension but a module...

@martinnormark
Copy link

Yes, I was just replying to @petester42 's suggestion.

@pmairoldi
Copy link
Collaborator

There is a distinction though. If the property or function comes from the subclass then its fine.

extension UIViewController {
    /// doesn't work
    override var title: String? {
        get {
            return "Cool"
        } set(new) {

        }
    }
}

class VC: UIViewController { }

extension VC {
    /// works
    override var title: String? {
        get {
            return "Cool"
        } set {

        }
    }
}

I thought it might work since BarChartRenderer is a subclass of ChartDataRendererBase but I haven't tried it.

@pmairoldi
Copy link
Collaborator

@danielgindi I believe this kind of issue could be solved if the renderers were protocol oriented and not subclasses. Then the charts could then take any object that adheres to lets say RenderProtocol and make it easier for other to modify the rendering.

@martinnormark
Copy link

@petester42 Should work it seems. Then you only need to subclass the BarChartView class, since that instantiates the renderer and there's no way to configure from the outside which renderer class it will use.

@danielgindi
Copy link
Collaborator

I think, maybe, we should try turning into public the important methods that deserve to be protected or are protected in the android counterpart.

https://developer.apple.com/swift/blog/?id=11

@danielgindi
Copy link
Collaborator

I think that maybe things that are "protected" in the Java counterpart, should be public here, but prefixed with an underscore, so the developer/user can know whether he/she is supposed to access it or not.
I don't get why Apple has decided that there is no use case for "protected". I guess it just made their life easier to optimize to C-level performance, when they know that they can't expect "dynamic" subclassing of stuff that was supposed to be "sealed".

@broncha
Copy link

broncha commented Nov 3, 2015

+1 I am working on a custom renderer but I cannot access the animator

@simonmcl
Copy link

simonmcl commented Nov 9, 2015

+1 Trying to add a gradient to the Cubic Line chart, can't get at the drawCubicFill method. Means i'm going to have to throw anyway cocoapods and import manually so I can modify it.

@broncha
Copy link

broncha commented Nov 12, 2015

Bumped again. I need to assign a custom highlighter to a custom chart, derived from bubble chart. And cant access _highlighter neither extend ChartHighlighter

@danielgindi
Copy link
Collaborator

Yes this will be address soon

On Thu, Nov 12, 2015 at 11:40 AM, Rajesh Sharma notifications@github.com
wrote:

Bumped again. I need to assign a custom highlighter to a custom chart,
derived from bubble chart. And cant access _highlighter neither extend
ChartHighlighter


Reply to this email directly or view it on GitHub
#357 (comment)
.

@broncha
Copy link

broncha commented Nov 12, 2015

👍

@broncha
Copy link

broncha commented Nov 12, 2015

how do you suggest I do it for now?

@iheart2code
Copy link
Author

@broncha you're going to need to bring the source code into your project and modify the files directly.

@iheart2code
Copy link
Author

@danielgindi perhaps take a loot at the Protocol-Oriented Programming video from WWDC 2015 for inspiration here.

https://developer.apple.com/videos/play/wwdc2015-408/

@iheart2code
Copy link
Author

The more I look at this library from a Swift/extensibility perspective, the more I realize that what I am asking is probably out of the scope of what this library intends, since it is supposed to be a direct port of a Java counterpart.

@pmairoldi
Copy link
Collaborator

As long as the API for the library remains similar to the Java version then it would be fine. Changes also go both ways. If you add something good to iOS charts then there is is a good chance it makes its way to Java version and vice versa. Don't let it discourage you from implementing something cool for this library.

You can try to give protocol oriented programming for the ChartHighlighter a shot and see if it's feasible.

@broncha
Copy link

broncha commented Nov 19, 2015

About the custom highlighter part, I had to copy the entire ChartHighlighter and modified getHighlight.
Then introduce new _highlight property in my custom chart initialize . Of course this new _highlight was not accessed by the chart library. So I also had to override getHighlightByTouchPoint in my custom chart to use the new _highlight.

@liuxuan30
Copy link
Member

@broncha It seems you could replaced the _highlighter in initialize() with yours, then you don't have to override getHighlightByTouchPoint if no more actions to take? Subclass ChartHighlighter won't need you to copy the entire code.

@iheart2code
Copy link
Author

@petester42 took your advice. Please let me know if I went in the right direction with that. The sample app just works without any modifications. Unit tests don't seem to cover that part, but they all passed.

@liuxuan30
Copy link
Member

another quesiton, shall we make initialize() to public rather than internal? I saw some people want to replace the renderer, but they cannot override initialize() outside of the library:http://stackoverflow.com/questions/33994223/fetching-current-x-position-when-graph-is-set-to-scroll-in-ios-charts/34011274?noredirect=1#comment55831925_34011274

@martinnormark
Copy link

That's exactly what I intend to do. Subclass an existing renderer, and override methods. Then pass that renderer to the chart view.

Perhaps you could define your own renderer by setting a public property instead?

@liuxuan30
Copy link
Member

I am using the source files, so have no trouble of override internal stuff. But seems lots of people are sub classing outside of the library.

@danielgindi
Copy link
Collaborator

Moving to protocol oriented - will cause problem with keeping cross-platform API. Instead, everything in the renderers is now completely public :-)

@martinnormark
Copy link

Excellent 👍

Any idea when you'll cut a new release?

@danielgindi
Copy link
Collaborator

Probably around a day.

Just checking for any incompatibilities that I can find with the Android API, as this is the time to fix them, along with the other structural changes that were done (and also for critical bugs that must be included in this version)

@jenjia
Copy link

jenjia commented Jun 29, 2016

@martinnormark how do you pass the your custom renderer to the chartView?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants