Fix13608 IllegalStateException(gotoNode) 2 #13619
Conversation
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
Extract superclass InfoForFile with common - file name - delimiter, line separator - method saveToFile
Ensure the cloning of gameData for files generated through the Export Menu is done asynchronously
- new method chooseExportFileWithDefaultName
- StatsInfo.java is an extraction from method ExportMenu.createAndSaveStats - Further simplifications are done to choose the export file and this logic is reused for method exportUnitCharts
- ensure usage of constants for delimiter and line separator - use append instead of write - ensure number of delimiters per line = 1 + #UnitTypes
…mber of delimiters)
- allow multiple player names per circle
|
@frigoref this is a 1000 line diff review with 29 commits. We need to discuss a strategy for how to get something like this merged. |
| * @param defaultFileName default name of the file | ||
| * @return path to the chosen file | ||
| */ | ||
| public static Optional<Path> chooseExportFileWithDefaultName( |
There was a problem hiding this comment.
Keep in mind scopes. Swing-lib should not know anything about TripleA concepts, it should not know what an "export file" is. This method belongs closer to where it's used.
There was a problem hiding this comment.
Maybe the name is too specific for "export file" and it should refer more to the generic description in the documentation.
What do you think about chooseFileWithDefaultNameFromUserDirectory?
| public static Optional<GameData> cloneGameDataWithHistory( | ||
| GameData gameData, boolean enableSeeking) { | ||
| final var cloneOptions = GameDataManager.Options.builder().withHistory(true).build(); | ||
| Optional<GameData> optionalGameDataClone = cloneGameData(gameData, cloneOptions); |
There was a problem hiding this comment.
I don't think you needed to have added another method signature to hide the "parameter type" Options
The existing pattern would allow for something like this:
GameDataManager.Options.builder().withHistory(true).withSeeking(true).build();
Using the Options parameter has some strengths.
- The "boolean" parameter presents a poor API, a usability issue (which we do mention to avoid in the java-conventions)
- If we need a case where there is no history copy but we want seeking, we would have to do contortions to have the right set of parameters and method names.
Nit: methods that provide overloaded signatures should be grouped together. Though, this method probably should not exist, instead enhance the existing pattern and it'll be a better API and hopefully less overall code too.
There was a problem hiding this comment.
That sounds good. Shall I provide another commit for this then?
| final var cloneOptions = GameDataManager.Options.builder().withHistory(true).build(); | ||
| Optional<GameData> optionalGameDataClone = cloneGameData(gameData, cloneOptions); | ||
| if (enableSeeking) { | ||
| optionalGameDataClone.ifPresent(clone -> clone.getHistory().enableSeeking(null)); |
There was a problem hiding this comment.
After all that work to help with null-safety, and now we're pro-actively passing a null value!
enableSeeking accepting a historyPanel for a glorified set operation makes no sense. I really have no clue what "enableSeeking" means and why the historyPanel could be null. Having HistoryPanel in the API doesn't make sense.
There was a problem hiding this comment.
I agree that we should try to avoid passing null where possible, but according to the code it is possible and therefore I do not see why this should be an issue here. Could you elaborate?
My idea would be to annotated the parameter as well as the member that is assigned with the parameter as Nullable.
Concerning enableSeeking I did not want to break anything, but I didn't fully understand its usage either. To me it seemed to be related to nextChangeIndex in comment before their declarations:
// Index at which point we are in history. Only valid if seekingEnabled is true.
private int nextChangeIndex;
private boolean seekingEnabled = false;| .orElseThrow( | ||
| () -> | ||
| new IllegalStateException( | ||
| "Cloning game data for StatisticsAggregator failed.")); |
There was a problem hiding this comment.
This is not enough information to present to a user. We need to capture more. Generally the user should be made aware of:
- what happened
- what the impact is
- and what they can do about it
I think we need to look at the API for cloning game data history objects and how errors are handled in detail.
The existing code is really bad:
final GameData clone = GameDataUtils.cloneGameData(gameData, cloneOptions).orElse(null);
A stack trace that has a static message in it is a slight improvement over NullPointerExceptoin.. but not much.
There was a problem hiding this comment.
You are right, but we cannot save the world in a big bang. A slight improvement is better than no improvement and to me IllegalStateExceptions are rarely helpful for the user, but for some developer analyzing the issue.
|
Slice in PR #13728 merged |
2 participants
Similar to PR #13615, but including also a rework on the export menu.
Notes to Reviewer
This is a major rework with several smaller commits.
In case reviewing all together is faster, I submit this PR. It can be sliced in individual PRs based on the commits if preferred.