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

Samples: fix non-terminating samples #600

Merged
merged 4 commits into from
Aug 18, 2023
Merged

Samples: fix non-terminating samples #600

merged 4 commits into from
Aug 18, 2023

Conversation

wirew0rm
Copy link
Member

There are mainly 3 reasons for the samples not to terminate:

  • SimplePerformanceMeter (and the ProfilerInfoBox based on it) This class registers some javafx hooks which will cause the application to fail to terminate. For now it is removed from the samples, but should be repaired before merging this
  • PaddedGrowAxis used a non deamonised Updateer thread
  • The Midi based dataset for the Waterfall plot causes problems commented out for now but should be repaired before merge
  • Histogram2DimSample called runFx from a javafx timer, after removing that, it terminates correctly

followup of #571, see there for further context

@wirew0rm wirew0rm temporarily deployed to coverage August 11, 2023 19:17 — with GitHub Actions Inactive
@wirew0rm wirew0rm temporarily deployed to coverage August 11, 2023 19:17 — with GitHub Actions Inactive
@wirew0rm wirew0rm temporarily deployed to coverage August 11, 2023 19:17 — with GitHub Actions Inactive
@pr-explainer-bot
Copy link

Pull Request Review Markdown

Hey there! 👋 Here's a summary of the previous tasks and their results. Let's get started!

Changes

  1. Removed the ProfilerInfoBox from the getHeaderBar method in ChartIndicatorSample.java.
  2. Removed the ProfilerInfoBox from the getUserToolBarItems method in DataViewerSample.java.
  3. Removed the ProfilerInfoBox from the getHeaderBar method in ErrorDataSetRendererSample.java.
  4. Removed the ProfilerInfoBox from the getHeaderBar method in ErrorDataSetRendererStylingSample.java.
  5. Removed the ProfilerInfoBox from the getHeaderBar method in OscilloscopeAxisSample.java.

Suggestions

  1. In ChartIndicatorSample.java, lines 114-115, the ProfilerInfoBox is commented out. It seems like it was temporarily disabled.
  2. In DataViewerSample.java, line 142, the ProfilerInfoBox is commented out. It seems like it was temporarily disabled.
  3. In ErrorDataSetRendererSample.java, lines 66-67, the ProfilerInfoBox is commented out. It seems like it was temporarily disabled.
  4. In ErrorDataSetRendererStylingSample.java, lines 264-265, the ProfilerInfoBox is commented out. It seems like it was temporarily disabled.
  5. In OscilloscopeAxisSample.java, lines 192-193, the SimplePerformanceMeter is commented out. It seems like it was temporarily disabled.

Bugs

There are no potential bugs found in the code.

Improvements

  1. In PaddedAutoGrowAxisSample.java, lines 58-61, the code...
  2. In RollingBufferSample.java, lines 130-133, the ProfilerInfoBox instance is commented out. It seems like it was used for debugging purposes. Consider removing the commented code to improve code cleanliness.
  3. In RollingBufferSortedTreeSample.java, lines 95-97, the SimplePerformanceMeter instance is commented out. It seems like it was used for performance monitoring. Consider removing the commented code to improve code cleanliness.
  4. In RollingBufferLegacySample.java, lines 78-80, the SimplePerformanceMeter instance is commented out. It seems like it was used for performance monitoring. Consider removing the commented code to improve code cleanliness.
  5. In AbstractBasicFinancialApplication.java, lines 159-161, the ProfilerInfoBox instance is commented out. It seems like it was used for debugging purposes. Consider removing the commented code to improve code cleanliness.

Rating

Please rate the code from 0 to 10 based on the following criteria:

  • Readability
  • Performance
  • Security

Feel free to provide a brief explanation for your rating.

That's it for the summary! If you have any questions or need further assistance, feel free to ask. Good luck with your pull request! 🚀

@wirew0rm wirew0rm temporarily deployed to deploy August 11, 2023 19:19 — with GitHub Actions Inactive
@wirew0rm wirew0rm temporarily deployed to deploy August 11, 2023 19:21 — with GitHub Actions Inactive
@codecov
Copy link

codecov bot commented Aug 11, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.04% 🎉

Comparison is base (ca15519) 49.62% compared to head (cea6e52) 49.66%.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #600      +/-   ##
============================================
+ Coverage     49.62%   49.66%   +0.04%     
- Complexity     6236     6242       +6     
============================================
  Files           379      379              
  Lines         37593    37579      -14     
  Branches       6159     6157       -2     
============================================
+ Hits          18654    18663       +9     
+ Misses        17710    17692      -18     
+ Partials       1229     1224       -5     
Files Changed Coverage Δ
...n/java/io/fair_acc/chartfx/ui/ProfilerInfoBox.java 98.78% <100.00%> (+18.57%) ⬆️
...fair_acc/chartfx/utils/SimplePerformanceMeter.java 89.06% <100.00%> (ø)

... and 3 files with indirect coverage changes

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

@wirew0rm wirew0rm temporarily deployed to coverage August 17, 2023 18:12 — with GitHub Actions Inactive
@wirew0rm wirew0rm temporarily deployed to coverage August 17, 2023 18:12 — with GitHub Actions Inactive
@wirew0rm wirew0rm temporarily deployed to coverage August 17, 2023 18:12 — with GitHub Actions Inactive
@wirew0rm wirew0rm temporarily deployed to coverage August 17, 2023 18:13 — with GitHub Actions Inactive
@wirew0rm wirew0rm temporarily deployed to coverage August 17, 2023 18:13 — with GitHub Actions Inactive
@wirew0rm wirew0rm temporarily deployed to coverage August 17, 2023 18:13 — with GitHub Actions Inactive
@wirew0rm wirew0rm marked this pull request as ready for review August 17, 2023 18:13
@wirew0rm wirew0rm requested a review from ennerf August 17, 2023 18:13
@wirew0rm wirew0rm temporarily deployed to deploy August 17, 2023 18:16 — with GitHub Actions Inactive
@wirew0rm wirew0rm temporarily deployed to deploy August 17, 2023 18:16 — with GitHub Actions Inactive
Moved the property updates into the already existing animation timer.
Also fixes the ProfilerInfoBox by adding the meter to a visible node.
needed to call the close method on various midi resources.
Still not terminating reliably, but at least it now only blocks
termination if the sample was shown. I guess we can live with that for
now.
@wirew0rm wirew0rm temporarily deployed to coverage August 18, 2023 11:26 — with GitHub Actions Inactive
@wirew0rm wirew0rm temporarily deployed to coverage August 18, 2023 11:26 — with GitHub Actions Inactive
@wirew0rm wirew0rm temporarily deployed to coverage August 18, 2023 11:26 — with GitHub Actions Inactive
@wirew0rm wirew0rm temporarily deployed to coverage August 18, 2023 11:26 — with GitHub Actions Inactive
@wirew0rm wirew0rm temporarily deployed to coverage August 18, 2023 11:26 — with GitHub Actions Inactive
@wirew0rm wirew0rm temporarily deployed to coverage August 18, 2023 11:26 — with GitHub Actions Inactive
@sonarcloud
Copy link

sonarcloud bot commented Aug 18, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@wirew0rm wirew0rm temporarily deployed to deploy August 18, 2023 11:30 — with GitHub Actions Inactive
@wirew0rm wirew0rm temporarily deployed to deploy August 18, 2023 11:30 — with GitHub Actions Inactive
@wirew0rm wirew0rm merged commit 33be6ac into main Aug 18, 2023
12 checks passed
@wirew0rm wirew0rm deleted the fixSampleTerminate branch August 18, 2023 11:44
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