vim: Add support for moving to first, middle and last visible lines (H, L, M)

[vbhavsar]

This change implements the vim motion commands to move the cursor to the top, middle and bottom of the visible view. This feature is requested in #4941.

This change takes inspiration from crates/vim/src/normal/scroll.rs.

A note on the behavior of these commands: Because NeovimBackedTestContext requires compatibility with nvim, the current implementation causes slightly non-standard behavior: it causes the editor to scroll a few lines. The standard behavior causes no scrolling. It is easy enough to account for the margin by adding VERTICAL_SCROLL_MARGIN. However, doing so will cause test failures due to the disparity between nvim and zed states. Perhaps NeovimBackedTestContext should have a switch to be more tolerant for such cases.

Release Notes:

• Added support for moving to top, middle and bottom of the screen in vim mode (H, M, and L) (#4941).

[vbhavsar]

Ancillary change

[vbhavsar]

After looking at the failing test, I'm having second thoughts about this implementation. The number of places that need to pass in a cloned Editor instance is too many. I am considering moving WindowTop implementation outside of the move_point structure and perform its own Vim::update. Any advice would be great. I'm very new to rust and zed :)

[ConradIrwin]

@vbhavsar Thanks for taking a pass at this!

If you'd like to pair on getting this over the line, feel free to book a slot here: https://calendly.com/conradirwin/pairing, Otherwise I'm happy if you want to forge ahead, notes below:

I agree with you that we shouldn't be cloning the whole editor everywhere. I think we should instead add another field or two to TextLayoutDetails, which was introduced to solve a similar problem (and I notice you are already passing to your implementation :D).

If we track the first visible row (as a DisplayPoint) and the number of visible rows, then we should be able to calculate what we need in the actions for H, M and L.

In terms of neovim compatibility, currently Zed mostly assumes you have :set scrolloff=3. I think we should emulate that here too to avoid oddities when you hit H<down> and Zed scrolls up. The tests for test_ctrl_d_u have an example of this if you need inspiration. (This will add more pressure to make Zed's hard-coded 3 a configuration option, which we can also do, though probably as a separate PR).

[vbhavsar]

@ConradIrwin Thanks for the great feedback!

Per your suggestion, I am trying to add the first visible row as a DisplayPoint to TextLayoutDetails. The issue I'm running into is that I seem to need the ViewContext in order to get the display point (via scroll_manager). But in editor::text_layout_details I only have access to the WindowContext, which is a parent of ViewContext.

The best idea I have is to change text_layout_details to accept the ViewContext instead of WindowContext, but that's a pretty drastic change that will touch a lot of code. Do you have better ideas?

Here is the change I am attempting to make:

    pub fn text_layout_details(&self, cx: WindowContext) -> TextLayoutDetails {
        let anchor = self.scroll_manager.anchor().anchor;
        let display_map = self.display_map.read(&cx);
        let display_snapshot = ??;// --> Need mut ViewContext to get a DisplaySnapshot 
        let first_visible_row = anchor.to_display_point(display_snapshot); // --> need display_snapshot to get display_point from anchor

        TextLayoutDetails {
            text_system: cx.text_system().clone(),
            editor_style: self.style.clone().unwrap(),
            rem_size: cx.rem_size(),
            first_visible_row: first_visible_row,
            visible_rows: self.visible_line_count(),
        }
    }

[ConradIrwin]

@vbhavsar Two thoughts, though I haven't looked the whole way:

• Maybe it's better to keep it as an Anchor and resolve it in the motion (which already has a DisplayMap).
• All the callers to .text_layout_details likely already have an &mut ViewContext<Editor>, so that may not be too difficult a change?

Look forward to pairing tomorrow!

[vbhavsar]

Keeping it as an anchor is an excellent suggestion. I like that work is done only when it is needed by the action.

The minor con with this approach is that the anchor field will need to be public rather than crate-public. I think that's okay, but let me know if you feel differently.

[vbhavsar]

@ConradIrwin The updated PR supports M and L motions as well! It became a lot easier once TextLayoutDetails contained the necessary data. Thanks for the great suggestions. We may not need to pair program after all :)

On a separate note, it's a bit scary that setting an invalid display point causes the whole program to crash. Is that being tracked as a bug?

[ConradIrwin]

Awesome, thank you! This is a great improvement.

I'm going to merge as is, but if you felt like extra credit.. then implementing count support for these (and respecting scrolloff) would be good next steps.

Crashing on invalid offsets is by design, failing fast finds failures first (or fffff..... which is how it makes you feel :D).

[vbhavsar]

Fffff is hilariously accurate. Thanks for enlightening me.