[RDY] Add curdir as a default viewoption (vim-patch:8.0.128) #7447

EricR86 · 2017-10-27T15:33:24Z

The option enables the current local directory set by ":lcd" to be saved
to views which is the current default behaviour. The option can be
removed to disable this behaviour.

This addresses Issue #7435

This fix also addresses issues in other plugins:
vim-stay - Issue 10
fzf - Issue 1085

justinmk · 2017-10-27T17:04:08Z

👍

There's already a "curdir" option so "lcd" or "lcurdir" makes sense.

EricR86 · 2017-10-27T17:19:10Z

@justinmk, now that I think about it would "localcurdir" make more sense since "curdir" is the accepted nomenclature for "cd"?

justinmk · 2017-10-27T17:36:54Z

"curdir" is the accepted nomenclature for "cd"?

It isn't really. It's more a question of following the local pattern of the other flag names for this specific option. More generally across all options and the codebase there's no consistent nomenclature.

"localcurdir" is fine too.

EricR86 · 2017-10-27T18:59:30Z

So far the patch seems to work. I'm open for comments on naming, fixes, etc.

cc @kopischke @blueyed

blueyed · 2017-10-27T19:09:32Z

Awesome, thanks a lot!
Will give this a try.

kopischke · 2017-10-27T19:51:07Z

I concur with @blueyed: looks like an awesome improvement. Might be worth submitting upstream too.

blueyed · 2017-10-27T22:26:13Z

This is really nice.
Especially since it works out of the box with vim-stay's recommended set viewoptions=cursor,folds,slash,unix.

justinmk · 2017-10-28T13:21:56Z

Needs a test. Could use test/functional/ex_cmds/mksession_spec.lua as a template for a new file, test/functional/ex_cmds/mkview_spec.lua.

You can run the test locally like this:

TEST_FILE=test/functional/ex_cmds/mkview_spec.lua make functionaltest

justinmk · 2017-10-30T18:33:29Z

src/nvim/ex_docmd.c

-  if (wp->w_localdir != NULL) {
+
+  /* Do not save if the current flag is view options and and the local
+   * directory option not set set 


justinmk · 2017-10-30T18:35:00Z

test/functional/ex_cmds/mkview_spec.lua

+      end
+    end
+    lfs.rmdir(view_dir)
+    lfs.rmdir(local_dir)


helpers.rmdir() should be more robust / avoids the need for lines 26-31.

mhinz · 2017-11-02T14:21:41Z

test/functional/ex_cmds/mkview_spec.lua

+  after_each(function()
+    -- Remove any views created in the view directory
+    rmdir(view_dir)
+    lfs.rmdir(local_dir)


Forgot to remove the lfs? :)

No, either function would be fine. However it might actually be best to keep lfs in there since we are assuming the local_dir to be empty and lfs would error if it was not.

local_dir is only used for the ":lcd" option.

EricR86 · 2017-11-02T14:45:08Z

I have marked the PR as ready.

Notably, QuickBuild seems to have a single different (seemingly unrelated) test failure on different architectures every time I rebase (or --amend & --force). The changeset a39c8b7 on master where I rebased on to also was merged in with a different test failure from Quickbuild on a different architecture.

jamessan · 2017-11-11T17:35:36Z

Bram merged it upstream, but named it curdir. This does make more sense since there's also a curdir value for 'sessionoptions'.

Can you adapt to match upstream, remove the vim_diff.txt change, and include vim-patch:8.0.1289 in the commit message?

EricR86 · 2017-11-11T20:53:01Z

I'll change the code and option to match the patch Bram merged upstream. (ref: vim/vim#2316)

@jamessan Does vim-patch:8.0.128 need to be included somewhere specific in the commit message or should it be the commit message?

justinmk · 2017-11-11T20:57:34Z

Anywhere in the commit message is fine.

vim-patch:8.0.128 The option enables the current local directory set by ":lcd" to be saved to views which is the current default behaviour. The option can be removed to disable this behaviour.

justinmk · 2017-11-18T01:55:25Z

Thanks for updating. Patch number was 8.0.1289, I'll fix this during the merge.

@EricR86 @blueyed do we want the default to not include "curdir"? I suspect it won't break anything important. #7435 (comment) sounds like the current default behavior is rarely desired.

justinmk · 2017-11-18T11:03:24Z

Merged. Thanks @EricR86 !

The flag enables the current local directory set by ":lcd" to be saved to views which is the current default behaviour. The option can be removed to disable this behaviour. closes #7435 vim-patch:8.0.1289

FEATURES: a6de144 'viewoptions': add "curdir" flag neovim#7447 b6a603f node.js remote-plugin support neovim#7458 f5d4da0 :checkhealth : validate 'runtimepath' neovim#7526 FIXES: e6beb60 :terminal : fix crash on resize neovim#7547 f19e5d6 work around gnome-terminal memory leak neovim#7573 07931ed 'guicursor': use DECSCUSR for xterm-likes neovim#7576 f185c73 'os_open: UV_EINVAL on NULL filename' neovim#7561 e8af34d win: provider: Detect(): return *.cmd path neovim#7577 eacd788 :checkhealth : fix check for npm and yarn neovim#7569 a43a573 health.vim: normalize slashes for script path neovim#7525 69e3308 cmake: install runtime/rgb.txt d0b05e3 runtime: syntax error in `runtime/syntax/tex.vim` neovim#7518 55d8967 tutor: some fixes neovim#7510 CHANGES: 9837a9c remove legacy alias to `v:count` neovim#7407 c5f001a runtime: revert netrw update neovim#7557 67e4529 defaults: scrollback=10000 neovim#7556 881f9e4 process_close(): uv_unref() detached processes neovim#7539

blueyed · 2017-11-18T16:35:49Z

do we want the default to not include "curdir"?

I would vote for yes - but am not sure if it is worth to be incompatible with Vim there.
I think plugins like vim-stay etc could configure it automatically instead.

EricR86 · 2017-11-18T16:50:31Z

I have an obvious bias in terms of having the default viewoptions not including "curdir".
I cannot see a situation where the behavior of saving the local/window current directory to a view is desired but I'm sure if you change the default it'll end up breaking someone's workflow.

As @blueyed has mentioned, if someone is explicitly changing their viewoptions the "curdir" option will often be omitted anyway.

* 'master' of https://github.com/neovim/neovim: (148 commits) vim-patch:8.0.0283 version bump NVIM v0.2.2 tui: setrgbf/setrgbb: emit semicolons for VTE 'viewoptions': add "curdir" flag neovim#7447 win: provider: Detect(): return *.cmd path (neovim#7577) os_nodetype: rework os_open, os_stat: UV_EINVAL on NULL filename tui: 'guicursor': use DECSCUSR for xterm-likes (neovim#7576) lint neovim#7562 :checkhealth: fix check for npm and yarn (neovim#7569) doc: Fix pathshorten() example (neovim#7571) health.vim: define highlights as `default` (neovim#7560) runtime: revert netrw update (neovim#7557) defaults: scrollback=10000 (neovim#7556) doc: test/README.md: migrate wiki info (neovim#7552) vim-patch:8.0.0227 (neovim#7548) test/unit/path_spec: expect correct buffer size (neovim#7514) health.vim: normalize slashes for script path (neovim#7525) :terminal : fix crash on resize (neovim#7547) ...

marvim added the WIP label Oct 27, 2017

EricR86 changed the title ~~[WIP] Add lcd as a default viewoption~~ [RFC] Add lcd as a default viewoption Oct 27, 2017

marvim added RFC and removed WIP labels Oct 27, 2017

justinmk added this to the 0.2.2 milestone Oct 29, 2017

justinmk reviewed Oct 30, 2017

View reviewed changes

EricR86 force-pushed the viewoptions-lcd-default branch 3 times, most recently from 2a1a2c7 to 70248a0 Compare November 2, 2017 13:16

mhinz reviewed Nov 2, 2017

View reviewed changes

EricR86 changed the title ~~[RFC] Add lcd as a default viewoption~~ [RDY] Add lcd as a default viewoption Nov 2, 2017

marvim added RDY and removed RFC labels Nov 2, 2017

EricR86 changed the title ~~[RDY] Add lcd as a default viewoption~~ [WIP] Add lcd as a default viewoption Nov 11, 2017

marvim added WIP and removed RDY labels Nov 11, 2017

justinmk removed this from the 0.2.2 milestone Nov 13, 2017

justinmk added this to the 0.2.3 milestone Nov 13, 2017

Add curdir as a default viewoption

dc77539

vim-patch:8.0.128 The option enables the current local directory set by ":lcd" to be saved to views which is the current default behaviour. The option can be removed to disable this behaviour.

EricR86 force-pushed the viewoptions-lcd-default branch from 70248a0 to dc77539 Compare November 13, 2017 14:44

EricR86 changed the title ~~[WIP] Add lcd as a default viewoption~~ [WIP] Add curdir as a default viewoption (vim-patch:8.0.128) Nov 13, 2017

EricR86 changed the title ~~[WIP] Add curdir as a default viewoption (vim-patch:8.0.128)~~ [RDY] Add curdir as a default viewoption (vim-patch:8.0.128) Nov 13, 2017

marvim added RDY and removed WIP labels Nov 13, 2017

justinmk closed this Nov 18, 2017

justinmk removed the RDY label Nov 18, 2017

justinmk modified the milestones: 0.2.3, 0.2.2 Nov 18, 2017

EricR86 deleted the viewoptions-lcd-default branch November 18, 2017 17:21

ZyX-I mentioned this pull request Dec 5, 2017

Fix edge cases for NULL typval_T Lists #4615

Closed

30 tasks

Uh oh!

[RDY] Add curdir as a default viewoption (vim-patch:8.0.128) #7447

[RDY] Add curdir as a default viewoption (vim-patch:8.0.128) #7447

Uh oh!

Conversation

EricR86 commented Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmk commented Oct 27, 2017

Uh oh!

EricR86 commented Oct 27, 2017

Uh oh!

justinmk commented Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EricR86 commented Oct 27, 2017

Uh oh!

blueyed commented Oct 27, 2017

Uh oh!

kopischke commented Oct 27, 2017

Uh oh!

blueyed commented Oct 27, 2017

Uh oh!

justinmk commented Oct 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmk Oct 30, 2017

Choose a reason for hiding this comment

Uh oh!

EricR86 Oct 31, 2017

Choose a reason for hiding this comment

Uh oh!

justinmk Oct 30, 2017

Choose a reason for hiding this comment

Uh oh!

EricR86 Oct 31, 2017

Choose a reason for hiding this comment

Uh oh!

mhinz Nov 2, 2017

Choose a reason for hiding this comment

Uh oh!

EricR86 Nov 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EricR86 commented Nov 2, 2017

Uh oh!

jamessan commented Nov 11, 2017

Uh oh!

EricR86 commented Nov 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmk commented Nov 11, 2017

Uh oh!

justinmk commented Nov 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmk commented Nov 18, 2017

Uh oh!

blueyed commented Nov 18, 2017

Uh oh!

EricR86 commented Nov 18, 2017

Uh oh!

Uh oh!

EricR86 commented Oct 27, 2017 •

edited

Loading

justinmk commented Oct 27, 2017 •

edited

Loading

justinmk commented Oct 28, 2017 •

edited

Loading

EricR86 Nov 2, 2017 •

edited

Loading

EricR86 commented Nov 11, 2017 •

edited

Loading

justinmk commented Nov 18, 2017 •

edited

Loading