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.

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

property tester (HasLanguageServerPropertyTester) not operating as expected #2

Closed
jonahgraham opened this issue Feb 16, 2023 · 3 comments
Milestone

Comments

@jonahgraham
Copy link
Member

The receiver that arrives in the property tester is sometimes just a simple Object instance.

I think that eclipse/lsp4e#393 is for the same issue, but with the current LSP4E snapshot build I made this change to unblock me. Things were working in e44c41f before the UI stuff went in.

$ git diff
diff --git a/org.eclipse.cdt.lsp/src/org/eclipse/cdt/lsp/server/HasLanguageServerPropertyTester.java b/org.eclipse.cdt.lsp/src/org/eclipse/cdt/lsp/server/HasLanguageServerPropertyTester.java
index 152dd7b..8081d47 100644
--- a/org.eclipse.cdt.lsp/src/org/eclipse/cdt/lsp/server/HasLanguageServerPropertyTester.java
+++ b/org.eclipse.cdt.lsp/src/org/eclipse/cdt/lsp/server/HasLanguageServerPropertyTester.java
@@ -24,9 +24,6 @@ public class HasLanguageServerPropertyTester extends PropertyTester {
 
        @Override
        public boolean test(Object receiver, String property, Object[] args, Object expectedValue) {
-               if (cLanguageServerProvider != null) {
-                       return cLanguageServerProvider.isEnabled(receiver);
-               }
                return true;
        }

So for now I raise this to track best workaround until LSP4E is resolved. (Is best workaround to use eclipse/lsp4e#400?)

@ghentschke
Copy link
Contributor

So for now I raise this to track best workaround until LSP4E is resolved. (Is best workaround to use eclipse/lsp4e#400?)

You're right. I still working on the LS enabledWhen issue of the contentTypeMapping.

The issue to be fixed is: prevent enabling of LS when switching editor tabs in CDT.
Test case would look like this:
GIVEN are several editor tabs opened in the C/C++ Editor as well as in the C/C++ Editor (LSP)
WHEN switching from an C/C++ Editor (LSP) editor tab to an C/C++ Editor tab with an external file
THEN the LS won't be enabled for the external file in the C/C++ Editor tab

The change #400 in LSP4E is important to solve that issue here. LSP4E has to provide the URI and the Resource of the file to be opened as additonal context variables. Then we can fetch these variables for the PropertyTester via the with statement in the plugin.xml.

The resource will be empty for external files (e.g. system header files). In this case URI jumps in:
There are two scenarios for opening/jumping to a file via F3 from within the source code:

  1. The external file is already opened in an editor tab: check first whether this file is already opened in a tab, if so check the type of the editor. If it's the LSP based editor return true, otherwise false.
  2. If the external file is not already opened, check the active editor. If it's the LSP editor, return true, otherwise false.

ghentschke added a commit to ghentschke/eclipse-cdt-lsp that referenced this issue Feb 16, 2023
temporary fix until LSP4E has been fixed #393
@ghentschke
Copy link
Contributor

@jonahgraham I think the PR #7 should fix this and is the preparation for LSP4E issue #400

With this change, the automatic detection of the editor to open a file with should be fixed. Except tab switching is still erroneous.

ghentschke added a commit that referenced this issue Feb 17, 2023
fix: property tester not operating as expected (#2)
@ghentschke
Copy link
Contributor

fixed with PR #7

@jonahgraham jonahgraham added this to the 1.0.0 milestone Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants