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

vim: Add support for ap and ip paragraph text objects #7687

Conversation

noritada
Copy link
Contributor

@noritada noritada commented Feb 12, 2024

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).

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Feb 12, 2024
@ConradIrwin ConradIrwin self-assigned this Feb 12, 2024
@ConradIrwin
Copy link
Member

@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.

@noritada
Copy link
Contributor Author

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

@noritada
Copy link
Contributor Author

noritada commented Feb 16, 2024

@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 noritada force-pushed the feat/vim-support-for-ap-ip-text-objects branch from ad5ef6c to f0756a9 Compare February 16, 2024 10:52
@ConradIrwin
Copy link
Member

@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
Copy link
Member

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

@noritada
Copy link
Contributor Author

@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?

@ConradIrwin
Copy link
Member

ConradIrwin commented Feb 27, 2024

@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.

@noritada
Copy link
Contributor Author

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 noritada force-pushed the feat/vim-support-for-ap-ip-text-objects branch from a7367e7 to ebb97f8 Compare February 28, 2024 17:30
@ConradIrwin
Copy link
Member

@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?

@noritada
Copy link
Contributor Author

noritada commented Mar 3, 2024

@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.

@ConradIrwin
Copy link
Member

Thanks for this @noritada!

@ConradIrwin ConradIrwin merged commit d223fe4 into zed-industries:main Mar 4, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants