Ensure MSBuildPanel buttons are instantiated

[raulsntos]

After #51220, MSBuildPanel was throwing a NullReferenceException on the _Notification method since the method can get called before the _Ready method where the buttons are instantiated so it was trying to set the icon of a null button. This PR initializes the panel in the constructor instead of using the _Ready method to make sure the buttons are never null.

An alternative implementation would be to leave the initialization in the _Ready method and check if the buttons are null in the _Notification method, but it doesn't seem like we need to wait for _Ready to do any of the initialization that MSBuildPanel does so it can be done in the constructor.

Bugsquad edit: Fixes #51344.

[YuriSizov]

An alternative implementation would be to leave the initialization in the _Ready method and check if the buttons are null in the _Notification method, but it doesn't seem like we need to wait for _Ready to do any of the initialization that MSBuildPanel does so it can be done in the constructor.

Indeed your current solution is correct (at least as far as we usually do it in Godot's codebase). Weird that I didn't experience the issue, or at least didn't notice it, but this change makes sense.

[neikeq]

As far as I remember, we do initialization on _Ready because the constructor is always called after hot-reload. If we were to do initialization in the constructor we would be adding extra child buttons every time assemblies were hot-reloaded.

[YuriSizov]

Is it a C# specific thing? Neither C++ side nor GDScript would do that (unless it was terminated improperly).

Also, why would it hot reload at all? 🙃

[neikeq]

Is it a C# specific thing? Neither C++ side nor GDScript would do that (unless it was terminated improperly).

I don't know if GDScript calls _init every time a script is reloaded. But GDScript doesn't need to reload all script, only the ones that change.

Also, why would it hot reload at all? 🙃

Because we need to destroy the domain, start a new one and reload assemblies back. So we need to reload everything if something changes.

[YuriSizov]

But shouldn't all the UI nodes be removed from the tree and destroyed when you do a complete reload? _Ready is way too late to add child nodes generally. _Ready should indicate that child nodes are also ready, and this way it's not possible.

[neikeq]

No, they are not removed on reload regardless of the language.
Regarding the constructor and _Ready, I want to improve this as I don't like it either. But right now this is how it works, so there's no point in discussing it in the scope of this PR.

[raulsntos]

As far as I remember, we do initialization on _Ready because the constructor is always called after hot-reload. If we were to do initialization in the constructor we would be adding extra child buttons every time assemblies were hot-reloaded.

That makes sense. I was under the impression the game assembly was the only one that needed to be reloaded.

Regarding the constructor and _Ready, I want to improve this as I don't like it either. But right now this is how it works, so there's no point in discussing it in the scope of this PR.

That would be nice.

I have updated the PR to use _Ready and check if the buttons are null instead.

[neikeq]

Null conditional operator ?. would be shorter and more readable. But it's nothing important.

[raulsntos]

I can't use it on an assignment: The left-hand side of an assignment must be a variable, property or indexer.

[akien-mga]

Thanks!