[RDY] New event: DirChanged #5928

mhinz · 2017-01-10T23:53:25Z

v:event.cwd and <afile> are set to the new directory
v:event.scope is set to either "global", "tab", "window", or "invalid" (which should never happen)

This is just a suggestion so far. What do you think?

Code
Documentation
Tests

justinmk · 2017-01-11T01:44:34Z

Might be worth referencing vim/vim#888 for ideas, e.g. the changes to apply_autocmds_group() and do_autochdir() as well as test stubs.

Alternatively I wonder if application of this event should live in vim_chdir() (and vim_chdirfile()). That also raises the question of whether we want the API function nvim_set_current_dir() to trigger this event. I have a feeling we need to answer this question sooner than later: "In general, should API functions trigger VimL autocmds?"

mhinz · 2017-01-11T13:18:49Z

Okay, I looked at the vim PR and added the commit that prevents expanding filenames during the execution of the autocmd.

About the handholding code in do_autocmd_event() I'm not so sure. Do we really want to add code that checks if a pattern given to autocmd DirChanged is either global, window, or tab? Currently we force * and put all information into v:event.

mhinz · 2017-01-12T14:12:06Z

Okay, now DirChanged gets triggered for any of the :cd commands and when setting 'autochdir'.

Tests and linters are green.

How do you like it so far?

justinmk · 2017-01-12T14:18:37Z

src/nvim/file_search.c

+  if (os_chdir((char *)dir) != 0) {
+    return FAIL;
+  }
+  apply_autocmd_dirchanged(dir, kCdScopeWindow);


It's confusing that this is done in vim_chdirfile but not vim_chdir.

Mostly because I'm not sure how to access the scope from within vim_chdir().

justinmk · 2017-01-12T14:20:48Z

test/functional/autocmd/dirchanged_spec.lua

+    end)
+
+    -- TODO(mhinz): Add more tests here if we start to make
+    --              use of the autocmd pattern.


To test the API, could use request("nvim_set_current_dir", ...), e.g. https://github.com/justinmk/neovim/blob/a85d116b1c4360da05ef1d6bc75e322b0b0b8e0d/test/functional/autocmd/bufenter_spec.lua#L16

justinmk · 2017-01-12T14:38:39Z

src/nvim/ex_docmd.c

+  case kCdScopeWindow:
+    snprintf(buf, sizeof buf, "window");
+    break;
+  default:


Omit default case, compiler will catch it.

I added it because clang gave me this warning:

warning: enumeration value 'kCdScopeInvalid' not handled in switch [-Wswitch] switch (scope) { ^

Other places in the code do this:

case kCdScopeInvalid: // Should never happen. assert(false);

They do that so that if new values for the enum get added you'll see the warning about it being unhandled instead of it being swallowed by the default case.

justinmk · 2017-01-12T14:41:47Z

src/nvim/ex_docmd.c

+  }
+
+  dict_add_nr_str(dict, "scope", 0L, (char_u *)buf);
+  dict_add_nr_str(dict, "path", 0L, new_dir);


Let's name this "cwd". for consistency with jobstart ().

justinmk · 2017-01-12T14:43:19Z

src/nvim/ex_docmd.c

+    snprintf(buf, sizeof buf, "tab");
+    break;
+  case kCdScopeWindow:
+    snprintf(buf, sizeof buf, "window");


I believe we always use parens for size of.

Still not a fan of sizeof(foo), because sizeof is not a function, even if its usage is similar. But I'll be nice. ;]

mhinz · 2017-01-12T15:45:51Z

Okay, I addressed all mentioned issues apart from the bit that apply_autocmd_dirchanged() should indeed be called from vim_chdir(), but I need to figure out the best way to propagate the scope to it or, even better, create a helper function gets the current scope by querying the current buffer/window/tabpage, to avoid duplicate code.

justinmk · 2017-01-12T15:57:19Z

test/functional/autocmd/dirchanged_spec.lua

+local request = h.request
+
+describe('DirChanged ->', function()
+  local wd = lfs.currentdir()


curdir would be more familiar name.

justinmk · 2017-01-12T15:59:08Z

test/functional/autocmd/dirchanged_spec.lua

+    end)
+
+    -- TODO(mhinz): Add more tests here if we start to make
+    --              use of the autocmd pattern.


This comment probably isn't needed, it's implied that changes should be accompanied by tests :)

justinmk · 2017-01-12T16:00:36Z

test/functional/autocmd/dirchanged_spec.lua

+
+  describe('"autocmd DirChanged', function()
+    it('*" sets v:event for all :cd variants', function()
+      execute('autocmd DirChanged * let event = copy(v:event)')


Globals should be prefixed with g:.

justinmk · 2017-01-12T16:00:50Z

test/functional/autocmd/dirchanged_spec.lua

+      execute('autocmd DirChanged * let event = copy(v:event)')
+
+      execute('lcd '..dirs[1])
+      eq({cwd=dirs[1], scope='window'}, eval('event'))


justinmk · 2017-01-12T16:03:26Z

test/functional/autocmd/dirchanged_spec.lua

+    end)
+  end)
+
+  describe('API ->', function()


Don't need nested describe blocks.

I thought it would be nicer for later additions to group everything logically right away.. maybe a bit overkill. :>

justinmk · 2017-01-12T16:05:40Z

test/functional/autocmd/dirchanged_spec.lua

+  end)
+
+  describe("'autochdir'", function()
+    it('triggers DirChanged', function()


Could just be

it("'autochdir' triggers DirChanged", ...

and then the nested describe isn't needed. The setup() should be in before_each() then.

justinmk · 2017-01-12T19:43:43Z

src/nvim/ex_docmd.c

+
+    if (vim_chdir(new_dir, scope)) {
+      EMSG(_(e_failed));
+    }


need an else{} block wrapping the stuff below, before the xfree call.

justinmk · 2017-01-12T19:46:40Z

src/nvim/types.h

+  kCdScopeGlobal,  ///< Affects the entire instance of Neovim.
+} CdScope;
+#define MIN_CD_SCOPE  kCdScopeWindow
+#define MAX_CD_SCOPE  kCdScopeGlobal


These should go in globals.h or vim.h, I think types.h only exists for unit tests and ultimately is something we want to eliminate. Its purpose was never clear.

justinmk · 2017-01-12T22:39:11Z

src/nvim/file_search.c

 {
  char_u *dir_name = find_directory_in_path(new_dir, STRLEN(new_dir),
                                            FNAME_MESS, curbuf->b_ffname);
  if (dir_name == NULL) {
    return -1;
  }
  int r = os_chdir((char *)dir_name);
+  apply_autocmd_dirchanged(dir_name, scope);


should not be applied if os_chdir fails

justinmk · 2017-01-12T22:40:48Z

test/functional/autocmd/dirchanged_spec.lua

+
+  before_each(clear)
+
+  it('"autocmd DirChanged *" sets v:event for all :cd variants', function()


need some tests that verify DirChanged is not published if :{t,l}cd fails (e.g. invalid directory)

Will it be good enough if I test that g:event didn't change with :cd in between or is there a better way to check that an event didn't fire?

That seems good. (:cd <invalid dir>)

justinmk · 2017-01-12T22:54:02Z

src/nvim/file_search.c

@@ -1522,6 +1523,35 @@ find_file_in_path_option (
  return file_name;
 }

+static void apply_autocmd_dirchanged(char_u *new_dir, CdScope scope)


Maybe do_autocmd_dirchanged is a better name. The precedent is do_autocmd_event() and yank_do_autocmd. The latter should probably be renamed to do_autocmd_textyankpost.

justinmk · 2017-01-12T22:56:44Z

src/nvim/file_search.c

+static void apply_autocmd_dirchanged(char_u *new_dir, CdScope scope)
+{
+  dict_T *dict = get_vim_var_dict(VV_EVENT);
+  char buf[8];


A check like the one in yank_do_autocmd would be worth adding here.

if (recursive || !has_event(EVENT_DIRCHANGED)) { // No autocommand was defined // or we changed directory from this autocommand. return; }

Speaking of which, let's prevent recursive DirChanged. (We can always add it later if it is needed for some reason)

justinmk · 2017-01-12T23:03:18Z

src/nvim/ex_docmd.c

-      post_chdir(scope);
+    if (vim_chdir(new_dir, scope)) {
+      EMSG(_(e_failed));
+      return;


Won't this cause a memory leak? the xfree(tofree) below will be missed. Better to keep the relevant part in an else{} block.

justinmk · 2017-01-13T03:25:25Z

src/nvim/ops.c

@@ -2538,18 +2538,18 @@ static void yank_copy_line(yankreg_T *reg, struct block_def *bd, size_t y_idx)
 ///
 /// @param oap Operator arguments.
 /// @param reg The yank register used.
-static void yank_do_autocmd(oparg_T *oap, yankreg_T *reg)
+static void do_autocmd_yank(oparg_T *oap, yankreg_T *reg)


Shouldn't it be do_autocmd_textyankpost, to be consistent with do_autocmd_dirchanged

justinmk · 2017-01-13T03:26:11Z

src/nvim/ops.c

  FUNC_ATTR_NONNULL_ALL
 {
-  static bool recursive = false;
+  static bool recursive_yank = false;


why change this? it's a local variable, the context is clear.

A static variable inside a function means that its scope is the function, but its lifetime is that of the entire program. Thus it shouldn't have such a general name to avoid name clashes in the future.

So, do_autocmd_textyankpost() will use recursive_textyankpost and do_autocmd_dirchanged() will use recursive_dirchanged.

Avoid name clashes with what? As you stated, the scope is the function, so do you expect this function to get another variable named recursive?

There you go, I just don't know C. I thought that "program-wide lifetime" equals "global namespace", too. :\

I'll change them back.

jamessan · 2017-01-13T12:48:07Z

src/nvim/ex_docmd.c

@@ -6952,7 +6952,7 @@ void free_cd_dir(void)
 /// Deal with the side effects of changing the current directory.
 ///
 /// @param scope  Scope of the function call (global, tab or window).


The new_dir parameter was added ... which isn't used?

jamessan · 2017-01-13T12:50:05Z

src/nvim/ops.c

  FUNC_ATTR_NONNULL_ALL
 {
  static bool recursive = false;

-  if (recursive || !has_event(EVENT_TEXTYANKPOST)) {
+  if (rerecursive || !has_event(EVENT_TEXTYANKPOST)) {


Typo rerecursive → recursive and on lines 2552, 2586.

🤦‍♂️

FEATURES: bc4a2e1 help, man.vim: "outline" (TOC) feature neovim#5169 58422f1 'guicursor' works in the TUI (and sends info to UIs) neovim#6423 129f107 api: nvim_get_mode() neovim#6247 0b59f98 api/ui: externalize tabline neovim#6583 bc6d868 'listchars': `Whitespace` highlight group neovim#6367 6afa7d6 writefile() obeys 'fsync' option neovim#6427 c60e409 eval.c refactor (also improves some error messages) neovim#5119 9d200cd getcompletion("cmdline") neovim#6376 2ea7bfc terminal: Support extra arguments in 'shell'. neovim#4504 bf51102 DirChanged autocmd neovim#5928 neovim#6262 1743df8 'cpoptions': "_" flag to toggle `cw` behaviour neovim#6235 22337b1 CTRL-R omits trailing ^M when pasting to cmdline neovim#6137 0e44916 :edit allows unescaped spaces in filename neovim#6119 abdbfd2 eval: Add id() function and make printf("%p") useful neovim#6095 bdfa147 findfile(), :find, gf work in :terminal. neovim#6009 2f38ed1 providers: Disable if `g:loaded_*` exists. b5560a6 setpos() can set lowercase marks in other buffers neovim#5753 7c513d6 Throttle :! output, pulse "..." message. neovim#5396 d2e8c76 v:exiting neovim#5651 :terminal improvements neovim#6185 neovim#6142 - cursor keeps position after leaving insert-mode. - 4ceec30 Follows output only if cursor is at end of buffer. - e7bbd35 new option: 'scrollback' - fedb844 quasi-support for undo and 'modifiable' - b45ddf7 disables 'list' by default - disables 'relativenumber' by default :help now contains full API documentation at `:help api`. man.vim saw numerous improvements. Windows support: - Windows is no longer "experimental", it is fully supported. - Windows package includes a GUI, curl.exe and other utilities. "Vim 8" features: partials, lambdas, packages. FIXES: 12fc1de ops: fix i<c-r> with multi-byte text neovim#6524 dd391bf Windows: system() and friends neovim#6497 13352c0 Windows: os_get_hostname() neovim#6413 16babc6 tui: Less-noisy mouse seqs neovim#6411 3a9dd13 (vim bug) folding edge-cases neovim#6207 f6946c6 job-control: set CLOEXEC on pty processes. neovim#5986 d1afd43 rplugin: Call s:LoadRemotePlugins() on startup. 1215084 backtick-expansion works with `shell=fish` neovim#6224 e32ec03 tui: Improved behavior after resize. neovim#6202 86c2adc edit.c: CTRL-SPC: Insert previously-inserted text. neovim#6090 c318d8e b:changedtick now follows VimL rules neovim#6112 34e24cb terminal: Initialize colors in reverse order neovim#6160 e889917 undo: Don't set b_u_curhead in ex_undojoin() neovim#5869 d25649f undo: :earlier, g-: Set b_u_seq_cur correctly. (neovim#6016) 043d8ba 'Visual-mode put from @. register' neovim#5782 42c922b open_buffer(): Do `BufEnter` for directories. 50d0d89 inccommand: Preview :sub commands only after delimiter neovim#5932 1420e10 CheckHealth improvements neovim#5519 c8d5e92 jobstart(): Return -1 if cmd is not executable. neovim#5671 CHANGES: NVIM_TUI_ENABLE_CURSOR_SHAPE was removed. Use 'guicursor' instead. See https://github.com/neovim/neovim/wiki/Following-HEAD#20170402 81525dc 'mouse=a' is no longer the default. (This will probably change again after it is improved.) neovim#6022 0c1f783 defaults: 'showcmd', 'belloff', 'ruler' neovim#6087 eb0e94f api: {get,set}_option update local options as appropriate neovim#6405 bdcb2a3 "Reading from stdin..." message was removed. neovim#6298

FEATURES: bc4a2e1 help, man.vim: "outline" (TOC) feature neovim#5169 58422f1 'guicursor' works in the TUI (and sends info to UIs) neovim#6423 129f107 api: nvim_get_mode() neovim#6247 0b59f98 api/ui: externalize tabline neovim#6583 bc6d868 'listchars': `Whitespace` highlight group neovim#6367 6afa7d6 writefile() obeys 'fsync' option neovim#6427 c60e409 eval.c refactor (also improves some error messages) neovim#5119 9d200cd getcompletion("cmdline") neovim#6376 2ea7bfc terminal: Support extra arguments in 'shell'. neovim#4504 bf51102 DirChanged autocmd neovim#5928 neovim#6262 1743df8 'cpoptions': "_" flag to toggle `cw` behaviour neovim#6235 22337b1 CTRL-R omits trailing ^M when pasting to cmdline neovim#6137 0e44916 :edit allows unescaped spaces in filename neovim#6119 abdbfd2 eval: Add id() function and make printf("%p") useful neovim#6095 bdfa147 findfile(), :find, gf work in :terminal. neovim#6009 2f38ed1 providers: Disable if `g:loaded_*` exists. b5560a6 setpos() can set lowercase marks in other buffers neovim#5753 7c513d6 Throttle :! output, pulse "..." message. neovim#5396 d2e8c76 v:exiting neovim#5651 :terminal improvements neovim#6185 neovim#6142 - cursor keeps position after leaving insert-mode. - 4ceec30 Follows output only if cursor is at end of buffer. - e7bbd35 new option: 'scrollback' - fedb844 quasi-support for undo and 'modifiable' - b45ddf7 disables 'list' by default - disables 'relativenumber' by default :help now contains full API documentation at `:help api`. man.vim saw numerous improvements. Windows support: - Windows is no longer "experimental", it is fully supported. - Windows package includes a GUI, curl.exe and other utilities. "Vim 8" features: partials, lambdas. SECURITY FIXES: CVE-2017-5953 CVE-2017-6349 CVE-2017-6350 neovim#6485 CHANGES: NVIM_TUI_ENABLE_CURSOR_SHAPE was removed. Use 'guicursor' instead. See https://github.com/neovim/neovim/wiki/Following-HEAD#20170402 81525dc 'mouse=a' is no longer the default. (This will probably change again after it is improved.) neovim#6022 0c1f783 defaults: 'showcmd', 'belloff', 'ruler' neovim#6087 eb0e94f api: {get,set}_option update local options as appropriate neovim#6405 bdcb2a3 "Reading from stdin..." message was removed. neovim#6298 FIXES: 12fc1de ops: fix i<c-r> with multi-byte text neovim#6524 dd391bf Windows: system() and friends neovim#6497 13352c0 Windows: os_get_hostname() neovim#6413 16babc6 tui: Less-noisy mouse seqs neovim#6411 3a9dd13 (vim bug) folding edge-cases neovim#6207 f6946c6 job-control: set CLOEXEC on pty processes. neovim#5986 d1afd43 rplugin: Call s:LoadRemotePlugins() on startup. 1215084 backtick-expansion works with `shell=fish` neovim#6224 e32ec03 tui: Improved behavior after resize. neovim#6202 86c2adc edit.c: CTRL-SPC: Insert previously-inserted text. neovim#6090 c318d8e b:changedtick now follows VimL rules neovim#6112 34e24cb terminal: Initialize colors in reverse order neovim#6160 e889917 undo: Don't set b_u_curhead in ex_undojoin() neovim#5869 d25649f undo: :earlier, g-: Set b_u_seq_cur correctly. (neovim#6016) 043d8ba 'Visual-mode put from @. register' neovim#5782 42c922b open_buffer(): Do `BufEnter` for directories. 50d0d89 inccommand: Preview :sub commands only after delimiter neovim#5932 1420e10 CheckHealth improvements neovim#5519 c8d5e92 jobstart(): Return -1 if cmd is not executable. neovim#5671

marvim added the RFC label Jan 11, 2017

mhinz force-pushed the autocmd/dir-changed branch from 4b2d8b0 to 24b9d50 Compare January 11, 2017 00:21

mhinz force-pushed the autocmd/dir-changed branch 2 times, most recently from fea80a7 to 5d0d350 Compare January 12, 2017 13:47

justinmk reviewed Jan 12, 2017

View reviewed changes

justinmk requested changes Jan 12, 2017

View reviewed changes

justinmk reviewed Jan 12, 2017

View reviewed changes

mhinz force-pushed the autocmd/dir-changed branch from 5d0d350 to 8b60767 Compare January 12, 2017 15:43

justinmk reviewed Jan 12, 2017

View reviewed changes

This was referenced Jan 12, 2017

Update the pwd on each request neovim/neovim-ruby#22

Merged

:pwd is not always the same as python os.getcwd() #3636

Closed

mhinz force-pushed the autocmd/dir-changed branch from 8b60767 to 07a4d49 Compare January 12, 2017 18:56

justinmk reviewed Jan 12, 2017

View reviewed changes

mhinz force-pushed the autocmd/dir-changed branch from 07a4d49 to 6f2d429 Compare January 12, 2017 21:36

justinmk reviewed Jan 12, 2017

View reviewed changes

mhinz force-pushed the autocmd/dir-changed branch from 6f2d429 to 0868a83 Compare January 13, 2017 02:05

justinmk reviewed Jan 13, 2017

View reviewed changes

mhinz force-pushed the autocmd/dir-changed branch 2 times, most recently from 8cf69da to bbca094 Compare January 13, 2017 10:15

mhinz force-pushed the autocmd/dir-changed branch from bbca094 to f2f01c0 Compare January 13, 2017 11:53

jamessan reviewed Jan 13, 2017

View reviewed changes

mhinz added 7 commits January 13, 2017 15:29

New event: DirChanged

bd80257

Don't expand filenames during autocmd

30f775f

Trigger DirChanged on 'autochdir'

197f50b

Add autocmd/dirchanged_spec.lua

a2f8ada

Move apply_autocmd_dirchanged() to vim_chdir()

a05779a

Document DirChanged

20867e6

Rename yank_do_autocmd() to do_autocmd_textyankpost()

1f7a119

mhinz force-pushed the autocmd/dir-changed branch from f2f01c0 to 1f7a119 Compare January 13, 2017 14:34

mhinz changed the title ~~[RFC] New event: DirChanged~~ [RDY] New event: DirChanged Jan 14, 2017

marvim added RDY and removed RFC labels Jan 14, 2017

justinmk approved these changes Jan 16, 2017

View reviewed changes

mhinz merged commit 340f79b into neovim:master Jan 16, 2017

mhinz deleted the autocmd/dir-changed branch January 16, 2017 12:36

zchee mentioned this pull request Jan 17, 2017

Fix cwd when loading server deoplete-plugins/deoplete-jedi#102

Closed

justinmk mentioned this pull request May 1, 2017

NVIM v0.2.0 #6634

Merged

mhinz mentioned this pull request May 3, 2017

news: version 0.2 neovim/neovim.github.io#151

Merged


		before_each(clear)

		it('"autocmd DirChanged *" sets v:event for all :cd variants', function()

[RDY] New event: DirChanged #5928

[RDY] New event: DirChanged #5928

Conversation

mhinz commented Jan 10, 2017 • edited Loading

justinmk commented Jan 11, 2017 • edited Loading

mhinz commented Jan 11, 2017 • edited Loading

mhinz commented Jan 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmk Jan 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhinz Jan 12, 2017 • edited Loading

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

mhinz Jan 12, 2017 • edited Loading

Choose a reason for hiding this comment

mhinz commented Jan 12, 2017

justinmk Jan 12, 2017 • edited Loading

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

justinmk Jan 12, 2017 • edited Loading

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

justinmk Jan 12, 2017 • edited Loading

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

mhinz Jan 13, 2017 • edited Loading

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

jamessan Jan 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhinz commented Jan 10, 2017 •

edited

Loading

justinmk commented Jan 11, 2017 •

edited

Loading

mhinz commented Jan 11, 2017 •

edited

Loading

mhinz commented Jan 12, 2017 •

edited

Loading

justinmk Jan 12, 2017 •

edited

Loading

mhinz Jan 12, 2017 •

edited

Loading

mhinz Jan 12, 2017 •

edited

Loading

justinmk Jan 12, 2017 •

edited

Loading

justinmk Jan 12, 2017 •

edited

Loading

justinmk Jan 12, 2017 •

edited

Loading

mhinz Jan 13, 2017 •

edited

Loading

jamessan Jan 13, 2017 •

edited

Loading