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

Caret position and view scrolling fixes #280

Merged
merged 17 commits into from Apr 16, 2021

Conversation

citizenmatt
Copy link
Member

A bunch of minor fixes to caret positioning and view scrolling. It looks like a big PR, but half of it is tests :)

  • Better positioning of the insert cursor with inlay hints that relate to preceding text. The caret is now placed between the inlay and the related text when in insert mode, and after the inlay in normal mode. Caret positioning behaves more intuitively with inlay hints in both insert and normal modes. (VIM-2230)
  • Scrolls the view to keep the caret visible when editing or moving in insert mode. Typing and deleting will scroll horizontally according to the sidescroll and sidescrolloff options. Moving up/down and typing enter will scroll vertically correctly, also following scrolloff and scrolljump. Tab in insert mode does not scroll correctly. See below.
  • Scrolls the view to keep the caret visible when deleting characters to the left of the cursor or when starting a new line above/below in normal mode would go offscreen due to scroll offsets.
  • Fix caret positioning bug if cursor is between 0 and sidescrolloff when moving the view
  • Improve handling of G to place the last line at the bottom of the file
  • Fix scrolling issues when moving caret within scrolloff lines of the bottom of a file, with virtual space (VIM-2177)
  • Fix various scrolling issues when virtual space at the end of a file is enabled, e.g. <C-F>/<C-B> and <C-E>/<C-Y>
  • Do not sound the error bell when Escape is handled by the IDE, e.g. when removing highlights

Note that <Tab> doesn't scroll the view correctly when in insert mode (although the <C-I> synonym does). This is a limitation of the way we handle conflicting shortcut keys. This key is handled by IdeaVim first and has a special case to ignore VK_TAB in insert mode. This means the other IDE actions get a chance to handle <Tab>, notably the live templates action, and means Emmet will work in insert mode (VIM-674, f6cb04c7). The downside is that if the live templates action doesn't have anything to expand, IdeaVim doesn't get another chance to handle it.

@citizenmatt
Copy link
Member Author

For the <Tab> issue, it might be worth looking at registering some special keys (tab, backspace, escape, etc.) as editor action handlers (EditorActionTab, EditorEscape etc.). This way, the special keys would get processed by action handlers first, and if they're not handled, then IdeaVim would still get a chance. On the other hand, this could mean that <Tab> would be processed by the live templates action even when not in insert mode. Perhaps a hybrid option would work? Pass along some special keys when in insert mode, with editor action handlers for fallbacks if the IDE actions don't handle?

@AlexPl292
Copy link
Member

That's a nice update, you're a scrolling master.
TBH, I don't see an issue if tab would be processed by live templates in normal mode, do you see some?

@AlexPl292 AlexPl292 merged commit a8a4142 into JetBrains:master Apr 16, 2021
@citizenmatt citizenmatt deleted the ideavim-sidescroll branch April 18, 2021 21:24
@citizenmatt
Copy link
Member Author

I'm hoping scrolling is mostly done now 😁 (apart from those way too large JavaDocs. Still not sure how to handle those…)

I don't think tab behaviour is too bad either way right now, and that comment was really as a thinking-out-loud, note-to-self kind of comment. I don't think it desperately needs a fix, but at some point it might be worth taking a look at how we handle some of those special keys to see if we can get even better handling.

@AlexPl292
Copy link
Member

I've missed, is this PR supposed to handle this case?
2021-04-19 10 08 59

@citizenmatt
Copy link
Member Author

Kind of, yes. Given an inlay that is related to preceding text, insert mode will now place the caret before the inlay. So in this case, the caret should appear after the n and before the :Boolean. The 123 would be appended to the n and push the inlay out.

However, this particular example isn't of an inlay related to preceding text, although it looks like it is, and indeed should be. I raised an issue with the Kotlin plugin (KTIJ-3768) but it turns out to be a bug in the platform. The API that the Kotlin plugin was using was failing to set the "related to preceding text" properly for all Kotlin hints like this. I've fixed it in the platform, but sadly only for 212.

You can see the actual change in action better with inline rename. This adds an inlay hint to the end of the text of the symbol to be renamed and this is correctly set to "relates to preceding text". The insert mode caret stays between the caret and the inlay hint. The normal mode caret only shows over text.

Screen.Recording.2021-04-23.at.12.24.02.am.mov

@AlexPl292
Copy link
Member

Aha, I see! I've just tried it on 212 and it works like that. I know why I haven't noticed this before, as I understand you place the caret before the inlay only in case of insert mode. But I usually navigate to the end of the word using e command and then pressing a. In this case the caret is placed after the inlay again.
Anyway, that's cool! Fixing the platform to fix IdeaVim issues is awesome :D

@citizenmatt
Copy link
Member Author

e then a should still work in master on 212 - it should place the caret after the text but before the inlay.

@AlexPl292
Copy link
Member

🤔 Doesn't work for me. This is 0.67-dev.6 and IU-212.2322
2021-04-23 10 53 50

@citizenmatt
Copy link
Member Author

To follow up on this, there are several things happening:

Firstly, in 211 and earlier, there is a bug in the platform that always makes Kotlin inlay hints apply to following text, instead of applying some to preceding text, like this type hint.

Secondly, the current code actually positions the caret incorrectly, because at the time it's positioned, the current mode hasn't yet been changed, and we calculate inlay handling for the wrong mode/caret shape.

Thirdly, in 211 and earlier IntelliJ would reinitialise editor settings whenever the caret changed between bar and block shape. This did a lot of unnecessary extra work (see IDEA-262153), including repositioning the caret to the current offset, in case e.g. tab size has changed. This will also recalculate visual positioning with regard to inlay hints, and ironically, repositions the caret to the original desired location.

212 has updated the handling of the bar/block caret switch handling to do less work, and no longer resets the caret location, so no longer "fixes" the incorrect location for IdeaVim's handling.

I've got a fix locally as part of a refactoring of caret handling. I hope to have a PR soon.

citizenmatt added a commit to citizenmatt/ideavim that referenced this pull request Jun 29, 2021
Fixes an issue where the caret was incorrectly positioned because it was moved before the mode was changed. This wasn't visible in 211 because a couple of bugs in the platform combined to put the caret in the right place.

See JetBrains#280, IDEA-262153 and KTIJ-3768
@citizenmatt citizenmatt mentioned this pull request Jun 30, 2021
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