Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 6 commits into from

Conversation

arames
Copy link

@arames arames commented Mar 28, 2017

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
Copy link
Owner

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
Copy link
Author

arames commented Mar 29, 2017

Thanks for the comments. Will address them and update.

@martanne
Copy link
Owner

martanne commented Apr 4, 2017

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
Copy link
Author

arames commented Apr 4, 2017 via email

By default commands still execute for the active window. But the target window
can now be specified as an argument, both in the vis internal code and from Lua.
Use `vis_window_focus` instead.
To reproduce the issue addressed, start with an empty `visrc.lua`, and three
file with some content.
Open a file and set line numbers.  Then open two files with one split command
`:sp file_2 file_3`. The view options were only propagated to the last file
opened.
Until now, vis was focusing on a window before it was open.
Now the process is:
- A window is opened, and the `WIN_OPEN` event is fired.
- Some more configuration of the window may happen.
- Vis focuses on the window.

Note that it implies that when `WIN_OPEN` is fired, the active window is *not*
the newly created window. So lua code suscribing to the event may want to
specify the window as a parameter to `vis:command`.

    vis.events.subscribe(vis.events.WIN_OPEN, function(new_win)
      -- This command applies to the currently active window, *not* to the newly
      -- created window.
      vis:command('set numbers')
      -- This command applies to the newly created window.
      vis:command('set numbers', new_win)
    end)
This even is fired when a wndow gains focus, and can be acted
upon by the user like any other event.
This even is fired when the vis mode changes, and can be acted upon
by the user like any other event.
@arames
Copy link
Author

arames commented Apr 5, 2017

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.

@@ -1162,7 +1164,8 @@ enum SamError sam_cmd(Vis *vis, const char *s) {
break;
}
}
vis_mode_switch(vis, completed ? VIS_MODE_NORMAL : VIS_MODE_VISUAL);
if (!completed)
vis_mode_switch(vis, VIS_MODE_VISUAL);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not to sure about this. It passes the tests, but I did not take time to investigate fully.
There is a potential issue here with recursive events being fired on mode switch

arames added a commit to arames/vis-line-numbers that referenced this pull request Apr 5, 2017
@martanne
Copy link
Owner

martanne commented Apr 5, 2017

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
Copy link
Owner

martanne commented Apr 7, 2017

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
Copy link
Author

arames commented Apr 7, 2017

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
Copy link
Contributor

mcepl commented May 19, 2024

@rnpnr Is this PR still relevant at all?

@rnpnr
Copy link
Collaborator

rnpnr commented May 21, 2024

@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.

@rnpnr rnpnr closed this May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants