[vim/vim] let 'previewpopup' like 'completepopup' (PR #18873)
arkissa
I’ve seen many separate checks in the code specifically for the preview popup window, blocking certain parameters. I’m not familiar with Vim’s codebase, and I don’t really understand why this is done. My understanding is that a preview popup window is just a normal popup window (please correct me if I’m wrong), I also don’t really get why those checks are needed. This PR is meant to address the issue in #18826 and allow previewpopup to have richer configuration, so I removed some of the checks that tested whether a popup was a preview. I’m not sure what side effects this might cause.
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/18873
Commit Summary
- e87e88a feat: Add more settings to 'previewpopup' like 'completepopup'
- 3aa549e update: changed 'previewpopup' test cases.
- f97487c runtime(doc): 'previewpopup' referenced from 'completepopup'.
File Changes
(4 files)
- M runtime/doc/windows.txt (2)
- M src/optionstr.c (12)
- M src/popupwin.c (26)
- M src/testdir/test_options.vim (4)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
arkissa
Arkissa left a comment (vim/vim#18873)@girishji @chrisbra Please review this PR
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#18873)this needs a bit of updates to the documentation, I guess we need to also link from 'previewpopup' (this should also document what values can actually be set) to 'pumborder'?. Also, can you please update the tests for all possible option values that this allows then?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
arkissa
@Arkissa pushed 3 commits.
- af1d3ad feat: Add more settings to 'previewpopup' like 'completepopup'
- 75fb8e0 update: changed 'previewpopup' test cases.
- b7e968e runtime(doc): 'previewpopup' referenced from 'completepopup'.
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
arkissa
Arkissa left a comment (vim/vim#18873)Sorry, I finally found some time to look this pr.
Also, can you please update the tests for all possible option values that this allows then?
I've use some time understanding how to use and create test cases, i will be update it later.
but i have same questions, the 'previewpopup' has borders by default, and now that we have a border option to configure, should this default border be retained or removed and left to the option to configure? @chrisbra
this is result.
image.png (view on web)
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
arkissa
@Arkissa pushed 2 commits.
You are receiving this because you are subscribed to this thread.
girish
@girishji commented on this pull request.
> @@ -957,8 +957,17 @@ settings. The option is a comma-separated list of values: height maximum height of the popup width maximum width of the popup highlight highlight group of the popup (default is Pmenu) + border border style see 'pumborder'
border border style (see 'pumborder')
> @@ -957,8 +957,17 @@ settings. The option is a comma-separated list of values: height maximum height of the popup width maximum width of the popup highlight highlight group of the popup (default is Pmenu) + border border style see 'pumborder' + borderhighlight highlight group for the popup border characters + close show close button: "on" (default) or "off", if + the value is "on", it must be set after border.
close show close button: "on" (default) or "off", and if
the value is "on", it must be set after border.
> Example: > :set previewpopup=height:10,width:60 + :set previewpopup=height:10,width:60,border:single,borderhilight:PmenuBorder + :set previewpopup=height:10,width:60,border:custom:─;│;─;│;┌;┐;┘;└,borderhilight:PmenuBorder
:set border:single,borderhilight:PmenuBorder
:set border:custom:─;│;─;│;┌;┐;┘;└
In src/popupwin.c:
> @@ -1988,9 +1988,9 @@ parse_popup_option(win_T *wp, int is_preview) int on = STRNCMP(arg, "on", 2) == 0 && arg + 2 == p; int off = STRNCMP(arg, "off", 3) == 0 && arg + 3 == p; - if ((!on && !off) || is_preview) + if ((!on && !off))
redundant ()
In src/popupwin.c:
> @@ -2000,7 +2000,7 @@ parse_popup_option(win_T *wp, int is_preview) int on = STRNCMP(arg, "on", 2) == 0 && arg + 2 == p; int off = STRNCMP(arg, "off", 3) == 0 && arg + 3 == p; - if ((!on && !off) || is_preview) + if ((!on && !off))
redundant ()
In src/popupwin.c:
> @@ -2016,7 +2016,7 @@ parse_popup_option(win_T *wp, int is_preview) int on = STRNCMP(arg, "on", 2) == 0 && arg + 2 == p; int off = STRNCMP(arg, "off", 3) == 0 && arg + 3 == p; - if ((!on && !off) || is_preview) + if ((!on && !off))
redundant ()
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
arkissa
@Arkissa pushed 10 commits.
- 6c67aa8 feat: Add more settings to 'previewpopup' like 'completepopup'
- 09cc807 update: changed 'previewpopup' test cases.
- 947cd2d runtime(doc): 'previewpopup' referenced from 'completepopup'.
- c8fee53 update: delete align options value.
- d96f0ff runtime(doc): Update 'previewpopup' option values information.
- 3ecf81f update: previewpopup first value.
- 2d3bed1 fix: test case to failed.
- 08abea3 update: close:on by default
- 57e6b96 feat: previewpopup test cases.
- fae0561 fix: resolve conversation.
You are receiving this because you are subscribed to this thread.
girish
@girishji approved this pull request.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.