Smart semicolon autocorrect by snjeza · Pull Request #3159 · redhat-developer/vscode-java

snjeza · 2023-06-19T13:36:29Z

Fixes #703
Requires eclipse-jdtls/eclipse.jdt.ls#2710

rgrunber · 2023-06-19T18:12:41Z

A few things :

I noticed that for a larger project, there is a point where one can edit a source file, while the language server is still loading. Maybe we need only register the capability under ServiceReady (like we do for paste delegate command) ? What ends up happening is the delegate commands are sent but do not produce a response. If a user types ; during this time, the character is completely ignored for a few seconds.

[Info  - 14:02:19] Jun. 19, 2023, 2:02:18 p.m. >> document/foldingRange
[Trace - 14:02:19] Sending request 'workspace/executeCommand - (29)'.
Params: {
    "command": "java.action.smartSemicolonDetection",
    "arguments": [
        "{\"uri\":\"file:///home/rgrunber/git/lemminx/org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/XMLLanguageServer.java\",\"position\":{\"line\":328,\"character\":24}}"
    ]
}
...
...
(~2s later)
...
...
[Trace - 14:02:21] Received response 'workspace/executeCommand - (29)' in 2122ms.
Result: {
    "uri": "file:///home/rgrunber/git/lemminx/org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/XMLLanguageServer.java",
    "position": {
        "line": 328,
        "character": 51
    }
}

Some other odd cases. Can we handle any of these ?

snjeza · 2023-06-20T20:43:45Z

I noticed that for a larger project, there is a point where one can edit a source file, while the language server is still loading. Maybe we need only register the capability under ServiceReady (like we do for paste delegate command) ? What ends up happening is the delegate commands are sent but do not produce a response. If a user types ; during this time, the character is completely ignored for a few seconds.

Fixed

snjeza · 2023-06-20T20:44:58Z

Some other odd cases. Can we handle any of these ?

It is related to eclipse-jdtls/eclipse.jdt.ls#2710. Checking...

hopehadfield · 2023-06-22T21:10:46Z

I was trying it out and noticed this case where the semicolon doesn't autocorrect. Is this something we can handle?

snjeza · 2023-06-22T23:10:07Z

I was trying it out and noticed this case where the semicolon doesn't autocorrect. Is this something we can handle?

~~I will try.~~
It has been fixed.

snjeza · 2023-06-23T13:55:01Z

Some other odd cases. Can we handle any of these ?

@rgrunber Could you, please, check it again.

I have also implemented the backspace char the same way as in Eclipse.

snjeza · 2023-06-23T14:00:02Z

The remaining issues:

snjeza · 2023-06-23T19:24:43Z

The remaining issues:

It has been fixed.
I have copied FastJavaPartitionScanner and dependent classes -eclipse-jdtls/eclipse.jdt.ls@6202173
@fbricon @rgrunber We could move them from o.e.jdt.ui/o.e.jface.text to o.e.jdt.core.manipulation/o.e.text.

rgrunber

Overall, looks good. Just some questions of style and where to place things.

I also noticed the following

I'm open to merging even with this as I suspect it might be some misbehaviour in the partition logic on the server-side.

rgrunber · 2023-07-18T19:30:46Z

 	}
 }
+
+let serverReady = false;


Are you using this variable here because you're trying to correctly handle the semicolon in the case of the text blocks also ?

I would have really like to move this into

vscode-java/src/standardLanguageClient.ts

Line 135 in 15ddbfe

registerPasteEventHandler(context, this.languageClient);

if there was a nice way to do so.

Would it make sense to create a helper method that sets :

oldPosition = null; newPosition = null;

so we can move the "smart semicolon" code into the standard client's onReady callback ?

Are you using this variable here because you're trying to correctly handle the semicolon in the case of the text blocks also ?

No, it isn't related to the text block. We are waiting for the server to be ready.

I would have really like to move this into

I have tried it, but I didn't succeed.

Is there an issue with the following ? Basically I just moved all the smart semicolon logic into a new file, smartSemicolonDetection.ts. I made the oldPostion/newPosition variables global in there as well and created a helper function (setSmartSemiColonDetectionState(old, new)) that can be used to access them from the text block logic. I also called registerSmartSemicolonDetection(..) directly in the standardLanguageClient.ts code where we listen for the ServiceReady notification.

patch

diff --git a/src/extension.ts b/src/extension.ts index 90fc982..d0de894 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -24,7 +24,7 @@ import { initialize as initializeRecommendation } from './recommendation'; import * as requirements from './requirements'; import { runtimeStatusBarProvider } from './runtimeStatusBarProvider'; import { serverStatusBarProvider } from './serverStatusBarProvider'; -import { ACTIVE_BUILD_TOOL_STATE, cleanWorkspaceFileName, getJavaServerMode, handleTextBlockClosing, onConfigurationChange, registerSmartSemicolonDetection, ServerMode } from './settings'; +import { ACTIVE_BUILD_TOOL_STATE, cleanWorkspaceFileName, getJavaServerMode, handleTextBlockClosing, onConfigurationChange, ServerMode } from './settings'; import { snippetCompletionProvider } from './snippetCompletionProvider'; import { JavaClassEditorProvider } from './javaClassEditor'; import { StandardLanguageClient } from './standardLanguageClient'; @@ -35,6 +35,7 @@ import { Telemetry } from './telemetry'; import { getMessage } from './errorUtils'; import { TelemetryService } from '@redhat-developer/vscode-redhat-telemetry/lib'; import { activationProgressNotification } from "./serverTaskPresenter"; +import { registerSmartSemicolonDetection } from './smartSemicolonDetection'; const syntaxClient: SyntaxLanguageClient = new SyntaxLanguageClient(); const standardClient: StandardLanguageClient = new StandardLanguageClient(); diff --git a/src/settings.ts b/src/settings.ts index 8cd580f..091c8ee 100644 --- a/src/settings.ts +++ b/src/settings.ts @@ -7,6 +7,7 @@ import { Commands } from './commands'; import { cleanupLombokCache } from './lombokSupport'; import { ensureExists, getJavaConfiguration } from './utils'; import { apiManager } from './apiManager'; +import { setSmartSemiColonDetectionState } from './smartSemicolonDetection'; const DEFAULT_HIDDEN_FILES: string[] = ['**/.classpath', '**/.project', '**/.settings', '**/.factorypath']; const IS_WORKSPACE_JDK_ALLOWED = "java.ls.isJdkAllowed"; @@ -29,9 +30,6 @@ export const ORGANIZE_IMPORTS_ON_PASTE = 'actionsOnPaste.organizeImports'; // ja let oldConfig: WorkspaceConfiguration = getJavaConfiguration(); const gradleWrapperPromptDialogs = []; -let oldPosition: Position = null; -let newPosition: Position = null; - export function onConfigurationChange(workspacePath: string, context: ExtensionContext) { return workspace.onDidChangeConfiguration(params => { if (!params.affectsConfiguration('java')) { @@ -334,8 +332,7 @@ export function handleTextBlockClosing(document: TextDocument, changes: readonly } if (lastChange.text !== '"""";') { if (lastChange.text !== ';') { - oldPosition = null; - newPosition = null; + setSmartSemiColonDetectionState(null, null); } return; } @@ -358,68 +355,3 @@ export function handleTextBlockClosing(document: TextDocument, changes: readonly } } -let serverReady = false; - -export function registerSmartSemicolonDetection(context: ExtensionContext) { - apiManager.getApiInstance().serverReady().then(() => { - serverReady = true; - }); - context.subscriptions.push(commands.registerCommand(Commands.SMARTSEMICOLON_DETECTION_CMD, async () => { - if (!isSemichar() && window.activeTextEditor.document.fileName.endsWith(".java")) { - const params: SmartDetectionParams = { - uri: window.activeTextEditor.document.uri.toString(), - position: window.activeTextEditor!.selection.active, - }; - const response: SmartDetectionParams = await commands.executeCommand(Commands.EXECUTE_WORKSPACE_COMMAND, Commands.SMARTSEMICOLON_DETECTION, JSON.stringify(params)); - if (response !== null) { - window.activeTextEditor!.edit(editBuilder => { - oldPosition = window.activeTextEditor!.selection.active; - editBuilder.insert(response.position, ";"); - window.activeTextEditor.selections = [new Selection(response.position, response.position)]; - newPosition = window.activeTextEditor!.selection.active; - }); - return; - } - } - window.activeTextEditor!.edit(editBuilder => { - editBuilder.insert(window.activeTextEditor!.selection.active, ";"); - }); - newPosition = null; - oldPosition = null; - })); - context.subscriptions.push(commands.registerCommand(Commands.SMARTSEMICOLON_DETECTION_SEMICHAR, async () => { - if (isSemichar()) { - window.activeTextEditor!.edit(editBuilder => { - editBuilder.insert(oldPosition, ";"); - const delRange = new Range(newPosition, new Position(newPosition.line, newPosition.character + 1)); - editBuilder.delete(delRange); - window.activeTextEditor.selections = [new Selection(oldPosition, oldPosition)]; - oldPosition = null; - newPosition = null; - }); - return; - } - window.activeTextEditor!.edit(() => { - commands.executeCommand("deleteLeft"); - }); - oldPosition = null; - newPosition = null; - })); -} - -interface SmartDetectionParams { - uri: String; - position: Position; -} - -function isSemichar() { - const enabled = getJavaConfiguration().get<boolean>("edit.smartSemicolonDetection"); - const isSemichar = serverReady && window.activeTextEditor.selections.length === 1 && enabled && oldPosition !== null && newPosition !== null; - if (isSemichar) { - const active = window.activeTextEditor!.selection.active; - const prev = new Position(active.line, active.character === 0 ? 0 : active.character - 1); - return newPosition.isEqual(prev); - } - return isSemichar; -} - diff --git a/src/smartSemicolonDetection.ts b/src/smartSemicolonDetection.ts new file mode 100644 index 0000000..fceb270 --- /dev/null +++ b/src/smartSemicolonDetection.ts @@ -0,0 +1,73 @@ +'use strict'; + +import { commands, ExtensionContext, Position, Range, Selection, window } from 'vscode'; +import { Commands } from './commands'; +import { getJavaConfiguration } from './utils'; + +let oldPosition: Position = null; +let newPosition: Position = null; + +export function registerSmartSemicolonDetection(context: ExtensionContext) { + context.subscriptions.push(commands.registerCommand(Commands.SMARTSEMICOLON_DETECTION_CMD, async () => { + if (!isSemichar() && window.activeTextEditor.document.fileName.endsWith(".java")) { + const params: SmartDetectionParams = { + uri: window.activeTextEditor.document.uri.toString(), + position: window.activeTextEditor!.selection.active, + }; + const response: SmartDetectionParams = await commands.executeCommand(Commands.EXECUTE_WORKSPACE_COMMAND, Commands.SMARTSEMICOLON_DETECTION, JSON.stringify(params)); + if (response !== null) { + window.activeTextEditor!.edit(editBuilder => { + oldPosition = window.activeTextEditor!.selection.active; + editBuilder.insert(response.position, ";"); + window.activeTextEditor.selections = [new Selection(response.position, response.position)]; + newPosition = window.activeTextEditor!.selection.active; + }); + return; + } + } + window.activeTextEditor!.edit(editBuilder => { + editBuilder.insert(window.activeTextEditor!.selection.active, ";"); + }); + newPosition = null; + oldPosition = null; + })); + context.subscriptions.push(commands.registerCommand(Commands.SMARTSEMICOLON_DETECTION_SEMICHAR, async () => { + if (isSemichar()) { + window.activeTextEditor!.edit(editBuilder => { + editBuilder.insert(oldPosition, ";"); + const delRange = new Range(newPosition, new Position(newPosition.line, newPosition.character + 1)); + editBuilder.delete(delRange); + window.activeTextEditor.selections = [new Selection(oldPosition, oldPosition)]; + oldPosition = null; + newPosition = null; + }); + return; + } + window.activeTextEditor!.edit(() => { + commands.executeCommand("deleteLeft"); + }); + oldPosition = null; + newPosition = null; + })); +} + +interface SmartDetectionParams { + uri: String; + position: Position; +} + +function isSemichar() { + const enabled = getJavaConfiguration().get<boolean>("edit.smartSemicolonDetection"); + const isSemichar = window.activeTextEditor.selections.length === 1 && enabled && oldPosition !== null && newPosition !== null; + if (isSemichar) { + const active = window.activeTextEditor!.selection.active; + const prev = new Position(active.line, active.character === 0 ? 0 : active.character - 1); + return newPosition.isEqual(prev); + } + return isSemichar; +} + +export function setSmartSemiColonDetectionState(oldPos: Position, newPos: Position) { + oldPosition = oldPos; + newPos = newPos; +} \ No newline at end of file diff --git a/src/standardLanguageClient.ts b/src/standardLanguageClient.ts index 6b07f11..2b0f6a6 100644 --- a/src/standardLanguageClient.ts +++ b/src/standardLanguageClient.ts @@ -40,6 +40,7 @@ import { getAllJavaProjects, getJavaConfig, getJavaConfiguration } from "./utils import { Telemetry } from "./telemetry"; import { TelemetryEvent } from "@redhat-developer/vscode-redhat-telemetry/lib"; import { registerDocumentValidationListener } from './diagnostic'; +import { registerSmartSemicolonDetection } from './smartSemicolonDetection'; const extensionName = 'Language Support for Java'; const GRADLE_CHECKSUM = "gradle/checksum/prompt"; @@ -137,6 +138,7 @@ export class StandardLanguageClient { // clients may not have properly configured documentPaste logger.error(error); } + registerSmartSemicolonDetection(context); activationProgressNotification.hide(); if (!hasImported) { showImportFinishNotification(context);

It works fine. I have updated the PR.

snjeza · 2023-07-18T23:44:52Z

@rgrunber I have updated this PR and eclipse-jdtls/eclipse.jdt.ls#2710

Signed-off-by: Snjezana Peco <snjezana.peco@redhat.com>

rgrunber

Overall, this looks good. Just some very minor things to handle. Then we can merge and address any other issues.

There's another small thing I noticed but I'm open to handling it in a separate PR to avoid complicating this one further.

We set javaLSReadyin Started at

vscode-java/src/standardLanguageClient.ts

Lines 152 to 155 in a60b5e4

    
           case 'Started': 
        
           	this.status = ClientStatus.started; 
        
           	serverStatus.updateServerStatus(ServerStatusKind.ready); 
        
           	commands.executeCommand('setContext', 'javaLSReady', true);

. This is important because after it's set, the commands can be triggered.

However, we only register the semicolon detection commands in ServiceReady . For large projects there can be a gap between Started and ServiceReady so there is a moment where the commands may be activated (after Started) but before we have registerd the command (at ServiceReady). The solution is probably to create a separate context state that's triggered within ServiceReady. Update: According to the logs started+serviceready happen not even a second apart so maybe I've just been lucky reproducing the warning. Let's say it's lower priority and we can address this part later.

snjeza · 2023-07-21T23:45:00Z

@rgrunber I have updated the PR.

rgrunber · 2023-07-22T00:30:04Z

There's another issue. Even when java.edit.smartSemicolonDetection.enabled is false, the delegate command still runs because !isSemichar() is still false. I think we need an additional variable to check if it's disabled and skip that delegate command if block. See https://github.com/redhat-developer/vscode-java/pull/3159/files#diff-364a380511abddd995a22cf86f0a35cc9acedbcf311367d0432c7ca9995fdfa3R12 .

Also it looks to me like the purpose of isSemichar() is to detect whether we applied the smart semicolon insertion, so that we know that the next backspace should undo the smart insertion. If so, I think it would be better to call it didSmartSemicolonInsertion

snjeza · 2023-07-22T13:09:44Z

There's another issue. Even when java.edit.smartSemicolonDetection.enabled is false, the delegate command still runs because !isSemichar() is still false. I think we need an additional variable to check if it's disabled and skip that delegate command if block.

Fixed.

Also it looks to me like the purpose of isSemichar() is to detect whether we applied the smart semicolon insertion, so that we know that the next backspace should undo the smart insertion. If so, I think it would be better to call it didSmartSemicolonInsertion

Fixed.

rgrunber

Change looks fine to me. We can address some of the issues in later contributions. The important part is it's disabled by default for now and doesn't introduce any overhead to regular semicolon/backspace usage. Just need to squash + merge.

Contributed by Roland Grunberg <rgrunber@redhat.com>

This reverts commit 3ad9b16.

snjeza requested review from fbricon, gorkem, jdneo, rgrunber and testforstephen June 19, 2023 13:36

snjeza mentioned this pull request Jun 19, 2023

Smart semicolon autocorrect eclipse-jdtls/eclipse.jdt.ls#2710

Merged

snjeza added the in progress label Jun 20, 2023

snjeza force-pushed the issue-703 branch from 75e1d43 to 99dd082 Compare June 20, 2023 20:42

snjeza requested a review from hopehadfield June 22, 2023 23:10

snjeza force-pushed the issue-703 branch 3 times, most recently from 0061d14 to d4e4d21 Compare June 23, 2023 13:29

rgrunber requested changes Jul 18, 2023

View reviewed changes

snjeza changed the title ~~Smart semicolon autocorrect~~ [WIP] Smart semicolon autocorrect Jul 18, 2023

openshift-ci Bot added the do-not-merge/work-in-progress label Jul 18, 2023

snjeza force-pushed the issue-703 branch 2 times, most recently from 9d99d16 to 70ed480 Compare July 18, 2023 23:41

snjeza changed the title ~~[WIP] Smart semicolon autocorrect~~ Smart semicolon autocorrect Jul 18, 2023

openshift-ci Bot removed the do-not-merge/work-in-progress label Jul 18, 2023

snjeza force-pushed the issue-703 branch from 70ed480 to 4cd6429 Compare July 18, 2023 23:57

snjeza removed the in progress label Jul 19, 2023

snjeza force-pushed the issue-703 branch from 079a98f to 1a61bea Compare July 20, 2023 22:12

github-actions Bot added the has conflicts label Jul 21, 2023

Smart semicolon autocorrect

0a66a10

Signed-off-by: Snjezana Peco <snjezana.peco@redhat.com>

snjeza force-pushed the issue-703 branch 3 times, most recently from b63ef9e to a23bdc3 Compare July 21, 2023 18:54

rgrunber approved these changes Jul 21, 2023

View reviewed changes

Comment thread src/commands.ts Outdated

Comment thread src/smartSemicolonDetection.ts Outdated

Comment thread package.json Outdated

snjeza added in progress and removed has conflicts labels Jul 21, 2023

snjeza changed the title ~~Smart semicolon autocorrect~~ [WIP] Smart semicolon autocorrect Jul 21, 2023

openshift-ci Bot added the do-not-merge/work-in-progress label Jul 21, 2023

snjeza force-pushed the issue-703 branch from a23bdc3 to 0d03cd1 Compare July 21, 2023 23:37

snjeza changed the title ~~[WIP] Smart semicolon autocorrect~~ Smart semicolon autocorrect Jul 21, 2023

openshift-ci Bot removed the do-not-merge/work-in-progress label Jul 21, 2023

snjeza removed the in progress label Jul 21, 2023

snjeza force-pushed the issue-703 branch from 0d03cd1 to a57a217 Compare July 22, 2023 13:08

rgrunber approved these changes Jul 24, 2023

View reviewed changes

Move registerSmartSemicolonDetection to standardLanguageClient

df6606e

Contributed by Roland Grunberg <rgrunber@redhat.com>

rgrunber force-pushed the issue-703 branch from a57a217 to df6606e Compare July 24, 2023 20:02

rgrunber merged commit 3ad9b16 into redhat-developer:master Jul 24, 2023

rgrunber mentioned this pull request Jul 26, 2023

Smart semicolon autocorrect? #703

Closed

testforstephen mentioned this pull request Jul 27, 2023

The entire class name is deleted after pressing the backspace key on the keyboard #3226

Closed

testforstephen added a commit that referenced this pull request Jul 27, 2023

Revert "Smart semicolon autocorrect (#3159)"

77954d4

This reverts commit 3ad9b16.

testforstephen mentioned this pull request Jul 27, 2023

Revert "Smart semicolon autocorrect" #3227

Merged

rgrunber pushed a commit that referenced this pull request Jul 27, 2023

Revert "Smart semicolon autocorrect (#3159)"

3c3a8f3

This reverts commit 3ad9b16.

rgrunber pushed a commit that referenced this pull request Jul 27, 2023

Revert "Smart semicolon autocorrect (#3159)"

829e8c6

This reverts commit 3ad9b16.

rgrunber mentioned this pull request Sep 7, 2023

command 'java.edit.smartSemicolonDetectionUndo' not found when clicking backspace in Java file #3277

Closed

	case 'Started':
	this.status = ClientStatus.started;
	serverStatus.updateServerStatus(ServerStatusKind.ready);
	commands.executeCommand('setContext', 'javaLSReady', true);

Uh oh!

Conversation

snjeza commented Jun 19, 2023

Uh oh!

rgrunber commented Jun 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snjeza commented Jun 20, 2023

Uh oh!

snjeza commented Jun 20, 2023

Uh oh!

hopehadfield commented Jun 22, 2023

Uh oh!

snjeza commented Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snjeza commented Jun 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snjeza commented Jun 23, 2023

Uh oh!

snjeza commented Jun 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rgrunber left a comment

Choose a reason for hiding this comment

Uh oh!

rgrunber Jul 18, 2023

Choose a reason for hiding this comment

Uh oh!

snjeza Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rgrunber Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snjeza Jul 20, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

snjeza commented Jul 18, 2023

Uh oh!

rgrunber left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

snjeza commented Jul 21, 2023

Uh oh!

rgrunber commented Jul 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snjeza commented Jul 22, 2023

Uh oh!

rgrunber left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rgrunber commented Jun 19, 2023 •

edited

Loading

snjeza commented Jun 22, 2023 •

edited

Loading

snjeza commented Jun 23, 2023 •

edited

Loading

snjeza commented Jun 23, 2023 •

edited

Loading

snjeza Jul 18, 2023 •

edited

Loading

rgrunber Jul 20, 2023 •

edited

Loading

rgrunber left a comment •

edited

Loading

rgrunber commented Jul 22, 2023 •

edited

Loading

rgrunber left a comment •

edited

Loading