Codechange: Cache ScriptConfig for the slot in constructor

[SamuXarick]

Motivation / Problem

GSConfigWindow seems to mix usage of this->gs_config and GameConfig::GetConfig(). And ScriptListWindow could also cache ScriptConfig for the slot in constructor instead calling GetConfig() everytime. Then the static function can be removed. #10458 (review)

While at it, maybe also invalidate debug window (to show/hide break string), but the invalidate code of that window doesn't really do anything in that regard. #10458 (comment) , #10458 (comment)

Possible programmatic simplification Game and Script OnClick handlers. #10458 (comment)

Description

Limitations

When changing ai_developer_tools, the invalidate call on the Debug Window doesn't really show or hide the break box (to do).

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')
  • newgrf_debug_data.h may need updating.
  • PR must be added to API tracker

[2TallTyler]

It would be nice to see these changes split into commits that explain what you're changing and why. We have a PR template and commit naming convention for a reason, but you've ignored the template and crammed unrelated changes into your commit. Your current commit title is perfect for the caching change, but code simplification and fixes to previous PRs should have their own commits (so to be clear, this PR should have at least 3 commits for the changes it contains).

Here's a useful article about commit messages that might help you figure out how to structure them.

I'd also encourage you to use the PR template. The code and commit message tell me that you're caching the ScriptConfig, but I can't read your mind to understand why. Does caching make something faster? Is it more consistent with other parts of the code? Is the new InvalidateWindowData() call to fix something in a previous PR? If you explain this in the Problem / Motivation section, I don't have to guess, and can review your PR much more effectively. This doesn't need a long backstory and test cases for each change, just help me understand your motivation behind the change. 🙂

[2TallTyler]

Why is this being changed now? Was it missed in #10058?

[SamuXarick]

InvalidateWindowData(WC_GAME_OPTIONS, WN_GAME_OPTIONS_GS) was missing in #10058
InvalidateWindowClassesData(WC_SCRIPT_DEBUG) is added to supposedly show or hide the debug break box when ai_developer_tools setting change, though the invalidate code doesn't really do that yet.

[2TallTyler]

I'm going to mark the PR as a draft until you resolve that, just so it's clear that it's not ready for review. Once you fix it, you can mark it ready for review again.

[2TallTyler]

Can I suggest more descriptive commit names? Remember, people looking through the commit history can't see at a glance while files were changed:

• Codechange: Simplify code in some parts -> Codechange: Simplify script GUI OnClick handlers
• Fix: Typos -> Fix: Typos in AI and script GUI

[2TallTyler]

This is a solution in search of a problem. The new code is not any cleaner and doesn't seem more performant. If you can show some data that says otherwise, I'm happy to reopen. 😄

I'd be happy to see a PR of your commit that fixes the typos though, those are always welcome. 😉