Add SourceViewer#computeStyleRanges() to highlight external documents by tobiasmelcher · Pull Request #3868 · eclipse-platform/eclipse.platform.ui

tobiasmelcher · 2026-04-06T20:34:53Z

Introduces computeStyleRanges(IDocument, IRegion) on SourceViewer, which applies the viewer's own presentation reconciler and partitioner to an external document, returning the resulting SWT StyleRanges for the given region without affecting the viewer's current document.
This method will be used by the unified diff implementation to calculate the syntax coloring for the parts rendered inside the code mining which are not part of the original editor document (see eclipse-platform/eclipse.platform#2560).

github-actions · 2026-04-06T20:48:23Z

Test Results

852 files ± 0 852 suites ±0 53m 33s ⏱️ -47s
7 908 tests + 9 7 665 ✅ + 9 243 💤 ±0 0 ❌ ±0
20 226 runs +27 19 571 ✅ +27 655 💤 ±0 0 ❌ ±0

Results for commit 392609f. ± Comparison against base commit 9b991c8.

♻️ This comment has been updated with latest results.

tobiasmelcher · 2026-04-07T08:33:30Z

test error org.eclipse.e4.ui.tests.workbench.PartRenderingEngineTests#ensureCleanUpAddonCleansUp not related to this change (#3581)

vogella · 2026-04-17T11:07:54Z

test error org.eclipse.e4.ui.tests.workbench.PartRenderingEngineTests#ensureCleanUpAddonCleansUp not related to this change (#3581)

This test is fixed now (since yesterday)

vogella

Idea is sound and SourceViewer is a reasonable place for this. Main concern is exception-safety of the partitioner swap (see inline on the final connect(originalDocument)); the rest are tightenings.

vogella · 2026-04-17T11:12:19Z

+	 * The viewer's partitioner is temporarily connected to the given document so that its
+	 * presentation repairers can compute the correct highlighting. The viewer's own document is not
+	 * affected.
+	 * </p>


The Javadoc says "The viewer's own document is not affected", but this method temporarily disconnects the original document's partitioner and reconnects it only on the happy path. Worth documenting the side effect on the original document's partitioner state, and the UI-thread-only assumption.

vogella · 2026-04-17T11:12:19Z

+	 * @throws BadLocationException if {@code damage} is outside the bounds of {@code document}
+	 * @since 3.31
+	 */
+	public List<StyleRange> computeStyleRanges(IDocument document, IRegion damage) throws BadLocationException {


Nit: damage leaks reconciler/repairer jargon into a public API. For callers who aren't thinking in damage/repair terms, region would read more naturally.

vogella · 2026-04-17T11:12:19Z

+		}
+		IDocument originalDocument= getDocument();
+		IDocumentPartitioner partitioner= originalDocument.getDocumentPartitioner();
+		document.setDocumentPartitioner(partitioner);


This document.setDocumentPartitioner(partitioner) is redundant with the IDocumentExtension3 branch below, which sets a partitioner on document again via ext.setDocumentPartitioner(partition, ...). In the common case the partitioner is set twice. Consider picking one path.

vogella · 2026-04-17T11:12:19Z

+				try {
+					originalDocumentPartitioner.connect(document);
+				} finally {
+					ext.setDocumentPartitioner(partition, originalDocumentPartitioner);
+				}


The finally here runs ext.setDocumentPartitioner(partition, originalDocumentPartitioner) on the external document even when connect(document) throws. That isn't a safety pattern — it's guaranteed cleanup pointed at the wrong object. If connect fails, the original document's partitioner is already disconnected and nothing restores it.

vogella · 2026-04-17T11:12:19Z

+		if (originalDocumentPartitioner != null) {
+			originalDocumentPartitioner.connect(originalDocument);
+		}


Not exception-safe. If TextUtilities.computePartitioning or any repairer.createPresentation above throws, this connect(originalDocument) never runs and the viewer is left with its own document's partitioner connected to the external document. The whole swap-and-use block should be wrapped in try { ... } finally { if (originalDocumentPartitioner != null) originalDocumentPartitioner.connect(originalDocument); } (plus restoring any partitioner mapping that was changed on the original document).

Introduces computeStyleRanges(IDocument, IRegion) on SourceViewer, which applies the viewer's own presentation reconciler and partitioner to an external document, returning the resulting SWT StyleRanges for the given region without affecting the viewer's current document.

tobiasmelcher · 2026-04-17T17:07:34Z

c47d26a

thanks a lot @vogella for the detailed review. I fixed the issues with c47d26a - could you please verify? Thanks a lot.

danthe1st · 2026-04-17T19:27:34Z

I'm not a committer but I guess it might be useful to add some tests ensuring there won't be any regressions here?

trancexpress · 2026-04-17T19:46:48Z

I'm not a committer but I guess it might be useful to add some tests ensuring there won't be any regressions here?

For API especially.

tobiasmelcher · 2026-04-18T17:06:11Z

I'm not a committer but I guess it might be useful to add some tests ensuring there won't be any regressions here?

For API especially.

@danthe1st @trancexpress test added with d6456a7

vogella · 2026-04-18T19:29:46Z

Thanks for the iteration @tobiasmelcher. Two remaining concerns before I'm comfortable approving. The unit-test angle can be handled in a follow-up.

External document left with a dangling partitioner reference. In the IDocumentExtension3 branch you call ((IDocumentExtension3) document).setDocumentPartitioner(partition, originalDocumentPartitioner) but never clear it. After the method returns, the caller's external document still holds a reference to a partitioner that the finally block has (correctly) reconnected to originalDocument. The else branch has the same leak via document.setDocumentPartitioner(originalDocument.getDocumentPartitioner()). Could you null out / restore the external document's partitioner in finally so the caller's document is left in the state we found it?
originalDocument may be null. If computeStyleRanges is called on a viewer whose document hasn't been set yet, originalExt.getDocumentPartitioner(partition) and originalDocument.getDocumentPartitioner() will NPE. A small guard (early return or Assert.isNotNull) would make the contract explicit.

SourceViewer#computeStyleRanges()

tobiasmelcher · 2026-04-19T13:51:45Z

Thanks for the iteration @tobiasmelcher. Two remaining concerns before I'm comfortable approving. The unit-test angle can be handled in a follow-up.

External document left with a dangling partitioner reference. In the IDocumentExtension3 branch you call ((IDocumentExtension3) document).setDocumentPartitioner(partition, originalDocumentPartitioner) but never clear it. After the method returns, the caller's external document still holds a reference to a partitioner that the finally block has (correctly) reconnected to originalDocument. The else branch has the same leak via document.setDocumentPartitioner(originalDocument.getDocumentPartitioner()). Could you null out / restore the external document's partitioner in finally so the caller's document is left in the state we found it?

originalDocument may be null. If computeStyleRanges is called on a viewer whose document hasn't been set yet, originalExt.getDocumentPartitioner(partition) and originalDocument.getDocumentPartitioner() will NPE. A small guard (early return or Assert.isNotNull) would make the contract explicit.

@vogella thanks a lot for the review - should be now done with 392609f

vogella · 2026-04-19T17:34:02Z

+				documentExt.setDocumentPartitioner(partition, originalDocumentPartitioner);
+			} else {
+				externalDocPartitioner= document.getDocumentPartitioner();
+				document.setDocumentPartitioner(originalDocument.getDocumentPartitioner());


This else branch is inconsistent with the IDocumentExtension3 path above: it swaps the partitioner reference on the external document via setDocumentPartitioner but never calls disconnect() / connect(document). The partitioner is still connected to originalDocument while being asked to partition document, which can produce wrong results or corrupt the partitioner's internal state. The matching finally branch has the same issue.

Since Document (the standard implementation) always implements IDocumentExtension3, one clean option is to drop the else branch entirely and require IDocumentExtension3 on both documents (throwing or returning an empty list otherwise). Alternatively, mirror the disconnect/connect dance from the happy path:

Suggested change

document.setDocumentPartitioner(originalDocument.getDocumentPartitioner());

} else {

externalDocPartitioner= document.getDocumentPartitioner();

IDocumentPartitioner origPartitioner= originalDocument.getDocumentPartitioner();

if (origPartitioner != null) {

origPartitioner.disconnect();

origPartitioner.connect(document);

}

document.setDocumentPartitioner(origPartitioner);

}

The finally branch would need the symmetric disconnect/reconnect back to originalDocument. Also, please add a test that exercises a non-IDocumentExtension3 document so this path isn't untested.

vogella · 2026-04-19T17:34:12Z

+		Assert.isTrue(Display.getCurrent() != null, "computeStyleRanges must be called from SWT UI thread"); //$NON-NLS-1$
+		IDocument originalDocument= getDocument();
+		Assert.isNotNull(originalDocument, "viewer must have a document before calling computeStyleRanges"); //$NON-NLS-1$
+		String partition= IDocumentExtension3.DEFAULT_PARTITIONING;


Naming nit: this variable holds a partitioning name (a namespace like DEFAULT_PARTITIONING), not a partition/content-type. IPresentationReconcilerExtension#getDocumentPartitioning() and IDocumentExtension3#getDocumentPartitioner(String partitioning) use the term "partitioning". Renaming to partitioning (and extPartition → extPartitioning) would match surrounding API vocabulary and make the calls below read correctly.

Suggested change

String partition= IDocumentExtension3.DEFAULT_PARTITIONING;

String partitioning= IDocumentExtension3.DEFAULT_PARTITIONING;

Requires the same rename at the uses further down (documentExt.getDocumentPartitioner(partition), setDocumentPartitioner(partition, ...), TextUtilities.computePartitioning(document, partition, ...)).

vogella · 2026-04-19T17:34:20Z

+				externalDocPartitioner= document.getDocumentPartitioner();
+				document.setDocumentPartitioner(originalDocument.getDocumentPartitioner());
+			}
+			TextPresentation presentation= new TextPresentation(region, 1000);


Magic number: 1000 is an expected-size hint for TextPresentation. For small regions it over-allocates; for large regions it under-allocates. Derive it from the region length (or drop the hint and use the default constructor) so it scales with actual input.

Suggested change

TextPresentation presentation= new TextPresentation(region, 1000);

TextPresentation presentation= new TextPresentation(region, Math.max(region.getLength() / 10, 16));

I got this magic value from PresentationReconciler.createPresentation. There is no constructor which allows to pass only a damage region. I will take over your proposal.

tobiasmelcher force-pushed the computeStyleRanges branch from 6571f85 to 18539e5 Compare April 7, 2026 07:29

tobiasmelcher force-pushed the computeStyleRanges branch from 18539e5 to 0afb18c Compare April 7, 2026 10:46

tobiasmelcher mentioned this pull request Apr 17, 2026

Introduce Unified Diff display mode to Compare editor eclipse-platform/eclipse.platform#2560

Draft

vogella reviewed Apr 17, 2026

View reviewed changes

tobias-melcher added 2 commits April 17, 2026 18:32

Fix exception-safety and Javadoc of SourceViewer#computeStyleRanges()

c47d26a

tobiasmelcher force-pushed the computeStyleRanges branch from 0afb18c to c47d26a Compare April 17, 2026 17:06

add SourceViewerComputeStyleRangesTest

d6456a7

Fix partitioner leak and add null guard in

392609f

SourceViewer#computeStyleRanges()

vogella reviewed Apr 19, 2026

View reviewed changes

-				document.setDocumentPartitioner(originalDocument.getDocumentPartitioner());
+			} else {
+				externalDocPartitioner= document.getDocumentPartitioner();
+				IDocumentPartitioner origPartitioner= originalDocument.getDocumentPartitioner();
+				if (origPartitioner != null) {
+					origPartitioner.disconnect();
+					origPartitioner.connect(document);
+				}
+				document.setDocumentPartitioner(origPartitioner);
+			}

	String partition= IDocumentExtension3.DEFAULT_PARTITIONING;
	String partitioning= IDocumentExtension3.DEFAULT_PARTITIONING;

	TextPresentation presentation= new TextPresentation(region, 1000);
	TextPresentation presentation= new TextPresentation(region, Math.max(region.getLength() / 10, 16));

Conversation

tobiasmelcher commented Apr 6, 2026

Uh oh!

github-actions bot commented Apr 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

tobiasmelcher commented Apr 7, 2026

Uh oh!

vogella commented Apr 17, 2026

Uh oh!

vogella left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobiasmelcher commented Apr 17, 2026

Uh oh!

danthe1st commented Apr 17, 2026

Uh oh!

trancexpress commented Apr 17, 2026

Uh oh!

tobiasmelcher commented Apr 18, 2026

Uh oh!

vogella commented Apr 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tobiasmelcher commented Apr 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

github-actions bot commented Apr 6, 2026 •

edited

Loading

vogella commented Apr 18, 2026 •

edited

Loading