-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
move ordering to waveformmarkset #12237
Conversation
fc01e45
to
725eb69
Compare
still making some improvement post-merge. |
fyi, a renaming is probably in order too, since mathematical sets don't have a concept of ordering... |
…, hover in waveformviewer in reverse draw order
725eb69
to
2245220
Compare
All comments have been addressed, @Swiftb0y There is still room for more refactoring (but isn't there always 😄?) but I don't want to go there now. I left a TODO comment though. |
also, would you mind looking into the pre-commit failure? I think its just a whitespace issue. |
Sure, will do, though I'd like to understand how it is possible to commit things that fail, while using pre-commit locally. DONE |
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.
LGTM. Thank you. I highly value your refactoring contributions.
Same here. |
This is a basically a code quality refactoring, but has the added bonus of fixing an inconsistency between the waveform overview and the waveform viewer.
WOverview had both a member
WaveformMarkSet m_marks
and a memberm_marksToRender
(a sorted map of the visible marks) , and code to update the second. This functionality makes more sense to live insideWaveformMarkSet
.With this refactoring, WOverview only has
WaveformMarkSet m_marks
, and when iteratoring overm_marks
, this will be over the sorted marks to render, and WOverview doesn't need to know anything about this.As a bonus, WaveformRenderMark, now uses the some ordering as WOverview (I believe there was a bug about this, or maybe a discussion in zulip, but I can't find it).