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 4 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)
}
}
152 changes: 147 additions & 5 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 = dataSets
Copy link
Member

Choose a reason for hiding this comment

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

is there any implicit declare of dataSets here, I don't see init(arrayLiteral elements: ChartDataSetProtocol...) declared.
Besides, there used to be a PR that removes _ from some interval vars. Any guideline when to use _ and not? I remember I asked a similar question.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For the _, this should be changed, but that isn't in the scope of this PR.

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.

The first part is an error I will fix. Should be _dataSets = elements


self.initialize(dataSets: _dataSets)
}

@objc public init(dataSets: [ChartDataSetProtocol]?)
{
super.init()
Expand Down Expand Up @@ -757,3 +764,138 @@ 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)
{
self._dataSets.append(newElement)
calcMinMax(dataSet: newElement)
}

public func remove(at position: Index) -> Element
{
let element = self._dataSets.remove(at: position)
calcMinMax()
Copy link
Member

Choose a reason for hiding this comment

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

shall we use notifyDataChanged instead of calcMinMax here? in CombinedChartData, they works differently then their super class.

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 thought about that. I think we should keep the way it is now for this PR. The existing open func removeDataSet(_ dataSet: ChartDataSetProtocol!) -> Bool uses calcMinMax()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe we can rethink that in another PR though.

return element
}

public func removeFirst() -> Element
{
let element = self._dataSets.removeFirst()
notifyDataChanged()
return element
}

public func removeFirst(_ n: Int)
{
self._dataSets.removeFirst(n)
notifyDataChanged()
}

public func removeLast() -> Element
{
let element = self._dataSets.removeLast()
notifyDataChanged()
return element
}

public func removeLast(_ n: Int)
{
self._dataSets.removeLast(n)
notifyDataChanged()
}

// public func removeSubrange(_ bounds: Range<Index>) {
Copy link
Member

Choose a reason for hiding this comment

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

don't need to call notifyDatChanged here()? how about in replaceSubrange. Shall we consider add some unit tests for these type of changes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We do because we are accessing the backing var so it's never called. In a future PR I will remove the backing var and refactor this. Currently this works the exact same way as the other add/remove methods, but are simply Swift Protocol versions (as they should be).

// self.dataSets.removeSubrange(bounds)
// notifyDataChanged()
// }
}

// MARK: Swift Accessors
extension ChartData
{
//TODO: Reevaluate if warning is still true
Copy link
Member

Choose a reason for hiding this comment

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

I'm close to finish review soon, do we handle TODO here or in future PRs?

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 meant to delete it.

/// 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?
{
get
{
guard let index = index(forLabel: label, ignoreCase: ignoreCase) else { return nil }
return self[index]
}
}

public subscript(entry: ChartDataEntry) -> Element?
{
get
{
guard let index = index(where: { $0.entryForXValue(entry.x, closestToY: entry.y) === entry }) else { return nil }
return self[index]
}
}

public func appendEntry(_ e: ChartDataEntry, toDataSet dataSetIndex: Index)
{
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)
}
}
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
7 changes: 6 additions & 1 deletion Source/Charts/Data/Implementations/Standard/ScatterChartData.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 ScatterChartData: BarLineScatterCandleBubbleChartData
{
public override init()
public required init()
{
super.init()
}
Expand All @@ -23,6 +23,11 @@ open class ScatterChartData: BarLineScatterCandleBubbleChartData
{
super.init(dataSets: dataSets)
}

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

/// - returns: The maximum shape-size across all DataSets.
@objc open func getGreatestShapeSize() -> CGFloat
Expand Down