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

[WIP] Add nvim_add_autocommand API #10844

Closed

Conversation

andreypopp
Copy link
Contributor

This PR adds new API nvim_add_autocommand to register autocommands in Lua by provinding a Lua callback.

@@ -2250,6 +2250,56 @@ Array nvim_get_proc_children(Integer pid, Error *err)
return rvobj;
}

Boolean nvim_add_autocommand(
uint64_t channel_id,
String event, String pat, DictionaryOf(LuaRef) opts, Error *err
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we'd like to use just plain callback as the last argument. We are missing API harness for LuaRef though right now.

char_u *next_ev;
LuaRef cb = LUA_NOREF;

event_T event_nr = event_name2nr((char_u *)event.data, &next_ev);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

next_ev is only needed for event_name2nr but not used here. Maybe we can have better API for converting names to event_T enum.

@@ -2250,6 +2250,56 @@ Array nvim_get_proc_children(Integer pid, Error *err)
return rvobj;
}

Boolean nvim_add_autocommand(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to support buffer local, once, multiple events.

AUGROUP_ALL
);

return retval != FAIL;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we do api_set_error here as well?

@@ -5427,7 +5429,11 @@ static void show_autocmd(AutoPat *ap, event_T event)
msg_col = 14;
if (got_int)
return;
msg_outtrans(ac->cmd);
if (ac->lua_cmd != LUA_NOREF) {
// TODO: what to print here?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to figure out what to print here.

XFREE_CLEAR(ac->cmd);
}
au_need_clean = true;
}

// Delete one command from an autocmd pattern.
static void au_del_cmd(AutoCmd *ac)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function was used only in a single place, I've figure out it's better w/o this indirection.

assert(ac->lua_cmd != LUA_NOREF);
Array args = ARRAY_DICT_INIT;
executor_exec_lua_cb(ac->lua_cmd, "autocommand", args, false);
if (ac->once) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's slightly different semantics here — with VimL we mark command as removed before executing but here we are marking it as removed after we execute the callback.

I think I'm confused how cleanup of aucmds works now.

Copy link
Member

Choose a reason for hiding this comment

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

It could make difference with both ++once and ++nested at the same time. I would suggest to "take" the reference, by saving ac->lua_cmd to a local varable and (when ++once ) setting ac->lua_cmd = LUA_NOREF before calling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, I've left code which frees lua ref in au_cleanup for "just in case" but my understanding is that it should free it at the moment of marking as deleted.

@marvim marvim added the WIP label Aug 25, 2019
@tjdevries
Copy link
Contributor

@andreypopp Do you have any desire to continue this PR? I think it would be a great feature to get merged.

@andreypopp
Copy link
Contributor Author

Unfortunately don’t have time to work on that right now, so if anyone could pick this up — that’d be great, the feature is very useful indeed.

@h-michael
Copy link
Contributor

If nobody else can I try it? However, I'm not so familiar with C, so it may take some time.

@tjdevries
Copy link
Contributor

@h-michael wait a week and see if I can finish it :) If I can't, ping me again and you should try. I have started a little bit of work on it today.

@tjdevries
Copy link
Contributor

Working on this in #12378 . Will complete there.

@tjdevries tjdevries closed this Jul 2, 2020
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.

5 participants