Add setting to support method implementation code lens

[hopehadfield]

Fixes #3813

Requires eclipse-jdtls/eclipse.jdt.ls#3333

Applies to abstract and interface methods, and protected by the java.methodImplementationsCodeLens.enabled setting (false by default).

[hopehadfield]

@rgrunber I wasn't sure if there should be some dependency between the new setting and the java.implementationsCodeLens.enabled setting. Right now, they're independent, but I can update it if this doesn't make sense.

[rgrunber]

Options are either :

1. Rename java.implementationsCodeLens.enabled to java.typeImplementationsCodeLens.enabled and keep the other separate.
2. Rename java.implementationsCodeLens.enabled to something like java.implementationsCodeLens whose value is an enum : types, methods, all, none

I'm inclined towards (2). The thing with this, is it'd be ideal to add some code that still respects the old setting and it's values (true, false) if they're discovered, since users may still have them in their configuration. So true would map to types and false to none.

[hopehadfield]

Options are either :

1. Rename java.implementationsCodeLens.enabled to java.typeImplementationsCodeLens.enabled and keep the other separate.
2. Rename java.implementationsCodeLens.enabled to something like java.implementationsCodeLens whose value is an enum : types, methods, all, none

I'm inclined towards (2). The thing with this, is it'd be ideal to add some code that still respects the old setting and it's values (true, false) if they're discovered, since users may still have them in their configuration. So true would map to types and false to none.

Sounds good to me. I'll implement that second option with support for the old setting.

[hopehadfield]

@rgrunber my one concern with this approach is if users are unaware of the old setting in their configuration and intend to use the new one, then updates to the new setting won't be reflected accurately (i.e. if the old setting is true, but the user intends for the setting to be "none", then mapping the old setting to "types" and using this value for the new setting would give undesired behaviour).

Another alternative would be to keep the same name for the setting (java.implementationsCodeLens.enabled). If the user hasn't updated the setting since this change, then we map the boolean to the appropriate enum. Otherwise, it'll be overwritten by the user's update and no further action is required. Obviously not an ideal naming convention for an enum but this feels like the best solution that doesn't involve ignoring the old configuration entirely.

[rgrunber]

The problem with keeping it the same is that .enabled implies a boolean value in our settings. Does it help if you adopt the convention that the existence of the newer settings takes precedence over the older one ? Then you just have to detect whether the user really touched the newer setting or if it's just set to the default. I think we have code that checks that.

Have a look at isPreferenceOverridden and see if that does the trick.

[hopehadfield]

Yes that would definitely work, I didn't know if we could check that the default had been intentionally set or not.

I can't find isPreferenceOverridden in jdt.ls, jdt.ui, or jdt.core... not sure if I'm overlooking it or if it's in a different package?

[rgrunber]

vscode-java/src/utils.ts

Line 14 in 309c705

export function isPreferenceOverridden(section: string): boolean {

[hopehadfield]

Ah, I was looking to do the check on the server-side.

I tried using isPreferenceOverridden but it evaluates as false when the setting is set to the default value (even if it has been set to another value and then set back to default)... not sure what the way around this is. It feels like we would want the new setting value to be respected so long as the user has accessed/viewed the new setting, though I'm not sure this is possible?

[rgrunber]

Ah, I was looking to do the check on the server-side.

I forgot that we might have to do the same on the server side for other clients 😐 . I guess we can look at that later. I feel like we made this decision once before on another PR.

I tried using isPreferenceOverridden but it evaluates as false when the setting is set to the default value (even if it has been set to another value and then set back to default)... not sure what the way around this is. It feels like we would want the new setting value to be respected so long as the user has accessed/viewed the new setting, though I'm not sure this is possible?

This seems more like a quirk of VS Code. When a value is set to match the default through the Settings UI, VS Code removes the setting from the underlying settings.json file as if it were just following the default. Seems like that's a recipe for disaster if the default ever changes, but I'm fine with treating that as "not overridden" given that it's VS Codes fault.

However, I don't think we have to worry about that because when the updated extension is installed, there will be no "boolean" checkbox for java.implementationsCodeLens.enabled. The old setting won't be "visible", and the new (java.implementationsCodeLens) one will have a dropdown. They'll only be able to set the old value through the settings.json file and isPreferenceOverridden will detect that.

Let me know if that makes sense.

[hopehadfield]

Sorry for the delay, got swamped with studying for exams. I updated the setting, though please note that I named it java.implementationCodeLens as opposed to java.implementationsCodeLens because isPreferenceOverridden would evaluate to true for the updated setting (even if it wasn't set) so long as java.implementationsCodeLens.enabled was defined, making the check redundant.

Please let me know if this update works.

[rgrunber]

Overall, look good and mostly ready to merge. Just a minor logic issue.

I've played with this and it seems to work well in supporting the deprecated option.

[rgrunber]

This is probably good enough. Did you see this approach somewhere in the code ? What do you think of :

const deprecatedImplementations = typeof javaConfig.implementationsCodeLens?.enabled === 'boolean';

Then you won't need that undefined check or the risk that javaConfig.implementationsCodeLens.enabled might be something other than a boolean that gets converted to a boolean.

[hopehadfield]

Isn't this expression just evaluating if javaConfig.implementationsCodeLens.enabled is a boolean or not? If enabled is false, this would return true.

What about

if (!isPreferenceOverridden("java.implementationCodeLens") && typeof javaConfig.implementationsCodeLens?.enabled === 'boolean'){
    const deprecatedImplementations = javaConfig.implementationsCodeLens.enabled;
    javaConfig.implementationCodeLens = deprecatedImplementations ? "types" : "none";

[rgrunber]

👍 Yeah I think we agree. I just wasn't specific. You'd just have 2 different values. One for whether the property is a boolean, and one for the actual boolean value. Here's what I had :

diff --git a/src/utils.ts b/src/utils.ts
index 0a06f45..58f8c22 100644
--- a/src/utils.ts
+++ b/src/utils.ts
@@ -273,11 +273,9 @@ export async function getJavaConfig(javaHome: string) {
 		javaConfig.configuration.runtimes = await addAutoDetectedJdks(userConfiguredJREs);
 	}
 
-	if (!isPreferenceOverridden("java.implementationCodeLens") && javaConfig.implementationsCodeLens){
-		const deprecatedImplementations: boolean | undefined = javaConfig.implementationsCodeLens.enabled;
-		if (deprecatedImplementations !== undefined){
-			javaConfig.implementationCodeLens = deprecatedImplementations ? "types" : "none";
-		}
+	const deprecatedImplementations = typeof javaConfig.implementationsCodeLens.enabled === 'boolean';
+	if (!isPreferenceOverridden("java.implementationCodeLens") && deprecatedImplementations){
+		javaConfig.implementationCodeLens = javaConfig.implementationsCodeLens.enabled ? "types" : "none";
 	}
 
 	return javaConfig;

I think either is fine.

[hopehadfield]

Thanks for the advice and review! 😄 @rgrunber