Beats: Remove legacy BeatIterator class in favor of std iterator

[Holzhaus]

The custom, legacy iterator API was only used for rendering beats on the
waveform and for importing beats from Serato BeatGrids.

Since the new iterator API is way more powerful and can be used
with the iterator-based functions from <algorithm>, it makes sense to
replace the only occurrence and then remove the old API entirely.

Based on #4255.

[Holzhaus]

Ok, ready to review.

[daschuer]

Sorry for jumping in here, after merging #4255, but it I have discovered now that I have not fully understand the concept of clatest() and cend()

Unfortunately there are zero comments in beats.h explaining this. Can you catch up that in this PR?

I think I have read the original commit as if clatest()++ = cend() but It turns out that this is wrong.
Maybe we can improve the naming a bit.

Here, I have hard time to understand how the new code id equivalent to the old.

[Holzhaus]

I added some comments to beats.h in 9480561.

[Holzhaus]

Basically the it != clatest() check prevents an integer overflow here.

[daschuer]

Ah Ok, thank you for the comments.

The confusing part was/is that this "violates" some regular laws around iterators.

Normally you should never access the end() iterator value and never continue to iterate it, because the result is undefined and is a risk for a segfault.

This is done here regularly, because clatest takes this role.

This was the source if my confusion. I also expect that various helper functions will follow the normal iterator rules, which leads to dificult understand call trees and side effects.

How about introducing two type of iterators?
One for editing purpose that only has the anotaded beats between begin() and end() and an "endless" iterator that allows to iterate over the entire int range. Depending on the use case we can take one or another,both with "normal" end() behavior.

Alternatively we may rename the clatest() to cend() and introduce a new one cAnotatonEnd() or something that expresses the implication that it is appropriate to fetch beats after the end of the anotaded beats.

[Holzhaus]

Ah Ok, thank you for the comments.

The confusing part was/is that this "violates" some regular laws around iterators.

Normally you should never access the end() iterator value and never continue to iterate it, because the result is undefined and is a risk for a segfault.

This is done here regularly, because clatest takes this role.

This was the source if my confusion. I also expect that various helper functions will follow the normal iterator rules, which leads to dificult understand call trees and side effects.

My reasoning was that cbegin returns an iterator that points at the first element of the container, and cend points at the element behind the last element of the container.

Hence, cbegin points at the first beat of the first beat marker(all earlier beats may be before the beginning of the track) and cend points at the end beat marker, i.e. the beat after the last beat of the last beat marker (all beats behind it may be after the end of the track). But this is pretty technical and I can see the issue.

How about introducing two type of iterators? One for editing purpose that only has the anotaded beats between begin() and end() and an "endless" iterator that allows to iterate over the entire int range. Depending on the use case we can take one or another,both with "normal" end() behavior.

cbegin/cend are just methods of the container that return iterators, so I don't see how multiple iterator types would solve that.

Alternatively we may rename the clatest() to cend() and introduce a new one cAnotatonEnd() or something that expresses the implication that it is appropriate to fetch beats after the end of the anotaded beats.

Yes, I'm open to rename cbegin/cend and make cearliest/clatest the new cbegin/cend. But that is unrelated to this PR and out of scope. I'll open a new PR.

[daschuer]

Thank you to consider this.

cbegin/cend are just methods of the container that return iterators, so I don't see how multiple iterator types would solve that.

My idea was that we have two or more types of iterators for the differen ways to iterate over the beats. All of them can use begin() and end() as you may expect from any iterator and specialized for the range you need.

Maybe we can also introduce different container types as "view" in the original container.

Like "all annotated beats view" "all beat view unlimited" "measures view" "downbeat view" or whatever we need.

Just an idea ...

[daschuer]

LGTM

[daschuer]

Merge? What do you think to the "view" idea?

[Holzhaus]

Merge?

Yup.

What do you think to the "view" idea?

I don't think we need two different classes, but maybe a dedicated iterator for beat markers could make sense. In general, we already have that because you can iterate over the beat marker vector instead of iterating over individual beats. I need to think about that some more.

However, I can open a PR which renames the methods as a first step soon.

[daschuer]

Thank you.