ChartViewBase cleanup #3034

jjatie · 2017-11-23T00:39:28Z

For the most part, condensing logic and using guard where appropriate
Removed optionality of many internal variables as they were only optional to allow for deferred initialization. This is now replaced with lazy vars.
Removed empty initializer overrides.
fileprivate is now private

codecov-io · 2017-11-23T01:03:02Z

Codecov Report

Merging #3034 into 4.0.0 will decrease coverage by 0.01%.
The diff coverage is 27.69%.

@@            Coverage Diff            @@
##           4.0.0    #3034      +/-   ##
=========================================
- Coverage   20.9%   20.88%   -0.02%     
=========================================
  Files        111      111              
  Lines      15458    15390      -68     
  Branches     250      251       +1     
=========================================
- Hits        3231     3214      -17     
+ Misses     12187    12136      -51     
  Partials      40       40

Impacted Files	Coverage Δ
Source/Charts/Charts/RadarChartView.swift	`0% <0%> (ø)`	⬆️
Source/Charts/Charts/PieRadarChartViewBase.swift	`0% <0%> (ø)`	⬆️
Source/Charts/Charts/BarLineChartViewBase.swift	`22.52% <100%> (-0.04%)`	⬇️
Source/Charts/Charts/ChartViewBase.swift	`21.81% <27.11%> (-0.33%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e126844...c35d469. Read the comment docs.

liuxuan30 · 2017-12-18T11:42:25Z

Source/Charts/Charts/ChartViewBase.swift


    /// This property is deprecated - Use `chartDescription.text` instead.
    @objc @available(*, deprecated: 1.0, message: "Use `chartDescription.text` instead.")
    open var descriptionText: String
    {
-        get { return chartDescription?.text ?? "" }
-        set { chartDescription?.text = newValue }
+        get { return chartDescription.text ?? "" }


it seems we have a future conflict that these will be removed, but we change it now?
I'm not sure if we should merge #2996 into master now or for 4.0. I don't think it's worthy to refactor what will be removed

It’s not a big deal though, and I will likely be the one handling merge conflicts. In this case, these deprecations need to be changed in order for the project to build.

I think we should merge into master, and I’ll handle any merge conflicts when we merge into 4.0 in the future.

I have merged #2996 to make things easier.
btw, I need to think about if changing to lazy fits. something like description or empty texts should be released if not used later, not retaining all the time (though memory pressure is no longer a thing, but we should keep a good habit?)

Conflicts are resolved

liuxuan30 · 2017-12-18T11:51:01Z

Source/Charts/Charts/ChartViewBase.swift

@@ -500,8 +463,7 @@ open class ChartViewBase: NSUIView, ChartDataProvider, AnimatorDelegate
    /// - parameter callDelegate: Should the delegate be called for this change
    @objc open func highlightValue(x: Double, y: Double, dataSetIndex: Int, callDelegate: Bool)
    {
-        guard let data = _data else
-        {
+        guard let data = _data else {


code style is make { a separate line

liuxuan30 · 2017-12-18T11:51:32Z

Source/Charts/Charts/ChartViewBase.swift

@@ -443,19 +410,15 @@ open class ChartViewBase: NSUIView, ChartDataProvider, AnimatorDelegate
    /// null or an empty array to undo all highlighting. 
    /// This should be used to programmatically highlight values.
    /// This method *will not* call the delegate.
-    @objc open func highlightValues(_ highs: [Highlight]?)
+    @objc open func highlightValues(_ highs: [Highlight])


I remember highlight objects are allowed to be nil

liuxuan30 · 2017-12-22T02:54:09Z

Source/Charts/Charts/ChartViewBase.swift

-        }
+        _indicesToHighlight = highs ?? []
+
+        lastHighlighted = _indicesToHighlight.isEmpty


one line is enough for ?: (code style here just uses one line)

liuxuan30 · 2017-12-22T03:01:36Z

Source/Charts/Charts/ChartViewBase.swift

-            if entry == nil
-            {
-                h = nil
+        var high = highlight


I'm feeling this rewrite makes harder to understand the logic (the guard else{} part). If we cannot find a better one, I suggest add simple comment here.

liuxuan30 · 2017-12-22T03:03:13Z

Source/Charts/Charts/ChartViewBase.swift

@@ -519,7 +480,7 @@ open class ChartViewBase: NSUIView, ChartDataProvider, AnimatorDelegate
    /// CandleStick-Chart.
    @objc open func getHighlightByTouchPoint(_ pt: CGPoint) -> Highlight?
    {
-        if _data === nil
+        guard _data != nil else


Not feeling guard is better here.

The convention is as the Swift Book states:

You use a guard statement to require that a condition must be true in order for the code after the guard statement to be executed.

While technically it doesn't matter if we use an if or a guard, the guard does make it clear that we need a condition to be true to continue.

Yeah I agree with @jjatie on this one - we are used to C-style languages, while Swift does not aim at being one... For Swift it's more natural to put a guard

liuxuan30 · 2017-12-22T03:05:38Z

Source/Charts/Charts/ChartViewBase.swift

-            {
-                continue
-            }
+            guard entryIndex <= Int(Double(set.entryCount) * _animator.phaseX) else { continue }


is guard in swift as fast as if, in terms of assembling code? I'm feeling we are using lots of guard, when it just means if condition. If guard is slower than if, I would rather not to use too many.

They compile the same. I think you're just not used to seeing guard yet since it isn't in objc.

The convention is as the Swift Book states:

You use a guard statement to require that a condition must be true in order for the code after the guard statement to be executed.

While technically it doesn't matter if we use an if or a guard, the guard does make it clear that we need a condition to be true to continue.

Most of what you know as "optionals", and stuff that work with optionals, and guards and stuff - are compile time. You want to be sure that something is not null and unwrap it, and unwrapping does not mean that you have more code there - it just means that the compiler trusts the non-null object and allows you to refer to it without the ? operator

When I have time, I would create a sample app and check what guard and if implementation looks like in asm code. Ideally should not "that" impact the performance. I was just obsessed a little recently about the asm code 😂

liuxuan30 · 2017-12-22T03:10:32Z

Source/Charts/Charts/ChartViewBase.swift

-            {
-                val = 0.999
+            switch newValue {
+            case ..<0.0: _dragDecelerationFrictionCoef = 0


where can I find this magic definition.. I just can find things like case 1..<5: in the manual.

One Sided Ranges are new in Swift 4.

liuxuan30 · 2017-12-22T03:19:32Z

Source/Charts/Charts/ChartViewBase.swift

        #endif

-        _animator = Animator()


so if we uses lazy, how do we override the property in subclass to a new class object? e.g some users may write their own animator class, or XAxis class, and just override initialize() to replace them.

They are still able to do this exactly the same as they have before.

Then we should add some comments/notes that people should override initialize() to replace the objects with their classes?

Why? We never did before. As far as the consumer is concerned, nothing has changed other than it is no longer an implicitly unwrapped optional. Swift initialization already works that way where you must override the initializer to provide a different default value for a stored property.

* fix #1830. credit from #2049 * add combined chart unit tests for iOS, tvOS (macOS only have build process) * use iterater rather than index

Fixed using wrong axis (Issue #2257)

* Removed redundant ivars in favour of proper access control * Moved initialization of axes to their declaration to keep the same optionality exposed.

* Replaced relevant `ChartUtils` methods with `Double` extensions Improves readability. `nextUp` is built in and provides the same functionality. * Updated `ChartUtilsTests` to match changes

* add option to build demo projects unit tests on iOS * add ChartsDemo-OSX build test.

liuxuan30 · 2017-12-28T02:36:00Z

@jjatie I'm confused. We have a card for PRs merged to master, and 4.0 card merge to 4.0 branch. You moved most of them from master to 4.0 card, shall we change the branch?

jjatie · 2017-12-28T02:37:20Z

Yep! I missed a couple I guess. Things will be easier if I just move them to 4.0 and they aren't adding/changing functionality so they can wait until 4.0

liuxuan30 · 2017-12-28T02:48:02Z

alright, one last comment. do you think any chance anyone would nil out some of those old optionals? Like legend.
Corner case: Someone displays legend and later want to remove it, rather than disable, he would like to nil legend out (like to save memory)

jjatie · 2017-12-28T02:58:13Z

You're looking at the internal properties. The consumer facing properties were already non-optional.

liuxuan30 · 2017-12-28T12:51:29Z

Let's try the new tests then. looks good to me

liuxuan30 · 2017-12-28T12:52:11Z

Source/Charts/Charts/ChartViewBase.swift


    /// object responsible for animations
-    internal var _animator: Animator!
+    internal lazy var _animator: Animator = {


code style for blocks? { another line?

I guess so?

liuxuan30 · 2017-12-28T12:52:58Z

Source/Charts/Charts/ChartViewBase.swift

-
-        attrs[NSAttributedStringKey.font] = description.font
-        attrs[NSAttributedStringKey.foregroundColor] = description.textColor
+        let attrs: [NSAttributedStringKey : Any] = [


how about [

Looking through the rest of library, [ and ( seems to be on the same line.

liuxuan30 · 2017-12-28T12:53:48Z

Source/Charts/Charts/ChartViewBase.swift

-                h = nil
+        var high = highlight
+        guard
+            let h = high,


can we use single let here

Unfortunately, no. I tried, but couldn't find a way because we need to assign the original highlight.

liuxuan30 · 2017-12-28T12:53:59Z

Source/Charts/Charts/ChartViewBase.swift

+        guard
+            let h = high,
+            let entry = _data?.entryForHighlight(h)
+            else {


sorry about missing these code style, { another line

I thought we agreed this was the style for multiline guard statements?

oh is it? sorry don't remember.

liuxuan30 · 2017-12-28T12:54:09Z

Source/Charts/Charts/ChartViewBase.swift

-            {
-                _indicesToHighlight = [h!]
-            }
+                if callDelegate {


sorry about missing these code style, { another line

liuxuan30 · 2017-12-28T12:54:15Z

Source/Charts/Charts/ChartViewBase.swift

+        // set the indices to highlight
+       _indicesToHighlight = [h]
+
+        if callDelegate {


sorry about missing these code style, { another line

liuxuan30 · 2017-12-28T12:54:38Z

Source/Charts/Charts/ChartViewBase.swift

-            if val < 0.0
-            {
-                val = 0.0
+            switch newValue {


{ another line or?

I guess so. Seems weird to me on switch statements.

liuxuan30 · 2017-12-28T13:08:52Z

@jjatie looks like these commits are no with latest rake file. Feel free to just test on your side or merge master(like practice rebase)

jjatie · 2017-12-28T13:11:37Z

Oops! Fixing now.

liuxuan30 · 2017-12-29T04:46:12Z

any idea why the UT is not latest?

jjatie · 2017-12-29T12:48:30Z

4.0.0 is out of sync with master

fix a small bug

liuxuan30 · 2018-01-02T08:21:29Z

please merge and rebase on master, once the CI passed, this can be merged I think.

For the most part, condensing logic and using `guard` where appropriate Removed optionality of many internal variables as they were only optional to allow for deferred initialization. This is now replaced with lazy vars. Removed empty initializer overrides. `fileprivate` is now `private`

* Renamed `IMarker` to `Marker` following Swift API guidelines. * Renamed `IAxisValueFormatter` to `AxisValueFormatter` * Renamed `IFillFormatter` to `FillFormatter` * Renamed `IValueFormatter` to `ValueFormatter` * Renamed `IHighlighter` to `Highlighter` * Renamed `I*DataSet` to `*DataSetProtocol` to follow Swift API guidelines * Fixed naming of `LineRadarChartDataSetProtocol` and `RadarChartDataSetProtocol` from previous commit * Renamed "Interfaces" to "DataProviders" for clarity * Updated Demos to for new type naming

* Renderer is now a protocol Renamed Renderers, and organized the Renderer folder. * DataRenderer is now a protocol * AxisRenderer is now a protocol

jjatie · 2018-01-03T02:31:03Z

I still don't understand how you want me to rebase 4.0.0 branch since I can't push it up without review. I rebased this PR, but obviously 4.0.0 has not been rebased.

Also, I noticed that when I rebased from master, the merging took many old commits that I had to manually correct whereas pulling master did not. Very frustrating.

liuxuan30 · 2018-01-03T05:36:17Z

Oh I see. If you see the old conflicts you already resolved but once again appear when you rebase, you are on the "right rebase way" I'm talking about, and yes, it's frustrating.

What I meant in the other thread is, I hope you no longer need to have those "merging from upstream master..." commits in your commit log by using rebase.

When you rebase, the old conflicts you resolved will jump again, and if it's easy to fix again, just continue. If it's a lot of burden, let's simply pull master, and then we squash the commits when we want to merge the pull requests on github page.

The whole point using rebase with this project is to squash the commits into one or remove useless commits before filing the PR or updating the PR. The purpose is to make the project commit logs simple and easy to search.

Hope this clarify? For this PR, I saw conflicts now, you can just revert back to the commit you think is all good and pull master then, let CI pass and click "squash and merge", don't need to rebase then.

jjatie · 2018-01-03T10:23:04Z

You can't undo a rebase like this.

I figured out what my problem is with the rebasing in this case. You can't rebase from two different branches at the same time which is what we need to do for PRs going into 4.0.0. You can either rebase from master OR 4.0.0, not both. In order for rebasing to work here, 4.0.0 needs to be regularly rebased from master, and then PRs for 4.0.0 need to rebase off the freshly rebased 4.0.0. The problem with this is that there is no way rebase 4.0.0 without review.

Most guides will tell you that you should never rebase a branch someone else might touch, which in this case is the 4.0.0 branch. While I disagree with rebasing in general because you lose history, I'm okay doing it for PR branches. If we are going to rebase, all PRs should be rebasing from the branch they are PRing into. I'll create an issue about this and we can update the docs from there for future PRs to be accepted.

This PR is garbage now, but luckily #3045 includes all the changes I made here. I will update that PR with the current 4.0.0. They should have been the same PR in the first place, but I wasn't sure what changes might be accepted at the time I made them.

liuxuan30 · 2018-01-04T01:58:40Z

You can't undo a rebase like this.

to clarify, can't rebase abort action plus git reset --harddo the job?

jjatie · 2018-01-04T08:25:35Z

Not when you need to rebase from multiple branches

jjatie mentioned this pull request Nov 23, 2017

Chartviewbase redundant ivar #3045

Merged

liuxuan30 mentioned this pull request Dec 18, 2017

Removed methods and properties deprecated in 1.0 #2996

Merged

liuxuan30 reviewed Dec 18, 2017

View reviewed changes

liuxuan30 self-assigned this Dec 18, 2017

Fixed using wrong axis (Issue #2257)

88f1811

liuxuan30 mentioned this pull request Dec 19, 2017

Replaced backing variables with proper access control #3104

Closed

liuxuan30 reviewed Dec 22, 2017

View reviewed changes

liuxuan30 and others added 4 commits December 25, 2017 09:10

fix #1830. credit from #2049 (#2874)

7238ba9

* fix #1830. credit from #2049 * add combined chart unit tests for iOS, tvOS (macOS only have build process) * use iterater rather than index

Merge pull request #3114 from defranke/HorizontalBarChart-Axis

64bc2d1

Fixed using wrong axis (Issue #2257)

Removed redundant ivars in BarLineChartViewBase (#3043)

c8ae3ea

* Removed redundant ivars in favour of proper access control * Moved initialization of axes to their declaration to keep the same optionality exposed.

Update 4.0.0 with master (#3135)

bbeb88d

jjatie and others added 2 commits December 27, 2017 21:29

Replaced relevant ChartUtils methods with Double extensions (#2994)

1658a63

* Replaced relevant `ChartUtils` methods with `Double` extensions Improves readability. `nextUp` is built in and provides the same functionality. * Updated `ChartUtilsTests` to match changes

add option to build demo projects unit tests on iOS (#3121)

48ac6a6

* add option to build demo projects unit tests on iOS * add ChartsDemo-OSX build test.

jjatie changed the base branch from master to 4.0.0 December 28, 2017 02:37

liuxuan30 reviewed Dec 28, 2017

View reviewed changes

Update ViewPortHandler.swift (#3143)

9706fa1

fix a small bug

jjatie and others added 8 commits January 2, 2018 22:00

Pulled latest master

386d357

Fixes for PR

8e53b3a

Changes for PR review.

27caae6

Renderer protocols (#3136)

9503634

* Renderer is now a protocol Renamed Renderers, and organized the Renderer folder. * DataRenderer is now a protocol * AxisRenderer is now a protocol

Code style fixes

d0643a3

Code style fixes

8e67c9e

jjatie closed this Jan 3, 2018

jjatie deleted the chartviewbase-cleanup branch January 6, 2018 10:48

jjatie mentioned this pull request Jan 6, 2018

Minor cleanup to Highlighter types #3003

Merged

ChartViewBase cleanup #3034

ChartViewBase cleanup #3034

Conversation

jjatie commented Nov 23, 2017

codecov-io commented Nov 23, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liuxuan30 Dec 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjatie Dec 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjatie Dec 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjatie Dec 28, 2017 • edited Loading

Choose a reason for hiding this comment

liuxuan30 commented Dec 28, 2017

jjatie commented Dec 28, 2017

liuxuan30 commented Dec 28, 2017

jjatie commented Dec 28, 2017

liuxuan30 commented Dec 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liuxuan30 commented Dec 28, 2017

jjatie commented Dec 28, 2017

liuxuan30 commented Dec 29, 2017

jjatie commented Dec 29, 2017

liuxuan30 commented Jan 2, 2018

jjatie commented Jan 3, 2018

liuxuan30 commented Jan 3, 2018 • edited Loading

jjatie commented Jan 3, 2018

liuxuan30 commented Jan 4, 2018 • edited Loading

jjatie commented Jan 4, 2018

codecov-io commented Nov 23, 2017 •

edited

Loading

liuxuan30 Dec 18, 2017 •

edited

Loading

jjatie Dec 24, 2017 •

edited

Loading

jjatie Dec 24, 2017 •

edited

Loading

jjatie Dec 28, 2017 •

edited

Loading

liuxuan30 commented Jan 3, 2018 •

edited

Loading

liuxuan30 commented Jan 4, 2018 •

edited

Loading