Send notification to client when xml.catalogs contains an invalid path

[xorye]

For redhat-developer/vscode-xml#217

My plan is to check for invalid paths from XMLTextDocumentService, and send a notification to the client if an invalid path exists here:
https://github.com/eclipse/lemminx/blob/977a96757b1f9cd472b81394e4cfd2dd866867b5/org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/XMLTextDocumentService.java#L424-L429

The reason why I have this in XMLTextDocumentService is that it is easy detect incorrect paths and send notifications here, because there is easy access to XMLLanguageServer#languageClient.

The problem with this approach is that path validation happens twice, once in the code above and once here:
https://github.com/eclipse/lemminx/blob/84d268957060b53aee211bd75a34b4536e9dab1e/org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/extensions/contentmodel/uriresolver/XMLCatalogResolverExtension.java#L102-L131

Would this be ok?

Signed-off-by: David Kwon dakwon@redhat.com

[angelozerr]

Would this be ok?

I would like to avoid doing that because it's a catalog feature, so it should be managed in the catalog extension (see XMLCatalogPlugin which manages is able to return the proper catalog.xsd from urn:oasis:names:tc:entity:xmlns:xml:catalog namespace). I mean if there are some validation to do, it should be done in XMLCatalogPlugin#doSave(ISaveContext context)

[xorye]

I made an update to this PR with these changes:

1. create AbstractXMLNotification.java which manages sending the notification to the client
2. validating catalogs paths and sending the notification to the client from XMLCatalogPlugin.java

In order to do 2., I've implemented a way to access XMLLangaugeClientAPI from XMLExtensionsRegistry.

Please let me know if I'm (or not) on the right track with this PR.

What this PR is missing:

• Javadoc
• Tests
• Improved notification message

[xorye]

Current error message (should be improved):

[datho7561]

It might make more sense to name this class PathWarner, change cache to a Set, and store settingId as a attribute that is set in the constructor. Then you can make a different PathWarner for each setting you want to watch. I think this makes sense in this context, since the only time this class is being used, it is only tracking one setting.

[xorye]

I think it would make more sense to call this class AbstractXMLNotifier, since this generates a notification, but doesn't represent one itself.
It might make more sense to name this class PathWarner

I like this idea. This means LimitExceededWarnings should change to LimitExceededWarner too. I will change this soon.

change cache to a Set, and store settingId as a attribute that is set in the constructor.

I realized that for LimitExceededWarnings, the cache is being shared for all features: https://github.com/eclipse/lemminx/blob/c9fa0cc44faa55c80c5d06a5824039be948eec3c/org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/client/LimitExceededWarnings.java#L53

Right now, we only have one, so its not a problem. In my latest push for this PR, I separated the cache for each feature using a map (key is the feature, value is a Set), but perhaps its a bit messy.

If we want one feature = one instance, we must remove this method signature: https://github.com/eclipse/lemminx/blob/c9fa0cc44faa55c80c5d06a5824039be948eec3c/org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/client/LimitExceededWarnings.java#L53
and provide the feature in the constructor as you said.

I quickly made a branch (branched off from the branch for this current PR) that implements that change: https://github.com/xorye/lsp4xml/tree/217catalogs_path_cleaner_cache

[angelozerr]

@xorye I thing your design is very good now. The validation is processed in plugin + AbstractXMLNotification manages cache.

what is the problem with public void onResultLimitExceeded(String uri, LimitFeature feature) signature?

[xorye]

@angelozerr If we follow this suggestion:

It might make more sense to name this class PathWarner, change cache to a Set, and store settingId as a attribute that is set in the constructor.

Changing the cache type for PathWarnings would change the cache type for LimitExceededWarnings too. The cache would only store info for one feature, since the cache would be a Set<String> instead of Map<String, Set<String>>.

Therefore, it would make LimitExceededWarnings work only for one feature, which would change this method signature:
onResultLimitExceeded(String uri, LimitFeature feature)
to
onResultLimitExceeded(String uri)

So, there's no problem with onResultLimitExceeded(String uri, LimitFeature feature) right now in this PR, its just that if the suggestion is followed, it does not make sense to provide feature in onResultLimitExceeded(String uri, LimitFeature feature)

[xorye]

This PR has been updated with changes from #794.

Demo:

In the gif above, the first entry is an existing file. The others are not.

[fbricon]

You should use error messages

[xorye]

You should use error messages

Fixed:

I have added a MessageType parameter to the sendNotification method:
https://github.com/eclipse/lemminx/blob/97f920b9360505286d3fb493310fcb4e8ad6acc2/org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/services/IXMLNotificationService.java#L36

Everything has been squashed to one commit.

[angelozerr]

Great job @xorye !