feat: Validate preferences in lsp console #222

MituuZ · 2024-04-17T04:28:32Z

Set up common validation style for LSP console and new LS dialog for text fields and text areas.

Use similar validation for LSP console and new LS dialog. Prevent saving/creating if validations fail.

MituuZ · 2024-04-17T04:29:23Z

Fixes #210

Basic working implementation, but still needs some cleanup and docs

codecov · 2024-04-17T04:32:18Z

Codecov Report

Attention: Patch coverage is 0% with 63 lines in your changes are missing coverage. Please review.

Project coverage is 18.03%. Comparing base (69e8ef5) to head (80a1f95).

Files	Patch %	Lines
...vtools/lsp4ij/settings/ui/LanguageServerPanel.java	0.00%	22 Missing ⚠️
...devtools/lsp4ij/settings/ui/CommandLineWidget.java	0.00%	13 Missing ⚠️
.../devtools/lsp4ij/settings/ui/ServerNameWidget.java	0.00%	13 Missing ⚠️
...devtools/lsp4ij/settings/ui/ValidatableDialog.java	0.00%	5 Missing ⚠️
...t/devtools/lsp4ij/settings/LanguageServerView.java	0.00%	4 Missing ⚠️
...s/lsp4ij/settings/ui/ValidatableConsoleWidget.java	0.00%	4 Missing ⚠️
...s/lsp4ij/launching/ui/NewLanguageServerDialog.java	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #222      +/-   ##
==========================================
- Coverage   18.47%   18.03%   -0.44%     
==========================================
  Files         235      238       +3     
  Lines        8260     8461     +201     
  Branches     1561     1610      +49     
==========================================
  Hits         1526     1526              
- Misses       6451     6652     +201     
  Partials      283      283

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/main/java/com/redhat/devtools/lsp4ij/console/actions/ApplyLanguageServerSettingsAction.java

src/main/java/com/redhat/devtools/lsp4ij/settings/ui/ServerNameWidget.java

src/main/java/com/redhat/devtools/lsp4ij/settings/ui/ValidatableConsoleWidget.java

angelozerr · 2024-04-19T16:10:09Z

@MituuZ it should be nice to see all errors when you open dialog or change a field value.

As New dialog, preferences and LSP console detail uses the same componet LanguageServerPanel, I think you should delegate validation in languageServerPanel.doValidateAll()

I have tried to start quickly the idea https://github.com/redhat-developer/lsp4ij/compare/main...angelozerr:lsp4ij:poc_error?expand=1 so I think we should work just with ValidationInfo and not have an isValid method.

MituuZ · 2024-04-20T12:26:45Z

I'll take a look and re-implement the validation according to that

angelozerr · 2024-04-20T14:02:24Z

I did that quickly so dont hesitate to my code.

It should be very nice to see all errors or when you fix an error (ex server name) it should be nice to see the error in command.

As we use tabs, I wonder if we could se et the tab in red which contains error.

src/main/java/com/redhat/devtools/lsp4ij/settings/ui/ValidatableConsoleWidget.java

src/main/java/com/redhat/devtools/lsp4ij/settings/ui/CommandLineWidget.java

src/main/java/com/redhat/devtools/lsp4ij/settings/ui/LanguageServerPanel.java

MituuZ · 2024-04-24T03:02:50Z

@angelozerr Should I continue with the tab validations for the LSP console on this or should that be a new issue?

angelozerr · 2024-04-24T04:26:11Z

Let is do that in an annoter pr. I wanted just to share with you an idea that I had had

src/main/java/com/redhat/devtools/lsp4ij/settings/ui/ValidatableDialog.java

angelozerr · 2024-04-24T06:51:53Z

src/main/java/com/redhat/devtools/lsp4ij/settings/ui/ValidatableConsoleWidget.java

+ * A shared interface meant to simplify creating validatable components used in
+ * NewLanguageServerDialog and LanguageServerView (LSP console)
+ */
+public interface ValidatableConsoleWidget {


@fbricon what do you think about the ValidatableConsoleWidget name?

Thanks @fbricon . I think it should be renamed with ValidatableWidget

angelozerr · 2024-04-24T06:56:59Z

src/main/java/com/redhat/devtools/lsp4ij/settings/ui/ValidatableConsoleWidget.java

+     * Set a common error border for the widget
+     * @param jComponent interface implementor (e.g. setErrorBorder(this);)
+     */
+    default void setErrorBorder(JComponent jComponent) {


setErrorBorder is more an utility class than an API.

Here some suggestion:

remove setErrorBorder method

add a new void updateUi(boolean hasError) method which is called after the call of validate (hasError must be computed by comparing the size of validations before/after the cal of validate.

add a new method initializeValidator(ValidateDialog) which adds the proper listener (which is done today in the addValidator)

angelozerr · 2024-04-24T06:58:01Z

src/main/java/com/redhat/devtools/lsp4ij/settings/ui/ServerNameWidget.java

+            widgetValidations.add(new ValidationInfo(errorMessage, this));
+        }
+
+        if (widgetValidations.isEmpty()) {


Don't do that here, move this code in the new updateUi(boolean hasErrors) method

angelozerr · 2024-04-24T06:59:40Z

src/main/java/com/redhat/devtools/lsp4ij/settings/ui/LanguageServerPanel.java

+    }
+
+    private void addValidator(JTextComponent textComponent) {
+        textComponent.getDocument().addDocumentListener(new DocumentAdapter() {


Don't do that here but in the ValidableConsoleWidget impl in the initializeValidator(ValidatableDialog dialog) method

I think addValidator should just add in a list CommandLineWidget, ServerNameWidget (list of ValidatableConsoleWidget) and in doValidateAll should just iterate from this list.

private void addValidatableWidget(@Nullable ValidatableConsoleWidget validatableWidget) { if (validatableWidget == null) { return; } validatableWidget.initializeValidator(this); this.validatableWidgets.add(validatableWidget) }

angelozerr · 2024-04-24T07:21:24Z

src/main/java/com/redhat/devtools/lsp4ij/settings/ui/LanguageServerPanel.java

@@ -241,4 +256,25 @@ public ComboBox<ErrorReportingKind> getErrorReportingKindCombo() {
        return errorReportingKindCombo;
    }

+    public @NotNull List<ValidationInfo> doValidateAll() {


This method should look like this:

List<ValidationInfo> validations = new ArrayList<>(); for(var widget : validatableWidgets) { int previousSize = validations.size(); widget.validate(validations); boolean hasErrors = previousSize ! validations.size(); widget.updateUi(hasErrors); } return validations;

angelozerr · 2024-04-24T07:40:22Z

@MituuZ I did some suggestion to improve API. If you have no time to work on it, let's me know and I will do that.

MituuZ · 2024-04-24T07:56:19Z

@angelozerr

I can keep working on this if you'd like, most likely tomorrow morning. Though I do think you have a very clear vision on how this should be finished.

It might easier for you to finish this, and I can continue on #215 , which I started earlier today.

angelozerr · 2024-04-24T08:13:12Z

It might easier for you to finish this

Ok I will take care of that.

angelozerr · 2024-05-01T07:55:48Z

@MituuZ I have tried to work on this PR. At first using DialogWrapper is an excellent idea, thanks for that.

The thing which is bad in this PR is that we manage border (because JBTextArea doesn't manage border).. By managing border we loose some features like set a light red color when the JTectField is on error and loose the focus.

In otherwise, we must not manage border. I have tried to manage border only for JBTextArea, it starts working but when teh textarea is on error, the red border hide the cursor when cursor is on left of the textarea.

I need to investigate more this PR.

MituuZ · 2024-05-01T08:19:00Z

Yeah, I feel you.

Didn't want to handle the border manually either, but it seemed like the only way to get both fields working the same way and having the same styling for both. Didn't find what the default error borders are for the JBTextField so we could have copied them either.

angelozerr · 2024-05-01T08:32:12Z

Here the awfull code that I'm using:

public CommandLineWidget() {
        super(5, 0);
        super.setLineWrap(true);
        super.setWrapStyleWord(true);
        super.setFont(new JBTextField().getFont());
        super.setBorder(new JBTextField().getBorder());
        super.setMargin(new JBTextField().getMargin());
    }

When I will have something which will work, I will push my work.

MituuZ · 2024-05-01T08:50:39Z

Checking this it suggests using red and light-red for highlight colors on validation errors. I believe I did try light red, but if recall correctly it was a different color than what the JBTextField had:

https://jetbrains.design/intellij/principles/validation_errors/#how-it-works-1

angelozerr · 2024-05-01T09:02:54Z

https://jetbrains.design/intellij/principles/validation_errors/#how-it-works-1

Great links, thanks.

I need to study more the topic.

openshift-ci bot added the do-not-merge/work-in-progress label Apr 17, 2024

angelozerr reviewed Apr 17, 2024

View reviewed changes

src/main/java/com/redhat/devtools/lsp4ij/console/actions/ApplyLanguageServerSettingsAction.java Outdated Show resolved Hide resolved

angelozerr reviewed Apr 17, 2024

View reviewed changes

src/main/java/com/redhat/devtools/lsp4ij/settings/ui/ServerNameWidget.java Show resolved Hide resolved

angelozerr reviewed Apr 17, 2024

View reviewed changes

src/main/java/com/redhat/devtools/lsp4ij/settings/ui/ServerNameWidget.java Show resolved Hide resolved

angelozerr reviewed Apr 17, 2024

View reviewed changes

src/main/java/com/redhat/devtools/lsp4ij/settings/ui/ValidatableConsoleWidget.java Show resolved Hide resolved

MituuZ marked this pull request as ready for review April 19, 2024 03:21

openshift-ci bot removed the do-not-merge/work-in-progress label Apr 19, 2024

MituuZ marked this pull request as draft April 21, 2024 17:53

openshift-ci bot added the do-not-merge/work-in-progress label Apr 21, 2024

MituuZ force-pushed the validate-preferences-in-lsp-console branch from f112d35 to 49ccd6c Compare April 21, 2024 17:54

angelozerr reviewed Apr 21, 2024

View reviewed changes

src/main/java/com/redhat/devtools/lsp4ij/settings/ui/ValidatableConsoleWidget.java Outdated Show resolved Hide resolved

MituuZ commented Apr 23, 2024

View reviewed changes

src/main/java/com/redhat/devtools/lsp4ij/settings/ui/CommandLineWidget.java Outdated Show resolved Hide resolved

MituuZ commented Apr 23, 2024

View reviewed changes

src/main/java/com/redhat/devtools/lsp4ij/settings/ui/LanguageServerPanel.java Show resolved Hide resolved

Mitja Leino and others added 12 commits April 24, 2024 05:53

Try to add validation to the widgets

be8a0fd

Initial setup with a shared error border setter

1527f02

Create a shared interface to simplify

a43d3ab

Add javadoc for ValidatableConsoleWidget

d76179e

Don't enable presentation if language server view is not valid

36990bf

Remove unnecessary methods

922296b

Add license information, some docs and streamline validation interface

734dad2

Clean up

b52251b

Fix indent

cd623f8

Add missing overrider

cf6779f

Add ValidatableDialog class

6c8f0c0

Simplify and unify validations

2de6a9f

Clean up and add missing docs

80a1f95

MituuZ force-pushed the validate-preferences-in-lsp-console branch from c4b2799 to 80a1f95 Compare April 24, 2024 02:58

MituuZ marked this pull request as ready for review April 24, 2024 03:02

openshift-ci bot removed the do-not-merge/work-in-progress label Apr 24, 2024

angelozerr reviewed Apr 24, 2024

View reviewed changes

src/main/java/com/redhat/devtools/lsp4ij/settings/ui/ValidatableDialog.java Show resolved Hide resolved

angelozerr reviewed Apr 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Validate preferences in lsp console #222

feat: Validate preferences in lsp console #222

MituuZ commented Apr 17, 2024

MituuZ commented Apr 17, 2024

codecov bot commented Apr 17, 2024 •

edited

Loading

angelozerr commented Apr 19, 2024

MituuZ commented Apr 20, 2024

angelozerr commented Apr 20, 2024

MituuZ commented Apr 24, 2024

angelozerr commented Apr 24, 2024

angelozerr Apr 24, 2024

fbricon Apr 24, 2024

angelozerr Apr 24, 2024

angelozerr Apr 24, 2024 •

edited by fbricon

Loading

angelozerr Apr 24, 2024

angelozerr Apr 24, 2024

angelozerr Apr 24, 2024

angelozerr Apr 24, 2024

angelozerr commented Apr 24, 2024

MituuZ commented Apr 24, 2024

angelozerr commented Apr 24, 2024

angelozerr commented May 1, 2024

MituuZ commented May 1, 2024

angelozerr commented May 1, 2024

MituuZ commented May 1, 2024

angelozerr commented May 1, 2024

feat: Validate preferences in lsp console #222

Are you sure you want to change the base?

feat: Validate preferences in lsp console #222

Conversation

MituuZ commented Apr 17, 2024

MituuZ commented Apr 17, 2024

codecov bot commented Apr 17, 2024 • edited Loading

Codecov Report

angelozerr commented Apr 19, 2024

MituuZ commented Apr 20, 2024

angelozerr commented Apr 20, 2024

MituuZ commented Apr 24, 2024

angelozerr commented Apr 24, 2024

angelozerr Apr 24, 2024

Choose a reason for hiding this comment

fbricon Apr 24, 2024

Choose a reason for hiding this comment

angelozerr Apr 24, 2024

Choose a reason for hiding this comment

angelozerr Apr 24, 2024 • edited by fbricon Loading

Choose a reason for hiding this comment

angelozerr Apr 24, 2024

Choose a reason for hiding this comment

angelozerr Apr 24, 2024

Choose a reason for hiding this comment

angelozerr Apr 24, 2024

Choose a reason for hiding this comment

angelozerr Apr 24, 2024

Choose a reason for hiding this comment

angelozerr commented Apr 24, 2024

MituuZ commented Apr 24, 2024

angelozerr commented Apr 24, 2024

angelozerr commented May 1, 2024

MituuZ commented May 1, 2024

angelozerr commented May 1, 2024

MituuZ commented May 1, 2024

angelozerr commented May 1, 2024

codecov bot commented Apr 17, 2024 •

edited

Loading

angelozerr Apr 24, 2024 •

edited by fbricon

Loading