Call OnUnselected and OnSelected when removing a selected tab item

[redawl]

Description:

Fixes #4985

Checklist:

• Tests included.
• Lint and formatter run with no errors.
• Tests all pass.

[andydotxyz]

I think this assumes that the selected item is being removed.

It may be more of an optimisation rather than an actual error, but I think it should be checked.

[redawl]

I think this assumes that the selected item is being removed.

It may be more of an optimisation rather than an actual error, but I think it should be checked.

Thanks for noticing that. The code no longer assumes that the selected item is being removed.

I also uncovered another problem, as well as an existing bug. Added unit tests for both cases:

1. Removing the last tab caused a crash for the original commit.
2. Removing any item other than the last item while the last item was selected, was causing OnSelected to be called for the last item. This has now been fixed as well :)

[andydotxyz]

I don't understand this sorry. if we are replacing 2 items, where the second (index 1) is selected and then replace with 1 we get:

count = 1
selected = 1

Your comment seems to say that item 1 still exists (which it doesn't) but then sets selected to count - 1 (which is 0). The comments here do not seem to be consistent with the code.

Also should it not just be another case in the select rather than an if/else?

[redawl]

selected is the index of the selected item, it does not refer to a specific tabitem. State before and after setItems:
Before: [ item1, item2]
After: [item2]

From the tabs perspective, selection is tracked via index, so selected index needs to be moved down by 1 so that item2 still appears to be selected in the ui. However we do not want to call OnSelected since the selected tabItem didn't change (just its index).

Agreed about the if statement can just be two switch cases.

[redawl]

Hmm, I think I found another existing bug...

If you have 4 doctabs with the last one selected, and you remove tab 2, then tab 3 is selected instead of tab 4

[andydotxyz]

I may be reading it wrong, but this appears to be inside setItems - so the assumption that it's a removal of any particular item doesn't hold...

[redawl]

Correct. However, if selected >= count we know an item was removed. There is no other way for the new list to be shorter.