Support for textDocument/selectionRange #562
Conversation
|
This PR provides standard shortcut ALT+SHIFT+UP / ALT+SHIFT+DOWN to manage selection range like WTP. For XML, you will need the PR eclipse/lemminx#1507 You can test this PR with HTML language server for instance:
or with CSS language server:
|
angelozerr
force-pushed
the
selectionRange
branch
from
dbe6611
to
1224def
Compare
| if (document != null) { | ||
| LanguageServerDocumentExecutor executor = LanguageServers.forDocument(document) | ||
| .withCapability(ServerCapabilities::getSelectionRangeProvider); | ||
| if (executor.anyMatching()) { |
There was a problem hiding this comment.
should the executor.anyMatching condition not go inside if on the condition handler.isDirty? The else branch does not use the language server, right?
There was a problem hiding this comment.
I did that because I wanted to avoid storing in Styledtext the SelectionRangeHandler instance if there are no LS which supports SelectionRange but I can change that if you prefer.
There was a problem hiding this comment.
I am not sure I understand that, so I do not know if it is better to change it or, but at least we should add a comment why it is like it is at the moment, I think it is not something one can figure out from the code. What do you think?
There was a problem hiding this comment.
When user does several Alt+Sift+UP, Alt+Sift+DOWN, there is just one consume of the textDocument/SelectionRange (the first call of Alt+Sift+UP or Alt+Sift+DOWN. After that the result is cached on an instance of SelectionRangeHandler. When user change the cursor location, SelectionRangeHandler is removed. This instance is stored in the Styledtext (I consider that is an instance of the editor).
BUT if there are no language server which support textDocument/SelectionRange, there is no sense to store an instance of SelectionRangeHandler in a Styledtext.
Do you understand what I mean?
There was a problem hiding this comment.
@rubenporras I tried to add some comments, please tell me if it better and more understandable.
There was a problem hiding this comment.
Yes, it is a bit better now. I understand what you mean in your last post.
BTW: Do you think it would be possible to have some test?
There was a problem hiding this comment.
I fear that I will not have time to do that now -(
....lsp4e/src/org/eclipse/lsp4e/operations/selectionRange/LSPSelectionRangeAbstractHandler.java
Outdated
Show resolved
Hide resolved
angelozerr
force-pushed
the
selectionRange
branch
from
1224def
to
6b585c9
Compare
angelozerr
force-pushed
the
selectionRange
branch
from
6b585c9
to
9ed62c7
Compare
....lsp4e/src/org/eclipse/lsp4e/operations/selectionRange/LSPSelectionRangeAbstractHandler.java
Outdated
Show resolved
Hide resolved
....lsp4e/src/org/eclipse/lsp4e/operations/selectionRange/LSPSelectionRangeAbstractHandler.java
Outdated
Show resolved
Hide resolved
angelozerr
force-pushed
the
selectionRange
branch
from
9ed62c7
to
3edc091
Compare
|
apart from missing tests, it looks good to me, but I cannot invest much time to actually review it properly, maybe @mickaelistria could do the final review? |
As it's all internal and given we already both shared our main concerns and those got covered; I would be for just merging and improve later if need be. |
Support for textDocument/selectionRange
Fixes #561