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.

Sign up for GitHub

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

Jump to bottom

Minor changes to Animator #3005

Merged
merged 2 commits into from
Nov 17, 2017
Merged

Minor changes to Animator #3005

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ee337d7
Minor changes to Animator
jjatie Nov 14, 2017
50fdd2d
Merge remote-tracking branch 'upstream/master' into animator-cleanup
jjatie Nov 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 22 additions & 60 deletions Source/Charts/Animation/Animator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ open class Animator: NSObject

fileprivate var _easingX: ChartEasingFunctionBlock?
fileprivate var _easingY: ChartEasingFunctionBlock?

public override init()
{
super.init()
Expand All @@ -68,11 +68,10 @@ open class Animator: NSObject

@objc open func stop()
{
guard let _displayLink = _displayLink else {
return
}
_displayLink.remove(from: RunLoop.main, forMode: RunLoopMode.commonModes)
self._displayLink = nil
guard _displayLink != nil else { return }

_displayLink?.remove(from: .main, forMode: .commonModes)
_displayLink = nil

_enabledX = false
_enabledY = false
Expand All @@ -91,7 +90,7 @@ open class Animator: NSObject
stopBlock?()
}

fileprivate func updateAnimationPhases(_ currentTime: TimeInterval)
private func updateAnimationPhases(_ currentTime: TimeInterval)
{
if _enabledX
{
Expand All @@ -103,7 +102,7 @@ open class Animator: NSObject
elapsed = duration
}

phaseX = _easingX?(elapsed, duration) ?? Double(elapsed / duration)
phaseX = _easingX?(elapsed, duration) ?? elapsed / duration
}

if _enabledY
Expand All @@ -115,15 +114,8 @@ open class Animator: NSObject
{
elapsed = duration
}

if let _easingY = _easingY
{
phaseY = _easingY(elapsed, duration)
}
else
{
phaseY = Double(elapsed / duration)
}

phaseY = _easingY?(elapsed, duration) ?? elapsed / duration
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think using if let here instead of a one-liner here would be more readable

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree on this one. ?? is a very common operator in Swift (it's all over the stdlib). Would you prefer something like

phaseY = _easingY?(elapsed, duration)
    ?? elapsed / duration

or

phaseY = _easingY?(elapsed, duration) ??
    elapsed / duration

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also worth noting that on #2698 there were many cases of this being approved, including this line (thought it was written as phaseY = _easingY?(elapsed, duration) ?? Double(elapsed / duration)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My problem is not with the ?? My problem is with the optional closure. I think It would be clearer to unwrap it first with if let before calling it. It is minor though. It’s up to you.

Copy link
Collaborator Author

@jjatie jjatie Nov 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would agree with you in a case like:

let x = myFuncWithNonEscaping? {
     // Logic
} ?? myDefaultValue

But in this case I really think this is okay.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it cluttered but it doesn’t really matter.

}
}

Expand All @@ -134,7 +126,6 @@ open class Animator: NSObject
updateAnimationPhases(currentTime)

delegate?.animatorUpdated(self)

updateBlock?()

if currentTime >= _endTime
Expand Down Expand Up @@ -202,20 +193,11 @@ open class Animator: NSObject
/// - parameter xAxisDuration: duration for animating the x axis
/// - parameter yAxisDuration: duration for animating the y axis
/// - parameter easingOption: the easing function for the animation
@objc open func animate(xAxisDuration: TimeInterval, yAxisDuration: TimeInterval, easingOption: ChartEasingOption)
@objc open func animate(xAxisDuration: TimeInterval, yAxisDuration: TimeInterval, easingOption: ChartEasingOption = .easeInOutSine)
{
animate(xAxisDuration: xAxisDuration, yAxisDuration: yAxisDuration, easing: easingFunctionFromOption(easingOption))
}

/// Animates the drawing / rendering of the chart on both x- and y-axis with the specified animation time.
/// If `animate(...)` is called, no further calling of `invalidate()` is necessary to refresh the chart.
/// - parameter xAxisDuration: duration for animating the x axis
/// - parameter yAxisDuration: duration for animating the y axis
@objc open func animate(xAxisDuration: TimeInterval, yAxisDuration: TimeInterval)
{
animate(xAxisDuration: xAxisDuration, yAxisDuration: yAxisDuration, easingOption: .easeInOutSine)
}


/// Animates the drawing / rendering of the chart the x-axis with the specified animation time.
/// If `animate(...)` is called, no further calling of `invalidate()` is necessary to refresh the chart.
/// - parameter xAxisDuration: duration for animating the x axis
Expand All @@ -233,33 +215,23 @@ open class Animator: NSObject
// Take care of the first frame if rendering is already scheduled...
updateAnimationPhases(_startTimeX)

if _enabledX || _enabledY
if _enabledX || _enabledY,
_displayLink == nil
{
if _displayLink == nil
{
_displayLink = NSUIDisplayLink(target: self, selector: #selector(animationLoop))
_displayLink?.add(to: RunLoop.main, forMode: RunLoopMode.commonModes)
}
_displayLink = NSUIDisplayLink(target: self, selector: #selector(animationLoop))
_displayLink?.add(to: .main, forMode: .commonModes)
}
}

/// Animates the drawing / rendering of the chart the x-axis with the specified animation time.
/// If `animate(...)` is called, no further calling of `invalidate()` is necessary to refresh the chart.
/// - parameter xAxisDuration: duration for animating the x axis
/// - parameter easingOption: the easing function for the animation
@objc open func animate(xAxisDuration: TimeInterval, easingOption: ChartEasingOption)
@objc open func animate(xAxisDuration: TimeInterval, easingOption: ChartEasingOption = .easeInOutSine)
{
animate(xAxisDuration: xAxisDuration, easing: easingFunctionFromOption(easingOption))
}

/// Animates the drawing / rendering of the chart the x-axis with the specified animation time.
/// If `animate(...)` is called, no further calling of `invalidate()` is necessary to refresh the chart.
/// - parameter xAxisDuration: duration for animating the x axis
@objc open func animate(xAxisDuration: TimeInterval)
{
animate(xAxisDuration: xAxisDuration, easingOption: .easeInOutSine)
}


/// Animates the drawing / rendering of the chart the y-axis with the specified animation time.
/// If `animate(...)` is called, no further calling of `invalidate()` is necessary to refresh the chart.
/// - parameter yAxisDuration: duration for animating the y axis
Expand All @@ -277,30 +249,20 @@ open class Animator: NSObject
// Take care of the first frame if rendering is already scheduled...
updateAnimationPhases(_startTimeY)

if _enabledX || _enabledY
if _enabledX || _enabledY,
_displayLink == nil
{
if _displayLink == nil
{
_displayLink = NSUIDisplayLink(target: self, selector: #selector(animationLoop))
_displayLink?.add(to: RunLoop.main, forMode: RunLoopMode.commonModes)
}
_displayLink = NSUIDisplayLink(target: self, selector: #selector(animationLoop))
_displayLink?.add(to: .main, forMode: .commonModes)
}
}

/// Animates the drawing / rendering of the chart the y-axis with the specified animation time.
/// If `animate(...)` is called, no further calling of `invalidate()` is necessary to refresh the chart.
/// - parameter yAxisDuration: duration for animating the y axis
/// - parameter easingOption: the easing function for the animation
@objc open func animate(yAxisDuration: TimeInterval, easingOption: ChartEasingOption)
@objc open func animate(yAxisDuration: TimeInterval, easingOption: ChartEasingOption = .easeInOutSine)
{
animate(yAxisDuration: yAxisDuration, easing: easingFunctionFromOption(easingOption))
}

/// Animates the drawing / rendering of the chart the y-axis with the specified animation time.
/// If `animate(...)` is called, no further calling of `invalidate()` is necessary to refresh the chart.
/// - parameter yAxisDuration: duration for animating the y axis
@objc open func animate(yAxisDuration: TimeInterval)
{
animate(yAxisDuration: yAxisDuration, easingOption: .easeInOutSine)
}
}