Pretty sure there doesn't need to be a null check in every method 🤔 Some of the checks can't be probably reached.
From more obvious ones, _animation_selected(), _sync_animation() and can_drop_data_fw() do not need a check, because frames is not referenced there.

Pretty sure there doesn't need to be a null check in every method 🤔 Some of the checks can't be probably reached.
From more obvious ones, _animation_selected(), _sync_animation() and can_drop_data_fw() do not need a check, because frames is not referenced there.

Got it. I've removed unnecessary null checks.

I'd say we should at least consider the error being elsewhere first, or at least reduce the number of cases here, it feels very brute force to solve the problem this way

While I can understand where you're coming from, IMO this is consistent with how AnimationTrackEditor handles the same issue. For example, here. (A lot of similar examples can be found in the same file)

The other solution I had in mind was to disconnect the signals causing the crashes when frames is set to null here, and connecting them when frames is set to a non-null value. Does that sound better to you?

This sounds fine, my main concern was with if the check should just be elsewhere

If this is caused by signals or other things like that I think it's fine to do things this way!

It's more a question of "is this the best way to solve this", if the other way would be convoluted messing with signals then this is fine

This particular crash is caused by the SpriteFrames editor getting hidden, which causes frames to become null and also the SpinBox to lose focus, which in turn emits the value_changed signal. It's just unfortunate order of events and probably can't be fixed in a better way.

We could keep the fix limited to this single case. That risks another crash being discovered elsewhere, but prevents having too many unnecessary checks.

This particular crash is caused by the SpriteFrames editor getting hidden, which causes frames to become null and also the SpinBox to lose focus, which in turn emits the value_changed signal. It's just unfortunate order of events and probably can't be fixed in a better way.

We could keep the fix limited to this single case. That risks another crash being discovered elsewhere, but prevents having too many unnecessary checks.

That's fine too. Though I'll keep the null check in _frame_duration_changed while I'm at it, since as mentioned in the bug report, this issue also affects the frame duration spinbox.

Thanks!