Resolve conflict for 4.0 branch and master

[liuxuan30]

This is the pull request for merging 4.0 branch into master.

It's my bad that I forgot to directly merge master into 4.0 branch. Instead, I branch master to this ResolveConflict and merge 4.0 into this branch.

I have tried my best to resolve all conflicts and inconsistent code, but the code that does not trigger a conflict still has errors.

The plan is to review this carefully and get it merged before iOS 14 rolls out.

@jjatie @danielgindi if you have some time now, please help review it and maybe fix some compile errors. I'll keep fixing the compile errors until it could build and run. but meantime please help review as well. I will graducally add comment for things needed disccusion.

[jjatie]

This always returns true and is deprecated. Should be replace by self[dataSetIndex].append(e)

[jjatie]

I just checked and this is the only use of addEntry remaining in the project

[liuxuan30]

plese go ahread update it

[jjatie]

Since large changes are being made for 4.0.0, I think we should reconsider some of these, potentially complex, side effects. We should leave them there in the deprecated old API, but I think we should recommend that they call it themselves with the new Collection API. I've found in my own experience, when working with live data, most data is around the same value and I had to remove the various calc methods that fire when modifying a dataSet to get the desired performance

[jjatie]

We should probably deprecate these removeEntry methods to encourage standard Swift Collection API. i.e.

if let index = chartData[dataSetIndex].index(of: entry) {
    chartData[dataSetIndex].remove(at: index)
}

[liuxuan30]

how about mark some 2.0/3.0 APIs to deprecate, and remove in 4.1? or remove it now?

@danielgindi idea?

[jjatie]

Then this one could be index(ofXValue:) (on ChartDataSet)

[liuxuan30]

for simple improvements, please pull this branch and just update the code, usually I should be ok with it. I will check what you updated

[jjatie]

I suppose this and the above comment were for @danielgindi . There have been disagreements in the past around API changes related to this. We didn't have the Collection API then though.

[danielgindi]

@jjatie @danielgindi could any of you can take a look at why Bar Chart all failed? I see a shift on y axis, but couldn't remember what's causing the shift:

you can see the bar chart in whole on left side is a bit lower on y-axis direction than the right one.

It seems it's because func calculateLegendOffsets

There was a fix a while back to the legend offsets. It shifted all tests too... I'm not sure how the merge did not catch this as there are snapshots involved.

[danielgindi]

I think that if this branch is tested and works fine, and the whole point about it is a bigger shift towards Swift, away from ObjC compatibility - then just go ahead, merge it.
Let people test it as a beta or an rc for a few days.

I don't see anything that breaks compatibility with the Java counterpart also. The naming is converted to swift-like namings, but the workings stays the same.

[liuxuan30]

@jjatie @danielgindi could any of you can take a look at why Bar Chart all failed? I see a shift on y axis, but couldn't remember what's causing the shift:

you can see the bar chart in whole on left side is a bit lower on y-axis direction than the right one.
It seems it's because func calculateLegendOffsets

There was a fix a while back to the legend offsets. It shifted all tests too... I'm not sure how the merge did not catch this as there are snapshots involved.

yes this has been fixed.

[liuxuan30]

I think that if this branch is tested and works fine, and the whole point about it is a bigger shift towards Swift, away from ObjC compatibility - then just go ahead, merge it.
Let people test it as a beta or an rc for a few days.

I don't see anything that breaks compatibility with the Java counterpart also. The naming is converted to swift-like namings, but the workings stays the same.

so about deprecating APIs, green light from you? @danielgindi

[danielgindi]

I think that if this branch is tested and works fine, and the whole point about it is a bigger shift towards Swift, away from ObjC compatibility - then just go ahead, merge it.
Let people test it as a beta or an rc for a few days.
I don't see anything that breaks compatibility with the Java counterpart also. The naming is converted to swift-like namings, but the workings stays the same.

so about deprecating APIs, green light from you? @danielgindi

I see breaking changes, not deprecations, maybe I missed something. Anyway - that's what major versions are for right? :-)

[liuxuan30]

I think that if this branch is tested and works fine, and the whole point about it is a bigger shift towards Swift, away from ObjC compatibility - then just go ahead, merge it.
Let people test it as a beta or an rc for a few days.
I don't see anything that breaks compatibility with the Java counterpart also. The naming is converted to swift-like namings, but the workings stays the same.

so about deprecating APIs, green light from you? @danielgindi

I see breaking changes, not deprecations, maybe I missed something. Anyway - that's what major versions are for right? :-)

    /// The number of y-values this DataSet represents
    @available(*, deprecated, message: "Use `count` instead")
    open override var entryCount: Int { return count }
    
    /// - Throws: out of bounds
    /// if `i` is out of bounds, it may throw an out-of-bounds exception
    /// - Returns: The entry object found at the given index (not x-value!)
    @available(*, deprecated, message: "Use `subscript(index:)` instead.")
    open override func entryForIndex(_ i: Int) -> ChartDataEntry?
    {
        guard i >= startIndex, i < endIndex else {
            return nil
        }
        return self[i]
    }

we have 7 APIs marked as @available(*, deprecated") in master like above. So it looks like we can remove them now. @jjatie are there any other APIs we should pay attention? like APIs should mark as deprecated or to be removed?

[jjatie]

@liuxuan30 They have been marked deprecated, but those warnings have never been released. We should keep them for now, and remove in a subsequent release.

[liuxuan30]

@jjatie @danielgindi once it passes CI, seems we can merge and go to prelease 4.0 now

[liuxuan30]

@jjatie I created the prelease of 4.0, please add any documentation https://github.com/danielgindi/Charts/releases/tag/v4.0.0 if you think it's important. Thank you for all your efforts and patience!! I know it has been so long to get this done.