Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trigger live validation in async fashion #1408

Open
tortmayr opened this issue Oct 7, 2024 · 4 comments · May be fixed by eclipse-glsp/glsp-server-node#99
Open

Trigger live validation in async fashion #1408

tortmayr opened this issue Oct 7, 2024 · 4 comments · May be fixed by eclipse-glsp/glsp-server-node#99
Assignees
Labels
enhancement New feature or request

Comments

@tortmayr
Copy link
Contributor

tortmayr commented Oct 7, 2024

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.

@tortmayr tortmayr added the enhancement New feature or request label Oct 7, 2024
@ivy-lli
Copy link

ivy-lli commented Oct 7, 2024

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
Copy link
Contributor Author

tortmayr commented Oct 7, 2024

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
Copy link
Contributor

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
Copy link

ivy-lli commented Oct 7, 2024

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants