-
Notifications
You must be signed in to change notification settings - Fork 184
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
Add support for folding range request #2304
Conversation
Looks like CI is broken on Mac 🤷 |
This seems to be a general issue caused by incompatibility to outdated OpenSSL/libcrypto dependencies on macOS, and will persist until a Package Control version 4.0 is released: wbond/package_control#1612 (comment) We should disable CI on macOS in the meanwhile. For this PR, I noticed a minor bug which will show a greyed out "Lsp Fold" entry in the "Edit" menu, when the sidebar has focus. This is the same problem as previously described in #2187 and could be fixed by using a WindowCommand instead (but I think doing that would have other disadvantages, so I'm not sure yet whether it's feasible here). |
I've also added a |
Another idea I had would be to add a new user setting like {
// Controls the initial state of folding regions when a document gets opened.
// Note that only a few language servers have support for this.
"initial_folding_collapsed_state": [ // TODO find a better name
// "comment",
// "imports",
// "region",
],
} Then we could run the request in |
Added in b5a9d4f, since there were no objections in the meanwhile ;) |
// { | ||
// "caption": "LSP: Fold All Comment Blocks", | ||
// "command": "lsp_fold_all", | ||
// "args": {"kind": "comment"} | ||
// }, |
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.
So I guess that by leaving it disabled you've expected some feedback on it? I think that we should either remove or enable (leaning towards enabling) since it makes no sense to have commented out code in this file.
EDIT: I re-read your comment about it possibly not doing anything but I guess that's fine.
To improve that case, we can potentially just show some message in the status bar when nothing was done.
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.
I'm a bit undecided about it, but my intention here was to keep it in the file as an example, so it can be used as a reference in case someone asks how to add such an entry into the command palette, or maybe we decide to activate in the future. I think it would potentially be more appropriate if an LSP-* helper package would add this command, e.g. as "LSP-javascript: Fold All Comment Blocks", because they know whether the server supports the kind
or not. In that case it would be best to create a derived command like
from LSP.plugin.folding_range import LspFoldAllCommand
class LspJavascripFoldAllCommand(LspFoldAllCommand):
session_name = 'LSP-javascript'
and then use that command, so that it is only active for the particular server.
// Determines ranges which initially should be folded when a document is opened, | ||
// provided that the language server has support for this. | ||
"initially_folded": [ | ||
// "comment", | ||
// "imports", | ||
// "region", | ||
], | ||
|
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.
I don't know how I feel about adding it... Personally I would think that it would be kinda annoying to use but it's your call - I don't have strength to argue one way or another ;)
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.
Personally I also wouldn't use it (in general I rarely use folding - and if I do, only with the gutter buttons), but this was a request for the PHP server, which is linked above, and I also saw several requests for this in VS Code. Apparently it's a feature now in VS Code and in several other IDEs, see e.g. microsoft/vscode#40338 (comment) and microsoft/vscode#20802. The code needed for the implementation is quite simple, so I would see it as a potentially useful feature and would keep it. The only thing I'm not really happy with, is the setting name, but I don't have a better idea. Maybe something like "auto_fold_on_load" could be an alternative. VSCode has "editor.foldingImportsByDefault" as a boolean, but I think the design used here is better to allow any kinds.
ST doesn't provide an API to show fold buttons in the gutter for custom (LSP) folding ranges, but I think we can still make it available via the menu or a key binding. It could be useful if there is no built-in indentation based folding, and no ST syntax based folding available. And maybe one day it will be possible to provide the folding regions to the gutter via the API.
Example with LSP-TexLab (to be honest, the folding ranges from this server are a bit strange, but it could be the server's design decision):
fold.webm
LaTeX code
In practice it is probably more useful to define a key binding, instead of using the menu.
The advantage is that this feature causes almost no overhead, because the request is only sent on demand when you open the "Edit" menu or when invoked via key binding.
Initially I tried to make it available via the context menu, but it turned out to not really work, because for the context menu ST will call the command's
description
/is_visible
methods for submenu entries at the same time as for top-level menu entries. (The trick for the "Edit" menu is, that ST only calls those methods for submenu entries at the time when the submenu gets opened. So we already have the server response when that happens, and can show/hide the entry and an appropriate description if applicable - similar to the code actions in the menu.)