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

Add 'wrap' and other IDE-backed options #795

Merged
merged 26 commits into from
May 10, 2024

Conversation

citizenmatt
Copy link
Member

@citizenmatt citizenmatt commented Jan 8, 2024

This PR adds support for various Vim options that map to IntelliJ settings, such as 'wrap' and 'textwidth'. These options are implemented by the IntelliJ editor, rather than IdeaVim/vim-engine. It can also support options that are implemented by IdeaVim, but have equivalent settings in IntelliJ, such as 'scrolloff'.

  • Add the 'wrap' option to toggle soft wraps - VIM-1265
  • Add the 'breakindent' option to toggle indenting soft wraps to match the initial indent - VIM-2748
  • Add the 'list' option to toggle showing whitespace - VIM-267
  • Add the 'cursorline' option to toggle highlighting the current line
  • Add the 'textwidth' option. This is a local-to-buffer option and setting it will update the value in all open editors for the window. This Vim option maps to two IntelliJ settings - enabling wrap on type and the right margin value. It does not modify or reset the right margin when setting 'textwidth' to 0, mainly because the right margin visual guide is always displayed, and moving or hiding it would be unexpected for the user - VIM-1310
  • Add the 'colorcolumn' option to control visual guides. In Vim, you can add "+0" to the option to show the 'textwidth' value. IntelliJ ties the hard wrap margin visual guide to visibility of other visual guides, so we add this automatically. That is, if you :set colorcolumn=10,20,30, then the result of set colorcolumn? will always be colorcolumn=10,20,30,+0. You can disable visual guides, including the hard wrap right margin with :set colorcolumn=.
  • Map 'number' and 'relativenumber' to IntelliJ's number settings.
  • Support 'fileformat', 'bomb' and 'fileencoding' options. Vim will use these settings while saving, but IdeaVim will use IntelliJ's own actions to modify the file immediately.
  • Map 'scrolloff'/'scrolljump' and 'sidescrolloff'/'sidescroll' to their equivalent IntelliJ settings - VIM-3110
  • Fix incorrectly re-initialising options when the plugin is disabled and re-enabled. The option values are not cleared when disabling the plugin, so can be reused, and re-initialising would also reset any mapped IntelliJ values.

Other important changes:

  • The 'number' and 'relativenumber' options have been moved out of vim-engine, and placed in the main IdeaVIM module. There isn't an IdeaVim implementation of these options, only an integration with a host implementation. Fleet will need to provide it's own integration, and also register the options itself.
  • The relative line number painter has had minor improvements so that it works as expected with visual lines and soft wraps. Previously, it would show relative count from the caret's buffer line, rather than the caret's own visual line. This still isn't correct for Vim's handling of soft wraps, but is a better implementation for simple visual line movement.
  • Fix the somewhat confusing behaviour of :set[local] {option}< to reset an option back to the global value.

Mapping an IntelliJ setting to a Vim option is intended to behave intuitively for the user. When setting the Vim option, the equivalent IntelliJ setting is only updated if the Vim option is being set as a result of :set or :setlocal - when setting the default, the IntelliJ setting is not modified. When getting the option value for e.g. :set wrap?, IdeaVim will always use the current IntelliJ setting value. This means Vim options are effectively opt-in. Unless the user explicitly sets the option through the command line or in ~/.ideavimrc, then the current IntelliJ setting is used as normal, including when IdeaVim uses current option values to initialise new windows.

@citizenmatt citizenmatt force-pushed the feature/ide-backed-options branch 2 times, most recently from 39648d2 to 21599fb Compare January 10, 2024 01:40
@citizenmatt
Copy link
Member Author

citizenmatt commented Jan 10, 2024

I'm going to try again to add some more context without writing a massive essay 😁

Key points about the implementation:

  • IntelliJ is the source of truth - getting the value of a mapped Vim option will always get the current IntelliJ value. Changing the value of an IntelliJ setting is shown in e.g. :set wrap?.
  • Opt in to Vim options - unless a user explicitly sets a Vim option, the IntelliJ setting is not updated.
  • Introduce wrapper types for option values - IdeaVim internally stores Vim option values using wrapper types that track if a value is a default or has been explicitly set by the user.
    • If the wrapper type shows that we're initially setting a default value, the IntelliJ setting is not updated
    • If it shows we're setting an explicit value, we update the IntelliJ value
    • If it shows we're resetting an option back to default, we update the effective IntelliJ value to the global IntelliJ value, and mark it as default again
  • Only map effective values - we can sidestep the issue of different Vim option/IntelliJ setting scopes by only mapping the effective values. Vim's global value is only used for initialising new windows, so it is not necessary to map. Also, setting a persistent global IntelliJ value when using :setglobal and especially when using :set would likely be an unwelcome surprise to the user.
    • Local-to-buffer Vim options are handled by ensuring that the editor-local IDE value is updated in all editors for the current buffer/document.
    • Global Vim options such as 'scrolljump' are "faked" by setting the editor-local IDE value for all editors. Fortunately, the IntelliJ settings for these Vim options do not have a UI to set the editor-local IDE value, so we can be confident that when we set it, it's correct.
    • Global-local Vim options work similarly to global options - the global value is "faked" by setting the editor local IDE value for all editors, and overridden with the local Vim value per-editor.
    • Changes to mapped options will update the local/editor value of IntelliJ's global-local editor settings. For some settings such as 'wrap' this is fine, because there is a UI for updating the editor-local value. For others, such as textwidth, there isn't a UI for editing the IDE values at the editor level. Changing the (global) values in the UI won't necessarily update an open editor. The editor must be either reopened or reset to defaults with e.g. :set textwidth&. The user has opted in to Vim options with :set, so it's reasonable that the IntelliJ UI does not update those editors.
  • The IdeaVim values are overridden, not replaced - IdeaVim always stores what it thinks the values of Vim options are - not the overridden values. This previously stored value is passed to the override so it can tell if the current value has been changed in the IDE, which is useful when initialising new windows.
  • Initialising new windows works intuitively - IdeaVim uses the current global value (not mapped) to initialise a new window. This happens internally, using the wrapper types to know how the values should be set.
    • If the global value is default, the new window is initialised to default, and the IntelliJ setting is not modified. This uses IntelliJ's defaults for opening a window, and the effect is intuitive to the user.
    • If the global value has been explicitly set with :set, the effective value is also set as the global value, and this global value is used to initialise the new window, as expected in Vim initialisation.
    • If the value has been explicitly set with :set, but the effective value changed through the IDE, the IDE change is treated like a call to :setlocal - it only affects the local option. The new window will be initialised with the current global value instead.
    • Some initialisation scenarios also copy the current local value of local-to-window options. This is copied and applied using the wrapper types, so behaviour is very similar to above - default means no changes to the IntelliJ setting, explicitly set by user or by IDE means the IntelliJ setting is updated.

@citizenmatt citizenmatt marked this pull request as ready for review March 4, 2024 10:08
@citizenmatt
Copy link
Member Author

This PR is now ready for review. I've implemented all of the Vim options that have an equivalent IntelliJ implementation. The only exceptions are 'expandtab', 'tabstop' and 'shiftwidth' (VIM-1395). There is currently no way to modify these values so that the editor and code formatter pick them up.

@AlexPl292 AlexPl292 self-requested a review March 27, 2024 11:59
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.

Thank you for the massive change! Generally, I'm ready to merge it, but there are a few things I'd like to discuss.

I left two comments on the code where I'm confused about how the explicit option set works with changing the options in IntelliJ UI.

Generally, I believe two approaches would be good in case the user explicitly sets the option while it conflicts with the other source (IJ or Vim). Let's assume, the user has set nu in ~/.ideavimrc and tries to untick the checkbox in IJ settings:

  • First way is to disable line numbers in all editors. Because if the user changes a setting like that, they must expect it to come into effect immediately. This would be confusing the the setting is changed, but in the editor nothing happened. The ~/.ideavimrc file is not often modified, so it's easy to forget that it contains an explicit set nu.
    However, as I understand, this would be a bit complicated to implement because Vim still has an explicit value of the number option.

  • Second, probably more preferable way - avoid complications and disable IJ setting. We don't have this feature in the platform, but in case the user has an explicit set nu, we can turn off the setting in UI and add some tooltip explaining that this option is disabled because of Vim.

Generally, as I understand, this PR is mostly not about how the user actions in UI are processed, but how Vim processes the state of the IntelliJ settings regardless of how they are set. The chosen approach looks good to me, but I'm worried about the UX interaction for the user. I think this is confusing if the user changes an option in the settings and it doesn't change anything. Also, if you have explicit set nu and you right-click line numbers -> appearance -> untick show line numbers, the line numbers will be disabled in the current editor but not in the other editors. And the general option in IJ setting will be disabled, what usually means that all editors should not have the line numbers.

Also, I can't quite catch it, how were you able to turn off update of the editors when the setting in IJ changes?...

@citizenmatt
Copy link
Member Author

The right click in the gutter for line numbers can be a little confusing, because it's only showing the global value of the setting, while the line numbers might be enabled by the View | Active Editor | Show Line Numbers local setting, which is what IdeaVim sets. It's better to test with the "show line numbers" options in Search Everywhere, because you can see what the global and effective values are for the current editor.

If we use these actions, the current editor correctly updates when either local or global value is set, including when the Vim option has been explicitly set. Setting the local value is correctly reflected in the line numbers and the Vim option. Setting the global value will either set it to match the current local value (so no update) or also update the local value, and the line numbers and Vim option reflect this.

However, setting the global option with these actions does not necessarily affect other open windows. If the Vim option has been explicitly set, or if the user has set View | Active Editor | Show Line Numbers, the local value is already set, so changes to the global value have no effect, and only the current editor's local value is reset by the platform. Setting the global option in the Settings dialog does not even update the current editor.

Because of this, Vim options are essentially "opt-in". Once explicitly set, either in ~/.ideavimrc or on the command line, the local IntelliJ value is also set, and depending on the action used, this local value isn't necessarily updated by the platform when the global value changes. It is possible to use :set number& to reset the current window back to the global value, but this is only a copy of the global value, and further changes are not automatically reflected.

I was under the impression that there was no notification when a setting is changed, but I realise I was only looking at Editor.settings - local settings. There is a notification from EditorSettingsExternalizable.addPropertyListener. We can get notified when the global value is changed, which might allow us to react in a more intuitive manner.

However, I'm not sure on expected behaviour. Vim doesn't have a way to reset local options in all open windows, and IntelliJ does not reset the local values in all open windows either. On the other hand, this PR makes use of local values more than the average IntelliJ user would, so perhaps the least surprising thing would be to treat it as though there were no local values and reset them. We might also be able to tell when a local IntelliJ value is not an explicitly local value, but a local value set to support IdeaVim.

I'll have a think about how this could be implemented, and if the behaviour would be more intuitive.

@citizenmatt
Copy link
Member Author

I've been thinking about the different approaches we can take when the global IntelliJ setting changes, and I think the best thing we can do is to start tracking when the option is set. If it's last set from ~/.ideavimrc during startup, then we can kind of, just about, maybe, possibly consider it to be a "global" value. (It's not, Vim does have actually global values, but they serve a different purpose. This could be considered "global" in that it applies to all subsequently opened windows). And if it's a "global" value, then it's reasonable to update the editors when the actual IntelliJ global value changes.

If the value is set interactively, then you're explicitly saying you want it set locally, and changing the global IntelliJ value shouldn't modify this window/buffer. If the user sets the value from IntelliJ, with something like View | Active Editor, then they're setting it as a local value, and global changes won't modify it. IntelliJ currently works like this, and we won't be changing that. If the user sets it through IdeaVim, with :set or :setlocal, they're also setting it as a local value (most options in this PR are local to buffer/window), and again, we shouldn't override it. The user can reset to the global value with :set number&.

Interactively sourcing or reloading ~/.ideavimrc would not mark the option as special. This is because the script would be run in the context of the current editor, and wouldn't affect open windows. It is only special during startup, because it affects all subsequently opened editors.

I think I'm pretty happy with this. It feels like a good compromise between existing IntelliJ behaviour (explicitly set, don't override) and expected user behaviour (forgotten it's set in ~/.ideavimrc - why aren't my settings updating?) What do you think?

Also, what shall we do about this PR? Do you want to merge it and have these changes as a separate PR? I'm not likely to get this done before the end of next week.

@AlexPl292
Copy link
Member

Yes, this sounds good to me, at least from the description.
If you don't mind, I'll keep the PR open, and let's make further improvements here. I don't see a reason to merge this request if we'll change the behaviour in other updates.

@citizenmatt
Copy link
Member Author

Okay. I'm going to rebase and force push before I do any more, fix that conflict, etc.

@citizenmatt citizenmatt force-pushed the feature/ide-backed-options branch 2 times, most recently from a0ba209 to 9562133 Compare April 12, 2024 23:12
@citizenmatt
Copy link
Member Author

Force pushed with new updates. IdeaVim updates the editor's local override of settings when a Vim option changes, instead of changing the persistent global value. When the user changes the global value of an IDE setting with the Settings dialog, IdeaVim will now reset the editor's local setting override if:

  • The Vim option is global
  • The Vim option is global-local and the local option has not been set
  • The Vim option is a local option and was set during plugin startup (the global value is updated and used to initialise new windows, so this value propagates to other editors)

Essentially, if the user has explicitly and interactively set the effective value of a local option, or the local value of a global-local value, we respect that and do not modify the editor's local setting override. If the value was set during startup, then it's easy to consider that a global value, and it should be updated when the IDE's actual global value is changed.

@citizenmatt
Copy link
Member Author

I've updated the set-commands.md file to add all the new options in. It should be copied over to the wiki when merged.

Also supports overriding local-to-buffer options with IDE values, ensuring that changes to the option/IDE value are applied to all editors for the buffer.

Fixes VIM-1310
IntelliJ ties the hard wrap right margin guide with the other visual guides, and it's not possible to show one without the other. In Vim, you can show the hard wrap margin by adding "+0" to 'colorcolumn', so in IdeaVim, we automatically add this.
While they are core Vim options, they are implemented by the host, not by the engine. If another host wants to support these options, they can add them in their implementation layer.
It now shows visual lines relative to the caret's visual line, rather than relative to the caret's logical line. This still isn't correct, and we should be showing the relative count of Vim logical lines (buffer lines + fold lines) but this matches movement so is more helpful
No tests, as I don't know how to test interaction with saving to disk
No tests, as I don't know how to test interaction with saving to disk
No tests, as I don't know how to test interaction with saving to disk
String and number/toggle options have different and opposite behaviour for `:set {option}<` and `:setlocal {option}<`. This change matches Vim's behaviour.
Options are not reset if they've been explicitly set by the user (e.g. `:set list` or _View | Active Editor | Show Whitespaces_). They are reset if they were explicitly set in `~/.ideavimrc`.

Also bumps the IDE build number to 233.11799.241 in order to use EditorSettingsExternalizable.PropNames
@AlexPl292
Copy link
Member

This was epic, thank you!

@AlexPl292 AlexPl292 merged commit d00e802 into JetBrains:master May 10, 2024
3 of 4 checks passed
@citizenmatt citizenmatt deleted the feature/ide-backed-options branch May 10, 2024 12:35
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