[3.x] Fix TexturePreview crashing

[AaronRecord]

Fixes #50702 for 3.x

[YuriSizov]

You should still use NOTIFICATION_THEME_CHANGED and check if the fetched font is valid before duplicating and doing anything with it. Just because #50743 exists doesn't mean we should compromise on user experience when we can easily do the check here.

Font can still be invalid if, for example, someone uses a custom theme resource with that theme item not being set. With a check we can avoid the crash with a graceful behavior.

[AaronRecord]

You should still use NOTIFICATION_THEME_CHANGED and check if the fetched font is valid before duplicating and doing anything with it. Just because #50743 exists doesn't mean we should compromise on user experience when we can easily do the check here.

For some reason EditorAbout was changed to only update it's theme in NOTIFICATION_TREE_ENTER, (which might've been a mistake). Also I don't think the user changing the editor theme to use a custom theme resource is a very common occurrence, at least in 3.x.

Font can still be invalid if, for example, someone uses a custom theme resource with that theme item not being set. With a check we can avoid the crash with a graceful behavior.

Hmm, but then how come in the 18 other cases that do something similar ((\n\s*(case NOTIFICATION_ENTER_TREE:|case NOTIFICATION_THEME_CHANGED:)){2}), the only one that does a null check is one that uses a non constant string (in script_create_dialog.cpp) 🤔.

Edit: see #50746 (comment)

[YuriSizov]

Also I don't think the user changing the editor theme to use a custom theme resource is a very common occurrence, at least in 3.x.

Does it matter if it's common or not if it's a one line fix for a feature we are supposed to support? 🙃

For some reason EditorAbout was changed to only update it's theme in NOTIFICATION_TREE_ENTER, (which might've been a mistake).

It is a mistake, the signal is also sent when you change one of the predefined themes, or change configurable properties of the predefined themes. There are some parts of the editor that ignore that fact, and it's a bug every time.

Hmm, but then how come in the 18 other cases that do something similar ((\n\s*(case NOTIFICATION_ENTER_TREE:|case NOTIFICATION_THEME_CHANGED:)){2}), the only one that does a null check is one that uses a non constant string (in script_create_dialog.cpp) 🤔.

Most of the time we don't manually copy or touch the items returned from the theme and just pass them to the local override/property setter, where invalid values are handled either way. But I haven't checked all the 18 occurrences that you speak of, there may be possible crashes there as well.

---

The workaround is fine as a solution for the immediate problem, but I still think that graceful handling of properties that can be configured by the end user is a better approach overall, and other places may benefit from it too.

[akien-mga]

Thanks!