From 8da15b8c08de0da82fe56ec4964183f424f370f1 Mon Sep 17 00:00:00 2001 From: Matt Ellis Date: Fri, 15 Nov 2024 17:47:27 +0000 Subject: [PATCH] Manually track last selected editor This is important for initialising options. We can't rely on FileEditorManager.selectedTextEditor, as 2024.2 changed behaviour to reset to null during creation of a new editor. This fixes tests (and option initialisation) for 242. --- .../idea/vim/listener/VimListenerManager.kt | 50 +++++++++++++------ .../option/overrides/WrapOptionMapperTest.kt | 4 -- .../jetbrains/plugins/ideavim/VimTestCase.kt | 2 + 3 files changed, 36 insertions(+), 20 deletions(-) diff --git a/src/main/java/com/maddyhome/idea/vim/listener/VimListenerManager.kt b/src/main/java/com/maddyhome/idea/vim/listener/VimListenerManager.kt index 338b59a69e..e00507bedc 100644 --- a/src/main/java/com/maddyhome/idea/vim/listener/VimListenerManager.kt +++ b/src/main/java/com/maddyhome/idea/vim/listener/VimListenerManager.kt @@ -34,6 +34,7 @@ import com.intellij.openapi.editor.ex.DocumentEx import com.intellij.openapi.editor.ex.EditorEx import com.intellij.openapi.editor.ex.FocusChangeListener import com.intellij.openapi.editor.impl.EditorComponentImpl +import com.intellij.openapi.fileEditor.FileEditor import com.intellij.openapi.fileEditor.FileEditorManager import com.intellij.openapi.fileEditor.FileEditorManagerEvent import com.intellij.openapi.fileEditor.FileEditorManagerListener @@ -44,6 +45,7 @@ import com.intellij.openapi.fileEditor.ex.FileEditorWithProvider import com.intellij.openapi.fileEditor.impl.EditorComposite import com.intellij.openapi.fileEditor.impl.EditorWindow import com.intellij.openapi.observable.util.addKeyListener +import com.intellij.openapi.project.Project import com.intellij.openapi.project.ProjectManager import com.intellij.openapi.util.Disposer import com.intellij.openapi.util.Key @@ -103,8 +105,11 @@ import com.maddyhome.idea.vim.ui.widgets.macro.MacroWidgetListener import com.maddyhome.idea.vim.ui.widgets.macro.macroWidgetOptionListener import com.maddyhome.idea.vim.ui.widgets.mode.listeners.ModeWidgetListener import com.maddyhome.idea.vim.ui.widgets.mode.modeWidgetOptionListener +import org.jetbrains.annotations.TestOnly import java.awt.event.MouseAdapter import java.awt.event.MouseEvent +import java.lang.ref.WeakReference +import java.util.WeakHashMap import javax.swing.SwingUtilities /** @@ -126,23 +131,15 @@ import javax.swing.SwingUtilities * Make sure the selected editor isn't the new editor, which can happen if there are no other editors open. */ private fun getOpeningEditor(newEditor: Editor) = newEditor.project?.let { project -> - // Some TextEditor implementations create a dummy Editor instance on demand, e.g., while downloading a file to edit - // (see BaseRemoteFileEditor). This can cause recursion if the newly opened/created TextEditor is also the currently - // selected TextEditor, because we will be notified of the new dummy Editor before it has finished initialisation, and - // try to get its opening editor, causing a new dummy Editor to be created and notifications sent, and so on. - // This was reported for 232 and 233 (see VIM-3066), but I can't recreate in 241. The callstack looks different, now - // using coroutines, so it's possible the deadlock has been broken. However, it's sensible to leave the recursion - // guard in. - if (openingEditorRecursionGuard) return null - openingEditorRecursionGuard = true - try { - FileEditorManager.getInstance(project).selectedTextEditor?.takeUnless { it == newEditor } - } - finally { - openingEditorRecursionGuard = false - } + // We can't rely on FileEditorManager.selectedTextEditor because we're trying to retrieve the selected text editor + // while creating a text editor that is about to become the selected text editor. + // This worked fine for 2024.2, but internal changes for 2024.3 broke things. It appears that the currently selected + // text editor is reset to null while the soon-to-be-selected text editor is being created. We therefore track the + // last selected editor manually. + // Note that if we ever switch back to FileEditorManager.selectedTextEditor, be careful of recursion, because the + // actual editor might be created on-demand, which would notify our initialisation method, which would call us... + VimListenerManager.VimLastSelectedEditorTracker.getLastSelectedEditor(project)?.takeUnless { it == newEditor } } -private var openingEditorRecursionGuard = false internal object VimListenerManager { @@ -373,6 +370,26 @@ internal object VimListenerManager { } } + internal object VimLastSelectedEditorTracker { + // This stores a weak reference to an editor against a weak reference to a project, which means there is nothing + // keeping the project or editor from being garbage collected at any time. Stale keys are automatically expunged + // whenever the map is used. + private val selectedEditors = WeakHashMap>() + + fun getLastSelectedEditor(project: Project): Editor? = selectedEditors[project]?.get() + + internal fun setLastSelectedEditor(fileEditor: FileEditor?) { + (fileEditor as? TextEditor)?.editor?.let { editor -> + editor.project?.let { project -> selectedEditors[project] = WeakReference(editor) } + } + } + + @TestOnly + internal fun resetLastSelectedEditor(project: Project) { + selectedEditors.remove(project) + } + } + /** * Called when the selected file editor changes. In other words, when the user selects a new tab. Used to remember the * last selected file, update search highlights in the new tab, etc. This will be called with non-local Code With Me @@ -387,6 +404,7 @@ internal object VimListenerManager { FileGroup.fileEditorManagerSelectionChangedCallback(event) VimPlugin.getSearch().fileEditorManagerSelectionChangedCallback(event) IjVimRedrawService.fileEditorManagerSelectionChangedCallback(event) + VimLastSelectedEditorTracker.setLastSelectedEditor(event.newEditor) } } diff --git a/src/test/java/org/jetbrains/plugins/ideavim/option/overrides/WrapOptionMapperTest.kt b/src/test/java/org/jetbrains/plugins/ideavim/option/overrides/WrapOptionMapperTest.kt index bf620757c8..7662aed268 100644 --- a/src/test/java/org/jetbrains/plugins/ideavim/option/overrides/WrapOptionMapperTest.kt +++ b/src/test/java/org/jetbrains/plugins/ideavim/option/overrides/WrapOptionMapperTest.kt @@ -21,7 +21,6 @@ import org.jetbrains.plugins.ideavim.SkipNeovimReason import org.jetbrains.plugins.ideavim.TestWithoutNeovim import org.jetbrains.plugins.ideavim.VimTestCase import org.junit.jupiter.api.BeforeEach -import org.junit.jupiter.api.Disabled import org.junit.jupiter.api.Test import org.junit.jupiter.api.TestInfo import kotlin.test.assertFalse @@ -248,7 +247,6 @@ class WrapOptionMapperTest : VimTestCase() { } @Test - @Disabled("Doesn't work in 242") fun `test open new window after setting option copies value as explicitly set`() { enterCommand("set nowrap") assertCommandOutput("set wrap?", "nowrap") @@ -278,7 +276,6 @@ class WrapOptionMapperTest : VimTestCase() { } @Test - @Disabled("Doesn't work in 242") fun `test setlocal 'wrap' then open new window uses value from setglobal`() { enterCommand("setlocal nowrap") assertCommandOutput("setglobal wrap?", " wrap") @@ -294,7 +291,6 @@ class WrapOptionMapperTest : VimTestCase() { } @Test - @Disabled("Doesn't work in 242") fun `test setting global IDE value will update effective Vim value in new window initialised from value set during startup`() { try { injector.optionGroup.startInitVimRc() diff --git a/src/testFixtures/kotlin/org/jetbrains/plugins/ideavim/VimTestCase.kt b/src/testFixtures/kotlin/org/jetbrains/plugins/ideavim/VimTestCase.kt index 2dadf89a7d..9f2f250fd6 100644 --- a/src/testFixtures/kotlin/org/jetbrains/plugins/ideavim/VimTestCase.kt +++ b/src/testFixtures/kotlin/org/jetbrains/plugins/ideavim/VimTestCase.kt @@ -73,6 +73,7 @@ import com.maddyhome.idea.vim.helper.getGuiCursorMode import com.maddyhome.idea.vim.key.MappingOwner import com.maddyhome.idea.vim.key.ToKeysMappingInfo import com.maddyhome.idea.vim.listener.SelectionVimListenerSuppressor +import com.maddyhome.idea.vim.listener.VimListenerManager import com.maddyhome.idea.vim.newapi.IjVimEditor import com.maddyhome.idea.vim.newapi.globalIjOptions import com.maddyhome.idea.vim.newapi.ijOptions @@ -242,6 +243,7 @@ abstract class VimNoWriteActionTestCase { bookmarksManager?.bookmarks?.forEach { bookmark -> bookmarksManager.remove(bookmark) } + VimListenerManager.VimLastSelectedEditorTracker.resetLastSelectedEditor(fixture.project) SelectionVimListenerSuppressor.lock().use { fixture.tearDown() } ExEntryPanel.getInstance().deactivate(false) VimPlugin.getVariableService().clear()