-
Notifications
You must be signed in to change notification settings - Fork 764
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
Eagerly initialise local options and introduce option scope #667
Merged
AlexPl292
merged 25 commits into
JetBrains:master
from
citizenmatt:feature/eagerly-initialise-options
Jul 31, 2023
Merged
Eagerly initialise local options and introduce option scope #667
AlexPl292
merged 25 commits into
JetBrains:master
from
citizenmatt:feature/eagerly-initialise-options
Jul 31, 2023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AlexPl292
reviewed
Jul 26, 2023
vim-engine/src/main/kotlin/com/maddyhome/idea/vim/options/OptionDeclaredScope.kt
Outdated
Show resolved
Hide resolved
src/test/java/org/jetbrains/plugins/ideavim/option/OptionDeclaredScopeTest.kt
Show resolved
Hide resolved
vim-engine/src/main/kotlin/com/maddyhome/idea/vim/options/OptionScope.kt
Show resolved
Hide resolved
vim-engine/src/main/kotlin/com/maddyhome/idea/vim/options/OptionScope.kt
Outdated
Show resolved
Hide resolved
vim-engine/src/main/kotlin/com/maddyhome/idea/vim/vimscript/model/commands/SetCommand.kt
Outdated
Show resolved
Hide resolved
All issues resolved. I need to rebase against current master and will force push |
Previously, all local options were treated as local-to-window
Note that this temporarily changes the semantics of `:set` to always set the local option, instead of setting the global option (because we now eagerly initialise local values). Neither is correct, but we don't yet have a way to support the proper behaviour.
Matches Vim behaviour for `:set all&`
citizenmatt
force-pushed
the
feature/eagerly-initialise-options
branch
from
July 30, 2023 10:19
cb2bf6f
to
f096c85
Compare
Rebased and force pushed |
Awesome work! Merged! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is primarily intended to eagerly initialise local options, rather than the current lazy initialisation when get or set. This is important for future development of options that are backed by IDE functionality, such as
'wrap'
, which will control the IDE's soft wrap settings, and obviously need to be initialised when the editor is first opened (or initialised after plugin startup).However, Vim's option handling is complex and subtle. In order to know what options to initialise, and what value they should be initialised to, it is necessary to handle option scopes - global, local to buffer, local to window and global-local. Now might be a good time to get a coffee.
This PR will:
OptionDeclaredScope
and update each option declaration to specify its scope - global, local to buffer, local to window or global-local.'iskeyword'
and'matchpairs'
. The local value for these options is stored in the buffer (document) and shared between all open windows (editors). (Previously, local options were all "local to window")'scrolloff'
. These options are usually global, but can be overridden local to a buffer, or local to a window. This includes support for "unset" local values (e.g.:setlocal scrolloff?
will return-1
).OptionScope.AUTO(editor)
to get/set the "effective" value of an option. This meansOptionScope
now maps nicely to:setlocal
,:setglobal
and:set
.:set
) of a global option will update the global value. Setting the effective value of a local value will set the local value, but also the global value, so that we keep a track of the most recently set value. This is used during initialisation. Setting the effective value of a global-local option will set the global value. If the local value is set, it will be reset.AUTO
scope. Anyone that needs to know the value of an option in code will be interested in the effective value, and so should useAUTO
. Only:setlocal
,:setglobal
or:let
should need to use the explicitGLOBAL
orLOCAL
scopes (with the exception of extension management and some obsoleted compatibility functions).:setglobal
command.:set {option}<
syntax to reset an option back to the global value. This works for:setlocal
and:setglobal
too (although:setglobal {option}<
is effectively a no-op).:set all&
to reset all options to default values for the current editor only. The interesting side effect is that local to window options are reset for the current window (editor) only, but local to buffer options are changed for all windows for the current buffer, and global options are still reset for all windows. This matches Vim behaviour.:let &{option}
command to treat options without al:
org:
prefix asAUTO
scope, and behave like:set
.GlobalOptionChangeListener
to listen for changes to the value of a global option. It can only be used for global options, and will be called once per change. It does not pass any context, and is intended to be used by functionality that is not bound to a single editor, e.g.'showmode'
.EffectiveOptionValueChangeListener
to listen to changes to the effective value of an option. The option can be global, local or global-local. It will be called multiple times per change, once for each affected editor. This allows the callback to simply update the affected editor, rather than have to manage iterating over editors.Details on how Vim initialises options can be found in
:help local-options
and:help option-summary
. As it states, Vim mostly uses the value you'd expect, but the implementation of that is rather complicated.While not explicitly stated, it appears that Vim tries to make new windows look and feel like the window that opened them, ignoring any options that were explicitly set locally. In order to do this, local options maintain two values - a local value, and a "global" value. The
:set
command will update both the local effective value and the global value, to track the most recently set value. Initialising a new window will copy the global value of the opening window to the effective value of the new window. Initialising a split window however, will copy both the global and local values, so it acts more like a clone operation.This means that an option might be set in one window, but a second different window, with a previously set effective value, might be used to open a new window. The new window will get the global value set in the first window, not the effective value of the second, opening window.
This makes most sense for "local to buffer" options, which usually represent non-visible behaviour, such as
'fixendofline'
. Since there is nothing visible to compare, the user will intuitively expect the same end of line behaviour that they last, explicitly, set.However, this does not make sense for visual options, which are usually "local to window", such as
'wrap'
and'number'
. The user would naturally expect the new window to look like the opening window, but it could be initialised from a global value that was set from a different window. To handle this scenario, Vim's "local to window" options don't have a single global value, but a per-window "global" value (it is still referred to in the docs as global). This PR does not implement per-window "global" values (it's already big enough!), and this will be implemented in the next PR.This PR will initialise a new window by setting all "local to buffer" or "local to window" options' local value to the global value of the opening window's option. If the new window is a split (the new editor's document/buffer is the same as the opening editor's document/buffer), then the new window's local values are set from the local values of the opening window, so the new window looks and acts like the opening window.
The opening window is always the IDE's "current" editor. This gives us the correct opening window if it has focus (e.g. typing
:edit {file}
in the ex text field, or ctrl+click/Go to Definition), and is the last used editor if the focus is elsewhere (e.g. Search Everywhere, Project view, Find Usages, etc.).If there are no editor windows open, there is no current editor and this PR will initialise options to the default global value. Since per-window "global" is not yet implemented this currently has no impact (a split window is the only scenario that requires a local value, and there is always an opening window in that scenario). Once per-window "global" is supported, this will have an impact and will also be addressed in the next PR.