-
-
Notifications
You must be signed in to change notification settings - Fork 957
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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 😎 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your changes are causing the regression tests to crash. I haven't investigated in detail, but check the logs from the automated builds.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)