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

Bug fixing with one line, updating ChartViewBase.swift #2355

Merged
merged 2 commits into from
Apr 24, 2017
Merged

Bug fixing with one line, updating ChartViewBase.swift #2355

merged 2 commits into from
Apr 24, 2017

Conversation

Eric0625
Copy link
Contributor

fixing a bug: when clearing chart, the 'lastHighlighted' var isn't set to nil. This will cause user to tap twice to highlight the new value, if the new value's index is the same as the old lastHighlighted's.

fixing a bug: when clearing chart, the 'lastHighlighted' var isn't set to nil. This will cause user to tap twice to highlight the new value, if the new value's index is the same as the old 'lastHighlighted' var's.
@liuxuan30
Copy link
Member

do you mean you called clear() first, and then highlight? What's the context for clearing? Have you tried just set the new chart data directly?
First time I just clear() today :)

@Eric0625
Copy link
Contributor Author

I tried to highlight the first bar in my barChart, then I press a button to remove old data and fill in new ones, but I find out you need to tap twice to highlight the first bar of new data.

And I don't know how to "set the new chart data directly" without calling clear().

open func clear()
    {
        _data = nil
        _offsetsCalculated = false
        _indicesToHighlight.removeAll()
        setNeedsDisplay()
    }

Function clear() first sets _data to nil, IMO that sort of means "set the new chart data directly". Then it called _indicesToHighlight.removeAll(), which deletes all highlighted slices in the chart, so what's the meaning to keep the lastHighlighted variable? It's now representing a non-exist highlighted object.

@liuxuan30
Copy link
Member

What I mean is just call chartView.data = newData to update your entire chart data without calling clear()

@Eric0625
Copy link
Contributor Author

That's exactly what I did when using the chart for the first time: only set chartView.data with new data. Then the app behaved like this:

This was the initial status: I clicked the first bar
2017-04-20 11 33 12

Then I hit the button on top of it to fill the chart with new data, but you can see the first bar of new data is automatically selected, which is definitely not what I want:
2017-04-20 11 33 49

I've tried several times but it looks like that only to both call chartView.lastHighlighted=nil and _indicesToHighlight.removeAll() can you get the fully cleaned chart.

@liuxuan30
Copy link
Member

liuxuan30 commented Apr 20, 2017

I see. The lastHighlighted is reserved by default, which seems by design. I think some users will prefer keep it while some don't, so seems like we should have on/off switch, not just simply clear it.

If you can add a switch for it, I think I can manage to merge this PR, or I will do it later.

@Eric0625
Copy link
Contributor Author

Ok, I'll try it when I have time

 it controls whether to clear the lasthighlighted value or not
@liuxuan30
Copy link
Member

looks good to me. we will try fix travis first, and later handle some simple PRs, thanks for you patience.

@codecov-io
Copy link

codecov-io commented Apr 24, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@f23e872). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2355   +/-   ##
=========================================
  Coverage          ?   19.66%           
=========================================
  Files             ?      112           
  Lines             ?    13713           
  Branches          ?        0           
=========================================
  Hits              ?     2696           
  Misses            ?    11017           
  Partials          ?        0
Impacted Files Coverage Δ
Source/Charts/Charts/ChartViewBase.swift 24.28% <0%> (ø)

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 f23e872...3b9e7f4. Read the comment docs.

@liuxuan30 liuxuan30 merged commit cddf136 into ChartsOrg:master Apr 24, 2017
Neral pushed a commit to Neral/Charts that referenced this pull request Apr 24, 2017
…l/update_upstream

* commit '04c5820a6cca34b98cb48b8baab1dd44f44e857d': (61 commits)
  add switch for clearing `lastHighlighted` in clear() (ChartsOrg#2355)
  update schemes, move script to file like ChartsRealm
  use name instead of uuid, add run script for copying missing framework for ChartsTests
  1. use xcode 8.3 image 2. use Apple TV 1080p (10.2) and iPhone 7 (10.3) device id in Travis CI
  Fix Simple Bar Chart Demo, switch use of x and y values (ChartsOrg#2365)
  Converted swift 3.0 DBL_MIN leftover
  Removed leftover script from the combined Realm era
  Removed leftover scheme
  v3.0.2
  Removed unrequited script
  gitignore updates
  Added @discardableResult to silence warnings when it’s safe to ignore result
  Moved Realm stuff to https://github.com/danielgindi/ChartsRealm
  Remove line width minimum constraint
  Updated build-dependencies.sh
  loosen realm version requirement
  fix Xcode 8.3 compiler warnings
  fix ChartsOrg#2222 move default backgroundColor to initialize() as initWithCoder also needs it
  Updated to use Realm version 2.4.3
  Consider _yAxis.isDrawLimitLinesBehindDataEnabled for radar chart
  ...

# Conflicts:
#	Source/ChartsRealm/Data/RealmBarDataSet.swift
Neral pushed a commit to Neral/Charts that referenced this pull request Apr 24, 2017
…l/update_upstream

* commit '04c5820a6cca34b98cb48b8baab1dd44f44e857d': (61 commits)
  add switch for clearing `lastHighlighted` in clear() (ChartsOrg#2355)
  update schemes, move script to file like ChartsRealm
  use name instead of uuid, add run script for copying missing framework for ChartsTests
  1. use xcode 8.3 image 2. use Apple TV 1080p (10.2) and iPhone 7 (10.3) device id in Travis CI
  Fix Simple Bar Chart Demo, switch use of x and y values (ChartsOrg#2365)
  Converted swift 3.0 DBL_MIN leftover
  Removed leftover script from the combined Realm era
  Removed leftover scheme
  v3.0.2
  Removed unrequited script
  gitignore updates
  Added @discardableResult to silence warnings when it’s safe to ignore result
  Moved Realm stuff to https://github.com/danielgindi/ChartsRealm
  Remove line width minimum constraint
  Updated build-dependencies.sh
  loosen realm version requirement
  fix Xcode 8.3 compiler warnings
  fix ChartsOrg#2222 move default backgroundColor to initialize() as initWithCoder also needs it
  Updated to use Realm version 2.4.3
  Consider _yAxis.isDrawLimitLinesBehindDataEnabled for radar chart
  ...

# Conflicts:
#	Source/ChartsRealm/Data/RealmBarDataSet.swift
PeterSrost pushed a commit to sokol8/Charts that referenced this pull request Oct 31, 2018
add switch for clearing `lastHighlighted` in clear()

fixing a bug: when clearing chart, the 'lastHighlighted' var isn't set to nil. This will cause user to tap twice to highlight the new value, if the new value's index is the same as the old 'lastHighlighted' var's.

* adding a switch

 it controls whether to clear the lasthighlighted value or not
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