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

Chartdata collection conformance #3023

Merged
merged 10 commits into from
Jan 16, 2018
Merged

Chartdata collection conformance #3023

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6e80eb0
Added Collection conformances
jjatie Nov 18, 2017
36ca566
Fixed required initializers
jjatie Nov 20, 2017
cb32b08
ChartData adopts ExressibleByArrayLiteral
jjatie Dec 1, 2017
716f182
Pulled latest 4.0.0 and code style fixes
jjatie Jan 6, 2018
ca5afad
Updates for PR
jjatie Jan 11, 2018
8976b95
PR review fixes
jjatie Jan 12, 2018
f28d3d5
Removed unnecessary `get` from subscripts.
jjatie Jan 12, 2018
583dab6
Disabled `remove(at:)` for CombinedChartView
jjatie Jan 15, 2018
c0b7d65
Relocated `appendEntry(_:todataSet:)`
jjatie Jan 15, 2018
a71f87c
Removed methods from CombinedChartData
jjatie Jan 15, 2018
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
9 changes: 7 additions & 2 deletions Source/Charts/Data/Implementations/Standard/BarChartData.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import CoreGraphics

open class BarChartData: BarLineScatterCandleBubbleChartData
{
public override init()
public required init()
Copy link
Member

Choose a reason for hiding this comment

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

why many become required init() now?

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 a requirement of RangeReplaceableCollection

Copy link
Member

Choose a reason for hiding this comment

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

hmm.. My Dash API Doc does not show this to me

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

From Apple

Copy link
Member

Choose a reason for hiding this comment

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

image
Found a Dash bug :)

{
super.init()
}
Expand All @@ -23,7 +23,12 @@ open class BarChartData: BarLineScatterCandleBubbleChartData
{
super.init(dataSets: dataSets)
}


public required init(arrayLiteral elements: ChartDataSetProtocol...)
{
super.init(dataSets: elements)
}

/// The width of the bars on the x-axis, in values (not pixels)
///
/// **default**: 0.85
Expand Down
7 changes: 6 additions & 1 deletion Source/Charts/Data/Implementations/Standard/BarLineScatterCandleBubbleChartData.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import Foundation

open class BarLineScatterCandleBubbleChartData: ChartData
{
public override init()
public required init()
{
super.init()
}
Expand All @@ -22,4 +22,9 @@ open class BarLineScatterCandleBubbleChartData: ChartData
{
super.init(dataSets: dataSets)
}

public required init(arrayLiteral elements: ChartDataSetProtocol...)
{
super.init(dataSets: elements)
}
}
9 changes: 7 additions & 2 deletions Source/Charts/Data/Implementations/Standard/BubbleChartData.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import CoreGraphics

open class BubbleChartData: BarLineScatterCandleBubbleChartData
{
public override init()
public required init()
{
super.init()
}
Expand All @@ -23,7 +23,12 @@ open class BubbleChartData: BarLineScatterCandleBubbleChartData
{
super.init(dataSets: dataSets)
}


public required init(arrayLiteral elements: ChartDataSetProtocol...)
{
super.init(dataSets: elements)
}

/// Sets the width of the circle that surrounds the bubble when highlighted for all DataSet objects this data object contains
@objc open func setHighlightCircleWidth(_ width: CGFloat)
{
Expand Down
7 changes: 6 additions & 1 deletion Source/Charts/Data/Implementations/Standard/CandleChartData.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import Foundation

open class CandleChartData: BarLineScatterCandleBubbleChartData
{
public override init()
public required init()
{
super.init()
}
Expand All @@ -22,4 +22,9 @@ open class CandleChartData: BarLineScatterCandleBubbleChartData
{
super.init(dataSets: dataSets)
}

public required init(arrayLiteral elements: ChartDataSetProtocol...)
{
super.init(dataSets: elements)
}
}
199 changes: 183 additions & 16 deletions Source/Charts/Data/Implementations/Standard/ChartData.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

import Foundation

open class ChartData: NSObject
open class ChartData: NSObject, ExpressibleByArrayLiteral
{
internal var _yMax: Double = -Double.greatestFiniteMagnitude
internal var _yMin: Double = Double.greatestFiniteMagnitude
Expand All @@ -24,13 +24,20 @@ open class ChartData: NSObject

internal var _dataSets = [ChartDataSetProtocol]()

public override init()
public override required init()
{
super.init()

_dataSets = [ChartDataSetProtocol]()
}


public required init(arrayLiteral elements: ChartDataSetProtocol...)
{
super.init()

_dataSets = elements

self.initialize(dataSets: _dataSets)
}

@objc public init(dataSets: [ChartDataSetProtocol]?)
{
super.init()
Expand Down Expand Up @@ -491,22 +498,20 @@ open class ChartData: NSObject
}

/// Adds an Entry to the DataSet at the specified index. Entries are added to the end of the list.
@objc open func addEntry(_ e: ChartDataEntry, dataSetIndex: Int)
@objc(addEntry:dataSetIndex:)
open func appendEntry(_ e: ChartDataEntry, toDataSet dataSetIndex: Index)
{
if _dataSets.count > dataSetIndex && dataSetIndex >= 0
{
let set = _dataSets[dataSetIndex]

if !set.addEntry(e) { return }

calcMinMax(entry: e, axis: set.axisDependency)
}
else
guard indices.contains(dataSetIndex) else
{
print("ChartData.addEntry() - Cannot add Entry because dataSetIndex too high or too low.", terminator: "\n")
return
}

let set = self[dataSetIndex]
if !set.addEntry(e) { return }
calcMinMax(entry: e, axis: set.axisDependency)
}

/// Removes the given Entry object from the DataSet at the specified index.
@objc @discardableResult open func removeEntry(_ entry: ChartDataEntry, dataSetIndex: Int) -> Bool
{
Expand Down Expand Up @@ -757,3 +762,165 @@ open class ChartData: NSObject
return max
}
}

// MARK: MutableCollection
extension ChartData: MutableCollection
{
public typealias Index = Int
public typealias Element = ChartDataSetProtocol

public var startIndex: Index
{
return _dataSets.startIndex
}

public var endIndex: Index
{
return _dataSets.endIndex
}

public func index(after: Index) -> Index
{
return _dataSets.index(after: after)
}

public subscript(position: Index) -> Element
{
get{ return _dataSets[position] }
set{ self._dataSets[position] = newValue }
}
}

// MARK: RandomAccessCollection
extension ChartData: RandomAccessCollection
{
public func index(before: Index) -> Index
Copy link
Member

Choose a reason for hiding this comment

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

I saw RandomAccessCollection has more required methods to provide, but here only one index(before: Index) which just comes from BidirectionalCollection. What knowledge I missed here?

Copy link
Collaborator Author

@jjatie jjatie Jan 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

From the RandomAccessCollection doc:

The RandomAccessCollection protocol adds further constraints on the associated Indices and SubSequence types, but otherwise imposes no additional requirements over the BidirectionalCollection protocol. However, in order to meet the complexity guarantees of a random-access collection, either the index for your custom type must conform to the Strideable protocol or you must implement the index(_:offsetBy:) and distance(from:to:) methods with O(1) efficiency

The index of the collection (Int) conforms to Strideable, so we get the conformance for free.

{
return _dataSets.index(before: before)
}
}

// MARK: RangeReplaceableCollection
extension ChartData: RangeReplaceableCollection
Copy link
Member

Choose a reason for hiding this comment

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

Generally it should works fine with the collection protocol adoption. The devil may exist in CombinedChartData in the future, as it works differently than usual chart data.

We actually don't append/remove data sets into a combined data, we just assign new sub chartData. e.g.

    open override func removeDataSetByIndex(_ index: Int) -> Bool
    {
        print("removeDataSet(index) not supported for CombinedData", terminator: "\n")
        return false
    }

I think we should override the protocols in CombinedChartData to not allow such operations?

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 will do that right now : )

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

{
public func append(_ newElement: Element)
{
guard !(self is CombinedChartData) else
{
fatalError("append(_:) not supported for CombinedData")
}

_dataSets.append(newElement)
calcMinMax(dataSet: newElement)
}

public func remove(at position: Index) -> Element
{
guard !(self is CombinedChartData) else
{
fatalError("remove(at:) not supported for CombinedData")
}

let element = _dataSets.remove(at: position)
calcMinMax()
return element
}

public func removeFirst() -> Element
{
guard !(self is CombinedChartData) else
{
fatalError("removeFirst() not supported for CombinedData")
}

let element = _dataSets.removeFirst()
notifyDataChanged()
return element
}

public func removeFirst(_ n: Int)
{
guard !(self is CombinedChartData) else
{
fatalError("removeFirst(_:) not supported for CombinedData")
}

_dataSets.removeFirst(n)
notifyDataChanged()
}

public func removeLast() -> Element
{
guard !(self is CombinedChartData) else
{
fatalError("removeLast() not supported for CombinedData")
}

let element = _dataSets.removeLast()
notifyDataChanged()
return element
}

public func removeLast(_ n: Int)
{
guard !(self is CombinedChartData) else
{
fatalError("removeLast(_:) not supported for CombinedData")
}

_dataSets.removeLast(n)
notifyDataChanged()
}

public func removeSubrange<R>(_ bounds: R) where R : RangeExpression, ChartData.Index == R.Bound
{
guard !(self is CombinedChartData) else
{
fatalError("removeSubrange<R>(_:) not supported for CombinedData")
}

_dataSets.removeSubrange(bounds)
notifyDataChanged()
}

public func removeAll(keepingCapacity keepCapacity: Bool)
{
guard !(self is CombinedChartData) else
{
fatalError("removeAll(keepingCapacity:) not supported for CombinedData")
}

_dataSets.removeAll(keepingCapacity: keepCapacity)
notifyDataChanged()
}
}

// MARK: Swift Accessors
extension ChartData
{
/// Retrieve the index of a ChartDataSet with a specific label from the ChartData. Search can be case sensitive or not.
/// **IMPORTANT: This method does calculations at runtime, do not over-use in performance critical situations.**
///
/// - Parameters:
/// - label: The label to search for
/// - ignoreCase: if true, the search is not case-sensitive
/// - Returns: The index of the DataSet Object with the given label. `nil` if not found
public func index(forLabel label: String, ignoreCase: Bool) -> Index?
{
return ignoreCase
? index { $0.label?.caseInsensitiveCompare(label) == .orderedSame }
: index { $0.label == label }
}

public subscript(label: String, ignoreCase: Bool) -> Element?
{
guard let index = index(forLabel: label, ignoreCase: ignoreCase) else { return nil }
return self[index]
}

public subscript(entry: ChartDataEntry) -> Element?
{
guard let index = index(where: { $0.entryForXValue(entry.x, closestToY: entry.y) === entry }) else { return nil }
return self[index]
}
}
7 changes: 6 additions & 1 deletion Source/Charts/Data/Implementations/Standard/CombinedChartData.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ open class CombinedChartData: BarLineScatterCandleBubbleChartData
private var _candleData: CandleChartData!
private var _bubbleData: BubbleChartData!

public override init()
public required init()
{
super.init()
}
Expand All @@ -28,6 +28,11 @@ open class CombinedChartData: BarLineScatterCandleBubbleChartData
{
super.init(dataSets: dataSets)
}

public required init(arrayLiteral elements: ChartDataSetProtocol...)
Copy link
Member

Choose a reason for hiding this comment

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

have you checked if any tricky bug or trap (like index to a wrong position) regards CombinedChartData collection conformance? CombinedChartData is a wrapper putting other data together like barData, lineData.

Copy link
Collaborator Author

@jjatie jjatie Jan 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I made sure that all the new Swift methods/properties have the same implementations as the existing ones, so this shouldn't be an issue.

Copy link
Member

Choose a reason for hiding this comment

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

sure. It will take more time, need to catch up the dataset protocol first.

{
super.init(dataSets: elements)
}

@objc open var lineData: LineChartData!
{
Expand Down
7 changes: 6 additions & 1 deletion Source/Charts/Data/Implementations/Standard/LineChartData.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import Foundation
/// Data object that encapsulates all data associated with a LineChart.
open class LineChartData: ChartData
{
public override init()
public required init()
{
super.init()
}
Expand All @@ -23,4 +23,9 @@ open class LineChartData: ChartData
{
super.init(dataSets: dataSets)
}

public required init(arrayLiteral elements: ChartDataSetProtocol...)
{
super.init(dataSets: elements)
}
}
7 changes: 6 additions & 1 deletion Source/Charts/Data/Implementations/Standard/PieChartData.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import Foundation

open class PieChartData: ChartData
{
public override init()
public required init()
{
super.init()
}
Expand All @@ -23,6 +23,11 @@ open class PieChartData: ChartData
super.init(dataSets: dataSets)
}

public required init(arrayLiteral elements: ChartDataSetProtocol...)
{
super.init(dataSets: elements)
}

@objc var dataSet: PieChartDataSetProtocol?
{
get
Expand Down
9 changes: 7 additions & 2 deletions Source/Charts/Data/Implementations/Standard/RadarChartData.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ open class RadarChartData: ChartData
self.labels = labels
}

public override init()
public required init()
{
super.init()
}
Expand All @@ -38,7 +38,12 @@ open class RadarChartData: ChartData
{
super.init(dataSets: dataSets)
}


public required init(arrayLiteral elements: ChartDataSetProtocol...)
{
super.init(dataSets: elements)
}

open override func entryForHighlight(_ highlight: Highlight) -> ChartDataEntry?
{
return getDataSetByIndex(highlight.dataSetIndex)?.entryForIndex(Int(highlight.x))
Expand Down
Loading