guicursor! #337

citizenmatt · 2021-06-30T20:23:18Z

IntelliJ 2021.2 resolves the 16 year old issue IDEA-30308 to add more caret shapes (initially raised for IdeaVim! 😁). This PR uses the API to change the cursor based on the current mode and according to the the 'guicursor' option. This gives visual feedback for replace and op-pending modes, and also change character actions.

Screen.Recording.2021-06-30.at.6.11.45.pm.mov

Caret shape management is centralised, and mostly only changes when the mode changes. The most significant exception to this is during mouse selection, which forces the bar caret for more intuitive feedback
If the surround extension is enabled, there is a pause before changing the caret shape for op-pending. This is expected, as surround adds mappings for c, d, etc. and the mappings must timeout first (the same behaviour is seen in Vim).
Vim does not change the caret shape for select mode, but IdeaVim sets it to the insert mode caret. IdeaVim makes much more use of select mode than Vim, e.g. as the default for 'idearefactormode', so it makes sense to provide better visual feedback. By using the insert mode caret, select mode is intuitively seen as a selection with insert mode.
ExTextField is updated to draw the caret in line with the 'guicursor' options. Caret painting has been improved, too, showing the text while flashing, and resized to better fit the character grid.
The changes require 212, but the code will check the app version at runtime. If running on an older version of IntelliJ, the existing caret behaviour is used. If running on 212, the new caret API is used. This might mean that the Marketplace will email warnings about using incorrect APIs - IdeaVim requires 202+ but is (conditionally) using a 212 API. It might be possible to suppress this, or simply ignore the emails until 212 becomes the base version.
It is possible to test running against previous versions by setting the ideDir property for the runIde task in build.gradle.kts. E.g.:
```
runIde {
    ideDir.set("/Users/matt/Library/Application Support/JetBrains/Toolbox/apps/IDEA-U/ch-0/211.7628.21/IntelliJ IDEA 2021.1 EAP.app/Contents")
} 
```
The "use block caret" setting is now completely ignored ~~for 212+. It is still used to control the caret shape in 211 and earlier.~~ This has an impact on VIM-1475 for 212+. Control of the caret shape for 212+ requires updating 'guicursor'. Once merged, a comment should be added to VIM-1475. (Edit: I forgot that I deliberately removed this, in favour of 'guicursor', which offers more control, even for 211)
The "use block caret" setting is treated as an override to the 'guicursor' option. If "use block caret" is checked, it's treated as "use block caret everywhere", and block caret is used for all modes. If it's unchecked, then IdeaVim will use 'guicursor' handling.

A couple of misc bugs fixed along the way:

Caret visual position is also reset after a mode change. This will update visual position around inlays, which depends on caret shape - i.e. bar caret should be between the inlay and the preceding text, while block caret (or underscore) should never be drawn at the same visual position (over) an inlay. This fixes this comment from #280, as well as addressing IDEA-262153 and KTIJ-3768.
The ExShortcutKeyAction is now dumb aware. This had the unfortunate side effect of commands such as insert digraph (<C-K>) and insert register (<C-R>) not being available during indexing.

This PR also updates gradle.properties back up to latest EAP. This was downgraded to 211, but I don't know why.

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

Removes updateCaretState and unnecessary usages

We should avoid setting the shape explicitly, and let it update when the mode changes. Note that shape can affect the visual position of the caret around inlays (e.g. 'a' at the end of a rename hotspot with a trailing inlay for options will remain in between the text and the inlay, while 'l' in command mode will move after the inlay. Both positions are at the same text offset). We should still avoid explicitly setting shape before moving the caret. We can't guarantee the order of changing mode and moving the caret, so we update the visual position at the current offset when changing mode. (We're also currently using mode as an assumption of shape)

Changing EditorSettings.setBlockCursor does this via EditorImpl.updateCaretCursor

This changes VIM-1475 and ignores IntelliJ's "use block cursor" setting in favour of guicursor. Also affects caret placement around inlays and handling of template hotspots via idearefactormode

AlexPl292

Wow, that's an impressive update!

src/com/maddyhome/idea/vim/command/CommandState.kt

AlexPl292 · 2021-07-01T11:39:50Z

src/com/maddyhome/idea/vim/listener/VimListenerManager.kt

-        val caret = e.editor.caretModel.primaryCaret
+
+        /**
+         * If we make a small drag when moving the caret to the end of a line, it is very easy to get an unexpected


Oh Matt, thank you very much for finding time to explain the things that I had to explain.

src/com/maddyhome/idea/vim/option/TextOption.java

AlexPl292 · 2021-07-01T13:45:31Z

src/com/maddyhome/idea/vim/command/CommandState.kt

@@ -319,10 +339,10 @@ class CommandState private constructor() {
  }

  enum class SubMode {
-    NONE, SINGLE_COMMAND, REGISTER_PENDING, VISUAL_CHARACTER, VISUAL_LINE, VISUAL_BLOCK
+    NONE, SINGLE_COMMAND, REGISTER_PENDING, REPLACE_CHARACTER, VISUAL_CHARACTER, VISUAL_LINE, VISUAL_BLOCK


Do we add this new mode to catch the moment when the r is pressed, but replaced character is not yet typed by user?

I just don't understand why REPLACE mode is not enough.

Yes, that's it.

I introduced REPLACE_CHARACTER because I don't think this is a good fit for the existing REPLACE mode. It's tracking the state of an incomplete command, rather than actually switching into REPLACE mode. It doesn't feel right to do a full mode switch in the middle of command entry - we're not actually in replace mode, we're waiting for an argument to a command that will do a replace. We want to look like we're in replace mode, but not actually be in it.

And pragmatically, if we used REPLACE, we'd still need another piece of state to know when to pop the REPLACE mode if hitting <Esc> or entering an incorrect digraph. E.g. handleDigraph doesn't currently pop modes, so if we're entering a digraph, we don't know if we're entering it in actual replace mode, or as the argument for an incomplete action. The sub mode gives us everything we need, and tells us we're still in COMMAND or VISUAL mode (which we are, waiting for a command to complete), and gives us something we can update the caret with.

Okay, thank you for explanation. I have some thoughts about removing subMode at all because it doesn't exist in vim, but it might be a future refactoring. Let's use our current structure.

AlexPl292 · 2021-07-01T13:48:25Z

src/com/maddyhome/idea/vim/group/EditorGroup.java

-  public boolean isBarCursorSettings() {
-    return !EditorSettingsExternalizable.getInstance().isBlockCursor();
-  }
-
  public void editorCreated(@NotNull Editor editor) {


What if the user has this option enabled? They'll get a thin caret until they update the guicaret option, right?

No. For 212, this setting is mostly ignored. It controls whether the default caret is block or bar, and we don't use the default caret. If the user changes this setting during a session, the editor will redraw and use the default caret. This will remain visible until the user causes the Vim caret to change - changing mode or mouse drag.

Now, wait, I believe we use this property. In this way, the user can keep the block caret in insert mode. I don't remember how this support was enabled, was it a user request or something.

Would it be complicated to support this option to keep the block caret in insert mode with this option? In favor of backward compatibility. And this might be a simpler option to use block caret instead of diving into the guicursor option.

But if it's complicated, let's leave it without this option.

For 212, this setting is mostly ignored

My mistake. For 211 and 212, this setting is completely ignored :) The rest of the comment is right though - if the user changes the setting, it will be set until something Vim happens and we redraw the caret.

The request originally came from VIM-1475. I think 'guicursor' supersedes this, tbh, and so I removed it. It offers more control, and is the Vim way to control Vim behaviour. Personally, I think that if there is an IntelliJ and a Vim way to control Vim behaviour, the Vim way should take precedence. Having two settings provides two sources of truth, and it's not obvious which one wins - if the setting is checked, we'll use a block cursor, but if it's not checked, we'll use whatever 'guicursor' is set to use, which might be block, bar or underscore. If the setting is checked, should it apply to all modes, or just insert?

(An alternative would be some kind of sync mechanism, which would also be useful for things like line numbers and, eventually, wrap. But here it would still be difficult to map between them - would checking the option update 'guicursor' to block in all modes, or just insert, or what? There's also the question of per-Editor settings vs global IdeaVim options, but that's a bigger conversation)

Personally, I would prefer not to add the VIM-1475 solution back. That said, it can be done pretty easily, especially if we just say that "use block cursor" is a complete override - no matter what mode, just use block. You're the boss, your call :) Happy to go with your decision.

Done. Added support for the "use block caret" setting to override 'guicursor'. If set, "use block caret" now means "use block caret everywhere", and it is used for normal, insert, replace, and all other modes. If not set, we assume that means "use guicursor" and apply the appropriate caret shape.

See VIM-1756

# Conflicts: # src/com/maddyhome/idea/vim/listener/VimListenerManager.kt

AlexPl292 · 2021-09-29T10:02:30Z

src/com/maddyhome/idea/vim/helper/MessageHelper.kt

-object MessageHelper : AbstractBundle(BUNDLE) {
+object MessageHelper : AbstractBundle(IDEAVIM_BUNDLE) {
+
+  const val BUNDLE = IDEAVIM_BUNDLE


Sorry, why this variable was introduced?

Maybe it was to get the IDE inspections working to show the actual value of IDs passed to MessageHelper.message? But I don't really remember, and can't see how that change would help. Sorry...

AlexPl292 · 2021-09-29T10:08:03Z

I'll merge these changes after 1.7.3 bugfix release 👍

citizenmatt · 2021-09-29T23:04:42Z

Great, thanks!

# Conflicts: # src/com/maddyhome/idea/vim/helper/MessageHelper.kt # src/com/maddyhome/idea/vim/option/OptionsManager.kt

AlexPl292 · 2021-10-05T06:28:40Z

Finally merged! Thank you for a better caret!

citizenmatt added 28 commits June 4, 2021 16:45

Refactor showMode update

646a89d

Extract caret shape methods to helper

b50281f

Refactor caret shape functions

6c4bd9c

Remove resetCaret

e859b1c

Update caret visual attributes when mode changes

9c71b44

Removes updateCaretState and unnecessary usages

Rename "bound" options to "bounded"

dfbec1f

Simplify BoundedListOption

287ba70

Make ListOption generic + create StringListOption

e93a619

Fix typo

9f46e19

Add guicursor option

43620c2

Report errors while parsing guicursor option

9cf0a1a

Add guicursor to dictionary

8a55199

Provide hook for resetting cached values

ccd792b

Minor refactor

d19c776

Use guicursor options to draw caret

ad19dc0

Rename assert method

b4d40fa

Use replace mode caret for change character action

a6087dd

Force the caret visible when updating attributes

2f73dac

Changing EditorSettings.setBlockCursor does this via EditorImpl.updateCaretCursor

Use guicursor instead of editor cursor settings

f051231

This changes VIM-1475 and ignores IntelliJ's "use block cursor" setting in favour of guicursor. Also affects caret placement around inlays and handling of template hotspots via idearefactormode

Use guicursor options to draw ex caret

64be751

Improve caret painting in command line

1caf380

Make ExShortcutKeyAction dumb aware

fe7dc49

Use insert caret for select mode

0288a0f

Merge branch 'master' into feature/guicursor

0d840b2

Update to latest EAP

d6a99d4

Run linters

91585e1

AlexPl292 reviewed Jul 1, 2021

View reviewed changes

Merge branch 'master' into feature/guicursor

d63e3da

citizenmatt and others added 6 commits July 29, 2021 21:20

Treat use block caret option as caret override

70a45f3

Add tests for caret attributes in nested modes

6ab5380

See VIM-1756

Merge branch 'master' into feature/guicursor

ae21727

Merge branch 'master' into feature/guicursor

74b0e3b

# Conflicts: # src/com/maddyhome/idea/vim/listener/VimListenerManager.kt

Merge branch 'master' into feature/guicursor

c268407

# Conflicts: # src/com/maddyhome/idea/vim/listener/VimListenerManager.kt

isBlockCursor method doesn't work for UI tests at the moment

3fae2fd

AlexPl292 approved these changes Sep 29, 2021

View reviewed changes

AlexPl292 added the Merge approved label Sep 29, 2021

Merge branch 'master' into feature/guicursor

ee74367

# Conflicts: # src/com/maddyhome/idea/vim/helper/MessageHelper.kt # src/com/maddyhome/idea/vim/option/OptionsManager.kt

AlexPl292 merged commit 067af83 into JetBrains:master Oct 5, 2021

citizenmatt deleted the feature/guicursor branch October 5, 2021 13:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

guicursor! #337

guicursor! #337

citizenmatt commented Jun 30, 2021 •

edited

Loading

AlexPl292 left a comment

AlexPl292 Jul 1, 2021

AlexPl292 Jul 1, 2021

citizenmatt Jul 1, 2021

AlexPl292 Jul 1, 2021

AlexPl292 Jul 1, 2021

citizenmatt Jul 1, 2021

AlexPl292 Jul 1, 2021

AlexPl292 Jul 1, 2021

AlexPl292 Jul 1, 2021

citizenmatt Jul 2, 2021

citizenmatt Jul 29, 2021

AlexPl292 Sep 29, 2021

citizenmatt Oct 5, 2021

AlexPl292 commented Sep 29, 2021

citizenmatt commented Sep 29, 2021

AlexPl292 commented Oct 5, 2021

guicursor! #337

guicursor! #337

Conversation

citizenmatt commented Jun 30, 2021 • edited Loading

AlexPl292 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexPl292 commented Sep 29, 2021

citizenmatt commented Sep 29, 2021

AlexPl292 commented Oct 5, 2021

citizenmatt commented Jun 30, 2021 •

edited

Loading