From 331e3ce76e29129d9853bcf28e5de5f80e494053 Mon Sep 17 00:00:00 2001 From: Sheng Chen Date: Sat, 6 May 2023 15:28:11 +0800 Subject: [PATCH] Only append data on completion item selected - Instead of adding data to the completion items directly, now we only append those data when 'onDidSelect' happens. Because those data are only used by the registered ranking providers. Both server and client do not care about it actually. Signed-off-by: Sheng Chen --- .../core/contentassist/CompletionRanking.java | 5 +- .../CompletionProposalRequestor.java | 9 ++-- .../internal/handlers/CompletionHandler.java | 5 ++ .../handlers/CompletionResolveHandler.java | 2 - .../internal/handlers/CompletionResponse.java | 17 +++++++ .../CompletionRankingProviderTest.java | 48 ++++++++++++------- 6 files changed, 63 insertions(+), 23 deletions(-) diff --git a/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/contentassist/CompletionRanking.java b/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/contentassist/CompletionRanking.java index 74044456b4..a9bfff6098 100644 --- a/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/contentassist/CompletionRanking.java +++ b/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/contentassist/CompletionRanking.java @@ -105,7 +105,10 @@ public Map getData() { /** * A map data structure that will be appended to completion item's data field. When a completion * item is selected, the selected item will be passed to the provider. Providers can use the stored - * data to do post process on demand. + * data to do post process on demand. Please note that the data will only be appended when item + * selected event happens. The data will not exist during textDocument/completion and completionItem/resolve + * phases. + * *

* The key "COMPLETION_EXECUTION_TIME" is preserved to store the time calculating all the * completion items at the server side in millisecond. diff --git a/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/contentassist/CompletionProposalRequestor.java b/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/contentassist/CompletionProposalRequestor.java index aafe7269de..769e361169 100644 --- a/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/contentassist/CompletionProposalRequestor.java +++ b/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/contentassist/CompletionProposalRequestor.java @@ -16,6 +16,7 @@ import java.util.Arrays; import java.util.Comparator; import java.util.HashMap; +import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Set; @@ -244,6 +245,8 @@ public List getCompletionItems(IProgressMonitor monitor) { if (!proposals.isEmpty()){ initializeCompletionListItemDefaults(proposals.get(0)); } + + List> contributedData = new LinkedList<>(); //Let's compute replacement texts for the most relevant results only for (int i = 0; i < limit; i++) { CompletionProposal proposal = proposals.get(i); @@ -260,11 +263,8 @@ public List getCompletionItems(IProgressMonitor monitor) { item.setFilterText(item.getInsertText()); } } - Map itemData = (Map) item.getData(); Map rankingData = rankingResult.getData(); - for (String key : rankingData.keySet()) { - itemData.put(key, rankingData.get(key)); - } + contributedData.add(rankingData); } completionItems.add(item); } catch (Exception e) { @@ -281,6 +281,7 @@ public List getCompletionItems(IProgressMonitor monitor) { } response.setItems(completionItems); response.setCommonData(CompletionResolveHandler.DATA_FIELD_URI, uri); + response.setCompletionItemData(contributedData); CompletionResponses.store(response); return completionItems; diff --git a/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CompletionHandler.java b/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CompletionHandler.java index 4ecdb49401..6c64d44a42 100644 --- a/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CompletionHandler.java +++ b/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CompletionHandler.java @@ -178,6 +178,11 @@ public void onDidCompletionItemSelect(String requestId, String proposalId) throw ((Map)item.getData()).put(CompletionRanking.COMPLETION_EXECUTION_TIME, executionTime); } + Map contributedData = completionResponse.getCompletionItemData(pId); + if (contributedData != null) { + ((Map)item.getData()).putAll(contributedData); + } + List providers = ((CompletionContributionService) JavaLanguageServerPlugin.getCompletionContributionService()).getRankingProviders(); for (ICompletionRankingProvider provider : providers) { diff --git a/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CompletionResolveHandler.java b/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CompletionResolveHandler.java index 371feea515..5158185889 100644 --- a/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CompletionResolveHandler.java +++ b/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CompletionResolveHandler.java @@ -93,8 +93,6 @@ public CompletionResolveHandler(PreferenceManager manager) { public static final String DATA_FIELD_NAME = "name"; public static final String DATA_FIELD_REQUEST_ID = "rid"; public static final String DATA_FIELD_PROPOSAL_ID = "pid"; - public static final String DATA_FIELD_CONSTANT_VALUE = "constant_value"; - public static final String DATA_METHOD_DEFAULT_VALUE = "default_value"; public CompletionItem resolve(CompletionItem param, IProgressMonitor monitor) { diff --git a/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CompletionResponse.java b/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CompletionResponse.java index 2945fe7e81..e0ddcd15d7 100644 --- a/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CompletionResponse.java +++ b/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CompletionResponse.java @@ -12,6 +12,7 @@ *******************************************************************************/ package org.eclipse.jdt.ls.core.internal.handlers; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -38,6 +39,11 @@ public class CompletionResponse { private Map commonData = new HashMap<>(); private List proposals; private List items; + /** + * Stores the data that are specific to each completion item. + * Those data are contributed by the ranking providers. + */ + private List> completionItemData; public CompletionResponse() { id = idSeed.getAndIncrement(); @@ -109,4 +115,15 @@ public List getItems() { public void setItems(List items) { this.items = items; } + + public Map getCompletionItemData(int index) { + if (completionItemData == null || index >= completionItemData.size()) { + return Collections.emptyMap(); + } + return completionItemData.get(index); + } + + public void setCompletionItemData(List> completionItemData) { + this.completionItemData = completionItemData; + } } diff --git a/org.eclipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/handlers/CompletionRankingProviderTest.java b/org.eclipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/handlers/CompletionRankingProviderTest.java index 6858cce411..81fbdd632b 100644 --- a/org.eclipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/handlers/CompletionRankingProviderTest.java +++ b/org.eclipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/handlers/CompletionRankingProviderTest.java @@ -16,8 +16,13 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.reset; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; -import java.util.Arrays; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -36,7 +41,12 @@ import org.junit.After; import org.junit.Before; import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnitRunner; +@RunWith(MockitoJUnitRunner.class) public class CompletionRankingProviderTest extends AbstractCompilationUnitBasedTest { private static String COMPLETION_TEMPLATE = @@ -55,18 +65,20 @@ public class CompletionRankingProviderTest extends AbstractCompilationUnitBasedT " \"jsonrpc\": \"2.0\"\n" + "}"; + @Mock private TestRankingProvider provider; @Before public void setUp() { - provider = new TestRankingProvider(); + when(provider.rank(any(), any(), any(), any())).thenCallRealMethod(); + doNothing().when(provider).onDidCompletionItemSelect(any()); JavaLanguageServerPlugin.getCompletionContributionService().registerRankingProvider(provider); } @After public void tearDown() { JavaLanguageServerPlugin.getCompletionContributionService().unregisterRankingProvider(provider); - provider = null; + reset(provider); } @Test @@ -85,27 +97,32 @@ public void testRank() throws Exception { CompletionItem recommended = list.getItems().get(0); assertTrue(recommended.getLabel().startsWith("★")); - assertTrue(((Map)recommended.getData()).containsKey("foo")); assertEquals(recommended.getFilterText(), recommended.getInsertText()); } @Test - public void testOnDidCompletionItemSelect() throws Exception { + public void testOnDidCompletionItemSelect() throws Exception {ICompilationUnit unit = getWorkingCopy( + "src/java/Foo.java", + "public class Foo {\n"+ + " void foo() {\n"+ + " Integer.\n" + + " }\n"+ + "}\n"); + + requestCompletions(unit, "Integer."); CompletionHandler handler = new CompletionHandler(JavaLanguageServerPlugin.getPreferencesManager()); - CompletionResponse response = new CompletionResponse(); - CompletionItem completionItem = new CompletionItem(); - completionItem.setData(new HashMap<>()); - response.setItems(Arrays.asList(completionItem)); - CompletionResponses.store(response); - handler.onDidCompletionItemSelect(String.valueOf(response.getId()), "0"); - - assertTrue(provider.onDidCompletionItemSelectInvoked); + + ArgumentCaptor argument = ArgumentCaptor.forClass(CompletionItem.class); + handler.onDidCompletionItemSelect(String.valueOf((new CompletionResponse()).getId() - 1), "0"); + + verify(provider, times(1)).onDidCompletionItemSelect(argument.capture()); + Map data = (Map) argument.getValue().getData(); + assertEquals("bar", data.get("foo")); + assertTrue(data.containsKey(CompletionRanking.COMPLETION_EXECUTION_TIME)); } class TestRankingProvider implements ICompletionRankingProvider { - boolean onDidCompletionItemSelectInvoked = false; - @Override public CompletionRanking[] rank(List proposals, org.eclipse.jdt.core.CompletionContext context, ICompilationUnit unit, IProgressMonitor monitor) { CompletionRanking[] rankings = new CompletionRanking[proposals.size()]; @@ -120,7 +137,6 @@ public CompletionRanking[] rank(List proposals, org.eclipse. @Override public void onDidCompletionItemSelect(CompletionItem item) { - onDidCompletionItemSelectInvoked = true; } }