Refactor execution of vim script

Now we set the flag `executingVimscript` during execution of any vimscript and we run initialization of delayed plugins after every call for execute. This is needed to properly initialize plugins after call for `source` command. Previously this command initialized extensions as they met in the script, what may cause incorrect behaviour. With this update, we unified an approach for executing vim script.
JetBrains · Nov 3, 2023 · 288c66d · 288c66d
1 parent 44c8a97
commit 288c66d
Show file tree

Hide file tree

Showing 6 changed files with 57 additions and 61 deletions.
diff --git a/src/main/java/com/maddyhome/idea/vim/VimPlugin.java b/src/main/java/com/maddyhome/idea/vim/VimPlugin.java
@@ -306,11 +306,6 @@ private void registerIdeavimrc(VimEditor editor) {
    * This is required to ensure that all options are correctly initialised and registered. Must be before any commands
    * are executed.</li>
    * <li>~/.ideavimrc execution<br>
-   * <ul>
-   * <li>4.1 executes commands from the .ideavimrc file and 4.2 initializes extensions.</li>
-   * <li>4.1 MUST BE BEFORE 4.2. This is a flow of vim/IdeaVim initialization, firstly .ideavimrc is executed and then
-   * the extensions are initialized.</li>
-   * </ul>
    * </li>
    * <li>Components initialization<br>
    * This should happen after ideavimrc execution because VimListenerManager accesses `number` option
@@ -339,13 +334,9 @@ private void turnOnPlugin() {
     VimInjectorKt.getInjector().getOptionGroup().initialiseOptions();
 
     // 4) ~/.ideavimrc execution
-    // 4.1) Execute ~/.ideavimrc
     // Evaluate in the context of the fallback window, to capture local option state, to copy to the first editor window
     registerIdeavimrc(VimInjectorKt.getInjector().getFallbackWindow());
 
-    // 4.2) Initialize extensions. Always after 4.1
-    VimExtensionRegistrar.enableDelayedExtensions();
-
     // Turing on should be performed after all commands registration
     getSearch().turnOn();
     VimListenerManager.INSTANCE.turnOn();

diff --git a/src/main/java/com/maddyhome/idea/vim/ui/ReloadVimRc.kt b/src/main/java/com/maddyhome/idea/vim/ui/ReloadVimRc.kt
@@ -23,7 +23,6 @@ import com.intellij.openapi.util.Disposer
 import com.intellij.openapi.util.io.FileUtil
 import com.maddyhome.idea.vim.api.VimrcFileState
 import com.maddyhome.idea.vim.api.injector
-import com.maddyhome.idea.vim.extension.VimExtensionRegistrar
 import com.maddyhome.idea.vim.helper.MessageHelper
 import com.maddyhome.idea.vim.icons.VimIcons
 import com.maddyhome.idea.vim.key.MappingOwner
@@ -154,9 +153,6 @@ internal class ReloadVimRc : DumbAwareAction() {
 
     // Reload the ideavimrc in the context of the current window, as though we had called `:source ~/.ideavimrc`
     executeIdeaVimRc(editor.vim)
-
-    // Ensure newly added extensions are initialized
-    VimExtensionRegistrar.enableDelayedExtensions()
   }
 }
 

diff --git a/src/main/java/com/maddyhome/idea/vim/vimscript/Executor.kt b/src/main/java/com/maddyhome/idea/vim/vimscript/Executor.kt
@@ -20,6 +20,7 @@ import com.maddyhome.idea.vim.api.VimScriptExecutorBase
 import com.maddyhome.idea.vim.api.injector
 import com.maddyhome.idea.vim.ex.ExException
 import com.maddyhome.idea.vim.ex.FinishException
+import com.maddyhome.idea.vim.extension.VimExtensionRegistrar
 import com.maddyhome.idea.vim.history.HistoryConstants
 import com.maddyhome.idea.vim.newapi.vim
 import com.maddyhome.idea.vim.register.RegisterConstants.LAST_COMMAND_REGISTER
@@ -40,52 +41,61 @@ internal class Executor : VimScriptExecutorBase() {
 
   @Throws(ExException::class)
   override fun execute(script: String, editor: VimEditor, context: ExecutionContext, skipHistory: Boolean, indicateErrors: Boolean, vimContext: VimLContext?): ExecutionResult {
-    var finalResult: ExecutionResult = ExecutionResult.Success
+    try {
+      injector.vimscriptExecutor.executingVimscript = true
+      var finalResult: ExecutionResult = ExecutionResult.Success
 
-    val myScript = VimscriptParser.parse(script)
-    myScript.units.forEach { it.vimContext = vimContext ?: myScript }
+      val myScript = VimscriptParser.parse(script)
+      myScript.units.forEach { it.vimContext = vimContext ?: myScript }
 
-    for (unit in myScript.units) {
-      try {
-        val result = unit.execute(editor, context)
-        if (result is ExecutionResult.Error) {
+      for (unit in myScript.units) {
+        try {
+          val result = unit.execute(editor, context)
+          if (result is ExecutionResult.Error) {
+            finalResult = ExecutionResult.Error
+            if (indicateErrors) {
+              VimPlugin.indicateError()
+            }
+          }
+        } catch (e: ExException) {
+          if (e is FinishException) {
+            break
+          }
           finalResult = ExecutionResult.Error
           if (indicateErrors) {
+            VimPlugin.showMessage(e.message)
             VimPlugin.indicateError()
+          } else {
+            logger.warn("Failed while executing $unit. " + e.message)
+          }
+        } catch (e: NotImplementedError) {
+          if (indicateErrors) {
+            VimPlugin.showMessage("Not implemented yet :(")
+            VimPlugin.indicateError()
+          }
+        } catch (e: Exception) {
+          logger.warn("Caught: ${e.message}")
+          logger.warn(e.stackTrace.toString())
+          if (injector.application.isUnitTest()) {
+            throw e
           }
-        }
-      } catch (e: ExException) {
-        if (e is FinishException) {
-          break
-        }
-        finalResult = ExecutionResult.Error
-        if (indicateErrors) {
-          VimPlugin.showMessage(e.message)
-          VimPlugin.indicateError()
-        } else {
-          logger.warn("Failed while executing $unit. " + e.message)
-        }
-      } catch (e: NotImplementedError) {
-        if (indicateErrors) {
-          VimPlugin.showMessage("Not implemented yet :(")
-          VimPlugin.indicateError()
-        }
-      } catch (e: Exception) {
-        logger.warn("Caught: ${e.message}")
-        logger.warn(e.stackTrace.toString())
-        if (injector.application.isUnitTest()) {
-          throw e
         }
       }
-    }
 
-    if (!skipHistory) {
-      VimPlugin.getHistory().addEntry(HistoryConstants.COMMAND, script)
-      if (myScript.units.size == 1 && myScript.units[0] is Command && myScript.units[0] !is RepeatCommand) {
-        VimPlugin.getRegister().storeTextSpecial(LAST_COMMAND_REGISTER, script)
+      if (!skipHistory) {
+        VimPlugin.getHistory().addEntry(HistoryConstants.COMMAND, script)
+        if (myScript.units.size == 1 && myScript.units[0] is Command && myScript.units[0] !is RepeatCommand) {
+          VimPlugin.getRegister().storeTextSpecial(LAST_COMMAND_REGISTER, script)
+        }
       }
+      return finalResult
+    } finally {
+      injector.vimscriptExecutor.executingVimscript = false
+
+      // Initialize any extensions that were enabled during execution of this vimscript
+      // See the doc of this function for details
+      VimExtensionRegistrar.enableDelayedExtensions()
     }
-    return finalResult
   }
 
   override fun executeFile(file: File, editor: VimEditor, fileIsIdeaVimRcConfig: Boolean, indicateErrors: Boolean) {

diff --git a/src/test/java/org/jetbrains/plugins/ideavim/extension/CommonExtensionTests.kt b/src/test/java/org/jetbrains/plugins/ideavim/extension/CommonExtensionTests.kt
@@ -25,7 +25,6 @@ import com.maddyhome.idea.vim.extension.VimExtension
 import com.maddyhome.idea.vim.extension.VimExtensionFacade.putExtensionHandlerMapping
 import com.maddyhome.idea.vim.extension.VimExtensionFacade.putKeyMapping
 import com.maddyhome.idea.vim.extension.VimExtensionFacade.putKeyMappingIfMissing
-import com.maddyhome.idea.vim.extension.VimExtensionRegistrar
 import com.maddyhome.idea.vim.handler.Motion
 import com.maddyhome.idea.vim.helper.isEndAllowed
 import com.maddyhome.idea.vim.newapi.ij
@@ -268,6 +267,7 @@ class PlugMissingKeysTest : VimTestCase() {
       "Plug 'MyTest'",
     )
 
+    // Mapping to Z was override by the mapping to myKey
     val keyMappings = VimPlugin.getKey().getMapTo(MappingMode.NORMAL, injector.parser.parseKeys("<Plug>TestMissing"))
     kotlin.test.assertEquals(1, keyMappings.size)
     kotlin.test.assertEquals(injector.parser.parseKeys("myKey"), keyMappings.first().first)
@@ -284,6 +284,7 @@ class PlugMissingKeysTest : VimTestCase() {
       "map myKey <Plug>TestMissing",
     )
 
+    // Mapping to Z was override by the mapping to myKey
     val keyMappings = VimPlugin.getKey().getMapTo(MappingMode.NORMAL, injector.parser.parseKeys("<Plug>TestMissing"))
     kotlin.test.assertEquals(1, keyMappings.size)
     kotlin.test.assertEquals(injector.parser.parseKeys("myKey"), keyMappings.first().first)
@@ -323,7 +324,6 @@ class PlugMissingKeysTest : VimTestCase() {
     executeVimscript(text.joinToString("\n"), false)
     injector.vimscriptExecutor.executingIdeaVimRcConfiguration = false
     injector.vimscriptExecutor.executingVimscript = false
-    VimExtensionRegistrar.enableDelayedExtensions()
   }
 }
 

diff --git a/vim-engine/src/main/kotlin/com/maddyhome/idea/vim/api/VimscriptExecutor.kt b/vim-engine/src/main/kotlin/com/maddyhome/idea/vim/api/VimscriptExecutor.kt
@@ -14,6 +14,10 @@ import java.io.File
 
 public interface VimscriptExecutor {
 
+  /**
+   * True if Vimscript is under execution. This might be reading of .ideavimrc file, some :source command,
+   *   command from the ex-command line, or any other case.
+   */
   public var executingVimscript: Boolean
 
   /**

diff --git a/vim-engine/src/main/kotlin/com/maddyhome/idea/vim/vimscript/services/VimRcService.kt b/vim-engine/src/main/kotlin/com/maddyhome/idea/vim/vimscript/services/VimRcService.kt
@@ -131,17 +131,12 @@ public object VimRcService {
 
   @JvmStatic
   public fun executeIdeaVimRc(editor: VimEditor) {
-    try {
-      injector.vimscriptExecutor.executingVimscript = true
-      val ideaVimRc = findIdeaVimRc()
-      if (ideaVimRc != null) {
-        logger.info("Execute ideavimrc file: " + ideaVimRc.absolutePath)
-        injector.vimscriptExecutor.executeFile(ideaVimRc, editor, fileIsIdeaVimRcConfig =  true)
-      } else {
-        logger.info("ideavimrc file isn't found")
-      }
-    } finally {
-      injector.vimscriptExecutor.executingVimscript = false
+    val ideaVimRc = findIdeaVimRc()
+    if (ideaVimRc != null) {
+      logger.info("Execute ideavimrc file: " + ideaVimRc.absolutePath)
+      injector.vimscriptExecutor.executeFile(ideaVimRc, editor, fileIsIdeaVimRcConfig = true)
+    } else {
+      logger.info("ideavimrc file isn't found")
     }
   }