[vim/vim] Implement EVENT_DIRCHANGED and EVENT_DIRCHANGEDLOCAL and ... (#888)

[allen haim]

... fire autocmds on :cd and :lcd family of commands.

This is intended to fix #868.

Note that it only responds to user-initiated changes (:cd, :lcd, and synonyms); are there other ways the working directory can change that we should look out for?

---

You can view, comment on, or merge this pull request online at:

  https://github.com/vim/vim/pull/888

Commit Summary

• Implement EVENT_DIRCHANGED and EVENT_DIRCHANGEDLOCAL and fire autocmds on :cd and :lcd family of commands

File Changes

• M runtime/doc/autocmd.txt (11)
• M src/ex_docmd.c (6)
• M src/fileio.c (2)
• M src/vim.h (2)

Patch Links:

• https://github.com/vim/vim/pull/888.patch
• https://github.com/vim/vim/pull/888.diff

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

[toothpik]

On Mon, Jun 27, 2016 at 04:00:20PM -0700, allen haim wrote:
> ... fire autocmds on :cd and :lcd family of commands.

> This is intended to fix #868.

> Note that it only responds to user-initiated changes (:cd, :lcd, and
> synonyms); are there other ways the working directory can change that
> we should look out for?

have you covered the case where a user has 'autochdir' set and edits a
file on a different path?

[Allen Haim]

Thanks, good point. I've updated it to fire on :cd and :lcd (and synonyms) and also on autochdir.

Does it look better now?

There is now a single event 'DirChanged' which fires on these events.

('DirChangedLocal' from the first version didn't seem so useful any more).

[allen haim]

I'd be happy to write unit tests for this, but would first like to know if the approach is reasonable.

—
You are receiving this because you are subscribed to this thread.

Reply to this email directly, view it on GitHub, .

[Justin M. Keyes]

In src/ex_docmd.c:

> @@ -8928,6 +8928,10 @@ ex_cd(exarg_T *eap)
>  	    /* Echo the new current directory if the command was typed. */
>  	    if (KeyTyped || p_verbose >= 5)
>  		ex_pwd(eap);
> +
> +#ifdef FEAT_AUTOCMD
> +	    apply_autocmds(EVENT_DIRCHANGED, new_dir, new_dir, FALSE, curbuf);

The event "pattern" should say "global" or "window". expand('<afile>') can be used to get the directory name itself (though even that seems useless, because getcwd() should serve that purpose anyway).

apply_autocmds(EVENT_DIRCHANGED, "global", new_dir, FALSE, curbuf);

Advantages:

• removes the need for multiple event types.
• future-proof for when tab-local directories might be added in the future (Neovim has that already)

—
You are receiving this because you are subscribed to this thread.

Reply to this email directly, view it on GitHub, .

[Justin M. Keyes]

In src/ex_docmd.c:

> @@ -8928,6 +8928,10 @@ ex_cd(exarg_T *eap)
>  	    /* Echo the new current directory if the command was typed. */
>  	    if (KeyTyped || p_verbose >= 5)
>  		ex_pwd(eap);
> +
> +#ifdef FEAT_AUTOCMD
> +	    apply_autocmds(EVENT_DIRCHANGED, new_dir, new_dir, FALSE, curbuf);

Though I see you already removed DirChangedLocal, it could be useful for scripts to listen to only window-local directory changes. Though I cannot think of why.

autocmd DirChanged window ...

—
You are receiving this because you are subscribed to this thread.

Reply to this email directly, view it on GitHub, .

[allen haim]

In src/ex_docmd.c:

> @@ -8928,6 +8928,10 @@ ex_cd(exarg_T *eap)
>  	    /* Echo the new current directory if the command was typed. */
>  	    if (KeyTyped || p_verbose >= 5)
>  		ex_pwd(eap);
> +
> +#ifdef FEAT_AUTOCMD
> +	    apply_autocmds(EVENT_DIRCHANGED, new_dir, new_dir, FALSE, curbuf);

Hi, I don't think I agree that the pattern should be a keyword like "window" or "global". I find it more consistent with the other autocommands if it is a filename glob. Users can use '*' to match everything. Maybe we can think of another way to implement multiple event types if that's desired?

I'm also thinking that the third argument () should maybe just be NULL, because it's also confusing if is suddenly a directory.

And, I'm wondering if it should also check if the directory actually changed. Now it fires on each cd command, even if it didn't.

—
You are receiving this because you are subscribed to this thread.

Reply to this email directly, view it on GitHub, .

[Justin M. Keyes]

In src/ex_docmd.c:

> @@ -8928,6 +8928,10 @@ ex_cd(exarg_T *eap)
>  	    /* Echo the new current directory if the command was typed. */
>  	    if (KeyTyped || p_verbose >= 5)
>  		ex_pwd(eap);
> +
> +#ifdef FEAT_AUTOCMD
> +	    apply_autocmds(EVENT_DIRCHANGED, new_dir, new_dir, FALSE, curbuf);

I find it more consistent with the other autocommands if it is a filename glob.

The pattern is not always a filename (e.g. FileType autocommand), and it would be useless for this use-case.

The autocmd pattern is the primary way of selecting a subset of event messages to listen to.

—
You are receiving this because you are subscribed to this thread.

Reply to this email directly, view it on GitHub, .

[Bram Moolenaar]

I agree with Justin, using "global" or "window" works better.
I'l dropping the patch from the todo list until that's done and we have tests.

—
You are receiving this because you are subscribed to this thread.

Reply to this email directly, view it on GitHub, .

[allen haim]

@misterfish pushed 4 commits.

• 2a083fb Use "global" and "window" as the "file" pattern of the DirChanged
• e0b3609 Add feature guard around buffer allocation.
• 52b4f29 Don't try to expand the filename during DirChanged autocommand.
• 336063e Complain and return if the pattern for DirChanged autocommand is not

—
You are receiving this because you are subscribed to this thread.

View it on GitHub

[allen haim]

@misterfish pushed 1 commit.

• 86fc940 Fix typo double STRNCMP.

—
You are receiving this because you are subscribed to this thread.

View it on GitHub

[allen haim]

@misterfish pushed 1 commit.

• 0d1189a Update documentation for DirChanged autocommand.

—
You are receiving this because you are subscribed to this thread.

View it on GitHub

[allen haim]

@misterfish pushed 2 commits.

• f2c5e68 Fire 'window' version of DirChanged autocommand on autochdir.
• fab17e2 Match exact string length when checking DirChanged pattern.

—
You are receiving this because you are subscribed to this thread.

View it on GitHub

[allen haim]

@misterfish pushed 1 commit.

• 08f21c5 Use s: prefix for helper functions in unit test.

—
You are receiving this because you are subscribed to this thread.

View it on GitHub

[allen haim]

@misterfish pushed 1 commit.

• 3d4dd6d Fix explicit casts and compiler warnings.

—
You are receiving this because you are subscribed to this thread.

View it on GitHub

[allen haim]

@misterfish pushed 2 commits.

• bd0e3cf Add DirLocal to syntax file for vim.
• 3c11a91 Add unit tests for DirChanged events.

—
You are receiving this because you are subscribed to this thread.

View it on GitHub

[allen haim]

@misterfish pushed 1 commit.

• 64e3f26 Add (char_u *) cast to string literal.

—
You are receiving this because you are subscribed to this thread.

View it on GitHub

[Cimbali]

This could probably use some squashing, but is there any other reason it isn't merged yet?

[Andy Massimino]

I cleaned up this PR and made some small changes (removed unnecessary validation on autocmd pattern) andymass/vim@4e24de9 .

It differs from neovim's implementation since it does not set v:event. But their v:event seems completely useless since it just tells you getcwd() and global/window which is in <amatch>.

[allen haim]

Thanks for cleaning it up, Andy.

[Cimbali]

@misterfish If you can reset your branch to andymass/vim@4e24de9 and push it, that would update the PR. Otherwise a new one should be opened and this one closed I guess.

[allen haim]

@misterfish pushed 2 commits.

• 4e24de9 Add event: DirChanged
• 62296e6 Merge https://github.com/andymass/vim into 868-autocmd-cwd

—
You are receiving this because you are subscribed to this thread.

View it on GitHub

[allen haim]

Done.

[Bram Moolenaar]

Closed #888 via b7407d3.