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

[editor] Allow formatting only the selection #10204

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

[editor] Allow formatting only the selection #10204

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2ba0124
[editor] Allow formatting only the selection
ricardojlrufino May 14, 2020
7efb118
fix code formatting
ricardojlrufino May 14, 2020
cf81fdc
format selection using AStyle control tags
ricardojlrufino May 20, 2020
0918a5b
Avoid erros on format multi-line comments, add Tests
ricardojlrufino May 21, 2020
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
71 changes: 62 additions & 9 deletions app/src/cc/arduino/packages/formatter/AStyle.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,16 +29,20 @@

package cc.arduino.packages.formatter;

import static processing.app.I18n.tr;

import processing.app.Base;
import processing.app.BaseNoGui;
import processing.app.Editor;
import processing.app.helpers.FileUtils;
import processing.app.syntax.SketchTextArea;
import processing.app.tools.Tool;

import java.io.File;
import java.io.IOException;

import static processing.app.I18n.tr;
import java.util.regex.Pattern;
import javax.swing.text.BadLocationException;
import org.fife.ui.rsyntaxtextarea.RSyntaxDocument;

public class AStyle implements Tool {

Expand Down Expand Up @@ -76,15 +80,64 @@ public void init(Editor editor) {

@Override
public void run() {
String originalText = editor.getCurrentTab().getText();
String formattedText = aStyleInterface.AStyleMain(originalText, formatterConfiguration);

if (formattedText.equals(originalText)) {
editor.statusNotice(tr("No changes necessary for Auto Format."));
return;
}
SketchTextArea textArea = editor.getCurrentTab().getTextArea();

String originalText = textArea.getSelectedText();

// If no selection use all file
if (originalText == null || originalText.isEmpty()) {

String formattedText = aStyleInterface.AStyleMain(textArea.getText(), formatterConfiguration);
editor.getCurrentTab().setText(formattedText);

} else {
try {

// apply indentation control keywords.
String FORMAT_ON = "\n// *INDENT-ON* DYN\n";
String FORMAT_OFF = "\n// *INDENT-OFF* DYN\n";

editor.getCurrentTab().setText(formattedText);
RSyntaxDocument content = (RSyntaxDocument) textArea.getDocument();

textArea.beginAtomicEdit();

int selStart = editor.getCurrentTab().getSelectionStart();
int selEnd = editor.getCurrentTab().getSelectionStop();
int lineStart = textArea.getLineOfOffset(selStart);
int lineEnd = textArea.getLineOfOffset(selEnd);

// Calculate offsets from begin and end of each line.
int fristLineOffset = textArea.getLineStartOffset(lineStart);
int lastLineOffset = textArea.getLineEndOffset(lineEnd);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this line offset stuff also work correctly when the selection boundaries are just before or just after the newline? Probably something to add a test for all four cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this the example where I need to format 1 line only, and the cursor is at the beginning of the next line? It will force formatting on the 2 lines

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think so, yes. I think I would expect that if I select no characters on the second line, just the newline separating both, I would only format the first line, not the second?

For the start of the selection, it's probably mirrored: If I select just the preceding newline and no characters on the previous line, I would expect to not format the previous line.


// inserts change the length, use this to calculate new positios.
int offLength = FORMAT_OFF.length();
int onLength = FORMAT_ON.length();

content.insertString(0, FORMAT_OFF, null);
content.insertString(fristLineOffset + offLength, FORMAT_ON, null);
content.insertString(lastLineOffset + offLength + onLength, FORMAT_OFF,null);
originalText = content.getText(0, content.getLength());

String formattedText = aStyleInterface.AStyleMain(originalText, formatterConfiguration);

// Remove format tags
formattedText = formattedText.replaceAll(Pattern.quote(FORMAT_OFF), "");
formattedText = formattedText.replaceAll(Pattern.quote(FORMAT_ON), "");
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems too general: If the code already contained these format off tags, they would now be removed. I would suggest to remember the offset where the tag was inserted (offset from the end for the third tag), and remove it only from there. As a sanity check, you could check that astyle has indeed left all text up to and including the second tag (and from the third tag to the end) indeed unmodified, otherwise your offsets would not be valid anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem, i put keyword 'DYN' to allow this and not conflict with pre-existing format tags

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I see. I would still think this is somewhat fragile (a user might also put "DYN" in there), but I guess it works well enough in practice. On advantage of doing some offset calculations is that you can at the same time verify that nothing outside of the selection was changed.


textArea.setText(formattedText);
textArea.select(selStart, selStart);
Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAICS, this clears the selection and puts the cursor at the start? Might be good to keep the selection (requires recalculating the end of the selection, but that is probably easier if you solve my previous comment too).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will place the cursor at the beginning of the SELECTION. I would have to recalculate the positioning of the new format, to keep things simple and straightforward I decided not to do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No negative point for usability


} catch (BadLocationException e) {
editor.statusNotice(tr("Auto Format Error") + ": " + e.getLocalizedMessage());
e.printStackTrace();
return;
} finally {
textArea.endAtomicEdit();
}

}

// mark as finished
editor.statusNotice(tr("Auto Format finished."));
Expand Down