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

[Feature] added ability to rotate ChartLimitLine label #5085

Merged
merged 6 commits into from
Aug 22, 2023

Conversation

berbaspin
Copy link
Contributor

Goals ⚽

Added labelRotationAngle to ChartLimitLine

Testing Details πŸ”

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

@berbaspin
Copy link
Contributor Author

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

@liuxuan30
Copy link
Member

@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
Copy link
Contributor Author

berbaspin commented Jul 24, 2023

@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
Copy link
Member

liuxuan30 commented Jul 25, 2023

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
Copy link
Member

liuxuan30 commented Jul 25, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

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

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

@pmairoldi
Copy link
Collaborator

Seems fine to me.

@liuxuan30
Copy link
Member

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

@berbaspin
Copy link
Contributor Author

berbaspin commented Jul 28, 2023

@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
Copy link
Contributor Author

@liuxuan30, are there any improvements I should make?

@liuxuan30
Copy link
Member

liuxuan30 commented Aug 16, 2023

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
Copy link
Member

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;

labelLineRotated_Width/Height got the same position
@berbaspin
Copy link
Contributor Author

@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
Copy link
Member

thanks! let's wait for the CI results

@liuxuan30
Copy link
Member

liuxuan30 commented Aug 18, 2023

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

berbaspin commented Aug 19, 2023

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
Copy link
Member

liuxuan30 commented Aug 21, 2023

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
Copy link
Member

@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
Copy link
Contributor Author

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

@liuxuan30, I sent the invite

@liuxuan30
Copy link
Member

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
Copy link
Member

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.

add tvOS and iOS snapshots on x64 arch
@berbaspin
Copy link
Contributor Author

@liuxuan30, thank you! I merged your pull request

@liuxuan30 liuxuan30 merged commit e516b04 into ChartsOrg:master Aug 22, 2023
8 checks passed
@liuxuan30
Copy link
Member

All green and merged. Thank you!

drewster99 pushed a commit to drewster99/Charts that referenced this pull request Nov 8, 2023
* gindi-master: (55 commits)
  [Feature] added ability to rotate ChartLimitLine label (ChartsOrg#5085)
  update readme
  fix podspec
  update ci
  update readme versions
  fix build issue
  remove swift-algorithms package in favor of manully importing needed functions
  fix ChartsOrg#5033: always run snapshot tests in light mode
  update tvos
  undo framework bump
  bump min version to 13 for ios project
  Bump actions/checkout to v3
  Fix Xcode 14.3 import warnings
  add discardable results to operations that do not require result
  update readme to change name
  update workflow to run on release branches
  Change library name from Charts to DGCharts
  Unnecessary space Removal and Semicolons are removed
  Update README.md
  Turn on the BUILD_LIBRARY_FOR_DISTRIBUTION flag
  ...
apollonian pushed a commit to foldmoney/Charts that referenced this pull request Mar 11, 2024
* added labelRotationAngle to ChartLimitLine

* added limit line label rotation tests

* fixed the code style
labelLineRotated_Width/Height got the same position

* generated test images for apple TV

* add snapshots on x64 arch for tvOS and iOS

---------

Co-authored-by: Xuan <liuxuan30@noreply.github.com>
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.

None yet

3 participants