treeViewProtocol improvements

[ghostbuster91]

The main purpose of this ticket is to gather all information about how the treeViewProtocol works, what are the current problems and how it could be improved/simplified.

Current implementation problems

1. Server does not keep track of expanded/collapsed nodes metals/treeViewVisibilityDidChange

   The visibility did change notification is sent from the client to the server to notify that the visibility of a tree view has changed.

   However, if we then request for treeViewChildren on the root level we will get all the nodes with state collapsed despite all the notification that have been sent. This is problematic when combined with metals/treeViewVisibilityDidChange.

   The collapse did change notification is sent from the client to the server to notify that a tree node has either been collapsed or expanded.

   The natural reasoning for this is to stop sending metals/treeViewDidChange notifications from the server if the tree is not visible which saves some cpu cycles. But if we stop sending notification about changes, then the client has to request the full info when is reopened but then it loses information about expanded/collapsed nodes. Unless some more complex logic is implemented on the client side, but then why bother sending metals/treeViewVisibilityDidChange to the server?

   The question is which way to go. I think that it would be great to keep this state on the server, as it would decrease code duplication on the clients.

   Note: I found that in the code it seems to do nothing for non-root nodes - https://github.com/scalameta/metals/blob/v1.2.0/metals/src/main/scala/scala/meta/internal/tvp/MetalsTreeViewProvider.scala#L356

2. Server does not respond with capabilities.experimental.treeViewProvider According to the documentation:

   The Tree View Protocol is only enabled when both the client and server declare support for the protocol by adding an treeViewProvider: true field to the experimental section of the server and client capabilities in the initialize response.

   Then I came across this https://github.com/scalameta/metals/blob/7a8476107efe7be2608fdae82e819730d1325edd/website/blog/2020-07-23-configuring-a-client.md?plain=1#L174 which says that these options were moved to MetalsInitializationOptions. So probably the main documentation should be updated?

   That is not all yet as the server does not declar treeViewProvider in its capabilities. This is bad because it prevents ui clients from finding lsp server they should communicate with.

   client initialize request params

   ```json { "processId": 1674327, "rootPath": "/home/kghost/workspace/bootzooka", "rootUri": "file:///home/kghost/workspace/bootzooka", "initializationOptions": { "bspStatusBarProvider": "on", "statusBarProvider": "on", "compilerOptions": [], "debuggingProvider": true, "testExplorerProvider": true, "decorationProvider": true, "didFocusProvider": true, "disableColorOutput": true, "doctorProvider": "json", "doctorVisibilityProvider": true, "executeClientCommandProvider": true, "inputBoxProvider": true, "quickPickProvider": true, "treeViewProvider": true }, "capabilities": { "workspace": { "applyEdit": true, "workspaceEdit": { "resourceOperations": [ "rename", "create", "delete" ] }, "didChangeWatchedFiles": { "relativePatternSupport": true, "dynamicRegistration": false }, "symbol": { "symbolKind": { "valueSet": [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26 ] }, "dynamicRegistration": false }, "workspaceFolders": true, "configuration": true, "semanticTokens": { "refreshSupport": true } }, "textDocument": { "synchronization": { "willSave": true, "willSaveWaitUntil": true, "didSave": true, "dynamicRegistration": false }, "completion": { "completionItem": { "snippetSupport": true, "commitCharactersSupport": true, "documentationFormat": [ "markdown", "plaintext" ], "deprecatedSupport": true, "preselectSupport": true, "tagSupport": { "valueSet": [ 1 ] }, "insertReplaceSupport": true, "resolveSupport": { "properties": [ "documentation", "detail", "additionalTextEdits", "sortText", "filterText", "insertText", "textEdit", "insertTextFormat", "insertTextMode" ] }, "insertTextModeSupport": { "valueSet": [ 1, 2 ] }, "labelDetailsSupport": true }, "completionItemKind": { "valueSet": [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25 ] }, "contextSupport": true, "insertTextMode": 1, "completionList": { "itemDefaults": [ "commitCharacters", "editRange", "insertTextFormat", "insertTextMode", "data" ] }, "dynamicRegistration": false }, "hover": { "contentFormat": [ "markdown", "plaintext" ], "dynamicRegistration": false }, "signatureHelp": { "signatureInformation": { "documentationFormat": [ "markdown", "plaintext" ], "parameterInformation": { "labelOffsetSupport": true }, "activeParameterSupport": true }, "dynamicRegistration": false }, "references": { "dynamicRegistration": false }, "documentHighlight": { "dynamicRegistration": false }, "documentSymbol": { "symbolKind": { "valueSet": [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26 ] }, "hierarchicalDocumentSymbolSupport": true, "dynamicRegistration": false }, "declaration": { "linkSupport": true }, "definition": { "linkSupport": true }, "typeDefinition": { "linkSupport": true }, "implementation": { "linkSupport": true }, "codeAction": { "codeActionLiteralSupport": { "codeActionKind": { "valueSet": [ "", "quickfix", "refactor", "refactor.extract", "refactor.inline", "refactor.rewrite", "source", "source.organizeImports" ] } }, "isPreferredSupport": true, "dataSupport": true, "resolveSupport": { "properties": [ "edit" ] }, "dynamicRegistration": false }, "rename": { "prepareSupport": true, "dynamicRegistration": false }, "publishDiagnostics": { "relatedInformation": true, "tagSupport": { "valueSet": [ 1, 2 ] } }, "callHierarchy": { "dynamicRegistration": false }, "semanticTokens": { "requests": { "range": false, "full": { "delta": true } }, "tokenTypes": [ "namespace", "type", "class", "enum", "interface", "struct", "typeParameter", "parameter", "variable", "property", "enumMember", "event", "function", "method", "macro", "keyword", "modifier", "comment", "string", "number", "regexp", "operator", "decorator" ], "tokenModifiers": [ "declaration", "definition", "readonly", "static", "deprecated", "abstract", "async", "modification", "documentation", "defaultLibrary" ], "formats": [ "relative" ], "overlappingTokenSupport": true, "multilineTokenSupport": false, "serverCancelSupport": false, "augmentsSyntaxTokens": true, "dynamicRegistration": false } }, "window": { "workDoneProgress": true, "showMessage": { "messageActionItem": { "additionalPropertiesSupport": false } }, "showDocument": { "support": true } }, "experimental": { "treeViewProvider": true } }, "clientInfo": { "name": "Neovim", "version": "0.9.5" }, "trace": "off", "workspaceFolders": [ { "uri": "file:///home/kghost/workspace/bootzooka", "name": "/home/kghost/workspace/bootzooka" } ] } ```
   server init response

   ```json { "capabilities": { "textDocumentSync": { "openClose": true, "change": 1, "save": { "includeText": true } }, "hoverProvider": true, "completionProvider": { "resolveProvider": true, "triggerCharacters": [ ".", "*" ] }, "signatureHelpProvider": { "triggerCharacters": [ "(", "[", "," ] }, "definitionProvider": true, "typeDefinitionProvider": true, "implementationProvider": true, "referencesProvider": true, "documentHighlightProvider": true, "documentSymbolProvider": true, "workspaceSymbolProvider": true, "codeActionProvider": { "codeActionKinds": [ "quickfix", "refactor", "source.organizeImports" ] }, "codeLensProvider": { "resolveProvider": false }, "documentFormattingProvider": true, "documentRangeFormattingProvider": true, "documentOnTypeFormattingProvider": { "firstTriggerCharacter": "\n", "moreTriggerCharacter": [ "\"" ] }, "renameProvider": { "prepareProvider": true }, "foldingRangeProvider": true, "executeCommandProvider": { "commands": [ "analyze-stacktrace", "zip-reports", "list-build-targets", "extract-method", "debug-adapter-start", "new-scala-file", "build-connect", "reset-workspace", "doctor-run", "insert-inferred-type", "build-restart", "discover-jvm-run-command", "generate-bsp-config", "build-disconnect", "copy-worksheet-output", "presentation-compiler-restart", "browser-open-url:https://github.com/scalameta/metals-feature-requests/issues/new?template\u003dfeature-request.yml", "reset-choice", "goto", "open-new-github-issue", "new-scala-project", "ammonite-stop", "scalafix-run", "build-import", "inline-value", "sources-scan", "bsp-switch", "new-java-file", "reset-notifications", "extract-member-definition", "ammonite-start", "compile-cancel", "goto-super-method", "goto-position", "compile-cascade", "convert-to-named-arguments", "discover-tests", "scala-cli-stop", "super-method-hierarchy", "scala-cli-start", "file-decode", "compile-clean", "scalafix-run-only" ] }, "workspace": { "workspaceFolders": { "supported": true, "changeNotifications": true }, "fileOperations": { "willRename": { "filters": [ { "pattern": { "glob": "**/*.scala", "matches": "file" } }, { "pattern": { "glob": "**/", "matches": "folder" } } ] } } }, "callHierarchyProvider": true, "selectionRangeProvider": true, "semanticTokensProvider": { "legend": { "tokenTypes": [ "namespace", "type", "class", "enum", "interface", "struct", "typeParameter", "parameter", "variable", "property", "enumMember", "event", "function", "method", "macro", "keyword", "modifier", "comment", "string", "number", "regexp", "operator", "decorator" ], "tokenModifiers": [ "declaration", "definition", "readonly", "static", "deprecated", "abstract", "async", "modification", "documentation", "defaultLibrary" ] }, "range": false, "full": true }, "experimental": { "rangeHoverProvider": true } }, "serverInfo": { "name": "Metals", "version": "1.2.0" } } ```

3. Expanding nodes under Project does not work for non-compiled targets

   If I try to open a node under Project tab metals will return empty list of child nodes. Unless I first explicitly call compile cascade. Shouldn't then compilation be invoked on demand in such cases? If so, will metals send treeViewDidChange notification once it is ready? Will it send anything in the meantime indicating that there is a process in-progress?

Having said that, I am happy to do the necessary changes (updating docs, fixing implementation etc) once we decide what should be changed and how.

[tgodzik]

Server does not keep track of expanded/collapsed nodes

I don't think we should keep the state on the server, since the actual UI is on the client and it could easily go out of sync. However, I do understand that it would make it easier for clients, so I am not 100% against keeping the additional state on the server. But we should figure out a way for the client to get more optimal updates. I think the performance aspect currently is not greatly optimized.

Server does not respond with capabilities.experimental.treeViewProvider

This probably slipped under the radar, would be good to fix it.

Expanding nodes under Project does not work for non-compiled targets

I based the implementation on semanticdb at the time, but it might actually be better to rework that since we don't really need semanticdb. We could use the same as we do for outline. We need to make sure however that updates are being sent properly when files change.

[ghostbuster91]

btw the 3rd point is actually a regression as it used to work in metals 1.1.0:

here is what I used to get from metals 1.1.0 after expanding a project node:

``` [Trace - 08:42:09 PM] Received notification 'metals/treeViewNodeCollapseDidChange' Params: { "viewId": "metalsPackages", "nodeUri": "projects-/home/kghost/workspace/bootzooka:file:/home/kghost/workspace/bootzooka/backend/?id\u003dbackend!/_root_/", "collapsed": false } [Trace - 08:42:09 PM] Received request 'metals/treeViewChildren - (11)' Params: { "viewId": "metalsPackages", "nodeUri": "projects-/home/kghost/workspace/bootzooka:file:/home/kghost/workspace/bootzooka/backend/?id\u003dbackend!/_root_/" } [Trace - 08:42:09 PM] Sending response 'metals/treeViewChildren - (11)'. Processing request took 6ms Result: { "nodes": [] } [Trace - 08:42:09 PM] Sending notification 'metals/status' Params: { "text": "Compiling backend ", "level": "info", "show": true, "statusType": "metals" } [Trace - 08:42:09 PM] Sending notification 'metals/executeClientCommand' Params: { "command": "metals-model-refresh", "arguments": [] } [Trace - 08:42:09 PM] Sending notification 'metals/status' Params: { "text": "", "level": "info", "hide": true, "statusType": "metals" } ``` The first `treeViewChildren` response contain empty result but when we ask for the second time after collapsing/expanding a node metals correctly returns children. This however prevents any kind of caching on the UI. ``` [Trace - 00:42:57 PM] Received request 'metals/treeViewChildren - (12)' Params: { "viewId": "metalsPackages", "nodeUri": "projects-/home/kghost/workspace/bootzooka:file:/home/kghost/workspace/bootzooka/backend/?id\u003dbackend!/_root_/" } [Trace - 00:42:57 PM] Sending response 'metals/treeViewChildren - (12)'. Processing request took 9ms Result: { "nodes": [ { "viewId": "metalsPackages", "nodeUri": "projects-/home/kghost/workspace/bootzooka:file:/home/kghost/workspace/bootzooka/backend/?id\u003dbackend!/com/", "label": "com/", "tooltip": "com/", "collapseState": "expanded" } ] } ```

and here is what I get on 1.2.0:

``` [Trace - 08:44:36 PM] Received notification 'metals/treeViewNodeCollapseDidChange' Params: { "viewId": "metalsPackages", "nodeUri": "projects-/home/kghost/workspace/bootzooka:file:/home/kghost/workspace/bootzooka/backend/?id\u003dbackend!/_root_/", "collapsed": false } [Trace - 08:44:36 PM] Received request 'metals/treeViewChildren - (9)' Params: { "viewId": "metalsPackages", "nodeUri": "projects-/home/kghost/workspace/bootzooka:file:/home/kghost/workspace/bootzooka/backend/?id\u003dbackend!/_root_/" } [Trace - 08:44:36 PM] Sending response 'metals/treeViewChildren - (9)'. Processing request took 3ms Result: { "nodes": [] } ``` The problem here is that metals keep responding for subsequent requests with empty results ``` [Trace - 01:20:34 PM] Received request 'metals/treeViewChildren - (10)' Params: { "viewId": "metalsPackages", "nodeUri": "projects-/home/kghost/workspace/bootzooka:file:/home/kghost/workspace/bootzooka/backend/?id\u003dbackend!/_root_/" } [Trace - 01:20:34 PM] Sending response 'metals/treeViewChildren - (10)'. Processing request took 1ms Result: { "nodes": [] } ```

[ghostbuster91]

There is one more thing, when expanding a node, in case the returned node contains only one child metals returns its collapseState as expanded. It seems that in this case UI is supposed to continue sending treeViewChildren request for such nodes until a node that is collapsed is reached.

trace

``` [Trace - 00:42:57 PM] Received request 'metals/treeViewChildren - (12)' Params: { "viewId": "metalsPackages", "nodeUri": "projects-/home/kghost/workspace/bootzooka:file:/home/kghost/workspace/bootzooka/backend/?id\u003dbackend!/_root_/" } [Trace - 00:42:57 PM] Sending response 'metals/treeViewChildren - (12)'. Processing request took 9ms Result: { "nodes": [ { "viewId": "metalsPackages", "nodeUri": "projects-/home/kghost/workspace/bootzooka:file:/home/kghost/workspace/bootzooka/backend/?id\u003dbackend!/com/", "label": "com/", "tooltip": "com/", "collapseState": "expanded" } ] } [Trace - 00:42:57 PM] Received request 'metals/treeViewChildren - (13)' Params: { "viewId": "metalsPackages", "nodeUri": "projects-/home/kghost/workspace/bootzooka:file:/home/kghost/workspace/bootzooka/backend/?id\u003dbackend!/com/" } [Trace - 00:42:57 PM] Sending response 'metals/treeViewChildren - (13)'. Processing request took 5ms Result: { "nodes": [ { "viewId": "metalsPackages", "nodeUri": "projects-/home/kghost/workspace/bootzooka:file:/home/kghost/workspace/bootzooka/backend/?id\u003dbackend!/com/softwaremill/", "label": "softwaremill/", "tooltip": "com/softwaremill/", "collapseState": "expanded" } ] } [Trace - 00:42:57 PM] Received request 'metals/treeViewChildren - (14)' Params: { "viewId": "metalsPackages", "nodeUri": "projects-/home/kghost/workspace/bootzooka:file:/home/kghost/workspace/bootzooka/backend/?id\u003dbackend!/com/softwaremill/" } [Trace - 00:42:57 PM] Sending response 'metals/treeViewChildren - (14)'. Processing request took 8ms Result: { "nodes": [ { "viewId": "metalsPackages", "nodeUri": "projects-/home/kghost/workspace/bootzooka:file:/home/kghost/workspace/bootzooka/backend/?id\u003dbackend!/com/softwaremill/bootzooka/", "label": "bootzooka/", "tooltip": "com/softwaremill/bootzooka/", "collapseState": "expanded" } ] } [Trace - 00:42:57 PM] Received request 'metals/treeViewChildren - (15)' Params: { "viewId": "metalsPackages", "nodeUri": "projects-/home/kghost/workspace/bootzooka:file:/home/kghost/workspace/bootzooka/backend/?id\u003dbackend!/com/softwaremill/bootzooka/" } [Trace - 00:42:57 PM] Sending response 'metals/treeViewChildren - (15)'. Processing request took 165ms Result: { "nodes": [ { "viewId": "metalsPackages", "nodeUri": "projects-/home/kghost/workspace/bootzooka:file:/home/kghost/workspace/bootzooka/backend/?id\u003dbackend!/com/softwaremill/bootzooka/config/", "label": "config/", "tooltip": "com/softwaremill/bootzooka/config/", "collapseState": "collapsed" } ]} ```

Maybe this could be changed to a single request-response exchange?

[ghostbuster91]

Last but not least, similarly to the one above, reveal in tree workflow could be improved as well. Currently it returns list of nodes ids to recursively expand. So every reveal in tree action consists always of n + 1 calls (one for the list of ids and then one for every item on the list). I think that it could be simplified if we had introduced an option to fetch all list items at once. Sure it will be more data than doing it one by one, but due to the semantics of that action I think that it won't be that much more.

It could look as follow:

Request:

    method: metals/treeViewRevealChildren
    uriChain: string[];

Response:

interface TreeViewRevealChildrenResult {
  /** Expanded child nodes from the root down to the requested node. */
  nodes: TreeViewNode[];
}

where we would pass uriChain from the metals/treeViewReveal response.

Why not modify metals/treeViewReveal to return TreeViewRevealChildrenResult directly?

It is a viable alternative but I feel that having it separated offers greater modularity fixing at the same time the main downside of current reveal in tree workflow.

[tgodzik]

Finally managed to take a look at what you are suggesting. It all looks like valid improvements to the protocol, but we just didn't have the time to refactor it properly.

With the current implementation it looks like the state is cached on the server side, while in reality just some of it is.

I would rather simplify it greatly so that client has the state of the tree view and we make sure requests are done efficiently as suggested in your improvements.

Or if we decide to keep the state on the server the implementation of the tree view inside metals would need to be heavily reworked.

The issue with empty results being sent previously were probably related to no semanticdb being available, which should not be an issue no.