Changes introducing a couple of events to enable the 'vis-line-numbers' plugin

[arames]

Hi there,

My goal was to create a 'vis-line-numbers' plugin to have the line numbers as I like them. See https://github.com/arames/vis-line-numbers.

Please let me know if the changes to vis look sensible.

[martanne]

A couple of comments:

• I'm not sure I like adding a Win parameter to the command execution function, instead we could add 2 events WIN_ENTER, WIN_LEAVE (following the vim terminology here). In the former vis.win would already point to the "new" window while in the latter it would still refer to the "old" one. In your plugin this would also obsolete your loop through all windows. You could just use vis:command.

• The mode change hunk you "had to remove to make it work" is required for commands like :x/pattern which select all occurrences of pattern and then switch to visual mode.

• You should never call obj_ref_free unless it is the last reference to an object you want to free/reset. This is not the case in these window focus events. It might also be the reason for the next point.

• Your changes didn't pass the CI tests (see also make test), meaning there is obviously something wrong.

• Your indentation and white space handling seems off at times. This is also apparent when looking at the "Files changed" Github tab. Please fix it.

Thanks.

[arames]

Thanks for the comments. Will address them and update.

[martanne]

What is the state of this? Did you encounter any problems, or are you just busy with other things? Should I take care of these changes?

[arames]

I have it almost done. I had to fix/modify a few things. I'll upload the updated changes with comments soon.

On Tue, Apr 4, 2017 at 12:47 AM Marc André Tanner [email protected] wrote:

What is the state of this? Did you encounter any problems, or are you just busy with other things? Should I take care of these changes?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/martanne/vis/pull/525#issuecomment-291421363, or mute the thread https://github.com/notifications/unsubscribe-auth/ADnYauWfe7P8UQrCuQFGfMhvKICihtPnks5rsfV8gaJpZM4Mrzjc .

[arames]

PTAL. We may need to discuss a few things.

Here are a few notes for the updated request:

• You may want to merge commits 2, 3, and 4 independently of the commits for the new events. Let me know if you want me to create a separate pull request.

• To pass the tests one needs to change WIN_OPEN to WIN_ENTER in test/vis/visrc.lua.

• I found issues around the ordering of WIN_OPEN and WIN_ENTER. WIN_OPEN must trigger before WIN_ENTER, otherwise actions taken into WIN_ENTER could be overridden by 'default' settings applied for WIN_OPEN. But that means that WIN_OPEN is now fired with the newly opened window not in focus, so it is not backward compatible with user Lua code. To be able to execute command on the window opened but not yet in focus, I had to keep the command('cmd', win) change.

[martanne]

I fixed up a few things and pushed the result to the events branch. It doesn't yet seem stable. I noticed a few crashes. I don't have time right now, to look further into it.

And yes, recursive events will probably need to be handled somehow. Otherwise users will shoot themselves in the foot.

I also moved the mode change event into mode_set, meaning it will for example also be called when repeating stuff with . or when specifying a count as in 2itext<Escape> which expands to itext<Escape>.

[martanne]

I rebased the branch. Not really happy with the implementation, but it should now work? Can you please try it?

For your use case you can probably ignore the WIN_OPEN event because the effect will be immediately overridden by the subsequent WIN_ENTER.

[arames]

One use case is broken.:

• Open a non-empty file.
• :sp

The new split should have relative line numbers, but with your changes it does not. The problem is that the UIOptions are copied from the original split after the event is fired. That is why I was splitting the window opening process into creation/configure/focus.

[mcepl]

@rnpnr Is this PR still relevant at all?

[rnpnr]

@rnpnr Is this PR still relevant at all?

It seems super out of date. The ideas here could be revisited in the future with fresh eyes but this stale version isn't much help.