It is good that you identified the cause.

However, having two retrievable values to manage should be avoided from a maintainability perspective. So I believe the issue should be resolved by changing the update timing or adding resetting the state.

I'm with you that having two retrievable indices could get confusing. I don't think I understand what you mean with your alternatives though, can you elaborate a little?

In the end there are two different vectors involved (track_edits and Animation.tracks) which sneakily can be access with the same index most of the time, until you happen to have a filter. As long as that's the case I think it's very easy to use the wrong index by mistake. The problem could be avoided by always keeping track_edits the size of Animation.get_track_count(), adding null or hidden controls for the filtered-out tracks, but that seems bad for other reasons.

I could probably also drop get_track_edit_selected and instead do the loop inline in set_animation when finding the control to release focus from (it's not really needed elsewhere). But I kinda feel like for maintainability, leaving just one retrievable index (without any other refactoring) just leaves the door wide open to accidentally use that to index into track_edits again?

I could probably also drop get_track_edit_selected and instead do the loop inline in set_animation when finding the control to release focus from (it's not really needed elsewhere).

So in other words, I assume you can remove the focus immediately before filtering.

So in other words, I assume you can remove the focus immediately before filtering.

Mmm, do you just mean you'd prefer it like fa81a0a (temp commit for discussion purposes)? I feel like you have something more different in mind 😅

I think that's a similar way. So if there's something that keeps fetching the index, I believe the simplest approach to solve the issue is to abort that fetching by initialize the state.

Okay. So the actual error locations before were

1. Passing a animation track index into the _track_grab_focus signal callback, while the callback treated it as a track_edits index
2. _update_tracks fetching the currently selected track_edits index, but then comparing it against animation track indices (it does this to focus the correct control after reinitializing track_edits.)
3. _anim_duplicate_keys and _anim_paste_keys fetching the selected track_edits index, but then using it to manipulate the actual animation tracks

As seen in the last commit, it's easy enough to get rid of the need to fetch the selected track_edit index, but the places that are (or should be) fetching the real animation track index I think are not so easy. Currently the only way to know which track is selected is to derive it by finding the focused control, like _get_real_track_selected does. I'm not sure what state I can initialize to simplify the logic in these places?

I assume you just need to process sequence to remove all filtering in TrackEditor, perform the sort, and then reapply the filtering.

If maintaining an index for sorting is necessary, creating a temporary local array within the function should be fine.

The main thing to consider here is that the state of the filtered list after sorting and then removing the filter is undefined. For example, if changes like the following occur, the final result could be one of several possibilities:

User's operation:

1234 -[enable filtering]-> 124 -> 241 -[disable filtering]-> ????

then

3241 or 2341 or 2431 or 2413

If we want consistent results, I think you can ensure that filtered items always appear at the top like 2413.

Actual behavior on sort filtered 124 to 241:

1234 -[enable filtering]-> 124 -[disable filtering for sorting]-> 1234 -[Sorting]-> 2413 -[re-enable filtering]-> 241

Currently it's actually dependent on where exactly you drop the dragged track. It's always placed next to the track you drop it onto, either directly before or after it depending on which half it's dropped at:

I believe this is working about as well as it can, so I don't think this behaviour should change. It's not really affected by this bug, since even before this PR the operation of rearranging the animation tracks itself was always dealing only with unfiltered indices (and then it got things mixed up when trying to manage the UI focus).

Yeah, that should be fine for the reordering.
So if the steps described above are processed correctly internally, crashes should not occur.

remove all filtering in TrackEditor, perform the sort, and then reapply the filtering.

Cleaned up, squashed and rebased. Thanks for the review, diff is a bit smaller now.

Thanks!

Cherry-picked for 4.5.2.