Allow PieChartView to hide labels for tiny slices

[jjatie]

reimplemented #2062
Comes from #944

[jjatie]

@mgod Could you take a look too?

[liuxuan30]

restarting failed builds.

[mgod]

@jjatie this implementation still has the bug described in this comment: #944 (comment)

However, I think adding this behavior to the system even with the edge cases described would be a huge improvement over the existing behavior of labels for tiny slices. I'm mostly adding the comment here for some future developer confused by one of these edge cases doesn't have to dig too far.

[liuxuan30]

@mgod thanks for pointing out
@jjatie do you think you can tackle the challenge above in time? from the comment it seems not easy. If time does not allow, let's create an issue for that and put it to backlog.

[jjatie]

This is a problem that will always have trade offs. There is no perfect solution. As well, the more we do to accommodate edge cases the less deterministic it will be for the consumer.

@liuxuan30 I won't have time to work on this one.

[mgod]

@jjatie @liuxuan30 I do feel like the original solution for #944 is better than the #2062 version, even if the measurement bug gets fixed because it's much easier to understand and debug. Basically the #944 solution lets the developer set a drawSliceTextMinimumAngle and any slice that covers less than that angle doesn't get a label. No complicated math or weird edge cases.

There are still some cases where this drawing code would leave out labels that fit in the space (slices that are small, but happen to be along the x-axis, so would fit the label) and also some cases where very long labels will extend outside their slice.

I think these edge cases are much more predictable, though, than the ones with complex measuring to see if the label fits and are also much more predictable to the developer. The very long label issue would not be a case of labels disappearing for no reason, but instead something a developer can look at and realize they should limit the length of their label text.

I'd be happy to put together a new PR with this behavior, but only if both of you feel like it would be a reasonable solution here. @danielgindi didn't feel like it was magical enough, but it's been more than a year without a solution at this point. We could set a default minimum angle of 10 or 20 or something, which would probably resolve the small slices get overlapping labels problem for most developers.

[jjatie]

@mgod I'm in support of a new proposal. I'd ask that you branch from 4.0.0 to do so.

[liuxuan30]

I'm open to a better one, if your rework could fix the issue.
However, #944 author said

I'm closing this in favor of #2062, which appears to reimplement the same improvements in Swift 3.

I assume #2062 and #944 are the same (I didn't take a closer look), but you mentioned here #944 is better, which is weird. Before you actually get started, we need to understand the gap.

[mgod]

@rofreg (the author) is my cofounder and I'd love to have him chime in if he feels like it, but, basically, both #944 and #2062 solve the problem of tiny slices getting labels, though they do it in different ways. I believe he closed #944 because #2062 still solves the tiny slices getting labels problem and it had been about a year since he'd asked for clarification on what would need to be done for #944 to be considered acceptable.

#944 adds a new CGFloat property drawXLabelsMinimumAngle and slices with smaller angles than this don't get labels. It's 0 by default so all slices get labels.

#2062 (and this PR) add a new boolean property limitXLableSizeInSlice. If true, slices where the label width is longer than the length of the arc circumscribed by the slice (which isn't where the label gets drawn, so it's not really accurate) get hidden. It's false by default.

The only reason I consider #944 to be better than #2062 is that the edge cases where labels get draw/not draw when a human would choose otherwise are easy to understand in #944 and hard to understand in #2062. I believe this would still be true in an implementation fo #2062 that measured the correct geometry to see if the label would fit and that's much, much harder to code than the current implementation here.

I don't think there's a version of this code that can get to 100% good behavior all the time, but I think either implementation will do a good job most of the time.

[jjatie]

@mgod thank you for the clarification. I agree #944 is the better approach and is easier to understand.

There are a few performance critical things I need to work on for 3.1, would you be able to provide a current implementation for this before the end of the month?

[codecov-io]

Codecov Report

Merging #3182 into master will increase coverage by 0.04%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #3182      +/-   ##
==========================================
+ Coverage   22.98%   23.03%   +0.04%     
==========================================
  Files         116      116              
  Lines       15542    15922     +380     
  Branches      272      303      +31     
==========================================
+ Hits         3573     3668      +95     
- Misses      11933    12221     +288     
+ Partials       36       33       -3

Impacted Files	Coverage Δ
Source/Charts/Renderers/PieChartRenderer.swift	0% <0%> (ø)	⬆️
Source/Charts/Charts/PieChartView.swift	0% <0%> (ø)	⬆️
Source/Charts/Highlight/CombinedHighlighter.swift	11.11% <0%> (-1.94%)	⬇️
Source/Charts/Charts/ChartViewBase.swift	21.32% <0%> (-1.33%)	⬇️
Source/Charts/Highlight/ChartHighlighter.swift	1.79% <0%> (-0.79%)	⬇️
Source/Charts/Renderers/RadarChartRenderer.swift	0% <0%> (ø)	⬆️
Source/Charts/Charts/HorizontalBarChartView.swift	0% <0%> (ø)	⬆️
Source/Charts/Highlight/PieRadarHighlighter.swift	0% <0%> (ø)	⬆️
Source/Charts/Highlight/BarHighlighter.swift	0% <0%> (ø)	⬆️
Source/Charts/Highlight/PieHighlighter.swift	0% <0%> (ø)	⬆️
... and 6 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 43ccf8e...7c10085. Read the comment docs.

[mgod]

@jjatie I should be able to.

[mgod]

I've opened up #3218 against the 4.0.0 branch. I tried setting some default minimum for the pie chart, but on playing with the charts a bit, having a behavior that removes text from the chart without specifically opting in to it felt pretty bad to me, so I left it at 0 for the minimum angle that gets labels hidden.

[liuxuan30]

what should we target this for? 4.0 or 3.1?

[liuxuan30]

Closing this as we move to #3218 for 4.0 branch