Action release handlers

[alygin]

This PR adds support for handling action releases — events that are fired when the user releases all the modifier keys that were part of an action-triggering shortcut.

If the user holds modifiers and invokes several actions sequentially via shortcuts (same or different), only the last action is "released" when its modifier keys released.

The following methods were added to Div:

• capture_action_release()
• on_action_release()
• on_boxed_action_release()

They work similarly to capture_action(), on_action() and on_boxed_action().

Release Notes:

• Added support for action release handling (#8757).

Related Issues:

• Implements #8757
• Makes possible implementing #8258 (here's the main part of the implementation)
• Part of #7653

[ConradIrwin]

@alygin Thanks for this – it enables some neat features!

Looking at the implementation, I wonder if we're over-generalizing a little bit, and a simpler API would give us all we need. That said, I am aware you've thought about this more than me, so please let me know if I'm missing the point. (I'm also happy to chat through live if that would be helpful: https://calendly.com/conradirwin/pairing).

Things I don't think we need:

• to be able to store "release" actions for actions we aren't listening for.
• to distinguish between dispatch/bubble phases of released actions.

At one end of the spectrum I could imagine not adding this at all:

• The component would store the modifiers active on construction, and handle on_modifiers_changed itself (I'm not sure if you've explored this, if not it's probably worth doing at least once just to see how painful or not it is).

However, it does seem nice to provide an abstraction that hides the state management for you, particularly if we expect this to be used in several places and/or the state management is gnarly.

Maybe a good middle ground would be to do something like this:

        workspace.register_action(|workspace, _: &Toggle, cx| {
            let Some(file_finder) = workspace.active_modal::<Self>(cx) else {
                Self::open(workspace, cx);
                return;
            };
            file_finder.update(cx, |file_finder, cx| {
                file_finder
                    .picker
                    .update(cx, |picker, cx| picker.cycle_selection(cx))
            });

            cx.on_modifiers_released(|cx| {
              if workspace.active_modal::<Self>(cx).is_some() {
                  cx.dispatch_action(menu::Confirm.boxed_clone());
              };
            });
        });

This would still need most of the changes to the WindowContext to maintain the callbacks that need to be fired; but would let us avoid adding all the APIs to Div/interactive. This API would also probably "just work" in any context (for example when handling a KeyDown/MouseDown/KeyUp event too), which seems like an advantage for things that are not actions.

Thoughts?

[alygin]

@ConradIrwin, thank you for the feedback! Your ideas made me play a bit more with alternative implementations, and rethink a couple of edge cases.

The component would store the modifiers active on construction, and handle on_modifiers_changed itself (I'm not sure if you've explored this, if not it's probably worth doing at least once just to see how painful or not it is).

Actually, this was my first intention, but when I started implementing it, I found that I had to bring too much code to the tab switcher (or to the file finder). The reason is that it's not only the "modifiers released" event that we have to handle, but also many other cases that break the pattern, like when the user hits some not-in-the-shortcut key before releasing modifiers, or when the user changes modifiers without releasing them, or when something was opened programmatically (not manually) and the user just happened to hold some modifier keys at the moment, etc.

But I think you're right, in most practical cases it's not a problem at all, and on_modifiers_changed() should be enough. Just ignoring all the mentioned corner cases works good enough for features like the tab switcher.

Here's a playground for this impelementation: https://github.com/alygin/zed/pull/3/files

Unfortunately, there's a use case that bothers me a bit. Let's assume I hit ctrl-tab to open the tab switcher, and keep holding ctrl. Then I chaned my mind and decided to close the tab switcher without actually switching to anywhere, so I hit escape, which makes it ctrl-escape. But there's no such shortcut, and Zed ignores it. My options are:

• Register a ctrl-escape shortcut for the tab_switcher context. Feels a bit unnecessary, though may be ok.
• Handle escape in key_down(). Doesn't feel right.

What do you think of this case? Do I miss some better alternatives?

Maybe a good middle ground would be to do something like this:

It's not always about releasing modifiers. For instance, if the user pushes and holds some additional modifier befor releasing the initial ones. Thus, to work properly, this on_modifiers_release() should be bound to a shortcut, and we come to the on_action_release() again.

[ConradIrwin]

Interesting...

The reason is that it's not only the "modifiers released" event that we have to handle, but also many other cases that break the pattern, like when the user hits some not-in-the-shortcut key before releasing modifiers, or when the user changes modifiers without releasing them, or when something was opened programmatically (not manually) and the user just happened to hold some modifier keys at the moment, etc.

• I think it's totally fine to ignore the case where the dialogue is not opened via the keyboard (we can just make sure to open the dialogue with a different code-path if we need to).
• I'm not sure what you mean by "change modifiers without releasing them"?
• For the case of firing other keys, this does need some thinking through. From playing with VSCode:
• • cmd-p...cmd-p with cmd held should go "down" the list, but cmd-p cmd-p if you release cmd closes the dialogue again. (Also cmd-p...cmd-n should go "up" the list, but in VSCode this opens a new file :D).
• • ctrl-tab...ctrl-1 acts the same as ctrl-1, but it won't work without some extra code because the pane for pane::ActivateItem won't be on the dispatch stack.
• • ctrl-shift-tab...ctrl-tab should select the first tab (but in the current implementation I think it won't because when you lift the shift key that counts as a modifier release).

I think this means that we need to ensure these dialogues have their own key contexts. We may also need to make the context contain with_modifiers or similar so that we can distinguish between cmd-p...cmd-p and cmd-p cmd-p.

Register a ctrl-escape shortcut for the tab_switcher context. Feels a bit unnecessary, though may be ok.

I think that's totally fine and we want to support ctrl-1..9 in that context too (we get things like ctrl-n/p/enter for free right now because they're not scoped to any context).

Given that we need somewhat custom behavior in each of these cases, I think it's probably for the best that we go with the component-based approach for now (c.f. https://github.com/alygin/zed/pull/3/files) rather than trying to add a more bespoke gpui API until we have a few more examples implemented.

It'd be fun to work on this together if you have time, I have some availability listed here: https://calendly.com/conradirwin/pairing, but you can email [email protected] if none of those work for you.

[alygin]

I'm not sure what you mean by "change modifiers without releasing them"?

I mean the case when I press cmd-p, keep holding cmd, then add shift to it (which makes it cmd+shift), and release cmd, leaving only shift pressed. Thus, I've released all the modifiers from the initial shortcut (cmd), yet didn't release all the modifiers (shift is still pressed).

In this case I expect IDE to understand that initial modifiers have been released despite the fact that shift is still pressed. That's why I added Modifiers::is_subset_of() and use it along with checking Modifiers::modified(). And that's why I think the "middle ground" idea with cx.on_modifiers_released() will propbably not work properly.

Also cmd-p...cmd-n should go "up" the list, but in VSCode this opens a new file :D

Actually, Zed also opens new file on cmd-p...cmd-n, though I somehow belive it didn't do that before, but I'm not sure.

I think that's totally fine and we want to support ctrl-1..9

If I understand you correctly, this part slightly contradicts the idea with is_subset_of(). To make this scenario happen, we need to hold cmd after cmd+p, then add ctrl to it, then release cmd, then press 1, ending up with ctrl-1 (that's quite an excercise even for a seasoned pianist :D ). I would expect the file finder to close after releasing cmd, and this is how other editors behave (VSCode, JB's). But you expect it to keep the file finder open and perform the ctrl-1-bound action. Am I correct?

The latter is easier to implement, I guess, because we don't have to keep the initial modifiers state and check that is_subset_of().

Given that we need somewhat custom behavior in each of these cases, I think it's probably for the best that we go with the component-based approach for now

Agree

It'd be fun to work on this together if you have time

Sure. I'll try to find a suitable time slot for both of us.

[ConradIrwin]

If I understand you correctly, this part slightly contradicts the idea with is_subset_of(). To make this scenario happen, we need to hold cmd after cmd+p, then add ctrl to it, then release cmd, then press 1, ending up with ctrl-1 (that's quite an excercise even for a seasoned pianist :D ). I would expect the file finder to close after releasing cmd, and this is how other editors behave (VSCode, JB's). But you expect it to keep the file finder open and perform the ctrl-1-bound action. Am I correct?

I was meaning in the tab switcher (so ctrl-tab...ctrl-2). In VSCode cmd-p...cmd-2 switches to the second split which is a bit odd (but we'd probably end up with the same "feature" if we added cmd-2 at the workspace level).

[ConradIrwin]

@alygin do you want to push up the work we did today to this branch or close this one and send another?

[alygin]

@alygin do you want to push up the work we did today to this branch or close this one and send another?

I've added tests for common use cases and pushed it here. Thanks for your help!

Final implementation:

• Div now has on_modifiers_changed() method.
• As a practical use case, the File Finder uses this method to open a selected file when shortcut modifiers are released. For instance, cmd-p cmd-p switches to the previously opened file, while a single cmd-p initiates searching by text.

[ConradIrwin]

Thanks!

[maxbrunsfeld]

@alygin I need to revert this because it causes a problem for another common use case for the file finder:

• Open the file finder with cmd-p
• Navigate up and down with ctrl-p/ctrl-n, which are standard bindings for up and down on macOS
• Release the ctrl key
• 💥 The selected file is opened unexpectedly

I think we need to make the modifiers changed listener a bit more selective in some way, so that it works for the "release cmd-p" use case, but does not spuriously fire in this case.

[alygin]

Fixed: #9785