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

Use text scale in calculating visible window height #1499

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

countvajhula
Copy link
Contributor

@countvajhula countvajhula commented Jul 23, 2021

Fixes #1497

@tomdl89 your idea to use text scale works perfectly. Initially I did think it was over-reporting the window height, but upon closer inspection, it is reporting the total number of visible lines of text, not lines of code, which is exactly what we are looking for, and matches Vim behavior as well as far as I was able to check.

To test:

  1. create a buffer with short lines, e.g. 100ihello<RET><esc>
  2. now check (evil-window-visible-height) at different zoom levels. It should report the correct value
  3. create a buffer with long lines, e.g. 20ihello<space><esc> along with other, regular-length lines
  4. now check (evil-window-visible-height) again. It will show a larger number than what is actually displayed in line numbers on the screen.
  5. Looking at the line number column (e.g. global-display-line-numbers-mode), count the number of blanks between line numbers. These are lines that are actually displayed, but which do not correspond to line numbers because the lines are wrapped. Add this number to the visible number and you should get the number reported by evil-window-visible-height.

Looking at the behavior in Vim, I wasn't able to detect a difference at different zoom levels, or at the end of the buffer.

cc @chopptimus - please let us know if you have a chance to verify the fix and notice any differences, and thank you for reporting it 🙏

@tomdl89
Copy link
Member

tomdl89 commented Jul 23, 2021

That's great news, thanks. I'll take a look on the weekend. Agree I'd like @chopptimus to take a look if possible.

@countvajhula
Copy link
Contributor Author

I noticed that one of the edge cases that @chopptimus pointed out still fails -- if you have a long wrapped line that fills the entire screen, then C-d doesn't scroll the page.

Some notes towards a fix:

  • this happens because even though the window is scrolled by N visual lines, since point is still in the same wrapped line, nothing happens because the entire function is enclosed in a evil-save-column macro which returns point to the initial position in the line
  • I've noticed that Vim does not preserve the column but instead always goes to first non-blank. If we remove this macro, scrolling works, but I'm not sure this is the right fix since it doesn't conform to either existing evil or Vim behavior, and I think the existing evil behavior (of preserving column) is a good idea in principle.
  • Another possibility is to say, if after scrolling we are still on the same line, then call (next-line). It looks like Vim actually does something different here, but I don't see what the pattern is exactly, as it appears to scroll several lines and not just one.

Any preferences here @tomdl89 ?

@chopptimus
Copy link

chopptimus commented Jul 24, 2021

Vims behaviour when scrolling long wrapped lines is pretty inconsistent. If I have a window with 50 lines of text visible but only 5 lines of code, vim scrolls 4 lines, and if I have only a single line of code visible it scrolls a single line.

The fix works for the first issue I reported.

@tomdl89
Copy link
Member

tomdl89 commented Jul 24, 2021

Some notes from testing:

  • You'll need to (require face-remap) at the top of evil-commands.el to make sure text-scale-mode-... vars are always there when you need them
  • I agree it works perfectly when zoomed in, but it's not quite perfect when zoomed out. Probably acceptable though
  • I'm confused about evil-save-column - it doesn't seem to prevent moving column currently in master (when hitting C-d on a line that fills the window)? I'm unclear on how evil-scroll-up and evil-scroll-down are circumventing it.
  • Because of the above point, I do still think we need a fix before we can merge. I'd like to understand how the current implementation works better before changing it.
    Thanks :)

@countvajhula
Copy link
Contributor Author

@chopptimus , yeah that's exactly what I noticed too. @tomdl89 good points, I'll take another look when I have a chance unless you get to it first.

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.

evil-scroll-up and evil-scroll-down do not work correctly when end of buffer is visible
3 participants