Skip to content
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

Allow non-fixed popups to be shifted even when wrap is set #16247

Closed
wants to merge 1 commit into from

Conversation

bstaletic
Copy link
Contributor

As discussed, this is the implementation of #16231

This pull request makes it possible for a popup window to use both, fixed == 0 and wrap == 1.
This is achieved by first shifting the window to the left until it is either fully visible or is moved all the way to the left.
Then, if the popup window is all the way to the left and some line is still too long to fit on screen, wrapping is performed.

@chrisbra chrisbra closed this in 13c1153 Dec 19, 2024
@bfrg
Copy link
Contributor

bfrg commented Dec 20, 2024

There are some issues with this fix. When border and/or padding is set, the right side of the popup is cut off:
screenshot-2024-12-20_163856

Example:

vim9script

set showbreak=+++

def Test()
    popup_atcursor(repeat("0123456789 ", 20), {
        moved: 'any',
        border: [1, 1, 1, 1],
        padding: [0, 1, 0, 1],
    })
enddef

nnoremap gh <ScriptCmd>Test()<Cr>

There's also an extra line printed when the cursor is at the far right:
screenshot-2024-12-20_163728

@bstaletic
Copy link
Contributor Author

bstaletic commented Dec 20, 2024

The behaviour regarding cutting off the right edge padding did not change with the pull request.
It used to happen before whenever the popup was allowed to shift to the left.

I am not sure what you mean by the last part:

There's also an extra line printed when the cursor is at the far right:

EDIT: Maybe I'm wrong about the right edge paddig getting cut off before.

@bfrg
Copy link
Contributor

bfrg commented Dec 20, 2024

I am not sure what you mean by the last part:

There's also an extra line printed when the cursor is at the far right:

What I meant was that there's an extra empty line displayed in the popup (see the 2nd screenshot above).

@bstaletic
Copy link
Contributor Author

Got it.
First, if we moved the popup all the way left and it still goes off screen and wrapping is on, the maxwidth needs to be lowered by right_extra.
That solves the right edge padding being missing.

The second problem is caused by shifting left mutating wp, so line needs to be recalculated after shifting.

Recording of the fix: https://asciinema.org/a/Ha8ciJG5veAuvWqToImDlzOIb

Diff:

diff --git a/src/popupwin.c b/src/popupwin.c
index 404549097..417533016 100644
--- a/src/popupwin.c
+++ b/src/popupwin.c
@@ -1444,15 +1444,18 @@ popup_adjust_position(win_T *wp)

            if (shift_by > wp->w_wincol)
            {
-               int truncate_shift = shift_by - wp->w_wincol;
-
-               shift_by -= truncate_shift;
+               shift_by -= shift_by - wp->w_wincol;
+               if (wp->w_p_wrap)
+               {
+                   maxwidth -= right_extra;
+               }
            }

            wp->w_wincol -= shift_by;
            maxwidth += shift_by;
            wp->w_width = maxwidth;
        }
+       len = linetabsize(wp, lnum);
        if (wp->w_p_wrap)
        {
            while (len + margin_width > maxwidth)

I don't see a good way of testing this except with the screendump tests. Unfortunately, I don't yet know how they work and won't be near my computer for the next 10 days.

@chrisbra
Copy link
Member

@bfrg can you please test it? Regarading the screendump tests. It's not so hard. Have a look at e.g. this one here:

func Test_simple_popup()
CheckScreendump
let lines =<< trim END
call setline(1, range(1, 100))
hi PopupColor1 ctermbg=lightblue
hi PopupColor2 ctermbg=lightcyan
hi EndOfBuffer ctermbg=lightgrey
hi Comment ctermfg=red
call prop_type_add('comment', #{highlight: 'Comment'})
let winid = popup_create('hello there', #{line: 3, col: 11, minwidth: 20, highlight: 'PopupColor1'})
let winid2 = popup_create(['another one', 'another two', 'another three'], #{line: 3, col: 25, minwidth: 20})
call setwinvar(winid2, '&wincolor', 'PopupColor2')
END
call writefile(lines, 'XtestPopup', 'D')
let buf = RunVimInTerminal('-S XtestPopup', #{rows: 10})
call VerifyScreenDump(buf, 'Test_popupwin_01', {})

Now when you first run it, you will find the "failed" screendump file inside testdir/failed/. You can inspect the file using the term_dumpload() function and need to manually verify that the screen dump looks like you expect it. Once you are satisfied with it, copy it to the testdir/dumps directory and on next run of the test, Vim will verify the screendump test against the file in testdir/dumps directory. See also the README

@bfrg
Copy link
Contributor

bfrg commented Dec 22, 2024

I just tried the above patch and it doesn't work yet. The same script as above (notice the cursor position):

screenshot-2024-12-22_152535
screenshot-2024-12-22_152544

@bstaletic
Copy link
Contributor Author

This patch works better:

diff --git a/src/popupwin.c b/src/popupwin.c
index 404549097..10cc12221 100644
--- a/src/popupwin.c
+++ b/src/popupwin.c
@@ -1444,14 +1444,17 @@ popup_adjust_position(win_T *wp)

            if (shift_by > wp->w_wincol)
            {
-               int truncate_shift = shift_by - wp->w_wincol;
-
-               shift_by -= truncate_shift;
+               shift_by -= shift_by - wp->w_wincol;
+               if (wp->w_p_wrap)
+               {
+                   maxwidth -= right_extra;
+               }
            }

            wp->w_wincol -= shift_by;
            maxwidth += shift_by;
            wp->w_width = maxwidth;
+           len = linetabsize(wp, lnum);
        }
        if (wp->w_p_wrap)
        {

Still, if the text is wide, but does not wrap yet, and it gets shifted, then there's a len(&showbreak) extra spaces to the right.
I do not understand why, but linetabsize(wp, lnum) returns a larger number than expected.

Another recording:
https://asciinema.org/a/Bp4BSkyM7dgBZkczHSpMrPXgj

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.

3 participants