Fix crash disabling quantize

[daschuer]

Make m_marksToRender a std::map to have a constant sort key.

This fixes the crash due to changing sort keys reported in #11709

[ywwg]

I'll set this up with my js spammer just to confirm the fix :)

[uklotzde]

Using a std::map here might be convenient but is the wrong choice for this use case.

[daschuer]

@uklotzde, your review is pretty useless if you don't explain you opinion and don't add a proposal for improvements.

[uklotzde]

A map is used for dynamically adding and removing elements. In this case the keys are stable, but become inconsistent at the time when they are created. You will not be able to remove an element reliably by its key once it has been inserted. This only gets worse while time passes.

Using a map is deceiving. A developer will easily create a key from a WaveformMarkPointer and try to use it for accessing the map. Using a map makes it easy to make this and other mistakes.

[uklotzde]

I am done with my "useless" review.

[ywwg]

I see the point, that usually the key in a map is an accessor, and that the sample position is not really a useful way to access the items in this map. However this code is isolated and this hack is pretty self-contained. It is unlikely that this choice will cause large problems down the line, and if this turns out to be an inconvenient solution we can leave enough comments to explain why this choice was made. I think this is fine for now to fix the severe party-crashing bug.

We are always open to specific alternative approaches to fixing issues. I think ideally the solution would involve a dirty flag and snapshotting cue data at render time in a thread-safe way to render their data. Most likely all this code will disappear when the QML rewrite is done, so I'm not sure it's worth doing a 100%-correct-and-pretty solution here.

[daschuer]

Thank you for review. I have rebased this to 2.3. because the 2.3 branch is also affected form the crasher.

[uklotzde]

so I'm not sure it's worth doing a 100%-correct-and-pretty solution here.

Please note that I deliberately did not request "a 100%-correct-and-pretty solution"! I only pointed out that the current design and code (which includes comments) is prone to introducing new bugs.

[JoergAtGithub]

The text of the commit message seems to be accidentally here

[daschuer]

Oh, yes. I must have messed up the rebase to 2.3. Now the old message is back.

[ywwg]

Please note that I deliberately did not request "a 100%-correct-and-pretty solution"! I only pointed out that the current design and code (which includes comments) is prone to introducing new bugs.

I appreciate the review, truly. I think this solution is at least incrementally better than the current proven-dangerous code. Would you be opposed to merging as-is, or is it sufficient that we have raised the issue and are aware of the possible downsides?

[uklotzde]

All downsides should be documented, i.e. "Don't consider this map as a regular map! It is rebuilt periodically only for the purpose of sorting entries.".

That's the bare minimum, software engineering 101.

[uklotzde]

It is the duty of the PR owner to choose a suitable option to resolve the questions and concerns that have been expressed. I didn't exclude any of them.

[daschuer]

This this is still a regular map and I cannot confirm your concerns. The issue for me is that your complains are kind of common, not specific to this use case of this private container. I have considered different solutions and this is IMHO the best best we can pick in terms of performance and architecture. But maybe I am wrong, you can prove it by providing a different solution.
For now I will tweak the comments a bit and than I am done.

[ywwg]

Suggested change

	// This class stores the data used as a sorting key immutable.
	// We cannot use the the mark directly, because it can be
	// altered at any time.
	// This class provides an immutable sortkey for the WaveformMark using sample position and hotcue enum. IMPORTANT: the Mark's position may be changed after a key's creation, and those updates will not be reflected in these sortkeys. Therefore these keys should not be retained for long periods of time. Currently they are used to render marks on the Overview, a situation where temporarily incorrect mark position is acceptable.

[ywwg]

(Feel free to copy and paste to fix newlines, etc)

[daschuer]

Done.