Padding/spacing when rotating X-axis labels 90 degree

[thierryH91200]

This is an old bug
The problem is that the height is calculated 2 times

This is to avoid this kind of problem

Before

After

func calculateLegendOffsets 
offsetTop += xAxis.labelRotatedHeight`

and i resume

func calculateOffsets()
 let xlabelheight = xAxis.labelRotatedHeight + xAxis.yOffset
 offsetTop += xlabelheight

[liuxuan30]

eh oh, the tests failed. where 'the height is calculated 2 times'?

[thierryH91200]

Why the test fails I do not know

   internal override func calculateOffsets()
    {
        if !_customViewPortEnabled
        {
            var offsetLeft = CGFloat(0.0)
            var offsetRight = CGFloat(0.0)
            var offsetTop = CGFloat(0.0)
            var offsetBottom = CGFloat(0.0)
            
            **calculateLegendOffsets**(offsetLeft: &offsetLeft,
                                   offsetTop: &offsetTop,
                                   offsetRight: &offsetRight,
                                   offsetBottom: &offsetBottom)         <-----first time here in this function
            
            // offsets for y-labels
            if leftAxis.needsOffset
            {
                offsetLeft += leftAxis.requiredSize().width
            }
            
            if rightAxis.needsOffset
            {
                offsetRight += rightAxis.requiredSize().width
            }
            
            if xAxis.isEnabled && xAxis.isDrawLabelsEnabled
            {
                **let xlabelheight = xAxis.labelRotatedHeight + xAxis.yOffset**
                
                // offsets for x-labels
                if xAxis.labelPosition == .bottom
                {
                    **offsetBottom += xlabelheight**         <-----and still here
                }
                else if xAxis.labelPosition == .top
                {
                    offsetTop += **xlabelheight**              <-----and still here
                }
                else if xAxis.labelPosition == .bothSided
                {
                    offsetBottom += xlabelheight
                    offsetTop += xlabelheight
                }
            }
            
  .......
    }

    internal func calculateLegendOffsets(offsetLeft: inout CGFloat, offsetTop: inout CGFloat, offsetRight: inout CGFloat, offsetBottom: inout CGFloat)
    {
.......
            case .horizontal:
                
                switch _legend.verticalAlignment
                {
                case .top:
                    offsetTop += min(_legend.neededHeight, _viewPortHandler.chartHeight * _legend.maxSizePercent) + _legend.yOffset
                    if xAxis.isEnabled && xAxis.isDrawLabelsEnabled
                    {
                        **offsetTop += xAxis.labelRotatedHeight**       <-----here first time
                    }
                    
                case .bottom:
                    offsetBottom += min(_legend.neededHeight, _viewPortHandler.chartHeight * _legend.maxSizePercent) + _legend.yOffset
                    if xAxis.isEnabled && xAxis.isDrawLabelsEnabled
                    {
                        **offsetBottom += xAxis.labelRotatedHeight**      <-----here first time
                    }
                    
                    
                default:
                    break
                }
            }
        }
    }

[liuxuan30]

@thierryH91200 the image can't match, so it failed.. Not having time to look into yet

[thierryH91200]

see PhilJay/MPAndroidChart@3dfa56b
this is the same problem and soluce

[SvenMuc]

See also pull request #2214

[semireg]

👍

[jjatie]

@thierryH91200 Looks like you need to update the test. Previously the tests assumed that the double padding was correct. Since it is being compared to a snapshot, that snapshot needs to be updated accordingly.

[jjatie]

I would say this can also target 4.0.0. @liuxuan30 ?

[thierryH91200]

see PhilJay/MPAndroidChart@3dfa56b
this is the same problem and soluce

[jjatie]

@thierryH91200 I'm not saying your solution is wrong, I'm saying our tests are outdated. In order for this PR to be accepted you need to update the failing tests for the new implementation.

[liuxuan30]

I think for bug fixes, master is usually the branch to merge. We need to look at #2214 with this one, to determine if they are same/similar issues.

[liuxuan30]

@thierryH91200 are you able to check why tests fail? Do you forgot to update the test, if your commit will change some of the demo chart.
I'm triggering a new CI anyway.

[liuxuan30]

Seems we cannot get this done before 3.1. removing. However this is still a high priority fix.

[liuxuan30]

@jjatie do you think we can finish test and review this for 3.1? I checked the code change is quite small. But we need to understand the fix and test it, which takes descent time. As tomorrow is the deadline, I'm not sure if we can still solve this PR

[jjatie]

Yes, this will need to be 4.0