Fix some local options being used as global options #619
Conversation
|
Looks like GH wasn't able to handle it automatically :D |
|
😮 |
|
I'll rebase and force push 😀 |
Helper functions now take the editor rather than the text, ready for search to rely on per-editor options (i.e. '`iskeyword'`). Also standardises on `Int` for search parameters. While the file size is a `Long`, the editor returns a `CharSequence`, which is indexed by `Int`.
citizenmatt
force-pushed
the
refactor/fix-local-options
branch
from
f9c1e23 to
0c6425e
Compare
citizenmatt
force-pushed
the
refactor/fix-local-options
branch
from
0c6425e to
141b9bb
Compare
|
Rebased and ready for review, @AlexPl292, @lippfi |
| @@ -95,7 +97,8 @@ internal class NotificationService(private val project: Project?) { | |||
| "set ideajoin", | |||
| "idejoin" | |||
| ) { | |||
| injector.globalIjOptions().ideajoin = true | |||
| // 'ideajoin' is global-local, so we'll set it as a global value | |||
| injector.optionGroup.setOptionValue(IjOptions.ideajoin, OptionScope.GLOBAL, VimInt.ONE) | |||
There was a problem hiding this comment.
Why we don't have some accessors API for this case and use direct setOptionValue?
There was a problem hiding this comment.
Mainly because this is a global-local option which we don't currently support properly (but will in the next PR, currently in progress 😁).
The accessors are aimed at the common case of getting/setting the "effective" value of an option, but we don't really support "effective" scope in the API, only GLOBAL and LOCAL, and this makes it hard to handle global-local options correctly. Global-local options are usually global, but can be overridden with a local value, so the ideajoin accessor is a property on the EffectiveIjOptions class, with a scope of LOCAL(editor). Getting the value from the accessor works fine, as a side effect of the implementation - because we lazily initialise options, when the local value isn't set, we fall back to the global value. But set will set the value at LOCAL scope, which is not what we want here.
So for now, we need to explicitly set it at GLOBAL scope via setOptionValue.
I'm introducing an AUTO scope in the next PR which is the "effective" scope of an option, and the option accessors will use this to get/set at the right scope. The scopes now map closely to :set, :setglobal and :setlocal, and the API behaves the same as the commands. Given AUTO scope and a global-local option, getOptionValue (and the accessors) will return the local value if set, global otherwise. setOptionValue follows Vim behaviour and updates the global value, and resets/updates the local value. (It's important to update the global value so that we can eagerly initialise options for new windows, which is itself important for options that are required at window initialisation time, such as, finally, 'wrap' 😅)
All of which means that with the next PR, we can use the accessors API to set this value.
| @@ -16,7 +16,9 @@ import com.maddyhome.idea.vim.vimscript.model.datatypes.VimDataType | |||
| public abstract class VimOptionGroupBase : VimOptionGroup { | |||
| private val globalOptionsAccessor = GlobalOptions() | |||
| private val globalValues = mutableMapOf<String, VimDataType>() | |||
| private val globalParsedValues = mutableMapOf<String, Any>() | |||
There was a problem hiding this comment.
This makes me questioning how do we work in a multiconcurrent environment. And I'm nore really sure if we support multiconcurrency, but what abut changing this to some concurrent map "just in case"?
There was a problem hiding this comment.
I'd rather do it deliberately because we know it's being accessed concurrently, rather than just in case. And I think if this could be accessed concurrently, then so would a lot of other data structures we have. Or to put it another way, I'm not using this map any differently to the other maps in the class. We should probably look at this more deeply to see what our concurrency is.
|
Thank you, Matt! |
This PR is based on #616, and therefore includes all commits from that PR. I will rebase and update the PR after #616 is merged (or maybe GitHub automatically manages it. Let's find out!).(Rebased and force pushed on top ofmaster)Some options that should be treated as local options by the accessor APIs introduced in #616 are accessed as global options because there is no
editoravailable. This includes'iskeyword','matchpairs', and'virtualedit'as well as'ideajoin'and'idearefactor'.This PR passes the current editor around so that these local options can be properly used as local options. Note that
'virtualedit'is documented to be a global-local option, which means it's global, but can be overridden locally. We treat it as a local option, and the current implementation means we read the local value if set, and fall back to global value if not, which gives us a good approximation (:setwrites to the global value, which also helps). The same applies to'ideajoin'and'idearefactor'.I've also introduced
VimOptionGroup.getParsedEffectiveOptionValueas a simple cache for parsed values. If the cached, parsed value exists, it is returned. If not, the effective value is passed to a given provider to produce a parsed value, which is cached until the option is next changed. This reduces the need for a couple of listeners.