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

Bugfix for fix #1488, #1564 #1565

Merged
merged 2 commits into from
Sep 29, 2016
Merged

Bugfix for fix #1488, #1564 #1565

merged 2 commits into from
Sep 29, 2016

Conversation

liuxuan30
Copy link
Member

@liuxuan30 liuxuan30 commented Sep 27, 2016

Attempt to fix #1488, #1564
@danielgindi I need you carefully review this and give details about existing implementation about centerAxisLabels feature. My changes may lack of information and cause more issues, though I have tesed it in time line chart, bar chart, combined chart, multiple bar chart, line chart.

  1. I removed the dependency of getting initial n value, because first and last will be equal after zooming a lot
  2. I change centerAxisLabelsEnabled condition, if entry count > 0, we could turn on centerAxisLabel. I need context why it's > 1.
  3. I removed using local variable centeringEnabled, because axis.entries would be changed but centeringEnabled keeps reading old entryCount, which seems a dirty read issue
  4. I use offset: Double = interval / 2.0 instead of offset = (axis.entries[1] - axis.entries[0]) / 2.0, because axis.entries[1] - axis.entries[0] always equals to interval, and when there is only one entry, we still need a valid offset instead of 0

1. use axis.centerAxisLabelsEnabled instead of local variable to avoid dirty read
2. fix xAxis label missing issue
… will have missing x axis labels after zoom in)
@liuxuan30
Copy link
Member Author

@petester42
seeing

*** Building scheme "Realm" in Realm.xcworkspace

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself

But no more logs.. Have you met this?

@pmairoldi
Copy link
Collaborator

Ya realm takes too long to build on ci. Usually it's not a problem because the download limit is not reached. Just restart the build.

@danielgindi
Copy link
Collaborator

danielgindi commented Sep 29, 2016

@PhilJay Looks good to me, right?

Well except that var n should be axis.centerAxisLabelsEnabled ? 1 : 0, not 1. Otherwise it misplaces the labels
Also the interval thing should stay as it is - as the increment is not always interval

@danielgindi danielgindi merged commit 5170d18 into ChartsOrg:master Sep 29, 2016
@danielgindi
Copy link
Collaborator

Okay I've done some adjustments and merged

@liuxuan30 liuxuan30 deleted the bugfix branch October 8, 2016 01:12
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.

(v3.0 + Swift3.0) Time Line Chart Demo crash
3 participants