ennerf commented 1 year ago

PR with some tiny fixes:

Made list of BitState listeners thread-safe
Changed BenchPlugin to only allow a single open Window (fixes reset when a 2nd window is closed)
Limited default bench measures to the most interesting ones
Changed SimplePerformanceMeter to use the Node::isDirtyEmpty method instead of the Node::dirtyBits field which was changed/removed at one point.

pr-explainer-bot[bot] commented 1 year ago

Hey there! I see you need help summarizing the previous results into a Pull Request review markdown document. Let's get started!

Pull Request Review

Changes

Line 947 in Chart.java: The benchDrawCanvas variable is changed from recorder.newDuration("chart-drawCanvas") to recorder.newDebugDuration("chart-drawCanvas").
Line 298 in XYChart.java: The axis.setRecorder method call is changed from recorder.addPrefix("axis" + i++) to recorder.addPrefix("axis" + i).
Line 323 in XYChart.java: The gridRenderer.setRecorder method call is added with the recorder parameter.
Line 35 in BenchPlugin.java: The measurementFilter variable is added with the rec -> rec.atLevel(BenchLevel.Info).contains("draw") lambda expression.
Line 67 in BenchPlugin.java: The stage.showingProperty listener is added to handle the case when the stage is not showing.
Line 78 in BenchPlugin.java: The setFilter method is added with the measurementFilter parameter.
Line 100 in BenchPlugin.java: The disable method is modified to hide the stage and reset the recorder.
Line 132 in AbstractRendererXY.java: The benchDrawSingle variable is changed from recorder.newDuration("xy-draw-single") to recorder.newDebugDuration("xy-draw-single").

Suggestions

In BitState.java, consider using CopyOnWriteArrayList instead of ArrayList for changeListeners and invalidateListeners to improve thread safety.
In EventSource.java, remove the unnecessary synchronized block in the removeListener method.

Bugs

In EventSource.java, there is a potential bug in the addListener method where the comment mentions handling multithreaded changes to the listener, but no actual implementation is present.

Improvements

In BitState.java, the addChangeListener and addInvalidateListener methods can be refactored for better readability by using method chaining.

public BitState addChangeListener(StateListener listener) {
   changeListeners = changeListeners != null ? changeListeners : new CopyOnWriteArrayList<>();
   changeListeners.add(listener);
   return this;
}

public BitState addInvalidateListener(StateListener listener) {
   invalidateListeners = invalidateListeners != null ? invalidateListeners : new CopyOnWriteArrayList<>();
   invalidateListeners.add(listener);
   return this;
}

Rating

Overall rating: 7.5/10 Criteria: readability, performance, security Brief explanation: The code is generally readable, but there are some areas that could be improved. The performance seems to be fine, and there are no obvious security issues.

That's it! Your Pull Request review markdown document is ready. Good luck with your review!

codecov[bot] commented 1 year ago

Codecov Report

Attention: 37 lines in your changes are missing coverage. Please review.

Comparison is base (08a8be4) 48.04% compared to head (bd5c502) 48.04%.

:exclamation: Current head bd5c502 differs from pull request most recent head ea9c852. Consider uploading reports for the commit ea9c852 to get more accurate results

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #620 +/- ## ============================================ - Coverage 48.04% 48.04% -0.01% - Complexity 6195 6198 +3 ============================================ Files 374 374 Lines 38278 38280 +2 Branches 6103 6102 -1 ============================================ - Hits 18392 18390 -2 Misses 18740 18740 - Partials 1146 1150 +4 ``` | [Files](https://app.codecov.io/gh/fair-acc/chart-fx/pull/620?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fair-acc) | Coverage Δ | | |---|---|---| | [...n/java/io/fair\_acc/dataset/events/EventSource.java](https://app.codecov.io/gh/fair-acc/chart-fx/pull/620?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fair-acc#diff-Y2hhcnRmeC1kYXRhc2V0L3NyYy9tYWluL2phdmEvaW8vZmFpcl9hY2MvZGF0YXNldC9ldmVudHMvRXZlbnRTb3VyY2UuamF2YQ==) | `64.70% <100.00%> (-3.72%)` | :arrow_down: | | [...chart/src/main/java/io/fair\_acc/chartfx/Chart.java](https://app.codecov.io/gh/fair-acc/chart-fx/pull/620?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fair-acc#diff-Y2hhcnRmeC1jaGFydC9zcmMvbWFpbi9qYXZhL2lvL2ZhaXJfYWNjL2NoYXJ0ZngvQ2hhcnQuamF2YQ==) | `78.57% <0.00%> (ø)` | | | [...io/fair\_acc/chartfx/renderer/spi/GridRenderer.java](https://app.codecov.io/gh/fair-acc/chart-fx/pull/620?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fair-acc#diff-Y2hhcnRmeC1jaGFydC9zcmMvbWFpbi9qYXZhL2lvL2ZhaXJfYWNjL2NoYXJ0ZngvcmVuZGVyZXIvc3BpL0dyaWRSZW5kZXJlci5qYXZh) | `45.19% <0.00%> (ø)` | | | [...main/java/io/fair\_acc/dataset/events/BitState.java](https://app.codecov.io/gh/fair-acc/chart-fx/pull/620?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fair-acc#diff-Y2hhcnRmeC1kYXRhc2V0L3NyYy9tYWluL2phdmEvaW8vZmFpcl9hY2MvZGF0YXNldC9ldmVudHMvQml0U3RhdGUuamF2YQ==) | `55.41% <50.00%> (ø)` | | | [...r\_acc/chartfx/renderer/spi/AbstractRendererXY.java](https://app.codecov.io/gh/fair-acc/chart-fx/pull/620?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fair-acc#diff-Y2hhcnRmeC1jaGFydC9zcmMvbWFpbi9qYXZhL2lvL2ZhaXJfYWNjL2NoYXJ0ZngvcmVuZGVyZXIvc3BpL0Fic3RyYWN0UmVuZGVyZXJYWS5qYXZh) | `87.71% <60.00%> (ø)` | | | [...art/src/main/java/io/fair\_acc/chartfx/XYChart.java](https://app.codecov.io/gh/fair-acc/chart-fx/pull/620?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fair-acc#diff-Y2hhcnRmeC1jaGFydC9zcmMvbWFpbi9qYXZhL2lvL2ZhaXJfYWNjL2NoYXJ0ZngvWFlDaGFydC5qYXZh) | `58.13% <0.00%> (-0.46%)` | :arrow_down: | | [...fair\_acc/chartfx/utils/SimplePerformanceMeter.java](https://app.codecov.io/gh/fair-acc/chart-fx/pull/620?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fair-acc#diff-Y2hhcnRmeC1jaGFydC9zcmMvbWFpbi9qYXZhL2lvL2ZhaXJfYWNjL2NoYXJ0ZngvdXRpbHMvU2ltcGxlUGVyZm9ybWFuY2VNZXRlci5qYXZh) | `86.92% <55.55%> (-0.77%)` | :arrow_down: | | [.../java/io/fair\_acc/chartfx/plugins/BenchPlugin.java](https://app.codecov.io/gh/fair-acc/chart-fx/pull/620?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fair-acc#diff-Y2hhcnRmeC1jaGFydC9zcmMvbWFpbi9qYXZhL2lvL2ZhaXJfYWNjL2NoYXJ0ZngvcGx1Z2lucy9CZW5jaFBsdWdpbi5qYXZh) | `0.00% <0.00%> (ø)` | | ... and [4 files with indirect coverage changes](https://app.codecov.io/gh/fair-acc/chart-fx/pull/620/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fair-acc)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.