-
-
Notifications
You must be signed in to change notification settings - Fork 6k
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
Minor changes to logic in ViewPortJob
subclasses.
#2992
Minor changes to logic in ViewPortJob
subclasses.
#2992
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2992 +/- ##
==========================================
+ Coverage 19.64% 19.77% +0.13%
==========================================
Files 113 113
Lines 13792 13663 -129
==========================================
- Hits 2709 2702 -7
+ Misses 11083 10961 -122
Continue to review full report at Codecov.
|
Source/Charts/Jobs/ViewPortJob.swift
Outdated
@@ -17,11 +17,11 @@ import CoreGraphics | |||
open class ViewPortJob: NSObject | |||
{ | |||
@objc internal var point: CGPoint = CGPoint() | |||
@objc internal weak var viewPortHandler: ViewPortHandler? | |||
@objc internal weak var viewPortHandler: ViewPortHandler? // TODO: Can this be unowned? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
before merge, do we need to finish TODO? I am willing to check if we can use unowned
when I got time, but seems weak is totally fine here. What's your concern using weak?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weak
does work here, but I think what is actually intended is unowned
. In terms of function, nothing would change, but it does clean up a lot of guard
statements if we can use unowned
I feel like that is a separate PR and one that requires a bit more digging. I'm not sure what the policy is on TODOs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know the policy neither. but if it is another PR, could you remove the comment? I will merge later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Generally this PR looks great to use |
No description provided.