From 0ac67dfd522dd1eff9ff6f1aa98b6f4aefa4fc80 Mon Sep 17 00:00:00 2001 From: swlim9999 <350199194+swlim9999@users.noreply.github.com> Date: Sat, 31 Oct 2026 21:15:21 +0800 Subject: [PATCH 1/2] Fix #127: Format keeps the cursor, Format Selection changes only the selection - Range formatting is used whenever the context is not the whole document (previously only when it did not start at offset 0). - The range sent to the server ends before a line the selection only touches at column 0; edits are limited to the selected whole lines. - Server edits are applied as the minimal changed text (common prefix and suffix are kept), so unchanged code keeps its positions, and the caret and selection stay on their lines instead of jumping to the end. - LSP positions past a line or the document are clamped instead of removing text up to an incorrect limit. - Fixed the log messages (String.format with "{0}") and restore the interrupt flag when interrupted. Co-Authored-By: Claude Opus 5.5 --- .../python/editor/PythonReformatTask.java | 198 +++++++++++++++--- 1 file changed, 172 insertions(+), 26 deletions(-) diff --git a/src/main/java/org/netbeans/modules/python/editor/PythonReformatTask.java b/src/main/java/org/netbeans/modules/python/editor/PythonReformatTask.java index 79caea1..d3c31c6 100644 --- a/src/main/java/org/netbeans/modules/python/editor/PythonReformatTask.java +++ b/src/main/java/org/netbeans/modules/python/editor/PythonReformatTask.java @@ -10,8 +10,11 @@ import java.util.concurrent.ExecutionException; import java.util.logging.Level; import java.util.logging.Logger; +import javax.swing.SwingUtilities; import javax.swing.text.BadLocationException; import javax.swing.text.Document; +import javax.swing.text.Element; +import javax.swing.text.JTextComponent; import javax.swing.text.StyledDocument; import org.apache.commons.lang3.StringUtils; import org.eclipse.lsp4j.DocumentFormattingParams; @@ -21,6 +24,7 @@ import org.eclipse.lsp4j.TextDocumentIdentifier; import org.eclipse.lsp4j.TextEdit; import org.netbeans.modules.editor.NbEditorUtilities; +import org.netbeans.api.editor.EditorRegistry; import org.netbeans.modules.editor.indent.api.IndentUtils; import org.netbeans.modules.editor.indent.spi.Context; import org.netbeans.modules.editor.indent.spi.ExtraLock; @@ -43,7 +47,8 @@ public PythonReformatTask(Context context) { @Override public void reformat() throws BadLocationException { Document document = context.document(); - boolean rangeOrDoc = context.startOffset() != 0; + boolean rangeOrDoc = context.startOffset() != 0 + || context.endOffset() != document.getLength(); FileObject fileObject = NbEditorUtilities.getFileObject(document); if (fileObject != null && !StringUtils.containsAny(fileObject.getPath(), PythonUtility.EXCLUDED_DIRS)) { @@ -65,27 +70,47 @@ public void reformat() throws BadLocationException { } + /** The range sent to the server, and the whole lines its edits may change. */ + record SelectedLines(int requestStart, int requestEnd, int changeStart, int changeEnd) { + } + + static SelectedLines selectedLines(Document document, int start, int end) { + Element root = document.getDefaultRootElement(); + // A selection ending at the start of a line does not include that line. + // Formatters round ranges to whole lines and would otherwise format it. + if (end > start && root.getElement(root.getElementIndex(end)).getStartOffset() == end) { + end--; + } + int firstLineStart = root.getElement(root.getElementIndex(start)).getStartOffset(); + int lastLineEnd = Math.min(document.getLength(), + root.getElement(root.getElementIndex(end)).getEndOffset()); + return new SelectedLines(start, end, firstLineStart, lastLineEnd); + } + private void rangeFormat(FileObject fo, LSPBindings bindings) throws BadLocationException { + SelectedLines lines = selectedLines(context.document(), context.startOffset(), context.endOffset()); DocumentRangeFormattingParams drfp = new DocumentRangeFormattingParams(); drfp.setTextDocument(new TextDocumentIdentifier(Utils.toURI(fo))); drfp.setOptions(new FormattingOptions( IndentUtils.indentLevelSize(context.document()), IndentUtils.isExpandTabs(context.document()))); drfp.setRange(new Range( - Utils.createPosition(context.document(), context.startOffset()), - Utils.createPosition(context.document(), context.endOffset()))); - List edits = new ArrayList<>(); + Utils.createPosition(context.document(), lines.requestStart()), + Utils.createPosition(context.document(), lines.requestEnd()))); + List edits = new ArrayList<>(); try { - edits = new ArrayList<>(bindings.getTextDocumentService() - .rangeFormatting(drfp).get()); - } catch (InterruptedException | ExecutionException ex) { + edits = bindings.getTextDocumentService().rangeFormatting(drfp).get(); + } catch (InterruptedException ex) { + Thread.currentThread().interrupt(); + return; + } catch (ExecutionException ex) { Logger.getLogger(this.getClass().getName()).log(Level.INFO, - String.format("LSP document rangeFormat failed for {0}", fo), + "LSP document rangeFormat failed for " + fo, ex); } - applyTextEdits(edits); + applyTextEdits(edits, lines.changeStart(), lines.changeEnd()); } private void documentFormat(FileObject fo, LSPBindings bindings) @@ -95,48 +120,169 @@ private void documentFormat(FileObject fo, LSPBindings bindings) dfp.setOptions(new FormattingOptions( IndentUtils.indentLevelSize(context.document()), IndentUtils.isExpandTabs(context.document()))); - List edits = new ArrayList<>(); + List edits = new ArrayList<>(); try { - edits.addAll(bindings.getTextDocumentService().formatting(dfp).get()); - } catch (InterruptedException | ExecutionException ex) { + edits = bindings.getTextDocumentService().formatting(dfp).get(); + } catch (InterruptedException ex) { + Thread.currentThread().interrupt(); + return; + } catch (ExecutionException ex) { Logger.getLogger(this.getClass().getName()).log(Level.INFO, - String.format("LSP document format failed for {0}", fo), + "LSP document format failed for " + fo, ex); } - applyTextEdits(edits); + applyTextEdits(edits, context.startOffset(), context.endOffset()); } - private void applyTextEdits(List edits) { + private void applyTextEdits(List edits, int startLimit, int endLimit) { if (context.document() instanceof StyledDocument) { NbDocument.runAtomic((StyledDocument) context.document(), () -> { - applyEditsNoLock(context.document(), edits, context.startOffset(), - context.endOffset()); + applyEditsNoLock(context.document(), edits, startLimit, endLimit); }); } else { - applyEditsNoLock(context.document(), edits, context.startOffset(), - context.endOffset()); + applyEditsNoLock(context.document(), edits, startLimit, endLimit); } } public static void applyEditsNoLock(Document doc, List edits, Integer startLimit, Integer endLimit) { + if (edits == null || edits.isEmpty()) { + return; + } + List carets = new ArrayList<>(); + for (JTextComponent editor : EditorRegistry.componentList()) { + if (editor.getDocument() == doc) { + carets.add(new CaretLocation(editor)); + } + } edits .stream() .sorted(rangeReverseSort) .forEach(te -> { try { - int start = Utils.getOffset(doc, te.getRange().getStart()); - int end = Utils.getOffset(doc, te.getRange().getEnd()); - if ((startLimit == null || start >= startLimit) - && (endLimit == null || end >= 0 || endLimit >= 0)) { - //https://github.com/apache/netbeans/blob/862379894fc239268e55f00bb4ce337d4c4437b8/ide/lsp.client/src/org/netbeans/modules/lsp/client/Utils.java#L182 - doc.remove(start, (end - start < 0 || end - start > endLimit) ? endLimit - start : end - start); - doc.insertString(start, te.getNewText(), null); + int start = editOffset(doc, te.getRange().getStart()); + int end = editOffset(doc, te.getRange().getEnd()); + if (start < 0 || end < start || end > doc.getLength()) { + return; } + // Formatters replace whole lines or the whole document. Apply + // only the changed text: limits then apply to real changes, and + // unchanged code keeps its positions and annotations. + String original = doc.getText(start, end - start); + String replacement = te.getNewText() == null ? "" : te.getNewText(); + int prefix = commonPrefix(original, replacement); + int suffix = commonSuffix(original, replacement, prefix); + int changeStart = start + prefix; + int changeEnd = end - suffix; + String text = replacement.substring(prefix, replacement.length() - suffix); + if ((changeStart == changeEnd && text.isEmpty()) + || (startLimit != null && changeStart < startLimit) + || (endLimit != null && changeEnd > endLimit)) { + return; + } + carets.forEach(caret -> caret.replacing(changeStart, changeEnd)); + doc.remove(changeStart, changeEnd - changeStart); + doc.insertString(changeStart, text, null); } catch (BadLocationException ex) { Exceptions.printStackTrace(ex); } }); + carets.forEach(CaretLocation::restore); + } + + static int commonPrefix(String original, String replacement) { + int max = Math.min(original.length(), replacement.length()); + int length = 0; + while (length < max && original.charAt(length) == replacement.charAt(length)) { + length++; + } + // Never split a surrogate pair between unchanged and replaced text. + return length > 0 && Character.isHighSurrogate(original.charAt(length - 1)) ? length - 1 : length; + } + + static int commonSuffix(String original, String replacement, int prefix) { + int max = Math.min(original.length(), replacement.length()) - prefix; + int length = 0; + while (length < max && original.charAt(original.length() - 1 - length) + == replacement.charAt(replacement.length() - 1 - length)) { + length++; + } + return length > 0 && Character.isLowSurrogate(original.charAt(original.length() - length)) ? length - 1 : length; + } + + private static int editOffset(Document document, org.eclipse.lsp4j.Position position) { + if (position.getLine() < 0 || position.getCharacter() < 0) { + return -1; + } + Element root = document.getDefaultRootElement(); + // Python formatters use the following line as EOF even when the last + // line has no newline. LSP positions beyond a line/document are clamped. + if (position.getLine() >= root.getElementCount()) { + return document.getLength(); + } + Element line = root.getElement(position.getLine()); + int end = Math.min(document.getLength(), line.getEndOffset() - 1); + return line.getStartOffset() + Math.min(position.getCharacter(), end - line.getStartOffset()); + } + + /** Keep the cursor and selection on their original lines when a server + * replaces the whole document. Swing otherwise moves them to its end. */ + private static final class CaretLocation { + private final JTextComponent editor; + private final Document document; + private final int dotLine; + private final int dotColumn; + private final int markLine; + private final int markColumn; + private boolean dotReplaced; + private boolean markReplaced; + + CaretLocation(JTextComponent editor) { + this.editor = editor; + document = editor.getDocument(); + Element root = document.getDefaultRootElement(); + int dot = editor.getCaret().getDot(); + int mark = editor.getCaret().getMark(); + dotLine = root.getElementIndex(dot); + dotColumn = dot - root.getElement(dotLine).getStartOffset(); + markLine = root.getElementIndex(mark); + markColumn = mark - root.getElement(markLine).getStartOffset(); + } + + void replacing(int start, int end) { + int dot = editor.getCaret().getDot(); + int mark = editor.getCaret().getMark(); + dotReplaced |= start <= dot && dot < end; + markReplaced |= start <= mark && mark < end; + } + + private int offset(int line, int column) { + Element root = document.getDefaultRootElement(); + Element element = root.getElement(Math.min(line, root.getElementCount() - 1)); + return Math.min(element.getStartOffset() + column, + Math.min(document.getLength(), element.getEndOffset() - 1)); + } + + void restore() { + if (!dotReplaced && !markReplaced) { + return; + } + int dotAfterEdit = editor.getCaret().getDot(); + int markAfterEdit = editor.getCaret().getMark(); + Runnable update = () -> { + if (editor.getDocument() == document + && editor.getCaret().getDot() == dotAfterEdit + && editor.getCaret().getMark() == markAfterEdit) { + editor.getCaret().setDot(markReplaced ? offset(markLine, markColumn) : markAfterEdit); + editor.getCaret().moveDot(dotReplaced ? offset(dotLine, dotColumn) : dotAfterEdit); + } + }; + if (SwingUtilities.isEventDispatchThread()) { + update.run(); + } else { + SwingUtilities.invokeLater(update); + } + } } private static final Comparator rangeReverseSort = (s1, s2) -> { From cde1b6a3c9e432c97d5dff2cdc6048c235f6a888 Mon Sep 17 00:00:00 2001 From: swlim9999 <350199194+swlim9999@users.noreply.github.com> Date: Sun, 1 Nov 2026 10:16:55 +0800 Subject: [PATCH 2/2] Apply related formatter edits together and keep all carets (review of #146) - Formatter edits are applied to a copy of the text as LSP specifies (ranges refer to the original text, inserts at one position keep their order), and the result is compared with the original. Changed lines are split only where the code before the split is the same in both versions, and each part is applied or rejected as a whole. YAPF's separate deletions and insertions can no longer delete selected code, and whole-document YAPF formatting no longer reorders or drops lines. - If a rejected part re-indents code, no part does, so a block is never left half re-indented. - All carets are read with EditorCaret.getCarets() and restored with replaceCarets(), each at the same code as before (setDot() kept only the last caret, and EditorCaret merges carets that meet in replaced text). - Regression tests built from real yapf, autopep8 and Black responses; JUnit test dependency and a headless surefire setting in the pom. Co-Authored-By: Claude Opus 5.5 --- pom.xml | 11 + .../modules/python/editor/FormatterEdits.java | 322 ++++++++++++++++++ .../python/editor/PythonReformatTask.java | 224 ++++++------ .../python/editor/PythonReformatTaskTest.java | 263 ++++++++++++++ 4 files changed, 697 insertions(+), 123 deletions(-) create mode 100644 src/main/java/org/netbeans/modules/python/editor/FormatterEdits.java create mode 100644 src/test/java/org/netbeans/modules/python/editor/PythonReformatTaskTest.java diff --git a/pom.xml b/pom.xml index a826611..eece9e5 100644 --- a/pom.xml +++ b/pom.xml @@ -311,6 +311,11 @@ org.apache.maven.plugins maven-surefire-plugin 3.1.2 + + + true + + @@ -847,6 +852,12 @@ org-netbeans-modules-autoupdate-services ${netbeans.release.version} + + junit + junit + 4.13.2 + test + target/keystore diff --git a/src/main/java/org/netbeans/modules/python/editor/FormatterEdits.java b/src/main/java/org/netbeans/modules/python/editor/FormatterEdits.java new file mode 100644 index 0000000..72ee012 --- /dev/null +++ b/src/main/java/org/netbeans/modules/python/editor/FormatterEdits.java @@ -0,0 +1,322 @@ +package org.netbeans.modules.python.editor; + +import java.util.ArrayList; +import java.util.Comparator; +import java.util.List; +import org.eclipse.lsp4j.Position; +import org.eclipse.lsp4j.TextEdit; + +/** + * The changes a formatter's {@link TextEdit}s make to a text, optionally + * limited to the selected lines. + *

+ * Formatters describe the same change in different ways: pylsp's YAPF plugin + * deletes lines and inserts their formatted versions with separate edits, + * autopep8 replaces the whole text. The edits are therefore applied to a copy + * of the text, and the result is compared with the original. Each change + * found that way is applied or rejected as a whole, so a deletion is never + * applied without its insertion. + */ +final class FormatterEdits { + + /** Replaces {@code [start, end)} of the original text with {@code text}. */ + record Change(int start, int end, String text) { + } + + private record Segment(Change change, boolean reindents) { + } + + private FormatterEdits() { + } + + /** + * The text after the edits, applied as LSP specifies: every range refers + * to the original text, and inserts at the same position keep their + * order. Returns {@code null} if an edit is invalid or edits overlap. + */ + static String apply(String text, List edits) { + int[] lineStarts = lineStarts(text); + List changes = new ArrayList<>(); + for (TextEdit edit : edits) { + if (edit == null || edit.getRange() == null) { + return null; + } + int start = offset(text, lineStarts, edit.getRange().getStart()); + int end = offset(text, lineStarts, edit.getRange().getEnd()); + if (start < 0 || end < start) { + return null; + } + changes.add(new Change(start, end, edit.getNewText() == null ? "" : edit.getNewText())); + } + // A stable sort: inserts at the same position keep their order and + // come before a replacement starting there. + changes.sort(Comparator.comparingInt(Change::start).thenComparingInt(Change::end)); + StringBuilder result = new StringBuilder(); + int copied = 0; + for (Change change : changes) { + if (change.start() < copied) { + return null; + } + result.append(text, copied, change.start()).append(change.text()); + copied = change.end(); + } + return result.append(text, copied, text.length()).toString(); + } + + /** + * The changes from {@code original} to {@code formatted}, in ascending + * order, that lie within {@code [startLimit, endLimit]} ({@code null}: + * no limit). + *

+ * Changed lines are split where the code (the text without whitespace) + * before the split is the same in both versions, so the parts can be + * applied independently. A part reaching outside the limits is rejected. + * Python requires consistent indentation, so if a rejected part + * re-indents code, no part re-indents code. + */ + static List changes(String original, String formatted, Integer startLimit, Integer endLimit) { + List before = lines(original); + List after = lines(formatted); + int first = 0; + while (first < before.size() && first < after.size() && before.get(first).equals(after.get(first))) { + first++; + } + int endBefore = before.size(); + int endAfter = after.size(); + while (endBefore > first && endAfter > first + && before.get(endBefore - 1).equals(after.get(endAfter - 1))) { + endBefore--; + endAfter--; + } + List removed = before.subList(first, endBefore); + List added = after.subList(first, endAfter); + int start = String.join("", before.subList(0, first)).length(); + + List segments = new ArrayList<>(); + List boundaries = boundaries(removed, added); + for (int k = 1; k < boundaries.size(); k++) { + int[] from = boundaries.get(k - 1); + int[] to = boundaries.get(k); + String oldText = String.join("", removed.subList(from[0], to[0])); + String newText = String.join("", added.subList(from[1], to[1])); + if (!oldText.equals(newText)) { + int prefix = commonPrefix(oldText, newText); + int suffix = commonSuffix(oldText, newText, prefix); + Change change = new Change(start + prefix, start + oldText.length() - suffix, + newText.substring(prefix, newText.length() - suffix)); + segments.add(new Segment(change, !indents(oldText).equals(indents(newText)))); + } + start += oldText.length(); + } + + boolean reindentsOutside = segments.stream() + .anyMatch(segment -> segment.reindents() && !within(segment.change(), startLimit, endLimit)); + List changes = new ArrayList<>(); + for (Segment segment : segments) { + if (within(segment.change(), startLimit, endLimit) && !(segment.reindents() && reindentsOutside)) { + changes.add(segment.change()); + } + } + return changes; + } + + /** + * Where {@code offset} of the original text is after the changes: before + * the same non-whitespace character, or, if it was followed by + * whitespace, after the same preceding one. + */ + static int map(String original, List changes, int offset) { + boolean beforeCode = offset < original.length() && !Character.isWhitespace(original.charAt(offset)); + int shift = 0; + for (Change change : changes) { + boolean insertion = change.start() == change.end(); + if (offset < change.start() || (insertion && offset == change.start() && !beforeCode)) { + break; + } + if (offset >= change.end()) { + shift += change.text().length() - (change.end() - change.start()); + continue; + } + // Inside the replaced text: find the same code in the new text. + String text = change.text(); + int code = code(original.substring(change.start(), offset)).length(); + int index = 0; + while (index < text.length() && (code > 0 || (beforeCode && Character.isWhitespace(text.charAt(index))))) { + if (!Character.isWhitespace(text.charAt(index))) { + code--; + } + index++; + } + return change.start() + shift + index; + } + return offset + shift; + } + + /** + * Line boundaries {i, j} where {@code removed[0..i)} and + * {@code added[0..j)} contain the same code, from {0, 0} to + * {removed.size(), added.size()}. + */ + private static List boundaries(List removed, List added) { + List a = new ArrayList<>(); + removed.forEach(line -> a.add(code(line))); + List b = new ArrayList<>(); + added.forEach(line -> b.add(code(line))); + List boundaries = samePrefix(a, b); + // Then the same from the end, after the last boundary found so far. + List fromEnd = samePrefix(reversed(a), reversed(b)); + int[] last = boundaries.get(boundaries.size() - 1); + for (int k = fromEnd.size() - 1; k >= 0; k--) { + int i = a.size() - fromEnd.get(k)[0]; + int j = b.size() - fromEnd.get(k)[1]; + if (i >= last[0] && j >= last[1] && (i > last[0] || j > last[1])) { + last = new int[]{i, j}; + boundaries.add(last); + } + } + return boundaries; + } + + /** Boundaries {i, j} where {@code a[0..i)} and {@code b[0..j)} join to the same string. */ + private static List samePrefix(List a, List b) { + int common = commonPrefix(String.join("", a), String.join("", b)); + List boundaries = new ArrayList<>(); + int i = 0; + int j = 0; + int lengthA = 0; + int lengthB = 0; + while (true) { + if (lengthA == lengthB) { + if (lengthA > common) { + break; + } + boundaries.add(new int[]{i, j}); + // Pair blank lines with blank lines. + boolean blankA = i < a.size() && a.get(i).isEmpty(); + boolean blankB = j < b.size() && b.get(j).isEmpty(); + boolean nextA = i < a.size() && (blankA || !blankB); + boolean nextB = j < b.size() && (blankB || !blankA); + if (!nextA && !nextB) { + break; + } + if (nextA) { + lengthA += a.get(i++).length(); + } + if (nextB) { + lengthB += b.get(j++).length(); + } + } else if (lengthA < lengthB) { + if (i == a.size()) { + break; + } + lengthA += a.get(i++).length(); + } else { + if (j == b.size()) { + break; + } + lengthB += b.get(j++).length(); + } + } + return boundaries; + } + + private static List reversed(List lines) { + List reversed = new ArrayList<>(); + for (int k = lines.size() - 1; k >= 0; k--) { + reversed.add(new StringBuilder(lines.get(k)).reverse().toString()); + } + return reversed; + } + + private static boolean within(Change change, Integer startLimit, Integer endLimit) { + return (startLimit == null || change.start() >= startLimit) + && (endLimit == null || change.end() <= endLimit); + } + + /** The text without whitespace. */ + private static String code(String text) { + StringBuilder code = new StringBuilder(); + for (int k = 0; k < text.length(); k++) { + if (!Character.isWhitespace(text.charAt(k))) { + code.append(text.charAt(k)); + } + } + return code.toString(); + } + + /** The leading whitespace of each line that is not blank. */ + private static List indents(String text) { + List indents = new ArrayList<>(); + for (String line : lines(text)) { + int length = 0; + while (length < line.length() && line.charAt(length) != '\n' + && Character.isWhitespace(line.charAt(length))) { + length++; + } + if (length < line.length() && line.charAt(length) != '\n') { + indents.add(line.substring(0, length)); + } + } + return indents; + } + + /** The lines of the text, each with its line break. */ + private static List lines(String text) { + List lines = new ArrayList<>(); + int start = 0; + while (start < text.length()) { + int end = text.indexOf('\n', start) + 1; + if (end == 0) { + end = text.length(); + } + lines.add(text.substring(start, end)); + start = end; + } + return lines; + } + + private static int[] lineStarts(String text) { + List lines = lines(text); + int[] starts = new int[lines.size() + (text.endsWith("\n") || text.isEmpty() ? 1 : 0)]; + for (int k = 1; k < starts.length; k++) { + starts[k] = starts[k - 1] + lines.get(k - 1).length(); + } + return starts; + } + + private static int offset(String text, int[] lineStarts, Position position) { + if (position == null || position.getLine() < 0 || position.getCharacter() < 0) { + return -1; + } + // Python formatters use the line after the last one as the end of the + // text, also when it has no final newline. LSP positions beyond a + // line or the text are clamped. + if (position.getLine() >= lineStarts.length) { + return text.length(); + } + int start = lineStarts[position.getLine()]; + int end = position.getLine() + 1 < lineStarts.length + ? lineStarts[position.getLine() + 1] - 1 : text.length(); + return start + Math.min(position.getCharacter(), end - start); + } + + private static int commonPrefix(String original, String replacement) { + int max = Math.min(original.length(), replacement.length()); + int length = 0; + while (length < max && original.charAt(length) == replacement.charAt(length)) { + length++; + } + // Never split a surrogate pair between unchanged and replaced text. + return length > 0 && Character.isHighSurrogate(original.charAt(length - 1)) ? length - 1 : length; + } + + private static int commonSuffix(String original, String replacement, int prefix) { + int max = Math.min(original.length(), replacement.length()) - prefix; + int length = 0; + while (length < max && original.charAt(original.length() - 1 - length) + == replacement.charAt(replacement.length() - 1 - length)) { + length++; + } + return length > 0 && Character.isLowSurrogate(original.charAt(original.length() - length)) ? length - 1 : length; + } +} diff --git a/src/main/java/org/netbeans/modules/python/editor/PythonReformatTask.java b/src/main/java/org/netbeans/modules/python/editor/PythonReformatTask.java index d3c31c6..8f25ce0 100644 --- a/src/main/java/org/netbeans/modules/python/editor/PythonReformatTask.java +++ b/src/main/java/org/netbeans/modules/python/editor/PythonReformatTask.java @@ -5,16 +5,18 @@ * @author albilu */ import java.util.ArrayList; -import java.util.Comparator; +import java.util.Arrays; import java.util.List; import java.util.concurrent.ExecutionException; import java.util.logging.Level; import java.util.logging.Logger; import javax.swing.SwingUtilities; import javax.swing.text.BadLocationException; +import javax.swing.text.Caret; import javax.swing.text.Document; import javax.swing.text.Element; import javax.swing.text.JTextComponent; +import javax.swing.text.Position; import javax.swing.text.StyledDocument; import org.apache.commons.lang3.StringUtils; import org.eclipse.lsp4j.DocumentFormattingParams; @@ -25,6 +27,8 @@ import org.eclipse.lsp4j.TextEdit; import org.netbeans.modules.editor.NbEditorUtilities; import org.netbeans.api.editor.EditorRegistry; +import org.netbeans.api.editor.caret.CaretInfo; +import org.netbeans.api.editor.caret.EditorCaret; import org.netbeans.modules.editor.indent.api.IndentUtils; import org.netbeans.modules.editor.indent.spi.Context; import org.netbeans.modules.editor.indent.spi.ExtraLock; @@ -146,135 +150,84 @@ private void applyTextEdits(List edits, int startLimit, int } public static void applyEditsNoLock(Document doc, List edits, Integer startLimit, Integer endLimit) { - if (edits == null || edits.isEmpty()) { - return; - } - List carets = new ArrayList<>(); + List editors = new ArrayList<>(); for (JTextComponent editor : EditorRegistry.componentList()) { if (editor.getDocument() == doc) { - carets.add(new CaretLocation(editor)); + editors.add(editor); } } - edits - .stream() - .sorted(rangeReverseSort) - .forEach(te -> { - try { - int start = editOffset(doc, te.getRange().getStart()); - int end = editOffset(doc, te.getRange().getEnd()); - if (start < 0 || end < start || end > doc.getLength()) { - return; - } - // Formatters replace whole lines or the whole document. Apply - // only the changed text: limits then apply to real changes, and - // unchanged code keeps its positions and annotations. - String original = doc.getText(start, end - start); - String replacement = te.getNewText() == null ? "" : te.getNewText(); - int prefix = commonPrefix(original, replacement); - int suffix = commonSuffix(original, replacement, prefix); - int changeStart = start + prefix; - int changeEnd = end - suffix; - String text = replacement.substring(prefix, replacement.length() - suffix); - if ((changeStart == changeEnd && text.isEmpty()) - || (startLimit != null && changeStart < startLimit) - || (endLimit != null && changeEnd > endLimit)) { - return; - } - carets.forEach(caret -> caret.replacing(changeStart, changeEnd)); - doc.remove(changeStart, changeEnd - changeStart); - doc.insertString(changeStart, text, null); - } catch (BadLocationException ex) { - Exceptions.printStackTrace(ex); - } - }); - carets.forEach(CaretLocation::restore); - } - - static int commonPrefix(String original, String replacement) { - int max = Math.min(original.length(), replacement.length()); - int length = 0; - while (length < max && original.charAt(length) == replacement.charAt(length)) { - length++; - } - // Never split a surrogate pair between unchanged and replaced text. - return length > 0 && Character.isHighSurrogate(original.charAt(length - 1)) ? length - 1 : length; - } - - static int commonSuffix(String original, String replacement, int prefix) { - int max = Math.min(original.length(), replacement.length()) - prefix; - int length = 0; - while (length < max && original.charAt(original.length() - 1 - length) - == replacement.charAt(replacement.length() - 1 - length)) { - length++; - } - return length > 0 && Character.isLowSurrogate(original.charAt(original.length() - length)) ? length - 1 : length; + applyEdits(doc, edits, startLimit, endLimit, editors); } - private static int editOffset(Document document, org.eclipse.lsp4j.Position position) { - if (position.getLine() < 0 || position.getCharacter() < 0) { - return -1; + /** Applies the edits within the limits, keeping the carets of the editors at their code. */ + static void applyEdits(Document doc, List edits, Integer startLimit, Integer endLimit, + List editors) { + if (edits == null || edits.isEmpty()) { + return; } - Element root = document.getDefaultRootElement(); - // Python formatters use the following line as EOF even when the last - // line has no newline. LSP positions beyond a line/document are clamped. - if (position.getLine() >= root.getElementCount()) { - return document.getLength(); + try { + String text = doc.getText(0, doc.getLength()); + String formatted = FormatterEdits.apply(text, edits); + if (formatted == null) { + Logger.getLogger(PythonReformatTask.class.getName()).log(Level.INFO, + "Ignoring invalid or overlapping formatting edits: {0}", edits); + return; + } + List changes = FormatterEdits.changes(text, formatted, startLimit, endLimit); + if (changes.isEmpty()) { + return; + } + List carets = new ArrayList<>(); + for (JTextComponent editor : editors) { + carets.add(new Carets(editor)); + } + // Only the changed text is replaced: unchanged code keeps its + // positions and annotations. + for (int k = changes.size() - 1; k >= 0; k--) { + FormatterEdits.Change change = changes.get(k); + if (change.end() > change.start()) { + doc.remove(change.start(), change.end() - change.start()); + } + if (!change.text().isEmpty()) { + doc.insertString(change.start(), change.text(), null); + } + } + for (Carets caret : carets) { + caret.restore(text, changes); + } + } catch (BadLocationException ex) { + Exceptions.printStackTrace(ex); } - Element line = root.getElement(position.getLine()); - int end = Math.min(document.getLength(), line.getEndOffset() - 1); - return line.getStartOffset() + Math.min(position.getCharacter(), end - line.getStartOffset()); } - /** Keep the cursor and selection on their original lines when a server - * replaces the whole document. Swing otherwise moves them to its end. */ - private static final class CaretLocation { + /** + * The carets of an editor, kept at their code when formatting replaces + * the text around them. Swing would otherwise move them to the end of + * the replaced text, e.g. the end of the document. + */ + private static final class Carets { private final JTextComponent editor; - private final Document document; - private final int dotLine; - private final int dotColumn; - private final int markLine; - private final int markColumn; - private boolean dotReplaced; - private boolean markReplaced; + private final int[] original; - CaretLocation(JTextComponent editor) { + Carets(JTextComponent editor) { this.editor = editor; - document = editor.getDocument(); - Element root = document.getDefaultRootElement(); - int dot = editor.getCaret().getDot(); - int mark = editor.getCaret().getMark(); - dotLine = root.getElementIndex(dot); - dotColumn = dot - root.getElement(dotLine).getStartOffset(); - markLine = root.getElementIndex(mark); - markColumn = mark - root.getElement(markLine).getStartOffset(); - } - - void replacing(int start, int end) { - int dot = editor.getCaret().getDot(); - int mark = editor.getCaret().getMark(); - dotReplaced |= start <= dot && dot < end; - markReplaced |= start <= mark && mark < end; - } - - private int offset(int line, int column) { - Element root = document.getDefaultRootElement(); - Element element = root.getElement(Math.min(line, root.getElementCount() - 1)); - return Math.min(element.getStartOffset() + column, - Math.min(document.getLength(), element.getEndOffset() - 1)); + this.original = positions(editor); } - void restore() { - if (!dotReplaced && !markReplaced) { + void restore(String text, List changes) { + int[] target = new int[original.length]; + for (int k = 0; k < original.length; k++) { + target[k] = FormatterEdits.map(text, changes, original[k]); + } + int[] afterEdit = positions(editor); + if (Arrays.equals(target, afterEdit)) { return; } - int dotAfterEdit = editor.getCaret().getDot(); - int markAfterEdit = editor.getCaret().getMark(); + Document document = editor.getDocument(); Runnable update = () -> { - if (editor.getDocument() == document - && editor.getCaret().getDot() == dotAfterEdit - && editor.getCaret().getMark() == markAfterEdit) { - editor.getCaret().setDot(markReplaced ? offset(markLine, markColumn) : markAfterEdit); - editor.getCaret().moveDot(dotReplaced ? offset(dotLine, dotColumn) : dotAfterEdit); + // Unless the carets were moved meanwhile. + if (editor.getDocument() == document && Arrays.equals(positions(editor), afterEdit)) { + setPositions(editor, target); } }; if (SwingUtilities.isEventDispatchThread()) { @@ -283,19 +236,44 @@ void restore() { SwingUtilities.invokeLater(update); } } - } - private static final Comparator rangeReverseSort = (s1, s2) -> { - int l1 = s1.getRange().getEnd().getLine(); - int l2 = s2.getRange().getEnd().getLine(); - int c1 = s1.getRange().getEnd().getCharacter(); - int c2 = s2.getRange().getEnd().getCharacter(); - if (l1 != l2) { - return l2 - l1; - } else { - return c2 - c1; + /** Dot and mark of each caret, in the order of the editor's carets. */ + private static int[] positions(JTextComponent editor) { + Caret caret = editor.getCaret(); + if (caret instanceof EditorCaret) { + List carets = ((EditorCaret) caret).getCarets(); + int[] positions = new int[2 * carets.size()]; + for (int k = 0; k < carets.size(); k++) { + positions[2 * k] = carets.get(k).getDot(); + positions[2 * k + 1] = carets.get(k).getMark(); + } + return positions; + } + return caret == null ? new int[0] : new int[]{caret.getDot(), caret.getMark()}; } - }; + + private static void setPositions(JTextComponent editor, int[] positions) { + Caret caret = editor.getCaret(); + if (caret instanceof EditorCaret) { + // EditorCaret.setDot() would remove all carets but the last one, + // and carets that met in replaced text are already merged. + Document document = editor.getDocument(); + List dotsAndMarks = new ArrayList<>(); + try { + for (int position : positions) { + dotsAndMarks.add(document.createPosition(position)); + } + } catch (BadLocationException ex) { + Exceptions.printStackTrace(ex); + return; + } + ((EditorCaret) caret).replaceCarets(dotsAndMarks, null); + } else if (caret != null) { + caret.setDot(positions[1]); + caret.moveDot(positions[0]); + } + } + } @Override public ExtraLock reformatLock() { diff --git a/src/test/java/org/netbeans/modules/python/editor/PythonReformatTaskTest.java b/src/test/java/org/netbeans/modules/python/editor/PythonReformatTaskTest.java new file mode 100644 index 0000000..0ef1964 --- /dev/null +++ b/src/test/java/org/netbeans/modules/python/editor/PythonReformatTaskTest.java @@ -0,0 +1,263 @@ +package org.netbeans.modules.python.editor; + +import java.awt.Toolkit; +import java.util.ArrayList; +import java.util.List; +import javax.swing.JEditorPane; +import javax.swing.JTextArea; +import javax.swing.SwingUtilities; +import javax.swing.text.BadLocationException; +import javax.swing.text.Document; +import javax.swing.text.JTextComponent; +import javax.swing.text.Position; +import org.eclipse.lsp4j.Range; +import org.eclipse.lsp4j.TextEdit; +import org.junit.Test; +import org.netbeans.api.editor.caret.CaretInfo; +import org.netbeans.api.editor.caret.EditorCaret; +import org.netbeans.editor.BaseDocument; + +import static org.junit.Assert.assertEquals; + +/** + * The edits below are the responses of python-lsp-server 1.15.0 with yapf + * 0.43.0, autopep8 2.0.4 and python-lsp-black 2.0.0 (black 26.10.1). + */ +public class PythonReformatTaskTest { + + private static final String HEADER_SOURCE = "# header\nx=1\ny=2\nz=3\n"; + + /** Selection: line 1 with its line break, so it ends at column 0 of line 2. */ + private static final int SELECTION_START = 9; + private static final int SELECTION_END = 13; + + /** YAPF formats one line more than requested and deletes and inserts lines separately. */ + @Test + public void yapfFormatSelectionKeepsTheSelectedStatementAndOtherLines() throws Exception { + BaseDocument document = document(HEADER_SOURCE); + PythonReformatTask.SelectedLines lines = PythonReformatTask.selectedLines(document, SELECTION_START, SELECTION_END); + assertEquals("The request ends on the selected line", 12, lines.requestEnd()); + formatSelection(document, SELECTION_START, SELECTION_END, + edit(1, 0, 2, 0, ""), + edit(2, 0, 3, 0, ""), + edit(3, 0, 3, 0, "x = 1\n"), + edit(3, 0, 3, 0, "y = 2\n")); + assertEquals("# header\nx = 1\ny=2\nz=3\n", text(document)); + } + + @Test + public void autopep8FormatSelectionWithWholeDocumentEdit() throws Exception { + BaseDocument document = document(HEADER_SOURCE); + formatSelection(document, SELECTION_START, SELECTION_END, + edit(0, 0, 4, 0, "# header\nx = 1\ny=2\nz=3\n")); + assertEquals("# header\nx = 1\ny=2\nz=3\n", text(document)); + } + + @Test + public void blackFormatSelection() throws Exception { + BaseDocument document = document(HEADER_SOURCE); + formatSelection(document, SELECTION_START, SELECTION_END, + edit(1, 0, 2, 0, "x = 1\n")); + assertEquals("# header\nx = 1\ny=2\nz=3\n", text(document)); + } + + /** YAPF joins the selected statement's lines and also reformats the next line. */ + @Test + public void yapfFormatSelectionJoiningLines() throws Exception { + BaseDocument document = document("x = foo(\n 1)\ny=2\nz=3\n"); + formatSelection(document, 0, 16, + edit(0, 0, 1, 0, ""), + edit(1, 0, 2, 0, ""), + edit(2, 0, 3, 0, ""), + edit(3, 0, 3, 0, "x = foo(1)\n"), + edit(3, 0, 3, 0, "y = 2\n")); + assertEquals("x = foo(1)\ny=2\nz=3\n", text(document)); + } + + /** Re-indenting only the selected part of a block would break the code. */ + @Test + public void formatSelectionDoesNotReindentPartOfABlock() throws Exception { + List yapf = List.of( + edit(1, 0, 2, 0, ""), + edit(2, 0, 3, 0, ""), + edit(3, 0, 3, 0, " a = 1\n"), + edit(3, 0, 3, 0, " b = 2\n")); + BaseDocument document = document("if x:\n a=1\n b=2\n"); + formatSelection(document, 6, 12, yapf.toArray(TextEdit[]::new)); + assertEquals("if x:\n a=1\n b=2\n", text(document)); + // The same response for a selection of the whole block. + formatSelection(document, 6, 18, yapf.toArray(TextEdit[]::new)); + assertEquals("if x:\n a = 1\n b = 2\n", text(document)); + } + + @Test + public void insertsAtTheSamePositionKeepTheirOrder() throws Exception { + BaseDocument document = document("x=1\ny=2\n"); + PythonReformatTask.applyEdits(document, List.of( + edit(0, 0, 1, 0, ""), + edit(1, 0, 2, 0, ""), + edit(2, 0, 2, 0, "x = 1\n"), + edit(2, 0, 2, 0, "y = 2\n")), 0, document.getLength(), List.of()); + assertEquals("x = 1\ny = 2\n", text(document)); + } + + @Test + public void formatterEndOfDocumentPositionWithoutFinalNewline() throws Exception { + BaseDocument document = document("x=1"); + PythonReformatTask.applyEdits(document, List.of(edit(0, 0, 1, 0, "x = 1\n")), + 0, document.getLength(), List.of()); + assertEquals("x = 1\n", text(document)); + } + + @Test + public void invalidOrOverlappingEditsChangeNothing() throws Exception { + BaseDocument document = document("x=1\n"); + PythonReformatTask.applyEdits(document, List.of(edit(0, 0, 0, 3, "x = 1"), edit(0, 1, 0, 2, " = ")), + 0, document.getLength(), List.of()); + PythonReformatTask.applyEdits(document, List.of(edit(0, 2, 0, 1, "")), + 0, document.getLength(), List.of()); + PythonReformatTask.applyEdits(document, null, 0, document.getLength(), List.of()); + assertEquals("x=1\n", text(document)); + } + + /** + * #127: the cursor moved to the end of the document. Black replaces the + * whole text and adds blank lines above the cursor. + */ + @Test + public void formatKeepsTheCursorAtItsCode() throws Exception { + String source = "import os\ndef f( a,b ):\n return a+b\nx=f( 1,2 )\n"; + String formatted = "import os\n\n\ndef f(a, b):\n return a + b\n\n\nx = f(1, 2)\n"; + SwingUtilities.invokeAndWait(() -> { + JTextArea editor = new JTextArea(); + editor.setDocument(document(source)); + editor.setCaretPosition(source.indexOf("x=f")); + editor.moveCaretPosition(source.indexOf("f( 1")); + formatDocument(editor, List.of(edit(0, 0, 4, 0, formatted))); + assertEquals(formatted, editor.getText()); + assertEquals("Selection start", formatted.indexOf("x = f"), editor.getCaret().getMark()); + assertEquals("Cursor", formatted.indexOf("f(1"), editor.getCaret().getDot()); + }); + } + + private static final String CARETS_SOURCE = "# header\nx=1+2\nz = 3\n"; + private static final String CARETS_FORMATTED = "# header\nx = 1 + 2\nz = 3\n"; + private static final List CARETS_YAPF = List.of( + edit(1, 0, 2, 0, ""), + edit(2, 0, 2, 0, "x = 1 + 2\n")); + + @Test + public void formatKeepsAllCarets() throws Exception { + SwingUtilities.invokeAndWait(() -> { + JEditorPane editor = editorWithCarets(2, 2, 11, 11); + formatDocument(editor, CARETS_YAPF); + assertEquals(CARETS_FORMATTED, editor.getText()); + assertCarets(editor, 2, 2, 13, 13); + }); + } + + @Test + public void formatKeepsAllCaretsFromABackgroundThread() throws Exception { + List editor = new ArrayList<>(); + SwingUtilities.invokeAndWait(() -> editor.add(editorWithCarets(2, 2, 11, 11))); + formatDocument(editor.get(0), CARETS_YAPF); + SwingUtilities.invokeAndWait(() -> { + assertEquals(CARETS_FORMATTED, editor.get(0).getText()); + assertCarets(editor.get(0), 2, 2, 13, 13); + }); + } + + /** "header" selected, and in the changed line from "1" to the line end. */ + @Test + public void formatKeepsAllSelections() throws Exception { + SwingUtilities.invokeAndWait(() -> { + JEditorPane editor = editorWithCarets(8, 2, 14, 11); + formatDocument(editor, CARETS_YAPF); + assertEquals(CARETS_FORMATTED, editor.getText()); + assertCarets(editor, 8, 2, 18, 13); + }); + } + + /** EditorCaret merges carets that meet while text is replaced. */ + @Test + public void formatKeepsCaretsInTheSameChangedText() throws Exception { + SwingUtilities.invokeAndWait(() -> { + JEditorPane editor = editorWithCarets(10, 10, 12, 12); + formatDocument(editor, CARETS_YAPF); + assertEquals(CARETS_FORMATTED, editor.getText()); + assertCarets(editor, 11, 11, 15, 15); + }); + } + + /** An editor whose carets have the given dot and mark pairs, the last one added last. */ + private static JEditorPane editorWithCarets(int... dotsAndMarks) { + JEditorPane editor = new JEditorPane() { + @Override + public Toolkit getToolkit() { + // EditorCaret copies selections to the system selection, which + // fails without a display. It skips that if access is denied. + throw new SecurityException("No system selection in tests"); + } + }; + editor.setDocument(document(CARETS_SOURCE)); + EditorCaret caret = new EditorCaret(); + editor.setCaret(caret); + Document document = editor.getDocument(); + try { + caret.setDot(dotsAndMarks[1]); + caret.moveDot(dotsAndMarks[0]); + for (int k = 2; k < dotsAndMarks.length; k += 2) { + caret.addCaret(document.createPosition(dotsAndMarks[k]), Position.Bias.Forward, + document.createPosition(dotsAndMarks[k + 1]), Position.Bias.Forward); + } + } catch (BadLocationException ex) { + throw new AssertionError(ex); + } + assertCarets(editor, dotsAndMarks); + return editor; + } + + private static void assertCarets(JTextComponent editor, int... dotsAndMarks) { + List expected = new ArrayList<>(); + for (int position : dotsAndMarks) { + expected.add(position); + } + List actual = new ArrayList<>(); + for (CaretInfo caret : ((EditorCaret) editor.getCaret()).getCarets()) { + actual.add(caret.getDot()); + actual.add(caret.getMark()); + } + assertEquals("Dot and mark of each caret", expected, actual); + } + + /** As {@code rangeFormat()} for a selection from {@code start} to {@code end}. */ + private static void formatSelection(Document document, int start, int end, TextEdit... edits) { + PythonReformatTask.SelectedLines lines = PythonReformatTask.selectedLines(document, start, end); + PythonReformatTask.applyEdits(document, List.of(edits), lines.changeStart(), lines.changeEnd(), List.of()); + } + + /** As {@code documentFormat()}, which holds the document lock. */ + private static void formatDocument(JTextComponent editor, List edits) { + BaseDocument document = (BaseDocument) editor.getDocument(); + document.runAtomic(() -> PythonReformatTask.applyEdits(document, edits, 0, document.getLength(), List.of(editor))); + } + + private static BaseDocument document(String text) { + BaseDocument document = new BaseDocument(false, "text/plain"); + try { + document.insertString(0, text, null); + } catch (BadLocationException ex) { + throw new AssertionError(ex); + } + return document; + } + + private static String text(Document document) throws BadLocationException { + return document.getText(0, document.getLength()); + } + + private static TextEdit edit(int startLine, int startColumn, int endLine, int endColumn, String text) { + return new TextEdit(new Range(new org.eclipse.lsp4j.Position(startLine, startColumn), + new org.eclipse.lsp4j.Position(endLine, endColumn)), text); + } +}