Fix pulse triggers

[ennerf]

Contrary to what my previous testing showed, the pre/post layout hooks do not trigger a pulse. Those must have been caused by animations or other timers inside the apps I was testing with (sampler and some individual samples).

This caused some issues where concurrent updates would correctly register the hooks, but the chart would never get updated without something else triggering a pulse (e.g. mouse click, resize, etc).

PR changes:

• The hooks are always registered, but they are skipped if all states are clean
• Updates now always trigger a layout of a style group that is unmanaged and has an empty layoutChildren implementation. This triggers a pulse without forcing unnecessary work, i.e., it does not trigger a layout of the parent chart.
• Fixed a bug that caused auto ranges to always be invalidated even if they were not changed (this didn't show up the way we were doing it before)
• Cosmetic cleanup of the chart constructor

[codecov]

Codecov Report

Patch coverage: 96.77% and project coverage change: -0.06% ⚠️

Comparison is base (ce9ba7f) 49.72% compared to head (4cdeb1a) 49.67%.

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #601      +/-   ##
============================================
- Coverage     49.72%   49.67%   -0.06%     
+ Complexity     6247     6241       -6     
============================================
  Files           379      378       -1     
  Lines         37625    37583      -42     
  Branches       6165     6158       -7     
============================================
- Hits          18708    18668      -40     
+ Misses        17687    17686       -1     
+ Partials       1230     1229       -1     

Files Changed	Coverage Δ
...art/src/main/java/io/fair_acc/chartfx/XYChart.java	72.66% <66.66%> (-0.39%)	⬇️
...chart/src/main/java/io/fair_acc/chartfx/Chart.java	79.68% <97.56%> (+0.21%)	⬆️
...in/java/io/fair_acc/chartfx/ui/css/StyleGroup.java	100.00% <100.00%> (ø)
...c/main/java/io/fair_acc/chartfx/utils/FXUtils.java	89.09% <100.00%> (+0.85%)	⬆️
...main/java/io/fair_acc/dataset/events/BitState.java	55.90% <100.00%> (ø)

... and 7 files with indirect coverage changes

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

[pr-explainer-bot]

Pull Request Review

Hey there! 👋 Here's a summary of the previous results for the pull request review:

Changes

1. Removed import statement for io.fair_acc.chartfx.ui.utils.LayoutHook at line 9.
2. Added import statement for io.fair_acc.dataset.AxisDescription at line 10.
3. Removed import statement for javafx.scene.paint.Paint at line 14.
4. Removed layoutHooks variable declaration at line 16.
5. Removed layoutHooks.registerOnce() method call at line 19.
6. Removed layoutHooks change listener at lines 20-43.

Suggestions

1. Line 7: Added import statement for javafx.scene.Parent.
2. Line 17: Changed class description from 'A hidden group that holds styles' to 'A hidden node that holds styles'.
3. Line 18: Added description 'This node is unmanaged and does not do any layout'.
4. Line 30: The relocate(0, 0) method call seems unnecessary as the StyleGroup class extends Parent which does not perform layout.
5. Line 35: The layoutChildren() method is empty and does nothing. Consider removing it if it serves no purpose.

Bugs

1. File LayoutHook.java: Line 165 - The comment 'Meant to be called during the layout phase.' is misleading as the method hasRunPreLayout() is actually meant to be called outside the layout phase.
2. File LayoutHook.java: Line 97 - The comment 'Generally this should be called by the pulse by registering registerOnce(), but when registering during the CSS or layout this method may be called manually to get it executed within the same pulse.' is confusing and could be clarified.
3. Potential bug: In FXUtils.java, line 11, the import statement for io.fair_acc.dataset.utils.AssertUtils was added. Make sure this import is correct and necessary.
4. Potential bug: In FXUtils.java, line 341, the tryGetChartParent method was modified to return an Optional. Verify if this change is intended and doesn't introduce any issues.
5. Potential bug: In FXUtils.java, lines 342-393, the registerLayoutHooks method was added. Check if this method is implemented correctly and doesn't cause any problems.
6. Potential bug: In BitState.java, line 126, the clear method was modified to return the bits before clearing. Ensure that this change doesn't affect the functionality of the method.
7. Potential bug: In BitState.java, lines 369-375, the setDirtyAndGetDelta method was modified to return the previous state. Verify if this change is intended and doesn't introduce any issues.
8. Potential bug: In BitState.java, lines 413-417, the clear method was modified to return the current state. Check if this change is intended and doesn't affect the behavior of the method.

Improvements

One place in the code that could be refactored for better readability is in the StyleGroup class, specifically in the constructor public StyleGroup(ObservableList<Node> children). Instead of calling StyleUtil.hiddenStyleNode(this) and then adding this to the children list, it would be clearer to directly add a hidden style node to the children list. Here's a refactored version of the code:

public StyleGroup(ObservableList<Node> children) {
    children.add(StyleUtil.hiddenStyleNode(new Group()));
}

This eliminates the need for the StyleUtil.hiddenStyleNode() method and makes the code more concise.

Rating

Overall rating: 7.5/10

Criteria:

• Readability: The code is generally readable, but there are some areas where comments could be improved for better understanding.
• Performance: There doesn't seem to be any major performance issues.
• Security: There are no obvious security vulnerabilities in the code.

That's it for the summary! Let me know if you need any further assistance. 😄

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

[wirew0rm]

Looks good, thanks for following this up!