Fix invalid use of iterators in piano roll

[Rossmaxx]

I found a crash on the MSVC build. The bug seems to have been exposed post #6477. It required some adjustments.

Affected versions : MSVC build of latest master.
To reproduce : try to draw on an empty midi clip. It crashes with a debug assertion failed message.

[superpaik]

It builds and works ok.

[DomClark]

I think the loops should still iterate backwards - this was an intentional change in 8ebdee6 to better align with how the notes are drawn. See the commit message for more information.

As for fixes, I'd take the opportunity to tidy things up a bit. The first iterator loop can be replaced with std::find_if and reverse iterators:

auto it = std::find_if(notes.rbegin(), notes.rend(), [this, pos_ticks, key_num, edit_note](Note* note) {
	auto len = note->length();
	if(len < 0) { len = 4; }

	return pos_ticks >= note->pos()
		&& len > 0
		&& ((!edit_note && pos_ticks <= note->pos() + len && note->key() == key_num)
			|| (edit_note && pos_ticks <= note->pos() + NOTE_EDIT_LINE_WIDTH * TimePos::ticksPerBar() / m_ppb));
});

The second can be a standard for-loop:

for (auto it = notes.rbegin(); it != notes.rend(); ++it) {
	// ...
}

The last can also be replaced with std::find_if and reverse iterators:

const auto it = std::find_if(notes.rbegin(), notes.rend(), [pos_ticks, key_num](Note* note) {
	return pos_ticks >= note->pos()
		&& pos_ticks <= note->pos() + note->length()
		&& note->key() == key_num
		&& note->length() > 0;
});

[Rossmaxx]

I think the loops should still iterate backwards

fixed.

I think the find_if changes could be done in a later refactor PR.

@superpaik wanna test again?

[superpaik]

I've found this (not sure it's related to this PR). When drawing a note, and without releasing the mouse, drag the note to another position you get this message on the log: "noteEnd() triggered without corresponding activate()!" but it continuous to work properly.
The weird thing is that it only happens once, even if you close the pianoroll an create a new beat and do the same.
You need to create a new track (or duplicate the existing one), do the same, and you'll get the message again in the log.

Other than that, I haven't found anything else.

[Rossmaxx]

When drawing a note, and without releasing the mouse, drag the note to another position you get this message on the log: "noteEnd() triggered without corresponding activate()!" but it continuous to work properly.

I know of that bug. I faced that while testing #6595, which doesn't contain the std::vector change. That bug is not related to this pr. It's not related to that pr either. Tbh, pianoroll.cpp is a huge mess, might be hard to locate that bug. It's in dire need of a refactor, which is beyond the scope of this PR.

[Rossmaxx]

Just found the issue which superpaik mentioned #5373

[DomClark]

GitHub won't let me leave a comment there, but there is a decrement operation on line 2559 that should be an increment.

[DomClark]

I think a more appropriate title for this PR would be "Fix invalid use of iterators in piano roll". The problem isn't unique to MSVC - the code exhibits undefined behaviour and is wrong regardless of the compiler. It so happens that MSVC's standard library in debug mode includes assertions that highlight these issues, but the assertions themselves are not the bug. The aim is to fix the iterator problems, not silence the debug warnings.