vim-repeat Extension #110

dhleong · 2016-05-17T13:51:10Z

This PR refactors some input handling for Extensions to be able to feed input in a vim-like way, enabling a vim-repeat-style interface for supporting repeat from extension functions. This is another that may not be merged right away, but I'm submitting to open the dialog about how best to go about this.

I introduce here a global InputQueue (should it rather be per-editor, like the TestInputModel?) so we can more closely simulate vim's execute command with input queued up and processed in-order. The queue accessed indirectly for inputKeyStroke and inputString (via ModalEntry and TestInputModel), so it is transparent to the Extensions.

Since we're not actually vim, it may not be necessary to support this sort of input feeding (we could instead save some executable object that performs the repeat) and indeed it does bring up some weirdness in implementation. Specifically, the VimRepeat class uses the InputQueue directly to perform a sort of lazy/delayed run through the input queue, but other plugins need to use the regular VimExtensionFacade#executeNormal. This interplay seems to work, but it might open the door to potential bugs in the future, if there are particularly complicated extensions.

I originally had the Facade call the InputQueue always so it would be more symmetrical, but this introduces a weird timing condition when repeating gcip, for example. Since gc triggers the "lazy" execution of g@, that g would execute before the i, and the @ would execute before the p, causing you to enter insert mode unexpectedly. As it is now, all the Extension's intended executions and execute atomically, with the rest of the enqueued input happening after, as expected. Again, though, it's easy to imagine a situation where this would not be sufficient.

Included is repeat support for surround and the proposed commentary extension from #109, with tests that ensure the repeating cleans up when another change command is executed. Future extensions should probably test their own repeat-ability.

Let me know what you think!

(This PR addresses VIM-1118)

AlexPl292 · 2019-02-21T11:25:38Z

Hi, @dhleong!
Thank you for such a long wait! We will be happy to review the pull request, but do you have the opportunity to update it in connection with the latest changes and resolve all conflicts?

dhleong · 2019-02-21T14:14:35Z

Sure, I'll get on it as soon as I can.

…

On Thu, Feb 21, 2019 at 6:25 AM Alex Plate ***@***.***> wrote: Hi, @dhleong <https://github.com/dhleong>! Thank you for such a long wait! We will be happy to review the pull request, but do you have the opportunity to update it in connection with the latest changes and resolve all conflicts? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#110 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAx0FoQ0-1Se3BE5p1b7Tb5ls_Gj2HTkks5vPoJFgaJpZM4IgTAa> .

dhleong · 2019-02-21T14:23:48Z

Rebased against master—gradle test passes!

AlexPl292 · 2019-02-21T14:59:41Z

I'm not yet started with deep review, but do I understand correctly that this PR contains #109 and it would be better to review #109 firstly?

dhleong · 2019-02-21T15:14:37Z

I believe so. It's been a while but that sounds right :)

jonahx · 2019-06-02T18:07:07Z

@AlexPl292 Hey Alex, Thanks for working on this. It's a great feature. Any ETA on when it will get merged?

AlexPl292 · 2019-06-03T08:32:11Z

Hi @jonahx !
We've just finished a large work on #186. So, we will continue the PR review after finishing some of our small internal things.

AlexPl292

HI @dhleong!
I was able to review this pr. Thank you for work!
I have some questions in comments, could you please check them?

Actually, the implementation looks pretty simple. But I still think a bit more about potential drawbacks.

This PR was made quite a long time ago. What are your plans for it? Do you have time/desire to continue it?

I introduce here a global InputQueue (should it rather be per-editor, like the TestInputModel?)

Well, I guess no. Repeat should work in other editors as well.

AlexPl292 · 2019-06-17T14:13:22Z

src/com/maddyhome/idea/vim/extension/repeat/VimRepeat.java

+    @Override
+    public void execute(@NotNull Editor editor, @NotNull DataContext context) {
+      Project proj = editor.getProject();
+      if (proj == null) return;


Is this check still necessary?

getProject seems to be nullable?

AlexPl292 · 2019-06-17T14:14:09Z

src/com/maddyhome/idea/vim/ui/InputQueue.java

+  public static void executeNormal(@NotNull List<KeyStroke> keys, @NotNull Editor editor) {
+    enqueue(keys);
+
+    final EditorDataContext context = new EditorDataContext(editor);


Don't you want to pass context from VimRepeat?

I think the idea was that InputQueue needn't be exclusively owned by VimRepeat. We could, however, optionally support providing the data context.

src/com/maddyhome/idea/vim/ui/InputQueue.java

AlexPl292 · 2019-06-17T14:18:20Z

src/com/maddyhome/idea/vim/ui/InputQueue.java

+      //  instance of handleKey, so that any mapping actions
+      //  can appropriately happen "before" the extra strokes.
+      //  NB: Multiple recursive mappings might still break....
+      new DrainTask(editor, context).run();


run(), not start(). Everything correct?

I believe so; it looks like the task enqueues itself if necessary.

AlexPl292 · 2019-06-17T14:22:25Z

src/com/maddyhome/idea/vim/ui/InputQueue.java

+
+    @Override
+    public void run() {
+      KeyStroke key = dequeue();


I'm just a bit scared about this "recursion". Are you sure that there are no cases when the queue will be populated during this execution? Probably we should add a flag that will "close" the queue while this run() is running?

I think it's by design that it can get populated during run. I suppose it would make sense to add some failsafe in case some malfunctioning plugin caused an infinite loop, however.

AlexPl292 · 2019-06-17T14:44:32Z

src/com/maddyhome/idea/vim/extension/repeat/VimRepeat.java

+  }
+
+  public static void set(Editor editor, List<KeyStroke> sequence) {
+    repeatChangeCommand =


I'm not sure that CommandState.getInstance(editor).getLastChangeCommand(); is a proper way. Plugins do execute some other actions during it's execition.

I was able to broke behaviour like this:

Yank and paste text

Dot to repeat

Execute cs"' extension

Dot to repeat

Or is it what are talking about in

public void run() { KeyStroke key = dequeue(); if (key != null) { KeyHandler.getInstance().handleKey(myEditor, key, myContext); // NB: this doesn't always work; take repeating gcip, for example: // handling the `c` will trigger `execute(g@)`; the `g` will // execute first, then this, then the `@`. final Application application = ApplicationManager.getApplication(); application.invokeLater(this); } } }

?

miguel550 · 2019-07-24T16:09:33Z

Waiting for this since 2016

Includes support for repeating `ys`, with a test. Required some refactoring of the old, simple `executeNormal` so a string of keys can be processed by the right things, in the right order.

This isn't a great fix, however. Having two different versions of `executeNormal` seems like it's just asking for bugs in the future. It is, however, required for gc(motion)---as mentioned in the comments, the extension's executeNormal calls will get interposed with the characters enqueued for repeating.

Change tag support is incomplete at best

This makes the inputKeyStroke and inputString code not have to know about the InputQueue at all, as it should be

dhleong · 2019-07-24T18:41:05Z

Hi @AlexPl292, sorry for the late response. I've been busy with other projects and this sort of fell off my radar. I'm certainly still interested in the feature, but I'm not certain if this is the best approach. I believe I had in mind that perhaps the InputQueue could later be more generally useful for change repeating (so, for example, repeating a change that introduced an import as a side-effect wouldn't cause the editor to hang while it tried to run that import again), but even if that were the case it may be simpler for plugins to handle repeating via a functional interface rather than by enqueuing keystrokes. I think this PR was sort of a proof of concept/RFC.

Well, I guess no. Repeat should work in other editors as well.

I agree, but I don't think a global queue precludes that. I believe the idea behind the global queue was that queued input can potentially operate across editors.

It's been a while so I'll have to look into some of your other comments with regards to breaking things, and some of the code comments.

AlexPl292 · 2019-12-12T10:09:00Z

Hi! Thank you for your contribution, but we've ended up with another approach of implementing it: 59f4dc8

AlexPl292 self-assigned this Feb 21, 2019

AlexPl292 added the Waiting for reply label Feb 21, 2019

AlexPl292 added Waiting for updates and removed Waiting for reply labels Feb 21, 2019

dhleong force-pushed the dhleong/RepeatExtension branch from e7c2d17 to 20ad816 Compare February 21, 2019 14:20

AlexPl292 removed the Waiting for updates label Feb 21, 2019

AlexPl292 self-requested a review February 21, 2019 14:36

dhleong force-pushed the dhleong/RepeatExtension branch from 20ad816 to 53fcb9b Compare March 28, 2019 12:49

AlexPl292 force-pushed the master branch from 7879c16 to fa9b193 Compare May 31, 2019 09:25

AlexPl292 reviewed Jun 17, 2019

View reviewed changes

dhleong added 5 commits July 24, 2019 14:18

Add experimental VimRepeat extension

05610f8

Includes support for repeating `ys`, with a test. Required some refactoring of the old, simple `executeNormal` so a string of keys can be processed by the right things, in the right order.

Add repeat for commentary extension, with test

728bf8b

Add some prelim support for surround delete & change

d20e248

Change tag support is incomplete at best

Move InputQueue delegation inside TestInputModel

988bb01

This makes the inputKeyStroke and inputString code not have to know about the InputQueue at all, as it should be

dhleong force-pushed the dhleong/RepeatExtension branch from 53fcb9b to 988bb01 Compare July 24, 2019 18:20

Remove unused InputQueue.insert() method

33cc0aa

dhleong mentioned this pull request Aug 26, 2019

Another attempt at comment motion extension #109

Merged

AlexPl292 closed this Dec 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vim-repeat Extension #110

vim-repeat Extension #110

dhleong commented May 17, 2016

AlexPl292 commented Feb 21, 2019

dhleong commented Feb 21, 2019 via email

dhleong commented Feb 21, 2019

AlexPl292 commented Feb 21, 2019

dhleong commented Feb 21, 2019

jonahx commented Jun 2, 2019

AlexPl292 commented Jun 3, 2019

AlexPl292 left a comment

AlexPl292 Jun 17, 2019

dhleong Jul 24, 2019

AlexPl292 Jun 17, 2019

dhleong Jul 24, 2019

AlexPl292 Jun 17, 2019

dhleong Jul 24, 2019

AlexPl292 Jun 17, 2019

dhleong Jul 24, 2019

AlexPl292 Jun 17, 2019

AlexPl292 Jun 17, 2019

miguel550 commented Jul 24, 2019

dhleong commented Jul 24, 2019

AlexPl292 commented Dec 12, 2019

vim-repeat Extension #110

vim-repeat Extension #110

Conversation

dhleong commented May 17, 2016

AlexPl292 commented Feb 21, 2019

dhleong commented Feb 21, 2019 via email

dhleong commented Feb 21, 2019

AlexPl292 commented Feb 21, 2019

dhleong commented Feb 21, 2019

jonahx commented Jun 2, 2019

AlexPl292 commented Jun 3, 2019

AlexPl292 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguel550 commented Jul 24, 2019

dhleong commented Jul 24, 2019

AlexPl292 commented Dec 12, 2019