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.

Sign up for GitHub

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

Formatting based on external formatter #80

Merged
merged 11 commits into from
Jun 8, 2024
Merged

Formatting based on external formatter #80

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e2d42bc
Formatting based on external formatter
cottand Jun 1, 2024
794f80a
satisfy linter
cottand Jun 1, 2024
b25a9eb
remove unused import
cottand Jun 1, 2024
8074155
address review: feed file contents async
cottand Jun 3, 2024
791f5b4
remove src/gen
cottand Jun 3, 2024
8d08642
address review
cottand Jun 3, 2024
0a67c4f
address review
cottand Jun 3, 2024
cb81940
rename stateful OS-dependent lang settings to NixExternalFormatterSet…
cottand Jun 3, 2024
b3c2898
remove unused bundle id in plugin.xml
cottand Jun 3, 2024
68d26f1
add formatting to changelog
cottand Jun 3, 2024
bf1c923
Some adjustments after review
JojOatXGME Jun 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 107 additions & 0 deletions src/main/java/org/nixos/idea/format/NixExternalFormatter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
package org.nixos.idea.format;

import com.intellij.execution.ExecutionException;
import com.intellij.execution.configurations.GeneralCommandLine;
import com.intellij.execution.process.CapturingProcessAdapter;
import com.intellij.execution.process.OSProcessHandler;
import com.intellij.execution.process.ProcessEvent;
import com.intellij.formatting.service.AsyncDocumentFormattingService;
import com.intellij.formatting.service.AsyncFormattingRequest;
import com.intellij.openapi.util.NlsSafe;
import com.intellij.psi.PsiFile;
import com.intellij.util.execution.ParametersListUtil;
import org.jetbrains.annotations.NonNls;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import org.nixos.idea.file.NixFile;
import org.nixos.idea.settings.NixLangSettings;

import java.io.IOException;
import java.io.OutputStream;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.util.EnumSet;
import java.util.List;
import java.util.Set;

public final class NixExternalFormatter extends AsyncDocumentFormattingService {

@Override
protected @NotNull String getNotificationGroupId() {
return "NixIDEA";
JojOatXGME marked this conversation as resolved.
Show resolved Hide resolved
JojOatXGME marked this conversation as resolved.
Show resolved Hide resolved
}

@Override
protected @NotNull @NlsSafe String getName() {
return "NixIDEA";
}

@Override
public @NotNull Set<Feature> getFeatures() {
return EnumSet.noneOf(Feature.class);
}

@Override
public boolean canFormat(@NotNull PsiFile psiFile) {
return psiFile instanceof NixFile;
}


@Override
protected @Nullable FormattingTask createFormattingTask(@NotNull AsyncFormattingRequest request) {
NixLangSettings nixSettings = NixLangSettings.getInstance();
System.out.println("started fmt task");
JojOatXGME marked this conversation as resolved.
Show resolved Hide resolved
if (!nixSettings.isFormatEnabled()) {
return null;
}

var ioFile = request.getIOFile();
if (ioFile == null) return null;

@NonNls
var command = nixSettings.getFormatCommand();
List<String> argv = ParametersListUtil.parse(command, false, true);

try {
var commandLine = new GeneralCommandLine(argv);

OSProcessHandler handler = new OSProcessHandler(commandLine.withCharset(StandardCharsets.UTF_8));
OutputStream processInput = handler.getProcessInput();
Files.copy(ioFile.toPath(), processInput);
JojOatXGME marked this conversation as resolved.
Show resolved Hide resolved
processInput.flush();
processInput.close();
return new FormattingTask() {
@Override
public void run() {
handler.addProcessListener(new CapturingProcessAdapter() {

@Override
public void processTerminated(@NotNull ProcessEvent event) {
int exitCode = event.getExitCode();
if (exitCode == 0) {
request.onTextReady(getOutput().getStdout());
} else {
request.onError("NixIDEA", getOutput().getStderr());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question (non-blocking): Should we maybe use “Nix External Formatter” for the title? (Same in catch block.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is clearer to use the plugin name as title, so it is clear who is 'to blame' for the error, but I do not have a strong opinion.

I can change it to Nix External Formatter if you prefer

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with both. I think both have their pros and cons. 😅

}
}
});
handler.startNotify();
}

@Override
public boolean cancel() {
handler.destroyProcess();
return true;
}

@Override
public boolean isRunUnderProgress() {
return true;
}
};
} catch (ExecutionException | IOException e) {
request.onError("NixIDEA", e.getMessage());
return null;
}
}
}
9 changes: 8 additions & 1 deletion src/main/java/org/nixos/idea/lsp/NixLspSettingsConfigurable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,15 @@

import javax.swing.JComponent;
import javax.swing.JPanel;
import java.util.List;

public class NixLspSettingsConfigurable implements SearchableConfigurable, Configurable.Beta {
private static final List<CommandSuggestionsPopup.Suggestion> BUILTIN_SUGGESTIONS = List.of(
CommandSuggestionsPopup.Suggestion.builtin("<html>Use <b>nil</b> from nixpkgs</html>",
"nix --extra-experimental-features \"nix-command flakes\" run nixpkgs#nil"),
CommandSuggestionsPopup.Suggestion.builtin("<html>Use <b>nixd</b> from nixpkgs</html>",
"nix --extra-experimental-features \"nix-command flakes\" run nixpkgs#nixd")
);

private @Nullable JBCheckBox myEnabled;
private @Nullable RawCommandLineEditor myCommand;
Expand All @@ -43,7 +50,7 @@ public class NixLspSettingsConfigurable implements SearchableConfigurable, Confi
myCommand.getEditorField().getEmptyText().setText("Command to start Language Server");
myCommand.getEditorField().getAccessibleContext().setAccessibleName("Command to start Language Server");
myCommand.getEditorField().setMargin(myEnabled.getMargin());
new CommandSuggestionsPopup(myCommand, NixLspSettings.getInstance().getCommandHistory()).install();
new CommandSuggestionsPopup(myCommand, NixLspSettings.getInstance().getCommandHistory(), BUILTIN_SUGGESTIONS).install();

return FormBuilder.createFormBuilder()
.addComponent(myEnabled)
Expand Down
32 changes: 16 additions & 16 deletions src/main/java/org/nixos/idea/lsp/ui/CommandSuggestionsPopup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,20 +37,19 @@
import java.util.stream.Stream;

public final class CommandSuggestionsPopup {
// Implementation partially inspired by TextCompletionField

private static final List<Suggestion> BUILDIN_SUGGESTIONS = List.of(
Suggestion.builtin("<html>Use <b>nil</b> from nixpkgs</html>",
"nix --extra-experimental-features \"nix-command flakes\" run nixpkgs#nil"),
Suggestion.builtin("<html>Use <b>nixd</b> from nixpkgs</html>",
"nix --extra-experimental-features \"nix-command flakes\" run nixpkgs#nixd")
);
// Implementation partially inspired by TextCompletionField

private final @NotNull ExpandableTextField myEditor;
private final @NotNull Collection<String> myHistory;
private @Nullable ListPopup myPopup;
private final @NotNull List<Suggestion> mySuggestions;

public CommandSuggestionsPopup(@NotNull RawCommandLineEditor commandLineEditor, @NotNull Collection<String> history) {
public CommandSuggestionsPopup(@NotNull RawCommandLineEditor commandLineEditor,
@NotNull Collection<String> history,
@NotNull List<Suggestion> suggestions
) {
mySuggestions = suggestions;
myEditor = commandLineEditor.getEditorField();
myHistory = history;
}
Expand All @@ -72,7 +71,7 @@ public void install() {

public void show() {
if (myPopup == null) {
myPopup = new MyListPopup();
myPopup = new MyListPopup(mySuggestions);
myPopup.showUnderneathOf(myEditor);
}
}
Expand Down Expand Up @@ -110,8 +109,8 @@ public void caretUpdate(CaretEvent e) {
}

private final class MyListPopup extends ListPopupImpl implements JBPopupListener {
private MyListPopup() {
super(null, new MyListPopupStep());
private MyListPopup(List<Suggestion> suggestions) {
super(null, new MyListPopupStep(suggestions));
JojOatXGME marked this conversation as resolved.
Show resolved Hide resolved
// Disable focus in popup, so that the text field stays in focus.
setRequestFocus(false);
// Prevent the popup from overriding the paste-action.
Expand All @@ -127,7 +126,8 @@ protected void process(KeyEvent aEvent) {
switch (aEvent.getKeyCode()) {
// Do no handle left and right key,
// as it would prevent their usage in the text field while the popup is open.
case KeyEvent.VK_LEFT, KeyEvent.VK_RIGHT -> {}
case KeyEvent.VK_LEFT, KeyEvent.VK_RIGHT -> {
}
default -> super.process(aEvent);
}
}
Expand All @@ -138,13 +138,13 @@ public void onClosed(@NotNull LightweightWindowEvent event) {
}
}

private record Suggestion(
public record Suggestion(
@NotNull Icon icon,
@NotNull String primaryText,
@Nullable String secondaryText,
@NotNull String command
) {
static @NotNull Suggestion builtin(@NotNull String name, @NotNull String command) {
public static @NotNull Suggestion builtin(@NotNull String name, @NotNull String command) {
return new Suggestion(AllIcons.Actions.Lightning, name, command, command);
}

Expand All @@ -161,9 +161,9 @@ public String toString() {

private final class MyListPopupStep extends BaseListPopupStep<Suggestion> implements ListPopupStepEx<Suggestion> {

public MyListPopupStep() {
public MyListPopupStep(List<Suggestion> suggestions) {
super(null, Stream.concat(
BUILDIN_SUGGESTIONS.stream(),
suggestions.stream(),
myHistory.stream().map(Suggestion::history)
).toList());
}
Expand Down
78 changes: 78 additions & 0 deletions src/main/java/org/nixos/idea/settings/NixLangSettings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
package org.nixos.idea.settings;

import com.intellij.openapi.application.ApplicationManager;
import com.intellij.openapi.components.PersistentStateComponent;
import com.intellij.openapi.components.RoamingType;
import com.intellij.openapi.components.State;
import com.intellij.openapi.components.Storage;
import org.jetbrains.annotations.NotNull;

import java.util.ArrayDeque;
import java.util.Collection;
import java.util.Collections;
import java.util.Deque;

@State(name = "NixLangSettings", storages = @Storage(value = "nix-idea-tools.xml", roamingType = RoamingType.DISABLED))
public final class NixLangSettings implements PersistentStateComponent<NixLangSettings.State> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

polish: I would prefer if you keep this state class more specific for now. Like NixExternalFormatterSettings. Here are my two reasons:

  1. I don't want to rule out that we may add a simple built-in formatter to the plugin at some point.
  2. nix-idea-tools.xml is intended to contain configurations related to external dependencies, like installed applications. There may be other configurations in the future, which should not go into this file.

Copy link
Contributor Author

@cottand cottand Jun 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for reason (1), if you add an non-external formatter, I think it would appropriate to add its settings to this class also (rather than the existing settings in the "Build" section)

for reason (2), if you prefer a different file to nix-idea-tools.xml, we can change that. I am not very familiar with how the @State and @Storage abstraction works, I just wanted a settings screen for this plugin under the "Languages" section.

New settings can go underneath like in the picture below:

Screenshot 2024-06-03 at 21 18 54

I renamed 'Formatter Configuration' to ' External Formatter Configuration'

If you still feel like the external formatter deserves its own section, I am happy to make another one too though! Again, no strong opinions from me on this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern was mostly about where and how to store the configuration, not the UI. The configuration of a built-in formatter would be system-independent and can be synchronized across operating systems. External formatters should not be synchronized across operating systems. To make this separation, these two cases need to be stored in separate XML files.

The nix-idea-tools.xml is supposed to become the file storing these system-dependent configurations. This means your initial choice of that XML file was actually correct. Since this XML file will not contain other language-related configurations, I just don't want to call it NixLangSettings.

<application>
  <component name="NixLspSettings">
    <!-- LSP settings -->
  </component>
  <component name="NixExternalFormatterSettings">
    <!-- External formatter settings -->
  </component>
</application>

instead of

<application>
  <component name="NixLspSettings">
    <!-- LSP settings -->
  </component>
  <component name="NixLangSettings">
    <!-- Other system-dependent settings -->
  </component>
</application>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thanks for the explanation! That makes sense, I will apply your suggestion then.


// TODO: Use RoamingType.LOCAL with 2024.1

// Documentation:
// https://plugins.jetbrains.com/docs/intellij/persisting-state-of-components.html

private static final int MAX_HISTORY_SIZE = 5;

private @NotNull State myState = new State();

public static @NotNull NixLangSettings getInstance() {
return ApplicationManager.getApplication().getService(NixLangSettings.class);
}

public boolean isFormatEnabled() {
return myState.formatEnabled;
}

public void setFormatEnabled(boolean enabled) {
myState.formatEnabled = enabled;
}

public @NotNull String getFormatCommand() {
return myState.formatCommand;
}

public void setFormatCommand(@NotNull String command) {
myState.formatCommand = command;
addFormatCommandToHistory(command);
}

public @NotNull Collection<String> getCommandHistory() {
return Collections.unmodifiableCollection(myState.formatCommandHistory);
}

private void addFormatCommandToHistory(@NotNull String command) {
Deque<String> history = myState.formatCommandHistory;
history.remove(command);
history.addFirst(command);
while (history.size() > MAX_HISTORY_SIZE) {
history.removeLast();
}
}

@SuppressWarnings("ClassEscapesDefinedScope")
@Override
public void loadState(@NotNull State state) {
myState = state;
}

@SuppressWarnings("ClassEscapesDefinedScope")
@Override
public @NotNull State getState() {
return myState;
}

static final class State {
public boolean formatEnabled = false;
public @NotNull String formatCommand = "";
public Deque<String> formatCommandHistory = new ArrayDeque<>();
}
}
Loading