-
Notifications
You must be signed in to change notification settings - Fork 46
Extension restart fix #433
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
base: main
Are you sure you want to change the base?
Conversation
vscode/src/lsp/clientPromise.ts
Outdated
import { LOGGER } from "../logger"; | ||
import { NbProcessManager } from "./nbProcessManager"; | ||
import { clientInit } from "./initializer"; | ||
import { NbLanguageClient } from "./nbLanguageClient"; | ||
import { globalState } from "../globalState"; | ||
import { jdkDownloaderPrompt } from "../webviews/jdkDownloader/prompt"; |
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.
nit: remove unused import
@@ -62,10 +64,15 @@ export class ClientPromise { | |||
public restartExtension = async (nbProcessManager: NbProcessManager | null, notifyKill: boolean) => { | |||
if (this.activationPending) { | |||
LOGGER.warn("Server activation requested repeatedly, ignoring..."); | |||
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.
Why this return
statement is removed?
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.
- restartExtension gets triggered whenever the configurations are changed and in that case even if activationPending we would need to restart (as configs have changed).
- BTW this is linked with issue on windows, it gets stuck here because of the return statement sometimes . So to resolve that we need to remove this return and show the reload window to the user if required.
6ea48bc
to
d3a16fe
Compare
Moved the reload window pop up from jdk downloader to configuration change listener(restartExtension) triggered whenever any configuration is changed and extension is in bad state.
d3a16fe
to
d3721f8
Compare
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!
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.
Please rebase the branch to main
in order to remove unrelated changes in l10n files. Thanks.
Issue
Post activating the extension one of the two cases happen
1)
JDK Found
2)
No JDK Found
In case of "No JDK Found" we enter a bad state where even if the user changes the jdk path to some valid path we cannot restart unless the user reloads .
In case initially JDK found but later user changes the path to an invalid jdk home then also we enter
No JDK Found
state.Fix
Moved the reload window pop up from jdk downloader to configuration change listener(restartExtension) triggered whenever any configuration is changed and extension is in bad state.
This gives user a prompt to reload window so that extension can come out of the bad state.