Add complete_info() #3866

Shougo · 2019-01-26T11:34:51Z

I have added complete_mode() API.
It returns current ctrl_x mode as string.

It is useful for auto completion plugin.

codecov-io · 2019-01-26T11:53:37Z

Codecov Report

Merging #3866 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3866      +/-   ##
==========================================
- Coverage   79.22%   79.22%   -0.01%     
==========================================
  Files         105      105              
  Lines      141158   141166       +8     
==========================================
+ Hits       111837   111842       +5     
- Misses      29321    29324       +3

Impacted Files	Coverage Δ
src/edit.c	`86.18% <100%> (+0.05%)`	⬆️
src/evalfunc.c	`88.7% <100%> (ø)`	⬆️
src/if_xcmdsrv.c	`83.48% <0%> (-0.54%)`	⬇️
src/ex_cmds.c	`81.63% <0%> (-0.1%)`	⬇️
src/os_unix.c	`58.98% <0%> (-0.1%)`	⬇️
src/ui.c	`50.14% <0%> (-0.08%)`	⬇️
src/gui.c	`58.1% <0%> (-0.06%)`	⬇️
src/gui_gtk_x11.c	`48.37% <0%> (-0.05%)`	⬇️
src/terminal.c	`79% <0%> (-0.05%)`	⬇️
src/ex_docmd.c	`80.6% <0%> (+0.01%)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cbef8e1...7291887. Read the comment docs.

dpelle · 2019-01-26T11:47:58Z

src/edit.c

+    {
+	case 0:
+	    // Keyword completion
+	    mode = strdup("keyword");


Vim does not use strdup() which is not portable. It uses vim_strsave() instead.

dpelle · 2019-01-26T11:49:25Z

src/testdir/test_popup.vim

+  endfor
+  call delete('dummy.txt')
+
+  function! s:complTestEval() abort


Vim tests no longer uses the exclamation mark in "function!" but uses "func" … "endfunc".

dpelle · 2019-01-26T11:52:16Z

src/testdir/test_popup.vim

+    call complete(1, ['source', 'soundfold'])
+    return ''
+  endfunction
+  inoremap <F6>  <c-r>=s:complTestEval()<CR>


The test has a side-effect: it leaks this map. We should do iunmap before exiting the test.

dpelle · 2019-01-26T11:53:22Z

src/testdir/test_popup.vim

@@ -896,4 +896,48 @@ func Test_menu_only_exists_in_terminal()
  endtry
 endfunc

+func Test_popup_complete_mode()
+  new
+  inoremap <f5> <c-r>=complete_mode()<cr>


The test has a side-effect: it leaks this map. We should do iunmap before exiting the test.

Or put <buffer>.

Oh, it seems better.

dpelle · 2019-01-26T11:54:32Z

src/testdir/test_popup.vim

+  inoremap <f5> <c-r>=complete_mode()<cr>
+  call writefile([
+        \ '!rm	src/testdir/amiga.vim	/^cmap !rm !Delete all$/;"	m',
+        \], 'dummy.txt')


In general, temporary files created by Vim tests start with X presumably because it makes it easier to spot temporary files which were not removed.

dpelle · 2019-01-26T11:55:37Z

runtime/doc/eval.txt

+							*complete_mode()*
+complete_mode()
+		Return a string that indicates the current completion mode.
+		Note: It does not check popup menu is visible.


It does not check whether popup menu is visible

Shougo · 2019-01-26T12:05:41Z

I have fixed for reviews.

mattn · 2019-01-26T12:25:18Z

Totally, char_u* should be used instead of char* ?

Shougo · 2019-01-26T12:31:53Z

OK. I have changed.

brammool · 2019-01-26T14:13:20Z

I have added `complete_mode()` API. It returns current ctrl_x mode as string. It is useful for auto completion plugin.

I wonder if other completion state would be useful. I rather have one function to get the whole state, than a bunch of functions for several pieces of state. Unfortunately, the completion state is very complex, it's not easy to even explain what each state variable means. I've been meaning to refactor this, but it's a complex thing. About the implementation: Instead of using a big switch, it would be much more compact to use a table with the completion types and string. Something like: struct { int mode; char *name; } mode_table = { {0, "keyword"}, {CTRL_X_FILES, "files"}, }; +// complete_mode() implementation. +char_u *ins_compl_mode(void) +{ This should use Vim style function header.

…

-- 'I generally avoid temptation unless I can't resist it." -- Mae West /// Bram Moolenaar -- Bram@Moolenaar.net -- http://www.Moolenaar.net \\\ /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\ \\\ an exciting new programming language -- http://www.Zimbu.org /// \\\ help me help AIDS victims -- http://ICCF-Holland.org ///

Shougo · 2019-01-27T10:52:17Z

Thank you for the review.
I have changed the implementation to use table.

brammool · 2019-01-27T14:00:56Z

Thank you for the review. I have changed the implementation to use table.

It's a bit simpler this way: for (i = 0; i < sizeof(table)/ sizeof(struct mode_table); i++) if (table[i].mode == ctrl_x_mode) return (char_u *)table[i].name; return (char_u *)"unknown"; Please use Vim code style.

…

-- MORTICIAN: What? CUSTOMER: Nothing -- here's your nine pence. DEAD PERSON: I'm not dead! MORTICIAN: Here -- he says he's not dead! CUSTOMER: Yes, he is. DEAD PERSON: I'm not! The Quest for the Holy Grail (Monty Python) /// Bram Moolenaar -- Bram@Moolenaar.net -- http://www.Moolenaar.net \\\ /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\ \\\ an exciting new programming language -- http://www.Zimbu.org /// \\\ help me help AIDS victims -- http://ICCF-Holland.org ///

Shougo · 2019-01-28T00:10:29Z

OK. I have fixed it.

h-east · 2019-01-28T00:20:25Z

It does not seem to be reflected here.
https://groups.google.com/d/msg/vim_dev/6utE1ObSYY8/fOaLtsBUGwAJ

Shougo · 2019-01-28T22:57:25Z

Oh, I have not read your response.

I have included (and some changed) your patch.

mattn · 2019-01-29T00:27:19Z

src/edit.c


-    return (char_u *)"unknown";
+    return (char_u *)mode;
 }


if (ctrl_x_mode == CTRL_X_NOT_DEFINED_YET || compl_started) return mode_names[ctrl_x_mode & ~CTRL_X_WANT_IDENT]; return (char_u *)"";

h-east · 2019-01-29T00:34:00Z

It is better to overwrite by my patch as is.
Check the contents of my patch more.

I add to the document, add test cases, simplify the test file (Xdummy.txt), add code comment, and move the definition position of the function (ins_compl_mode) to the appropriate place.

Or it may be better for Bram to include my patch :)

h-east

I think your arrangement is subtle.
So, did you decide that this is unnecessary?

diff --git a/runtime/doc/insert.txt b/runtime/doc/insert.txt
index 0431043c2..2bb357ef4 100644
--- a/runtime/doc/insert.txt
+++ b/runtime/doc/insert.txt
@@ -642,6 +642,7 @@ and one of the CTRL-X commands.  You exit CTRL-X mode by typing a key that is
 not a valid CTRL-X mode command.  Valid keys are the CTRL-X command itself,
 CTRL-N (next), and CTRL-P (previous).
 
+To get the current completion mode, |complete_mode()| can be used.
 Also see the 'infercase' option if you want to adjust the case of the match.
 
 							*complete_CTRL-E*

h-east · 2019-02-02T11:19:36Z

src/edit.c

@@ -3589,6 +3590,36 @@ ins_compl_active(void)
    return compl_started;
 }

+// Return Insert completion mode name string


/* * Return Insert completion mode name string */

Why do you get worse?
It has already been pointed out by Bram.
See the document.

:h style-examples /function_name

It has already been pointed out by Bram.

Oh, I have not see it in the issue.
It is pointed in ML thread?

h-east · 2019-02-02T11:21:42Z

runtime/doc/eval.txt

+		   "files"	File names |i_CTRL-X_CTRL-F|
+		   "tags"	Tags |i_CTRL-X_CTRL-]|
+		   "path_defines"
+				Definition completion |i_CTRL-X_CTRL-D|


Why did you change it? The way it does not break is easier to see.

h-east · 2019-02-02T11:21:59Z

runtime/doc/eval.txt

+		   "path_defines"
+				Definition completion |i_CTRL-X_CTRL-D|
+		   "path_patterns"
+		   		Include completion |i_CTRL-X_CTRL-I|


Same above.

h-east · 2019-02-02T11:22:46Z

src/proto/edit.pro

@@ -20,6 +20,7 @@ char_u *find_word_end(char_u *ptr);
 int ins_compl_active(void);
 int ins_compl_add_tv(typval_T *tv, int dir);
 void ins_compl_check_keys(int frequency, int in_compl_func);
+char_u *ins_compl_mode(void);


This is after int ins_compl_active(void);

h-east · 2019-02-02T11:28:02Z

src/testdir/test_popup.vim

+  for [key, expected] in [
+        \ ["", ''],
+        \ ["\<C-X>", 'ctrl_x'],
+        \ ["\<C-X>\<C-N>", 'keyword'],


Test case is missing.
\ ["\<C-X>\<C-P>", 'keyword'],

I have added it.

h-east · 2019-02-02T11:28:50Z

src/edit.c

+    };
+
+    if (ctrl_x_mode == CTRL_X_NOT_DEFINED_YET || compl_started)
+	return mode_names[ctrl_x_mode & ~CTRL_X_WANT_IDENT];


Need a cast (char_u *).

prabirshrestha · 2019-03-08T00:31:04Z

I would also be interested in this patch.

I do see Bram's comment on

I wonder if other completion state would be useful. I rather have one
function to get the whole state, than a bunch of functions for several
pieces of state.

One option would be to have complete_info() that returns the following. Feel free to suggest better names.

{
    "mode": "omni",
    "items": [],
    "selected": -1, // index of items
    "height": 10,
    "width": 20
}

For start we could only return {"mode": "omni" } and add others as needed. We can add docs that says to check of key before accessing it.

Shougo · 2019-03-10T06:34:24Z

I have renamed complete_mode() to complete_info().

Patch to add complete_mode(). Shougo - #3866. Alternate patch by Hirohito
Higashi, 2019 Jan 27, included now?

@brammool I have already included Higashi's patch.

h-east · 2019-03-10T14:44:59Z

@Shougo
Do NOT use my patch.
I'll Pull Request for a few days

--
Best regards,
Hirohito Higashi (h_east)

prabirshrestha · 2019-03-10T18:26:38Z

runtime/doc/eval.txt

@@ -2267,6 +2267,7 @@ col({expr})			Number	column nr of cursor or mark
 complete({startcol}, {matches}) none	set Insert mode completion
 complete_add({expr})		Number	add completion match
 complete_check()		Number	check for key typed during completion
+complete_info()			String	get current completion information


Type should be changed to dictionary

prabirshrestha · 2019-03-10T18:26:53Z

runtime/doc/eval.txt

@@ -3536,6 +3537,36 @@ complete_check()				*complete_check()*
 		Only to be used by the function specified with the
 		'completefunc' option.

+							*complete_info()*
+complete_info()
+		Return a string that indicates the current completion


dictionary instead of string

mattn · 2019-03-11T00:04:56Z

I think this is enough to merge.

prabirshrestha · 2019-03-12T05:00:45Z

Verified that this patch works for asyncomplete v2 to ignore processing autocomplete if it is open due to vim's builtin completions.

Shougo · 2019-03-18T02:26:43Z

#4106 (comment)

I sent a patch that was almost completely rewritten on # 3866.
Despite the reviews I have repeatedly made, he kept subtle.
(and he has never said thanks-word)

OK. Thank you for your review.
I know your patch is better.

But you have said in other place "Your patch implementation is very bad. My patch should be included instead".
So I was stubborn sorry.

I will close the PR.

Note: He have blocked me in github. So I cannot comment the issue.

Shougo mentioned this pull request Jan 26, 2019

[WIP] Add complete_mode() neovim/neovim#9536

Closed

dpelle reviewed Jan 26, 2019

View reviewed changes

mattn reviewed Jan 29, 2019

View reviewed changes

h-east reviewed Feb 2, 2019

View reviewed changes

andymass mentioned this pull request Mar 7, 2019

add a way to detect the popup was opened by ctrl-x ctrl-f #4083

Closed

This was referenced Mar 8, 2019

[RFC]: add MenuPopupChanged autocmd neovim/neovim#9616

Merged

v2 prabirshrestha/asyncomplete.vim#124

Merged

Shougo added 11 commits March 10, 2019 15:08

Add complete_mode()

3e6dae7

Fix tests

75b3af8

Fix for reviews

238d2e6

Fix for reviews

b16c283

Use char_u instead

4c81fba

Use <buffer>

34a5ddd

Use table instead of big switch

d6da293

Fix the code style

ad655c9

Use Vim style

f89137f

Include @h-east's patch

061f618

Fix for mattn's review

10e29fc

Shougo added 3 commits March 10, 2019 15:08

Include other @h_east's patch

88a0383

Fix for reviews

b665ef5

Rename complete_mode() to complete_info()

7291887

Shougo force-pushed the complete_mode branch from b5b176d to 7291887 Compare March 10, 2019 06:33

prabirshrestha reviewed Mar 10, 2019

View reviewed changes

Shougo changed the title ~~Add complete_mode()~~ Add complete_info() Mar 10, 2019

Shougo changed the title ~~Add complete_info()~~ Add complete_info() Mar 10, 2019

Fix the documentation

a4be99e

h-east mentioned this pull request Mar 12, 2019

Add complete_info() function #4106

Closed

Shougo closed this Mar 18, 2019

Shougo deleted the complete_mode branch March 18, 2019 02:26

Uh oh!

Add complete_info() #3866

Add complete_info() #3866

Uh oh!

Conversation

Shougo commented Jan 26, 2019

Uh oh!

codecov-io commented Jan 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shougo commented Jan 26, 2019

Uh oh!

mattn commented Jan 26, 2019

Uh oh!

Shougo commented Jan 26, 2019

Uh oh!

brammool commented Jan 26, 2019 via email

Uh oh!

Shougo commented Jan 27, 2019

Uh oh!

brammool commented Jan 27, 2019 via email

Uh oh!

Shougo commented Jan 28, 2019

Uh oh!

h-east commented Jan 28, 2019

Uh oh!

Shougo commented Jan 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

h-east commented Jan 29, 2019

Uh oh!

h-east left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

h-east Feb 2, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

h-east Feb 2, 2019 • 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!

prabirshrestha commented Mar 8, 2019

Uh oh!

Shougo commented Mar 10, 2019

Uh oh!

h-east commented Mar 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Jan 26, 2019 •

edited

Loading

h-east Feb 2, 2019 •

edited

Loading

h-east Feb 2, 2019 •

edited

Loading