eclipse-glsp / glsp

Graphical language server platform for building web-based diagram editors
https://www.eclipse.dev/glsp
Other
198 stars 32 forks source link

Trigger live validation in async fashion #1408

Open tortmayr opened 1 week ago

tortmayr commented 1 week ago

Currently live validation is executed as part of the ModelSubmissionHandler.submitModelDirectly method. If a validator is present, we invoke validation and send the resulting SetMarkersAction in combination with the SetModelAction/UpdateModelAction. This sync approach means that we potentially significantly delay the entire model update process if the live validation is expensive.

A better approach would be to send a RequestMarkersAction instead of directly triggering the validation. This would mean that the model update is processed directly and validation is triggered in async fashion.

In addition, we probably should send a progress action to indicate to the user that validation is still in progress.

ivy-lli commented 1 week ago

Thanks for creating an issue for this.

In our codebase I solved it by send the validations in a CompletableFuture to the client, something like:

CompletableFuture.runAsync(() -> {
  dispatcher.dispatch(new SetMarkersAction(validate(), MarkersReason.LIVE));
});

Sadly the ModelSubmissionHandler is not easy customizable (or I didn't get how to do it), so I override the RequestModelActionHandler and the OperationActionHandler to trigger this async validation.

I'm not sure about the progress messages though. I think for the initial load this could make sense, but validations are also triggered on each operation and I think it wouldn't make much sense to show a progress message on each operation.

tortmayr commented 1 week ago

Sadly the ModelSubmissionHandler is not easy customizable (or I didn't get how to do it), so I override the RequestModelActionHandler and the OperationActionHandler to trigger this async validation.

Yeah, seems like the ModelSubmissionHandler is configured via annotation instead of an explicit bind-method in the DiagramModule. Nevertheless you can simply customize it like this:

   @Override
   protected void configureAdditionals() {
      bind(ModelSubmissionHandler.class).to(MyModelSubmissionHandler.class).in(Singleton.class);
   }
martin-fleck-at commented 1 week ago

I'm not sure about the progress messages though. I think for the initial load this could make sense, but validations are also triggered on each operation and I think it wouldn't make much sense to show a progress message on each operation.

I'm not entirely sure about that. Configuring Live Validation I would assume that the user is properly notified about the current validation state of the model as they do changes. If the live validation takes a very long time, we'd suggest to the user that the model is valid until the validation is complete which is simply untrue. That is why I'm more in favor of showing that the validation is in progress. What do you think?

ivy-lli commented 1 week ago

I think it always depends on how this validation message is presented to the user, how long it takes and what is validated.

E.g. if the validation is a toast (e.g. like the vscode messages), I think it is really distracting if for each operation (e.g. move an element) a message appears. But if the message will be shown as a little spinner (e.g. like the vscode status bar) it is maybe good to have this message.

I can only speak from our perspective, normally a validation should only take some ms. This is not great for blocking an initial model request, but so fast that it will show up more or less immediately. So this status message will not have much impact on a user.

But in the end, the integration can always control how the message is shown or if it is even filtered from the user. So it is fine for me, just wanted to add some thoughts here :)