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

Removed redundant lastHighlighted = nil #3178

Closed
wants to merge 1 commit into from
Closed

Removed redundant lastHighlighted = nil #3178

wants to merge 1 commit into from

Conversation

jjatie
Copy link
Collaborator

@jjatie jjatie commented Jan 13, 2018

highlightValue already takes care of it.

@codecov-io
Copy link

Codecov Report

Merging #3178 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3178      +/-   ##
==========================================
+ Coverage   22.99%   22.99%   +<.01%     
==========================================
  Files         116      116              
  Lines       15541    15540       -1     
  Branches      272      272              
==========================================
  Hits         3573     3573              
+ Misses      11932    11931       -1     
  Partials       36       36
Impacted Files Coverage Δ
Source/Charts/Charts/BarLineChartViewBase.swift 22.58% <ø> (+0.01%) ⬆️

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 0011c35...12f3978. Read the comment docs.

@jjatie jjatie requested a review from liuxuan30 January 14, 2018 04:23
@jjatie jjatie added this to the 3.1 milestone Jan 15, 2018
@liuxuan30
Copy link
Member

I only see highlightValues(_ highs: [Highlight]?) nil out lastHighlighted, but highlightValue does not.
There used to be a PR in func clear() specifically set lastHighlighted to nil because some small bugs, not cleaning lastHighlighted state and causes wrong highlight.
I think we need a deeper look to make sure this PR is safe.

@liuxuan30
Copy link
Member

@jjatie where you say lastHighlighted = nil is handled?

@jjatie
Copy link
Collaborator Author

jjatie commented Jan 23, 2018

ChartViewBase line 483

@jjatie
Copy link
Collaborator Author

jjatie commented Jan 23, 2018

This doesn't actually solve the bug the original author intended. I feel like highlighting is messy right now, and we should reevaluate it in the future. We probably can just close this PR, but I'll let you decide.

@liuxuan30
Copy link
Member

liuxuan30 commented Jan 23, 2018

ah I see. right a little messy. But why put lastHighlighted = h outside..
Why not just put both of them in
@objc open func highlightValue(_ highlight: Highlight?, callDelegate: Bool) with one line code?

@jjatie jjatie removed this from the 3.1 milestone Jan 23, 2018
@jjatie
Copy link
Collaborator Author

jjatie commented Jan 23, 2018

I didn't want to make the change without evaluating the impact everywhere. I'm going to close this, and reevaluate highlighting at a later time.

@jjatie jjatie closed this Jan 23, 2018
@jjatie jjatie deleted the redundant-highlight-nil branch January 23, 2018 10:16
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