You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@netbeans.apache.org by "Achal1607 (via GitHub)" <gi...@apache.org> on 2025/05/21 10:06:10 UTC
[PR] Refactored configuration management for LSP module [netbeans]
Achal1607 opened a new pull request, #8514:
URL: https://github.com/apache/netbeans/pull/8514
Currently, configuration requests require a round trip every time the language server needs a value from the client. For example, in `TextDocumentServiceImpl`, the `completion` method, which provides auto-complete suggestions, performs a round trip to fetch configuration values like `netbeans.javadoc.load.timeout` and `netbeans.completion.warning.time`. Additionally, listening to changes in specific configurations is difficult, as it requires registration in `WorkspaceServiceImpl` and it is not extensible.
This PR aims to address these challenges by introducing a new class, `ClientConfigurationManager`, which manages clients along with their configurations. It serves configuration values from a local cache if available, and only performs a request to the client when necessary. It also listens for configuration changes to keep the cache in sync.
There is also a new class `ConfigValueCache` which takes care of all the business logic of traversing the tree and maintaining configs according to the scopes as well.
Moreover, it allows listeners in the form of `BiConsumer` to be attached to specific configuration keys, enabling appropriate actions to be taken automatically when those configurations change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Refactored configuration management for LSP module [netbeans]
Posted by "lahodaj (via GitHub)" <gi...@apache.org>.
lahodaj commented on PR #8514:
URL: https://github.com/apache/netbeans/pull/8514#issuecomment-3017944564
Seems sensible to me. Please note that there was a test in `java.lsp.server` failing - I've restarted the test, so we'll see.
@sdedic - please let us know if you have any comments. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Refactored configuration management for LSP module [netbeans]
Posted by "Achal1607 (via GitHub)" <gi...@apache.org>.
Achal1607 commented on code in PR #8514:
URL: https://github.com/apache/netbeans/pull/8514#discussion_r2149338563
##########
java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/ConfigValueCache.java:
##########
@@ -0,0 +1,296 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.netbeans.modules.java.lsp.server.protocol;
+
+import com.google.gson.JsonElement;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.BiConsumer;
+import org.eclipse.lsp4j.ConfigurationItem;
+import org.eclipse.lsp4j.ConfigurationParams;
+
+/**
+ * Cache for configuration values with support for hierarchical keys and change
+ * notifications
+ *
+ * @author atalati
+ */
+public class ConfigValueCache {
+
+ private final ConcurrentHashMap<String, Object> rootCache = new ConcurrentHashMap<>();
+ private static final String ROOT_KEY_VALUE = "";
+
+ public void registerListener(String section, BiConsumer<String, JsonElement> listener) {
+ ConfigData configData = getCachedConfigData(section);
+ if (configData != null) {
+ configData.setConsumer(listener);
+ } else {
+ setConfigInTree(section, new ConfigData(listener));
+ }
+ }
+
+ public void registerCache(String section) {
+ ConfigData configData = getCachedConfigData(section);
+ if (configData == null) {
+ setConfigInTree(section, new ConfigData());
+ }
+ }
+
+ public JsonElement getConfigValue(String section, String scope) {
+ ConfigData configData = getCachedConfigData(section);
+ if (configData == null) {
+ return null;
+ } else if (scope == null) {
+ return configData.getRootValue();
+ }
+ return configData.getScopedValue(scope);
+ }
+
+ public void updateCache(NbCodeLanguageClient client, String section, Object cacheValue, JsonElement tree) {
+ if (tree == null || cacheValue == null) {
+ return;
+ }
+
+ ConfigData rootData;
+ ConfigValueCache currentCache = null;
+
+ if (isConfigDataInstance(cacheValue)) {
+ rootData = (ConfigData) cacheValue;
+ } else if (isConfigValueInstance(cacheValue)) {
+ currentCache = (ConfigValueCache) cacheValue;
+ rootData = currentCache.getRootCacheValue();
+ } else {
+ return;
+ }
+
+ if (rootData != null) {
+ rootData.setRootValue(tree);
+ try {
+ BiConsumer<String, JsonElement> consumer = rootData.getConsumer();
+ if (consumer != null) {
+ consumer.accept(section, tree);
+ }
+ } catch (RuntimeException e) {
Review Comment:
Sorry, I missed this change, will push this as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Refactored configuration management for LSP module [netbeans]
Posted by "lahodaj (via GitHub)" <gi...@apache.org>.
lahodaj commented on code in PR #8514:
URL: https://github.com/apache/netbeans/pull/8514#discussion_r2141864260
##########
java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/ConfigValueCache.java:
##########
@@ -0,0 +1,296 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.netbeans.modules.java.lsp.server.protocol;
+
+import com.google.gson.JsonElement;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.BiConsumer;
+import org.eclipse.lsp4j.ConfigurationItem;
+import org.eclipse.lsp4j.ConfigurationParams;
+
+/**
+ * Cache for configuration values with support for hierarchical keys and change
+ * notifications
+ *
+ * @author atalati
+ */
+public class ConfigValueCache {
+
+ private final ConcurrentHashMap<String, Object> rootCache = new ConcurrentHashMap<>();
+ private static final String ROOT_KEY_VALUE = "";
+
+ public void registerListener(String section, BiConsumer<String, JsonElement> listener) {
+ ConfigData configData = getCachedConfigData(section);
+ if (configData != null) {
+ configData.setConsumer(listener);
+ } else {
+ setConfigInTree(section, new ConfigData(listener));
+ }
+ }
+
+ public void registerCache(String section) {
+ ConfigData configData = getCachedConfigData(section);
+ if (configData == null) {
+ setConfigInTree(section, new ConfigData());
+ }
+ }
+
+ public JsonElement getConfigValue(String section, String scope) {
+ ConfigData configData = getCachedConfigData(section);
+ if (configData == null) {
+ return null;
+ } else if (scope == null) {
+ return configData.getRootValue();
+ }
+ return configData.getScopedValue(scope);
+ }
+
+ public void updateCache(NbCodeLanguageClient client, String section, Object cacheValue, JsonElement tree) {
+ if (tree == null || cacheValue == null) {
+ return;
+ }
+
+ ConfigData rootData;
+ ConfigValueCache currentCache = null;
+
+ if (isConfigDataInstance(cacheValue)) {
+ rootData = (ConfigData) cacheValue;
+ } else if (isConfigValueInstance(cacheValue)) {
+ currentCache = (ConfigValueCache) cacheValue;
+ rootData = currentCache.getRootCacheValue();
+ } else {
+ return;
+ }
+
+ if (rootData != null) {
+ rootData.setRootValue(tree);
+ try {
+ BiConsumer<String, JsonElement> consumer = rootData.getConsumer();
+ if (consumer != null) {
+ consumer.accept(section, tree);
+ }
+ } catch (RuntimeException e) {
Review Comment:
Any update on this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Refactored configuration management for LSP module [netbeans]
Posted by "Achal1607 (via GitHub)" <gi...@apache.org>.
Achal1607 commented on PR #8514:
URL: https://github.com/apache/netbeans/pull/8514#issuecomment-3497439151
Since it is a very old PR, I don't rember exact details, but will try to modify the commit by reading through the history of this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Refactored configuration management for LSP module [netbeans]
Posted by "lahodaj (via GitHub)" <gi...@apache.org>.
lahodaj commented on code in PR #8514:
URL: https://github.com/apache/netbeans/pull/8514#discussion_r2150180742
##########
java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/ClientConfigurationManager.java:
##########
@@ -0,0 +1,204 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.netbeans.modules.java.lsp.server.protocol;
+
+import com.google.gson.JsonElement;
+import com.google.gson.JsonObject;
+import java.net.MalformedURLException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.WeakHashMap;
+import java.util.concurrent.CompletableFuture;
+import java.util.function.BiConsumer;
+import org.eclipse.lsp4j.ConfigurationItem;
+import org.eclipse.lsp4j.ConfigurationParams;
+import org.eclipse.lsp4j.services.LanguageClient;
+import org.netbeans.api.project.FileOwnerQuery;
+import org.netbeans.api.project.Project;
+import org.netbeans.modules.java.lsp.server.Utils;
+import org.openide.filesystems.FileObject;
+import org.openide.filesystems.FileUtil;
+import org.openide.util.Lookup;
+
+/**
+ * Manages configuration settings for LSP clients
+ *
+ * @author atalati
+ */
+public class ClientConfigurationManager {
+
+ final WeakHashMap<LanguageClient, ConfigValueCache> clientCaches = new WeakHashMap<>();
+
+ private ClientConfigurationManager() {
+ }
+
+ public static ClientConfigurationManager getInstance() {
+ return SingletonHolder.INSTANCE;
+ }
+
+ private static class SingletonHolder {
+
+ private static final ClientConfigurationManager INSTANCE = new ClientConfigurationManager();
+ }
+
+ public void registerClient(LanguageClient client) {
+ clientCaches.put(client, new ConfigValueCache());
+ }
+
+ public void registerConfigChangeListener(NbCodeLanguageClient client, String section, BiConsumer<String, JsonElement> consumer) {
+ ConfigValueCache cache = clientCaches.get(client);
+ if (cache != null) {
+ cache.registerListener(section, consumer);
+ }
+ }
+
+ public void registerConfigCache(NbCodeLanguageClient client, String section) {
Review Comment:
Yes, that sounds reasonable. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Refactored configuration management for LSP module [netbeans]
Posted by "lahodaj (via GitHub)" <gi...@apache.org>.
lahodaj commented on PR #8514:
URL: https://github.com/apache/netbeans/pull/8514#issuecomment-3497906830
@Achal1607 , I think what @mbien means is that it would be cleaner if the commit just said "Refactored configuration management for LSP module", and didn't include these details:
```
addressed review comments
addressed some review comments
addressed review comments
fixed unit tests
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Refactored configuration management for LSP module [netbeans]
Posted by "Achal1607 (via GitHub)" <gi...@apache.org>.
Achal1607 commented on code in PR #8514:
URL: https://github.com/apache/netbeans/pull/8514#discussion_r2149340004
##########
java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/ClientConfigurationManager.java:
##########
@@ -0,0 +1,204 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.netbeans.modules.java.lsp.server.protocol;
+
+import com.google.gson.JsonElement;
+import com.google.gson.JsonObject;
+import java.net.MalformedURLException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.WeakHashMap;
+import java.util.concurrent.CompletableFuture;
+import java.util.function.BiConsumer;
+import org.eclipse.lsp4j.ConfigurationItem;
+import org.eclipse.lsp4j.ConfigurationParams;
+import org.eclipse.lsp4j.services.LanguageClient;
+import org.netbeans.api.project.FileOwnerQuery;
+import org.netbeans.api.project.Project;
+import org.netbeans.modules.java.lsp.server.Utils;
+import org.openide.filesystems.FileObject;
+import org.openide.filesystems.FileUtil;
+import org.openide.util.Lookup;
+
+/**
+ * Manages configuration settings for LSP clients
+ *
+ * @author atalati
+ */
+public class ClientConfigurationManager {
+
+ final WeakHashMap<LanguageClient, ConfigValueCache> clientCaches = new WeakHashMap<>();
+
+ private ClientConfigurationManager() {
+ }
+
+ public static ClientConfigurationManager getInstance() {
+ return SingletonHolder.INSTANCE;
+ }
+
+ private static class SingletonHolder {
+
+ private static final ClientConfigurationManager INSTANCE = new ClientConfigurationManager();
+ }
+
+ public void registerClient(LanguageClient client) {
+ clientCaches.put(client, new ConfigValueCache());
+ }
+
+ public void registerConfigChangeListener(NbCodeLanguageClient client, String section, BiConsumer<String, JsonElement> consumer) {
+ ConfigValueCache cache = clientCaches.get(client);
+ if (cache != null) {
+ cache.registerListener(section, consumer);
+ }
+ }
+
+ public void registerConfigCache(NbCodeLanguageClient client, String section) {
Review Comment:
Okay, so thinking on this lines, would this approach be fine:
When the caller requests for the config we will cache it by default and also expose a method which accepts `boolean` if the caller doesn't want to cache the value (whatever be the reason be it large object size or something else).
So, then it would be the responsibility of the caller to state explicitly if they don't want to cache the value. Would this approach work?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Refactored configuration management for LSP module [netbeans]
Posted by "Achal1607 (via GitHub)" <gi...@apache.org>.
Achal1607 commented on PR #8514:
URL: https://github.com/apache/netbeans/pull/8514#issuecomment-2952213992
@lahodaj addressed your review comments, please have a look again whenever it is possible. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Refactored configuration management for LSP module [netbeans]
Posted by "lahodaj (via GitHub)" <gi...@apache.org>.
lahodaj commented on code in PR #8514:
URL: https://github.com/apache/netbeans/pull/8514#discussion_r2149121008
##########
java/java.lsp.server/test/unit/src/org/netbeans/modules/java/lsp/server/protocol/ServerTest.java:
##########
@@ -346,7 +346,7 @@ void cancelDiagnostics(AtomicBoolean cancel) {
}
}
- class LspClient implements LanguageClient {
+class LspClient implements LanguageClient {
Review Comment:
Nit: this change seems unnecessary?
##########
java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/ClientConfigurationManager.java:
##########
@@ -0,0 +1,204 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.netbeans.modules.java.lsp.server.protocol;
+
+import com.google.gson.JsonElement;
+import com.google.gson.JsonObject;
+import java.net.MalformedURLException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.WeakHashMap;
+import java.util.concurrent.CompletableFuture;
+import java.util.function.BiConsumer;
+import org.eclipse.lsp4j.ConfigurationItem;
+import org.eclipse.lsp4j.ConfigurationParams;
+import org.eclipse.lsp4j.services.LanguageClient;
+import org.netbeans.api.project.FileOwnerQuery;
+import org.netbeans.api.project.Project;
+import org.netbeans.modules.java.lsp.server.Utils;
+import org.openide.filesystems.FileObject;
+import org.openide.filesystems.FileUtil;
+import org.openide.util.Lookup;
+
+/**
+ * Manages configuration settings for LSP clients
+ *
+ * @author atalati
+ */
+public class ClientConfigurationManager {
+
+ final WeakHashMap<LanguageClient, ConfigValueCache> clientCaches = new WeakHashMap<>();
+
+ private ClientConfigurationManager() {
+ }
+
+ public static ClientConfigurationManager getInstance() {
+ return SingletonHolder.INSTANCE;
+ }
+
+ private static class SingletonHolder {
+
+ private static final ClientConfigurationManager INSTANCE = new ClientConfigurationManager();
+ }
+
+ public void registerClient(LanguageClient client) {
+ clientCaches.put(client, new ConfigValueCache());
+ }
+
+ public void registerConfigChangeListener(NbCodeLanguageClient client, String section, BiConsumer<String, JsonElement> consumer) {
+ ConfigValueCache cache = clientCaches.get(client);
+ if (cache != null) {
+ cache.registerListener(section, consumer);
+ }
+ }
+
+ public void registerConfigCache(NbCodeLanguageClient client, String section) {
Review Comment:
I guess my concern is this: given the registration and the actual config read are on completely different places, it is very easy to imagine that when adding a configuration, only the config read will be added, and not the registration. And given it will all work(?), no one will notice, except it will be slower. And after a little while, no one will be sure anymore if the non-caching was intentional or an oversight.
I wonder if we have a particular setting now or in a foreseeable future that we don't want to cache. And if yes, is there a reason why the read cannot go directly through the API without the cache? (With a clear comment saying something along the lines of "not using the cache, as this setting is too big, and should be fetched anew every time, instead of being cache".)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Refactored configuration management for LSP module [netbeans]
Posted by "Achal1607 (via GitHub)" <gi...@apache.org>.
Achal1607 commented on PR #8514:
URL: https://github.com/apache/netbeans/pull/8514#issuecomment-3500881842
@lahodaj , @mbien pushed again with the updated commit message.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Refactored configuration management for LSP module [netbeans]
Posted by "Achal1607 (via GitHub)" <gi...@apache.org>.
Achal1607 commented on code in PR #8514:
URL: https://github.com/apache/netbeans/pull/8514#discussion_r2122894690
##########
java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/ClientConfigurationManager.java:
##########
@@ -0,0 +1,204 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.netbeans.modules.java.lsp.server.protocol;
+
+import com.google.gson.JsonElement;
+import com.google.gson.JsonObject;
+import java.net.MalformedURLException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.WeakHashMap;
+import java.util.concurrent.CompletableFuture;
+import java.util.function.BiConsumer;
+import org.eclipse.lsp4j.ConfigurationItem;
+import org.eclipse.lsp4j.ConfigurationParams;
+import org.eclipse.lsp4j.services.LanguageClient;
+import org.netbeans.api.project.FileOwnerQuery;
+import org.netbeans.api.project.Project;
+import org.netbeans.modules.java.lsp.server.Utils;
+import org.openide.filesystems.FileObject;
+import org.openide.filesystems.FileUtil;
+import org.openide.util.Lookup;
+
+/**
+ * Manages configuration settings for LSP clients
+ *
+ * @author atalati
+ */
+public class ClientConfigurationManager {
+
+ final WeakHashMap<LanguageClient, ConfigValueCache> clientCaches = new WeakHashMap<>();
+
+ private ClientConfigurationManager() {
+ }
+
+ public static ClientConfigurationManager getInstance() {
+ return SingletonHolder.INSTANCE;
+ }
+
+ private static class SingletonHolder {
+
+ private static final ClientConfigurationManager INSTANCE = new ClientConfigurationManager();
+ }
+
+ public void registerClient(LanguageClient client) {
+ clientCaches.put(client, new ConfigValueCache());
+ }
+
+ public void registerConfigChangeListener(NbCodeLanguageClient client, String section, BiConsumer<String, JsonElement> consumer) {
+ ConfigValueCache cache = clientCaches.get(client);
+ if (cache != null) {
+ cache.registerListener(section, consumer);
+ }
+ }
+
+ public void registerConfigCache(NbCodeLanguageClient client, String section) {
Review Comment:
Thought was that if a config doesn't want to be cached by the implementor then it can be skipped. Like lets say if we have editor preferences in the future as a configuration and it becomes a large object, then we may not want to cache the entire object, so it is left upto the implementor what they want to cache and what not.
This was my point of view but if you see value in caching the object on first query then we can change to that approach.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Refactored configuration management for LSP module [netbeans]
Posted by "lahodaj (via GitHub)" <gi...@apache.org>.
lahodaj commented on code in PR #8514:
URL: https://github.com/apache/netbeans/pull/8514#discussion_r2120241919
##########
java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/Server.java:
##########
@@ -1049,44 +1051,53 @@ private void collectProjectCandidates(FileObject fo, List<FileObject> candidates
private void initializeOptions() {
getWorkspaceProjects().thenAccept(projects -> {
- ConfigurationItem item = new ConfigurationItem();
- // PENDING: what about doing just one roundtrip to the client- we may request multiple ConfiguratonItems in one message ?
- item.setSection(client.getNbCodeCapabilities().getConfigurationPrefix() + NETBEANS_JAVA_HINTS);
- client.configuration(new ConfigurationParams(Collections.singletonList(item))).thenAccept(c -> {
- if (c != null && !c.isEmpty() && c.get(0) instanceof JsonObject) {
- textDocumentService.updateJavaHintPreferences((JsonObject) c.get(0));
- }
- else {
- textDocumentService.hintsSettingsRead = true;
- textDocumentService.reRunDiagnostics();
- }
- });
- item.setSection(client.getNbCodeCapabilities().getConfigurationPrefix() + NETBEANS_PROJECT_JDKHOME);
- client.configuration(new ConfigurationParams(Collections.singletonList(item))).thenAccept(c -> {
- JsonPrimitive newProjectJDKHomePath = null;
+ List<String> defaultConfigs = List.of(NETBEANS_JAVA_HINTS, NETBEANS_PROJECT_JDKHOME);
+ List<String> projectConfigs = List.of(NETBEANS_FORMAT, NETBEANS_JAVA_IMPORTS);
- if (c != null && !c.isEmpty() && c.get(0) instanceof JsonPrimitive) {
- newProjectJDKHomePath = (JsonPrimitive) c.get(0);
- } else {
- }
- textDocumentService.updateProjectJDKHome(newProjectJDKHomePath);
- });
+ AtomicReference<FileObject> projectDirectory = new AtomicReference<>(null);
Review Comment:
The use of `AtomicReference` here is probably unnecessary, correct? Can it be simply `FileObject projectDirectory`? It seems to be assign on one place only.
##########
java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/ClientConfigurationManager.java:
##########
@@ -0,0 +1,204 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.netbeans.modules.java.lsp.server.protocol;
+
+import com.google.gson.JsonElement;
+import com.google.gson.JsonObject;
+import java.net.MalformedURLException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.WeakHashMap;
+import java.util.concurrent.CompletableFuture;
+import java.util.function.BiConsumer;
+import org.eclipse.lsp4j.ConfigurationItem;
+import org.eclipse.lsp4j.ConfigurationParams;
+import org.eclipse.lsp4j.services.LanguageClient;
+import org.netbeans.api.project.FileOwnerQuery;
+import org.netbeans.api.project.Project;
+import org.netbeans.modules.java.lsp.server.Utils;
+import org.openide.filesystems.FileObject;
+import org.openide.filesystems.FileUtil;
+import org.openide.util.Lookup;
+
+/**
+ * Manages configuration settings for LSP clients
+ *
+ * @author atalati
+ */
+public class ClientConfigurationManager {
+
+ final WeakHashMap<LanguageClient, ConfigValueCache> clientCaches = new WeakHashMap<>();
+
+ private ClientConfigurationManager() {
+ }
+
+ public static ClientConfigurationManager getInstance() {
+ return SingletonHolder.INSTANCE;
+ }
+
+ private static class SingletonHolder {
+
+ private static final ClientConfigurationManager INSTANCE = new ClientConfigurationManager();
+ }
+
+ public void registerClient(LanguageClient client) {
+ clientCaches.put(client, new ConfigValueCache());
+ }
+
+ public void registerConfigChangeListener(NbCodeLanguageClient client, String section, BiConsumer<String, JsonElement> consumer) {
+ ConfigValueCache cache = clientCaches.get(client);
+ if (cache != null) {
+ cache.registerListener(section, consumer);
+ }
+ }
+
+ public void registerConfigCache(NbCodeLanguageClient client, String section) {
Review Comment:
I wonder what is the purpose of this registering. Can the `ConfigValueCache` be simply registered on the first query?
##########
java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/ConfigValueCache.java:
##########
@@ -0,0 +1,296 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.netbeans.modules.java.lsp.server.protocol;
+
+import com.google.gson.JsonElement;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.BiConsumer;
+import org.eclipse.lsp4j.ConfigurationItem;
+import org.eclipse.lsp4j.ConfigurationParams;
+
+/**
+ * Cache for configuration values with support for hierarchical keys and change
+ * notifications
+ *
+ * @author atalati
+ */
+public class ConfigValueCache {
+
+ private final ConcurrentHashMap<String, Object> rootCache = new ConcurrentHashMap<>();
+ private static final String ROOT_KEY_VALUE = "";
+
+ public void registerListener(String section, BiConsumer<String, JsonElement> listener) {
+ ConfigData configData = getCachedConfigData(section);
+ if (configData != null) {
+ configData.setConsumer(listener);
+ } else {
+ setConfigInTree(section, new ConfigData(listener));
+ }
+ }
+
+ public void registerCache(String section) {
+ ConfigData configData = getCachedConfigData(section);
+ if (configData == null) {
+ setConfigInTree(section, new ConfigData());
+ }
+ }
+
+ public JsonElement getConfigValue(String section, String scope) {
+ ConfigData configData = getCachedConfigData(section);
+ if (configData == null) {
+ return null;
+ } else if (scope == null) {
+ return configData.getRootValue();
+ }
+ return configData.getScopedValue(scope);
+ }
+
+ public void updateCache(NbCodeLanguageClient client, String section, Object cacheValue, JsonElement tree) {
+ if (tree == null || cacheValue == null) {
+ return;
+ }
+
+ ConfigData rootData;
+ ConfigValueCache currentCache = null;
+
+ if (isConfigDataInstance(cacheValue)) {
+ rootData = (ConfigData) cacheValue;
+ } else if (isConfigValueInstance(cacheValue)) {
+ currentCache = (ConfigValueCache) cacheValue;
+ rootData = currentCache.getRootCacheValue();
+ } else {
+ return;
+ }
+
+ if (rootData != null) {
+ rootData.setRootValue(tree);
+ try {
+ BiConsumer<String, JsonElement> consumer = rootData.getConsumer();
+ if (consumer != null) {
+ consumer.accept(section, tree);
+ }
+ } catch (RuntimeException e) {
Review Comment:
Catching and throwing away an exception like this is usually wrong. If there's something wrong, and the exception happens, we will never find out, things will just appear broken.
In cases there's a valid reason to continue after a callback fails with an exception, the exception should be logged, so that at least from the log we can find out what happened.
In cases we don't see a valid reason why the callback would fail, I would simply not catch the error.
##########
java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/ClientConfigurationManager.java:
##########
@@ -0,0 +1,204 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.netbeans.modules.java.lsp.server.protocol;
+
+import com.google.gson.JsonElement;
+import com.google.gson.JsonObject;
+import java.net.MalformedURLException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.WeakHashMap;
+import java.util.concurrent.CompletableFuture;
+import java.util.function.BiConsumer;
+import org.eclipse.lsp4j.ConfigurationItem;
+import org.eclipse.lsp4j.ConfigurationParams;
+import org.eclipse.lsp4j.services.LanguageClient;
+import org.netbeans.api.project.FileOwnerQuery;
+import org.netbeans.api.project.Project;
+import org.netbeans.modules.java.lsp.server.Utils;
+import org.openide.filesystems.FileObject;
+import org.openide.filesystems.FileUtil;
+import org.openide.util.Lookup;
+
+/**
+ * Manages configuration settings for LSP clients
+ *
+ * @author atalati
+ */
+public class ClientConfigurationManager {
+
+ final WeakHashMap<LanguageClient, ConfigValueCache> clientCaches = new WeakHashMap<>();
Review Comment:
This is not necessarily wrong, but I wonder if `NbCodeLanguageClient` should hold an instance of `ClientConfigurationManagement`. On the call sites it would then be like:
```
client.getConfigurationManagement().registerConfigChangeListener("<section>", consumer);
```
and it seems it could be more clearly non-leaking, and also even easier for to use.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Refactored configuration management for LSP module [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on PR #8514:
URL: https://github.com/apache/netbeans/pull/8514#issuecomment-3497349861
@Achal1607 just a nitpick for future PRs: it is better to describe in the commit msg what the commit changed or why the changes were made. When looking through history or running a git bisect "addressed review comments" is no longer helpful at this point.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Refactored configuration management for LSP module [netbeans]
Posted by "lahodaj (via GitHub)" <gi...@apache.org>.
lahodaj merged PR #8514:
URL: https://github.com/apache/netbeans/pull/8514
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Refactored configuration management for LSP module [netbeans]
Posted by "Achal1607 (via GitHub)" <gi...@apache.org>.
Achal1607 commented on PR #8514:
URL: https://github.com/apache/netbeans/pull/8514#issuecomment-3497369276
> @Achal1607 just a nitpick for future PRs: it is better to describe in the commit msg what the commit changed or why the changes were made. When looking through history or running a git bisect "addressed review comments" is no longer helpful at this point.
Thank you for the feedback! I’ll make sure to include more descriptive and meaningful commit messages in future PRs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Refactored configuration management for LSP module [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on PR #8514:
URL: https://github.com/apache/netbeans/pull/8514#issuecomment-3497397359
@Achal1607 you can still change it if you want ;)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists