Skip to content
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

Add setting to support method implementation code lens #3876

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hopehadfield
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Member

rgrunber commented Nov 27, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
Copy link
Member

rgrunber commented Nov 27, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

implementationsCodeLens support for method implementations
2 participants