Skip to content
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: do not call doctor.check() on each build target compilation end #3768

Merged
merged 8 commits into from
Mar 31, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 48 additions & 36 deletions docs/integrations/new-editor.md
Original file line number Diff line number Diff line change
Expand Up @@ -99,46 +99,48 @@ about below, and others used to be server properties that have been migrated to

The currently available settings for `InitializationOptions` are listed below.

```js
"InitializationOptions": {
"compilerOptions":{
"completionCommand": string,
"isCompletionItemDetailEnabled": boolean,
"isCompletionItemDocumentationEnabled": boolean,
"isCompletionItemResolve": boolean,
"isHoverDocumentationEnabled": boolean,
"isSignatureHelpDocumentationEnabled": boolean,
"overrideDefFormat": "ascii" | "unicode",
"parameterHintsCommand": string,
"snippetAutoIndent": boolean,
```typescript
interface InitializationOptions: {
compilerOptions: {
completionCommand?: string;
isCompletionItemDetailEnabled?: boolean;
isCompletionItemDocumentationEnabled?: boolean;
isCompletionItemResolve?: boolean;
isHoverDocumentationEnabled?: boolean;
isSignatureHelpDocumentationEnabled?: boolean;
overrideDefFormat?: "ascii" | "unicode";
parameterHintsCommand?: string;
snippetAutoIndent?: boolean;
}
"copyWorksheetOutputProvider": boolean,
"debuggingProvider": boolean,
"decorationProvider": boolean,
"didFocusProvider": boolean,
"disableColorOutput" boolean,
"doctorProvider": "json" | "html",
"executeClientCommandProvider": boolean,
"globSyntax": "vscode" | "uri"
"icons": "octicons" | "vscode" | "unicode",
"inlineDecorationProvider": boolean,
"inputBoxProvider": boolean,
"isExitOnShutdown" : boolean,
"isHttpEnabled": boolean,
"commandInHtmlFormat": "vscode" | "sublime",
"openFilesOnRenameProvider": boolean,
"openNewWindowProvider": boolean,
"quickPickProvider": boolean,
"renameFileThreshold": number,
"slowTaskProvider": boolean,
"statusBarProvider": "on" | "off" | "show-message" | "log-message",
"treeViewProvider": boolean
debuggingProvider?: boolean;
decorationProvider?: boolean;
inlineDecorationProvider?: boolean;
didFocusProvider?: boolean;
doctorProvider?: "json" | "html";
executeClientCommandProvider?: boolean;
globSyntax?: "vscode" | "uri";
icons?: "vscode" | "octicons" | "atom" | "unicode";
inputBoxProvider?: boolean;
isVirtualDocumentSupported?: boolean;
isExitOnShutdown?: boolean;
isHttpEnabled?: boolean;
openFilesOnRenameProvider?: boolean;
quickPickProvider?: boolean;
renameFileThreshold?: number;
slowTaskProvider?: boolean;
statusBarProvider?: "on" | "off" | "log-message" | "show-message";
treeViewProvider?: boolean;
testExplorerProvider?: boolean;
openNewWindowProvider?: boolean;
copyWorksheetOutputProvider?: boolean;
commandInHtmlFormat?: "vscode" | "sublime";
doctorVisibilityProvider?: boolean;
Comment on lines +102 to +137
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copypasted typescript interface from metals-language-client - I think it's easier to maintain it this way. It's also more accurate because it indicates that these properties are optional and don't have to be set.

}
```

You can also always check these in the
[`InitializationOptions.scala`](https://github.com/scalameta/metals/blob/main/metals/src/main/scala/scala/meta/internal/metals/InitializationOptions.scala)
file where you'll find all of the options and descriptions.
file where you'll find all of the options and descriptions. Alternatively you can check out the typescript equivalent - [`MetalsInitializationOptions.ts`](https://github.com/scalameta/metals-languageclient/blob/main/src/interfaces/MetalsInitializationOptions.ts)

##### `compilerOptions.completionCommand`

Expand Down Expand Up @@ -403,6 +405,16 @@ Boolean value signifying whether or not the client supports the

Default value: `false`

##### `testExplorerProvider`

Boolean value to signify whether or not the client implements the Test Explorer.

##### `doctorVisibilityProvider`

Boolean value to signify whether or not the client implements the `"metals/doctorVisibilityDidChange"`.
This JSON notification is used to keep track of doctor state. If client implements this provider then Metals server
will send updates to the doctor view.

#### Experimental Capabilities

All of the features that used to be set with `experimental` can now all be set
Expand Down Expand Up @@ -511,6 +523,7 @@ Returns `Hover` for specified text document and position - [lsp spec](https://mi

Metals also support an extended version of this method that supports hover for selection range.
The extended stucture of request params is the following:

```ts
interface HoverExtParams {
textDocument: TextDocumentIdentifier;
Expand All @@ -520,7 +533,6 @@ interface HoverExtParams {
}
```


### `workspace/didChangeWatchedFiles`

Optional. Metals uses a built-in file watcher for critical functionality such as
Expand Down Expand Up @@ -1014,7 +1026,7 @@ _Request_:
/**
* Currenly, only `pattern` field is used for search.
* See: https://github.com/scalameta/metals/issues/3234
*/
*/
interface TextSearchQuery {
/**
* The text pattern to search for.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,9 @@ case class ClientConfiguration(initialConfig: MetalsServerConfig) {
def disableColorOutput(): Boolean =
initializationOptions.disableColorOutput.getOrElse(false)

def isDoctorVisibilityProvider(): Boolean =
initializationOptions.doctorVisibilityProvider.getOrElse(false)

def codeLenseRefreshSupport(): Boolean = {
val codeLenseRefreshSupport: Option[Boolean] = for {
capabilities <- clientCapabilities
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ final class Compilations(
workspace: () => AbsolutePath,
languageClient: MetalsLanguageClient,
refreshTestSuites: () => Unit,
afterCompilation: () => Unit,
isCurrentlyFocused: b.BuildTargetIdentifier => Boolean,
compileWorksheets: Seq[AbsolutePath] => Future[Unit]
)(implicit ec: ExecutionContext) {
Expand Down Expand Up @@ -222,6 +223,7 @@ final class Compilations(
val result = compilation.asScala
.andThen { case result =>
updateCompiledTargetState(result)
afterCompilation()

// See https://github.com/scalacenter/bloop/issues/1067
classes.rebuildIndex(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ import org.eclipse.{lsp4j => l}
* copy results to the local buffer.
* @param disableColorOutput in the situation where your DAP client may not handle color codes in
* the output, you can enable this to strip them.
* @param doctorVisibilityProvider if the clients implements `metals/doctorVisibilityDidChange`
kpodsiad marked this conversation as resolved.
Show resolved Hide resolved
*/
final case class InitializationOptions(
compilerOptions: CompilerInitializationOptions,
Expand All @@ -73,7 +74,8 @@ final case class InitializationOptions(
testExplorerProvider: Option[Boolean],
openNewWindowProvider: Option[Boolean],
copyWorksheetOutputProvider: Option[Boolean],
disableColorOutput: Option[Boolean]
disableColorOutput: Option[Boolean],
doctorVisibilityProvider: Option[Boolean]
) {
def doctorFormat: Option[DoctorFormat.DoctorFormat] =
doctorProvider.flatMap(DoctorFormat.fromString)
Expand Down Expand Up @@ -109,6 +111,7 @@ object InitializationOptions {
None,
None,
None,
None,
None
)

Expand Down Expand Up @@ -163,7 +166,9 @@ object InitializationOptions {
openNewWindowProvider = jsonObj.getBooleanOption("openNewWindowProvider"),
copyWorksheetOutputProvider =
jsonObj.getBooleanOption("copyWorksheetOutputProvider"),
disableColorOutput = jsonObj.getBooleanOption("disableColorOutput")
disableColorOutput = jsonObj.getBooleanOption("disableColorOutput"),
doctorVisibilityProvider =
jsonObj.getBooleanOption("doctorVisibilityProvider")
)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@ class MetalsLanguageServer(
buildTargets.addData(mainBuildTargetsData)
private val buildTargetClasses =
new BuildTargetClasses(buildTargets)
private var doctor: Doctor = _

private val scalaVersionSelector = new ScalaVersionSelector(
() => userConfig,
Expand All @@ -178,12 +179,18 @@ class MetalsLanguageServer(
buffers,
buildTargets
)

val compilations: Compilations = new Compilations(
buildTargets,
buildTargetClasses,
() => workspace,
languageClient,
() => testProvider.refreshTestSuites(),
() => {
if (clientConfig.isDoctorVisibilityProvider())
doctor.executeRefreshDoctor()
else ()
},
buildTarget => focusedDocumentBuildTarget.get() == buildTarget,
worksheets => onWorksheetChanged(worksheets)
)
Expand Down Expand Up @@ -276,7 +283,6 @@ class MetalsLanguageServer(
var tables: Tables = _
var statusBar: StatusBar = _
private var embedded: Embedded = _
private var doctor: Doctor = _
var httpServer: Option[MetalsHttpServer] = None
var treeView: TreeViewProvider = NoopTreeViewProvider
var worksheetProvider: WorksheetProvider = _
Expand Down Expand Up @@ -424,7 +430,6 @@ class MetalsLanguageServer(
report => {
didCompileTarget(report)
compilers.didCompile(report)
doctor.check()
},
onBuildTargetDidCompile = { target =>
treeView.onBuildTargetDidCompile(target)
Expand Down Expand Up @@ -707,6 +712,7 @@ class MetalsLanguageServer(
diagnostics,
languageClient
)

doctor = new Doctor(
workspace,
buildTargets,
Expand All @@ -720,6 +726,7 @@ class MetalsLanguageServer(
mtagsResolver,
() => userConfig.javaHome
)

fileDecoderProvider = new FileDecoderProvider(
workspace,
compilers,
Expand Down Expand Up @@ -1716,6 +1723,7 @@ class MetalsLanguageServer(
.asJavaObject
case ServerCommands.RunDoctor() =>
Future {
doctor.onVisibilityDidChange(true)
kpodsiad marked this conversation as resolved.
Show resolved Hide resolved
doctor.executeRunDoctor()
}.asJavaObject
case ServerCommands.ListBuildTargets() =>
Expand Down Expand Up @@ -1968,6 +1976,14 @@ class MetalsLanguageServer(
}
}

@JsonNotification("metals/doctorVisibilityDidChange")
def doctorVisibilityDidChange(
params: DoctorVisibilityDidChangeParams
): CompletableFuture[Unit] =
Future {
doctor.onVisibilityDidChange(params.visible)
}.asJava

@JsonRequest("metals/treeViewChildren")
def treeViewChildren(
params: TreeViewChildrenParams
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ final class Doctor(
mtagsResolver: MtagsResolver,
javaHome: () => Option[String]
)(implicit ec: ExecutionContext) {
private val isVisible = new AtomicBoolean(false)
private val hasProblems = new AtomicBoolean(false)
private val problemResolver =
new ProblemResolver(
Expand All @@ -51,6 +52,10 @@ final class Doctor(
() => clientConfig.isTestExplorerProvider()
)

def onVisibilityDidChange(newState: Boolean): Unit = {
isVisible.set(newState)
}

/**
* Returns a full HTML page for the HTTP client.
*/
Expand All @@ -73,7 +78,7 @@ final class Doctor(
def executeRunDoctor(): Unit = {
executeDoctor(
ClientCommands.RunDoctor,
server => {
onServer = server => {
Urls.openBrowser(server.address + "/doctor")
}
)
Expand All @@ -97,33 +102,41 @@ final class Doctor(
def executeRefreshDoctor(): Unit = {
executeDoctor(
ClientCommands.ReloadDoctor,
server => {
onServer = server => {
server.reload()
}
)
}

/**
* @param clientCommand RunDoctor or ReloadDoctor
* @param onServer piece of logic that will be executed when http server is enabled
*/
private def executeDoctor(
clientCommand: ParametrizedCommand[String],
onServer: MetalsHttpServer => Unit
): Unit = {
if (
clientConfig.isExecuteClientCommandProvider && !clientConfig.isHttpEnabled
) {
val output = clientConfig.doctorFormat match {
case DoctorFormat.Json => buildTargetsJson()
case DoctorFormat.Html => buildTargetsHtml()
}
val params = clientCommand.toExecuteCommandParams(output)
languageClient.metalsExecuteClientCommand(params)
} else {
httpServer() match {
case Some(server) =>
onServer(server)
case None =>
scribe.warn(
"Unable to run doctor. Make sure `isHttpEnabled` is set to `true`."
)
val isVisibilityProvider = clientConfig.isDoctorVisibilityProvider()
val shouldDisplay = isVisibilityProvider && isVisible.get()
if (shouldDisplay || !isVisibilityProvider) {
if (
clientConfig.isExecuteClientCommandProvider && !clientConfig.isHttpEnabled
) {
val output = clientConfig.doctorFormat match {
case DoctorFormat.Json => buildTargetsJson()
case DoctorFormat.Html => buildTargetsHtml()
}
val params = clientCommand.toExecuteCommandParams(output)
languageClient.metalsExecuteClientCommand(params)
} else {
httpServer() match {
case Some(server) =>
onServer(server)
case None =>
scribe.warn(
"Unable to run doctor. Make sure `isHttpEnabled` is set to `true`."
)
}
}
}
}
Expand Down Expand Up @@ -562,3 +575,7 @@ final class Doctor(
private val noBuildTargetRecTwo =
"Try removing the directories .metals/ and .bloop/, then restart metals And import the build again."
}

case class DoctorVisibilityDidChangeParams(
visible: Boolean
)