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

[editor] Allow formatting only the selection #10204

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
103 changes: 94 additions & 9 deletions app/src/cc/arduino/packages/formatter/AStyle.java
Original file line number Diff line number Diff line change
Expand Up @@ -29,16 +29,21 @@

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;
import org.fife.ui.rsyntaxtextarea.Token;

public class AStyle implements Tool {

Expand Down Expand Up @@ -76,19 +81,99 @@ 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";

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.


// Avoid multi-line comments
fristLineOffset = navigateOffComments(textArea, fristLineOffset); // try caech (invalid selection)
lastLineOffset = navigateOffComments(textArea, lastLineOffset); // try caech (invalid selection)

// 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();
}

editor.getCurrentTab().setText(formattedText);
}

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

private int navigateOffComments(SketchTextArea textArea, int offset) throws BadLocationException {

Token token = textArea.modelToToken(offset);

// if line is a multiline comment, go back !!
if (token != null && token.getType() == Token.COMMENT_MULTILINE) {

int lineStart = textArea.getLineOfOffset(offset);
token = textArea.getTokenListForLine(lineStart);

while (token.getType() == Token.COMMENT_MULTILINE) {
if (lineStart == 0)
break;
token = token.getNextToken();
if (token == null)
token = textArea.getTokenListForLine(--lineStart);
}

return token.getOffset();

} else {
return offset;
}

}


@Override
public String getMenuTitle() {
Expand Down
174 changes: 174 additions & 0 deletions app/test/processing/app/AutoformatSelectionTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,174 @@
/*
* This file is part of Arduino.
*
* Copyright 2015 Arduino LLC (http://www.arduino.cc/)
*
* Arduino is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
*
* As a special exception, you may use this file as part of a free software
* library without restriction. Specifically, if other files instantiate
* templates or use macros or inline functions from this file, or you compile
* this file and link it with other files to produce an executable, this
* file does not by itself cause the resulting executable to be covered by
* the GNU General Public License. This exception does not however
* invalidate any other reasons why the executable file might be covered by
* the GNU General Public License.
*/

package processing.app;

import org.fest.swing.fixture.JMenuItemFixture;
import org.junit.Test;
import processing.app.helpers.SketchTextAreaFixture;

import static org.junit.Assert.assertEquals;

public class AutoformatSelectionTest extends AbstractGUITest {

private static String orig = "/**\n" +
" *Docs..\n" +
" */\n" +
"void loop() {\n" +
" // LED ON\n" +
"digitalWrite(LED_BUILTIN, HIGH);\n" +
"if (true) {\n" +
" delay( 1000)\n" +
"};\n" +
" digitalWrite(LED_BUILTIN, LOW);\n" +
" if (true) {delay( 1000 )}; // <<< UGLY FORMATTING\n" +
"}";


@Test
public void testSuite() {

selectIfMultilineCorrectSel();
selectIfMultilineCorrectPartialSel();
selectIfMultilineDocs();
selectIfSinglelineDocs();

}

/**
* select all if block
*/
public void selectIfMultilineCorrectSel() {


String espected = "/**\n" +
" *Docs..\n" +
" */\n" +
"void loop() {\n" +
" // LED ON\n" +
"digitalWrite(LED_BUILTIN, HIGH);\n" +
" if (true) {\n" +
" delay( 1000)\n" +
" };\n" +
" digitalWrite(LED_BUILTIN, LOW);\n" +
" if (true) {delay( 1000 )}; // <<< UGLY FORMATTING\n" +
"}";

validateFormat(78, 109, orig, espected);

}


/**
* selection starts in if (|true) and goto '}'
* expected: format entry block
*/
public void selectIfMultilineCorrectPartialSel() {

String espected = "/**\n" +
" *Docs..\n" +
" */\n" +
"void loop() {\n" +
" // LED ON\n" +
"digitalWrite(LED_BUILTIN, HIGH);\n" +
" if (true) {\n" +
" delay( 1000)\n" +
" };\n" +
" digitalWrite(LED_BUILTIN, LOW);\n" +
" if (true) {delay( 1000 )}; // <<< UGLY FORMATTING\n" +
"}";

validateFormat(81, 109, orig, espected);

}

/**
* selection starts in multiple docs /** *\/
* expected: format entry block
*/
public void selectIfMultilineDocs() {

String espected = "/**\n" +
" Docs..\n" +
"*/\n" +
"void loop() {\n" +
" // LED ON\n" +
" digitalWrite(LED_BUILTIN, HIGH);\n" +
"if (true) {\n" +
" delay( 1000)\n" +
"};\n" +
" digitalWrite(LED_BUILTIN, LOW);\n" +
" if (true) {delay( 1000 )}; // <<< UGLY FORMATTING\n" +
"}";

validateFormat(9, 51, orig, espected);

}

/**
* selection starts in single line coment // LED |ON
* expected: format comment
*/
public void selectIfSinglelineDocs() {

String espected = "/**\n" +
" *Docs..\n" +
" */\n" +
"void loop() {\n" +
" // LED ON\n" +
" digitalWrite(LED_BUILTIN, HIGH);\n" +
"if (true) {\n" +
" delay( 1000)\n" +
"};\n" +
" digitalWrite(LED_BUILTIN, LOW);\n" +
" if (true) {delay( 1000 )}; // <<< UGLY FORMATTING\n" +
"}";

validateFormat(42, 56, orig, espected);

}


public SketchTextAreaFixture validateFormat(int selStart, int selEnd, String orig, String espected) {
JMenuItemFixture menuToolsAutoFormat = window.menuItem("menuToolsAutoFormat");
menuToolsAutoFormat.requireEnabled();
SketchTextAreaFixture editor = window.textArea("editor");

editor.setText(orig);

editor.select(selStart, selEnd); // select frist if

menuToolsAutoFormat.click();

assertEquals(espected, editor.getText());

return editor;
}

}
11 changes: 11 additions & 0 deletions app/test/processing/app/helpers/SketchTextAreaComponentDriver.java
100644 → 100755
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,17 @@ protected SketchTextArea executeInEDT() {

});
}

public SketchTextArea select(final SketchTextArea target, int selectionStart, int selectionEnd) {
return GuiActionRunner.execute(new GuiQuery<SketchTextArea>() {

protected SketchTextArea executeInEDT() {
target.select(selectionStart, selectionEnd);
return target;
}

});
}

public Integer getCaretPosition(final SketchTextArea target) {
focusAndWaitForFocusGain(target);
Expand Down
5 changes: 5 additions & 0 deletions app/test/processing/app/helpers/SketchTextAreaFixture.java
100644 → 100755
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,11 @@ public SketchTextAreaFixture selectAll() {
driver.selectAll((SketchTextArea) target);
return this;
}

public SketchTextAreaFixture select(int selectionStart, int selectionEnd) {
driver.select((SketchTextArea) target, selectionStart, selectionEnd);
return this;
}

public int getCaretPosition() {
return driver.getCaretPosition((SketchTextArea) target);
Expand Down