Hi Christian & Bram,
I'm including images below, so I thought it better to write directly rather
than bothering the whole mailing list about it.
I couldn't see how to generate a single patch for three submits in
TortoiseGit,
but I've listed all three patches again below which I think should be
sufficient?
I don't know how proper test cases are created, but here's a description of
each issue, in the order the patches are given below.
Below I'm using "gvim.exe --clean" on Windows 10, and using vim's source
files
as an example.
=================
Issue 1 - Resizing a non-current window down to a single line would not show
the cursor line, but a line one away from it
=================
Steps to reproduce:
* Open normal.c
* Go roughly to middle of file and note which line the cursor is on
* Split window twice to get three windows
* Use mouse to drag the divider below the middle window up or down
* When dragging all the way up, top window no longer shows cursor line
* When dragging all the way down, bottom window no longer shows cursor line
In both cases they show the line above the cursor line instead.
This was because the call to set_topline() no longer happened when "sline
== 0".
=================
Issue 2 - Cursor line near top of window ending up at bottom of window after
resizing down to two lines
=================
Steps to reproduce:
* set scrolloff=0
* Open normal.c
* Go roughly to middle of file
* Split window to get two windows
* In top window, put cursor on the 2nd or 3rd top line of the window
* Use mouse to drag the divider up until the top window only has two lines
* Cursor line which was near top of window is now on the bottom line
* It should have maintained its ratio within the window, and stayed near
the top
=================
Issue 3 - Resize window to 2 lines, move cursor from top to bottom line,
resize
larger, and resize back down to 2 lines again. Cursor line was now at top
instead of bottom
=================
I don't seem to be able to reproduce this without the previous two patches.
Since patch 2 fixed the maths in one part, the other part which was also
wrong
may have become more apparent.
I think just count patch 2 and 3 as a single patch, which fixes the cursor
line
positioning in general.
For example, resize down to 2 lines, move cursor from top to bottom line,
resize larger, and it positions the cursor line at about 50% through the
window, which is wrong (should be 75%). Similarly, if cursor was on top
line
of 2-line window, and w_fraction is recalculated (see below) then resize
larger
and it stays all the way at the top, 0% (should be 25%).
But it's a bit tricky to test, because if the cursor is already on the
bottom
line after resizing down to 2 lines, moving it up and down again won't
recalculate w_fraction. This is a good thing, but for testing you need to
trigger re-calculation of w_fraction. You need to resize down to 3 lines
first, then move cursor to top line, then resize down to 2 lines which
should
leave cursor on top line, THEN move cursor to bottom line, and then you can
resize larger and see what happens.
I also notice with scrolloff=5 that in general, resizing a window to a
smaller
size (1 or 2 lines) and then back to the original size no longer returned
the
cursor line to its original position as intended. This is also fixed by
these
patches.
Just lots of little things in this area weren't quite right any more.
Let me know if this is sufficient,
Thanks,
Rob.
=================
The patches
=================
Subject: [PATCH 1/3] Fixed bug where resizing a non-current window down to a
single line would not show the cursor line, but a line one away from it.
---
src/window.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/src/window.c b/src/window.c
index f6e611535..d5d67999c 100644
--- a/src/window.c
+++ b/src/window.c
@@ -5823,7 +5823,6 @@ scroll_to_fraction(win_T *wp, int prev_height)
--wp->w_wrow;
}
}
- set_topline(wp, lnum);
}
else if (sline > 0)
{
@@ -5868,9 +5867,8 @@ scroll_to_fraction(win_T *wp, int prev_height)
lnum = 1;
wp->w_wrow -= sline;
}
-
- set_topline(wp, lnum);
}
+ set_topline(wp, lnum);
}
if (wp == curwin)
--
2.19.1.windows.1
Subject: [PATCH 2/3] Fixed cursor line near top of window ending up at
bottom
of window after resizing down to two lines.
---
src/window.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/window.c b/src/window.c
index d5d67999c..48e343263 100644
--- a/src/window.c
+++ b/src/window.c
@@ -5786,8 +5786,7 @@ scroll_to_fraction(win_T *wp, int prev_height)
lnum = wp->w_cursor.lnum;
if (lnum < 1) /* can happen when starting up */
lnum = 1;
- wp->w_wrow = ((long)wp->w_fraction * (long)height - 1L
- + FRACTION_MULT / 2) / FRACTION_MULT;
+ wp->w_wrow = ((long)wp->w_fraction * (long)height - 1L) /
FRACTION_MULT;
line_size = plines_win_col(wp, lnum, (long)(wp->w_cursor.col)) - 1;
sline = wp->w_wrow - line_size;
--
2.19.1.windows.1
Subject: [PATCH 3/3] Fixed: resize window to 2 lines, move cursor from top
to
bottom line, resize larger, and resize back down to 2 lines again. Cursor
line was now at top instead of bottom.
---
src/window.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/window.c b/src/window.c
index 48e343263..0c77d7914 100644
--- a/src/window.c
+++ b/src/window.c
@@ -5725,7 +5725,7 @@ set_fraction(win_T *wp)
{
if (wp->w_height > 1)
wp->w_fraction = ((long)wp->w_wrow * FRACTION_MULT
- + wp->w_height / 2) / (long)wp->w_height;
+ + FRACTION_MULT / 2) / (long)wp->w_height;
}
/*
--
2.19.1.windows.1
On Mon, 25 Feb 2019 at 18:49, Christian Brabandt <[email protected]> wrote:
>
> On Mo, 25 Feb 2019, Robert Webb wrote:
>
> > [Note: a third patch below]
> >
> > Hmm, hard to see how those changes relate to either of those issues.
> The description of their solutions don't directly relate either. Maybe
> other changes were what fixed these things?
> >
> > The change to set_fraction() for 7.4.280 looks a bit nonsensical too.
> w_fraction is an integer, scaled up by a large number FRACTION_MULT to
> allow it to act like a fixed point float. At the time I wrote that, there
> were no floats used anywhere in vim, and it seemed risky
> > somehow to start using them just for this, cross-platform-wise. Are
> floats used elsewhere now? If so we could make this a float instead which
> would simplify the math.
> >
> > Anyway, the scaling looks wrong now (it is mixing the scaled-up ints
> with ints that haven't been scaled up, and treating them the same). Could
> probably revert the change in set_fraction() and it would be better.
> >
> > In fact I do still see a bug with the w_fraction stuff. Resize down to
> two lines, move cursor from top line to bottom line, resize larger, then
> resize back down to 2 lines. Now cursor is on top line instead of bottom
> where it started. I suspect reverting set_fraction()
> > will fix that...
> > Yep, tried it and it fixed it. Patch below (please combine with
> previous patch).
> >
> > The other changes in the fix seem separate, and are probably what's
> fixing the 7.4.280 issue.
> >
> > The changes for 7.4.365 don't appear related. They change the indenting
> on the two calls to set_topline() in win_new_height(), but didn't move them
> inside the two if-branches. That must have happened earlier.
> > Ah, it was 7.4.309. I think it's the added "if (sline > 0)" check that
> fixes that, not moving the call to set_topline() which I think is an error.
> >
> > So I suspect my three patches (including the one below) should be safe.
> Are there tests for 7.4.280 and 7.4.309 to confirm?
>
> Sorry, I am a bit confused now. It is probably best to submit a squashed
> complete patch. If you can, try to create a test case for those fixed
> issues (or at least describe precisely how to reproduce the issue
> starting from vim --clean, then we can probably make a new-style test
> for it to make sure it won't regress).
>
> Best,
> Christian
> --
> Ein Meinungsaustausch ist, wenn ein Beamter mit seiner Meinung zu
> seinem Vorgesetzten geht und mit dessen Meinung zurückkommt.
> -- Andrej Gromyko
>
> --
> --
> You received this message from the "vim_dev" maillist.
> Do not top-post! Type your reply below the text you are replying to.
> For more information, visit http://www.vim.org/maillist.php
>
> ---
> You received this message because you are subscribed to the Google Groups
> "vim_dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to [email protected].
> For more options, visit https://groups.google.com/d/optout.
>
--
--
You received this message from the "vim_dev" maillist.
Do not top-post! Type your reply below the text you are replying to.
For more information, visit http://www.vim.org/maillist.php
---
You received this message because you are subscribed to the Google Groups
"vim_dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
