Add SourceViewer#computeStyleRanges() to highlight external documents

[tobiasmelcher]

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]

Test Results

   852 files  ± 0     852 suites  ±0   52m 53s ⏱️ - 1m 27s
 7 923 tests +24   7 680 ✅ +24  243 💤 ±0  0 ❌ ±0 
20 271 runs  +72  19 616 ✅ +72  655 💤 ±0  0 ❌ ±0 

Results for commit 3142c65. ± Comparison against base commit 9b991c8.

♻️ This comment has been updated with latest results.

[tobiasmelcher]

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

[vogella]

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.

[tobiasmelcher]

c47d26a

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

[danthe1st]

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]

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]

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]

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

1. 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?

2. 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.

[tobiasmelcher]

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

1. 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?
2. 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]

Thanks @tobiasmelcher LGTM, please merge.