-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Feat: add redrawcomplete event #3480
Conversation
Not certain how you feel about the new event names but happy to update them if you want. |
@@ -518,6 +529,8 @@ class Renderer extends EventEmitter<RendererEvents> { | |||
tailDelay(() => { | |||
renderTail(fromIndex + viewportLen, toIndex + viewportLen) | |||
}) | |||
} else { | |||
complete('tail') |
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.
I'm thinking, maybe this could be refactored into a promise, smth like:
// Draw the waveform in viewport chunks, each with a delay
return Promise.all([
new Promise((resolve) => {
const headDelay = this.createDelay()
const renderHead = (fromIndex: number, toIndex: number) => {
draw(fromIndex, toIndex)
if (fromIndex > 0) {
headDelay(() => {
renderHead(fromIndex - viewportLen, toIndex - viewportLen)
})
} else {
resolve(true)
}
}
renderHead(start, end)
}),
new Promise((resolve) => {
if (end >= len) {
resolve(true)
return
}
const tailDelay = this.createDelay()
const renderTail = (fromIndex: number, toIndex: number) => {
draw(fromIndex, toIndex)
if (toIndex < len) {
tailDelay(() => {
renderTail(fromIndex + viewportLen, toIndex + viewportLen)
})
} else {
resolve(true)
}
}
renderTail(end, end + viewportLen)
}),
])
I would also refactor those recursive timeouts into sequenced promises, but that's outside of the scope of this PR, I'll do it myself later.
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.
@katspaugh We thought about that, too; however, one downside to this is that createDelay
could end up canceling the setTimeout, which would mean that resolve never gets called and ends up adding unresolved promises as memory leaks.
This is especially problematic if the rendercomplete gets interrupted by a new render.
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.
True, good point. I’ll approve the PR as is then.
redraw: [] | ||
/** When all audio channel chunks of the waveform have drawn */ | ||
redrawcomplete: [] |
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.
I personally like the event name, it's clear!
Sick!!! |
Short description
Currently the
redraw
event firers beforerenderHead
andrenderTail
have completed. This adds a new eventredrawcomplete
that fires once both head and tail have completed rendering.In longer files
redrawcomplete
can take awhile to fire so we leftredraw
andredrawcomplete
as two separate eventsResolves #
Implementation details
How to test it
['zoom', 'redraw', 'redrawcomplete'].forEach((e) => wavesurfer.on(e, () => console.log(Date.now(), e)))
Screenshots
Checklist