Skip to content

Fix #127: Format keeps the cursor, Format Selection changes only the selection - #146

Open
swlim9999 wants to merge 2 commits into
albilu:masterfrom
swlim9999:pr/127-format-selection
Open

swlim9999 wants to merge 2 commits into
albilu:masterfrom
swlim9999:pr/127-format-selection

Conversation

@swlim9999

Copy link
Copy Markdown

Fixes #127. First of the per-issue PRs discussed in #145.

What changes (one file, PythonReformatTask.java):

  • Format Selection uses range formatting whenever the selection isn't the whole document (before, only when it didn't start at offset 0).
  • A selection ending at column 0 of a line no longer pulls that line in; edits outside the selected lines are dropped.
  • Server edits are applied as the minimal changed text, so unchanged code keeps its position, and the cursor/selection stay on their lines instead of jumping to the end of the file.
  • LSP positions past the end of a line or the document are clamped (the old code could remove text up to a wrong limit).
  • Small: the log messages used String.format with {0}, and the interrupt flag is now restored.

How to test

  1. Open a Python file with badly formatted code in several places, e.g. x=1 on line 3 and y=2 on line 10.
  2. Put the cursor in the middle of the file, run Source → Format (Alt+Shift+F): the code is formatted and the cursor stays on its line.
  3. Select only lines 2–4 and run Format again on a fresh copy: only x = 1 changes, line 10 stays y=2.
  4. Select lines ending at the start of the next line (triple-click/drag to column 0): that next line isn't changed.

Built with RELEASE310 and RELEASE290; tested by hand in NetBeans 30/31 on Linux. I also have unit tests for this in my branch, but since the repo has no test setup they aren't included. I can send them as a separate PR if you want a test suite.

Written with AI assistance (Claude Opus), reviewed and tested by me.

🤖 Generated with Claude Code

…y 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 <noreply@anthropic.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for your contribution and helping move this project forward

@albilu albilu left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for the contribution. The ordinary single-caret Black case now keeps the cursor on its original line, and the tested Black/autopep8 selection-boundary and EOF cases behave correctly.

Requesting changes before merging because two reproduced issues remain in the new code:

  1. P1 — YAPF Format Selection can delete selected code. The per-edit selection filter can accept a deletion while rejecting the corresponding insertion. Please handle related edits together so selection filtering cannot apply only half of a replacement.
  2. P2 — Caret restoration drops additional carets. Calling EditorCaret.setDot() to restore the last caret removes the other carets. Please preserve all caret/selection entries through the NetBeans multicaret APIs.

The inline comments include reproductions and expected behavior. Since you mentioned having tests in your branch, please include focused regression tests for these two cases in this PR, with the minimal test setup needed to run them. The tests should assert that selected statements are preserved and correctly formatted, unselected code remains unchanged, and multiple carets/selections survive formatting. Please also retain coverage for the original single-caret cursor-jump fix.

Validation: JDK 21 / RELEASE310 compilation passed. A temporary harness exercised 27 formatter-response cases using actual Black, autopep8 and YAPF output, plus NetBeans BaseDocument/EditorCaret checks under a virtual display on both the UI and background threads. The relevant behavior was compared with base commit b5201d4. This was harness validation; a full interactive IDE session was not performed.

Reviewed head: 0ac67dfd522dd1eff9ff6f1aa98b6f4aefa4fc80.

Comment on lines +178 to +180
if ((changeStart == changeEnd && text.isEmpty())
|| (startLimit != null && changeStart < startLimit)
|| (endLimit != null && changeEnd > endLimit)) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

[P1] Keep related deletion/insertion edits together when enforcing selection limits

Filtering each TextEdit independently can delete code. With YAPF enabled, use this file and select only x=1 plus its newline (ending at column 0 of the following line):

# header
x=1
y=2
z=3

Applying YAPF's actual range-formatting response through this code produces:

# header
y=2
z=3

The expected result is x = 1 with y=2 and z=3 unchanged. YAPF returns separate deletions and insertions: the deletion for x=1 is inside the allowed range, but its replacement insertion is outside that range and is discarded by this condition. The selected statement is therefore lost.

Please normalize/group the related edits against the original document before applying the selection limits, so an insertion cannot be rejected while its associated deletion is applied. If a replacement cannot safely be restricted to the selection, reject the related replacement as a whole. Please add a regression test asserting the complete resulting document.

Reproduced with python-lsp-server 1.15.0, yapf 0.43.0, and the compiled PR's selectedLines() / applyEditsNoLock() on a NetBeans BaseDocument.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed in cde1b6a. The edits are now applied to a copy of the text as LSP specifies (all ranges refer to the original text, and inserts at one position keep their order), and the result is compared with the original. Changed lines are split only where the code (the text without whitespace) before the split is the same in both versions. Each part is applied or rejected as a whole, so a deletion is never applied without its insertion.

Your example now gives x = 1 with y=2 and z=3 unchanged (yapfFormatSelectionKeepsTheSelectedStatementAndOtherLines, using the YAPF response you describe). Two related cases are tested as well: YAPF joining the selected statement's lines while also reformatting the next line (yapfFormatSelectionJoiningLines), and re-indentation. If a rejected part re-indents code, no part does, so a block is never left half re-indented (formatSelectionDoesNotReindentPartOfABlock).

Comment on lines +276 to +277
editor.getCaret().setDot(markReplaced ? offset(markLine, markColumn) : markAfterEdit);
editor.getCaret().moveDot(dotReplaced ? offset(dotLine, dotColumn) : dotAfterEdit);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

[P2] Preserve all NetBeans carets when restoring positions

EditorCaret.setDot() retains only the last caret, so this call removes additional carets even when they are outside the edited text. Capturing just getDot() / getMark() also omits the other caret/selection entries.

Reproduction case:

# header
x=1+2
z = 3

Place one caret in the unchanged header, then add the most recent caret inside x=1+2, and format with YAPF. With the actual YAPF-generated edits applied to NetBeans BaseDocument + EditorCaret, the base version retains two carets (2 -> 2), while this PR reduces them to one (2 -> 1). This occurs on both the UI thread and a background thread.

Please snapshot and restore all caret/selection entries using the NetBeans multicaret APIs (getCarets() and moveCarets() / replaceCarets() as appropriate), with the ordinary Swing-caret path as a fallback. Add a regression test verifying both the caret count and the preserved positions/selections, including a caret outside the changed range.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed in cde1b6a. All carets are read with getCarets() before the edits and restored with replaceCarets(), each at the same code: before the same character, or after the preceding one if whitespace followed. A plain Swing caret is the fallback. I used replaceCarets() because EditorCaret already merges carets that meet inside the replaced text (two carets in x=1+2 became one), and moveCarets() can't bring the second one back.

Tests: formatKeepsAllCarets (your case: a caret in the header, the most recent one in x=1+2), formatKeepsAllCaretsFromABackgroundThread, formatKeepsAllSelections and formatKeepsCaretsInTheSameChangedText check the caret count and each dot and mark. formatKeepsTheCursorAtItsCode keeps the single-caret #127 case, where Black adds blank lines above the cursor.

…lbilu#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 <noreply@anthropic.com>
@swlim9999

Copy link
Copy Markdown
Author

Thanks for the detailed review. Both points are fixed in cde1b6a, and the PR now includes tests:

  • Test setup (pom.xml): JUnit 4.13.2 with test scope, and java.awt.headless=true for surefire. mvn -B clean install -P dev as in builds.yml runs them, and they pass for RELEASE290 and RELEASE310, also without a display.
  • PythonReformatTaskTest: 13 tests built from real responses of python-lsp-server 1.15.0 with yapf 0.43.0, autopep8 2.0.4 and python-lsp-black 2.0.0. 10 of them fail on the previous head.

Also found while testing: with YAPF, whole-document Format reorders or drops lines, on master too. x=1\ny=2\n becomes empty, and # header\nx=1+2\nz = 3\n becomes # header\nz = 3\nx = 1 + 2\n, because the edits were applied one by one to the changing document. Applying them to the original text fixes this (insertsAtTheSamePositionKeepTheirOrder).

Other validation: a harness applied 648 real formatter responses (9 sample files × yapf, autopep8 and Black × the whole document and every line range). Whole-document results matched the formatter output, unselected lines never changed, results compiled whenever the input and the formatter output did, and 2,187 caret positions stayed at the same code. Caret restoration was also checked while holding the document lock, on the UI thread and on a background thread. I haven't done an interactive IDE session.

One test detail: EditorCaret updates the system selection and only catches SecurityException there, while the headless toolkit throws HeadlessException. The test editor's getToolkit() therefore throws a SecurityException.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

When you format a file, the cursor jumps to the end of the file

2 participants