Thank you for attempting to fix this @AidanV !

I suspect we might want to look for a simpler solution that does not involve the use of channels but can't suggest a different approach right now, as I'm not too familiar with how we're currently handling these. Will review the code and keep you updated 🙂

Hey @AidanV 👋

I spent some time getting more familiar with how we currently handle both ‎:g/[PATTERN]/[COMMAND] and ‎:v/[PATTERN]/[COMMAND], which helped me form a clearer picture of what these changes propose.

It looks like this pull request is tackling two issues at once, first, that ‎:%g and ‎:%v don’t affect the same lines as ‎:g and ‎:v, and second, the use of ‎VimNorm within ‎:g/‎:v.

On the former, your solution to replace ‎&range with ‎&None when calling ‎VimCommand.parse seems sound since we're checking ‎on_matching_lines.is_some(), and as far as I can tell, the ‎OnMatchingLines struct and its actions are only used with ‎:g/‎:v. So it feels correct to use the parsed range to determine the new selections but not forward that range to the dispatched action.

As for the ‎VimNorm issue, I believe sinceVimNorm spawns an asynchronous task, by the time that task runs, we’ve likely already called ‎MutableSelectionsCollection.select(vec![newest]), removing the selections that were set up. This is a known limitation with asynchronous actions, as we can’t reliably tell when their effects have been applied. The ‎action::Sequence action docs call this out:

NOTE: This does not wait for asynchronous actions to complete before running the next action.

Conrad has briefly mentioned that it would be good to introduce a way to track when an action has finished, but that’s not currently in progress. Unfortunately, I believe the suggested changes don’t solve this either, since they rely on the state of the ‎Workspace.dispatching_keystrokes task to infer completion but, as far as I can tell, that task seems only to track whether keystrokes were dispatched, not whether the action completed. We could still hit cases where keystrokes were dispatched but the action hasn’t run.

For now, I’d suggest two alternatives:

1. Replacing the cx.defer_in call with a nested cx.on_next_frame calls
   This isn’t a true solution to “run only after the action finishes,” but I believe it's simpler than the proposed approach and matches patterns already used elsewhere to address similar timing issues. If we go this route, we should leave a comment explaining the rationale, and I like that it avoids creating a channel and spawning tasks for every command, even when not using the normal command:

   cx.on_next_frame(window, |_editor, window, cx| {
       cx.on_next_frame(window, |editor, window, cx| {
           let newest = editor
               .selections
               .newest::<Point>(&editor.display_snapshot(cx));
           editor.change_selections(
               SelectionEffects::no_scroll(),
               window,
               cx,
               |s| {
                   s.select(vec![newest]);
               },
           );
           editor.end_transaction_at(Instant::now(), cx);
       });
   })

2. Pushing selections to action
   Since we already compute the new selections in ‎OnMatchingLines.run, we could pass these selections directly to the ‎VimNorm action? That would let ‎VimNorm set the selections during processing and avoid relying on selections remaining untouched until the action finishes.

Personally, I’d probably go with the range fix for ‎%:g and ‎%:v, and the ‎cx.on_next_frame approach for now, but I’m open to your thoughts. And apologies for the long reply! 🙂

Thank you for the detailed response @dinocosta!

I agree with Conrad that we should introduce a way to track action completion. I think that that is the "correct" solution for this problem, but since we don't have that, here is what I did:

I tried out your suggested implementation with cx.on_next_frame in this commit. This seems to work reliably, but weirdly the test fails, and I feel we can do better :)

In the most recent commit, when running OnMatchingLines we check to see if we have a VimNorm action. If we do, we inform the action that it will need to fix the selections and end the transaction itself, then we run the action and skip the equivalent selection code in OnMatchingLines. If I am understanding correctly, this effectively does the same thing as your second suggestion. I like this approach more, curious to hear your thoughts.

I tried out your suggested implementation with cx.on_next_frame in this commit. This seems to work reliably, but weirdly the test fails, and I feel we can do better :)

Interesting, do you remember why? If you do eventually chose to go the cx.on_next_frame path, I can help get it fixed 👍

In the most recent commit, when running OnMatchingLines we check to see if we have a VimNorm action. If we do, we inform the action that it will need to fix the selections and end the transaction itself, then we run the action and skip the equivalent selection code in OnMatchingLines. If I am understanding correctly, this effectively does the same thing as your second suggestion. I like this approach more, curious to hear your thoughts.

Thank you for tackling this! This approach seems more deterministic to me but I'm unsure if it makes sense for OnMatchingLines to calculate the new selections, start the transaction and update the selections and only then does VimNorm end the transaction, feels like we're now splitting the responsibility between two different places.

I wonder if it would be better to leverage VimNorm.range by introducing a new possible value to CommandRange that we could leverage here, which we would then set according to what was calculated on OnMatchingLines. If this new value was present, then the handling of VimNorm would actually be responsible for starting the transaction, doing the selections, running the normal commands and then collapsing the selection and ending the transaction, that way we keep all logic in the same place. What do you think? Let me know if this seems like a huge undertaking, as we can always look into it in a different Pull Request 🤔

Sorry for the slow progress on this. I haven't forgotten about it, just busy at the moment. I should be able to finish this up next week.

Interesting, do you remember why? If you do eventually chose to go the cx.on_next_frame path, I can help get it fixed 👍

After some investigating it looks like the cx.on_next_frame causes the cursors to not update before we check to see if it is the correct state in the tests. I tried adding cx.run_until_parked(); and I tried sleeping the thread, but neither seemed to work.

I wonder if it would be better to leverage VimNorm.range by introducing a new possible value to CommandRange that we could leverage here, which we would then set according to what was calculated on OnMatchingLines. If this new value was present, then the handling of VimNorm would actually be responsible for starting the transaction, doing the selections, running the normal commands and then collapsing the selection and ending the transaction, that way we keep all logic in the same place. What do you think? Let me know if this seems like a huge undertaking, as we can always look into it in a different Pull Request 🤔

I do think that this would work and would not be a huge undertaking, but I think that the CommandRange is not an ideal place for this, since it serves a very specific purpose and is used in quite a few places. Specifically, it is the range that is passed into every Vim command. I think that if we are going to special case this, then I much prefer the current solution in commit a255601. This is because it decreases the blast radius compared to modifying CommandRange.

I'm unsure if it makes sense for OnMatchingLines to calculate the new selections, start the transaction and update the selections and only then does VimNorm end the transaction, feels like we're now splitting the responsibility between two different places.

This is true. We are splitting the responsibility; however, someone stumbling on this code later on would not struggle to see the connection between the two since VimNorm is very visibly special cased in the OnMatchingLines code:

let (action, skip_fix_selections) = match action.as_any().downcast_ref::<VimNorm>() {
    Some(vim_norm) => {
        let mut vim_norm = vim_norm.clone();
        vim_norm.collapse_multicursor = true;
        (vim_norm.boxed_clone(), true)
    }
    None => (action, false),
};

Curious to hear your thoughts!

After giving it some more thought I went with something similar to the second solution you suggested:

2. Pushing selections to action
   Since we already compute the new selections in ‎OnMatchingLines.run, we could pass these selections directly to the ‎VimNorm action? That would let ‎VimNorm set the selections during processing and avoid relying on selections remaining untouched until the action finishes.

Now, we can give the VimNorm action an override_selections which will set up and tear down the selections. This also allowed for some nice simplifications and turned out in a way that I like a lot better.

Implementing this uncovered a bug in VimNorm where typing :norm I123 followed by undo would put the cursor at the end of the line instead of the beginning. It was just a one line change:

From:

old.0 = first_sel;

To:

old.0 = old.0.get(..1).unwrap_or(&[]).into();

I added regression tests for this behavior.

Since I ran into this on the path to fixing the bug this PR addresses (and it is so small) I think it is fair to include it in the PR, but if you think it is out of scope, I am happy to move this to a new PR.

I hope you are enjoying the break, and I look forward to hearing your thoughts in January!

Thank you for your help!