add always default folding reconciler

[ghentschke]

This enables folding in the generic text editor if the contributed folding reconciler cannot be applied e.g. LSP server does not provide folding support. Folding settings of several reconcilers will be applied to the viewer.

Background: see LSP4E issue 410

[mickaelistria]

The current tests seem to require the Indent reconciler to be ignored if some custom reconcilier is provided.

[ghentschke]

The current tests seem to require the Indent reconciler to be ignored if some custom reconcilier is provided.

Ok, I'll check that on monday

[ghentschke]

Adjusted test cases. Please review

[ghentschke]

Can somebody tell me why it does not build on Jenkins: Failed to execute goal org.eclipse.tycho.extras:tycho-p2-extras-plugin:4.0.0-SNAPSHOT:compare-version-with-baselines (compare-attached-artifacts-with-release) on project org.eclipse.text.quicksearch: Baseline and reactor have same fully qualified version, but with different content What can I do about this? I think this failure has nothing to do with my change?

[akurtakov]

I'll take a look.

[akurtakov]

Your change was not on top of master and usually this is the first thing I check in case of unrelated failures. The error itself means that the given bundle was changed but its version was not bumped in the current release stream.

[ghentschke]

Okay, thank you!

[ghentschke]

It looks like a desired perspective even with LSP4E: if a language server supports folding in a way that is specific to the language, it probably is more specialized and better than the default one, so the default one should be disabled.

Okay, but I think there should be a possibility to to disable the language server folding and use the indent based instead. This is important in cases the server does not supports folding or poorly (like clangd does actually). VSCode does support the enablement of the language server folding and uses the indent based as a fallback.
This leads to two approaches:

1. Add a separate enablement for folding in LSP4E - or -
2. When LSP4E detects that the LS does not support folding, the configuration of the generic text editor has to be changed dynamically in a way that the default folding should be used instead: Uninstall the LS folding reconciler and install the default one)

Any idea on another approach?

So I believe the tests should not be changed and that we should really be looking at a solution that uses the default reconciler until something better is available.

This implies for me, that the change has to be done in the folding reconciler provider? Today, I have no possibilities to disable the LSP based folding on the LSP4E side.

[mickaelistria]

As mentioned on the other ticket, another approach, which doesn't depend on LSP4E and seems more generic is to let the DefaultFoldingReconciler participate until some other folding annotation is added by another reconciler; and whenever another folding annotation is created, remove the default ones and disable (no-op) the default folding reconciler.

This is important in cases the server does not supports folding or poorly (like clangd does actually). VSCode does support the enablement of the language server folding and uses the indent based as a fallback.

This is not something we'll care about in Platform; such workarounds should be part of your language server integration (eg tweak it or the messages in some way that folding server capability is removed), or better be invested in fixing clangd. Overall, it would be more consensual and probably easier to just fix clangd.

[ghentschke]

As mentioned on the other ticket, another approach, which doesn't depend on LSP4E and seems more generic is to let the DefaultFoldingReconciler participate until some other folding annotation is added by another reconciler; and whenever another folding annotation is created, remove the default ones and disable (no-op) the default folding reconciler.

The reconcilers are running concurrently in separate threads. When a file gets opened in an editor it's not defined which reconciler runs first. Thus the DefaultFoldingReconciler can detect folding annotation from another reconciler for the first time when the file gets modified and the reconcile method gets called again:

Snippet from org.eclipse.ui.internal.genericeditor.folding.IndentFoldingStrategy.reconcile(DirtyRegion, IRegion): with foldingAnnotationFromOtherReconciler call to detect other reconcilers:

    @Override
    public void reconcile(DirtyRegion dirtyRegion, IRegion subRegion) {
        if (projectionAnnotationModel != null) {

            // these are what are passed off to the annotation model to
            // actually create and maintain the annotations
            List<Annotation> modifications = new ArrayList<>();
            List<FoldingAnnotation> deletions = new ArrayList<>();
            List<FoldingAnnotation> existing = new ArrayList<>();
            Map<Annotation, Position> additions = new HashMap<>();

            // if another reconciler is working, delete all folding annotations:
            if (foldingAnnotationFromOtherReconciler(dirtyRegion)) {
                deletions = getAllAnnotationsForDeletion(dirtyRegion);
                projectionAnnotationModel.modifyAnnotations(deletions.toArray(new Annotation[1]), additions,
                        modifications.toArray(new Annotation[0]));
                return;
            }
                        ...

I think this is not a proper solution either. Or am I missing something?

[mickaelistria]

The DefaultFoldingReconciler (or the IndentFoldingStrategy) can place a listener on the projectAnnotationModel, eg

projectionAnnotationModel.addAnnotationModelListener(model -> {
  IndentFoldingStrategy.this.hasExternalFoldingAnnotations = containsExtenalFoldingAnnotations(model);
  if (hasExternalFoldingAnnotations) {
    removeCurrentFoldingAnnotations();
  } else {
    initialReconcile();
  }
});

and reconcile starting with if (projectionAnnotationModel != null && !hasExternalFoldingAnnotations).

[ghentschke]

I added a new commit with a listener in IndentFoldingStrategy. Removed my changes in FoldingTest.java

This works but the folding test results are random now, because the event handling is not predictable. It's not safe that the default folding reconciler annotation has been removed when the test result will be checked by checkFolding.

[mickaelistria]

Thanks. I'll look at improving the tests later today. The idea is to make them "async", ie allow different results temporarily and make them wait until the expected annotations are there, or a timeout is hit.

[mickaelistria]

https://github.com/eclipse-platform/eclipse.platform.text/pull/151 will hopefully allow to verify this change. Once the test change is merged, we'll rebase here.

[mickaelistria]

@ghentschke Can you please try a manual rebase and a push force to your branch ?

[mickaelistria]

Eclipse Platform, like many other projects, tries to avoid merge commits (which are perceived as a pollution when reading history). Can you please try to reorganize your commits so it's only 1 commit rebased on top of current master?

[mickaelistria]

I'm a bit worried of merging it now that we're in RC cycles, as this is a critical part of the editor framework and we won't have much time to detect and fix regressions if need be. @ghentschke is it OK to wait a couple more weeks and merge it for 4.28 when master is open to it?

[ghentschke]

It's okay.

[mickaelistria]

Thanks. Don't hesitate to ping later if this seems forgotten!

[mickaelistria]

Can you please add a commit that bumps this bundle version by +0.0.100 ?

[mickaelistria]

Thank you!

[jukzi]

this added regression see https://github.com/eclipse-platform/eclipse.platform.text/issues/165 getProjectionAnnotationModel() can return null. maybe add a nullcheck in org.eclipse.ui.internal.genericeditor.folding.IndentFoldingStrategy.projectionEnabled() ?