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

Fixes for scrolling #252

Merged
merged 33 commits into from Sep 24, 2020
Merged

Fixes for scrolling #252

merged 33 commits into from Sep 24, 2020

Conversation

citizenmatt
Copy link
Member

@citizenmatt citizenmatt commented Sep 15, 2020

This PR introduces a number of fixes for scrolling, primarily for support of inline inlays, such as a parameter hints.

  • Fixes VIM-2104 - scrolloff instead of sidescrolloff is used for horizontal offset
  • Fix scrolljump not working
  • Improve handling of scrolljump to match Vim's fairly unintuitive rules
  • Fix VIM-1080 - handling of scrolling with virtual space
  • Add internal action to add test inline inlays, using the parameter hint renderer
  • Improve handling of inline inlays, especially around positioning after deletion, and also handling inlays associated with preceding text (e.g. Kotlin type annotations, as opposed to inlays associated with following text, such as Kotlin parameter hints)
  • Fix issues with side scrolling and inline inlays, including sidescrolloff and sidescroll(jump). Leading and trailing inlays are correctly positioned when scrolling offscreen.
    • Fixes VIM-1556 - Editor window is not correctly scrolled horizontally
    • Fixes VIM-1770 - Go to end of line conflicts with parameter name hints
  • Fix VIM-2110 - support for line scrolling with proportional fonts
  • Implement scroll screen half width actions (zL/zH)

Added tests:

  • Add tests for vertical scrolling with scrolloff and scrolljump
  • Add tests for scrolling up/down by line (<C-Y>/<C-E>)
  • Add tests for scrolling up/down by page (<C-B>/<C-F>)
  • Add tests and fixes for scrolling page (z+/z^)
  • Add tests for scrolling caret line to top/bottom/middle of page (zt/zb/zz + z<CR>/z-/z.)
  • Add tests for scrolling half page up/down (<C-U>/<C-D>)
  • Add tests for scrolling caret column to first/last column of page (zs/ze)
  • Add tests for scrolling screen one column to left/right (zl/zh)
  • Add tests for delete character left/right (+inlays) (X/x)

Known issues:

  • Virtual space handling doesn't exactly match Vim (e.g. <C-F> at the end of file doesn't scroll up), but it at least behaves sensibly
  • Side scroll offset is calculated on visual column to correctly handle closed inline folds, but both inline inlays and folds are implemented as visual columns, which means the offset isn't technically correct - it is not an offset of characters. The alternative is to iterate over visual columns and skip columns that are folds or inlays. I think this minor inaccuracy in a feature designed to show additional context is acceptable.

Questions:

  • Now that virtual space is working, shall we enable it for all Vim-enabled editors? This would match Vim behaviour, but might be a little intrusive, so we could also add an option to disable it (it would be on by default to match Vim)

Now very closely follows Vim's somewhat unintuitive handling. Doesn't work properly with soft wraps (like a lot of other parts of IdeaVim)
Behaviour matches Vim, apart from soft wraps
i_<C-Down> and i_<C-Up> are not standard Vim mappings, but can be set up in .ideavimrc if required
E.g. navigation around Kotlin type annotations, replacing a character with a preceding parameter hint
Fixes VIM-1556, fixes VIM-1770, fixes VIM-2110
@citizenmatt citizenmatt marked this pull request as ready for review September 17, 2020 14:30
Copy link
Member

@AlexPl292 AlexPl292 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, this is a giant PR, just awesome! This is great that IdeaVim would have much less issues with inline hints :)
I'm actually ready to merge it.

Now that virtual space is working, shall we enable it for all Vim-enabled editors? This would match Vim behaviour, but might be a little intrusive, so we could also add an option to disable it (it would be on by default to match Vim)

Do you mean that now we can automatically enable IJ "virtual space" setting, right?

src/com/maddyhome/idea/vim/group/MotionGroup.java Outdated Show resolved Hide resolved
@citizenmatt
Copy link
Member Author

Yeah, apologies for the size of the PR, it just kept on growing 😄 Horizontal scrolling touches a lot of places and it took a lot of new tests to make sure it was done.

As for virtual space, yes, I mean that we can now enable IJ's virtual space - both the "Show virtual space at the bottom of the file" option, and the "Allow caret placement after the end of line". This would give us behaviour in line with Vim. It would also allow us to implement the other virtualedit options.

My only concern is if it's too big a change for the user - we'd be overriding the settings, so there's no way to disable this. Perhaps we should add an ideavirtualspace setting, or a virtualedit=noideaspace option (which is ignored if all, block, insert or onemore is also set)? Or just implement it and see if anyone complains.

Whatever the answer, it should go in a separate PR 😁

@AlexPl292
Copy link
Member

No, it's absolutely fine. And I like the test coverage, sometimes contributors skip it :)

Okay, I get what you mean about virtual space. Yes, let's start with this PR and deal with virtual space later.

@AlexPl292 AlexPl292 merged commit 0dd47c1 into JetBrains:master Sep 24, 2020
@citizenmatt citizenmatt deleted the VIM-2104 branch September 24, 2020 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants