use formatDocument as fallback for willSaveWaitUntil

[ghentschke]

if server does not support willSaveWaitUntil the code can be formatted prior to save
OSGi services are used to obtain the formatting enable and the regions to be formatted prior to buffer save.

fixes #761

[ghentschke]

The advantage of this proposal is, that the client can handle the operations which should be done prior to a document save. In this case formatting. The format enable and the range of the formatting can be modified on document level.

[ghentschke]

@rubenporras and @mickaelistria WDYT?

[ghentschke]

Hi @mickaelistria @rubenporras, could you please give me a feedback, if this proposal is acceptable? I would be happy to add this feature in the cdt-lsp release for Eclipse 23-09.

[mickaelistria]

The advantage of this proposal is, that the client can handle the operations which should be done prior to a document save.

So shouldn't it be called something more generic like PreSaveDocumentChanges ? Do you have other consumers for this extension at the moment?

obtain the formatting enable and the regions to be formatted prior to buffer save.

What are the possible values you're interested in "whole document", "modified lines"...? Can't this be made generic in LSP4E with some preference? (and CDT would propagate its own preference to LSP4E)

I would be happy to add this feature in the cdt-lsp release for Eclipse 23-09.

I would be happy as well, but I'm afraid it may be a bit short in time. Let's do our best and try anyway.

[ghentschke]

The advantage of this proposal is, that the client can handle the operations which should be done prior to a document save.

So shouldn't it be called something more generic like PreSaveDocumentChanges ?

A more generic name was my first intention as well, but currently it supports only formatting. When there is more stuff to do in the future, I think it's better to add new interfaces/services and do not mix them with the IFormatOnSave

Do you have other consumers for this extension at the moment?

Do you mean for the IFormatOnSave OSGi service? No.

obtain the formatting enable and the regions to be formatted prior to buffer save.

What are the possible values you're interested in "whole document", "modified lines"...?

Yes, that would be the preferences we want to start with.

Can't this be made generic in LSP4E with some preference? (and CDT would propagate its own preference to LSP4E)

It could. I would like to do this in a next step. But I think these preferences are located best under the editor. Since it controls the editor behavior. LSP4E is part of the implementation.

I would be happy to add this feature in the cdt-lsp release for Eclipse 23-09.

I would be happy as well, but I'm afraid it may be a bit short in time. Let's do our best and try anyway.

Thank you!

[mickaelistria]

Do you mean for the IFormatOnSave OSGi service? No.

So I don't get why using a service pattern if there is only 1 implementation, that is built in with the only consumer?

[ghentschke]

Do you mean for the IFormatOnSave OSGi service? No.

So I don't get why using a service pattern if there is only 1 implementation, that is built in with the only consumer?

Sorry. Maybe I don't understand which consumer do you mean? LSP4E ist the consumer, right? The idea is, that cdt-lsp provides this service to LSP4E.
Maybe we can get rid of org.eclipse.lsp4e.DefaultFormatOnSave since its only purpose is to provide a service, when there is no bundle that can provide this service, so that org.eclipse.lsp4e.DocumentContentSynchronizer.formatOnSave won't be null.
But I'am already checking for null in org.eclipse.lsp4e.DocumentContentSynchronizer.documentAboutToBeSaved():

if (formatOnSave != null) {
  formatOnSave.call(f -> formatDocument(f.isEnabledFor(document), f.getFormattingRegions(LSPEclipseUtils.toBuffer(document))));
}

[ghentschke]

Sorry my mistake, formatOnSave can never be null in the org.eclipse.lsp4e.DocumentContentSynchronizer. When the ServiceCaller cannot find any service, it does not call formatDocument at all.

[mickaelistria]

that cdt-lsp provides this service to LSP4E.

Ah OK, so there are 2 implementations: the default one built-in LSP4E, and another one provided by CDT.
If you only want to start with "whole document" and "modified lines", those do not require CDT. So why would CDT take care of providing the service implementation while nothing depends on CDT?
I understand the need for CDT to control it per-editor or per-document; but I don't think CDT should provide the implementation, it should "just" tell LSP4E which one of the common strategies to use. Or maybe at least have LSP4E containing the implementations and the service being "formatOnSaveStrategyProvider" which would just return new org.eclipse.lsp4e.format.ModifiedLineStrategy() or new org.eclipse.lsp4e.formal.WholeDocumentStrategy() or a new org.eclipse.lsp4e.format.NoFormatStrategy().

FYI, this does also relate to a bigger topic of per-editor preferences which has several issues open about.

[ghentschke]

So why would CDT take care of providing the service implementation while nothing depends on CDT?

Good point. I can add a LSP4E Preference setting to enable this feature.

but I don't think CDT should provide the implementation, it should "just" tell LSP4E which one of the common strategies to use.

I understand. The idea behind public IRegion[] getFormattingRegions(ITextFileBuffer buffer) was, to be more flexible, when there should be added more formatting options/strategies (e.g. lines modified vs head revision, ...). The advantage is, that the interface would be more generic and stable and we can add more formatting options without changing LSP4E.

FYI, this does also relate to a bigger topic of per-editor preferences which has several issues open about.

Thank you. I'll have a look on that.

[ghentschke]

Maybe we can put the format-on-save LSP4E preferences topic in a new issue?

[ghentschke]

Or maybe at least have LSP4E containing the implementations and the service being "formatOnSaveStrategyProvider" which would just return new org.eclipse.lsp4e.format.ModifiedLineStrategy() or new org.eclipse.lsp4e.formal.WholeDocumentStrategy() or a new org.eclipse.lsp4e.format.NoFormatStrategy().

Could this be done in a next step in a separate interface/service like IFormatStrategies?

[mickaelistria]

Maybe we can put the format-on-save LSP4E preferences topic in a new issue?

Sure, it's a hard one I believe which would also require change to Eclipse Text Framework. Not something we can fix soon.

[ghentschke]

I removed the default implementation of IFormatOnSave from LSP4E since it makes only sense if it can be enabled in the LSP4E preferences in a following iteration.

[mickaelistria]

Could this be done in a next step in a separate interface/service like IFormatStrategies?

Sure we can. What I want to avoid now is that consumers start to believe this OSGi service is safe for public usage. Discussions here has revealed that this is not necessarily the best approach to resolve the underlying problem so it's not one we will want to maintain; we will want to drastically change it later. We need a way to prevent people from building on top of "unstable" content.
I believe the refactoring to add the formatting implementations in LSP4E and make the service just return the most appropriate for a document is much cleaner now and shouldn't be too hard. So if it can be done as part of this patch, it would be great.
If it cannot and you can put all the necessary flags to not make the interface public, we can do with it as a 1st iteration; but that wouldn't be my favorite choice (but I'm not the one doing the work, so it's easy to tell so :P)

[ghentschke]

So if it can be done as part of this patch, it would be great.

I'll try to add a commit for this.

[rubenporras]

Hi @mickaelistria @rubenporras, could you please give me a feedback, if this proposal is acceptable? I would be happy to add this feature in the cdt-lsp release for Eclipse 23-09.

I do not have currently time to continue looking at this, so whatever you and Mickael agree should be fine.

[ghentschke]

I added a FormatRegionsProvider. This class provides the regions to be formatted in a document depending on a FormatStrategy. Latter can be delivered by external bundles like CDT via OSGi service. Therefor I changed the IFormatOnSave interface:

public interface IFormatOnSave {

	/**
	 * Returns the {@link FormatStrategy} for the given document.
	 * @param document
	 * @return FormatStrategy
	 */
	FormatStrategy getFormatStrategy(IDocument document);

}

public enum FormatStrategy {
	/**
	 * No formatting. Disables the formatting for the given document.
	 */
	NO_FORMAT,
	/**
	 * Format all lines
	 */
	ALL_LINES,
	/**
	 * Format edited lines
	 */
	EDITED_LINES;
}

@mickaelistria Is this what you had in mind as you mentioned this?:

I believe the refactoring to add the formatting implementations in LSP4E and make the service just return the most appropriate for a document is much cleaner now and shouldn't be too hard.

[ghentschke]

I implemented the FormatRegionsProvider as a OSGi service as well. This makes it easier to implement other region providers in the future.

[ghentschke]

Still not happy with the enum FormatStrategy. It's an anemic object.

[mickaelistria]

See inline comments for a proposal to get rid of the enum: return 1 implementation instead of a enum value.

[mickaelistria]

This could be 3 different service implementation: NoFormatRegionProvider, AllLinesRegionsProvider, ModifiedLinesRegionProvider

[mickaelistria]

If you split 3 different implementation from FormatRegionsProvider, then you can decide to return 1 of this implementation directly.

[ghentschke]

Good point. But I am little unsure how to handle this public interface, which a foreign bundle has to implement:

public interface IFormatOnSave {

	/**
	 * Returns the {@link FormatStrategy} for the given document.
	 * @param document
	 * @return FormatStrategy
	 */
	FormatStrategy getFormatStrategy(IDocument document);

}

What should be the retuern type of getFormatStrategy when the foreign bundle should not implement IFormatRegionsProvider?

[ghentschke]

I think I get your point here.

[ghentschke]

Maybe this is more a general text editor feature after all...

Yes, its an editor feature which relies on the corresponding LS. The link to the appropriate LS should be done via the isEnbaledFor method. That's why I want to add this method to the IFormatRegionsProvider interface in this PR.

I'll commit a new PR to choose the right IFormatRegionsProvider service for the given document.

[mickaelistria]

The link to the appropriate LS should be done via the isEnbaledFor method

The link to the LS is already known of LSP4E, but what we miss here is the link between the service implementation and the language server that contributes it. If we make it a field of the extension point, then we have this link and don't need to add isEnabledFor to resolve to the proper LS.

[ghentschke]

then we have this link and don't need to add isEnabledFor to resolve to the proper LS.

That's true, but then we have fixed coupling to the bundle which implements the extension point. It could be that the format-on-save handling is implemented in the ui or editor bundle. With this OSGI service we are more flexible where to implement it.

[mickaelistria]

ok. so would you like it to be merged in the current state?

[ghentschke]

How can we know that CDT one is the one to use over Wild Web Developer one for .c files and the other way round for .js files?

@mickaelistria : This should be fixed with my last commit. If its okay, you can merge it :-)

[ghentschke]

Are there any open topics here?

[mickaelistria]

Inheritance quickly becomes a burden. I would instead suggest here

public interface IFormatRegionsStrategyProvider {
    /**
     * @return an IFormatRegionsComputer suitable for this document,
     * or <code>null</code> if the current provider cannot equip the document
     * for formatting regions.
     */
    public IFormatRegionsComputer getFormatRegionsComputer(Document document);
}

and FormatAllLins and FormatEditedLines would implement IFormatRegionsComputer.

So consumers could just implement eg

public class FormatEditedLinesOnSaveForMyJSXFiles implements IFormatRegionsStrategyProvider {
   public IFormatRegionsComputer getFormatRegionsComputer(Document document) {
     if (!isJSXFile(document)) {
       return null;
     }
     return new FormatEditedLines();
  }
}

[ghentschke]

The proposal above will lead to this call in DocumentContentSynchronizer:

return formatRegionsStrategyProvider.getFormatRegionsComputer(document).getFormattingRegions(document);

This will fail if getFormatRegionsComputer(document) returns null, So a check for null must be applied.

The only reason for:

public interface IFormatRegionsProvider extends IFormatRegions 

is to distinguish between the provider (IFormatRegionsProvider) and the computer (IFormatRegions). The provider should be implemented as OSGI service . It contains the computer implementation (IFormatRegions). In that case, the call is even simpler:

return formatRegionsProvider.getFormattingRegions(document);

[mickaelistria]

This gives the impression that this particular provider can cause the document to not be formatted at all by returning null even if some other suitable IFormatRegions for this document can contribute ranges to format.

[ghentschke]

even if some other suitable IFormatRegions for this document can contribute ranges to format.

The idea is, that null shall be returned if no formatting is enabled for the document/project

[mickaelistria]

Because of the contributive nature of Eclipse Platform and OSGi service, 1 provider returning null doesn't mean that there is no provider that can apply. This is related to my former questions about the case of multiple extensions to handle different LS or documents: when this happens, we need something to pick the right one and use it; the right one may depend on various things.

[ghentschke]

1 provider returning null doesn't mean that there is no provider that can apply.

There should be a single IFormatRegionsProvider provider for a LS definition. The filtering is done by the serverDefinitionId property in the OSGI component definition. This is my IFormatRegionsProvider implementation for the CDT LS:

@Component(property = { "serverDefinitionId:String=org.eclipse.cdt.lsp.server" })
public class FormatOnSave implements IFormatRegionsProvider {
	private final FormatEditedLines formatEditedLines = new FormatEditedLines();

	@Reference
	private EditorConfiguration configuration;

	@Override
	public IRegion[] getFormattingRegions(IDocument document) {
		var file = LSPEclipseUtils.getFile(document);
		if (file != null) {
			var editorOptions = configuration.options(file);
			if (editorOptions != null && editorOptions.formatOnSave()) {
				if (editorOptions.formatAllLines()) {
					return new IRegion[] { new Region(0, document.getLength()) };
				}
				if (editorOptions.formatEditedLines()) {
					return formatEditedLines.getFormattingRegions(document);
				}
			}
		}
		return null;
	}

}

This is related to my former questions about the case of multiple extensions to handle different LS or documents: when this happens, we need something to pick the right one and use it; the right one may depend on various things.

Please have a look at org.eclipse.lsp4e.DocumentContentSynchronizer.getFormatRegions() . There we pick up the IFormatRegionsProvider based on the languageServerWrapper.serverDefinition.id as filter:
var serviceReferences = bundleContext.getAllServiceReferences(IFormatRegionsProvider.class.getName(), serverId);

[ghentschke]

Currently we pick up the first one, if there are several provider for the same LS id:
reference = serviceReferences[0];

the right one may depend on various things.

If there is a use case for more than one IFormatRegionsProvider for a language server definition (e.g. for each LS instance) then the filter could be modified.

[ghentschke]

Maybe IFormatRegionsProvider could implement a method which returns the serverID for filtering purposes. IMO the filtering via OSGi component property is more charming, since its a one-liner:
var serviceReferences = bundleContext.getAllServiceReferences(IFormatRegionsProvider.class.getName(), serverId)

I think it provides also more flexibility in the future. A new filter property can be easily added to the component definition without breaking the API.

[mickaelistria]

OK, I missed the service property to filter on the LS id.
So now that this is here, I see less need to sepaate the IFormatRegionsProvider and the IFormatRegions. Maybe those 2 standard strategies could just be implemented as public static methods in IFormatRegionsProvider so we can get rid of IFormatRegions while still allowing clients to easily use one or the other of those strategies?

[ghentschke]

Maybe those 2 standard strategies could just be implemented as public static methods in IFormatRegionsProvider so we can get rid of IFormatRegions while still allowing clients to easily use one or the other of those strategies?

Or as default implementations instead of public static ?

[mickaelistria]

default means that consumers are welcome and somehow encourage to override them. Here we do not want consumers to override how to compute whole document or only modified lines as they are standard behavior; so default is not the right thing IMO.

[mickaelistria]

I like this latest version a lot. @ghentschke Is it OK with you if we merge this one ?

[ghentschke]

Yes!

[mickaelistria]

Thank you!