Closed
Conversation
Contributor
|
I don't know why but it seems that test fails when window height is short. Here's the test results in my environment:
Environment:
|
Contributor
|
Problem: 'splitscroll' test takes too long.
Solution: Get rid of nested for loops.
I think this should test most of what we actually wanted to test with
the nested loops. Tested option values generated by `call
add(v:errors, "so=".run.", tab=".(run % 5).", winbar=".(run % 2).",
ls=".(run % 3).", ea=".(run % 2).", sb=".(run % 3).", pos=".pos)`:
```vim
so=0, tab=0, winbar=0, ls=0, ea=0, sb=0, pos=H
so=1, tab=1, winbar=1, ls=1, ea=1, sb=1, pos=M
so=2, tab=1, winbar=0, ls=2, ea=0, sb=1, pos=L
so=3, tab=1, winbar=1, ls=0, ea=1, sb=0, pos=H
so=4, tab=1, winbar=0, ls=1, ea=0, sb=1, pos=L
so=5, tab=0, winbar=1, ls=2, ea=1, sb=1, pos=M
so=6, tab=1, winbar=0, ls=0, ea=0, sb=0, pos=H
so=7, tab=1, winbar=1, ls=1, ea=1, sb=1, pos=M
so=8, tab=1, winbar=0, ls=2, ea=0, sb=1, pos=L
so=9, tab=1, winbar=1, ls=0, ea=1, sb=0, pos=H
so=10, tab=0, winbar=0, ls=1, ea=0, sb=1, pos=L
```
This should be much better. I tried not using "winbar" and "tab", but
it didn't make the resizing go away.
Most of the values are binary, but 'scrolloff' is a value. Now there is
only one test with 'scrolloff' set to zero. I would suggest to do about
half the tests with 'scrolloff' zero. Perhaps something like:
so = run % 2 == 0 ? 0 : run / 2
…--
hundred-and-one symptoms of being an internet addict:
86. E-mail Deficiency Depression (EDD) forces you to e-mail yourself.
/// 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 ///
|
Contributor
Author
Yeah it has to do with the 'scrolloff' insert mode case. I think I have found the fix.
Yeah you're right. Although 'scrolloff' is the more complicated case that could do with more testing, only doing a single run with 'so==0' might result in missing things. |
Contributor
|
> I don't know why but it seems that test fails when window height is
> short. At least in my environment, test fails when window height <=
> 30.
Yeah it has to do with the 'scrolloff' insert mode case. I think I
have found the fix.
> Most of the values are binary, but 'scrolloff' is a value. Now there is
> only one test with 'scrolloff' set to zero.
Yeah you're right. Although 'scrolloff' is the more complicated case
that could do with more testing, only doing a single run with 'so==0'
might result in missing things.
BTW: I can avoid the window resizing by using:
" disallow window resizing
let save_WS = &t_WS
set t_WS=
And later restoring the option:
let &t_WS = save_WS
I suppose the place where you tried before had t_WS empty by default.
…--
Ed's Radiator Shop: The Best Place in Town to Take a Leak.
/// 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 ///
|
Contributor
Author
To reiterate: if the windows are resized while we are not in normal mode (e.g. by autocommands/plugins), we want to make sure the cursor position stays valid, rather than changing it. Currently we were doing scroll_to_fraction with w_fraction set to 0 or 1, depending if the cursor went out of view above or below the window, to scroll the minimal amount. This does not work properly when w_height < 'scrolloff'. The fix I propose is just scrolling the cursor to the middle of the window instead. This works for the w_height < 'scrolloff' case and does not seem too offensive otherwise, even assuming 'scrolloff == 0'. Since scrolling will happen anyways we might as well place the cursor in the middle of the screen if that is where you were typing/selecting. |
f0fbb0c to
b097c5c
Compare
Contributor
Author
|
Latest commit tests these values: Increasing
Should I add it? |
Codecov Report
@@ Coverage Diff @@
## master #11139 +/- ##
========================================
Coverage 81.79% 81.79%
========================================
Files 162 162
Lines 188332 188443 +111
Branches 42834 42851 +17
========================================
+ Hits 154040 154132 +92
- Misses 21756 21760 +4
- Partials 12536 12551 +15
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
luukvbaal
commented
Sep 15, 2022
src/window.c
Outdated
| // Make sure cursor is closer to topline than botline. | ||
| if (so == wp->w_height / 2 | ||
| && nlnum - wp->w_topline > wp->w_botline - 1 - nlnum) | ||
| wp->w_fraction += 1L; |
Contributor
Author
There was a problem hiding this comment.
I think this will be covered by the test if we increase the loop range. Should we? Not sure how to determine what range will cover it. I tested it on my local system and confirmed this fixes a problem but when this happens depends on the window height the test runs in.
ba65918 to
0c89b1e
Compare
Contributor
|
Latest commit tests these values:
```
so=0, tab=0, winbar=0, ls=0, ea=0, sb=0, pos=H
so=1, tab=1, winbar=1, ls=1, ea=1, sb=1, pos=M
so=2, tab=1, winbar=0, ls=2, ea=0, sb=1, pos=L
so=0, tab=1, winbar=1, ls=0, ea=1, sb=0, pos=H
so=4, tab=1, winbar=0, ls=1, ea=0, sb=1, pos=L
so=5, tab=0, winbar=1, ls=2, ea=1, sb=1, pos=M
so=0, tab=1, winbar=0, ls=0, ea=0, sb=0, pos=H
so=7, tab=1, winbar=1, ls=1, ea=1, sb=1, pos=M
so=8, tab=1, winbar=0, ls=2, ea=0, sb=1, pos=L
so=0, tab=1, winbar=1, ls=0, ea=1, sb=0, pos=H
so=10, tab=0, winbar=0, ls=1, ea=0, sb=1, pos=L
```
Increasing `run` range as far as 100 still passes the test so that
makes me confident that 'scrolloff' is now handled correctly.
OK. Some combinations are not tested though, e.g. so=0 with ls=1.
> BTW: I can avoid the window resizing by using:
" disallow window resizing
let save_WS = &t_WS
set t_WS=
Should I add it?
Did you manage the reproduce the window resizing? I suppose it would
still happen with a reduced number of loops. Might as well do that.
…--
hundred-and-one symptoms of being an internet addict:
90. Instead of calling you to dinner, your spouse sends e-mail.
/// 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 ///
|
Contributor
|
@luukvbaal commented on this pull request.
> @@ -6436,7 +6435,11 @@ win_fix_cursor(int normal)
}
else
{ // Ensure cursor stays visible if we are not in normal mode.
- wp->w_fraction = top ? 0 : FRACTION_MULT;
+ wp->w_fraction = 0.5 * FRACTION_MULT;
+ // Make sure cursor is closer to topline than botline.
+ if (so == wp->w_height / 2
+ && nlnum - wp->w_topline > wp->w_botline - 1 - nlnum)
+ wp->w_fraction += 1L;
I think this will be covered by the test if we increase the loop
range. Should we? Not sure how to determine what range will cover it.
I tested it on my local system and confirmed this fixes a problem but
when this happens depends on the window height the test runs in.
We actually still have FEAT_FLOAT around float computations.
It probably isn't needed in most places, it actually means that we have
all the float functions, such as atan() and strod().
I think it's OK to use 0.5. If nothing breaks on any system then I'll
have a look into changing FEAT_FLOAT into the actually intended
HAVE_FLOAT_FUNCS. Or perhaps FEAT_EVAL_FLOAT. Or check each function
separately and only disable a tiny part that is using one.
…--
hundred-and-one symptoms of being an internet addict:
91. It's Saturday afternoon in the middle of May and you
are on 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 ///
|
Contributor
Author
Yes, it is rather arbitrary. Increasing the loop range will increase the coverage. Since the runtime is significantly smaller now, I guess we could afford to. But it's more about testing different values. I don't think each combination of options leads to different control flow in the code. If that were the case, we would need the nested loops.
I didn't try any further but no I couldn't reproduce it on my current system. I've seen it happening when I was testing gvim though. Feel free to add it, and/or increase the loop range when you include this patch, IDK if it matters.
TBH I added this because I was porting it to neovim and their CI was complaining about it. Do as you see fit. |
Problem: 'splitscroll' test takes too long. Cursor position is not always maintained when not in normal mode. Solution: Get rid of nested for loops. Center cursor in window when not in insert mode.
0c89b1e to
81d2302
Compare
Contributor
|
> OK. Some combinations are not tested though, e.g. so=0 with ls=1.
Yes, it is rather arbitrary. Increasing the loop range will increase
the coverage. Since the runtime is significantly smaller now, I guess
we could afford to. But it's more about testing different values. I
don't think each combination of options leads to different control
flow in the code. If that were the case, we would need the nested
loops.
OK. If any problems get reported we can add a specific regression test,
that usually works better than random conbinations. Since it's a setup
that someone actually uses vs. a synthetic one.
> Did you manage the reproduce the window resizing? I suppose it would
> still happen with a reduced number of loops. Might as well do that.
I didn't try any further but no I couldn't reproduce it on my current
system. I've seen it happening when I was testing gvim though. Feel
free to add it, and/or increase the loop range when you include this
patch, IDK if it matters.
I do still see it, so let me add that. Might actually use this in more
tests, it should improve speed and reduce flakiness.
> We actually still have FEAT_FLOAT around float computations.
> It probably isn't needed in most places, it actually means that we have
> all the float functions, such as atan() and strod().
> I think it's OK to use 0.5. If nothing breaks on any system then I'll
> have a look into changing FEAT_FLOAT into the actually intended
> HAVE_FLOAT_FUNCS. Or perhaps FEAT_EVAL_FLOAT. Or check each function
> separately and only disable a tiny part that is using one.
TBH I added this because I was porting it to neovim and their CI was
complaining about it. Do as you see fit.
I asked about it on the vim-dev maillist, no comments so far...
…--
From "know your smileys":
~#:-( I just washed my hair, and I can't do nuthin' with it.
/// 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
commented
Sep 16, 2022
| 'splitbelow' 'sb' new window from split is below the current one | ||
| 'splitright' 'spr' new window is put right of the current one | ||
| 'splitscroll' 'spsc' determines scroll behavior when splitting windows | ||
| 'splitscroll' 'spsc' determines scroll behavior for split windows |
Contributor
|
@luukvbaal commented on this pull request.
> -'splitscroll' 'spsc' determines scroll behavior when splitting windows
+'splitscroll' 'spsc' determines scroll behavior for split windows
This change was missed in 594f9e0.
Quite often runtime file updates are bundled up. Only when introducing
something new or changing syntax it gets included with the patch.
…--
From "know your smileys":
:q vi user saying, "How do I get out of this damn emacs editor?"
/// 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 ///
|
zeertzjq
added a commit
to luukvbaal/neovim
that referenced
this pull request
Oct 6, 2022
vim-patch:9.0.0445: when opening/closing window text moves up/down
Problem: When opening/closing window text moves up/down.
Solution: Add the 'splitscroll' option. When off text will keep its
position as much as possible.
vim/vim@29ab524
vim-patch:9.0.0455: a few problems with 'splitscroll'
Problem: A few problems with 'splitscroll'.
Solution: Fix 'splitscroll' problems. (Luuk van Baal, closes vim/vim#11117)
vim/vim@5ed3917
vim-patch:9.0.0461: 'scroll' is not always updated
Problem: 'scroll' is not always updated.
Solution: Call win_init_size() at the right place.
vim/vim@470a141
vim-patch:9.0.0465: cursor moves when cmdwin is closed when 'splitscroll' is off
Problem: Cursor moves when cmdwin is closed when 'splitscroll' is off.
Solution: Temporarily set 'splitscroll' when jumping back to the original
window. (closes vim/vim#11128)
vim/vim@e697d48
vim-patch:9.0.0469: cursor moves if cmdwin is closed when 'splitscroll' is off
Problem: Cursor moves if cmdwin is closed when 'splitscroll' is off.
Solution: Skip win_fix_cursor if called when cmdwin is open or closing.
(Luuk van Baal, closes vim/vim#11134)
vim/vim@3735f11
vim-patch:9.0.0478: test for 'splitscroll' takes too much time
Problem: Test for 'splitscroll' takes too much time.
Solution: Only test some of the combinations. (Luuk van Baal, closes vim/vim#11139)
vim/vim@594f9e0
vim-patch:9.0.0486: text scrolled with 'nosplitscroll', autocmd win and help
Problem: Text scrolled with 'nosplitscroll', autocmd win opened and help
window closed.
Solution: Skip win_fix_scroll() in more situations. (Luuk van Baal,
closes vim/vim#11150)
vim/vim@d5bc762
vim-patch:9.0.0505: various problems with 'nosplitscroll'
Problem: Various problems with 'nosplitscroll'.
Solution: Fix 'nosplitscroll' problems. (Luuk van Baal, closes vim/vim#11166)
vim/vim@faf1d41
vim-patch:9.0.0555: scrolling with 'nosplitscroll' in callback changing curwin
Problem: Scrolling with 'nosplitscroll' in callback changing curwin.
Solution: Invalidate w_cline_row in the right place. (Luuk van Baal,
closes vim/vim#11185)
vim/vim@20e5856
vim-patch:9.0.0603: with 'nosplitscroll' folds are not handled correctly
Problem: With 'nosplitscroll' folds are not handled correctly.
Solution: Take care of closed folds when moving the cursor. (Luuk van Baal,
closes vim/vim#11234)
vim/vim@7c1cbb6
vim-patch:9.0.0605: dump file missing
Problem: Dump file missing.
Solution: Add the missing dump file. (issue vim/vim#11234)
vim/vim@439a2ba
vim-patch:9.0.0647: the 'splitscroll' option is not a good name
Problem: The 'splitscroll' option is not a good name.
Solution: Rename 'splitscroll' to 'splitkeep' and make it a string option,
also supporting "topline". (Luuk van Baal, closes vim/vim#11258)
vim/vim@13ece2a
vim-patch:9.0.0667: ml_get error when 'splitkeep' is "screen"
Problem: ml_get error when 'splitkeep' is "screen". (Marius Gedminas)
Solution: Check the botline is not too large. (Luuk van Baal,
closes vim/vim#11293, closes vim/vim#11292)
vim/vim@346823d
zeertzjq
pushed a commit
to neovim/neovim
that referenced
this pull request
Oct 6, 2022
vim-patch:9.0.0445: when opening/closing window text moves up/down
Problem: When opening/closing window text moves up/down.
Solution: Add the 'splitscroll' option. When off text will keep its
position as much as possible.
vim/vim@29ab524
vim-patch:9.0.0455: a few problems with 'splitscroll'
Problem: A few problems with 'splitscroll'.
Solution: Fix 'splitscroll' problems. (Luuk van Baal, closes vim/vim#11117)
vim/vim@5ed3917
vim-patch:9.0.0461: 'scroll' is not always updated
Problem: 'scroll' is not always updated.
Solution: Call win_init_size() at the right place.
vim/vim@470a141
vim-patch:9.0.0465: cursor moves when cmdwin is closed when 'splitscroll' is off
Problem: Cursor moves when cmdwin is closed when 'splitscroll' is off.
Solution: Temporarily set 'splitscroll' when jumping back to the original
window. (closes vim/vim#11128)
vim/vim@e697d48
vim-patch:9.0.0469: cursor moves if cmdwin is closed when 'splitscroll' is off
Problem: Cursor moves if cmdwin is closed when 'splitscroll' is off.
Solution: Skip win_fix_cursor if called when cmdwin is open or closing.
(Luuk van Baal, closes vim/vim#11134)
vim/vim@3735f11
vim-patch:9.0.0478: test for 'splitscroll' takes too much time
Problem: Test for 'splitscroll' takes too much time.
Solution: Only test some of the combinations. (Luuk van Baal, closes vim/vim#11139)
vim/vim@594f9e0
vim-patch:9.0.0486: text scrolled with 'nosplitscroll', autocmd win and help
Problem: Text scrolled with 'nosplitscroll', autocmd win opened and help
window closed.
Solution: Skip win_fix_scroll() in more situations. (Luuk van Baal,
closes vim/vim#11150)
vim/vim@d5bc762
vim-patch:9.0.0505: various problems with 'nosplitscroll'
Problem: Various problems with 'nosplitscroll'.
Solution: Fix 'nosplitscroll' problems. (Luuk van Baal, closes vim/vim#11166)
vim/vim@faf1d41
vim-patch:9.0.0555: scrolling with 'nosplitscroll' in callback changing curwin
Problem: Scrolling with 'nosplitscroll' in callback changing curwin.
Solution: Invalidate w_cline_row in the right place. (Luuk van Baal,
closes vim/vim#11185)
vim/vim@20e5856
vim-patch:9.0.0603: with 'nosplitscroll' folds are not handled correctly
Problem: With 'nosplitscroll' folds are not handled correctly.
Solution: Take care of closed folds when moving the cursor. (Luuk van Baal,
closes vim/vim#11234)
vim/vim@7c1cbb6
vim-patch:9.0.0605: dump file missing
Problem: Dump file missing.
Solution: Add the missing dump file. (issue vim/vim#11234)
vim/vim@439a2ba
vim-patch:9.0.0647: the 'splitscroll' option is not a good name
Problem: The 'splitscroll' option is not a good name.
Solution: Rename 'splitscroll' to 'splitkeep' and make it a string option,
also supporting "topline". (Luuk van Baal, closes vim/vim#11258)
vim/vim@13ece2a
vim-patch:9.0.0667: ml_get error when 'splitkeep' is "screen"
Problem: ml_get error when 'splitkeep' is "screen". (Marius Gedminas)
Solution: Check the botline is not too large. (Luuk van Baal,
closes vim/vim#11293, closes vim/vim#11292)
vim/vim@346823d
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem: 'splitscroll' test takes too long. Cursor position is not always maintained when not in normal mode.
Solution: Get rid of nested for loops. Center cursor in window when not in normal mode.
I think this should test most of what we actually wanted to test with the nested loops. Tested option values generated by
call add(v:errors, "so=".run.", tab=".(run % 5).", winbar=".(run % 2).", ls=".(run % 3).", ea=".(run % 2).", sb=".(run % 3).", pos=".pos):EDIT: investigating failing test.