Skip to content

Conversation

rhysd
Copy link
Member

@rhysd rhysd commented Oct 14, 2016

Problem

When '--embed' passed to command line arguments, stdin and stdout are used for PRC. But when multiple files are also passed to the arguments, nvim wrongly sends 'N files to edit' message to its stdout. As the result, attaching to process from frontend failed.

Solution

Do not show the message on embedded mode.

@bfredl
Copy link
Member

bfredl commented Oct 14, 2016

Is this message really filling any purpose? Can't we just remove it completely?

@justinmk
Copy link
Member

@rhysd Nice catch! I looked around and did not find any other cases of printf or mch_msg. (And msg_use_printf() checks embedded_mode for all other cases)

I agree with @bfredl , we can remove this message entirely. It briefly appears before loading the UI.

@rhysd
Copy link
Member Author

rhysd commented Oct 14, 2016

@bfredl @justinmk

Oh, I see. I also agree with removing the message with my usage.

Problem:
    When '--embed' passed to command line arguments, stdin and stdout
    are used for PRC. But when multiple files are also passed to the
    arguments, nvim wrongly sends 'N files to edit' message to its
    stdout. As the result, attaching to process from frontend failed.

Solution:
    Remove the message because it doesn't fill any purpose.
@justinmk justinmk merged commit 79d77da into neovim:master Oct 14, 2016
@rhysd rhysd deleted the patch-2 branch October 14, 2016 16:16
@jamessan
Copy link
Member

jamessan commented Oct 14, 2016

Is this message really filling any purpose?

It tells the user how many files have been given on the command-line, which is relevant because if you don't visit all of them before doing :q then Vim will warn you before letting a subsequent :q actually quit.

Wouldn't it have been better to fix the actual problem of directly using printf instead?

@bfredl
Copy link
Member

bfredl commented Oct 14, 2016

@jamessan well I thought the message was unnecessary for that very reason, nvim will warn anyway on :q. And typically the user already know they passed more than one file, this message does nothing to tell how that's affecting :q behavior.

@justinmk
Copy link
Member

if you don't visit all of them before doing :q then Vim will warn you before letting a subsequent :q actually quit.

But that still works, which is why I didn't think the message before startup is necessary. If nvim starts quickly the user can't see the printf'd message anyway.

@jamessan
Copy link
Member

And typically the user already know they passed more than one file

Only if they directly invoked nvim, rather than a script doing so or feeding the output of a glob/command as the argument list.

But that still works, which is why I didn't think the message before startup is necessary.

I didn't imply that part didn't work. I meant that the indication ahead of time provides positive feedback about what is available to work with rather than negative feedback when trying to exit Vim.

However, you're right that it's not seen either way if the ui starts fast enough. That would imply that it really should have been a message inside of Vim, but that's not the case so I guess removal is the best option.

@justinmk
Copy link
Member

👍 to showing a message in Vim's message area.

justinmk added a commit to justinmk/neovim that referenced this pull request Oct 27, 2016
FEATURES:
0b5a7e4 neovim#4432 API: external UIs can render custom popupmenu
c6ac4f8 neovim#4934 API: call any API method from vimscript
31df051 neovim#4568 API: nvim_call_atomic(): multiple calls in a single request
b268ba3 neovim#5424 API: nvim_win_get_number(), nvim_tabpage_get_number()
e7e2844 has("nvim-1.2.3") checks for a specific Nvim version
522b885 neovim#5295, neovim#5493 `:CheckHealth` checks tmux, terminfo, performance
719dae2 neovim#5384 events: allow event processing in getchar()
f25797f neovim#5386 API: metadata: Nvim version & API level
22dfe69 neovim#5389 API: metadata: "since", "deprecated_since"
605e743 Added QuickFixLine highlight group

CHANGES:
4af6ec7 neovim#5253 perf: Disable clipboard in do_cmdline()
6e9f329 neovim#5299 perf: Skip foldUpdate() in insert-mode.
9d4fcec neovim#5426 perf: Do not auto-update folds for some foldmethods.
eeec0ca neovim#5419 tui: Default to normal-mode cursor shape.

FIXES:
e838452 neovim#5436 tui: Fix "weird characters" / "bleeding termcodes"
10a54ad neovim#5243 signal_init: Always unblock SIGCHLD.
bccb49b neovim#5316 eval.c: Fix memory leak for detached pty job
626065d neovim#5227 tchdir: New tab should inherit CWD.
cd321b7 neovim#5292 getcwd(): Return empty string if CWD is invalid.
6127eae shada: Fix non-writeable ShaDa directory handling
ca65514 neovim#2789 system(): Respect shellxescape, shellxquote
2daf54e neovim#4874 Restore vim-like tab dragging
0c536b5 neovim#5319 syntax.c: Support bg/fg special color-names.
3c53371 neovim#4972 from justinmk/schedule-ui_refresh
68bcb32 neovim#4789 tui.c: Do not wait for tui loop on teardown.
c8b6ec2 neovim#5409 v:count broken in command-line window
6bc3bce neovim#5461 fix emoji display
51937e1 neovim#5470 fix :terminal with :argadd, :argu
79d77da neovim#5481 external UIs: opening multiple files from command-line
657ba62 neovim#5501 rplugin: resolve paths in manifest file
6a6f188 neovim#5502 system('foo &', 'bar'): Show error, don't crash.
1ff162c neovim#5515 os_nodetype: open fd with O_NONBLOCK
2a6c5bb neovim#5450 modeline: Handle version number overflow.
0ade1bb neovim#5225 CI tests now run against Windows!
justinmk added a commit to justinmk/neovim that referenced this pull request Oct 28, 2016
FEATURES:
0b5a7e4 neovim#4432 API: external UIs can render custom popupmenu
c6ac4f8 neovim#4934 API: call any API method from vimscript
31df051 neovim#4568 API: nvim_call_atomic(): multiple calls in a single request
b268ba3 neovim#5424 API: nvim_win_get_number(), nvim_tabpage_get_number()
e7e2844 has("nvim-1.2.3") checks for a specific Nvim version
522b885 neovim#5295, neovim#5493 `:CheckHealth` checks tmux, terminfo, performance
719dae2 neovim#5384 events: allow event processing in getchar()
f25797f neovim#5386 API: metadata: Nvim version & API level
22dfe69 neovim#5389 API: metadata: "since", "deprecated_since"
605e743 Added QuickFixLine highlight group

CHANGES:
4af6ec7 neovim#5253 perf: Disable clipboard in do_cmdline()
6e9f329 neovim#5299 perf: Skip foldUpdate() in insert-mode.
9d4fcec neovim#5426 perf: Do not auto-update folds for some foldmethods.
eeec0ca neovim#5419 tui: Default to normal-mode cursor shape.

FIXES:
e838452 neovim#5436 tui: Fix "weird characters" / "bleeding termcodes"
10a54ad neovim#5243 signal_init: Always unblock SIGCHLD.
bccb49b neovim#5316 eval.c: Fix memory leak for detached pty job
626065d neovim#5227 tchdir: New tab should inherit CWD.
cd321b7 neovim#5292 getcwd(): Return empty string if CWD is invalid.
6127eae shada: Fix non-writeable ShaDa directory handling
ca65514 neovim#2789 system(): Respect shellxescape, shellxquote
2daf54e neovim#4874 Restore vim-like tab dragging
0c536b5 neovim#5319 syntax.c: Support bg/fg special color-names.
3c53371 neovim#4972 from justinmk/schedule-ui_refresh
68bcb32 neovim#4789 tui.c: Do not wait for tui loop on teardown.
c8b6ec2 neovim#5409 v:count broken in command-line window
6bc3bce neovim#5461 fix emoji display
51937e1 neovim#5470 fix :terminal with :argadd, :argu
79d77da neovim#5481 external UIs: opening multiple files from command-line
657ba62 neovim#5501 rplugin: resolve paths in manifest file
6a6f188 neovim#5502 system('foo &', 'bar'): Show error, don't crash.
1ff162c neovim#5515 os_nodetype: open fd with O_NONBLOCK
2a6c5bb neovim#5450 modeline: Handle version number overflow.
0ade1bb neovim#5225 CI tests now run against Windows!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants