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

add visibility state and toggling of datasets #481

Merged
merged 13 commits into from
Feb 10, 2022

Conversation

ennerf
Copy link
Collaborator

@ennerf ennerf commented Jan 15, 2022

This is an outline for what the toggle feature could look like. It's not ready for merging. Below are some questions I've run into so far:

  1. What's the best way to omitt the hidden datasets in the axis range computation? Ideally something that could work on the Chart baseclass?

  2. Is there a good way to deal with access to the dataset, e.g., EditDataSet without supporting it in every plugin?

@RalphSteinhagen
Copy link
Member

Very nice PR and good addition to chart-fx! 👍

@ennerf
Copy link
Collaborator Author

ennerf commented Jan 17, 2022

I added the suggested changes and ran through all samples. The IMO noteworthy ones are:

EditDataSetSample
Hiding a dataset does not remove the point selection. Without modifications it is possible to select both datasets, hide one, and then apply operations on hidden points resulting in indexing errors. I changed it such that points remain selected, but that any modifying operations are ignored.

MetaDataRendererSample
hiding random walk also hides the gaussy dataset. Maybe this is actually be the desired behavior?

@ennerf ennerf marked this pull request as ready for review January 17, 2022 16:54
Copy link
Member

@RalphSteinhagen RalphSteinhagen left a comment

Choose a reason for hiding this comment

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

Hi @ennerf looks good.

It doesn't compile in the CI/CD due to JavaFX dependencies in non-UI modules (new BooleanProperty used for visibility state) but that IMO could be easily replaced by a simple boolean isVisible state and using the existing event notification mechanisms, e.g. in AbstractDataSet

    public boolean isVisible() { return isVisible; };

    public void setVisible(boolean visible) {
        if (visible != isVisible) {
            isVisible = visible;
            fireInvalidated(new UpdatedMetaDataEvent(this, "changed visibility"));
        }
    }

With these changes it should compile/work as expected. Please have a look (e.g. merge) also at the code-formatter (Restyled.io bot PR) and static-code analysis checks.

After that this could be merged. Thanks for this very nice PR! 👍

@wirew0rm
Copy link
Member

wirew0rm commented Jan 20, 2022

Sorry, I kind of missed the updates in this PR.

MetaDataRendererSample hiding random walk also hides the gaussy dataset. Maybe this is actually be the desired behavior?

This is actually a bug in the sample I also discovered when I investigated the 11.2.6 axis regression. The gaussy dataset implements incorrect range information by overwriting AbstractTestFunction's get() function but not updating the range. So when you toggle visibiltiy of the other datasets, the axis range changes to something outside of the gauss functions x-value range. You probably can get it to show up again by manually zooming. I have a partial fix for that here, so don't worry about this one.

@codecov
Copy link

codecov bot commented Feb 9, 2022

Codecov Report

Merging #481 (f4b7b7a) into master (89a5ad3) will decrease coverage by 0.00%.
The diff coverage is 43.63%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #481      +/-   ##
============================================
- Coverage     53.63%   53.62%   -0.01%     
- Complexity     7477     7489      +12     
============================================
  Files           389      389              
  Lines         40952    40989      +37     
  Branches       6604     6615      +11     
============================================
+ Hits          21963    21982      +19     
- Misses        17404    17417      +13     
- Partials       1585     1590       +5     
Impacted Files Coverage Δ
...rc/main/java/de/gsi/chart/plugins/EditDataSet.java 23.21% <0.00%> (-0.19%) ⬇️
...gsi/chart/renderer/spi/ContourDataSetRenderer.java 76.04% <0.00%> (ø)
...a/de/gsi/chart/renderer/spi/HistogramRenderer.java 66.25% <0.00%> (-1.16%) ⬇️
...gsi/chart/renderer/spi/LabelledMarkerRenderer.java 83.87% <0.00%> (-1.10%) ⬇️
.../gsi/chart/renderer/spi/MountainRangeRenderer.java 0.00% <0.00%> (ø)
...e/gsi/chart/renderer/spi/ReducingLineRenderer.java 0.00% <0.00%> (ø)
...rt/renderer/spi/financial/CandleStickRenderer.java 88.60% <0.00%> (-0.64%) ⬇️
.../chart/renderer/spi/financial/HighLowRenderer.java 85.33% <0.00%> (-0.67%) ⬇️
...-dataset/src/main/java/de/gsi/dataset/DataSet.java 100.00% <ø> (ø)
...e/gsi/chart/renderer/spi/ErrorDataSetRenderer.java 79.11% <33.33%> (-0.37%) ⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89a5ad3...f4b7b7a. Read the comment docs.

wirew0rm
wirew0rm previously approved these changes Feb 10, 2022
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.

Very nice feature, I really like how it turned out!

@RalphSteinhagen RalphSteinhagen merged commit e51f141 into fair-acc:master Feb 10, 2022
@milo-gsi
Copy link
Contributor

Thanks for providing that very helpful feature, Florian!
Just stumbled over it "accidentally" and I love it.

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

5 participants