[vim/vim] create popup_setbuf() function (PR #15026)
Christian Brabandt
related: #15006
Problem: cannot change buffer in a popup
Solution: add popup_setbuf() function
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/15026
Commit Summary
- 653e09f Problem: cannot change buffer in a popup
File Changes
(15 files)
- M runtime/doc/popup.txt (15)
- M runtime/doc/tags (1)
- M runtime/doc/usr_41.txt (1)
- M src/evalfunc.c (3)
- M src/popupwin.c (45)
- M src/proto/popupwin.pro (1)
- A src/testdir/dumps/Test_popup_beval_3.dump.failed (10)
- A src/testdir/dumps/Test_popup_setbuf_01.dump (10)
- A src/testdir/dumps/Test_popup_setbuf_02.dump (10)
- A src/testdir/dumps/Test_popup_setbuf_03.dump (10)
- A src/testdir/dumps/Test_popup_setbuf_04.dump (10)
- A src/testdir/dumps/Test_popup_setbuf_05.dump (10)
- A src/testdir/dumps/Test_popup_setbuf_06.dump (10)
- M src/testdir/test_popupwin.vim (51)
- M src/testdir/test_vim9_builtin.vim (5)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
@yegappan commented on this pull request.
In src/popupwin.c:
> @@ -2844,6 +2844,51 @@ f_popup_settext(typval_T *argvars, typval_T *rettv UNUSED)
popup_adjust_position(wp);
}
+/*
+ * popup_setbuf({id}, {bufnr})
+ */
+ void
+f_popup_setbuf(typval_T *argvars, typval_T *rettv UNUSED)
+{
+ int id;
+ win_T *wp;
+ buf_T *buf;
+
+ if (in_vim9script()
As this is a new function, this check for the Vim9 script can be removed. In the existing functions this check was added to not break backward compatibility with legacy scripts.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
@yegappan requested changes on this pull request.
In src/popupwin.c:
> +/*
+ * popup_setbuf({id}, {bufnr})
+ */
+ void
+f_popup_setbuf(typval_T *argvars, typval_T *rettv UNUSED)
+{
+ int id;
+ win_T *wp;
+ buf_T *buf;
+
+ if (in_vim9script()
+ && (check_for_number_arg(argvars, 0) == FAIL
+ || check_for_buffer_arg(argvars, 1) == FAIL))
+ return;
+
+ if (check_for_buffer_arg(argvars, 1) == FAIL)
If the previous comment is addressed, then this check can be removed.
In src/popupwin.c:
> + int id; + win_T *wp; + buf_T *buf; + + if (in_vim9script() + && (check_for_number_arg(argvars, 0) == FAIL + || check_for_buffer_arg(argvars, 1) == FAIL)) + return; + + if (check_for_buffer_arg(argvars, 1) == FAIL) + return; + + id = (int)tv_get_number(&argvars[0]); + wp = find_popup_win(id); + if (wp == NULL) + return;
Should the return value be set to -1 in this case?
In src/popupwin.c:
> + return; + + if (check_for_buffer_arg(argvars, 1) == FAIL) + return; + + id = (int)tv_get_number(&argvars[0]); + wp = find_popup_win(id); + if (wp == NULL) + return; + + buf = tv_get_buf_from_arg(&argvars[1]); + + if (buf == NULL) + return; +#ifdef FEAT_TERMINAL + else if (buf->b_term != NULL && popup_terminal_exists())
Is the "else" needed here as the previous "if" check does a return?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
@chrisbra pushed 1 commit.
- 6d9edee Problem: cannot change buffer in a popup
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
@chrisbra commented on this pull request.
In src/popupwin.c:
> @@ -2844,6 +2844,51 @@ f_popup_settext(typval_T *argvars, typval_T *rettv UNUSED)
popup_adjust_position(wp);
}
+/*
+ * popup_setbuf({id}, {bufnr})
+ */
+ void
+f_popup_setbuf(typval_T *argvars, typval_T *rettv UNUSED)
+{
+ int id;
+ win_T *wp;
+ buf_T *buf;
+
+ if (in_vim9script()
Thanks, adjusted.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
> +/*
+ * popup_setbuf({id}, {bufnr})
+ */
+ void
+f_popup_setbuf(typval_T *argvars, typval_T *rettv UNUSED)
+{
+ int id;
+ win_T *wp;
+ buf_T *buf;
+
+ if (in_vim9script()
+ && (check_for_number_arg(argvars, 0) == FAIL
+ || check_for_buffer_arg(argvars, 1) == FAIL))
+ return;
+
+ if (check_for_buffer_arg(argvars, 1) == FAIL)
fixed.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
> + return; + + if (check_for_buffer_arg(argvars, 1) == FAIL) + return; + + id = (int)tv_get_number(&argvars[0]); + wp = find_popup_win(id); + if (wp == NULL) + return; + + buf = tv_get_buf_from_arg(&argvars[1]); + + if (buf == NULL) + return; +#ifdef FEAT_TERMINAL + else if (buf->b_term != NULL && popup_terminal_exists())
fixed.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
> + int id; + win_T *wp; + buf_T *buf; + + if (in_vim9script() + && (check_for_number_arg(argvars, 0) == FAIL + || check_for_buffer_arg(argvars, 1) == FAIL)) + return; + + if (check_for_buffer_arg(argvars, 1) == FAIL) + return; + + id = (int)tv_get_number(&argvars[0]); + wp = find_popup_win(id); + if (wp == NULL) + return;
I made the function return a boolean now. So should be resolved now.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
made a few minor adjustments:
- let it return true/false on success/failure
- checked if the buffer is already displayed in the popup
- add new tests for opening a second terminal window in a popup
- updated documentation slightly
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
errael
I noticed #15006 (comment), which seems relevant, was not commented on. Why introduce a new function if not needed?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
bfrg
Wouldn't it be more useful to generalize the function such that it can be called for any window and not just popups? I mean, it's just a matter of time until some requests something like win_setbuf({winid}, {bufnr}).
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
Wouldn't it be more useful to generalize the function such that it can be called for any window and not just popups? I mean, it's just a matter of time until someone requests something like win_setbuf({winid}, {bufnr}).
I don't like that :) we have :b for that already.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
Copying here my comment from #15006:
> I noticed the describtion of popup_settext() explicitly limit the second arg not the buffer number, while popup_create() allowed the first arg. Would it be more unified to remove this limitation instead of creating a new interface?
Honestly, I don't understand that remark from popup_settext(). It says:
{text} is the same as supplied to |popup_create()|, except that a buffer number is not allowed.
But at popup_create() we don't have the {text} argument. I suppose it refers to the {what} of popup_create? I guess we could change popup_settext() and make {text} also take a buffer number (however not a buffer name). That could work.
Also, I am not sure, if we should relax the possible argument types for the second arg. Now it is already '`String` or `list`. Not sure if it is good design to now also allow `Number`. That seems to defeat the purpose of checking argument types in Vim9.
@yegappan what do you think?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
But at popup_create() we don't have the {text} argument. I suppose it refers to the {what} of popup_create? I guess we could change popup_settext() and make {text} also take a buffer number (however not a buffer name). That could work.
Also, I am not sure, if we should relax the possible argument types for the second arg. Now it is already '
Stringorlist. Not sure if it is good design to now also allowNumber. That seems to defeat the purpose of checking argument types in Vim9.
@yegappan what do you think?
It is better to have strict type checking to detect errors (also it helps with Vim9 code compilation).
So instead of changing the second argument to accept a number, you can either add a third
optional buffer argument or preferably a Dict argument. But using a separate dedicated function
for changing the buffer displayed in a popup window is more cleaner.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
errael
But at popup_create() we don't have the {text} argument. I suppose it refers to the {what} of popup_create? I guess we could change popup_settext() and make {text} also take a buffer number (however not a buffer name). That could work.
Also, I am not sure, if we should relax the possible argument types for the second arg. Now it is already '
Stringorlist. Not sure if it is good design to now also allowNumber. That seems to defeat the purpose of checking argument types in Vim9.
@yegappan what do you think?It is better to have strict type checking to detect errors (also it helps with Vim9 code compilation). So instead of changing the second argument to accept a number, you can either add a third optional buffer argument or preferably a Dict argument. But using a separate dedicated function for changing the buffer displayed in a popup window is more cleaner.
Does it matter that {what} is widely used throughout the popup API? And in fact {text} is the exception? Or that the proposed function is inconsistent with the API? Currently popup_settext() is the only popup function that has a parameter {text}. Changing {text} to {what} (and then renaming all the {what} to {text} ;) ) is consistent. I'd be surprised if there is any measurable function/compilation performance difference by including bufnr. What is less confusing for the developer?
I agree that using a dict for the popup API is good, but it's too late for that, isn't it? Maybe each of the 6 functions that take {what} should get a companion function that takes a bufnr, and using bufnr in {what} should be deprecated?
I don't care that much; I'm glad there's some discussion about it. I don't see the word "API" in :help development, maybe something in there about best practices for API development would be good. So function overloading is considered bad practice, use dictionaries and give an error if the same parameter is specified more that once. E.g. the {what} parameter would have 3 possibilities in the dictionary what_string, what_list_string, what_number and it's an error to specify more than one of them. Is that the idea?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
errael
E.g. the
{what}parameter would have 3 possibilities in the dictionarywhat_string,what_list_string,what_numberand it's an error to specify more than one of them. Is that the idea?
Oh, sadly with that approach there's no compile time checking like there is now with overloaded functions.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
I don't see the word "API" in :help development, maybe something in there about best practices for API development would be good. So function overloading is considered bad practice, use dictionaries and give an error if the same parameter is specified more that once
It's not only API, we need to be consistent in all areas, autocommands, functions, options, etc. This includes naming, arguments and types, documentation, messages (also for internal non-user facing functions). It's hard to get this right and hard to document what are our guidelines are for new features.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
bfrg
I don't like that :) we have :b for that already.
Yes, but it can only be set for the current window. To set the buffer of another window we need to use win_execute(winnr, $'buffer {bufnr}') or something like that which looks weird. And now for popups we have popup_setbuf(). IMO it would have been cleaner if there was one single function for both type of windows.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.