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

2732 slurp barf multicursor #2733

Draft
wants to merge 24 commits into
base: dev
Choose a base branch
from

Conversation

pbwolf
Copy link
Contributor

@pbwolf pbwolf commented Feb 17, 2025

What has changed?

  • Slurp and barf support multi-cursor editing (backward and forward: four commands in all).
  • Internally (paredit.ts), multi-cursor concerns are factored out and separated from per-command strategy:
    • The core algorithm of each command - the calculation of ModelEdits for a single cursor - is extracted to a new function.
      • Following the example of rewrapSexpr, the new functions produce an array of changeRange edits. Previous use of deleteRange or insertString by slurp and barf is changed to changeRange.
        • The 100% changeRange technique worked OK in VS Code but flunked the unit tests, exposing a bug and requiring a correction in the model's emulation of how VS Code moves cursors after edits. The emulator was used only(?) for tests, but, as described below, this PR involves it with post-edit reformatting too.
    • A single new shared multi-cursor-collation function to deduplicate and sort all cursors' ModelEdits is extracted from rewrapSexpr.
    • In sum: rewrap, slurp, and barf all use the same multi-cursor coordination heuristics, which may be general enough to help extend further editing commands for multiple cursors.
  • Barf no longer requires an async touch-up of the selection positions: reducing likelihood of a race condition between two barf commands issued in rapid succession. Async touch-up is fraught in the multi-cursor case; simpler to let VS Code automatically float cursors on the surf of changeRange edits. (Slurp already did not require async touch-up of selections.)
  • New multi-cursor tests of slurp and barf, forward and backward, are based on the first, simplest single-cursor test of the four respective functions.
  • Single-cursor unit tests of slurp and barf are not modified.
  • Unit tests have been added to memorialize that forwardBarf and backwardBarf keep the cursor from being left outside the shortened form.

Slurp and barf trigger reformatting. This PR makes reformatting multicursor-capable (because reformatting had canceled multi-cursor mode).

  • Structural-editing reformatting formats all edited points, and preserves all cursors so you can proceed to issue further multi-cursor editing commands.
    • Reformatting no longer relies on editing commands to provide a formatDepth. Instead, the ranges to reformat are computed from the list of edits. Specifically: each edit (such as insertion or removal of a parenthesis) affects a certain small range of the document. These edits are all performed in one transaction and therefore have positions relative to the pre-edit document (their position does not reflect insertions or removals to their left), as required by TextEditorEdit. After the edit transaction commits, the structural edits' positions are shifted to correspond to the expected post-edit document. Each of the resulting positions is considered ripe for reformatting, or more precisely, the list enclosing each position is considered ripe. Those lists may overlap, but the edit transaction that reformats is not permitted to mention overlapping ranges. Shortest disjoint lists are found that subsume all the lists that need reformatting. Those are submitted to the formatter.
    • Reformatting preserves multiple cursors by not replacing the whole text blob with a formatted version of the same, but rather translating the difference between the two into little changeRange edits that only adjust the whitespace. The pre-formatting and re-formatted texts are translated into a series of (whitespace, non-whitespace) tuples, then those tuples are split as finely as necessary (in case reformatting inserted space where there was none) to result in the same number of tuples and same sequence of non-whitespace strings in the pre- and re-formatted versions. Then the non-whitespace bits are disregarded, the pre- and re- sequences of whitespace are compared, and TextEdits are composed to expand or shrink each pre- whitespace node to the corresponding re-formatted node.
      • Adjusting-whitespace-only appears to have the unexpected benefit of less syntax-color flickering.
  • The Format Current Form command shares some code with structural-editing reformatting and also now works with multiple cursors. Each cursor is translated to a range-to-reformat as before. Those might overlap, so minimal disjoint lists are found that encompass those ranges. The first character of each of those lists is used to seed a reformatting, which again computes a range-to-reformat, computes reformatted text, and computes a bunch of TextEdits to adjust whitespace. All whitespace edits resulting from those reformattings are executed in a single transaction.
  • The Format Selection command is a delegate of Format Current Form when the top-level needs attention, so Format Selection, too, adjusts the document not by replacing text with formatted text, but by adjusting each little run of whitespace.

Loose ends:

  • Slurp, barf, and rewrap do not check whether the user enabled multi-cursor features (nor is a {multicursor:false} option heeded), because the single-cursor case is not a distinct procedure. This deviation from documentation could be remedied in the documentation... but on the other hand, paredit's other multi-cursor support has been established for two or three years and the burden of supporting a feature flag might no longer be required?

Wobbly parts:

  • I did not find tests for reformatting. There might be differences around the fringes, related to the more indirect determination of ranges to reformat.

Fixes #2732

My Calva PR Checklist

I have:

  • Read How to Contribute.
  • Directed this pull request at the dev branch. (Or have specific reasons to target some other branch.)
  • Made sure I have changed the PR base branch, so that it is not published. (Sorry for the nagging.)
  • Made sure there is an issue registered with a clear problem statement that this PR addresses, (created the issue if it was not present).
    • Updated the [Unreleased] entry in CHANGELOG.md, linking the issue(s) that the PR is addressing.
  • Figured if anything about the fix warrants tests on Mac/Linux/Windows/Remote/Whatever, and either tested it there if so, or mentioned it in the PR.
  • Added to or updated docs in this branch, if appropriate
  • Tests
    • Tested the particular change
    • Figured if the change might have some side effects and tested those as well.
  • Formatted all JavaScript and TypeScript code that was changed. (use the prettier extension or run npm run prettier-format)
  • Confirmed that there are no linter warnings or errors (use the eslint extension, run npm run eslint before creating your PR, or run npm run eslint-watch to eslint as you go).

Ping @PEZ, @bpringe, @corasaurus-hex, @Cyrik

@pbwolf pbwolf marked this pull request as draft February 17, 2025 05:22
Copy link

netlify bot commented Feb 17, 2025

Deploy Preview for calva-docs ready!

Name Link
🔨 Latest commit eea04fc
🔍 Latest deploy log https://app.netlify.com/sites/calva-docs/deploys/67bd146995e02700088cd73e
😎 Deploy Preview https://deploy-preview-2733--calva-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@pbwolf pbwolf marked this pull request as ready for review February 17, 2025 05:25
@PEZ
Copy link
Collaborator

PEZ commented Feb 17, 2025

You are awesome, @pbwolf! ❤️

I will have a closer look tomorrow. Must 💤 now.

@pbwolf pbwolf marked this pull request as draft February 22, 2025 21:40
@PEZ
Copy link
Collaborator

PEZ commented Feb 23, 2025

I found this issue, don't know if you are aware:

[:a |:b]

barf forward

Expected:

[:a|] :b

Actual:

[:a] |:b

It's as if it attaches the cursor position to the closest form. Barfing forward from here works as expected:

[:a| :b]

@pbwolf
Copy link
Contributor Author

pbwolf commented Feb 23, 2025

@PEZ I added 2 unit tests to check that barf forward keeps the cursor within the shortened list, and made it do so.

@pbwolf
Copy link
Contributor Author

pbwolf commented Feb 24, 2025

Hmm. Barf backward, also, can leave the cursor outside the narrowed form, unlike in version 486.

@pbwolf
Copy link
Contributor Author

pbwolf commented Feb 25, 2025

Added a unit test that barf backward keeps the cursor within the shortened list, and made it do so.

@pbwolf pbwolf marked this pull request as ready for review February 25, 2025 01:00
@PEZ
Copy link
Collaborator

PEZ commented Feb 25, 2025

I forgot to report back that I tested the barf forward after your fix and that it worked great. Now have also tested barf backward and it behaves beautifully. And I am getting addicted to multi cursor slurp and barf!

@pbwolf
Copy link
Contributor Author

pbwolf commented Feb 25, 2025

"Undo" treats the formatting and the slurp/barf as two distinct events. Is that tolerable?
(It has the benefit, that if something surprises you, you can distinguish a-surprise-during-formatting from a-surprise-during-structural-editing.)

@PEZ
Copy link
Collaborator

PEZ commented Feb 25, 2025

Undo is a bit strange also in current Calva. Now tried with this branch, since you mentioned it. I think it is acceptable, as in I rather have multi cursor slurp and barf with these extra undo's than staying on single cursor. But it is a bit confusing. Especially when the formatting didn't do anything. We could release as is and try make it better later?

@pbwolf
Copy link
Contributor Author

pbwolf commented Feb 27, 2025

Undo is a bit strange also in current Calva. Now tried with this branch, since you mentioned it. I think it is acceptable, as in I rather have multi cursor slurp and barf with these extra undo's than staying on single cursor. But it is a bit confusing. Especially when the formatting didn't do anything. We could release as is and try make it better later?

I agree

@pbwolf pbwolf marked this pull request as draft February 28, 2025 12:30
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.

2 participants