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

AnimatedMoveViewJob crash #3061

Open
Ramonfan opened this issue Dec 1, 2017 · 13 comments
Open

AnimatedMoveViewJob crash #3061

Ramonfan opened this issue Dec 1, 2017 · 13 comments

Comments

@Ramonfan
Copy link

Ramonfan commented Dec 1, 2017

AnimatedMoveViewJob don't overwrite animationEnd

@liuxuan30
Copy link
Member

give more details please.

@liuxuan30
Copy link
Member

liuxuan30 commented Dec 14, 2017

I now understand this.

    internal func animationEnd()
    {
        // Override this
    }

it changed to

    internal func animationEnd()
    {
        fatalError("`animationEnd()` must be overriden by subclasses")
    }

@jjatie Just try ChartsDemo and swipe in the line chart, it will trigger this.

I think we should revert this back to the old implementation here. Not every subclass need to override this, some of them just do nothing. What you think?
adding empty override seems strange, as this is not must-override method.

liuxuan30 added a commit that referenced this issue Dec 14, 2017
revert animationUpdate() and animationEnd()
not trigger crash if subclass does nothing
@jjatie
Copy link
Collaborator

jjatie commented Dec 14, 2017

I think it would be better to explicitly override it with an empty implementation. Thoughts?

@liuxuan30
Copy link
Member

yeah that's where we think differently. asking every subclass to just add an empty override is introducing duplicated code?
Using fatal what I understand is if you don't do that, bad things would happen for sure. But this is not true for the animator? As by default it does nothing at the animation end, but when you want to add something, you can add in subclass then.

@jjatie
Copy link
Collaborator

jjatie commented Dec 14, 2017

#3099

@liuxuan30
Copy link
Member

liuxuan30 commented Dec 14, 2017

I already added #3098 :)

@jjatie
Copy link
Collaborator

jjatie commented Dec 14, 2017

I agree that's the objective-c way to do it. In swift, AnimatedViewPortJob would be a protocol. This is one of those spots where we're kind of stuck between two worlds.

@liuxuan30
Copy link
Member

when we are changing to protocol we could change then, like optional

@jjatie
Copy link
Collaborator

jjatie commented Dec 14, 2017

Yep! That will probably be another year or two though.

@jjatie
Copy link
Collaborator

jjatie commented Dec 14, 2017

@liuxuan30 by the way. How is 4.0.0 kept up to date. Would it be appropriate for me to PR master into 4.0.0?

@liuxuan30
Copy link
Member

liuxuan30 commented Dec 14, 2017

It's a headache, as more and more PRs coming to both master and 4.0.
one way is to let git mark the conflicts and we solve it once and for all. Or keep merging master into 4.0, and solve every conflict. Seems both are headaches.
Will get you being able to pushing to 4.0 directly make it easier?

@jjatie
Copy link
Collaborator

jjatie commented Dec 14, 2017

I don't mind solving the merge conflicts every so often. I think it will be easier to do it periodically, than all at once at the end. I guess I will PR when I think the diff is becoming too big.

@liuxuan30
Copy link
Member

ok. I will try ping daniel if this week passed.

liuxuan30 added a commit that referenced this issue Dec 14, 2017
FreddyZeng added a commit to FreddyZeng/Charts that referenced this issue Jan 2, 2018
* 'master' of https://github.com/danielgindi/Charts: (23 commits)
  Update ViewPortHandler.swift (ChartsOrg#3143)
  add option to build demo projects unit tests on iOS (ChartsOrg#3121)
  Replaced relevant `ChartUtils` methods with `Double` extensions (ChartsOrg#2994)
  Update 4.0.0 with master (ChartsOrg#3135)
  Removed redundant ivars in BarLineChartViewBase (ChartsOrg#3043)
  fix ChartsOrg#1830. credit from ChartsOrg#2049 (ChartsOrg#2874)
  Makes ChartsDemo compiling again (ChartsOrg#3117)
  Fixed using wrong axis (Issue ChartsOrg#2257)
  Removed methods and properties deprecated in 1.0 (ChartsOrg#2996)
  for ChartsOrg#3061 revert animationUpdate() and animationEnd() not trigger crash if subclass does nothing
  The backing var is not necessary. (ChartsOrg#3000)
  Replaced `ChartUtils.Math` in favour of an extension on `FloatingPoint` (ChartsOrg#2993)
  Minor logic cleanup (ChartsOrg#3041)
  Fix a bug may cause infinite loop. (ChartsOrg#3073)
  for ChartsOrg#2745. chart should be weak.
  fileprivate -> private (ChartsOrg#3042)
  Removed `isKind(of:)`
  Removed @objc from internal properties
  Fixes for PR
  Made use of `==` where appropriate to simplify logic
  ...

# Conflicts:
#	Source/Charts/Data/Implementations/Standard/LineChartDataSet.swift
#	Source/Charts/Renderers/BarChartRenderer.swift
FreddyZeng added a commit to FreddyZeng/Charts that referenced this issue Jan 20, 2018
* 'master' of https://github.com/danielgindi/Charts: (34 commits)
  Fixed X-Axis Labels Not Showing (ChartsOrg#3154) (ChartsOrg#3174)
  fix programatical unhighlighting for BarCharView (ChartsOrg#3159)
  Give the users customizable axis label limits (Fixes ChartsOrg#2085) (ChartsOrg#2894)
  bump pod version
  chart views now use open legend renderer property instead of internal one (ChartsOrg#3149)
  Fix axis label disappear when zooming in deep enough (ChartsOrg#3132)
  added DataApproximator+N extension (ChartsOrg#2848)
  Minor cleanup to Highlighter types (ChartsOrg#3003)
  Refactored ChartUtils method into CGPoint extension (ChartsOrg#3087)
  Update ViewPortHandler.swift (ChartsOrg#3143)
  add option to build demo projects unit tests on iOS (ChartsOrg#3121)
  Replaced relevant `ChartUtils` methods with `Double` extensions (ChartsOrg#2994)
  Update 4.0.0 with master (ChartsOrg#3135)
  Removed redundant ivars in BarLineChartViewBase (ChartsOrg#3043)
  fix ChartsOrg#1830. credit from ChartsOrg#2049 (ChartsOrg#2874)
  Makes ChartsDemo compiling again (ChartsOrg#3117)
  Fixed using wrong axis (Issue ChartsOrg#2257)
  Removed methods and properties deprecated in 1.0 (ChartsOrg#2996)
  for ChartsOrg#3061 revert animationUpdate() and animationEnd() not trigger crash if subclass does nothing
  The backing var is not necessary. (ChartsOrg#3000)
  ...

# Conflicts:
#	Source/Charts/Data/Implementations/Standard/LineChartDataSet.swift
#	Source/Charts/Highlight/BarHighlighter.swift
#	Source/Charts/Renderers/BarChartRenderer.swift
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

3 participants