-
Notifications
You must be signed in to change notification settings - Fork 27
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
feat: Language server config UX improvements #645
feat: Language server config UX improvements #645
Conversation
…lds are now JSON editor text fields. I also fixed some layout issues that were keeping those text fields from stretching vertically to fill the available space.
@@ -287,9 +287,7 @@ private JPanel createSettings(JComponent description, boolean launchingServerDef | |||
launchingServerDefinition ? LanguageServerPanel.EditionMode.EDIT_USER_DEFINED : | |||
LanguageServerPanel.EditionMode.EDIT_EXTENSION, project); | |||
this.mappingPanel = languageServerPanel.getMappingsPanel(); | |||
return builder | |||
.addComponentFillVertically(new JPanel(), 50) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why this was added originally, but it was resulting in a significant unusable "padding" in the bottom half (or so) of all tabbed editors. I don't see any negative side-effects of removing it, and it allows the two JSON text editors to expand vertically to fill the available space in the Configuration tab.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SCWells72 this panel is also used in the standard préférences of IJ with Language Servers entre.
If remember I did that to have a correct UI in this preference.
Please check that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -44,6 +44,7 @@ public AbstractServerMappingTablePanel(ColumnInfo<ServerMappingSettings, String> | |||
new ArrayList<>(), | |||
0); | |||
table = new TableView<>(model); | |||
table.setColumnSelectionAllowed(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provides a better experience (IMO) when selecting a specific cell for editing after adding a new row.
// Immediately select the first cell in the new row | ||
ApplicationManager.getApplication().invokeLater(() -> { | ||
int newRowIndex = table.getRowCount() - 1; | ||
table.getSelectionModel().setSelectionInterval(newRowIndex, newRowIndex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in the PR itself, this could instead just start editing the first cell in the new row. I tried that and felt like it was a little too aggressive, but I'm open to other opinions, of course.
table.getListTableModel().removeRow(table.getSelectedRow()); | ||
fireStateChanged(); | ||
} | ||
return anActionButton -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was just changing an anonymous inner class into a lambda.
|
||
public JsonTextField(@NotNull Project project) { | ||
// Create and initialize the editor text field | ||
EditorTextFieldProvider service = ApplicationManager.getApplication().getService(EditorTextFieldProvider.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried just having this be a LanguageTextField
, but it wasn't behaving properly at all. I remember having to "wrap" one created by EditorTextFieldProvider
in this manner in my own plugin to get the desired results, and it does so here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No skills about that. I trust you.
)); | ||
Language jsonLanguage = Language.findLanguageByID(JSON_LANGUAGE_NAME); | ||
if (jsonLanguage == null) { | ||
jsonLanguage = PlainTextFileType.INSTANCE.getLanguage(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should never happen, but being defensive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can happen if Jetbrains product (perhaps CLion?) have no JSON plugin. So it is very good to do that.
LSP4IJ should not have s tron dependency to JSON plugin. If you need to add JSON dependency (ex: when you will add JSON Shcme support) it should be done as optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I understand having discussed this in the intellij-platform
Slack channel with the JetBrains folks recently, it should be safe to assume that JSON is always available even though it's broken out into a separate plugin now. It's more a logical separation for JetBrains than a phyiscal one at runtime. They're even fixing an issue in the plugin verifier where it doesn't currently "find" the JSON plugin in 2024.3. So the check I've added is defensive, but it should never happen.
add(editorTextField, BorderLayout.CENTER); | ||
} | ||
|
||
// Proxy some simple accessors to the editor text field |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These provide the same API as the previous JBTextField
for existing callers.
super.setWrapStyleWord(true); | ||
super.setFont(JBFont.regular()); | ||
super.getEmptyText().setText(LanguageServerBundle.message("language.server.configuration.emptyText")); | ||
public class LanguageServerConfigurationWidget extends JsonTextField { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically these could be removed and JsonTextField
could be used directly. However, I was thinking that they might diverge slightly when we start adding support for JSON schema validation and such. Might make sense to keep them distinct until we know whether that still makes sense or not when other features are added.
@@ -79,7 +79,7 @@ public LanguageServerPanel(FormBuilder builder, JComponent description, EditionM | |||
|
|||
private void createUI(FormBuilder builder, JComponent description, EditionMode mode) { | |||
JBTabbedPane tabbedPane = new JBTabbedPane(); | |||
builder.addComponent(tabbedPane); | |||
builder.addComponentFillVertically(tabbedPane, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's important the the entire tabbed pane be allowed to stretch vertically to accommodate the contents of its tabs.
@@ -180,7 +180,7 @@ private static FormBuilder addTab(JBTabbedPane tabbedPane, String tabTitle, bool | |||
if (addToTop) { | |||
tabPanel.addToTop(builder.getPanel()); | |||
} else { | |||
tabPanel.add(builder.getPanel()); | |||
tabPanel.addToCenter(builder.getPanel()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually behaviorally the same, but it made sense to me to be explicit since the panel is definitely for a BorderLayout
.
Your PR looks amazing. Let me try it after the release and I will try to merge it asap in order to you can implement json schéma which will provide a fanatstic feature to customize language server. For css we could create a json schéma by extracting the json schema which declares the css settings. For typescript we will need to write that at hand. Please note that css settings use dot , perhaps we will need to split the configuration with dot before sending to the language server. |
…SON schema is determined.
gradle.properties
Outdated
@@ -13,7 +13,7 @@ platformType=IC | |||
platformVersion=2023.2 | |||
# Plugin Dependencies -> https://plugins.jetbrains.com/docs/intellij/plugin-dependencies.html | |||
# Example: platformPlugins = com.intellij.java, com.jetbrains.php:203.4449.22 | |||
platformPlugins=com.redhat.devtools.intellij.telemetry:1.2.1.62, textmate, properties | |||
platformPlugins=com.redhat.devtools.intellij.telemetry:1.2.1.62, textmate, properties, com.intellij.modules.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had to add a dependency on JSON, but it should be available in all JetBrains IDEs as I mentioned to you directly. I have the same type of dependency in my own commercial plugin, and I've already been reassured by JetBrains that it's fine in spite of their recent move to a distinct JSON plugin.
/** | ||
* JSON schema file provider for the TypeScript language server. | ||
*/ | ||
class TypeScriptLanguageServerConfigurationJsonSchemaFileProvider extends LSP4IJJsonSchemaFileProvider { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class should not exists. You should load schema by using the name settings.schema.json because the idea is to provide the capability to define a json schema with ui and export your language server to share it.
As it is working like a charm an code was very simple, I merge the PR. Thanks so much @SCWells72 ! |
This is not yet intended for merge but rather for early feedback. It currently includes the following changes:
Feel free to provide any feedback on these changes before I move into a next phase that would include: