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

[RFC] Add tab-local working directory to session file #6859

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@HiPhish
Contributor

HiPhish commented Jun 7, 2017

The ':tcd' command is the first tab-specific command written to the file and it is wrapped inside an 'if has('nvim')' block to keep the session file compatible with Vim.

There are no tests because I don't know how to test something like this. Some pointers would be appreciated.

@@ -8848,12 +8848,11 @@ makeopens (
tab_firstwin = firstwin; /* first window in tab page "tabnr" */
tab_topframe = topframe;
for (tabnr = 1;; ++tabnr) {

This comment has been minimized.

@mhinz

mhinz Jun 7, 2017

Member

Linter wants tabnr++ instead of ++tabnr here. (I know you didn't change it, but the linter looks at surrounding lines as well.)

int need_tabnew = false;
int cnr = 1;
if ((ssop_flags & SSOP_TABPAGES)) {
tabpage_T *tp = find_tabpage(tabnr);
if (tp == NULL)

This comment has been minimized.

@mhinz

mhinz Jun 7, 2017

Member

Linter wants:

if (tp == NULL) {
  break;
}
@mhinz

mhinz approved these changes Jun 7, 2017

Apart from the two linter nitpicks, this looks good to me.

@justinmk

This comment has been minimized.

Member

justinmk commented Jun 7, 2017

test/functional/legacy/092_mksession_cursor_cols_utf8_spec.lua tests session behavior by writing the session file somewhere and then manually verifying that some lines exist in the session file. It would be better to instead write the file, start a new test instance, and verify that the tab-local stuff was restored as expected.

In a new test file, test/functional/ex_cmds/mksession_spec.lua :

helpers.command('mksession! Xtest_session.vim')
helpers.command('qall!')
-- Create a new test instance of nvim.
local nvim2 = helpers.spawn(helpers.nvim_argv)
-- Henceforth all test utilities operate on this new instance.
helpers.set_session(nvim2)

helpers.command('source Xtest_session.vim')
-- Test that tab-local CWD was restored in the correct tabs.
...

see test/functional/api/server_requests_spec.lua for an existing usage of helpers.set_session() (which isn't related to "Vim sessions", it refers to the nvim test instance).

@justinmk

add test

@HiPhish

This comment has been minimized.

Contributor

HiPhish commented Jun 8, 2017

As it turns out the :mksession command will not restore a tab if the tab has no file in it. What is the best way of giving Nvim some files to work with? The contents of the files don't matter, it's just that there has to be some file in the window of the tab. This is not related to my PR, it was the same before as well.

@ZyX-I

This comment has been minimized.

Contributor

ZyX-I commented Jun 8, 2017

@HiPhish What’s the point of saving and restoring tabs with empty buffers? If you need certain layout it is better to write a VimL script which creates that certain layout then fight with :mksession whose purpose is to store editing sessions.

To the question: you can always open /dev/null, it gives EOF when somebody tries to read it thus counting as an empty file when opened in Neovim.

@HiPhish

This comment has been minimized.

Contributor

HiPhish commented Jun 8, 2017

@ZyX-I I thought :mksession was supposed to capture the state of the application, so it would make sense to restore empty tabs as well, even if it would be not of much use. Maybe my impression of :mksession was wrong. Anyway, using /dev/null as the file works, thank you.

@HiPhish

This comment has been minimized.

Contributor

HiPhish commented Jun 10, 2017

My test is failing on Windows:

Passed in:
(string) 'C:\projects\neovim\Xtest-functional-mksession_spec.tab'
Expected:
(string) 'C:\projects\neovim/Xtest-functional-mksession_spec.tab'

This is how I construct the expected path in Lua: cwd_dir .. '/' .. tab_dir. It looks like Windows wants \ to be the path separator, how do I construct portable directory paths in tests?

@ZyX-I

This comment has been minimized.

Contributor

ZyX-I commented Jun 10, 2017

@HiPhish fnamemodify('.', ':p')[-1] for VimL, package.config:sub(1) for lua (in tests they should give the same results unless you set &shellslash). Though first check helpers, it may have something needed.

@HiPhish

This comment has been minimized.

Contributor

HiPhish commented Jun 10, 2017

@ZyX-I Thank you, I found get_pathsep in helpers which does what you described. Hopefully this will work now. What is odd is that in cd_spec I used a literal / as well and my PR got merged :/ I guess Windows was not a concern back then?

describe('A session file', function()
-- The file name use for each session
local session_file = 'Xtest_session.vim'

This comment has been minimized.

@ZyX-I

ZyX-I Jun 10, 2017

Contributor

Temporary files normally have names like Xtest-functional-ex_cmds-mksession.vim.

This comment has been minimized.

@HiPhish

HiPhish Jun 10, 2017

Contributor

Done.

it('restores tab-local working directories', function()
local tab_dir = 'Xtest-functional-ex_cmds-mksession_spec.tab'
local cwd_dir = helpers.call('getcwd')
lfs.mkdir(tab_dir)

This comment has been minimized.

@ZyX-I

ZyX-I Jun 10, 2017

Contributor

This should go to before_each, removing directory to after_each, just as any other cleanup.

local session_file = 'Xtest-functional-ex_cmds-mksession.vim'
before_each(function()
helpers.clear()

This comment has been minimized.

@ZyX-I

ZyX-I Jun 10, 2017

Contributor

You should not use helpers.… anywhere, but just after local helpers: it is current convention that you “import” all entities you use via local clear = helpers.clear\nlocal call = helpers.call\n… (BTW, about call: call('getcwd') I normally express as funcs.getcwd(), it appears to also be more popular then [helpers.]call).

lfs.mkdir(tab_dir)
-- There need to be some files being edited, so we will use /dev/null
helpers.command('edit /dev/null')

This comment has been minimized.

@ZyX-I

ZyX-I Jun 10, 2017

Contributor

Is this working on Windows? Unlike creating layout for the test I would suggest to just pick two files like Xtest-functional-ex_cmds-mksession-tmpfile{1,2}. If I am not mistaking, they do not need to exist.

helpers.command('edit /dev/null')
helpers.command('tcd ' .. tab_dir)
helpers.command('tabfirst')
helpers.command('mksession! ' .. session_file)

This comment has been minimized.

@ZyX-I

ZyX-I Jun 10, 2017

Contributor

No bang, if session file exists then something is wrong: normally it should’ve been deleted by after_each of the previous test run.

-- Create a new test instance of Nvim, and henceforth all test
-- utilities operate on this new instance.
local nvim2 = helpers.spawn(helpers.nvim_argv)
helpers.set_session(nvim2)

This comment has been minimized.

@ZyX-I

ZyX-I Jun 10, 2017

Contributor

Just run clear in place of using three lines: two with nvim2 and one with qall!.

-- utilities operate on this new instance.
local nvim2 = helpers.spawn(helpers.nvim_argv)
helpers.set_session(nvim2)
local pathsep = helpers.get_pathsep() -- Needed for portable path strings

This comment has been minimized.

@ZyX-I

ZyX-I Jun 10, 2017

Contributor

This is used in one line only. Would be logical to just call get_pathsep() directly there.

|| ses_put_fname(fd, tp->tp_localdir, &ssop_flags) == FAIL
|| fputs(" | endif", fd) < 0
|| put_eol(fd) == FAIL) {
return FAIL;

This comment has been minimized.

@ZyX-I

ZyX-I Jun 10, 2017

Contributor

Looks like something too indented: 2-space indent should align return with space after if.

@HiPhish

This comment has been minimized.

Contributor

HiPhish commented Jun 10, 2017

I think all issues should be sorted out now.

lfs.mkdir(tab_dir)
end)
-- Each test should start with a clean session file

This comment has been minimized.

@ZyX-I

ZyX-I Jun 10, 2017

Contributor

Comment would be logical if cleanup was in before_each(), not here and with a single test only. Actually cleanup has two purposes: first, have a clean state for the next tests or the next test suite runs. Second, do not make various files appear in git status: current working directory of busted is a root directory of the repository, Xtest* files are not ignored.

And other tests do not have such comments.

local cwd_dir = funcs.getcwd()
-- There need to be some files being edited, so we will use dummy files
command('edit Xtest-functional-ex_cmds-mksession-tmpfile1')

This comment has been minimized.

@ZyX-I

ZyX-I Jun 10, 2017

Contributor

Better move to near other Xtest-… occurrences: in case somebody will want to move test file around this would be handy. Actually I would write the whole thing as

local file_prefix = 'Xtest-functional-ex_cmds-mksession'
local session_file = file_prefix .. '.vim'
--
local tab_dir = file_prefix .. '.d'
--
local tmpfile_base = file_prefix .. '-tmpfile'
@ZyX-I

ZyX-I approved these changes Jun 10, 2017

Other then last comments (rather minor compared to previous set of comments) looks OK.

describe('A session file', function()
-- The file name use for each session

This comment has been minimized.

@ZyX-I

ZyX-I Jun 11, 2017

Contributor

“Used”, not “use”.

HiPhish HiPhish
Add tab-local working directory to session file
The ':tcd' command is the first tab-specific command written to the file
and it is wrapped inside an 'if has('nvim')' block to keep the session
file compatible with Vim.
@justinmk

This comment has been minimized.

Member

justinmk commented Jun 11, 2017

Merged. Adjusted during the merge:

  • The commit message should include the related ticket.
  • Commit message should be prefixed with a scope (see CONTRIBUTING.md)
  • Remove verbose comments
  • Remove unnecessary newlines
  • :mksession is sufficient as the describe() message, and is consistent with other ex_cmds tests.

@justinmk justinmk closed this Jun 11, 2017

justinmk added a commit that referenced this pull request Jun 11, 2017

mksession: Restore tab-local working directory #6859
The ':tcd' command is the first tab-specific command written to the file
and it is wrapped inside an 'if has('nvim')' block to keep the session
file compatible with Vim.

Closes #6678

@HiPhish HiPhish deleted the HiPhish:session-tcd branch Jun 11, 2017

@mantognini

This comment has been minimized.

mantognini commented Jun 16, 2017

Thanks for this patch, highly appreciated! :-)

I noticed that it works fine as long as no terminal is open. Here is an example:

$ cd some/path
$ mkdir A B
$ echo "first" > A/first
$ echo "second" > B/second


$ nvim
:tcd A
:e first
:tabnew
:tcd ../B
:e second
:SaveSession test
:qa
$ nvim
:OpenSession test
// pwd in each tab prints the expected results!
:qa


$ nvim
:tcd A
:e first
:vs
:te fish
// shell reports pwd as expected: some/path/A
:tabnew
:tcd ../B
:e second
:SaveSession test
:qa
$ nvim
:OpenSession test
// no content in current buffer (content should be "second"), NERDTree says "no file for current buffer" but status line reads "B/second" and :pwd returns expected value (some/path/B)
:tabnext
// "first" is correctly open and populated
// shell is in wrong directory, but :pwd returns correct value

Do other people get the same unexpected results?

@justinmk

This comment has been minimized.

Member

justinmk commented Jun 16, 2017

Open a new issue

justinmk added a commit to justinmk/neovim that referenced this pull request Nov 8, 2017

NVIM v0.2.1
FEATURES:
0e873a3 Lua(Jit) built-in neovim#4411
5b32bce Windows: `:terminal` neovim#7007
7b0ceb3 UI/API: externalize cmdline neovim#7173
b67f58b UI/API: externalize wildmenu neovim#7454
b23aa1c UI: 'winhighlight' neovim#6597
17531ed UI: command-line coloring (`:help input()-highlight`) neovim#6364
244a1f9 API: execute lua directly from the remote api neovim#6704
45626de API: `get_keymap()` neovim#6236
db99982 API: `nvim_get_hl_by_name()`, `nvim_get_hl_by_id()` neovim#7082
dc68538 menu_get() function neovim#6322
9db42d4 :cquit : take an error code argument neovim#7336
9cc185d job-control: serverstart(): support ipv6 neovim#6680
1b7a9bf job-control: sockopen() neovim#6594
6efe84a clipboard: fallback to tmux clipboard neovim#6894
6016ac2 clipboard: customize clipboard with `g:clipboard` neovim#6030
3a86dd5 ruby: override ruby host via `g:ruby_host_prog` neovim#6841
16cce1a debug: $NVIM_LOG_FILE neovim#6827
0cba3da `:checkhealth` built-in, validates $VIMRUNTIME neovim#7399

FIXES:
105d680 TUI: more terminals, improve scroll/resize neovim#6816
cb912a3 :terminal : handle F1-F12, other keys neovim#7241
619838f inccommand: improve performance neovim#6949
04b3c32 inccommand: Fix matches for zero-width neovim#7487
60b1e8a inccommand: multiline, other fixes neovim#7315
f1f7f3b inccommand: Ignore leading modifiers in the command neovim#6967
1551f71 inccommand: fix 'gdefault' lockup neovim#7262
6338199 API: bufhl: support creating new groups neovim#7414
541dde3 API: allow K_EVENT during operator-pending
8c732f7 terminal: adjust for 'number' neovim#7440
5bec946 UI: preserve wildmenu during jobs/events neovim#7110
c349083 UI: disable 'lazyredraw' during ui_refresh. neovim#6259
51808a2 send FocusGained/FocusLost event instead of pseudokey neovim#7221
133f8bc shada: preserve unnamed register on restart neovim#4700
1b70a1d shada: avoid assertion on corrupt shada file neovim#6958
9f534f3 mksession: Restore tab-local working directory neovim#6859
de1084f fix buf_write() crash neovim#7140
7f76986 syntax: register 'Normal' highlight group neovim#6973
6e7a8c3 RPC: close channel if stream was closed neovim#7081
85f3084 clipboard: disallow recursion; show hint only once neovim#7203
8d1ccb6 clipboard: performance, avoid weird edge-cases neovim#7193
01487d4 'titleold' neovim#7358
01e53a5 Windows: better path-handling, separator (slash) hygiene neovim#7349
0f2873c Windows: multibyte startup arguments neovim#7060

CHANGES:
9ff0cc7 :terminal : start in normal-mode neovim#6808
032b088 lower priority of 'cursorcolumn', 'colorcolumn' neovim#7364
2a3bcd1 RPC: Don't delay notifications when request is pending neovim#6544
023f67c :terminal : Do not change 'number', 'relativenumber' neovim#6796
1ef2d76 socket.c: Disable Nagle's algorithm on TCP sockets neovim#6915
6720fe2 help: `K` tries Vim help instead of manpage neovim#3104
7068370 help, man.vim: change "outline" map to `gO` neovim#7405

@justinmk justinmk referenced this pull request Nov 8, 2017

Merged

NVIM v0.2.1 #7505

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment