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.

Sign up for GitHub

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

Jump to bottom

Configure limit for textDocument/documentSymbol #637

Merged
merged 8 commits into from
Apr 20, 2020
Merged

Configure limit for textDocument/documentSymbol #637

merged 8 commits into from
Apr 20, 2020

Conversation

xorye
Copy link

@xorye xorye commented Apr 15, 2020

This PR is for redhat-developer/vscode-xml#237 . I would appreciate some feedback on it so far.

This PR currently implements the fix only for SymbolInformations.

Here is the vscode-xml PR that goes along with this PR: redhat-developer/vscode-xml#238

Signed-off-by: David Kwon [email protected]

@xorye xorye changed the title Draft PR for maxItemsComputed [Draft PR] for maxItemsComputed Apr 15, 2020
@angelozerr
Copy link
Contributor

@xorye see my draft PR #638 which should fix all issues that you have noticed.

The main idea of my draft PR is to:

  • return SymbolInformationsResult instead of List<SymbolInformation> from XMLSymbolProvider.
public class SymbolInformationsResult extends ArrayList<SymbolInformation> {

	private static final long serialVersionUID = 1L;

	private transient boolean resultLimitExceeded;

	public boolean isResultLimitExceeded() {
		return resultLimitExceeded;
	}

	public void setResultLimitExceeded(boolean resultLimitExceeded) {
		this.resultLimitExceeded = resultLimitExceeded;
	}

}

  • check in XMLTextDocumentService if isResultLimitExceeded is equals to true to send the JSON notification to the client.

  • when symbols are added in the XMLSymbolProvider, throw a private exception ResultLimitExceededException in XMLSymbolProvider to stop the adding of symbol.

Please note I write my PT quickly without testing it, perhaps there are some bugs.

I would like to avoid using standard LSP showMessage and I would prefer having a custom LSP notification for that (like JSON LS does), I mean having a custom notification 'xml/resultLimitReached'. Client could react of this notification with other mean than just display a message if he wants.

@xorye
Copy link
Author

xorye commented Apr 16, 2020

Please note, my latest push is missing lots of javadoc, I will address that tomorrow

Demo: After setting xml.symbols.maxItemsComputed to 9 in the VS Code settings, this message appears when document symbols are computed:
image

angelozerr
angelozerr reviewed Apr 20, 2020
View reviewed changes
org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/XMLLanguageServer.java Outdated
@@ -27,6 +27,7 @@
import org.eclipse.lemminx.commons.ModelTextDocument;
import org.eclipse.lemminx.commons.ParentProcessWatcher.ProcessLanguageServer;
import org.eclipse.lemminx.customservice.AutoCloseTagResponse;
import org.eclipse.lemminx.customservice.XMLLanguageClientAPI;
import org.eclipse.lemminx.customservice.XMLCustomService;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you rename XMLCustomService to XMLLanguageServerAPI please.

angelozerr
angelozerr reviewed Apr 20, 2020
View reviewed changes
org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/XMLServerLauncher.java Outdated
* @param wrapper - a function for plugging in additional message
* consumers
*/
public static Launcher<LanguageClient> createServerLauncher(LanguageServer server, InputStream in, OutputStream out,
Copy link
Contributor

Choose a reason for hiding this comment

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

private static

angelozerr
angelozerr reviewed Apr 20, 2020
View reviewed changes
org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/XMLTextDocumentService.java Outdated
@@ -460,6 +492,10 @@ public void updateCodeLensSettings(XMLCodeLensSettings newSettings) {
codeLensSettings.setEnabled(newSettings.isEnabled());
}

public void updateMaxItemsComputed(int maxItemsComputed) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add Javadoc

@xorye xorye changed the title [Draft PR] for maxItemsComputed Document symbols limit for textDocument/documentSymbol Apr 20, 2020
@xorye
Copy link
Author

xorye commented Apr 20, 2020

@angelozerr for the new javadoc, I am planning to use
@see org.eclipse.lemminx.customservice.ActionableNotification
instead of
See ActionableNotification

Would that be good?

dkwon17 added 2 commits April 20, 2020 12:11
@dkwon17
Address PR feedback
e3f6405 
Signed-off-by: David Kwon <[email protected]>
@dkwon17
Add javadoc links
37fe737 
Signed-off-by: David Kwon <[email protected]>
@fbricon fbricon merged commit f9cb5dc into eclipse-lemminx:master Apr 20, 2020
@fbricon fbricon changed the title Document symbols limit for textDocument/documentSymbol Configure limit for textDocument/documentSymbol Apr 20, 2020
@fbricon fbricon added enhancement New feature or request symbols labels Apr 20, 2020
@fbricon fbricon added this to the 0.12.0 milestone Apr 20, 2020
fbricon
fbricon reviewed Apr 21, 2020
View reviewed changes
org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/XMLTextDocumentService.java
* @param xmlDocument the xml document that symbols were being computed for
* @param symbolSettings the symbol settings
*/
private void sendSymbolLimitNotification(DOMDocument xmlDocument, XMLSymbolSettings symbolSettings) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should keep track whether the message has already been sent for this URI, for the rest of this JVM session, i.e keep a static Set of impacted URIs. See redhat-developer/vscode-xml#243

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@angelozerr angelozerr angelozerr left review comments

@fbricon fbricon fbricon left review comments

Assignees
No one assigned
Labels
enhancement New feature or request symbols
Projects
None yet
Milestone
0.12.0
Development

Successfully merging this pull request may close these issues.
4 participants
@xorye @angelozerr @fbricon @dkwon17