New logfile cleanup by bacota · Pull Request #14535 · vassalengine/vassal

bacota · 2026-03-04T23:59:40Z

No description provided.

Co-authored-by: bacota <2348796+bacota@users.noreply.github.com>

Copilot

Pull request overview

Adds a new “clean logfile” feature intended to produce a replayable .vlog without undo operations, and wires it into the BasicLogger UI/menu.

Changes:

Add LogCleaner utility and a new BasicLogger action/menu item to clean .vlog files.
Add new i18n strings for the cleaner UI and messages.
Modify build/release configuration and scripts (Maven modules/plugins, launcher script, Makefile packaging).

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
vassal-app/src/main/resources/VASSAL/i18n/VASSAL.properties	Adds UI strings for the new logfile cleaning action.
vassal-app/src/main/java/VASSAL/build/module/LogCleaner.java	Introduces the log-cleaning implementation for .vlog files.
vassal-app/src/main/java/VASSAL/build/module/BasicLogger.java	Adds a menu action to select an input .vlog and write a cleaned output .vlog.
vassal-app/pom.xml	Comments out multiple build-quality plugins (checkstyle/javadoc/spotbugs).
pom.xml	Comments out the `vassal-doc` module from the Maven reactor.
dist/VASSAL.sh	Changes launcher behavior (shebang, disables Java version check, hard-codes JAVA path).
Makefile	Changes Linux packaging target behavior (no longer produces the tarball).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-05T00:06:06Z

@@ -369,7 +371,7 @@
                            </script>
                        ]]>
                    </bottom>
-                    <additionalJOption>--allow-script-in-comments</additionalJOption>
+                    <additionalJOption>allow-script-in-comments</additionalJOption>
                    <doclint>none</doclint>
                </configuration>
                <executions>
@@ -380,9 +382,10 @@
                        </goals>
                    </execution>
                </executions>
-            </plugin>
+                </plugin>
+                -->
+              <!-- 
            <plugin>
-              <!-- This does not work with Java 25 -->
                <groupId>com.github.spotbugs</groupId>
                <artifactId>spotbugs-maven-plugin</artifactId>
                <configuration>
@@ -399,8 +402,10 @@
                            <goal>check</goal>
                        </goals>
                    </execution>
-                </executions>
-            </plugin>
+                    </executions>
+                    </plugin>
+                    -->
+


The maven-javadoc-plugin and spotbugs-maven-plugin blocks are being commented out here. That weakens verification in the app module and appears unrelated to the log-cleaning feature; it can also mask real issues in CI. Please restore these plugins (or move/justify the change in a separate PR).

Copilot · 2026-03-05T00:06:07Z

+BasicLogger.clean_logfile_success=Successfully cleaned log file: {0}
+BasicLogger.clean_logfile_failed=Failed to clean log file: {0}


These new user-facing strings mix "Logfile" (one word) with "log file" (two words). The surrounding BasicLogger strings consistently use "Logfile", so it would be more consistent to use the same spelling/capitalization in the success/failure messages.

Suggested change

BasicLogger.clean_logfile_success=Successfully cleaned log file: {0}

BasicLogger.clean_logfile_failed=Failed to clean log file: {0}

BasicLogger.clean_logfile_success=Successfully cleaned Logfile: {0}

BasicLogger.clean_logfile_failed=Failed to clean Logfile: {0}

Copilot · 2026-03-05T00:06:07Z

+  private static Command cleanLog(Command logCommand) {
+    // Get the direct subcommands (which should be LogCommand objects and UndoCommand markers)
+    final Command[] subCommands = logCommand.getSubCommands();
+
+    // Track which commands to remove
+    final boolean[] toRemove = new boolean[subCommands.length];
+
+    // Find and mark undo operations
+    for (int i = 0; i < subCommands.length; i++) {
+      final Command cmd = subCommands[i];
+
+      // Check if this is an UndoCommand(true) - start of undo
+      if (cmd instanceof BasicLogger.UndoCommand) {
+        final BasicLogger.UndoCommand undoCmd = (BasicLogger.UndoCommand) cmd;
+        if (undoCmd.isInProgress()) {
+          // Found start of undo - mark it for removal
+          toRemove[i] = true;
+
+          // Find the matching UndoCommand(false)
+          int undoEnd = -1;
+          for (int j = i + 1; j < subCommands.length; j++) {
+            final Command endCmd = subCommands[j];
+            if (endCmd instanceof BasicLogger.UndoCommand) {
+              final BasicLogger.UndoCommand endUndoCmd = (BasicLogger.UndoCommand) endCmd;
+              if (!endUndoCmd.isInProgress()) {
+                undoEnd = j;
+                break;
+              }
+            }
+          }
+
+          if (undoEnd != -1) {
+            // Mark the end undo command for removal
+            toRemove[undoEnd] = true;
+
+            // Mark all commands between start and end for removal (the undone action)
+            for (int j = i + 1; j < undoEnd; j++) {
+              toRemove[j] = true;
+            }
+
+            // Mark the command before the undo start (the original command being undone)
+            // Need to find the previous LogCommand
+            for (int j = i - 1; j >= 0; j--) {
+              if (subCommands[j] instanceof BasicLogger.LogCommand) {
+                toRemove[j] = true;
+                break;
+              }
+            }


cleanLog() looks for BasicLogger.UndoCommand directly in logCommand.getSubCommands(), but in .vlog files undo markers are encoded as the logged command inside BasicLogger.LogCommand wrappers (see BasicLogger.write()/LogCommand). As a result, this cleaner will not remove any undo operations in real logs. Consider iterating over the LogCommand sequence and inspecting each LogCommand’s getLoggedCommand() (and/or assembling commands) to detect undo operations, instead of checking the raw root subcommands.

Copilot · 2026-03-05T00:06:07Z

+    // Rebuild command tree without removed commands
+    // The root command is the beginning state (restore command), we want to preserve it
+    // but with only the cleaned LogCommand subcommands
+    Command cleanedRoot = logCommand;
+
+    // Clear the existing subcommands by creating a new root of the same type
+    // For simplicity, we'll just append to a NullCommand which will work for encoding
+    cleanedRoot = new NullCommand();
+
+    // Append only the commands we want to keep
+    for (int i = 0; i < subCommands.length; i++) {
+      if (!toRemove[i]) {
+        cleanedRoot.append(subCommands[i]);
+      }
+    }
+


cleanLog() replaces the decoded root restore command with new NullCommand(), which drops the save-game restore sequence (SetupCommand/version/pieces/components). A cleaned log without the restore command is unlikely to replay correctly. The cleaner should preserve the original restore command (and its non-log subcommands) and only remove/adjust the appended logged move commands.

Suggested change

// Rebuild command tree without removed commands

// The root command is the beginning state (restore command), we want to preserve it

// but with only the cleaned LogCommand subcommands

Command cleanedRoot = logCommand;

// Clear the existing subcommands by creating a new root of the same type

// For simplicity, we'll just append to a NullCommand which will work for encoding

cleanedRoot = new NullCommand();

// Append only the commands we want to keep

for (int i = 0; i < subCommands.length; i++) {

if (!toRemove[i]) {

cleanedRoot.append(subCommands[i]);

}

}

// Rebuild command chain without removed commands

// The root command is the beginning state (restore command); we must preserve it

// and only adjust its subsequent (logged) subcommands.

Command cleanedRoot = logCommand;

// Build a new chain of kept subcommands

Command firstKept = null;

Command lastKept = null;

for (int i = 0; i < subCommands.length; i++) {

if (!toRemove[i]) {

final Command kept = subCommands[i];

if (firstKept == null) {

firstKept = kept;

lastKept = kept;

}

else {

lastKept.append(kept);

lastKept = kept;

}

}

}

// Terminate the chain explicitly to avoid dangling old references

if (lastKept != null) {

lastKept.setNext(null);

}

// Attach the cleaned chain to the original root restore command

cleanedRoot.setNext(firstKept);

Copilot · 2026-03-05T00:06:08Z

+
+    // Extract metadata - cast to SaveMetaData as that's what .vlog files use
+    final SaveMetaData metadata = (SaveMetaData) MetaDataFactory.buildMetaData(inputFile);
+
+    // Clean the log
+    final Command cleanedCommand = cleanLog(logCommand);
+


MetaDataFactory.buildMetaData(inputFile) can return null (non-zip/invalid) or a different AbstractMetaData subtype; the unchecked cast to SaveMetaData can throw ClassCastException/NullPointerException and will not be caught by the caller (which only catches IOException). Please validate the return type and throw an IOException (with a helpful message) when the input is not a valid .vlog/save file.

Suggested change

// Extract metadata - cast to SaveMetaData as that's what .vlog files use

final SaveMetaData metadata = (SaveMetaData) MetaDataFactory.buildMetaData(inputFile);

// Clean the log

final Command cleanedCommand = cleanLog(logCommand);

// Extract metadata - ensure we have a SaveMetaData as that's what .vlog files use

final Object metaDataObject = MetaDataFactory.buildMetaData(inputFile);

if (!(metaDataObject instanceof SaveMetaData)) {

throw new IOException("File '" + inputFile + "' is not a valid VASSAL save/log file.");

}

final SaveMetaData metadata = (SaveMetaData) metaDataObject;

// Clean the log

final Command cleanedCommand = cleanLog(logCommand);

Copilot · 2026-03-05T00:06:08Z

    <modules>
        <module>vassal-app</module>
        <module>vassal-deprecation</module>
-        <module>vassal-doc</module>
+        <!--
+            <module>vassal-doc</module>
+            -->
        <module>release-prepare</module>


This PR is titled/logically focused on log-file cleanup, but it also comments out the vassal-doc Maven module. Removing modules from the reactor affects CI and release builds and seems unrelated to the feature change. Please revert this (or move it to a dedicated build/CI PR with rationale).

Copilot · 2026-03-05T00:06:08Z

+            <!--
            <plugin>
                <artifactId>maven-checkstyle-plugin</artifactId>
                <configuration>
                    <configLocation>src/test/resources/checkstyle-checks.xml</configLocation>
                    <suppressionsLocation>src/test/resources/checkstyle-suppressions.xml</suppressionsLocation>
                    <logViolationsToConsole>true</logViolationsToConsole>
-                    <!-- consoleOutput=true prints warnings in addition to errors -->
                    <consoleOutput>true</consoleOutput>


The maven-checkstyle-plugin configuration is now commented out. Disabling static checks in vassal-app changes build quality gates and seems unrelated to adding log cleaning functionality. Please revert this change (or justify it in a separate build-focused PR).

Copilot · 2026-03-05T00:06:08Z

+      }
+    }
+
+    throw new IOException("Invalid log file format");


The error text "Invalid log file format" is hard-coded and loses context about which file failed. Since this error is surfaced via the UI action, please include the file name/path and use the i18n Resources mechanism (or at least a more specific message) so users can diagnose failures.

Suggested change

throw new IOException("Invalid log file format");

throw new IOException("Invalid log file format: " + logFile);

Copilot · 2026-03-05T00:06:09Z

+        WriteErrorDialog.error(ex, outputFile);
+        GameModule.getGameModule().warn(
+          Resources.getString("BasicLogger.clean_logfile_failed", ex.getMessage()));  //$NON-NLS-1$
+      }


LogCleaner.cleanLogFile() can currently throw unchecked exceptions (e.g., ClassCastException from metadata casting / NullPointerException from invalid metadata) which won’t be caught here because the handler only catches IOException. To avoid crashing the EDT, either make LogCleaner reliably wrap validation failures in IOException, or broaden this catch to handle RuntimeException and report a user-friendly error.

Suggested change

}

}

catch (RuntimeException ex) {

WriteErrorDialog.error(ex, outputFile);

GameModule.getGameModule().warn(

Resources.getString("BasicLogger.clean_logfile_failed", ex.getMessage())); //$NON-NLS-1$

}

Copilot · 2026-03-05T00:06:09Z

+        GameModule.getGameModule().warn(
+          Resources.getString("BasicLogger.clean_logfile_failed", ex.getMessage()));  //$NON-NLS-1$


The failure warning currently inserts ex.getMessage() into BasicLogger.clean_logfile_failed, but that message can be null and doesn’t identify which file failed. Consider reporting the input/output file path(s) in the warning and logging the exception separately (the detailed error is already shown via WriteErrorDialog).

Suggested change

GameModule.getGameModule().warn(

Resources.getString("BasicLogger.clean_logfile_failed", ex.getMessage())); //$NON-NLS-1$

final String fileInfo = inputFile.getPath() + " -> " + outputFile.getPath();

GameModule.getGameModule().warn(

Resources.getString("BasicLogger.clean_logfile_failed", fileInfo)); //$NON-NLS-1$

This reverts commit 52d9a55.

rlament · 2026-03-07T03:37:41Z

Trying this on my log file did not remove the undo commands. The scan needs to do a recursive descent while keeping track of the levels to match up the undo's with commands. My sample log file is below (vlogs are zips so just change the extension back to vlog). It should load in any module. It's from a scrap module used for test purposes. Ignore the game mismatch and the Bad Data messages. It contains a series of chat messages mixed with some undo's. The chat messages are the easiest way to test undo/redo. Stepping through the log file looks like this:

<ray> - A
<ray> - B
<ray> - C
* UNDO: <ray> - C
* UNDO: <ray> - B
* UNDO: <ray> - A
<ray> - D
<ray> - E
<ray> - F
* UNDO: <ray> - F
<ray> - G

It seems you have pushed some files that are specific to your build environment. In this PR there should only be the java files and VASSAL.properties.

I edit the SKIPS line in the Makefile and it builds (release-linux) without changing any other configuration files.
SKIPS:=-Dasciidoctor.skip=true -Dlicense.skipDownloadLicenses

Did you forget to push PlayerWindow.java? I didn't see the fileMenu.add(mm.addKey("BasicLogger.clean_logfile")) to add the clean command to the menu.

minimal_module4.zip

rlament · 2026-03-07T03:44:59Z

+    // Append only the commands we want to keep
+    for (int i = 0; i < subCommands.length; i++) {
+      if (!toRemove[i]) {
+        cleanedRoot.append(subCommands[i]);


Use assignment to get rid of the top level NullCommand.
cleanedRoot = cleanedRoot.append(subCommands[i]);

Copilot's solution is far too complicated.

I wonder whether this PR is desirable from the viewpoint of facilitating cheating in PBEM games, or to put it more kindly, presenting a temptation to those who having a run of bad luck.

I know that this is a controversial topic but it is not the only example of Vassal's design relying on obscurity to keep players on the straight and narrow.

Assuming the legitimate use-case warrants the PR, perhaps some mitigation can be considered. Eg insert a warning into the log when undone actions are removed. Such as a summary at the end of each block of undos.

Fair PBEM involves considerable faith and trust in opponents' honesty. I had the similar thought about making it easier to redo moves. As it is there are other ways to redo for a better outcome particularily in the PBEM setting.

How often are there legitimate corrections in log files? It seems that this would only be an occasional occurrence. Although in situations where there are multiple overlapping undo/redo steps it can be difficult to follow.

Perhaps a better approach is not writing the output to a file. Instead, filter out the undo commands on playback. Effectively moving the filter from the producer to the consumer. A preference setting could enable this feature. The chat log could output a "skipping undo" message in place of the do/undo block.

Copilot AI and others added 5 commits February 18, 2026 17:56

Initial plan

9208104

Add LogCleaner class and menu integration

679c219

Co-authored-by: bacota <2348796+bacota@users.noreply.github.com>

Optimize cleanLog to work directly with subcommands

fc8244e

Co-authored-by: bacota <2348796+bacota@users.noreply.github.com>

mandatory style fixes

6727c7b

build fixes for bruce

efcdbc9

Copilot AI review requested due to automatic review settings March 4, 2026 23:59

Copilot started reviewing on behalf of bacota March 5, 2026 00:00 View session

Copilot AI reviewed Mar 5, 2026

View reviewed changes

bacota and others added 2 commits March 4, 2026 19:15

Update BasicLogger.java to implement buffered logging

52d9a55

Revert "Update BasicLogger.java to implement buffered logging"

dddc510

This reverts commit 52d9a55.

rlament reviewed Mar 7, 2026

View reviewed changes

		BasicLogger.clean_logfile_success=Successfully cleaned log file: {0}
		BasicLogger.clean_logfile_failed=Failed to clean log file: {0}

-    // Rebuild command tree without removed commands
-    // The root command is the beginning state (restore command), we want to preserve it
-    // but with only the cleaned LogCommand subcommands
-    Command cleanedRoot = logCommand;
-    // Clear the existing subcommands by creating a new root of the same type
-    // For simplicity, we'll just append to a NullCommand which will work for encoding
-    cleanedRoot = new NullCommand();
-    // Append only the commands we want to keep
-    for (int i = 0; i < subCommands.length; i++) {
-      if (!toRemove[i]) {
-        cleanedRoot.append(subCommands[i]);
-      }
-    }
+    // Rebuild command chain without removed commands
+    // The root command is the beginning state (restore command); we must preserve it
+    // and only adjust its subsequent (logged) subcommands.
+    Command cleanedRoot = logCommand;
+    // Build a new chain of kept subcommands
+    Command firstKept = null;
+    Command lastKept = null;
+    for (int i = 0; i < subCommands.length; i++) {
+      if (!toRemove[i]) {
+        final Command kept = subCommands[i];
+        if (firstKept == null) {
+          firstKept = kept;
+          lastKept = kept;
+        }
+        else {
+          lastKept.append(kept);
+          lastKept = kept;
+        }
+      }
+    }
+    // Terminate the chain explicitly to avoid dangling old references
+    if (lastKept != null) {
+      lastKept.setNext(null);
+    }
+    // Attach the cleaned chain to the original root restore command
+    cleanedRoot.setNext(firstKept);

	throw new IOException("Invalid log file format");
	throw new IOException("Invalid log file format: " + logFile);

		GameModule.getGameModule().warn(
		Resources.getString("BasicLogger.clean_logfile_failed", ex.getMessage())); //$NON-NLS-1$

Uh oh!

Conversation

bacota commented Mar 4, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

rlament commented Mar 7, 2026

Uh oh!

rlament Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

riverwanderer Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

rlament Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants