add resource to be opened to evaluate method to provide info for tester

[ghentschke]

fix for #393

[mickaelistria]

LSP4E manipulates more than resources. The common item is the IDocument. Would it be possible to use it instead?

[ghentschke]

The idea is, that the object behind the openResource variable could be any object like IFile, IDocument or IEditorInput and that the enabledWhen tester for contentTypeMapping in the org.eclipse.lsp4e.languageServer extension point should handle this. In my case a PropertyTester checks the object type and handles it appropriately (similar to HasLanguageServerPropertyTester.java)

I named the variable openResource because it should be the object that LPS4E wants to manipulate. Then I can check in my property tester if LSP4E should do that or not. (files in old C-Projects should not cause clangd language server to be started). Trials with editorInput or viewer variables in my test failed because i have to check the file to be opened not the actual.

Maybe the variable name could be better like openObject or object or manipulatedObject ...

[ghentschke]

LSP4E manipulates more than resources. The common item is the IDocument. Would it be possible to use it instead?

Using IDocument now in evaluation contest as default variable now

[mickaelistria]

One important thing is that sometimes, document may not pre-exist. In such case, the operation needs to create the document and then dispose it later. See for example how LSSearchQuery achieves that.

[mickaelistria]

Shouldn't we delete those?

[ghentschke]

We could, then the caller has to use isEnabled(null) when there is no document available (e.g. in org.eclipse.lsp4e.ui.EnableDisableLSJob.run(IProgressMonitor):

	@Override
	protected IStatus run(IProgressMonitor monitor) {
		for (ContentTypeToLanguageServerDefinition changedDefinition : serverDefinitions) {
			LanguageServerDefinition serverDefinition = changedDefinition.getValue();
			if (serverDefinition != null) {
				if (!changedDefinition.isEnabled(null)) {
         ...

I would prefer to delete those methods without argument, because then the developer is urged to provide a IDocument when using this method.

[ghentschke]

One important thing is that sometimes, document may not pre-exist. In such case, the operation needs to create the document and then dispose it later. See for example how LSSearchQuery achieves that.

I'll check that

[mickaelistria]

Shouldn't we delete those?

[ghentschke]

same as above

[mickaelistria]

I would prefer to delete those methods without argument, because then the developer is urged to provide a IDocument when using this method.

OK.

[ghentschke]

@mickaelistria: Updated change. The evaluate method gets an URI and creates a document if its not pre-existing. It will be disposed after the check. Removed isExtensionEnabled() and isEnabled() call without arguments. Had to modify the tests according to the removal.

[mickaelistria]

Using the URI is a good approach. However I think we should avoid setting a default variable; and instead explicitly include it in the context. I am not certain that the URI is the right variable to make default and thus not setting the default variable allows to not make a mistake that will be hard to resolve.
For consistency, we should try to add same variables or at least use the same names for variables as in GenericContentTypeRelatedExtension .
What do you think?

[ghentschke]

For me it's okay to do not use the default variable and use additionalVariables instead. In this case the user has to explicitly use the with statement to access these variables in a PropertyTester. But I think this would be okay.

I would like to add the following variables: document, resource, uri Uri is most flexible, because the user can determine external files. This is very important when handling C/C++ header files. The LS shall not be enabled on all external files.

[ghentschke]

org.eclipse.lsp4e.enablement.EnablementTester.evaluate(URI) looks like this when adding document, resource, and uri as variables. uri ist most reliable because document and resource can be null for external files:

	public boolean evaluate(URI uri) {
		boolean temporaryLoadDocument = false;
		IResource resource = null;
		try {
			IDocument document = null;
			resource = LSPEclipseUtils.findResourceFor(uri);
			if (resource != null) {
				document = LSPEclipseUtils.getExistingDocument(resource);
				if (document == null) {
					document = LSPEclipseUtils.getDocument(resource);
					temporaryLoadDocument = true;
				}
			}
			if (document == null) {
				temporaryLoadDocument = false;
			}
			final var context = new EvaluationContext(parent.get(), new Object());
			context.addVariable("document", //$NON-NLS-1$
					document != null ? document : IEvaluationContext.UNDEFINED_VARIABLE);
			context.addVariable("resource", //$NON-NLS-1$
					resource != null ? resource : IEvaluationContext.UNDEFINED_VARIABLE);
			context.addVariable("uri", //$NON-NLS-1$
					uri != null ? uri : IEvaluationContext.UNDEFINED_VARIABLE);
			context.setAllowPluginActivation(true);
			return expression.evaluate(context).equals(EvaluationResult.TRUE);
		} catch (CoreException e) {
			LanguageServerPlugin.logError("Error occured during evaluation of enablement expression", e); //$NON-NLS-1$
		} finally {
			if (temporaryLoadDocument && resource != null) {
				try {
					FileBuffers.getTextFileBufferManager().disconnect(resource.getFullPath(), LocationKind.IFILE, new NullProgressMonitor());
				} catch (CoreException e) {
					LanguageServerPlugin.logError(e);
				}
			}
		}

[mickaelistria]

Thanks for this useful addition!