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] terminal: handle &confirm and :confirm on unloading #8726

Merged
merged 2 commits into from Jul 12, 2018

Conversation

Projects
None yet
5 participants
@mhinz
Member

mhinz commented Jul 11, 2018

Show a proper confirmation dialog when trying to unload a terminal buffer while
the confirm option is set or when :confirm is used.

Fixes #4651

@KillTheMule

This comment has been minimized.

Contributor

KillTheMule commented Jul 11, 2018

Can you dump the screen in an array of strings and assert each one individually? For my plugin I've modified Screen:expect to not compare certain lines, maybe we need to make that more flexible so uses such as this are covered.

bool dialog_unload_terminal(buf_T *buf) {
char_u buff[DIALOG_MSG_SIZE];
dialog_msg(buff, _("Unload \"%s\"?"),

This comment has been minimized.

@bfredl

bfredl Jul 11, 2018

Member

Should we clarify "Kill and unload", or something like that?

This comment has been minimized.

@mhinz

mhinz Jul 11, 2018

Member

Or maybe "Closing", since that is a term we often use with terminals? Like TermClose etc.?

@marvim marvim added the RFC label Jul 11, 2018

@mhinz

This comment has been minimized.

Member

mhinz commented Jul 11, 2018

@KillTheMule The channel taught me about

-- any: true: Succeed if `expected` matches ANY screen line(s).
:)

I added screen tests.

@mhinz mhinz force-pushed the mhinz:terminal/confirmation branch 2 times, most recently from 7180d7c to 17117ca Jul 11, 2018

terminal: handle &confirm and :confirm on unloading
Show a proper confirmation dialog when trying to unload a terminal buffer while
the confirm option is set or when :confirm is used.

Fixes #4651

@mhinz mhinz force-pushed the mhinz:terminal/confirmation branch from 17117ca to 6dd6ff1 Jul 11, 2018

@KillTheMule

This comment has been minimized.

Contributor

KillTheMule commented Jul 11, 2018

Yeah I knew about that, but that's not what one wants most of the time, right? I mean, usually in tests there are some blank lines which makes this kind of assert kind of redundant.

(e) Oh wait, hahaha, I misunderstood, good one!

char_u buff[DIALOG_MSG_SIZE];
dialog_msg(buff, _("Close \"%s\"?"),
(buf->b_fname != NULL) ? buf->b_fname : (char_u *)_("Untitled"));

This comment has been minimized.

@justinmk

justinmk Jul 11, 2018

Member

just untranslated "?" is fine instead of "Untitled" (which is not common in vim, more like MS Word).

This comment has been minimized.

@mhinz

mhinz Jul 11, 2018

Member

I took that directly from dialog_changed():

neovim/src/nvim/ex_cmds2.c

Lines 1288 to 1290 in 9adb6ed

dialog_msg(buff, _("Save changes to \"%s\"?"),
(buf->b_fname != NULL) ?
buf->b_fname : (char_u *)_("Untitled"));

But I agree with the "?". It probably never happens that the current terminal buffer's name is NULL.

EMSG2(_("E89: %s will be killed(add ! to override)"),
(char *)buf->b_fname);
if (p_confirm || cmdmod.confirm) {
if (dialog_close_terminal(buf) == false) {

This comment has been minimized.

@justinmk

justinmk Jul 11, 2018

Member

we don't usually compare booleans explicitly like this, it's ok to use !.

@mhinz mhinz merged commit 01570f1 into neovim:master Jul 12, 2018

2 of 5 checks passed

QuickBuild Build pr-8726 finished with status FAILED
Details
codecov/patch 0% of diff hit (target 81.46%)
Details
codecov/project 69.97% (-11.5%) compared to 9f8bd77
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mhinz mhinz deleted the mhinz:terminal/confirmation branch Jul 12, 2018

@justinmk justinmk removed the RFC label Jul 12, 2018

justinmk added a commit that referenced this pull request Jul 18, 2018

NVIM v0.3.1
FEATURES:
07499a8 #8709 man.vim: C highlighting for EXAMPLES section
07f82ad #8699 TUI: urxvt: also send xterm focus-reporting seqs
40911e4 #8616 API: emit nvim_buf_lines_event from :terminal
c46997a #8546 fillchars: Add "eob" flag

FIXES:
74d19f6 #8576 startup: avoid blank stdin buffer if other files were opened
4874214 #8737 Only waitpid() for processes that we care about
cd6e7e8 #8743 Check all child processes for exit in SIGCHLD handler
c230ef2 #8746 channel.c: Prevent channel_destroy_early() from freeing uninitialized rpc stuff
0ed8b12 #8681 transstr_buf: fix length comparison
d241f27 #8708 TUI: Fix standout mode
9afed40 #8698 man.vim: fix for mandoc
e889640 #8682 provider/node: npm --loglevel silent
1cbc830 #8613 API: nvim_win_set_cursor: set curswant
bf6048e #8628 checkhealth: Python: fix VIRTUAL_ENV check
3cc3506 #8528 checkhealth: node.js: also search yarn

CHANGES:
b751449 #8619 defaults: shortmess+=F
1248178 #8578 highlight: high-priority CursorLine if fg is set.
01570f1 #8726 terminal: handle &confirm and :confirm on unloading
56065bb #8721 screen: truncate showmode messages
bf2460e #7551 buffer: fix copying :setlocal options
c1c14fa #8520 Ex mode: always "improved" (gQ)
050f397 #7992 options: remove 'maxcombine` option (always 6)

INTERNAL:
463da84 #7992 screen: use UTF-8 representation

UtkarshMe added a commit to UtkarshMe/neovim that referenced this pull request Jul 28, 2018

terminal: handle &confirm and :confirm on unloading (neovim#8726)
Show a proper confirmation dialog when trying to unload a terminal buffer while
the confirm option is set or when :confirm is used.

Fixes neovim#4651

UtkarshMe added a commit to UtkarshMe/neovim that referenced this pull request Jul 28, 2018

NVIM v0.3.1
FEATURES:
07499a8 neovim#8709 man.vim: C highlighting for EXAMPLES section
07f82ad neovim#8699 TUI: urxvt: also send xterm focus-reporting seqs
40911e4 neovim#8616 API: emit nvim_buf_lines_event from :terminal
c46997a neovim#8546 fillchars: Add "eob" flag

FIXES:
74d19f6 neovim#8576 startup: avoid blank stdin buffer if other files were opened
4874214 neovim#8737 Only waitpid() for processes that we care about
cd6e7e8 neovim#8743 Check all child processes for exit in SIGCHLD handler
c230ef2 neovim#8746 channel.c: Prevent channel_destroy_early() from freeing uninitialized rpc stuff
0ed8b12 neovim#8681 transstr_buf: fix length comparison
d241f27 neovim#8708 TUI: Fix standout mode
9afed40 neovim#8698 man.vim: fix for mandoc
e889640 neovim#8682 provider/node: npm --loglevel silent
1cbc830 neovim#8613 API: nvim_win_set_cursor: set curswant
bf6048e neovim#8628 checkhealth: Python: fix VIRTUAL_ENV check
3cc3506 neovim#8528 checkhealth: node.js: also search yarn

CHANGES:
b751449 neovim#8619 defaults: shortmess+=F
1248178 neovim#8578 highlight: high-priority CursorLine if fg is set.
01570f1 neovim#8726 terminal: handle &confirm and :confirm on unloading
56065bb neovim#8721 screen: truncate showmode messages
bf2460e neovim#7551 buffer: fix copying :setlocal options
c1c14fa neovim#8520 Ex mode: always "improved" (gQ)
050f397 neovim#7992 options: remove 'maxcombine` option (always 6)

INTERNAL:
463da84 neovim#7992 screen: use UTF-8 representation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment