Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we could a debug pixel here to tell us how often this case is actually being triggered? (I'm not convinced that we do, but just asking anyway)
I was also wondering if we should default to selecting the last tab in the tab manager, in the case that the index somehow ends up being higher than the total count.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You got me thinking - I'm wondering if somehow the array is empty. It shouldn't be, but TabManager isn't really thread safe.
1: at this point currentIndex will always be >= count and cause a problem if the array is accessed
2: if currentIndex >= 1 this could cause a problem
3: finally ok
Without adding a bunch of locking, could change it to this to tighten it up a bit:
Worth changing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds like a reasonable change. Tbh I would expect the tab manager to always be accessed from the main thread, but if that's not the case then yeah tidying this up sounds good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah that's a good point. I don't want to change too much so I'll leave it.
Am also going to not make the other changes you suggest: