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

Overhauled dataset styling #603

Merged
merged 92 commits into from
Sep 18, 2023
Merged

Overhauled dataset styling #603

merged 92 commits into from
Sep 18, 2023

Conversation

ennerf
Copy link
Collaborator

@ennerf ennerf commented Aug 18, 2023

Before

The styling was previously based on Strings that could be applied to datasets as well as to individual data points. Some reasons for doing it this way were that the style needs to be storable and usable without a JavaFX dependency (DataSets are not dependent on JavaFX).

Problems

  • cumbersome to use
  • inefficient
  • lots of string manipulation
  • the style string used =, which is not valid CSS
  • the style properties did not match the JavaFX naming conventions

After

Each renderer is now part of the SceneGraph and generates a styleable node for each dataset (a dataset can be in multiple renderers, but only once per renderer). The nodes get styled with JavaFX properties and apply the source dataSet.getStyle(), which was changed to match the JavaFX properties. The individual styles still use strings, but there is now no more overhead when they are not used.

  • The Financial themes were completely removed and replaced with equivalent CSS based themes and pseudo classes
  • The default colors are now set via CSS and the theme can be changed via pseudo classes
  • Major cleanup of the renderers. A lot of boiler plate was removed and it's now more obvious where colors are coming from
  • The hatchShiftByIndex now also applies without custom styles, so multiple datasets can show error surfaces simultaneously
    image

Ways to style DataSets

1) directly on the Node

Quick way to style individual datasets

var styleNode = renderer.getStyleNode(dataSet);
styleNode.setFill(Color.RED);

2) via CSS and classes

Good way to style certain classes of data that should always look the same

dataSet.addStyleClasses("cartesian-data-x");
.cartesian-data-x {
  -fill-color: red !important;
}
.cartesian-data-y {
  -fill-color: green !important;
}
.cartesian-data-z {
  -fill-color: blue !important;
}

3) via style strings and a utility class

// generates: "visibility: visible; -fx-font: System; -fx-font-weight: bold;"
String style = DataSetStyleBuilder.instance()
    .setFontWeight("bold")
    .setFont("System")
    .setVisible(true)
    .build();
dataSet.setStyle(style);

Other Behavior changes

  • The style properties became consistent with JavaFX, so user applications that have stored datasets with the old properties need to do some translation
  • Removed the getDataSets from the Chart. The method is now a utility method that returns the datasets of the first renderer.
  • Legend icons were fixed and now disappear when a renderer doesn't need one
  • The point styles and the renderer caches are now significantly more efficient (fixes resolve performance issue caused by using DoubleArrayCache.getArrayExact #557)
  • The sampler app now has buttons for ScenicView and CSSFx

Bugs and things that need to be looked at after this PR

  • We should take a look at whether always adding a default renderer makes sense
  • The EditDataSet relied on the dataset visibility, but also only used the Chart::getDataSets, so I'm not sure what to do there
  • Similar story for the UpdateAxisLabels plugin
  • History intensity creates colors that aren't properly shifted, e.g., a less intense red becomes green
  • Many of the examples with custom styling need to be updated
  • Many renderers had code like below, but it's not clear to me why that is there. Needs to be added somewhere else.
// update categories in case of category axes for the first (index == '0') indexed data set
if (dataSetIndex == 0) {
	if (getFirstAxis(Orientation.HORIZONTAL) instanceof CategoryAxis) {
		final CategoryAxis axis = (CategoryAxis) getFirstAxis(Orientation.HORIZONTAL);
		axis.updateCategories(dataSet);
	}

	if (getFirstAxis(Orientation.VERTICAL) instanceof CategoryAxis) {
		final CategoryAxis axis = (CategoryAxis) getFirstAxis(Orientation.VERTICAL);
		axis.updateCategories(dataSet);
	}
}

More styling that didn't fit into this PR

The PR is already very large as is, so I didn't want to pack in more. However, after merging there are a few more styling things that I'd like to take a look at:

  • improve the CSS color lookup and add a theme class to support Modena-based and AtlantaFX-based themes (e.g. darkmode) out of the box
  • go over the used colors again and make sure they make sense. Instead of using -fx-stroke for lines and markers, maybe it makes sense to have dedicated -fx-line-color and -fx-marker-color properties

  • some of the renderer properties should probably be moved to the dataset nodes (e.g. marker size)

  • There are many classes that aren't used much anymore (e.g. Events) that should get removed once they are removed from the examples

@pr-explainer-bot
Copy link

Pull Request Review - Summary

Hey there! 👋 Here's a summary of the previous tasks and their results for the pull request review. Let's dive in!

Changes

  1. In task1, import statements were changed in various files to improve code organization and maintain consistency.
  2. In task2, suggestions were made to improve code readability and consistency at specific lines.
  3. In task3, potential bugs were identified in specific files and recommendations were provided.
  4. In task4, suggestions were made to refactor code for better readability in a specific file.

Suggestions

  1. In task2, specific lines were pointed out where improvements can be made, such as removing unnecessary empty lines and adding helpful comments.

Bugs

  1. In task3, specific files were identified with missing newlines and semicolons, which should be added to fix the issues.

Improvements

  1. In task4, a specific method in the Chart.java file was suggested to be refactored for better readability.

Rating

Unfortunately, no rating was provided for the code in task5. It would have been helpful to have a rating on a scale of 0 to 10, considering criteria like readability, performance, and security.

That's it for the summary! If you have any further questions or need more details, feel free to ask. And don't forget, we also offer a premium plan that can handle bigger pull requests with more context. 😉

Happy coding!

@ennerf
Copy link
Collaborator Author

ennerf commented Aug 18, 2023

I recorded a flight recording for 60 seconds running the ChartHighUpdateRateSample with an extended Buffer capacity of 75k.

Main branch before layout changes
total 151 gcs, longest gc at 489ms and many old ones at >50ms. Heap grew to 8GB

image

Current main branch after layout changes
total 75 gcs, longest gc at 71ms and still quite a few old ones at >30ms. Heap grew to 6GB

image

This PR
total 5 gcs, longest gc 5ms (initial CSS), and 2.3ms in steady state. Heap grew to 0.2GB

image

(cherry picked from commit c453325)
improved benchmarking tools
@sonarcloud
Copy link

sonarcloud bot commented Sep 18, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 61 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 125 Code Smells

No Coverage information No Coverage information
0.9% 0.9% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

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.

Really great changes, this will make consistent and styling a lot easier while at the same time improving performance. Thanks a lot for this big contribution @ennerf!

Also includes the latency measurement tools from #604.

Reformatting will be done in a separate tree-wide commit.

@wirew0rm wirew0rm merged commit 3846311 into main Sep 18, 2023
8 of 12 checks passed
@wirew0rm wirew0rm deleted the ennerf/dataset-styling branch September 18, 2023 17:09
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.

2 participants