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

Initial zoom in Zoomer is incorrect #660

Merged
merged 2 commits into from
Feb 27, 2024

Conversation

ToniMarc1990
Copy link
Contributor

+ Clamp the plot coordinates to the axis range before querying the values for display
Copy link

codecov bot commented Feb 22, 2024

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 48.06%. Comparing base (ed86318) to head (2ea0ced).
Report is 1 commits behind head on main.

Files Patch % Lines
.../main/java/io/fair_acc/chartfx/plugins/Zoomer.java 0.00% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main     #660      +/-   ##
============================================
- Coverage     48.08%   48.06%   -0.02%     
- Complexity     6220     6221       +1     
============================================
  Files           374      374              
  Lines         38309    38309              
  Branches       6117     6117              
============================================
- Hits          18419    18415       -4     
- Misses        18732    18735       +3     
- Partials       1158     1159       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

Hi, welcome to chart-fx and thanks for following this up and providing a fix. I've only looked at the diff yet, but from that it looks good.

dataMin = axis.getValueForDisplay(minPlotCoordinate.getY());
dataMax = axis.getValueForDisplay(maxPlotCoordinate.getY());
dataMin = axis.getValueForDisplay(Math.min(axis.getHeight(), minPlotCoordinate.getY()));
dataMax = axis.getValueForDisplay(Math.min(axis.getHeight(), maxPlotCoordinate.getY()));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
dataMax = axis.getValueForDisplay(Math.min(axis.getHeight(), maxPlotCoordinate.getY()));
dataMax = axis.getValueForDisplay(Math.max(0, Math.min(axis.getHeight(), maxPlotCoordinate.getY())));

Is this sufficient or should it also clamp to >0? That would probably the rarer case, but I don't see any fundamental reason for only limiting one direction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about this case too and would say that it either never occurs or is rare.
I committed you suggested change :-).

Copy link

sonarcloud bot commented Feb 25, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

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.

LGTM, thanks again for contributing.

Since #636 describes multiple problems, does this fix only one aspects or are they all caused by this root cause? Otherwise i'll investigate and update the issue.

@wirew0rm wirew0rm merged commit 6ab7a28 into fair-acc:main Feb 27, 2024
5 of 8 checks passed
@ToniMarc1990
Copy link
Contributor Author

LGTM, thanks again for contributing.

Since #636 describes multiple problems, does this fix only one aspects or are they all caused by this root cause? Otherwise i'll investigate and update the issue.

@wirew0rm: Thanks for merging this fix. When is the next planned/unplanned release? We want to integrate ChartFX in our application and thus need this fix ASAP.

@RalphSteinhagen
Copy link
Member

@ToniMarc1990 we certainly can target a release soon.

May I ask you for a favour:
You seem to indicate that you are using ChartFX in an industrial setting which we are happy to support. We are continuously looking into fostering a more robust community around ChartFX without intruding on user privacy or application purposes.

To highlight ChartFX's versatility and advocate for continuing public support, we would like to gather user stories, applications, and companies without implying any endorsement or financial commitment from said author/company/team.

Would you/your company be willing to contribute by sharing:

  • Your company name, logo and domain of operation (robotics, medical, finance, research, ...).
  • A brief description of how you use ChartFX.
  • Screenshots showing ChartFX in your projects (sans sensitive info).
  • Any public links or materials featuring ChartFX.

Your input will be invaluable in showcasing real-world applications and securing resources for ChartFX's growth. The primary purpose is to highlight the many diverse applications of ChartFX and the JavaFX eco-system in general and to demonstrate how we use public resources to benefit not only GSI/FAIR but also industry and academia alike.

There are many nay-sayers who claim that engaging with the public costs more than it benefits -- which we'd like to dispel.

We are aware that being the first in any list is always the hardest. Thus, we would first collect public support from about a dozen companies/institutes before setting up a sub-page. Please reach out on our matrix channel or via fair-c2-admin@gsi.de if you prefer.

N.B. The matrix channel is also a good place to get in contact with the other ChartFX developers in a more timely and inclusive manner.

@ToniMarc1990
Copy link
Contributor Author

@ToniMarc1990 we certainly can target a release soon.

May I ask you for a favour: You seem to indicate that you are using ChartFX in an industrial setting which we are happy to support. We are continuously looking into fostering a more robust community around ChartFX without intruding on user privacy or application purposes.

To highlight ChartFX's versatility and advocate for continuing public support, we would like to gather user stories, applications, and companies without implying any endorsement or financial commitment from said author/company/team.

Would you/your company be willing to contribute by sharing:

  • Your company name, logo and domain of operation (robotics, medical, finance, research, ...).
  • A brief description of how you use ChartFX.
  • Screenshots showing ChartFX in your projects (sans sensitive info).
  • Any public links or materials featuring ChartFX.

Your input will be invaluable in showcasing real-world applications and securing resources for ChartFX's growth. The primary purpose is to highlight the many diverse applications of ChartFX and the JavaFX eco-system in general and to demonstrate how we use public resources to benefit not only GSI/FAIR but also industry and academia alike.

There are many nay-sayers who claim that engaging with the public costs more than it benefits -- which we'd like to dispel.

We are aware that being the first in any list is always the hardest. Thus, we would first collect public support from about a dozen companies/institutes before setting up a sub-page. Please reach out on our matrix channel or via fair-c2-admin@gsi.de if you prefer.

N.B. The matrix channel is also a good place to get in contact with the other ChartFX developers in a more timely and inclusive manner.

@RalphSteinhagen : We are interested in sharing information about us and the usage of ChartFX in our product. I forwarded the inquiry to my manager.

@ichaus-we
Copy link
Contributor

Since #636 describes multiple problems, does this fix only one aspects or are they all caused by this root cause? Otherwise i'll investigate and update the issue.

Thanks for the new release.
Problem 1 (of 4) is fixed by this. See my update in #636

@RalphSteinhagen
Copy link
Member

@ichaus-we have the same request: You seem to be using ChartFX in an industrial context. Would your company be willing to share details about its domain, example applications/screenshots, and be mentioned (Logo, Name) as one of the libs users?

This would help us a lot with building a strong(er) user base and long-term maintenance commitment.

Thanks in advance for your consideration.

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

4 participants