[WIP] Use OrganizeImportsOperation in o.e.jdt.core.manipulation. #566

rgrunber · 2018-02-21T20:17:20Z

With http://eclip.se/506239, OrganizeImportsOperation is now API
ImportReferencesCollector is now API in
org.eclipse.jdt.core.manipulation
non-UI methods of SharedASTProvider are exposed in
org.eclipse.jdt.core.manipulation through CoreASTProvider
Preferences such as ORGIMPORTS_IMPORTORDER can now be defined entirely
through the IEclipsePreferences facility. Simply use
JavaManipulation.setJavaUIPluginId() to let the JDT know which plug-in
node will be used to store preferences, and then default/runtime
preferences for consumption.
StubUtility.createImportRewrite() methods are now public API in
CodeStyleConfiguration

Signed-off-by: Roland Grunberg [email protected]

I figured I'd get the ball rolling on this now that it finally got accepted into JD UI. There are of course still a few items to fix up, and possibly more as I find them :

Move preference related code into the preference manager
AFAICT JDT-LS caches many ASTs but JDT only ever cached the one for the active document. This patch returns to the old behaviour. Is this a bad thing ? Do we really see a huge gain from caching many different ASTs ? The JDT SharedASTProvider javadoc mention that more than one AST should never be held in memory for performance reasons.
- Need replacements for sharedASTProvider's clearASTCreationCount() getASTCreationCount() getCacheSize()
- Easiest solution if these methods are really wanted for testing is to wrap all of CoreASTProvider (can't be subclassed) by some new class and provide those methods there
Wait until Oxygen M6 is released so we can stop depending on the 4.8-I-builds to build this change, and just use the current 4.8milestones in the target platform

eclipse-ls-bot · 2018-02-21T20:17:22Z

Can one of the admins verify this patch?

fbricon · 2018-02-21T21:01:10Z

add to whitelist

fbricon · 2018-02-21T21:04:10Z

Nice one @rgrunber, I didn't see that one coming. I love to see more deleted code than added :-)

fbricon · 2018-02-21T22:07:16Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/preferences/Preferences.java

+			result.append(val);
+			result.append(";"); //$NON-NLS-1$
+		}
+		pref.put(CodeStyleConfiguration.ORGIMPORTS_IMPORTORDER, result.toString());


pref.put(CodeStyleConfiguration.ORGIMPORTS_IMPORTORDER, String.join(";", importOrder));

Fixed in updated version.

fbricon · 2018-02-21T22:17:29Z

I'm surprised the server behaves very well without all the AST caching @aeschli introduced in SharedASTProvider.
@aeschli do you want to comment on @rgrunber remarks about that?

aeschli · 2018-02-22T08:00:11Z

I don't have any numbers if caching multiple AST is worth it, it probably all depends on how the language client uses the server. I'm fine with caching less.

@rgrunber But I wonder if it is a good idea to have a cache in the jdt.core.codemanipulation...

fbricon · 2018-02-22T14:44:10Z

@tsmaeder @yaohaizh @svor @tolusha I strongly advise you to check whether your extensions will be impacted by that change

fbricon · 2018-02-22T20:24:33Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/JavaLanguageServerPlugin.java

@@ -142,6 +143,9 @@ public void start(BundleContext bundleContext) throws BackingStoreException {
 		preferenceManager.addPreferencesChangeListener(preferencesChangeListener);
 		logInfo(getClass() + " is started");
 		configureProxy();
+
+		// Set the ID to use for preference lookups
+		JavaManipulation.setJavaUIPluginId(PLUGIN_ID);


How about you rename that method upstream to something like setPreferenceStoreId or setPreferenceStorePluginId?

This change should be in for Photon M6, https://git.eclipse.org/r/118209/ . New methods will be getPreferenceNodeId/setPreferenceNodeId . The setter returns true if the value was successfully set and false otherwise.

fbricon · 2018-02-22T21:20:46Z

Funny thing is, with this change, #552 can not be reproduced \o/

gorkem · 2018-02-23T01:25:46Z

@rgrunber remind me to buy you your favorite beverage next time I am at the office.

maxandersen · 2018-02-23T23:08:34Z

This is awesome @rgrunber!

tolusha · 2018-02-26T08:20:49Z

@fbricon @tsmaeder
There isn't much impact on eclipse/che-ls-jdt
Just refactoring stuff.

ghost · 2018-03-05T15:26:06Z

@rgrunber nice work!

- With http://eclip.se/506239, OrganizeImportsOperation is now API - ImportReferencesCollector is now API in org.eclipse.jdt.core.manipulation - non-UI methods of SharedASTProvider are exposed in org.eclipse.jdt.core.manipulation through CoreASTProvider - Preferences such as ORGIMPORTS_IMPORTORDER can now be defined entirely through the IEclipsePreferences facility. Simply use JavaManipulation.setPreferenceNodeId() to let the JDT know which plug-in node will be used to store preferences, and then default/runtime preferences for consumption. - StubUtility.createImportRewrite() methods are now public API in CodeStyleConfiguration - Copy over ASTNodes.getEnclosingDeclaration() and Bindings.findNullnessDefault to avoid growing the patch for now. Signed-off-by: Roland Grunberg <[email protected]>

fbricon · 2018-03-15T13:16:42Z

We'll merge this after the 0.15.0 release

fbricon · 2018-03-15T20:08:39Z

Merged as e0bb56a

rgrunber · 2018-03-15T20:28:18Z

@fbricon Thanks for taking this. I was going to push some additional changes, mainly polishing up the target platform now that 4.8milestones can be used but I see you've done that already.

fbricon reviewed Feb 21, 2018

View reviewed changes

fbricon added the debt label Feb 21, 2018

rgrunber force-pushed the organize-imports branch from 9747fd6 to 450401c Compare February 22, 2018 19:24

fbricon reviewed Feb 22, 2018

View reviewed changes

fbricon added this to the Mid March 2018 milestone Mar 1, 2018

rgrunber force-pushed the organize-imports branch from 450401c to b7d74f0 Compare March 6, 2018 19:47

fbricon modified the milestones: Mid March 2018, End March 2018 Mar 15, 2018

fbricon closed this Mar 15, 2018

fbricon modified the milestones: End March 2018, Mid March 2018 Mar 15, 2018

rgrunber deleted the organize-imports branch March 15, 2018 20:20

rgrunber mentioned this pull request Mar 19, 2018

Broken import autocompletion #591

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Use OrganizeImportsOperation in o.e.jdt.core.manipulation. #566

[WIP] Use OrganizeImportsOperation in o.e.jdt.core.manipulation. #566

rgrunber commented Feb 21, 2018 •

edited

Loading

eclipse-ls-bot commented Feb 21, 2018

fbricon commented Feb 21, 2018

fbricon commented Feb 21, 2018

fbricon Feb 21, 2018

rgrunber Feb 22, 2018

fbricon commented Feb 21, 2018

aeschli commented Feb 22, 2018 •

edited

Loading

fbricon commented Feb 22, 2018

fbricon Feb 22, 2018

rgrunber Mar 5, 2018

fbricon commented Feb 22, 2018

gorkem commented Feb 23, 2018

maxandersen commented Feb 23, 2018

tolusha commented Feb 26, 2018

ghost commented Mar 5, 2018

fbricon commented Mar 15, 2018

fbricon commented Mar 15, 2018

rgrunber commented Mar 15, 2018 •

edited

Loading

[WIP] Use OrganizeImportsOperation in o.e.jdt.core.manipulation. #566

[WIP] Use OrganizeImportsOperation in o.e.jdt.core.manipulation. #566

Conversation

rgrunber commented Feb 21, 2018 • edited Loading

eclipse-ls-bot commented Feb 21, 2018

fbricon commented Feb 21, 2018

fbricon commented Feb 21, 2018

fbricon Feb 21, 2018

Choose a reason for hiding this comment

rgrunber Feb 22, 2018

Choose a reason for hiding this comment

fbricon commented Feb 21, 2018

aeschli commented Feb 22, 2018 • edited Loading

fbricon commented Feb 22, 2018

fbricon Feb 22, 2018

Choose a reason for hiding this comment

rgrunber Mar 5, 2018

Choose a reason for hiding this comment

fbricon commented Feb 22, 2018

gorkem commented Feb 23, 2018

maxandersen commented Feb 23, 2018

tolusha commented Feb 26, 2018

ghost commented Mar 5, 2018

fbricon commented Mar 15, 2018

fbricon commented Mar 15, 2018

rgrunber commented Mar 15, 2018 • edited Loading

rgrunber commented Feb 21, 2018 •

edited

Loading

aeschli commented Feb 22, 2018 •

edited

Loading

rgrunber commented Mar 15, 2018 •

edited

Loading