Skip to content

Fix ghosts being left behind when reopening Terraform window with a keyboard shortcut#2974

Closed
LeeSpork wants to merge 2 commits intoOpenLoco:masterfrom
LeeSpork:2741-fix
Closed

Fix ghosts being left behind when reopening Terraform window with a keyboard shortcut#2974
LeeSpork wants to merge 2 commits intoOpenLoco:masterfrom
LeeSpork:2741-fix

Conversation

@LeeSpork
Copy link
Copy Markdown
Contributor

@LeeSpork LeeSpork commented Mar 2, 2025

Fixes #2741

I moved the open() function because:

  1. it needed to be defined after BuildWalls::removeWallGhost() for my changes to compile
  2. Now all the functions in the Terraform namespace are together, instead of this one function being randomly wedged between the PlantTrees and ClearArea namespaces for no apparent reason.

LeeSpork
LeeSpork commented Mar 2, 2025
View reviewed changes
src/OpenLoco/src/Ui/Windows/TerraForm.cpp
PlantTrees::removeTreeGhost();
BuildWalls::removeWallGhost();

window->callOnMouseUp(Common::widx::tab_plant_trees);
Copy link
Copy Markdown
Contributor Author

@LeeSpork LeeSpork Mar 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling window->callOnMouseUp here seems redundant. The callers of open() already call this themselves! I didn't feel brave enough to remove it in this PR though.

@duncanspumpkin
Copy link
Copy Markdown
Contributor

duncanspumpkin commented Mar 3, 2025

I think we could just add a tool abort that removes the ghosts.

@LeeSpork
Copy link
Copy Markdown
Contributor Author

LeeSpork commented Mar 4, 2025 •
edited
Loading

I think we could just add a tool abort that removes the ghosts.

I guess one could write abort functions for each terraform tool, though I believe the position where I added lines in Terraform::open() is the only place where you could reasonably detect the tool being "aborted" in this way.

i.e. in place of lines +2891 to +2892 in this PR, I think you would probably have switch (window->currentTab) {...}

@duncanspumpkin
Copy link
Copy Markdown
Contributor

duncanspumpkin commented Mar 4, 2025

I think we could just add a tool abort that removes the ghosts.

I guess one could write abort functions for each terraform tool, though I believe the position where I added lines in Terraform::open() is the only place where you could reasonably detect the tool being "aborted" in this way.

i.e. in place of lines +2891 to +2892 in this PR, I think you would probably have switch (window->currentTab) {...}

A tool abort is automatically called on tab change that's why i think it would be simpler

@AaronVanGeffen
Copy link
Copy Markdown
Member

AaronVanGeffen commented May 6, 2025

I think we could just add a tool abort that removes the ghosts.

I agree this would be the way to go for this.

@LeeSpork LeeSpork mentioned this pull request Oct 9, 2025
Merged
@duncanspumpkin
Copy link
Copy Markdown
Contributor

duncanspumpkin commented Oct 9, 2025

#3307 supersedes this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Using a keyboard shortcut to open the terraform window when it is already open on the "Plant Trees" or "Walls & Fences" tab leaves a ghost behind

3 participants

@LeeSpork @duncanspumpkin @AaronVanGeffen