-
Notifications
You must be signed in to change notification settings - Fork 388
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
Disable the keybinding of formatting document #272
Conversation
@@ -45,7 +50,8 @@ | |||
|
|||
private Severity incompleteClasspathSeverity; | |||
private FeatureStatus updateBuildConfigurationStatus; | |||
private boolean referencesCodeLensEnabled; | |||
private boolean referencesCodeLensEnabled = true; | |||
private boolean javaFormatEnable = true; |
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.
javaFormatEnabled
@@ -133,6 +144,11 @@ private Preferences setReferencesCodelensEnabled(boolean enabled) { | |||
return this; | |||
} | |||
|
|||
public Preferences setJavaFormatEnable(boolean enable) { |
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.
setJavaFormatEnabled
@@ -155,6 +171,10 @@ public boolean isReferencesCodeLensEnabled() { | |||
return referencesCodeLensEnabled; | |||
} | |||
|
|||
public boolean isJavaFormatEnable() { |
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.
isJavaFormatEnabled
@@ -28,6 +28,11 @@ | |||
public static final String REFERENCES_CODE_LENS_ENABLED_KEY = "java.referencesCodeLens.enabled"; | |||
|
|||
/** | |||
* Preference key to enable/disable formatter. | |||
*/ | |||
public static final String JAVA_FORMAT_ENABLE_KEY = "java.format.enable"; |
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 think we need to be consistent with ourselves here, even if we break consistency with vscode and its javascript.referencesCodeLens.enabled
vs javascript.format.enable
pref keys. So I'd use java.format.enabled
instead
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.
The PR uses the client/registerFeature message because the client/registerCapability message doesn't work. |
The latest vscode-languageclient correctly uses client/registerCapability (my version in nodes_modules is 3.2.2), That's what should be used |
@@ -90,8 +90,8 @@ InitializeResult initialize(InitializeParams param){ | |||
capabilities.setWorkspaceSymbolProvider(Boolean.TRUE); | |||
capabilities.setReferencesProvider(Boolean.TRUE); | |||
capabilities.setDocumentHighlightProvider(Boolean.TRUE); | |||
capabilities.setDocumentFormattingProvider(Boolean.TRUE); | |||
capabilities.setDocumentRangeFormattingProvider(Boolean.TRUE); | |||
// capabilities.setDocumentFormattingProvider(Boolean.TRUE); |
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.
the capabilities should be set during init, if the client doesn't support dynamic capabilities
@@ -176,6 +181,25 @@ public void didChangeConfiguration(DidChangeConfigurationParams params) { | |||
Preferences prefs = Preferences.createFrom(javaConfig); | |||
preferenceManager.update(prefs); | |||
} | |||
if (!preferenceManager.getPreferences().isJavaFormatEnabled()) { | |||
Unregistration unregistration = new Unregistration(Preferences.FORMATTING_ID, Preferences.TEXT_DOCUMENT_FORMATTING); |
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.
should check if dynamic registration of formatting capabilities is available.
in ClientPreferences, new method should check if v3Supported and capabilities.getTextDocument().getFormatting().getDynamicRegistration() is true
Unregistration unregistration = new Unregistration(Preferences.FORMATTING_ID, Preferences.TEXT_DOCUMENT_FORMATTING); | ||
List<Unregistration> unregistrations = new ArrayList<>(); | ||
unregistrations.add(unregistration); | ||
unregistration = new Unregistration(Preferences.FORMATTING_RANGE_ID, Preferences.TEXT_DOCUMENT_RANGE_FORMATTING); |
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.
should check if dynamic registration of range formatting capabilities are available.
in ClientPreferences, new method should check if v3Supported and capabilities.getTextDocument().getRangeFormatting().getDynamicRegistration() is true
UnregistrationParams unregistrationParams = new UnregistrationParams(unregistrations); | ||
client.unregisterFeature(unregistrationParams); | ||
// client.unregisterCapability(unregistrationParams); | ||
} else { |
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.
need same checks
@@ -43,6 +47,12 @@ | |||
*/ | |||
@JsonNotification("language/actionableNotification") | |||
void sendActionableNotification(ActionableNotification notification); | |||
|
|||
@JsonRequest("client/unregisterFeature") | |||
CompletableFuture<Void> unregisterFeature(UnregistrationParams params); |
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.
unnecessary new method. please remove it.
CompletableFuture<Void> unregisterFeature(UnregistrationParams params); | ||
|
||
@JsonRequest("client/registerFeature") | ||
CompletableFuture<Void> registerFeature(RegistrationParams params); |
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.
unnecessary new method. please remove it.
* register a new feature handler on the client side. | ||
*/ | ||
@JsonRequest("client/registerFeature") | ||
public void registerFeature(RegistrationParams params) { |
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.
unnecessary new method. please remove it.
* to register a new feature handler on the client side. | ||
*/ | ||
@JsonRequest("client/unregisterFeature") | ||
public void unregisterFeature(UnregistrationParams params) { |
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.
unnecessary new method. please remove it.
List<Unregistration> unregistrations = new ArrayList<>(); | ||
unregistrations.add(unregistration); | ||
UnregistrationParams unregistrationParams = new UnregistrationParams(unregistrations); | ||
client.unregisterFeature(unregistrationParams); |
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.
use client.unregisterCapability
Unregistration unregistration = new Unregistration(Preferences.FORMATTING_RANGE_ID, Preferences.TEXT_DOCUMENT_RANGE_FORMATTING); | ||
unregistrations.add(unregistration); | ||
UnregistrationParams unregistrationParams = new UnregistrationParams(unregistrations); | ||
client.unregisterFeature(unregistrationParams); |
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.
use client.unregisterCapability
List<Registration> registrations = new ArrayList<>(); | ||
registrations.add(registration); | ||
RegistrationParams registrationParams = new RegistrationParams(registrations); | ||
client.registerFeature(registrationParams); |
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.
use client.registerCapability
Registration registration = new Registration(Preferences.FORMATTING_RANGE_ID, Preferences.TEXT_DOCUMENT_RANGE_FORMATTING); | ||
registrations.add(registration); | ||
RegistrationParams registrationParams = new RegistrationParams(registrations); | ||
client.registerFeature(registrationParams); |
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.
use client.registerCapability
* @see {@link org.eclipse.lsp4j.services.LanguageClient#unregisterCapability(RegistrationParams)} | ||
*/ | ||
public void unregisterCapability(UnregistrationParams params) { | ||
client.unregisterCapability(params).join(); |
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.
don't join, that makes the server unresponsive, waiting for a response that never comes.
* @see {@link org.eclipse.lsp4j.services.LanguageClient#registerCapability(RegistrationParams)} | ||
*/ | ||
public void registerCapability(RegistrationParams params) { | ||
client.registerCapability(params).join(); |
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.
don't join, that makes the server unresponsive, waiting for a response that never comes.
"pool-1-thread-1" #24 prio=5 os_prio=31 tid=0x00007f9926366800 nid=0xb303 waiting on condition [0x0000700009406000]
java.lang.Thread.State: WAITING (parking)
at sun.misc.Unsafe.park(Native Method)
- parking to wait for <0x000000078120e1f0> (a java.util.concurrent.CompletableFuture$Signaller)
at java.util.concurrent.locks.LockSupport.park(LockSupport.java:175)
at java.util.concurrent.CompletableFuture$Signaller.block(CompletableFuture.java:1693)
at java.util.concurrent.ForkJoinPool.managedBlock(ForkJoinPool.java:3323)
at java.util.concurrent.CompletableFuture.waitingGet(CompletableFuture.java:1729)
at java.util.concurrent.CompletableFuture.join(CompletableFuture.java:1934)
at org.eclipse.jdt.ls.core.internal.JavaClientConnection.registerCapability(JavaClientConnection.java:156)
at org.eclipse.jdt.ls.core.internal.handlers.JDTLanguageServer.didChangeConfiguration(JDTLanguageServer.java:197)
Not sure if this is a bug in LSP4J, i.e. maybe registerCapability
should return a Future. cc @akosyakov @spoenemann
Signed-off-by: Snjezana Peco <snjezana.peco@redhat.com>
Looks good now, thanks @snjeza. I'll wait for the current vscode-java release to complete before merging |
Signed-off-by: Snjezana Peco snjezana.peco@redhat.com
See follow up: redhat-developer/vscode-java#245