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: Don't try to rename OWNER_DEITY signs in-game #9716

Merged
merged 1 commit into from Nov 28, 2021

Conversation

2TallTyler
Copy link
Member

@2TallTyler 2TallTyler commented Nov 24, 2021

Motivation / Problem

Many city builder Game Scripts use a sign below the town name to show the current growth rate. Players can click on this sign which brings up the GUI to edit the sign, but they are actually blocked from renaming the sign. The same goes for signs created by other players.

Why show the rename GUI if we're not allowed to do anything with it?

sign_error

Description

If we're not allowed to rename a sign, don't even open the rename GUI.

The exception is if we're in Scenario Editor, which can open savegames by changing the file extension, and needs to be allowed to edit anything including signs.

Limitations

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR touches english.txt or translations? Check the guidelines
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

LordAro
LordAro previously approved these changes Nov 24, 2021
Copy link
Member

@LordAro LordAro left a comment

Comment Ian"t great - it explains "what", which the code already does. Comments should explain "why", if needed

@ldpl
Copy link
Contributor

@ldpl ldpl commented Nov 24, 2021

It's very possible to edit signs of other companies. For good or not but currently, you just highjack the sign when you edit it. The only ownership check CmdRenameSign does is for OWNER_DEITY which is entirely different from the check in this PR:

if (si->owner == OWNER_DEITY && _current_company != OWNER_DEITY && _game_mode != GM_EDITOR) return CMD_ERROR;

@2TallTyler 2TallTyler changed the title Fix: Don't try to rename signs a company doesn't own Fix: Don't try to rename OWNER_DEITY signs in-game Nov 24, 2021
@2TallTyler
Copy link
Member Author

@2TallTyler 2TallTyler commented Nov 24, 2021

It's very possible to edit signs of other companies.

That's...odd. I've amended the check and PR title to reflect the real itch being scratched by this PR, which is trying to edit a sign we're not allowed to change. Hopefully the comment better explains the "why" now, too.

@ldpl
Copy link
Contributor

@ldpl ldpl commented Nov 24, 2021

Maybe move sign ownership check to a separate function? To reduce the chance of anyone making the same mistake again.

@2TallTyler
Copy link
Member Author

@2TallTyler 2TallTyler commented Nov 24, 2021

It's a pretty specific usecase, but something like CompanyCanRenameSign()?

@2TallTyler
Copy link
Member Author

@2TallTyler 2TallTyler commented Nov 26, 2021

CompanyCanRenameSign() created and now both CmdRenameSign() and HandleClickOnSign() use it instead of duplicating the same code.

src/signs_gui.cpp Outdated Show resolved Hide resolved
@LordAro LordAro added the backport requested label Nov 28, 2021
@glx22 glx22 merged commit 802ca4e into OpenTTD:master Nov 28, 2021
17 checks passed
@2TallTyler 2TallTyler deleted the sign_rename branch Nov 28, 2021
TrueBrain pushed a commit to TrueBrain/OpenTTD that referenced this issue Jan 3, 2022
@TrueBrain TrueBrain added backported and removed backport requested labels Jan 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants