Fix: Stop Construction Windows Closing on Company Rename (Issue #7479) #7521
Conversation
|
I think the risk is invalid windows remaining open if switching to another company, or even switching to being a spectator. You should probably add a variable testing if the new company ID is equal to the old and only close windows if they are different. |
|
Or better yet, invalidate the windows to recreate them 😎 |
|
Crashing during regression Edit: |
|
I think you'll need to squash and force-push your branch for the commit checker to be happy. |
Fix: Invalidate Company Windows for Redraw Fix: Theoretically Fix Rergession Test Failure Fix: Fix Trailing Whitespace Fix: Fix trailing whitespace 2
|
Yep; the commit checker is satisfied now. :) |
There was a problem hiding this comment.
This fix is wrong. If you switch to Spectator in multiplayer then your construction windows don't close as they should.
The bug report is that construction windows close when you change your client name but remain in the same company. Hence the fix should be to only close the construction windows if the active company actually changed.
| DeleteConstructionWindows(); | ||
| /* Recreate company windows */ | ||
| if (Company::IsValidID(new_company)) | ||
| { |
There was a problem hiding this comment.
The opening brace of if, for, while etc. statements needs to go on the same line as the statement. Or make a braces-free single-statement body on one line.
|
Hi, there's been no activity on this for nearly 4 months. If no more effort will be put into this PR, it will be closed in the next week or so |
|
This won't get finished. If someone else wants to take this over, feel free. Otherwise, I'm closing for now. |
While unaware of the ramifications or importance of this "fix", I completed it anyway as per request of issue #7479. If this is still a bad idea, please let me know why and I will try to find a compromise for both you and the issue author. Thanks :)