text is scrolled for 'nosplitscroll' when window has folds

[luukvbaal]

Problem: Text is scrolled for 'nosplitscroll' when window has folds.
Solution: Take into account fold length when calculating cursor position to avoid scrolling.

The current scroll behavior makes no sense whatsoever when folds are present(I was aware earlier but had ignored it until now as I don't really use folds). Adds a new function win_walk_fold() that moves the cursor a certain amount of lines up or down closed folds. Should it live in fold.c? Does fold.c already contain a similar function I could have reused? I'll check myself once more and look into adding a test. Could do with some testing, not sure if this works properly for nested folds and whatnot. Only tested with fold.c default fold state so far.

[codecov]

Codecov Report

Merging #11234 (b77315b) into master (838b746) will decrease coverage by 0.02%.
The diff coverage is 93.93%.

@@            Coverage Diff             @@
##           master   #11234      +/-   ##
==========================================
- Coverage   81.82%   81.79%   -0.03%     
==========================================
  Files         162      162              
  Lines      188749   188777      +28     
  Branches    42918    42947      +29     
==========================================
- Hits       154441   154412      -29     
- Misses      21759    21816      +57     
  Partials    12549    12549              

Flag	Coverage Δ
huge-clang-none	82.71% <93.93%> (-0.01%)	⬇️
huge-gcc-none	54.64% <44.61%> (-0.01%)	⬇️
huge-gcc-testgui	?
huge-gcc-unittests	0.29% <0.00%> (-0.01%)	⬇️
linux	82.40% <93.93%> (-0.10%)	⬇️
mingw-x64-HUGE	76.49% <90.32%> (+<0.01%)	⬆️
mingw-x86-HUGE	77.36% <90.32%> (+0.01%)	⬆️
windows	78.14% <90.32%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/edit.c	86.41% <88.00%> (+0.07%)	⬆️
src/normal.c	90.93% <92.85%> (+<0.01%)	⬆️
src/window.c	89.00% <100.00%> (+0.06%)	⬆️
src/gui_gtk_x11.c	51.00% <0.00%> (-0.59%)	⬇️
src/message.c	78.67% <0.00%> (-0.34%)	⬇️
src/buffer.c	86.51% <0.00%> (-0.27%)	⬇️
src/regexp_nfa.c	89.44% <0.00%> (-0.27%)	⬇️
src/optionstr.c	91.07% <0.00%> (-0.27%)	⬇️
src/hardcopy.c	76.83% <0.00%> (-0.22%)	⬇️
src/insexpand.c	88.21% <0.00%> (-0.18%)	⬇️
... and 16 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[luukvbaal]

Test added, ready as far as I'm concerned.

[luukvbaal]

Latest commit should improve test coverage.

[brammool]

Problem: Text is scrolled for 'nosplitscroll' when window has folds. Solution: Take into account fold length when calculating cursor position to avoid scrolling. The current scroll behavior makes no sense whatsoever when folds are present(I was aware earlier but had ignored it until now as I don't really use folds). Adds a new function win_walk_fold() that moves the cursor a certain amount of lines up or down closed folds. Should it live in fold.c? Does fold.c already contain a similar function I could have reused? I'll check myself once more and look into adding a test. Could do with some testing, not sure if this works properly for nested folds and whatnot. Only tested with fold.c default fold state so far.

The closest is probably what is in cursor_up() and cursor_down(). Not sure it's exactly what you want. Maybe add a test when it's working as you intended, then try optimizing the code and check the test doesn't fail.

…

-- hundred-and-one symptoms of being an internet addict: 181. You still remember the "happy tune" your modem made while dialing your ISP. /// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\ /// \\\ \\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ /// \\\ help me help AIDS victims -- http://ICCF-Holland.org ///

[luukvbaal]

Right, it currently works as intended and the test is added. I'll see if I can reuse cursor_up/down.

[luukvbaal]

Yeah no those functions only operate on curwin/buf(sensibly). But indeed they basically do what I want. Do you think it would be worth mis-using curwin/temp by setting it temporarily in win_fix_scroll, assuming the same result can be achieved? Or is adding win_walk_fold fine?

[luukvbaal]

Hmm it would look like this in theory. Would be nice if it worked but it doesn't seem to(yet).

diff --git a/src/window.c b/src/window.c
index ceceb7383..3a961b0ce 100644
--- a/src/window.c
+++ b/src/window.c
@@ -6362,6 +6362,8 @@ win_fix_scroll(int resize)
 {
     win_T    *wp;
     linenr_T lnum;
+    win_T *curwp = curwin;
+    buf_T *curbp = curbuf;
 
     skip_update_topline = TRUE;  // avoid scrolling in curs_columns()
     FOR_ALL_WINDOWS(wp)
@@ -6375,13 +6377,15 @@ win_fix_scroll(int resize)
 		lnum = wp->w_cursor.lnum;
 		int diff = (wp->w_winrow - wp->w_prev_winrow)
 					    + (wp->w_height - wp->w_prev_height);
-#ifdef FEAT_FOLDING
-		if (hasAnyFolding(wp))
-		    win_walk_fold(wp, wp->w_botline - 1, abs(diff), diff > 0);
+		wp->w_cursor.lnum = wp->w_botline - 1;
+		curwin = wp;
+		curbuf = wp->w_buffer;
+		if (diff > 0)
+		    cursor_down(diff, FALSE);
 		else
-#endif
-		    wp->w_cursor.lnum = MIN(wp->w_buffer->b_ml.ml_line_count,
-					    wp->w_botline - 1 + diff);
+		    cursor_up(diff, FALSE);
+		curwin = curwp;
+		curbuf = curbp;
 		// Bring the new cursor position to the bottom of the screen.
 		wp->w_fraction = FRACTION_MULT;
 		scroll_to_fraction(wp, wp->w_prev_height);
@@ -6413,9 +6417,6 @@ win_fix_scroll(int resize)
 win_fix_cursor(int normal)
 {
     win_T    *wp = curwin;
-#ifdef FEAT_FOLDING
-    int      fold = hasAnyFolding(wp);
-#endif
     long     so = get_scrolloff_value();
     linenr_T nlnum = 0;
     linenr_T lnum = wp->w_cursor.lnum;
@@ -6430,15 +6431,10 @@ win_fix_cursor(int normal)
 #endif
 
     so = MIN(wp->w_height / 2, so);
-    top = wp->w_topline + so;
-    bot = wp->w_botline - 1 - so;
-#ifdef FEAT_FOLDING
-    if (fold && so)
-    {
-	top = win_walk_fold(wp, wp->w_topline, so, TRUE);
-	bot = win_walk_fold(wp, wp->w_botline - 1, so, FALSE);
-    }
-#endif
+    wp->w_cursor.lnum = wp->w_topline;
+    top = (cursor_down(so, FALSE), wp->w_cursor.lnum);
+    wp->w_cursor.lnum = wp->w_botline - 1;
+    bot = (cursor_up(so, FALSE), wp->w_cursor.lnum);
 
     // Check if cursor position is above or below visible range.
     if (lnum > bot && (wp->w_botline - wp->w_buffer->b_ml.ml_line_count) != 1)
@@ -6465,36 +6461,6 @@ win_fix_cursor(int normal)
     }
 }
 
-/*
- * Walk cursor "steps" up or down from "start".  Return resulting cursor lnum.
- */
-#ifdef FEAT_FOLDING
-    static linenr_T
-win_walk_fold(win_T *wp, linenr_T start, int steps, int down)
-{
-    long       fold_len;
-    foldinfo_T foldinfo;
-
-    wp->w_cursor.lnum = start;
-
-    while (steps-- && wp->w_cursor.lnum > 1
-	    && wp->w_cursor.lnum < wp->w_buffer->b_ml.ml_line_count)
-    {
-	fold_len = foldedCount(wp, wp->w_cursor.lnum, &foldinfo);
-
-	if (down)
-	    wp->w_cursor.lnum += fold_len ? fold_len : 1;
-	else
-	{
-	    fold_len = fold_len ? foldedCount(wp, foldinfo.fi_lnum, NULL) : 1;
-	    wp->w_cursor.lnum -= fold_len;
-	}
-    }
-
-    return wp->w_cursor.lnum;
-}
-#endif
-
 /*
  * Set the height of a window.
  * "height" excludes any window toolbar.

[brammool]

Yeah no those functions only operate on curwin/buf(sensibly). But indeed they basically do what I want. Do you think it would be worth mis-using curwin/temp by setting it temporarily in win_fix_scroll, assuming the same result can be achieved? Or is adding win_walk_fold fine?

It is always good to re-use code that is intended to do the same thing. Currently cursor_up() works on curwin, but the block inside the "if (n > 0)" could easily be changed to accept a window pointer. You probably don't need the coladvance() call, so just moving that block to a separate function can work. Same for cursor_down(). I would do the refactoring, but let's first make sure you can actually use the functions after refactoring.

…

-- hundred-and-one symptoms of being an internet addict: 183. You move your coffeemaker next to your computer. /// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\ /// \\\ \\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ /// \\\ help me help AIDS victims -- http://ICCF-Holland.org ///

[luukvbaal]

Yes, it seems to work(didn't work before because I was passing a negative value to cursor_up()). Thanks for pointing me in the right direction(even though I already got it working), of course it's better to reuse code and I would have never though to have looked in edit.c.

Yes refactoring to accept a window pointer shouldn't be hard, do you think it's preffered over setting it temporarily in win_fix_scroll()?

I don't think the coladvance() call harms and it's probably preferred in win_fix_cursor() and only redundant(not harmful) in win_fix_scroll() since it's only used for scroll position. Not sure it's necessary to refactor it out.

[luukvbaal]

Does the sharthand comma notation have precendence in vim?
Or better yet we could we have cursor_up/down return w_cursor.lnum? It only returns FAIL/OK currently so it shouldn't be a problem, can return 0 instead of FAIL.

In other words, do you want

wp->w_cursor.lnum = wp->w_topline;
cursor_down(wp, so, FALSE)
top = wp->w_cursor.lnum;
wp->w_cursor.lnum = wp->w_botline - 1;
cursor_up(wp, so, FALSE)
bot = wp->w_cursor.lnum;

,

wp->w_cursor.lnum = wp->w_topline;
top = (cursor_down(wp, so, FALSE), wp->w_cursor.lnum);
wp->w_cursor.lnum = wp->w_botline - 1;
bot = (cursor_up(wp, so, FALSE), wp->w_cursor.lnum);

or

wp->w_cursor.lnum = wp->w_topline;
top = cursor_down(wp, so, FALSE);
wp->w_cursor.lnum = wp->w_botline - 1;
bot = cursor_up(wp, so, FALSE);

[luukvbaal]

Second commit re-uses cursor_up/down and seems to provide the same correct behavior as my earlier solution. Ready now as far as I'm concerned.

[brammool]

Does the sharthand comma notation have precendence in vim?

I don't like an assignment with a comma, it is confusing and the only purpose is to save a line of text. Try reading it out loud.

Or better yet we could we have cursor_up/down return w_cursor.lnum? It only returns FAIL/OK currently so it shouldn't be a problem, can return 0 instead of FAIL.

Returning zero for FAIL seems OK, since a zero line number is invalid.

…

-- hundred-and-one symptoms of being an internet addict: 186. You overstay in the office so you can have more time surfing the net. /// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\ /// \\\ \\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ /// \\\ help me help AIDS victims -- http://ICCF-Holland.org ///

[brammool]

Second commit re-uses cursor_up/down and seems to provide the same correct behavior as my earlier solution. Ready now as far as I'm concerned.

That call to coladvance if "wp == curwin" looks strange. I would rather split it in two functions, as I originally suggested.

…

-- hundred-and-one symptoms of being an internet addict: 187. You promise yourself that you'll only stay online for another 15 minutes...at least once every hour. /// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\ /// \\\ \\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ /// \\\ help me help AIDS victims -- http://ICCF-Holland.org ///

[brammool]

Yes, it seems to work(didn't work before because I was passing a negative value to cursor_up()). Thanks for pointing me in the right direction(even though I already got it working), of course it's better to reuse code and I would have never though to have looked in edit.c. Yes refactoring to accept a window pointer shouldn't be hard, do you think it's preffered over setting it temporarily in win_fix_scroll()?

Yes, passing the window pointer is always better. We only temporarily change curwin here and there to avoid having to pass it to many levels, but it may lead to trouble (e.g. "curbuf" might be invalid, unless it is changed too).

I don't think the coladvance() call harms and it's probably preferred in win_fix_cursor() and only redundant(not harmful) in win_fix_scroll() since it's only used for scroll position. Not sure it's necessary to refactor it out.

It has the side effect of changing fields in the window structure, thus unless you actually want to move to a certain column, better avoid it. It can be slow too.

…

-- hundred-and-one symptoms of being an internet addict: 185. You order fast food over the Internet /// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\ /// \\\ \\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ /// \\\ help me help AIDS victims -- http://ICCF-Holland.org ///

[luukvbaal]

That call to coladvance if "wp == curwin" looks strange. I would rather split it in two functions, as I originally suggested.

Is introducing this what you had in mind or am I misunderstanding?

cursor_down_advance(...)
{
    cursor_down(...)
    coladvance(wp->w_curswant);
}

[luukvbaal]

That call to coladvance if "wp == curwin" looks strange. I would rather split it in two functions, as I originally suggested.

Is introducing this what you had in mind or am I misunderstanding?

cursor_down_advance(...)
{
    cursor_down(...)
    coladvance(wp->w_curswant);
}

Assuming yes, the latest commit does just that. Anyhow, the patch is functional. Do as you see fit with the implementation(or elaborate on any structural changes and I can do it if you like).

[brammool]

> > That call to coladvance if "wp == curwin" looks strange. I would rather split it in two functions, as I originally suggested. > > Is introducing this what you had in mind or am I misunderstanding? > > ```c > cursor_down_advance(...) > { > cursor_down(...) > coladvance(wp->w_curswant); > } > ``` Assuming yes, the latest commit does just that. Anyhow, the patch is functional. Do as you see fit with the implementation(or elaborate on any structural changes and I can do it if you like).

I rather move the inner part of the functions to a separate function, so that we don't have to change all the calls. Also need to change hasFolding() to hasFoldingWin(). I'll take care of it.

…

-- hundred-and-one symptoms of being an internet addict: 191. You rate eating establishments not by the quality of the food, but by the availability of electrical outlets for your PowerBook. /// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\ /// \\\ \\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ /// \\\ help me help AIDS victims -- http://ICCF-Holland.org ///

[luukvbaal]

I rather move the inner part of the functions to a separate function, so that we don't have to change all the calls. Also need to change hasFolding() to hasFoldingWin(). I'll take care of it.

I think you still might need to update the calls that check for "cursor_up/down == OK", now that we are returning cursor.lnum. For example in edit.c:4723.

- if (cursor_up(1L, TRUE) == OK) 
+ if (cursor_up(1L, TRUE))

[luukvbaal]

I rather move the inner part of the functions to a separate function, so that we don't have to change all the calls. Also need to change hasFolding() to hasFoldingWin(). I'll take care of it.

I think you still might need to update the calls that check for "cursor_up/down == OK", now that we are returning cursor.lnum. For example in edit.c:4723.

- if (cursor_up(1L, TRUE) == OK) 
+ if (cursor_up(1L, TRUE))

Nevermind, I see only cursor_up_inner has the new return type.

[luukvbaal]

I do see the test is failing(it didn't on my branch), I'll see if I can spot the issue.

[luukvbaal]

I do see the test is failing(it didn't on my branch), I'll see if I can spot the issue.

I think you simply forgot to add the fourth screendump file "Test_nosplitscroll_fold_4.dump".

[brammool]

I do see the test is failing(it didn't on my branch), I'll see if I can spot the issue.

Somehow the fourth dump file didn't get included in the patch. I'll fix that now.

…

-- Press any key to continue, press any other key to quit. /// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\ /// \\\ \\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ /// \\\ help me help AIDS victims -- http://ICCF-Holland.org ///