Skip to content

Conversation

@frigoref
Copy link
Member

@frigoref frigoref commented Aug 10, 2025

Fix for issue #13608 by mainly
Extract StatisticsAggregator.cloneGameData to GameDataUtils.java as GameDataUtils.cloneGameDataWithHistory including the option to enableSeeking

ExportMenu.java / StatisticsAggregator.java

  • Use new method with enableSeeking = true

TripleAFrame.java

  • method showHistory: Use new method with enableSeeking = false as it is called later if needed

Notes to Reviewer

  1. PR Fix13608 IllegalStateException(gotoNode) 2 #13619 is also containing a rework of the export menu code that could be checked together to speedup the review process.
  2. It is a bit odd that the field History.seekingEnabled is checked and can cause dumps if it has the wrong value. I didn't understand why exactly, so I kept it, but maybe we should change this to call method enableSeeking everytime this flag should be true in the future.

Extract StatisticsAggregator.cloneGameData to GameDataUtils.java as GameDataUtils.cloneGameDataWithHistory including the option to enableSeeking

ExportMenu.java / StatisticsAggregator.java
- Use new method with enableSeeking = true

TripleAFrame.java
- method showHistory: Use new method with enableSeeking = false as it is called later if needed
cleanup with spotlessJavaApply
Avoid FileExistsException by ensuring empty directory folder and avoiding to use Files.createDirectory

SetupFrame.java
- new method verifySelectedOutIsEmpty
Fix pmdMain for "Empty catch block"
@DanVanAtta
Copy link
Member

@frigoref could you spell out more clearly what/where the fix is? Also, it's really important to not mix clean-ups, improvements, rewrites, overhauls with code fixes. If there are any problems outside of the code fix, then we now have errors in the fixes. Rolling back those errors then rolls back fixes. For that reason, let's try to keep fixes relatively atomic and precise, so that there is little reason to ever roll them back.

@DanVanAtta
Copy link
Member

It is a bit odd that the field History.seekingEnabled is checked and can cause dumps if it has the wrong value. I didn't understand why exactly, so I kept it, but maybe we should change this to call method enableSeeking everytime this flag should be true in the future.

To actually fix this problem, I think we need to dig in here. Otherwise seemingly we are adding patching around something that is not understood. If someone else actually wants to try and fix this, the patching I believe will add yet more confusion.

@DanVanAtta
Copy link
Member

@asvitkine can you comment on what 'enableSeeking' is?

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.

3 participants