eclipse / lsp4jakarta

Language Server for Jakarta EE
Eclipse Public License 2.0
33 stars 51 forks source link

Diagnostics delayed by 1 keystroke #26

Closed rzgry closed 2 years ago

rzgry commented 3 years ago

Diagnostics added by https://github.com/MicroShed/jakarta-ls/pull/23 are always 1 keystroke behind.

kathrynkodama commented 3 years ago

The compilation unit being resolved here: https://github.com/MicroShed/jakarta-ls/blob/master/jakarta-eclipse/src/org/jakarta/jdt/JDTUtils.java#L111 is one keystroke behind the current document.

This results in an empty diagnostics list: https://github.com/MicroShed/jakarta-ls/blob/master/jakarta-eclipse/src/org/jakarta/jdt/JDTServicesManager.java#L51-L53

PENGYUXIONG commented 3 years ago

I dont have much progress on this bug still.

My thought is that, either https://github.com/MicroShed/jakarta-ls/blob/26641efb51a3fbd0e2f05118d0d4f1548395418d/jakarta-eclipse/src/org/jakarta/jdt/JDTUtils.java#L117

is returning incorrect Ifile which I am not sure how to verify

or it can possibly be JavaCore.createCompilationUnitFrom(resource); from https://github.com/MicroShed/jakarta-ls/blob/26641efb51a3fbd0e2f05118d0d4f1548395418d/jakarta-eclipse/src/org/jakarta/jdt/JDTUtils.java#L125 is not behave properly

kathrynkodama commented 3 years ago

Adding the following to https://github.com/MicroShed/jakarta-ls/blob/master/jakarta.ls/src/main/java/io/microshed/jakartals/JakartaLanguageServer.java#L41 will update diagnostics on save:

serverCapabilities.setTextDocumentSync(TextDocumentSyncKind.Incremental);

Keystroke delay still exists without saving

tiganov commented 3 years ago

Adding a delay before createCompilationUnitFrom is called in JDTUtils.java seems to resolve the keystroke delay, but I'm not quite sure why, yet.

try {
    Thread.sleep(1000);
} catch (InterruptedException e) {
    e.printStackTrace();
}
return JavaCore.createCompilationUnitFrom(resource);
tiganov commented 2 years ago

It appears ICompilationUnit.isConsistent returns false immediately after the unit is created in JDTServicesManager.getJavaDiagnostics(). Introducing a sufficient delay (like I've shown in my previous comment) will allow the underlying resource to catch up (and then isConsistent will return true). Therefore, adding a while loop to allow the unit to catch up solves the keystroke delay.

public List<PublishDiagnosticsParams> getJavaDiagnostics(JakartaDiagnosticsParams javaParams) {
    [...]
        ICompilationUnit unit = JDTUtils.resolveCompilationUnit(u);
        try {
            while (!unit.isConsistent()) { }
        } catch (JavaModelException e) { }
        for (DiagnosticsCollector d : diagnosticsCollectors) {
            d.collectDiagnostics(unit, diagnostics);
        }
    [...]
}

Alternatively, the loop can be added to JDTUtils.resolveCompilationUnit, instead.

kathrynkodama commented 2 years ago

It appears ICompilationUnit.isConsistent returns false immediately after the unit is created in JDTServicesManager.getJavaDiagnostics(). Introducing a sufficient delay (like I've shown in my previous comment) will allow the underlying resource to catch up (and then isConsistent will return true). Therefore, adding a while loop to allow the unit to catch up solves the keystroke delay.

public List<PublishDiagnosticsParams> getJavaDiagnostics(JakartaDiagnosticsParams javaParams) {
    [...]
        ICompilationUnit unit = JDTUtils.resolveCompilationUnit(u);
        try {
            while (!unit.isConsistent()) { }
        } catch (JavaModelException e) { }
        for (DiagnosticsCollector d : diagnosticsCollectors) {
            d.collectDiagnostics(unit, diagnostics);
        }
    [...]
}

Alternatively, the loop can be added to JDTUtils.resolveCompilationUnit, instead.

This looks great, thanks @tiganov! Any concerns with unit.isConsistent() never returning true and getting stuck in that while loop?

tiganov commented 2 years ago

@kathrynkodama I haven't had issues with the while loop getting stuck, but I will put a timeout condition just in case.

kathrynkodama commented 2 years ago

Closed with #201