zed icon indicating copy to clipboard operation
zed copied to clipboard

vim: Add support for `ap` and `ip` paragraph text objects

Open noritada opened this issue 1 year ago • 10 comments

This PR adds support for ap/ip text objects in Vim mode and allows users to perform paragraph-based operations.

Cases where compatibility with Neovim's behavior is checked, cases where there are known differences in behavior with Neovim (cases where the landing position is other than the beginning of the line), and cases where the Neovim behavior in the test suite seems strange are separated in the test code so that they can be identified.

Release Notes:

  • Added support for ap and ip paragraph text objects in Vim mode (#7359).

noritada avatar Feb 12 '24 10:02 noritada

@noritada thank you for taking a pass at this!

I have a number of suggestions for improvement. If you'd like to work together on getting this over the line, feel free to book time here: https://calendly.com/conradirwin/pairing; otherwise I'll upload comments to Github and we can try to do this async.

ConradIrwin avatar Feb 12 '24 16:02 ConradIrwin

Thank you very much! I've booked at Calendly.

noritada avatar Feb 12 '24 16:02 noritada

@ConradIrwin I have updated the main part. Could you review it?

"Paragraphs" in vim include not only non-blank paragraphs but also "blank paragraphs," which is a little different from what most people think of as paragraphs. I felt that it would be difficult to understand the code without shared recognition of this specification, so I have documented the specification in the doc comment.

Since changes for normal/delete.rs is just for edge cases, I think I should confirm the main part first.

noritada avatar Feb 16 '24 10:02 noritada

@noritada Thanks for this, I like the new approach!

A few notes:

  • In practice it seems like vim_mode style paragraphs need completely different code from the existing paragraph code in the editor. I think it would be better to make our own copies of those functions vim_paragraph_end and put them in the vim crate instead of adding the boolean. (I had hoped they would be more similar, but I was wrong).
  • I think the target_visual_mode should return Mode::VisualLine for these (When used in Visual mode it is made linewise. from the docs). That would explain the test failures for visual mode.

ConradIrwin avatar Feb 16 '24 20:02 ConradIrwin

@noritada Please let me know if you're still planning to work on this.

ConradIrwin avatar Feb 23 '24 19:02 ConradIrwin

@ConradIrwin I think I have completed all of the necessary changes to the code that you pointed out.

However, after investigating the issue of not being able to properly test v i p and v a p, I found that it was due to the way we test visual line mode, so I am attempting to fix it. Since this fix involves a large change on its own, I have split the changes to PR #8333.

Could you please take a look at this PR to see why tests for the visual line mode is not passing?

noritada avatar Feb 24 '24 10:02 noritada

@noritada I think even with the fixes to the tests, there are still bugs to be addressed here:

## vip should work on the last line of a file with no trailing newline:

# initial state:
ˇThe quick brown fox jumps over the lazy dog.
# keystrokes:
v i p
# neovim state:
«Tˇ»he quick brown fox jumps over the lazy dog.
# zed state:
ˇThe quick brown fox jumps over the lazy dog.
## vip should select at least the first character of the last line

# initial state:
ˇThe quick brown fox jumps
 over the lazy dog.

# keystrokes:
v i p
# neovim state:
«The quick brown fox jumps
 ˇ»over the lazy dog.

# zed state:
«The quick brown fox jumps
ˇ» over the lazy dog.

In this case the zed line-mode selection does not extend to the last line of the paragraph.

It is probably easier to create test-cases manually instead of using assert_binding_matches_all. That is a little overkill (though it's kind of nice for fuzz-testing, it's quite hard to develop against as you can't keep track of progress). I would suggest making some more concrete tests like:

#[gpui::test]
fn test_visual_paragrah_object(cx: &mut gpui:::TestAppContext) {
        cx.assert_shared_state(indoc! {
            "ˇThe quick brown
            fox jumps over
            the lazy dog
            "})
        .await;
        cx.simulate_shared_keystrokes(["v", "i", "p"]).await;

        cx.assert_shared_state(indoc! {
            "«ˇThe quick brown
            fox jumps over
            t»he lazy dog
            "})
        .await;
}

Happy to work together on this if you want help getting it over the line: https://calendly.com/conradirwin/pairing.

ConradIrwin avatar Feb 27 '24 03:02 ConradIrwin

Thank you very much! I understand the policy.

By building and using the app, I have understood the problem and have fixed the end position of the selection. I will also make changes for edge cases and make the code testable.

noritada avatar Feb 27 '24 17:02 noritada

@noritada One comment for you, but this seems to work well for me manually. Is there anything else you want to do before I merge?

ConradIrwin avatar Feb 29 '24 02:02 ConradIrwin

@ConradIrwin Sorry for my absence for a few days.

There was an issue with v i p for only one blank "paragraph" resulting in the selection including a subsequent line and I wanted to fix it and I have fixed this issue. Thank you for your patience. I am glad you are merging.

noritada avatar Mar 03 '24 17:03 noritada

Thanks for this @noritada!

ConradIrwin avatar Mar 04 '24 23:03 ConradIrwin