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

Label refactoring #575

Merged
merged 14 commits into from
Mar 16, 2023
Merged

Label refactoring #575

merged 14 commits into from
Mar 16, 2023

Conversation

ennerf
Copy link
Collaborator

@ennerf ennerf commented Mar 9, 2023

This PR removes the 20MB icu4j dependency and implements a custom number formatter based on Schubfach. It also removes the temporary string conversions from DigitNumberArithmetic. The format changed a bit to make labels easier to read. Here is a video of the currently implemented behavior: https://youtu.be/Od5KhBZrWiU.

Open questions

  • The DefaultFormatter now handles log cases as well. For now I added a deprecated annotation to DefaultLogFormatter, but maybe it should be deleted?
  • The DefaultFormatter::fromString used the format for parsing. Did that do anything special that does not work with Double::parse?
  • What is the rangeIndex variable in DefaultFormatter? I didn't see any uses of it.
  • The default formatter interprets NumberFormatter::setPrecision as the number of after comma digits plus one. Is this reasonable or should we change the name to setAfterCommaDigits and potentially break backwards compatibility?
  • Should the exponential form use a lower case e to make it easier to distinguish between significand and exponent? e.g.

image
image
image image

TODOs

  • Remove commented out debug prints

Behavior differences

Before After Comment
image image unnecessary after comma zeros are removed
image image exponential axes render as plain numbers if all ticks are within the plain format range
image image labels are rounded to the first significant digit
image image plain labels keep their form and just add extra digits

Known issues

Before After Comment
image image The zero values in the exponential format can be tiny and stick out (same behavior, so not a regression). Sometimes zero gets rendered as negative zero. The plain format has a workaround, but the easiest place to fix this may be in the tick renderer.

@sonatype-lift
Copy link

sonatype-lift bot commented Mar 9, 2023

🛠 Lift Auto-fix

Some of the Lift findings in this PR can be automatically fixed. You can download and apply these changes in your local project directory of your branch to review the suggestions before committing.1

# Download the patch
curl https://lift.sonatype.com/api/patch/github.com/fair-acc/chart-fx/575.diff -o lift-autofixes.diff

# Apply the patch with git
git apply lift-autofixes.diff

# Review the changes
git diff

Want it all in a single command? Open a terminal in your project's directory and copy and paste the following command:

curl https://lift.sonatype.com/api/patch/github.com/fair-acc/chart-fx/575.diff | git apply

Once you're satisfied, commit and push your changes in your project.

Footnotes

  1. You can preview the patch by opening the patch URL in the browser.

Copy link
Member

@wirew0rm wirew0rm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR removes the 20MB icu4j dependency and implements a custom number formatter based on Schubfach. It also removes the temporary string conversions from DigitNumberArithmetic. The format changed a bit to make labels easier to read. Here is a video of the currently implemented behavior: https://youtu.be/Od5KhBZrWiU.

Very nice work, thanks for tackling this, looks very good. And special thanks for documenting all the changes and open questions so nicely!

I tried to answer the questions directly in the code when applicable to the best of my understanding, but I

Should the exponential form use a lower case e to make it easier to distinguish between significand and exponent? e.g.

I agree that it's more readable, but outside of programming I think it is avoided to prevent mistaking it with the fundamental constant. This is not a problem for axes, but it is not uncommon to have a UI which displays a chart and formula side by side and one would want to have consistent number representation between them. 1,23456789ᴇ14 is another variant i've seen in the wild and there even is a dedicated unicode symbol 6.022⏨23 but I think it is not that common.
It looks like this is configurable via the locale dependent java.text.DecimalFormatSymbols.
I'm also looking into making more things Stylable properties, so we could have something like .chart {scientific-notation-separator: "e";} in the user css. Out of scope for this PR.

Behavior differences

I agree with all of them being clear improvements except for the logarithmic axis, where people might actually prefer the default of the exponential notation.

Known issues
The zero values in the exponential format can be tiny and stick out (same behavior, so not a regression). Sometimes zero gets rendered as negative zero. The plain format has a workaround, but the easiest place to fix this may be in the tick renderer.

This would mean a special case in the renderer to render a zero label if the value is very small compared to the range?

@ennerf
Copy link
Collaborator Author

ennerf commented Mar 11, 2023

This would mean a special case in the renderer to render a zero label if the value is very small compared to the range?

I think so. It's difficult to back out the zero at a lower point and forward the information to the formatter. The previous implementation has the same issue though, so maybe this should be in another PR.

@ennerf
Copy link
Collaborator Author

ennerf commented Mar 12, 2023

I didn't like the rounding of 0.25 steps to [0.0, 0.3, 0.5, 0.8, 1.0], so I added an extra digit for cases where the next digit is not zero.

image image image

@wirew0rm wirew0rm temporarily deployed to coverage March 15, 2023 14:33 — with GitHub Actions Inactive
@codecov
Copy link

codecov bot commented Mar 15, 2023

Codecov Report

Patch coverage: 73.47% and project coverage change: +0.21 🎉

Comparison is base (b990aca) 51.75% compared to head (56a6dcb) 51.96%.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #575      +/-   ##
============================================
+ Coverage     51.75%   51.96%   +0.21%     
- Complexity     6337     6400      +63     
============================================
  Files           364      363       -1     
  Lines         36798    37028     +230     
  Branches       5992     6063      +71     
============================================
+ Hits          19043    19243     +200     
- Misses        16500    16522      +22     
- Partials       1255     1263       +8     
Impacted Files Coverage Δ
...ava/io/fair_acc/chartfx/axes/spi/AbstractAxis.java 76.04% <ø> (-0.26%) ⬇️
...fair_acc/chartfx/utils/DecimalStringConverter.java 0.00% <ø> (ø)
...artfx/utils/ScientificNotationStringConverter.java 0.00% <ø> (ø)
...main/java/io/fair_acc/chartfx/utils/Schubfach.java 53.57% <53.57%> (ø)
..._acc/chartfx/axes/spi/format/DefaultFormatter.java 70.52% <75.94%> (-0.91%) ⬇️
...io/fair_acc/chartfx/utils/NumberFormatterImpl.java 90.75% <92.68%> (+28.84%) ⬆️

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@wirew0rm wirew0rm temporarily deployed to coverage March 15, 2023 14:48 — with GitHub Actions Inactive
@wirew0rm wirew0rm temporarily deployed to coverage March 15, 2023 14:48 — with GitHub Actions Inactive
Copy link
Member

@wirew0rm wirew0rm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the work you put into this, looks and feels great, lets get this merged.

@wirew0rm wirew0rm merged commit 8d349d9 into fair-acc:main Mar 16, 2023
@ennerf ennerf deleted the ennerf/refactor-labels branch March 16, 2023 17:37
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

2 participants