-
Notifications
You must be signed in to change notification settings - Fork 8
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
fix: add an internal debounce to setOutline + move code to OutlineView #118
base: master
Are you sure you want to change the base?
Conversation
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.
Not sure why, but the jasmine fails to await inside beforeEach
.
I added some console.log statements and it looks like it is awaiting in beforeEach. I'm not sure what is wrong. |
I am using |
return "list-unordered" | ||
} | ||
|
||
setOutline(outlineTree: OutlineTree[], editor: TextEditor, isLarge: boolean) { | ||
public setOutline(editor: TextEditor | undefined) { | ||
return new Promise<void>((resolve) => { |
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.
Another simpler approach is to not care about when this function is resolved, and use a sleep
function in the tests. I actually started with that. But jasmine wasn't awaiting the sleep
function either.
function sleep(ms = 1000) {
return new Promise((resolve) => {
setTimeout(resolve, ms)
})
}
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.
One other thing is to make a timer using a while loop. I haven't tried that though
function sleep(delay) {
const start = new Date().getTime();
while (new Date().getTime() < start + delay) {
// do nothing
};
}
if (this.isRendering) { | ||
// return if setOutline is already called and not finished yet. | ||
resolve() | ||
return |
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.
If secondary calls return here the timeout never gets cleared and reset below.
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.
But this.isRendering
becomes false inside the setTimeout
callback. 🤔
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.
Yes so the workflow goes like this:
- Call
setOutline
;this.isRendering
is set to true; timer is created. - Call
setoutline
;this.isRendering
is true so returns early; timer is not changed.
The second call should reset the timer before returning. Otherwise the clearTimeout(this.setOutlineTimeout)
is redundant because it will only be called after the timeout has already completed and this.isRendering
is set to false.
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.
The main reason I created this PR is that we have three observers:
- When the active text editor changes
- When the text changes
- When a provider is detected
In the beginning, these three might happen together resulting in three calls to getOutline
. We may be able to handle this better than debouncing the calls.
For example, what if an active text editor is opened, but its provider is called late? The first call would return immediately due to a missing provider, but the second call actually has useful data.
Maybe this a good opportunity for a memoized function instead of a debounced function. Essentially, cache the input and results and given the same input as before return the cached results instead of recalculating. |
setOutline
was being called multiple times because of having different observers that were calling it.