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

Prevent relayouting of the chart on indicator change #485

Merged
merged 4 commits into from
Feb 11, 2022

Conversation

wirew0rm
Copy link
Member

@wirew0rm wirew0rm commented Feb 8, 2022

Set the layout property of the indicator triangle to unmanaged s.t.
changes on the triangle will not invalidate the layout of its parent foreground pane.

Since the position of the triangle is already computed manually and
guaranteed to stay inside of the plot bounds, this should be the correct
solution here.

From the openjfx documentation for the managed property:

Defines whether or not this node's layout will be managed by it's parent.
If the node is managed, it's parent will factor the node's geometry
into its own preferred size and layoutBounds calculations and will lay it
out during the scene's layout pass. If a managed node's layoutBounds changes,
it will automatically trigger relayout up the scene-graph to the nearest
layout root (which is typically the scene's root node).

If the node is unmanaged, its parent will ignore the child in both preferred
size computations and layout. Changes in layoutBounds will not trigger
relayout above it. If an unmanaged node is of type Parent, it will act as a
"layout root", meaning that calls to Parent.requestLayout() beneath it will
cause only the branch rooted by the node to be relayed out, thereby isolating
layout changes to that root and below. It's the application's responsibility
to set the size and position of an unmanaged node.

By default all nodes are managed.
https://openjfx.io/javadoc/17/javafx.graphics/javafx/scene/Node.html#managedProperty

solves #476

@ennerf does this solve your indicator performance issue?

This might also apply to other nodes in the chart, especially with all the plugins that work on the overlay so we should check if there are more nodes to set to unmanaged.

Set the layout property of the indicator triangle to unmanaged s.t.
changes on the triangle will not invalidate the layout of its parent.

Since the position of the triangle is already computed manually and
guaranteed to stay inside of the plot bounds, this should be the correct
solution here.

From the openjfx documentation:
> Defines whether or not this node's layout will be managed by it's parent.
> If the node is managed, it's parent will factor the node's geometry
> into its own preferred size and layoutBounds calculations and will lay it
> out during the scene's layout pass. If a managed node's layoutBounds changes,
> it will automatically trigger relayout up the scene-graph to the nearest
> layout root (which is typically the scene's root node).
>
> If the node is unmanaged, its parent will ignore the child in both preferred
> size computations and layout. Changes in layoutBounds will not trigger
> relayout above it. If an unmanaged node is of type Parent, it will act as a
> "layout root", meaning that calls to Parent.requestLayout() beneath it will
> cause only the branch rooted by the node to be relayed out, thereby isolating
> layout changes to that root and below. It's the application's responsibility
> to set the size and position of an unmanaged node.
>
> By default all nodes are managed.
https://openjfx.io/javadoc/17/javafx.graphics/javafx/scene/Node.html#managedProperty

solves #476
@wirew0rm wirew0rm temporarily deployed to coverage February 8, 2022 15:11 Inactive
@wirew0rm wirew0rm temporarily deployed to coverage February 8, 2022 15:11 Inactive
@wirew0rm wirew0rm requested a review from ennerf February 8, 2022 15:11
@codecov
Copy link

codecov bot commented Feb 8, 2022

Codecov Report

Merging #485 (89897a0) into master (03ab1a9) will increase coverage by 0.00%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #485   +/-   ##
=========================================
  Coverage     53.63%   53.64%           
- Complexity     7480     7484    +4     
=========================================
  Files           389      389           
  Lines         40952    40958    +6     
  Branches       6604     6604           
=========================================
+ Hits          21965    21972    +7     
- Misses        17404    17407    +3     
+ Partials       1583     1579    -4     
Impacted Files Coverage Δ
...gsi/chart/plugins/AbstractRangeValueIndicator.java 0.00% <0.00%> (ø)
...c/main/java/de/gsi/chart/plugins/ChartOverlay.java 0.00% <0.00%> (ø)
...rc/main/java/de/gsi/chart/plugins/EditDataSet.java 23.34% <0.00%> (-0.05%) ⬇️
...si/chart/plugins/AbstractSingleValueIndicator.java 75.67% <100.00%> (+0.22%) ⬆️
...a/de/gsi/chart/plugins/AbstractValueIndicator.java 65.21% <100.00%> (+0.25%) ⬆️
...in/java/de/gsi/chart/plugins/DataPointTooltip.java 72.44% <100.00%> (+0.21%) ⬆️
.../main/java/de/gsi/chart/axes/spi/AbstractAxis.java 76.19% <0.00%> (-0.15%) ⬇️
...hart/plugins/measurements/DataSetMeasurements.java 75.77% <0.00%> (+0.34%) ⬆️
...ava/de/gsi/chart/plugins/YWatchValueIndicator.java 86.27% <0.00%> (+0.98%) ⬆️
.../java/de/gsi/dataset/locks/DefaultDataSetLock.java 60.21% <0.00%> (+2.15%) ⬆️

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 03ab1a9...89897a0. Read the comment docs.

@wirew0rm wirew0rm temporarily deployed to deploy February 8, 2022 15:16 Inactive
@ennerf
Copy link
Collaborator

ennerf commented Feb 8, 2022

That fixed it. Good find 👍

ennerf
ennerf previously approved these changes Feb 8, 2022
@wirew0rm wirew0rm temporarily deployed to coverage February 8, 2022 18:25 Inactive
@wirew0rm wirew0rm temporarily deployed to coverage February 8, 2022 18:25 Inactive
@wirew0rm wirew0rm temporarily deployed to coverage February 8, 2022 18:25 Inactive
@wirew0rm wirew0rm temporarily deployed to coverage February 8, 2022 18:25 Inactive
@wirew0rm wirew0rm temporarily deployed to deploy February 8, 2022 18:29 Inactive
@wirew0rm wirew0rm temporarily deployed to deploy February 8, 2022 18:29 Inactive
@lgtm-com
Copy link

lgtm-com bot commented Feb 8, 2022

This pull request introduces 1 alert when merging 4456a66 into 03ab1a9 - view on LGTM.com

new alerts:

  • 1 for Dereferenced variable may be null

@wirew0rm wirew0rm temporarily deployed to coverage February 9, 2022 08:24 Inactive
@wirew0rm wirew0rm temporarily deployed to coverage February 9, 2022 08:24 Inactive
@wirew0rm wirew0rm temporarily deployed to coverage February 9, 2022 08:24 Inactive
@wirew0rm wirew0rm temporarily deployed to deploy February 9, 2022 08:27 Inactive
@wirew0rm wirew0rm temporarily deployed to coverage February 9, 2022 12:20 Inactive
@wirew0rm wirew0rm temporarily deployed to coverage February 9, 2022 12:20 Inactive
@wirew0rm wirew0rm temporarily deployed to deploy February 9, 2022 12:24 Inactive
@wirew0rm wirew0rm merged commit 138d362 into master Feb 11, 2022
@wirew0rm wirew0rm deleted the preventRelayoutByIndicator branch February 11, 2022 15:25
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