-
Notifications
You must be signed in to change notification settings - Fork 845
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
Java completion enhanced to support no insertion of method parameters #7747
base: master
Are you sure you want to change the base?
Java completion enhanced to support no insertion of method parameters #7747
Conversation
This is based on end-user feedback received in oracle/javavscode#197 1. Added a user preference for insertion of method parameters in code completions. - `"completion-insert-text-parameters"` - default value = `true` - Also available via: - ide/editor.lib2: `EditorPreferencesKeys.COMPLETION_INSERT_TEXT_PARAMETERS` - java/java.completion: `Utilities.isCompletionInsertTextParameters()` 2. Added disabling of the above preference from the LSP client via the client config key `PREFIX + "java.completion.disable.insertMethodParameters"` - Updation occurs in `TextDocumentServiceImpl.completion()` 3. Added an extra arg `insertTextParams` in the following interface methods with the default value `true`: - `JavaCompletionTask.ItemFactory`: - `createExecutableItem` - `createThisOrSuperConstructorItem` - `JavaCompletionTask.TypeCastableItemFactory`: - `createTypeCastableExecutableItem` 4. Invoked the above methods in `JavaCompletionTask` with this parameter value set by `Utilities.isCompletionInsertTextParameters()`. 5. Enhanced the implementations of the above methods in `JavaCompletionCollector` and `JavaCompletionItem` to insert text for method arguments when `insertTextParams` (or `!memberRef`). - The cursor would be placed inside the parentheses if it is not a no-args method completion. 6. Added a follow-up LSP command to the completion item "editor.action.triggerParameterHints" for showing the signature help when not inserting text parameters. - This is a necessary guide for the user. - It would not be triggered without this addition, since the trigger character '(' is not typed by the user. 7. Unrelated minor fixes in `JavaCompletionCollector.ItemFactoryImpl`: - Added the default prefix ("nbls") to the usages of the follow-up command "java.complete.abstract.methods". - Without this, the command would not get launched by the client. - Added to the builder in `createExecutableItem()`, the instantiated follow-up command "nbls.java.complete.abstract.methods". Signed-off-by: Siddharth Srinivasan <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went through the patch - overall seems sensible. Added a few comments here and there. But, I think @dbalek should review - he might have some different views.
My probably biggest complaint would be that there are not tests checking the new behavior.
java/java.completion/src/org/netbeans/modules/java/completion/JavaCompletionTask.java
Outdated
Show resolved
Hide resolved
java/java.editor/src/org/netbeans/modules/editor/java/JavaCompletionCollector.java
Outdated
Show resolved
Hide resolved
java/java.editor/src/org/netbeans/modules/editor/java/JavaCompletionCollector.java
Outdated
Show resolved
Hide resolved
java/java.editor/src/org/netbeans/modules/editor/java/JavaCompletionCollector.java
Outdated
Show resolved
Hide resolved
java/java.editor/src/org/netbeans/modules/editor/java/JavaCompletionCollector.java
Outdated
Show resolved
Hide resolved
@@ -295,7 +295,7 @@ public final class SimpleValueNames { | |||
public static final String COMPLETION_AUTO_POPUP = "completion-auto-popup"; // NOI18N | |||
|
|||
/** | |||
* Whether the code completion query search will be case sensitive | |||
* Whether the code completion query search will be case-sensitive | |||
* Values: java.lang.Boolean | |||
*/ | |||
public static final String COMPLETION_CASE_SENSITIVE = "completion-case-sensitive"; // NOI18N |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might make sense to put the option name to the API, as is done for other options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't quite understand this suggestion. I refrained from adding to SimpleValueNames
because the comment in EditorPreferenceKeys
mentioned that the latter is meant for private keys, while the former for public, devel or friend keys.
I can move the key here if I misunderstood. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed the unrelated grammatical change in this file. Thanks.
Thanks a lot @lahodaj. |
Is there an IDE options UI for this?! I'm curious at the reasoning for adding this where it is? It seems similar to the guess filled method arguments option? Between them they feel like they cover 3 states? (no fill, parameter names, guess values) |
Thanks @neilcsmith-net. I will try to add this.
Yes, that progression in the complexity for insertions makes sense to me.
Given that the latter is already a defined preference key, it seems easier to maintain backward compatibility and a minimal state increase with the added boolean flag. When implementing the UI above, I could try to introduce joint states for these 2. However, it may become difficult to move the first setting to other languages. Let me know if this analysis makes sense. |
Incorporated review feedback about hoisting to existing if conditions; and adding few descriptive comments. Signed-off-by: Siddharth Srinivasan <[email protected]>
Sure, I agree with keeping as two boolean flags. The UI could handle valid combinations / or just visually disable one checkbox if desired. I was just curious as to the different reasons / pros and cons of where the keys are registered. Obviously the guess arguments key is in the options panel UI code itself - https://github.com/apache/netbeans/blob/master/java/java.editor/src/org/netbeans/modules/java/editor/options/CodeCompletionPanel.java#L52 Let's let @dbalek review that. |
… parameters 1. Added the config key "jdk.java.completion.disable.insertMethodParameters" to disable insertion of method arguments. 2. Added an *"in-review"* version of the NB patch PR apache/netbeans#7747 which implements this enhancement in the NBLS. Signed-off-by: Siddharth Srinivasan <[email protected]>
This configurable enhancement is based on end-user feedback received in oracle/javavscode#197
Added a user preference for insertion of method parameters in code completions.
"completion-insert-text-parameters"
true
EditorPreferencesKeys.COMPLETION_INSERT_TEXT_PARAMETERS
Utilities.isCompletionInsertTextParameters()
Added disabling of the above preference from the LSP client via the client config key
PREFIX + "java.completion.disable.insertMethodParameters"
TextDocumentServiceImpl.completion()
Added an extra arg
insertTextParams
in the following interface methods with the default valuetrue
:JavaCompletionTask.ItemFactory
:createExecutableItem
createThisOrSuperConstructorItem
JavaCompletionTask.TypeCastableItemFactory
:createTypeCastableExecutableItem
Invoked the above methods in
JavaCompletionTask
with this parameter value set byUtilities.isCompletionInsertTextParameters()
.Enhanced the implementations of the above methods in
JavaCompletionCollector
andJavaCompletionItem
to insert text for method arguments wheninsertTextParams
(or!memberRef
).Added a follow-up LSP command to the completion item "editor.action.triggerParameterHints" for showing the signature help when not inserting text parameters.
Unrelated minor fixes in
JavaCompletionCollector.ItemFactoryImpl
:createExecutableItem()
, the instantiated follow-up command "nbls.java.complete.abstract.methods".