Kira Mourão

All calling code already checks this. Happy to add it in if we think it's safer though?

Done.

Well I agree that I was wrong to remove the synchronised call, and have put it back. However it’s only indirectly because of iteration - the code in getSequenceAt isn’t itself iterating (although callers do) but the calls to size() and then get() mean that anything changing the size of the list between the calls could break getSequenceAt.

The delay in resolving this was because I was looking to see if a for-each call replacing the current for loop iteration using getSequenceAt would be better. It works fine, and is already synchronised by SynchronizedList so avoids needing further synchronisation, but benchmarking shows that it’s actually slower, at least the way I implemented it.

It does seem a bit pointless to have all the overhead of a SynchronizedList and then still have to manually synchronise every time it’s used - might as well just use a non-thread safe data structure and do the synchronisation manually, but that’s a wider issue…

ok

ok

I changed the VisibleColsCollection constructor, though it makes it inconsistent with VisibleSeqsCollection (which actually needs AlignmentI)

Renamed to be more generic. This code predates the HiddenColumns changes, and I've now updated it to take a an iterator instead of a list, which has simplified the code. It should be able to take a hidden ranges or visible ranges iterator which means it can do duty as either visible or hidden columns iterator (NB iterates over individual columns not ranges).