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.

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

View port handler nonnil #2980

Merged
merged 13 commits into from
Nov 27, 2017
Merged

View port handler nonnil #2980

merged 13 commits into from
Nov 27, 2017

Conversation

jjatie
Copy link
Collaborator

@jjatie jjatie commented Nov 13, 2017

No description provided.

@codecov-io
Copy link

codecov-io commented Nov 13, 2017

Codecov Report

Merging #2980 into master will decrease coverage by 0.13%.
The diff coverage is 40%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2980      +/-   ##
==========================================
- Coverage   19.77%   19.64%   -0.14%     
==========================================
  Files         113      113              
  Lines       13620    13508     -112     
==========================================
- Hits         2694     2653      -41     
+ Misses      10926    10855      -71
Impacted Files Coverage Δ
Source/Charts/Utils/ViewPortHandler.swift 26.07% <ø> (+1.26%) ⬆️
...Renderers/BarLineScatterCandleBubbleRenderer.swift 68% <ø> (ø) ⬆️
...rce/Charts/Renderers/XAxisRendererRadarChart.swift 0% <ø> (ø) ⬆️
...ce/Charts/Renderers/CandleStickChartRenderer.swift 0% <ø> (ø) ⬆️
...rts/Renderers/LineScatterCandleRadarRenderer.swift 10.52% <ø> (+1.43%) ⬆️
...rce/Charts/Renderers/YAxisRendererRadarChart.swift 0% <ø> (ø) ⬆️
Source/Charts/Renderers/PieChartRenderer.swift 0% <ø> (ø) ⬆️
Source/Charts/Renderers/LineRadarRenderer.swift 7.69% <ø> (+0.54%) ⬆️
Source/Charts/Renderers/RadarChartRenderer.swift 0% <ø> (ø) ⬆️
Source/Charts/Renderers/BubbleChartRenderer.swift 0% <0%> (ø) ⬆️
... and 15 more

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 122013e...b565773. Read the comment docs.

@liuxuan30
Copy link
Member

liuxuan30 commented Nov 15, 2017

@jjatie is helping a lot. @danielgindi do you want to take these similar PRs about swift improvements?

@jjatie jjatie mentioned this pull request Nov 20, 2017
@jjatie
Copy link
Collaborator Author

jjatie commented Nov 23, 2017

@liuxuan30 Now that you've looked at the other related PRs, is it okay for this to be merged?

@jjatie jjatie mentioned this pull request Nov 23, 2017
@liuxuan30
Copy link
Member

I‘m checking every day but can't guarantee I can review daily; I would try. Reviewing slowly right now, but I can get some merged for sure.

@liuxuan30 liuxuan30 self-requested a review November 24, 2017 06:20
@liuxuan30
Copy link
Member

I suggest in the future, similar improvements like remove optional can be squash into one PR, each commit could be one class or one topic to review, to reduce dependency and conflicts and makes it easier for you.

@jjatie
Copy link
Collaborator Author

jjatie commented Nov 24, 2017

Fixed

@@ -221,8 +221,7 @@ open class ChartViewBase: NSUIView, ChartDataProvider, AnimatorDelegate
_animator = Animator()
_animator.delegate = self

_viewPortHandler = ViewPortHandler()
_viewPortHandler.setChartDimens(width: bounds.size.width, height: bounds.size.height)
_viewPortHandler = ViewPortHandler(width: bounds.size.width, height: bounds.size.height)
Copy link
Member

Choose a reason for hiding this comment

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

I think in some cases, setChartDimens() is useful rather than re-init again.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In some cases it is, that is why the method still exists. In this case it is not useful, as the only thing that is happening is what already happens in the initializer I changed it to.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the initialize method, so it does need to be inited here.

@@ -12,21 +12,23 @@
import Foundation
import CoreGraphics

//@objc(ChartRenderer)
Copy link
Member

Choose a reason for hiding this comment

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

what is this, commented out code or documentation? If it's doc, maybe use doc style comment like /* or other

Copy link
Collaborator Author

@jjatie jjatie Nov 27, 2017

Choose a reason for hiding this comment

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

That shouldn't be here at all. I was experimenting, but ultimately moved it all to another branch/PR. I missed this in the cleanup

let viewPortHandler = self.viewPortHandler
else { return }


Copy link
Member

Choose a reason for hiding this comment

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

empty line? Not sure if this is github display issue, but if old guard is deleted, no space line needed. (ignore if there is no empty line)

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'll fix

@liuxuan30
Copy link
Member

generally it's good, but I need to take a look at viewport handler myself if nil is fine.

Removed extraneous comment
Removed extra whitespace
@jjatie
Copy link
Collaborator Author

jjatie commented Nov 27, 2017

Fixed based on comments

viewPortHandler = self.viewPortHandler
else { return }


Copy link
Member

Choose a reason for hiding this comment

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

empty line?

let viewPortHandler = self.viewPortHandler
else { return }


Copy link
Member

Choose a reason for hiding this comment

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

empty line?

let viewPortHandler = self.viewPortHandler
else { return }


Copy link
Member

Choose a reason for hiding this comment

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

empty line?

let viewPortHandler = self.viewPortHandler
else { return }


Copy link
Member

Choose a reason for hiding this comment

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

empty line?

{
}


Copy link
Member

Choose a reason for hiding this comment

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

empty line?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This one should be here. The rest are now fixed

viewPortHandler = self.viewPortHandler
else { return }


Copy link
Member

Choose a reason for hiding this comment

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

empty line?

@liuxuan30
Copy link
Member

Finished reviewing. ChartViewBase guaranteed viewPortHandler will be initialized anyway, so renderer's subclass will always have viewPortHandler. Thus removing optional ? and ! seems reasonable.

@liuxuan30 liuxuan30 merged commit fb02977 into ChartsOrg:master Nov 27, 2017
@jjatie jjatie deleted the view-port-handler-nonnil branch November 27, 2017 05:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants