Skip to content

Reduce NullCommand nesting on the pausedCommands deque#14391

Merged
uckelman merged 2 commits intovassalengine:release-3.7from
rlament:nullcommand-nesting
Apr 23, 2026
Merged

Reduce NullCommand nesting on the pausedCommands deque#14391
uckelman merged 2 commits intovassalengine:release-3.7from
rlament:nullcommand-nesting

Conversation

@rlament
Copy link
Copy Markdown
Contributor

@rlament rlament commented Dec 10, 2025

While debugging another issue I noticed nesting of NullCommands. This is inefficient. The nesting means there are recursive descents for various methods.

The idiom for appending sub-commands to a command that may be a NullCommand is:
command = command.append(subcommand)

The assignment turns the append into a replace when the command is a NullCommand. When the command is guaranteed not to be NullCommand, the assignment is not necessary.

This pull request pops the top element from the deque, appends the sub-command with an assignment and then pushes it back on the deque.

On this before image note the 5 NullCommands.
nullcommand_before

After only 1 NullCommand remains.
nullcommand_after

This minimizes the nesting of NullCommands on the deque.
@riverwanderer riverwanderer added this to the 3.7.20 milestone Jan 11, 2026
@riverwanderer
Copy link
Copy Markdown
Collaborator

Tagged this PR for next release; is anything holding it back ?

@uckelman
Copy link
Copy Markdown
Contributor

There aren't any tests demonstrating that behavior hasn't changed.

@rlament
Copy link
Copy Markdown
Contributor Author

rlament commented Jan 12, 2026

Fair enough. I've added a unit test. It is a simpler example than the captured images show. Run this test on the existing code with a breakpoint at the end. Inspecting the command variable should show two NullCommands and a DisplayCommand. Note the nesting. With the change there should be a most one NullCommand regardless of the number of commands generated by DoActionButton.

@uckelman uckelman modified the milestones: 3.7.20, 3.7.21 Feb 6, 2026
@uckelman uckelman removed this from the 3.7.21 milestone Apr 9, 2026
@uckelman uckelman added this to the 3.7.22 milestone Apr 23, 2026
@uckelman uckelman merged commit 4b6c776 into vassalengine:release-3.7 Apr 23, 2026
3 checks passed
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