[Feature] added ability to rotate ChartLimitLine label

[berbaspin]

Goals :soccer:

Added labelRotationAngle to ChartLimitLine

Testing Details :mag:

Tested on line chart and horizontal bar chart both XAxis and YAxis and all four label positions.

[berbaspin]

@pmairoldi, @FelixHerrmann, @liuxuan30, could I get a review of this PR?

[liuxuan30]

@berbaspin could you share a screenshot, what this can achieve? how 'configurable' the angle can be? Have you tested it thoroughfuly?

btw if the angle is fully configurable, I would hope we could add a UT for this feature? like define 0/30/45/60/90/180 degree rotation for one chart(e.g. line chart) so we make sure this is robust.

[berbaspin]

@liuxuan30 previously, we had a functionality that allowed us to rotate the xAxis label. I added the same functionality for Limit Line. As example: 90 degrees.

But also we can use non-standard angle. Examples: 17 and 285 degrees

We can rotate limit line without xAxis label rotation.

I tested this on Bar Chart, Line Chart and Horizontal Chart. For all types of label position (leftTop, leftBottom, rightTop, rightBottom). Examples:

Yeah, I also can write a UT for this feature. Will it be enough to write 3 tests with different angles?

   func testLimitLineLabelRotatedBy90() {
        let limitline = ChartLimitLine(limit: 50, label: "Limit Line")
        limitline.labelPosition = .rightTop
        limitline.labelRotationAngle = 90
        chart.leftAxis.addLimitLine(limitline)
        assertChartSnapshot(matching: chart)
    }

    func testLimitLineLabelRotatedBy30() {
        let limitline = ChartLimitLine(limit: 50, label: "Limit Line")
        limitline.labelPosition = .leftBottom
        limitline.labelRotationAngle = 30
        chart.leftAxis.addLimitLine(limitline)
        assertChartSnapshot(matching: chart)
    }

    func testLimitLineLabelDefaultRotation() {
        let limitline = ChartLimitLine(limit: 50, label: "Limit Line")
        limitline.labelPosition = .rightBottom
        chart.leftAxis.addLimitLine(limitline)
        assertChartSnapshot(matching: chart)
    }

[liuxuan30]

previously, we had a functionality that allowed us to rotate the xAxis label

So we are just adding limitline label rotation, no xAxis label rotation, right?

What's the center of the rotated limitline label? e.g. where's the center of the limitline label in different angel? Is the center same as non rotated position?

[liuxuan30]

Yeah, I also can write a UT for this feature. Will it be enough to write 3 tests with different angles?

Yeah, I suggest we add 30/45/60/90/120/180 to cover the typical angles

[berbaspin]

So we are just adding limitline label rotation, no xAxis label rotation, right?

@liuxuan30, yes. My pull request contains only limitline label rotation. I used rotation anchor - CGPoint(x: 0.0, y: 0.0). It allows to be accurate in calculations and not cross the limit line with the label regardless of the angle of rotation. Examples are below. I also compared the limit line label rotation with xAxis labels rotations and they looks the same.

Example of rotation by 3 angles (0, 45, 90) of horizontal limit line.

Example of rotation by 3 angles (45, 90, 180) of vertical limit line.

Example of rotation by 120 degrees with xAxis.

[berbaspin]

Yeah, I suggest we add 30/45/60/90/120/180 to cover the typical angles

Are the tests above correct? I can append 45/60/120/180 to them and update current pull request.

[liuxuan30]

if this is the case, I'm kind of ok.

@pmairoldi @jjatie any comments? I think we could get this merged.

[pmairoldi]

Seems fine to me.

[liuxuan30]

ok then. @berbaspin please help add the tests, and we will take a look and move forward, thank you for your efforts!

[berbaspin]

@liuxuan30, cool! Should I create new commit with tests or will you add them by yourself? Below tests with different label positions and angles (LineChartTests).

func testLimitLineLabelRotatedBy180() {
        addLimitLineWithRotatedAngle(labelPosition: .rightBottom, labelRotationAngle: 180)
        assertChartSnapshot(matching: chart)
    }

    func testLimitLineLabelRotatedBy120() {
        addLimitLineWithRotatedAngle(labelPosition: .rightTop, labelRotationAngle: 120)
        assertChartSnapshot(matching: chart)
    }

    func testLimitLineLabelRotatedBy90() {
        addLimitLineWithRotatedAngle(labelPosition: .leftTop, labelRotationAngle: 90)
        assertChartSnapshot(matching: chart)
    }

    func testLimitLineLabelRotatedBy45() {
        addLimitLineWithRotatedAngle(labelPosition: .leftBottom, labelRotationAngle: 45)
        assertChartSnapshot(matching: chart)
    }

    func testLimitLineLabelRotatedBy30() {
        addLimitLineWithRotatedAngle(labelPosition: .leftBottom, labelRotationAngle: 30)
        assertChartSnapshot(matching: chart)
    }

    func testLimitLineLabelDefaultRotation() {
        addLimitLineWithRotatedAngle(labelPosition: .rightTop, labelRotationAngle: 0)
        assertChartSnapshot(matching: chart)
    }

    private func addLimitLineWithRotatedAngle(labelPosition: ChartLimitLine.LabelPosition, labelRotationAngle: CGFloat) {
        let limitline = ChartLimitLine(limit: 50, label: "Limit Line")
        limitline.labelPosition = labelPosition
        limitline.labelRotationAngle = labelRotationAngle
        chart.leftAxis.addLimitLine(limitline)
    }

[berbaspin]

@liuxuan30, are there any improvements I should make?

[liuxuan30]

Sorry for the late response, took a week off work. I have completed the review, left a few comments.

Meanwhile, I have triggered the work flow so we can check the CI result later

[liuxuan30]

regarding Swift / Build framework (OS=16.1,name=Apple TV 4K (3rd generation), build test) seems you forgot to generate test images for apple TV;

[berbaspin]

@liuxuan30, I added two commits:

1. fixed the code style
2. labelLineRotated_Width/Height got the same position
3. generated test images for apple TV

please remind me why we can abandon align here?

I quickly looked at align, it is often used here:

let drawPoint = getDrawPoint(text: text, point: point, align: align, attributes: attributes)

We can abandon align, because it was used only for calculating the position of the label. But I used a different methodology.

In method, that was used before, the anchor was CGPoint(x: 0.5, y: 0.5) and TextAlignment (.left or .right). It works fine with non rotated labels. That's why I decided to сhange label x position by calculating labelLineRotatedWidth and set anchor to CGPoint(x: 0.0, y: 0.0). I think that for HorizontalBarChartView is used similar calculations for xAxis label:

let xlabelwidth = xAxis.labelRotatedWidth

        if xAxis.isEnabled
        {
            // offsets for x-labels
            if xAxis.labelPosition == .bottom
            {
                offsetLeft += xlabelwidth
            }
            else if xAxis.labelPosition == .top
            {
                offsetRight += xlabelwidth
            }
            else if xAxis.labelPosition == .bothSided
            {
                offsetLeft += xlabelwidth
                offsetRight += xlabelwidth
            }
        }

[liuxuan30]

thanks! let's wait for the CI results

[liuxuan30]

@liuxuan30, I added two commits:

1. fixed the code style
2. labelLineRotated_Width/Height got the same position
3. generated test images for apple TV

please remind me why we can abandon align here? I quickly looked at align, it is often used here: let drawPoint = getDrawPoint(text: text, point: point, align: align, attributes: attributes)

We can abandon align, because it was used only for calculating the position of the label. But I used a different methodology.

In method, that was used before, the anchor was CGPoint(x: 0.5, y: 0.5) and TextAlignment (.left or .right). It works fine with non rotated labels. That's why I decided to сhange label x position by calculating labelLineRotatedWidth and set anchor to CGPoint(x: 0.0, y: 0.0). I think that for HorizontalBarChartView is used similar calculations for xAxis label:

let xlabelwidth = xAxis.labelRotatedWidth

        if xAxis.isEnabled
        {
            // offsets for x-labels
            if xAxis.labelPosition == .bottom
            {
                offsetLeft += xlabelwidth
            }
            else if xAxis.labelPosition == .top
            {
                offsetRight += xlabelwidth
            }
            else if xAxis.labelPosition == .bothSided
            {
                offsetLeft += xlabelwidth
                offsetRight += xlabelwidth
            }
        }

I'm kind of concerned that, it used to support align.center, but when we removed this, how do we implement .center behavior? If a current user uses .center, then after the new update, how to do it?

[berbaspin]

@liuxuan30, sorry, but I can't find the place, where we can change the alignment of the limitline's label. Neither ChartLimitLine, nor ComponentBase has such property. We only have the ability to change the position of the ChartLimitLine Label. We have enum:

public enum LabelPosition: Int
    {
        case leftTop
        case leftBottom
        case rightTop
        case rightBottom
    }

There are no cases centerTop or centerBottom.

This function also doesn't have the alignment parameter. open override func renderLimitLines(context: CGContext)

But there is an initialization of align and a value assignment depending of LabelPosition. (I found only here)

renderLimitLines calls another function drawText. And here we have two types of it (they both available in current version of DGCharts and I didn't make any changes to them):

1. public func drawText(_ text: String, at point: CGPoint, align: TextAlignment, anchor: CGPoint = CGPoint(x: 0.5, y: 0.5), angleRadians: CGFloat = 0.0, attributes: [NSAttributedString.Key : Any]?)
2. public func drawText(_ text: String, at point: CGPoint, anchor: CGPoint = CGPoint(x: 0.5, y: 0.5), angleRadians: CGFloat, attributes: [NSAttributedString.Key : Any]?)

The first function was used with ChartLimitLine label in current version of DGCharts. But I replaced it with the second one. If we check the first function, we will see, that it calls the second one, if rotation angle is not 0.0.

If user overrides one of this function open override func renderLimitLines(context: CGContext) or public func drawText, my changes, that I offer in this pull request, will not take an effect on them, because the user has his own implementation.

P.S. Just for test. I took the current of DGCharts and tried to change align for leftAxis limit line label from .left to .center in open override func renderLimitLines(context: CGContext). It didn't fit in the center.

[berbaspin]

Previously I added generated images for Model: Apple TV 4K (3rd generation). tvOS 16.1 (20K67). And CI failed again. I think it happens, because I created images only for AppleSilicon (both iOS and tvOS).

✗ testLimitLineLabelDefaultRotation, failed - No reference was found on disk. Automatically recorded snapshot: …
✗ testLimitLineLabelRotatedBy120, failed - No reference was found on disk. Automatically recorded snapshot: …
✗ testLimitLineLabelRotatedBy180, failed - No reference was found on disk. Automatically recorded snapshot: …
✗ testLimitLineLabelRotatedBy30, failed - No reference was found on disk. Automatically recorded snapshot: …
✗ testLimitLineLabelRotatedBy45, failed - No reference was found on disk. Automatically recorded snapshot: …
✗ testLimitLineLabelRotatedBy90, failed - No reference was found on disk. Automatically recorded snapshot: 

Can you create them for x86_64? Using Rosetta I got an error:

debugserver is x86_64 binary running in translation, attached failed.

[liuxuan30]

Can you create them for x86_64? Using Rosetta I got an error:

I can. I will update today or tomorrow.

Let's check if iOS CI works, it was canceld that I don't know how.

@berbaspin please add me with write acess to your Charts fork repo

[liuxuan30]

@liuxuan30, sorry, but I can't find the place, where we can change the alignment of the limitline's label. Neither ChartLimitLine, nor ComponentBase has such property. We only have the ability to change the position of the ChartLimitLine Label. We have enum:

public enum LabelPosition: Int
    {
        case leftTop
        case leftBottom
        case rightTop
        case rightBottom
    }

There are no cases centerTop or centerBottom.

This function also doesn't have the alignment parameter. open override func renderLimitLines(context: CGContext)

But there is an initialization of align and a value assignment depending of LabelPosition. (I found only here)

renderLimitLines calls another function drawText. And here we have two types of it (they both available in current version of DGCharts and I didn't make any changes to them):

1. public func drawText(_ text: String, at point: CGPoint, align: TextAlignment, anchor: CGPoint = CGPoint(x: 0.5, y: 0.5), angleRadians: CGFloat = 0.0, attributes: [NSAttributedString.Key : Any]?)
2. public func drawText(_ text: String, at point: CGPoint, anchor: CGPoint = CGPoint(x: 0.5, y: 0.5), angleRadians: CGFloat, attributes: [NSAttributedString.Key : Any]?)

The first function was used with ChartLimitLine label in current version of DGCharts. But I replaced it with the second one. If we check the first function, we will see, that it calls the second one, if rotation angle is not 0.0.

If user overrides one of this function open override func renderLimitLines(context: CGContext) or public func drawText, my changes, that I offer in this pull request, will not take an effect on them, because the user has his own implementation.

P.S. Just for test. I took the current of DGCharts and tried to change align for leftAxis limit line label from .left to .center in open override func renderLimitLines(context: CGContext). It didn't fit in the center.

My mistake. My memory with Charts is rusty. You are right that this is the entry for drawText, not the opposite. I was thinking you are modifying drawText(), but actually it's renderLimitLines. So should be fine.

Let's fix the CI issues and get it merged

[berbaspin]

@berbaspin please add me with write acess to your Charts fork repo

@liuxuan30, I sent the invite

[liuxuan30]

it's weird that the iOS CI keeps failing:

2023-08-22T01:32:35.7435390Z ▸ Touching DGChartsTests.xctest (in target 'DGChartsTests' from project 'Charts')
2023-08-22T01:34:35.7255440Z ##[debug]Re-evaluate condition on job cancellation for step: 'Build framework - OS=16.2,name=iPhone 14 Pro'.
2023-08-22T01:34:48.3605590Z ##[error]The operation was canceled.
2023-08-22T01:34:48.3620880Z ##[debug]System.OperationCanceledException: The operation was canceled.
2023-08-22T01:34:48.3621590Z ##[debug]   at System.Threading.CancellationToken.ThrowOperationCanceledException()
2023-08-22T01:34:48.3622510Z ##[debug]   at GitHub.Runner.Sdk.ProcessInvoker.ExecuteAsync(String workingDirectory, String fileName, String arguments, IDictionary`2 environment, Boolean requireExitCodeZero, Encoding outputEncoding, Boolean killProcessOnCancel, Channel`1 redirectStandardIn, Boolean inheritConsoleHandler, Boolean keepStandardInOpen, Boolean highPriorityProcess, CancellationToken cancellationToken)
2023-08-22T01:34:48.3623790Z ##[debug]   at GitHub.Runner.Common.ProcessInvokerWrapper.ExecuteAsync(String workingDirectory, String fileName, String arguments, IDictionary`2 environment, Boolean requireExitCodeZero, Encoding outputEncoding, Boolean killProcessOnCancel, Channel`1 redirectStandardIn, Boolean inheritConsoleHandler, Boolean keepStandardInOpen, Boolean highPriorityProcess, CancellationToken cancellationToken)
2023-08-22T01:34:48.3625040Z ##[debug]   at GitHub.Runner.Worker.Handlers.DefaultStepHost.ExecuteAsync(IExecutionContext context, String workingDirectory, String fileName, String arguments, IDictionary`2 environment, Boolean requireExitCodeZero, Encoding outputEncoding, Boolean killProcessOnCancel, Boolean inheritConsoleHandler, String standardInInput, CancellationToken cancellationToken)
2023-08-22T01:34:48.3626040Z ##[debug]   at GitHub.Runner.Worker.Handlers.ScriptHandler.RunAsync(ActionRunStage stage)
2023-08-22T01:34:48.3626470Z ##[debug]   at GitHub.Runner.Worker.ActionRunner.RunAsync()
2023-08-22T01:34:48.3626950Z ##[debug]   at GitHub.Runner.Worker.StepsRunner.RunStepAsync(IStep step, CancellationToken jobCancellationToken)

I have triggered a master branch CI to see what's going on

it's strange that master passed CI easily. Not sure why this PR always has canceled. Need investigate further.

[liuxuan30]

it's weird that the iOS CI keeps failing:

2023-08-22T01:32:35.7435390Z ▸ Touching DGChartsTests.xctest (in target 'DGChartsTests' from project 'Charts')
2023-08-22T01:34:35.7255440Z ##[debug]Re-evaluate condition on job cancellation for step: 'Build framework - OS=16.2,name=iPhone 14 Pro'.
2023-08-22T01:34:48.3605590Z ##[error]The operation was canceled.
2023-08-22T01:34:48.3620880Z ##[debug]System.OperationCanceledException: The operation was canceled.
2023-08-22T01:34:48.3621590Z ##[debug]   at System.Threading.CancellationToken.ThrowOperationCanceledException()
2023-08-22T01:34:48.3622510Z ##[debug]   at GitHub.Runner.Sdk.ProcessInvoker.ExecuteAsync(String workingDirectory, String fileName, String arguments, IDictionary`2 environment, Boolean requireExitCodeZero, Encoding outputEncoding, Boolean killProcessOnCancel, Channel`1 redirectStandardIn, Boolean inheritConsoleHandler, Boolean keepStandardInOpen, Boolean highPriorityProcess, CancellationToken cancellationToken)
2023-08-22T01:34:48.3623790Z ##[debug]   at GitHub.Runner.Common.ProcessInvokerWrapper.ExecuteAsync(String workingDirectory, String fileName, String arguments, IDictionary`2 environment, Boolean requireExitCodeZero, Encoding outputEncoding, Boolean killProcessOnCancel, Channel`1 redirectStandardIn, Boolean inheritConsoleHandler, Boolean keepStandardInOpen, Boolean highPriorityProcess, CancellationToken cancellationToken)
2023-08-22T01:34:48.3625040Z ##[debug]   at GitHub.Runner.Worker.Handlers.DefaultStepHost.ExecuteAsync(IExecutionContext context, String workingDirectory, String fileName, String arguments, IDictionary`2 environment, Boolean requireExitCodeZero, Encoding outputEncoding, Boolean killProcessOnCancel, Boolean inheritConsoleHandler, String standardInInput, CancellationToken cancellationToken)
2023-08-22T01:34:48.3626040Z ##[debug]   at GitHub.Runner.Worker.Handlers.ScriptHandler.RunAsync(ActionRunStage stage)
2023-08-22T01:34:48.3626470Z ##[debug]   at GitHub.Runner.Worker.ActionRunner.RunAsync()
2023-08-22T01:34:48.3626950Z ##[debug]   at GitHub.Runner.Worker.StepsRunner.RunStepAsync(IStep step, CancellationToken jobCancellationToken)

I have triggered a master branch CI to see what's going on

it's strange that master passed CI easily. Not sure why this PR always has canceled. Need investigate further.

I trigged a test CI https://github.com/danielgindi/Charts/actions/runs/5933792579, it seems after correctly generating snapshots for the specific arch, it can now pass. the cancelled failures look like due to arch issue. @berbaspin you can merge my PR in your repo and try again.

[berbaspin]

@liuxuan30, thank you! I merged your pull request

[liuxuan30]

All green and merged. Thank you!