Merge branch 'main' into bugfix/telemetry

laileni-aws · web-flow · commit 394eba5cf209 · 2024-06-20T11:34:17.000-07:00
diff --git a/.changes/next-release/bugfix-a6c159d0-f2cb-469d-9c46-49a378eaaf60.json b/.changes/next-release/bugfix-a6c159d0-f2cb-469d-9c46-49a378eaaf60.json
@@ -0,0 +1,4 @@
+{
+  "type" : "bugfix",
+  "description" : "Fix IDE auto completion settings potentially overwritten by Q inline suggestion"
+}
diff --git a/plugins/amazonq/codewhisperer/jetbrains-community/src/software/aws/toolkits/jetbrains/services/codewhisperer/popup/CodeWhispererPopupManager.kt b/plugins/amazonq/codewhisperer/jetbrains-community/src/software/aws/toolkits/jetbrains/services/codewhisperer/popup/CodeWhispererPopupManager.kt
@@ -3,7 +3,6 @@
 
 package software.aws.toolkits.jetbrains.services.codewhisperer.popup
 
-import com.intellij.codeInsight.CodeInsightSettings
 import com.intellij.codeInsight.hint.ParameterInfoController
 import com.intellij.codeInsight.lookup.LookupManager
 import com.intellij.idea.AppMode
@@ -264,14 +263,19 @@ class CodeWhispererPopupManager {
 
     fun cancelPopup(popup: JBPopup) {
         popup.cancel()
+        Disposer.dispose(popup)
     }
 
     fun closePopup(popup: JBPopup) {
         popup.closeOk(null)
+        Disposer.dispose(popup)
     }
 
     fun closePopup() {
-        myPopup?.closeOk(null)
+        myPopup?.let {
+            it.closeOk(null)
+            Disposer.dispose(it)
+        }
     }
 
     fun showPopup(
@@ -342,11 +346,6 @@ class CodeWhispererPopupManager {
                 popup.size = popup.preferredContentSize
             }
         } else {
-            val originalAutoPopupCompletionLookup = CodeInsightSettings.getInstance().AUTO_POPUP_COMPLETION_LOOKUP
-            CodeInsightSettings.getInstance().AUTO_POPUP_COMPLETION_LOOKUP = false
-            Disposer.register(popup) {
-                CodeInsightSettings.getInstance().AUTO_POPUP_COMPLETION_LOOKUP = originalAutoPopupCompletionLookup
-            }
             if (!AppMode.isRemoteDevHost()) {
                 popup.show(relativePopupLocationToEditor)
             } else {
diff --git a/plugins/amazonq/codewhisperer/jetbrains-community/src/software/aws/toolkits/jetbrains/services/codewhisperer/service/CodeWhispererService.kt b/plugins/amazonq/codewhisperer/jetbrains-community/src/software/aws/toolkits/jetbrains/services/codewhisperer/service/CodeWhispererService.kt
@@ -3,9 +3,9 @@
 
 package software.aws.toolkits.jetbrains.services.codewhisperer.service
 
-import com.intellij.codeInsight.CodeInsightSettings
 import com.intellij.codeInsight.hint.HintManager
 import com.intellij.notification.NotificationAction
+import com.intellij.openapi.Disposable
 import com.intellij.openapi.application.ApplicationInfo
 import com.intellij.openapi.application.ApplicationManager
 import com.intellij.openapi.application.runInEdt
@@ -69,6 +69,7 @@ import software.aws.toolkits.jetbrains.services.codewhisperer.popup.CodeWhispere
 import software.aws.toolkits.jetbrains.services.codewhisperer.settings.CodeWhispererSettings
 import software.aws.toolkits.jetbrains.services.codewhisperer.telemetry.CodeWhispererTelemetryService
 import software.aws.toolkits.jetbrains.services.codewhisperer.util.CaretMovement
+import software.aws.toolkits.jetbrains.services.codewhisperer.util.CodeInsightsSettingsFacade
 import software.aws.toolkits.jetbrains.services.codewhisperer.util.CodeWhispererConstants
 import software.aws.toolkits.jetbrains.services.codewhisperer.util.CodeWhispererConstants.SUPPLEMENTAL_CONTEXT_TIMEOUT
 import software.aws.toolkits.jetbrains.services.codewhisperer.util.CodeWhispererUtil.getCompletionType
@@ -90,9 +91,14 @@ import software.aws.toolkits.telemetry.CodewhispererTriggerType
 import java.util.concurrent.TimeUnit
 
 @Service
-class CodeWhispererService {
+class CodeWhispererService : Disposable {
+    private val codeInsightSettingsFacade = CodeInsightsSettingsFacade()
     private var refreshFailure: Int = 0
 
+    init {
+        Disposer.register(this, codeInsightSettingsFacade)
+    }
+
     fun showRecommendationsInPopup(
         editor: Editor,
         triggerTypeInfo: TriggerTypeInfo,
@@ -691,16 +697,8 @@ class CodeWhispererService {
     }
 
     private fun addPopupChildDisposables(popup: JBPopup) {
-        val originalTabExitsBracketsAndQuotes = CodeInsightSettings.getInstance().TAB_EXITS_BRACKETS_AND_QUOTES
-        CodeInsightSettings.getInstance().TAB_EXITS_BRACKETS_AND_QUOTES = false
-        Disposer.register(popup) {
-            CodeInsightSettings.getInstance().TAB_EXITS_BRACKETS_AND_QUOTES = originalTabExitsBracketsAndQuotes
-        }
-        val originalAutoPopupCompletionLookup = CodeInsightSettings.getInstance().AUTO_POPUP_COMPLETION_LOOKUP
-        CodeInsightSettings.getInstance().AUTO_POPUP_COMPLETION_LOOKUP = false
-        Disposer.register(popup) {
-            CodeInsightSettings.getInstance().AUTO_POPUP_COMPLETION_LOOKUP = originalAutoPopupCompletionLookup
-        }
+        codeInsightSettingsFacade.disableCodeInsightUntil(popup)
+
         Disposer.register(popup) {
             CodeWhispererPopupManager.getInstance().reset()
         }
@@ -763,6 +761,8 @@ class CodeWhispererService {
         HintManager.getInstance().showErrorHint(editor, message, HintManager.UNDER)
     }
 
+    override fun dispose() {}
+
     companion object {
         private val LOG = getLogger<CodeWhispererService>()
         private const val MAX_REFRESH_ATTEMPT = 3
diff --git a/plugins/amazonq/codewhisperer/jetbrains-community/src/software/aws/toolkits/jetbrains/services/codewhisperer/util/CodeInsightsSettingsFacade.kt b/plugins/amazonq/codewhisperer/jetbrains-community/src/software/aws/toolkits/jetbrains/services/codewhisperer/util/CodeInsightsSettingsFacade.kt
@@ -0,0 +1,98 @@
+// Copyright 2024 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package software.aws.toolkits.jetbrains.services.codewhisperer.util
+
+import com.intellij.codeInsight.CodeInsightSettings
+import com.intellij.openapi.Disposable
+import com.intellij.openapi.util.Disposer
+import com.intellij.openapi.util.SimpleModificationTracker
+import org.jetbrains.annotations.VisibleForTesting
+import software.aws.toolkits.core.utils.error
+import software.aws.toolkits.core.utils.getLogger
+import kotlin.reflect.KMutableProperty
+
+class CodeInsightsSettingsFacade : SimpleModificationTracker(), Disposable {
+    private inner class ChangeAndRevert<T : Any>(
+        val p: KMutableProperty<T>,
+        val value: T,
+        parentDisposable: Disposable
+    ) {
+        val origin: T = p.getter.call()
+        var isComplete: Boolean = false
+            private set
+
+        init {
+            Disposer.register(parentDisposable) {
+                revert()
+            }
+        }
+
+        fun commit(): ChangeAndRevert<T> {
+            p.setter.call(value)
+            return this
+        }
+
+        fun revert() {
+            if (isComplete) {
+                return
+            }
+
+            p.setter.call(origin)
+            isComplete = true
+        }
+    }
+
+    private val settings by lazy {
+        CodeInsightSettings.getInstance()
+    }
+
+    private var pendingReverts = listOf<ChangeAndRevert<*>>()
+    val pendingRevertCounts: Int
+        get() = pendingReverts.size
+
+    @VisibleForTesting
+    internal fun revertAll() {
+        if (pendingReverts.count { !it.isComplete } == 0) {
+            return
+        }
+
+        pendingReverts.forEach {
+            it.revert()
+        }
+
+        pendingReverts = pendingReverts.filter {
+            !it.isComplete
+        }.toMutableList()
+    }
+
+    fun disableCodeInsightUntil(parentDisposable: Disposable) {
+        revertAll()
+        val toReverts = mutableListOf<ChangeAndRevert<*>>()
+
+        ChangeAndRevert(settings::TAB_EXITS_BRACKETS_AND_QUOTES, false, parentDisposable).commit().also {
+            toReverts.add(it)
+        }
+
+        ChangeAndRevert(settings::AUTO_POPUP_COMPLETION_LOOKUP, false, parentDisposable).commit().also {
+            toReverts.add(it)
+        }
+
+        if (pendingReverts.count { !it.isComplete } != 0) {
+            LOG.error { "trying to overwrite users' settings without reverting all previous overwrites" }
+        }
+        pendingReverts = toReverts
+
+        Disposer.register(parentDisposable) {
+            revertAll()
+        }
+    }
+
+    override fun dispose() {
+        revertAll()
+    }
+
+    companion object {
+        val LOG = getLogger<CodeInsightsSettingsFacade>()
+    }
+}
diff --git a/plugins/amazonq/codewhisperer/jetbrains-community/tst/software/aws/toolkits/jetbrains/services/codewhisperer/util/CodeInsightsSettingsFacadeTest.kt b/plugins/amazonq/codewhisperer/jetbrains-community/tst/software/aws/toolkits/jetbrains/services/codewhisperer/util/CodeInsightsSettingsFacadeTest.kt
@@ -0,0 +1,137 @@
+// Copyright 2024 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package software.aws.toolkits.jetbrains.services.codewhisperer.util
+
+import com.intellij.codeInsight.CodeInsightSettings
+import com.intellij.openapi.Disposable
+import com.intellij.openapi.application.ApplicationManager
+import com.intellij.openapi.util.Disposer
+import com.intellij.testFramework.ProjectExtension
+import com.intellij.testFramework.junit5.TestDisposable
+import com.intellij.testFramework.replaceService
+import org.assertj.core.api.Assertions.assertThat
+import org.junit.jupiter.api.BeforeEach
+import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.extension.RegisterExtension
+import org.mockito.kotlin.spy
+import org.mockito.kotlin.times
+import org.mockito.kotlin.verify
+
+class CodeInsightsSettingsFacadeTest {
+    private lateinit var settings: CodeInsightSettings
+    private lateinit var sut: CodeInsightsSettingsFacade
+
+    @TestDisposable
+    private lateinit var disposable: Disposable
+
+    companion object {
+        @JvmField
+        @RegisterExtension
+        val projectExtension = ProjectExtension()
+    }
+
+    @BeforeEach
+    fun setUp() {
+        sut = spy(CodeInsightsSettingsFacade())
+        settings = spy { CodeInsightSettings() }
+
+        ApplicationManager.getApplication().replaceService(
+            CodeInsightSettings::class.java,
+            settings,
+            disposable
+        )
+    }
+
+    @Test
+    fun `disableCodeInsightUntil should revert when parent is disposed`() {
+        val myFakePopup = Disposable {}.also {
+            Disposer.register(disposable, it)
+        }
+
+        // assume users' enable the following two codeinsight functionalities
+        settings.TAB_EXITS_BRACKETS_AND_QUOTES = true
+        assertThat(settings.TAB_EXITS_BRACKETS_AND_QUOTES).isTrue
+        settings.AUTOCOMPLETE_ON_CODE_COMPLETION = true
+        assertThat(settings.AUTO_POPUP_COMPLETION_LOOKUP).isTrue
+
+        // codewhisperer disable them while popup is shown
+        sut.disableCodeInsightUntil(myFakePopup)
+
+        assertThat(settings.TAB_EXITS_BRACKETS_AND_QUOTES).isFalse
+        assertThat(settings.AUTO_POPUP_COMPLETION_LOOKUP).isFalse
+        assertThat(sut.pendingRevertCounts).isEqualTo(2)
+
+        // popup is closed and disposed
+        Disposer.dispose(myFakePopup)
+
+        // revert changes made by codewhisperer
+        verify(sut, times(2)).revertAll()
+        assertThat(settings.TAB_EXITS_BRACKETS_AND_QUOTES).isTrue
+        assertThat(settings.AUTO_POPUP_COMPLETION_LOOKUP).isTrue
+    }
+
+    @Test
+    fun `revertAll should revert back all changes made by codewhisperer`() {
+        settings.TAB_EXITS_BRACKETS_AND_QUOTES = true
+        assertThat(settings.TAB_EXITS_BRACKETS_AND_QUOTES).isTrue
+        settings.AUTOCOMPLETE_ON_CODE_COMPLETION = true
+        assertThat(settings.AUTO_POPUP_COMPLETION_LOOKUP).isTrue
+
+        sut.disableCodeInsightUntil(disposable)
+
+        assertThat(settings.TAB_EXITS_BRACKETS_AND_QUOTES).isFalse
+        assertThat(settings.AUTO_POPUP_COMPLETION_LOOKUP).isFalse
+
+        assertThat(sut.pendingRevertCounts).isEqualTo(2)
+
+        sut.revertAll()
+        assertThat(sut.pendingRevertCounts).isEqualTo(0)
+        assertThat(settings.TAB_EXITS_BRACKETS_AND_QUOTES).isTrue
+        assertThat(settings.AUTO_POPUP_COMPLETION_LOOKUP).isTrue
+    }
+
+    @Test
+    fun `disableCodeInsightUntil should always flush pending reverts before making next changes`() {
+        val myFakePopup = Disposable {}.also {
+            Disposer.register(disposable, it)
+        }
+        val myAnotherFakePopup = Disposable {}.also {
+            Disposer.register(disposable, it)
+        }
+
+        // assume users' enable the following two codeinsight functionalities
+        settings.TAB_EXITS_BRACKETS_AND_QUOTES = true
+        assertThat(settings.TAB_EXITS_BRACKETS_AND_QUOTES).isTrue
+        settings.AUTOCOMPLETE_ON_CODE_COMPLETION = true
+        assertThat(settings.AUTO_POPUP_COMPLETION_LOOKUP).isTrue
+
+        // codewhisperer disable them while popup_1 is shown
+        sut.disableCodeInsightUntil(myFakePopup)
+        assertThat(settings.TAB_EXITS_BRACKETS_AND_QUOTES).isFalse
+        assertThat(settings.AUTO_POPUP_COMPLETION_LOOKUP).isFalse
+        assertThat(sut.pendingRevertCounts).isEqualTo(2)
+        verify(sut, times(1)).revertAll()
+
+        // unexpected issue happens and popup_1 is not disposed correctly and popup_2 is created
+        sut.disableCodeInsightUntil(myAnotherFakePopup)
+        assertThat(settings.TAB_EXITS_BRACKETS_AND_QUOTES).isFalse
+        assertThat(settings.AUTO_POPUP_COMPLETION_LOOKUP).isFalse
+        // should still be 2 because previous ones should be reverted before preceding next changes
+        assertThat(sut.pendingRevertCounts).isEqualTo(2)
+        verify(sut, times(1 + 1)).revertAll()
+
+        Disposer.dispose(myAnotherFakePopup)
+
+        assertThat(sut.pendingRevertCounts).isEqualTo(0)
+        verify(sut, times(1 + 1 + 1)).revertAll()
+        assertThat(settings.TAB_EXITS_BRACKETS_AND_QUOTES).isTrue
+        assertThat(settings.AUTO_POPUP_COMPLETION_LOOKUP).isTrue
+    }
+
+    @Test
+    fun `dispose should call revertAll to revert all changes made by CodeWhisperer`() {
+        sut.dispose()
+        verify(sut).revertAll()
+    }
+}
diff --git a/plugins/core/jetbrains-community/src/software/aws/toolkits/jetbrains/core/credentials/DefaultToolkitConnectionManager.kt b/plugins/core/jetbrains-community/src/software/aws/toolkits/jetbrains/core/credentials/DefaultToolkitConnectionManager.kt
@@ -4,6 +4,7 @@
 package software.aws.toolkits.jetbrains.core.credentials
 
 import com.intellij.ide.ActivityTracker
+import com.intellij.openapi.application.ApplicationInfo
 import com.intellij.openapi.application.ApplicationManager
 import com.intellij.openapi.components.PersistentStateComponent
 import com.intellij.openapi.components.State
@@ -76,6 +77,7 @@ class DefaultToolkitConnectionManager : ToolkitConnectionManager, PersistentStat
 
             null
         } ?: defaultConnection?.let {
+            if (ApplicationInfo.getInstance().build.productCode == "GW") return null
             if (feature.supportsConnectionType(it)) {
                 return it
             }
diff --git a/plugins/core/jetbrains-community/src/software/aws/toolkits/jetbrains/core/credentials/ToolkitAuthManager.kt b/plugins/core/jetbrains-community/src/software/aws/toolkits/jetbrains/core/credentials/ToolkitAuthManager.kt
diff --git a/plugins/core/jetbrains-community/src/software/aws/toolkits/jetbrains/core/webview/LoginBrowser.kt b/plugins/core/jetbrains-community/src/software/aws/toolkits/jetbrains/core/webview/LoginBrowser.kt

-Original file line number
+Diff line change
@@ @@ -0,0 +1,4 @@ @@
 +{
 +  "type" : "bugfix",
 +  "description" : "Fix IDE auto completion settings potentially overwritten by Q inline suggestion"
 +}