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

Conversation

@bennyman123abc
Copy link

bennyman123abc commented Apr 18, 2019

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

bennyman123abc commented Apr 19, 2019

Or better yet, invalidate the windows to recreate them 😎

Copy link
Contributor

nielsmh left a comment

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 Apr 20, 2019

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 bennyman123abc force-pushed the bennyman123abc:issue/7479 branch from 73d46b3 to e170b8e Apr 20, 2019
@bennyman123abc
Copy link
Author

bennyman123abc commented Apr 20, 2019

Yep; the commit checker is satisfied now. :)

Copy link
Contributor

nielsmh left a 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))
{

This comment has been minimized.

Copy link
@nielsmh

nielsmh Apr 24, 2019

Contributor

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

bennyman123abc commented Aug 18, 2019

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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.