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.

Sign up for GitHub

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

Fix some local options being used as global options #619

Merged
merged 10 commits into from
May 17, 2023
Merged

Fix some local options being used as global options #619

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
fdd1795
Treat 'matchpairs' as a local option
citizenmatt Apr 1, 2023
64bd48c
Treat 'ideajoin' as a global-local option
citizenmatt Apr 1, 2023
9727476
Treat 'idearefactor' as a global-local option
citizenmatt Apr 2, 2023
1a08fd5
Treat 'virtualedit' as a global-local option
citizenmatt Apr 2, 2023
91510c0
Refactor search helper companion to interface
citizenmatt Apr 2, 2023
e22fdad
Treat 'iskeyword' as a local-to-buffer option
citizenmatt Apr 3, 2023
11a9010
Introduce simple cache for parsed option values
citizenmatt Apr 4, 2023
16ed774
Add helper functions to storage service
citizenmatt Apr 4, 2023
141b9bb
Use options API for more cached values
citizenmatt Apr 4, 2023
a5430ff
Fix typo in ideajoin notification
citizenmatt Apr 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions src/main/java/com/maddyhome/idea/vim/action/VimShortcutKeyAction.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ import com.maddyhome.idea.vim.listener.AceJumpService
import com.maddyhome.idea.vim.listener.AppCodeTemplates.appCodeTemplateCaptured
import com.maddyhome.idea.vim.newapi.globalIjOptions
import com.maddyhome.idea.vim.newapi.vim
import com.maddyhome.idea.vim.options.OptionScope
import com.maddyhome.idea.vim.vimscript.model.datatypes.VimString
import java.awt.event.InputEvent
import java.awt.event.KeyEvent
import javax.swing.KeyStroke
Expand Down Expand Up @@ -255,15 +257,13 @@ internal class VimShortcutKeyAction : AnAction(), DumbAware/*, LightEditCompatib
* if the pressed key is presented in this list. The caches are used to speedup the process.
*/
private object LookupKeys {
private var parsedLookupKeys: Set<KeyStroke> = parseLookupKeys()

init {
VimPlugin.getOptionGroup().addListener(IjOptions.lookupkeys, { parsedLookupKeys = parseLookupKeys() })
fun isEnabledForLookup(keyStroke: KeyStroke): Boolean {
val parsedLookupKeys =
injector.optionGroup.getParsedEffectiveOptionValue(IjOptions.lookupkeys, OptionScope.GLOBAL, ::parseLookupKeys)
return keyStroke !in parsedLookupKeys
}

fun isEnabledForLookup(keyStroke: KeyStroke): Boolean = keyStroke !in parsedLookupKeys

private fun parseLookupKeys() = injector.globalIjOptions().lookupkeys
private fun parseLookupKeys(value: VimString) = IjOptions.lookupkeys.split(value.asString())
.map { injector.parser.parseKeys(it) }
.filter { it.isNotEmpty() }
.map { it.first() }
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/maddyhome/idea/vim/group/ChangeGroup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,7 +454,7 @@ public void indentRange(@NotNull VimEditor editor,
int weoff = editor.bufferPositionToOffset(epos);
int pos;
for (pos = wsoff; pos <= weoff; pos++) {
if (CharacterHelper.charType(chars.charAt(pos), false) != CharacterHelper.CharacterType.WHITESPACE) {
if (CharacterHelper.charType(editor, chars.charAt(pos), false) != CharacterHelper.CharacterType.WHITESPACE) {
break;
}
}
Expand Down
10 changes: 2 additions & 8 deletions src/main/java/com/maddyhome/idea/vim/group/IjOptionProperties.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,6 @@ public open class GlobalIjOptions(scope: OptionScope = OptionScope.GLOBAL) : Glo
public var trackactionids: Boolean by optionProperty(IjOptions.trackactionids)
public var visualdelay: Int by optionProperty(IjOptions.visualdelay)

// TODO: Handle these options as global-local
// Decide if they should live in global or effective options when we support global-local
// (I suspect they should live in effective, because we'll always want to read the local value. We are unlikely to
// ever set from code, but we'd expect normal `:set` behaviour, which appears to be to write to the global value).
// Also double check that these options should be global-local
public var ideajoin: Boolean by optionProperty(IjOptions.ideajoin)
public var idearefactormode: String by optionProperty(IjOptions.idearefactormode)

// Temporary options to control work-in-progress behaviour
public var octopushandler: Boolean by optionProperty(IjOptions.octopushandler)
public var oldundo: Boolean by optionProperty(IjOptions.oldundo)
Expand All @@ -51,4 +43,6 @@ public open class GlobalIjOptions(scope: OptionScope = OptionScope.GLOBAL) : Glo
*/
public class EffectiveIjOptions(scope: OptionScope.LOCAL): GlobalIjOptions(scope) {
public var ideacopypreprocess: Boolean by optionProperty(IjOptions.ideacopypreprocess)
public var ideajoin: Boolean by optionProperty(IjOptions.ideajoin)
public var idearefactormode: String by optionProperty(IjOptions.idearefactormode)
}
7 changes: 5 additions & 2 deletions src/main/java/com/maddyhome/idea/vim/group/NotificationService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,10 @@ import com.maddyhome.idea.vim.key.ShortcutOwner
import com.maddyhome.idea.vim.key.ShortcutOwnerInfo
import com.maddyhome.idea.vim.newapi.globalIjOptions
import com.maddyhome.idea.vim.options.OptionConstants
import com.maddyhome.idea.vim.options.OptionScope
import com.maddyhome.idea.vim.statistic.ActionTracker
import com.maddyhome.idea.vim.ui.VimEmulationConfigurable
import com.maddyhome.idea.vim.vimscript.model.datatypes.VimInt
import com.maddyhome.idea.vim.vimscript.services.VimRcService
import java.awt.datatransfer.StringSelection
import java.io.File
Expand Down Expand Up @@ -93,9 +95,10 @@ internal class NotificationService(private val project: Project?) {
AppendToIdeaVimRcAction(
notification,
"set ideajoin",
"idejoin"
"ideajoin"
) {
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we don't have some accessors API for this case and use direct setOptionValue?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

},
)

Expand Down
6 changes: 4 additions & 2 deletions src/main/java/com/maddyhome/idea/vim/group/visual/IdeaSelectionControl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ import com.maddyhome.idea.vim.listener.VimListenerManager
import com.maddyhome.idea.vim.newapi.vim
import com.maddyhome.idea.vim.options.OptionConstants
import com.maddyhome.idea.vim.vimscript.model.options.helpers.IdeaRefactorModeHelper
import com.maddyhome.idea.vim.vimscript.model.options.helpers.isIdeaRefactorModeKeep
import com.maddyhome.idea.vim.vimscript.model.options.helpers.isIdeaRefactorModeSelect

internal object IdeaSelectionControl {
/**
Expand Down Expand Up @@ -123,7 +125,7 @@ internal object IdeaSelectionControl {
}

private fun dontChangeMode(editor: Editor): Boolean =
editor.isTemplateActive() && (IdeaRefactorModeHelper.keepMode() || editor.editorMode.hasVisualSelection)
editor.isTemplateActive() && (editor.vim.isIdeaRefactorModeKeep || editor.editorMode.hasVisualSelection)

private fun chooseNonSelectionMode(editor: Editor): VimStateMachine.Mode {
val templateActive = editor.isTemplateActive()
Expand All @@ -148,7 +150,7 @@ internal object IdeaSelectionControl {
if (logReason) logger.debug("Enter select mode. Selection source is mouse and selectMode option has mouse")
VimStateMachine.Mode.SELECT
}
editor.isTemplateActive() && IdeaRefactorModeHelper.selectMode() -> {
editor.isTemplateActive() && editor.vim.isIdeaRefactorModeSelect -> {
if (logReason) logger.debug("Enter select mode. Template is active and selectMode has template")
VimStateMachine.Mode.SELECT
}
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/maddyhome/idea/vim/group/visual/VisualGroup.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import com.maddyhome.idea.vim.newapi.vim

internal fun moveCaretOneCharLeftFromSelectionEnd(editor: Editor, predictedMode: VimStateMachine.Mode) {
if (predictedMode != VimStateMachine.Mode.VISUAL) {
if (!predictedMode.isEndAllowed) {
if (!editor.vim.isEndAllowed(predictedMode)) {
editor.caretModel.allCarets.forEach { caret ->
val lineEnd = IjVimEditor(editor).getLineEndForOffset(caret.offset)
val lineStart = IjVimEditor(editor).getLineStartForOffset(caret.offset)
Expand Down
1 change: 0 additions & 1 deletion src/main/java/com/maddyhome/idea/vim/helper/CaretVisualAttributesHelper.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ internal fun Editor.hasBlockOrUnderscoreCaret() = isBlockCursorOverride() ||
internal object GuicursorChangeListener : OptionChangeListener<VimString> {
override fun processGlobalValueChange(oldValue: VimString?) {
AttributesCache.clear()
GuiCursorOptionHelper.clearEffectiveValues()
localEditors().forEach { it.updatePrimaryCaretVisualAttributes() }
}
}
Expand Down
20 changes: 19 additions & 1 deletion src/main/java/com/maddyhome/idea/vim/helper/CommandStateExtensions.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,16 @@
package com.maddyhome.idea.vim.helper

import com.intellij.openapi.editor.Editor
import com.maddyhome.idea.vim.api.Options
import com.maddyhome.idea.vim.api.hasValue
import com.maddyhome.idea.vim.api.injector
import com.maddyhome.idea.vim.command.CommandState
import com.maddyhome.idea.vim.command.VimStateMachine
import com.maddyhome.idea.vim.command.engine
import com.maddyhome.idea.vim.command.ij
import com.maddyhome.idea.vim.newapi.vim
import com.maddyhome.idea.vim.options.OptionConstants
import com.maddyhome.idea.vim.options.OptionScope

internal val VimStateMachine.Mode.hasVisualSelection
get() = when (this) {
Expand All @@ -40,8 +45,21 @@ public val Editor.mode: CommandState.Mode
* COMPATIBILITY-LAYER: New method
* Please see: https://jb.gg/zo8n0r
*/
@Deprecated("Please migrate to VimEditor.isEndAllowed which can correctly access virtualedit at the right scope",
replaceWith = ReplaceWith("VimEditor.isEndAllowed"))
public val CommandState.Mode.isEndAllowed: Boolean
get() = this.engine.isEndAllowed
get() {
fun possiblyUsesVirtualSpace(): Boolean {
// virtualedit is GLOBAL_OR_LOCAL_TO_WINDOW. We should NOT be using the global value!
return injector.optionGroup.hasValue(Options.virtualedit, OptionScope.GLOBAL, OptionConstants.virtualedit_onemore)
}

return when (this.engine) {
VimStateMachine.Mode.INSERT, VimStateMachine.Mode.VISUAL, VimStateMachine.Mode.SELECT -> true
VimStateMachine.Mode.COMMAND, VimStateMachine.Mode.CMD_LINE, VimStateMachine.Mode.REPLACE, VimStateMachine.Mode.OP_PENDING -> possiblyUsesVirtualSpace()
VimStateMachine.Mode.INSERT_NORMAL, VimStateMachine.Mode.INSERT_VISUAL, VimStateMachine.Mode.INSERT_SELECT -> possiblyUsesVirtualSpace()
}
}

internal var Editor.subMode
get() = this.vim.vimStateMachine.subMode
Expand Down
Loading