Conversation
… no pending calls and finished workspace parsing, file parsing, and intellisense updates.
|
@sergior2 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
| return !this.model.isInitializingWorkspace.Value | ||
| && !this.model.isIndexingWorkspace.Value | ||
| && !this.model.isParsingWorkspace.Value | ||
| && !this.model.isParsingFiles.Value | ||
| && !this.model.isUpdatingIntelliSense.Value; |
There was a problem hiding this comment.
Can this just be?
| return !this.model.isInitializingWorkspace.Value | |
| && !this.model.isIndexingWorkspace.Value | |
| && !this.model.isParsingWorkspace.Value | |
| && !this.model.isParsingFiles.Value | |
| && !this.model.isUpdatingIntelliSense.Value; | |
| return !this.IsTagParsing() && !this.model.isUpdatingIntelliSense.Value; |
Nit: you didn't introduce this, but since all these properties being checked belong to this.model, I might expect there to be a ClientModel.IsIdle(): bool or something.
There was a problem hiding this comment.
Will add the suggested change, although I'm not sure I follow when you say there to be a ClientModel.IsIdle()?
Is that not what isIdle() currently fulfilling?
There was a problem hiding this comment.
He's saying add a public method on ClientModel instead of DefaultClient. Though there is already DefaultClient.IsTagParsing which I think is what you care about. isUpdatingIntelliSense wasn't what I thought you wanted to block on. So it looks like the property you want already exists.
There was a problem hiding this comment.
You don't actually need to check for "idle" right? I thought you just wanted to know when tag parsing is done.
There was a problem hiding this comment.
Yeah, I would think the IsTagParsing should be sufficient.
| } | ||
|
|
||
| async function onWaitForIdle(timeout?: number): Promise<boolean> { | ||
| return clients.getDefaultClient().waitForIdle(timeout); |
There was a problem hiding this comment.
It appears our status indicator is per-workspace folder, so it seems like this won't work for workpace folders > than the 1st.
There was a problem hiding this comment.
This seems strange though...might be a pre-existing problem. It seems like the status indicator should be shared among all workspace folders.
There was a problem hiding this comment.
Maybe you can wait for all for all the clients? Instead of just the default.
There was a problem hiding this comment.
Maybe something like this generated by Copilot:
const waitForIdlePromises: Promise<boolean>[] = [];
clients.forEach(client => {
waitForIdlePromises.push(client.waitForIdle(timeout));
});
const waitForIdleResults: boolean[] = await Promise.all(waitForIdlePromises);
return waitForIdleResults.every(result => result);
There was a problem hiding this comment.
But having the status be per-workspace folder doesn't really make sense -- but no one has complained yet as far as I know. i.e. there could be bugs/issues with that, not sure, when switching to file in a different workspace, I see the status be "incorrect" (i.e. idle instead of processing).
There was a problem hiding this comment.
I originally had this implemented as the following:
const pending: Promise<boolean>[] = [];
clients.forEach(client => pending.push(client.waitForIdle(timeout)));
const results: boolean[] = await Promise.all(pending); 1
return results.every(result => result);
But was told that only reports status notifications to the 1st client, and advised instead to use getDefaultClient. Would this not bind the notifications across?
There was a problem hiding this comment.
Sorry for the late reply, comments were not updating on my end for some reason
There was a problem hiding this comment.
What? Yeah, it seems like you should waitForIdle across all clients. I'm seeing them per-client, although that is probably a pre-existing issue as mentioned previously.
| if (timeout !== undefined && timeout <= 0) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Consider moving these checks "up" to the command handler and changing the signature to waitForIdle(timeout: number).
Also, if devtools (or whoever calls this) supplies an invalid timeout, maybe we raise an exception instead of returning false?
…gation back to the caller.
New handler to help determine if parsing has reached an idle state. Meaning no pending calls and finished workspace parsing, file parsing, and intellisense updates.