Skip to content
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

Closed
wants to merge 1 commit into from
Closed

Fix: Stop Construction Windows Closing on Company Rename (Issue #7479) #7521

wants to merge 1 commit into from

Conversation

bennyman123abc
Copy link

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 :)

@nielsmh
Copy link
Contributor

nielsmh commented Apr 18, 2019

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.

@bennyman123abc
Copy link
Author

Or better yet, invalidate the windows to recreate them 😎

Copy link
Contributor

@nielsmh nielsmh left a 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.

src/company_cmd.cpp Outdated Show resolved Hide resolved
@glx22
Copy link
Contributor

glx22 commented Apr 19, 2019

Crashing during regression

+Crash reason:
+ERROR: Signal:  Segmentation fault (11)
+ERROR: Message: <none>
+
+OpenTTD version:
+ERROR: Version:    20190419--g105c051272 (0)
+ERROR: NewGRF ver: 1a006d64
+ERROR: Bits:       32
+ERROR: Endian:     little
+ERROR: Dedicated:  no
+ERROR: Build date: Apr 19 2019 00:35:11
+
+Stacktrace:
+ERROR: [00] ./openttd(_ZNK12CrashLogUnix13LogStacktraceEPcPKc+0x40) [0x569c0350]
+ERROR: [01] ./openttd(_ZNK8CrashLog12FillCrashLogEPcPKc+0xde) [0x56868a2e]
+ERROR: [02] ./openttd(_ZNK8CrashLog12MakeCrashLogEv+0x67) [0x56868cc7]
+ERROR: [03] ./openttd(+0x3ac27b) [0x569c027b]
+ERROR: [04] linux-gate.so.1(__kernel_sigreturn+0) [0xf7f4ffe0]
+ERROR: [05] ./openttd(_Z24InvalidateCompanyWindowsPK7Company+0x14) [0x56851104]
+ERROR: [06] ./openttd(_Z15SetLocalCompany5Owner+0x7c) [0x5685160c]
+ERROR: [07] ./openttd(_Z13GenerateWorld12GenWorldModejjb+0x91) [0x568c33b1]
+ERROR: [08] ./openttd(_Z12openttd_mainiPPc+0x1afc) [0x569aecfc]
+ERROR: [09] ./openttd(main+0x73) [0x5676d733]
+ERROR: [10] /lib/i386-linux-gnu/libc.so.6(__libc_start_main+0xf6) [0xf5c45286]
+ERROR: [11] ./openttd(+0x17cd25) [0x56790d25]

Edit: GenerateWorld() calls SetLocalCompany(COMPANY_SPECTATOR), Company::GetIfValid(new_company) then returns nullptr and InvalidateCompanyWindow(nullptr) crashes

src/company_cmd.cpp Outdated Show resolved Hide resolved
@nielsmh
Copy link
Contributor

nielsmh commented Apr 20, 2019

I think you'll need to squash and force-push your branch for the commit checker to be happy.

@nielsmh nielsmh dismissed their stale review April 20, 2019 10:43

Whitespace and regression failure fixed

Fix: Invalidate Company Windows for Redraw

Fix: Theoretically Fix Rergession Test Failure

Fix: Fix Trailing Whitespace

Fix: Fix trailing whitespace 2
@bennyman123abc
Copy link
Author

Yep; the commit checker is satisfied now. :)

Copy link
Contributor

@nielsmh nielsmh left a 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))
{
Copy link
Contributor

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.

@LordAro
Copy link
Member

LordAro commented Aug 17, 2019

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

@bennyman123abc
Copy link
Author

This won't get finished. If someone else wants to take this over, feel free. Otherwise, I'm closing for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants