diff --git a/org.eclipse.wildwebdeveloper.tests/src/org/eclipse/wildwebdeveloper/tests/TestMarkdown.java b/org.eclipse.wildwebdeveloper.tests/src/org/eclipse/wildwebdeveloper/tests/TestMarkdown.java index 4fadc00322..fb24ee083f 100644 --- a/org.eclipse.wildwebdeveloper.tests/src/org/eclipse/wildwebdeveloper/tests/TestMarkdown.java +++ b/org.eclipse.wildwebdeveloper.tests/src/org/eclipse/wildwebdeveloper/tests/TestMarkdown.java @@ -16,13 +16,20 @@ import static org.eclipse.wildwebdeveloper.markdown.MarkdownDiagnosticsManager.MARKDOWN_MARKER_TYPE; import static org.junit.jupiter.api.Assertions.assertTrue; +import java.lang.reflect.InvocationHandler; +import java.lang.reflect.Proxy; import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; +import java.util.function.BooleanSupplier; import java.util.stream.Collectors; +import org.eclipse.core.filebuffers.FileBuffers; +import org.eclipse.core.filebuffers.LocationKind; import org.eclipse.core.resources.IFile; import org.eclipse.core.resources.IMarker; import org.eclipse.core.resources.IResource; @@ -34,11 +41,15 @@ import org.eclipse.lsp4e.LanguageServerWrapper; import org.eclipse.lsp4e.LanguageServiceAccessor; import org.eclipse.lsp4e.operations.completion.LSContentAssistProcessor; +import org.eclipse.lsp4j.DocumentDiagnosticParams; +import org.eclipse.lsp4j.DocumentDiagnosticReport; +import org.eclipse.lsp4j.services.LanguageServer; import org.eclipse.ui.PlatformUI; import org.eclipse.ui.editors.text.TextEditor; import org.eclipse.ui.ide.IDE; import org.eclipse.ui.tests.harness.util.DisplayHelper; import org.eclipse.wildwebdeveloper.Activator; +import org.eclipse.wildwebdeveloper.markdown.MarkdownDiagnosticsManager; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; @@ -48,6 +59,105 @@ record MarkdownTest(String markdown, String messagePattern, int severity) { @ExtendWith(AllCleanRule.class) class TestMarkdown { + private record DiagnosticSpy(AtomicInteger calls, + AtomicReference> lastFuture, LanguageServer server) { + } + + private static DiagnosticSpy newDiagnosticSpy() { + final var calls = new AtomicInteger(); + final var lastFuture = new AtomicReference>(); + + final Object textDocumentService = Proxy.newProxyInstance(TestMarkdown.class.getClassLoader(), + new Class[] { org.eclipse.lsp4j.services.TextDocumentService.class }, (proxy, method, args) -> { + if ("diagnostic".equals(method.getName()) && args != null && args.length == 1 + && args[0] instanceof DocumentDiagnosticParams) { + calls.incrementAndGet(); + final var fut = new CompletableFuture(); + lastFuture.set(fut); + return fut; + } + if (CompletableFuture.class.isAssignableFrom(method.getReturnType())) { + return CompletableFuture.completedFuture(null); + } + return null; + }); + + final InvocationHandler serverHandler = (proxy, method, args) -> (switch (method.getName()) { + case "getTextDocumentService" -> textDocumentService; + case "getWorkspaceService" -> null; + case "initialize", "shutdown" -> CompletableFuture.completedFuture(null); + case "exit" -> null; + default -> null; + }); + + final var server = (LanguageServer) Proxy.newProxyInstance(TestMarkdown.class.getClassLoader(), + new Class[] { LanguageServer.class }, serverHandler); + return new DiagnosticSpy(calls, lastFuture, server); + } + + private static boolean waitUpTo(final long timeoutMs, final BooleanSupplier condition) + throws InterruptedException { + final long deadline = System.currentTimeMillis() + timeoutMs; + while (System.currentTimeMillis() < deadline) { + if (condition.getAsBoolean()) + return true; + Thread.sleep(20); + } + return condition.getAsBoolean(); + } + + @Test + void refreshDiagnosticsDoesNothingWhenNoMarkdownBuffersOpen() throws Exception { + final var project = ResourcesPlugin.getWorkspace().getRoot() + .getProject(getClass().getName() + ".nobuf." + System.nanoTime()); + project.create(null); + project.open(null); + + final IFile file = project.getFile("doc.md"); + file.create("# Title\n".getBytes(StandardCharsets.UTF_8), true, false, null); + + final var spy = newDiagnosticSpy(); + MarkdownDiagnosticsManager.refreshAllOpenMarkdownFiles(spy.server()); + + // Wait for debounce window + execution time; should still do nothing since no + // Markdown buffer is open. + assertTrue(waitUpTo(2_000, () -> spy.calls().get() == 0), + "Diagnostic requests should not be made when no Markdown buffers are open"); + } + + @Test + void refreshDiagnosticsIsDedupedWhileInFlight() throws Exception { + final var project = ResourcesPlugin.getWorkspace().getRoot() + .getProject(getClass().getName() + ".dedupe." + System.nanoTime()); + project.create(null); + project.open(null); + + final IFile file = project.getFile("open.md"); + file.create("# Title\n".getBytes(StandardCharsets.UTF_8), true, false, null); + + final var mgr = FileBuffers.getTextFileBufferManager(); + mgr.connect(file.getFullPath(), LocationKind.IFILE, null); + try { + final var spy = newDiagnosticSpy(); + + MarkdownDiagnosticsManager.refreshAllOpenMarkdownFiles(spy.server()); + assertTrue(waitUpTo(2_000, () -> spy.calls().get() == 1), + "Expected exactly one diagnostic request for the open Markdown buffer"); + + // Trigger another refresh while the first diagnostic is still in-flight; should + // not start a second diagnostic. + MarkdownDiagnosticsManager.refreshAllOpenMarkdownFiles(spy.server()); + assertTrue(waitUpTo(2_000, () -> spy.calls().get() == 1), "Expected in-flight refresh to be de-duplicated"); + + final var fut = spy.lastFuture().get(); + if (fut != null && !fut.isDone()) { + fut.complete(null); + } + } finally { + mgr.disconnect(file.getFullPath(), LocationKind.IFILE, null); + } + } + @Test void diagnosticsCoverTypicalMarkdownIssues() throws Exception { var project = ResourcesPlugin.getWorkspace().getRoot().getProject(getClass().getName() + System.nanoTime()); diff --git a/org.eclipse.wildwebdeveloper/src/org/eclipse/wildwebdeveloper/markdown/MarkdownDiagnosticsManager.java b/org.eclipse.wildwebdeveloper/src/org/eclipse/wildwebdeveloper/markdown/MarkdownDiagnosticsManager.java index 833d89d2ab..abd9a5d733 100644 --- a/org.eclipse.wildwebdeveloper/src/org/eclipse/wildwebdeveloper/markdown/MarkdownDiagnosticsManager.java +++ b/org.eclipse.wildwebdeveloper/src/org/eclipse/wildwebdeveloper/markdown/MarkdownDiagnosticsManager.java @@ -13,9 +13,15 @@ package org.eclipse.wildwebdeveloper.markdown; import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; +import java.util.IdentityHashMap; import java.util.List; +import java.util.Locale; +import java.util.Set; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.TimeUnit; import org.eclipse.core.filebuffers.FileBuffers; import org.eclipse.core.filebuffers.IFileBuffer; @@ -28,6 +34,15 @@ import org.eclipse.core.runtime.CoreException; import org.eclipse.core.runtime.ILog; import org.eclipse.core.runtime.IPath; +import org.eclipse.core.runtime.IProgressMonitor; +import org.eclipse.core.runtime.IStatus; +import org.eclipse.core.runtime.Platform; +import org.eclipse.core.runtime.Status; +import org.eclipse.core.runtime.content.IContentType; +import org.eclipse.core.runtime.content.IContentTypeManager; +import org.eclipse.core.runtime.jobs.IJobChangeEvent; +import org.eclipse.core.runtime.jobs.Job; +import org.eclipse.core.runtime.jobs.JobChangeAdapter; import org.eclipse.jface.text.BadLocationException; import org.eclipse.jface.text.IDocument; import org.eclipse.lsp4e.LanguageServers; @@ -49,8 +64,78 @@ */ public final class MarkdownDiagnosticsManager { + private static final String MARKDOWN_CONTENT_TYPE_ID = "org.eclipse.tm4e.language_pack.markdown"; public static final String MARKDOWN_MARKER_TYPE = "org.eclipse.wildwebdeveloper.markdown.problem"; + private static final long REFRESH_DEBOUNCE_MS = 250; + private static final long DIAGNOSTICS_TIMEOUT_SECONDS = 30; + + private static final Set OPEN_MARKDOWN_FILES = ConcurrentHashMap.newKeySet(); + + /** De-dupes diagnostic pulls so repeated refresh requests do not start overlapping diagnostics for the same file/server */ + private static final ConcurrentHashMap> IN_FLIGHT_REFRESHES = new ConcurrentHashMap<>(); + + /** Servers that requested a refresh since the last debounce run (identity-based: some LS proxies do not implement hashCode()) */ + private static final Set PENDING_REFRESH_SERVERS = Collections.newSetFromMap(new IdentityHashMap<>()); + + /** Debounced, serialized refresh runner (avoids a thread pileup when multiple refreshes are requested in quick succession) */ + private static Job REFRESH_JOB; + + /** Guards access to REFRESH_JOB and the pending refresh bookkeeping below */ + private static final Object REFRESH_JOB_LOCK = new Object(); + + /** Set when a refresh is requested while the job is running (rescheduled in the job-complete listener) */ + private static boolean REFRESH_RESCHEDULE_REQUESTED; + + private static boolean isMarkdownFile(final IFile file) { + if (file == null) + return false; + try { + final IContentTypeManager ctm = Platform.getContentTypeManager(); + final IContentType markdownCT = ctm.getContentType(MARKDOWN_CONTENT_TYPE_ID); + if (markdownCT != null) { + final IContentType fileCT = ctm.findContentTypeFor(file.getName()); + if (fileCT != null && fileCT.isKindOf(markdownCT)) + return true; + } + } catch (final Exception ex) { + ILog.get().warn(ex.getMessage(), ex); + } + + // Fallback: cheap extension check (and keeps behavior if TM4E is absent / not initialized) + final String ext = file.getFileExtension(); + if (ext == null) + return false; + return switch (ext.toLowerCase(Locale.ROOT)) { + case "md", "markdown", "mdown" -> true; + default -> false; + }; + } + + private static IFile toWorkspaceFile(final IPath location) { + if (location == null) + return null; + + final var root = ResourcesPlugin.getWorkspace().getRoot(); + final IResource res = root.findMember(location); + if (res instanceof final IFile file) + return file; + return root.getFileForLocation(location); + } + + private static List snapshotOpenMarkdownFiles() { + if (OPEN_MARKDOWN_FILES.isEmpty()) + return List.of(); + + final var out = new ArrayList(OPEN_MARKDOWN_FILES.size()); + for (final IFile file : OPEN_MARKDOWN_FILES) { + if (file != null && file.exists() && isMarkdownFile(file)) { + out.add(file); + } + } + return out; + } + static { // Remove problem markers when a Markdown text buffer is disposed (editor closed) FileBuffers.getTextFileBufferManager().addFileBufferListener(new IFileBufferListener() { @@ -66,7 +151,15 @@ public void bufferContentReplaced(final IFileBuffer buffer) { @Override public void bufferCreated(final IFileBuffer buffer) { - // no-op + try { + final IPath location = buffer.getLocation(); + final IFile file = toWorkspaceFile(location); + if (file != null && file.exists() && isMarkdownFile(file)) { + OPEN_MARKDOWN_FILES.add(file); + } + } catch (final Exception ex) { + ILog.get().warn(ex.getMessage(), ex); + } } @Override @@ -75,18 +168,17 @@ public void bufferDisposed(final IFileBuffer buffer) { if (location == null) return; - /* - * remove all problem markers on editor close - */ try { - final var root = ResourcesPlugin.getWorkspace().getRoot(); - final var res = root.findMember(location); - if (res instanceof final IFile file && file.exists()) { - final String name = file.getName().toLowerCase(); - if (name.endsWith(".md") || name.endsWith(".markdown") || name.endsWith(".mdown")) { - clearMarkers(file); - } - } + final IFile file = toWorkspaceFile(location); + if (file == null || !file.exists() || !isMarkdownFile(file)) + return; + + OPEN_MARKDOWN_FILES.remove(file); + + /* + * remove all problem markers on editor close + */ + clearMarkers(file); } catch (Exception ex) { ILog.get().warn(ex.getMessage(), ex); } @@ -119,7 +211,18 @@ public void underlyingFileDeleted(final IFileBuffer buffer) { @Override public void underlyingFileMoved(final IFileBuffer buffer, IPath path) { - // no-op + try { + final IFile newFile = toWorkspaceFile(buffer.getLocation()); + final IFile otherFile = toWorkspaceFile(path); + if (newFile != null) + OPEN_MARKDOWN_FILES.remove(newFile); + if (otherFile != null) + OPEN_MARKDOWN_FILES.remove(otherFile); + if (newFile != null && newFile.exists() && isMarkdownFile(newFile)) + OPEN_MARKDOWN_FILES.add(newFile); + } catch (final Exception ex) { + ILog.get().warn(ex.getMessage(), ex); + } } }); @@ -192,51 +295,125 @@ private static void clearMarkers(final IFile file) throws CoreException { file.deleteMarkers(MARKDOWN_MARKER_TYPE, true, IResource.DEPTH_ZERO); } - private static List extractDiagnostics(final DocumentDiagnosticReport report) { - if (report == null) + private static List extractDiagnostics(final RelatedFullDocumentDiagnosticReport full) { + if (full == null) return List.of(); - if (report.isLeft()) { - final RelatedFullDocumentDiagnosticReport full = report.getLeft(); - final var out = new ArrayList(); - if (full.getItems() != null) - out.addAll(full.getItems()); - if (full.getRelatedDocuments() != null) { - for (final Either rel : full.getRelatedDocuments() - .values()) { - if (rel != null && rel.isLeft()) { - final FullDocumentDiagnosticReport rfull = rel.getLeft(); - if (rfull.getItems() != null) { - out.addAll(rfull.getItems()); - } + + final var out = new ArrayList(); + if (full.getItems() != null) + out.addAll(full.getItems()); + if (full.getRelatedDocuments() != null) { + for (final Either rel : full.getRelatedDocuments().values()) { + if (rel != null && rel.isLeft()) { + final FullDocumentDiagnosticReport rfull = rel.getLeft(); + if (rfull.getItems() != null) { + out.addAll(rfull.getItems()); } - // if rel.isRight() -> unchanged for that related doc: ignore } + // if rel.isRight() -> unchanged for that related doc: ignore } - return out; - } else if (report.isRight()) { + } + return out; + } + + private static void handleDiagnosticReport(final IFile file, final DocumentDiagnosticReport report) { + if (file == null || !file.exists() || report == null) + return; + + if (report.isRight()) { // Unchanged for the main document: do not touch markers + return; + } + + if (report.isLeft()) { + applyMarkers(file, extractDiagnostics(report.getLeft())); } - return List.of(); } - public static void refreshAllOpenMarkdownFiles(final LanguageServer languageServer) { - // Collect .md-like files from open workspace editors would be ideal; for simplicity, scan workspace for *.md, *.markdown, *.mdown - try { - ResourcesPlugin.getWorkspace().getRoot().accept(res -> { - if (res.getType() == IResource.FILE) { - final String name = res.getName().toLowerCase(); - if (name.endsWith(".md") || name.endsWith(".markdown") || name.endsWith(".mdown")) { - refreshFile((IFile) res, languageServer); + private static void scheduleRefreshAllOpenMarkdownFiles(final LanguageServer languageServer) { + if (languageServer == null) + return; + + synchronized (REFRESH_JOB_LOCK) { + PENDING_REFRESH_SERVERS.add(languageServer); + if (REFRESH_JOB == null) { + REFRESH_JOB = new Job("Wild Web Developer Markdown diagnostics refresh") { + @Override + protected IStatus run(final IProgressMonitor monitor) { + final List languageServersToRefresh; + synchronized (REFRESH_JOB_LOCK) { + if (PENDING_REFRESH_SERVERS.isEmpty()) + return Status.OK_STATUS; + languageServersToRefresh = new ArrayList<>(PENDING_REFRESH_SERVERS); + PENDING_REFRESH_SERVERS.clear(); + } + + if (monitor.isCanceled()) + return Status.OK_STATUS; + + final List openFiles = snapshotOpenMarkdownFiles(); + if (openFiles.isEmpty()) + return Status.OK_STATUS; + + for (final LanguageServer ls : languageServersToRefresh) { + if (monitor.isCanceled()) + break; + for (final IFile file : openFiles) { + if (monitor.isCanceled()) + break; + refreshFile(file, ls); + } + } + return Status.OK_STATUS; } - return false; - } - return true; - }); - } catch (final Exception ex) { - ILog.get().warn(ex.getMessage(), ex); + }; + REFRESH_JOB.setSystem(true); + REFRESH_JOB.addJobChangeListener(new JobChangeAdapter() { + @Override + public void done(final IJobChangeEvent event) { + synchronized (REFRESH_JOB_LOCK) { + if (!REFRESH_RESCHEDULE_REQUESTED && PENDING_REFRESH_SERVERS.isEmpty()) + return; + REFRESH_RESCHEDULE_REQUESTED = false; + + if (REFRESH_JOB == null || REFRESH_JOB.getState() != Job.NONE) { + REFRESH_RESCHEDULE_REQUESTED = true; + return; + } + + try { + REFRESH_JOB.schedule(REFRESH_DEBOUNCE_MS); + } catch (final IllegalStateException ex) { + // Job got scheduled concurrently, try again when it completes. + REFRESH_RESCHEDULE_REQUESTED = true; + } + } + } + }); + } + + // Debounce: keep only the latest refresh request. + // + // Avoid (re-)scheduling while RUNNING; schedule() would throw IllegalStateException. + // Instead, mark pending and let the JobChangeListener reschedule once it completes. + if (REFRESH_JOB.getState() == Job.RUNNING) { + REFRESH_RESCHEDULE_REQUESTED = true; + return; + } + + try { + REFRESH_JOB.cancel(); + REFRESH_JOB.schedule(REFRESH_DEBOUNCE_MS); + } catch (final IllegalStateException ex) { + REFRESH_RESCHEDULE_REQUESTED = true; + } } } + public static void refreshAllOpenMarkdownFiles(final LanguageServer languageServer) { + scheduleRefreshAllOpenMarkdownFiles(languageServer); + } + public static void refreshFile(final IFile file) { try { if (file == null || !file.exists()) @@ -254,18 +431,31 @@ public static void refreshFile(final IFile file) { } private static void refreshFile(final IFile file, final LanguageServer languageServer) { - try { - if (file == null || !file.exists()) - return; + if (file == null || !file.exists() || languageServer == null) + return; + + // Include language server identity so de-duping does not hide refreshes across different server instances. + final String key = file.getFullPath().toString() + "@" + System.identityHashCode(languageServer); + IN_FLIGHT_REFRESHES.compute(key, (k, existing) -> { + if (existing != null && !existing.isDone()) + return existing; final String uri = toLspFileUri(file); final var params = new DocumentDiagnosticParams(); params.setTextDocument(new TextDocumentIdentifier(uri)); - final DocumentDiagnosticReport report = languageServer.getTextDocumentService().diagnostic(params).get(); - applyMarkers(file, extractDiagnostics(report)); - } catch (final Exception ex) { - ILog.get().warn(ex.getMessage(), ex); - } + + final CompletableFuture started = languageServer.getTextDocumentService() + .diagnostic(params) + .orTimeout(DIAGNOSTICS_TIMEOUT_SECONDS, TimeUnit.SECONDS) + .thenAccept(report -> handleDiagnosticReport(file, report)) + .exceptionally(ex -> { + ILog.get().warn(ex.getMessage(), ex); + return null; + }); + + started.whenComplete((v, ex) -> IN_FLIGHT_REFRESHES.remove(k, started)); + return started; + }); } private static int toIMarkerSeverity(final DiagnosticSeverity sev) { diff --git a/org.eclipse.wildwebdeveloper/src/org/eclipse/wildwebdeveloper/markdown/MarkdownLanguageClient.java b/org.eclipse.wildwebdeveloper/src/org/eclipse/wildwebdeveloper/markdown/MarkdownLanguageClient.java index bed4e837bd..a5feb7ddd8 100644 --- a/org.eclipse.wildwebdeveloper/src/org/eclipse/wildwebdeveloper/markdown/MarkdownLanguageClient.java +++ b/org.eclipse.wildwebdeveloper/src/org/eclipse/wildwebdeveloper/markdown/MarkdownLanguageClient.java @@ -183,7 +183,8 @@ public CompletableFuture> configuration(final ConfigurationParams p // Acknowledge diagnostic refresh requests and trigger a diagnostic pull @Override public CompletableFuture refreshDiagnostics() { - return CompletableFuture.runAsync(() -> MarkdownDiagnosticsManager.refreshAllOpenMarkdownFiles(getLanguageServer())); + MarkdownDiagnosticsManager.refreshAllOpenMarkdownFiles(getLanguageServer()); + return CompletableFuture.completedFuture(null); } /** @@ -240,7 +241,7 @@ public CompletableFuture>> parseMarkdown(final Map MarkdownDiagnosticsManager.refreshFile(file)); + MarkdownDiagnosticsManager.refreshFile(file); } } catch (final Exception ignore) { }