IDEA-303581 Do not disable Settings Sync when plugin state change has failed

slightly refactor SettingsSyncPluginInstallerImpl to make it suitable for testing

GitOrigin-RevId: 7d77a3e578bdb4ed61ae98b7f94d22bbe92c4413

plugins/settings-sync/src/com/intellij/settingsSync/plugins/SettingsSyncPluginInstallerImpl.kt

@@ -7,6 +7,7 @@ import com.intellij.notification.NotificationType
 import com.intellij.openapi.application.ApplicationManager
 import com.intellij.openapi.application.ApplicationNamesInfo
 import com.intellij.openapi.application.ex.ApplicationEx
+import com.intellij.openapi.components.SettingsCategory
 import com.intellij.openapi.diagnostic.logger
 import com.intellij.openapi.extensions.PluginId
 import com.intellij.openapi.progress.ProgressIndicator
@@ -14,21 +15,22 @@ import com.intellij.openapi.progress.ProgressManager
 import com.intellij.openapi.updateSettings.impl.PluginDownloader
 import com.intellij.settingsSync.NOTIFICATION_GROUP
 import com.intellij.settingsSync.SettingsSyncBundle
+import com.intellij.settingsSync.SettingsSyncEvents
+import com.intellij.settingsSync.SettingsSyncSettings
 import com.intellij.util.Consumer
 import com.intellij.util.concurrency.annotations.RequiresBackgroundThread
 
-internal class SettingsSyncPluginInstallerImpl(private val notifyErrors: Boolean) : SettingsSyncPluginInstaller {
+internal open class SettingsSyncPluginInstallerImpl(private val notifyErrors: Boolean) : SettingsSyncPluginInstaller {
   companion object {
     val LOG = logger<SettingsSyncPluginInstallerImpl>()
   }
 
   @RequiresBackgroundThread
   override fun installPlugins(pluginsToInstall: List<PluginId>) {
-    if (pluginsToInstall.isEmpty() ||
-        ApplicationManager.getApplication().isUnitTestMode // Register TestPluginManager in Unit Test Mode
-    ) return
+    if (pluginsToInstall.isEmpty())
+      return
     ApplicationManager.getApplication().invokeAndWait {
-      val prepareRunnable = PrepareInstallationRunnable(pluginsToInstall, notifyErrors)
+      val prepareRunnable = PrepareInstallationRunnable(pluginsToInstall) { pluginId, indicator -> createDownloader(pluginId, indicator) }
       if (ProgressManager.getInstance().runProcessWithProgressSynchronously(
           prepareRunnable, SettingsSyncBundle.message("installing.plugins.indicator"), true, null)) {
         installCollected(prepareRunnable.getInstallers())
@@ -38,15 +40,51 @@ internal class SettingsSyncPluginInstallerImpl(private val notifyErrors: Boolean
 
   private fun installCollected(installers: List<PluginDownloader>) {
     val pluginsRequiredRestart = mutableListOf<String>()
-    installers.forEach {
-      if (!it.installDynamically(null)) {
-        pluginsRequiredRestart.add("'${it.pluginName}'")
+    var settingsChanged = false
+    val settings = SettingsSyncSettings.getInstance()
+    for (installer in installers) {
+      try {
+        if (!install(installer)) {
+          pluginsRequiredRestart.add("'${installer.pluginName}'")
+        }
+        LOG.info("Setting sync installed plugin ID: ${installer.id.idString}")
+      } catch (ex: Exception) {
+        // currently, we don't install plugins that have missing dependencies.
+        // TODO: toposort plugin with dependencies.
+        // TODO: Skip installation dependent plugins, if any dependency fails to install.
+        LOG.warn("An exception occurred while installing plugin ${installer.id.idString}. Will disable syncing this plugin")
+        settings.setSubcategoryEnabled(SettingsCategory.PLUGINS, installer.id.idString, false)
+        settingsChanged = true
       }
-      LOG.info("Setting sync installed plugin ID: ${it.id.idString}")
+    }
+    if (settingsChanged){
+      SettingsSyncEvents.getInstance().fireCategoriesChanged()
     }
     notifyRestartNeeded(pluginsRequiredRestart)
   }
 
+  open internal fun install(installer: PluginDownloader): Boolean = installer.installDynamically(null)
+
+  open internal fun createDownloader(pluginId: PluginId, indicator: ProgressIndicator): PluginDownloader? {
+    val descriptor = MarketplaceRequests.getInstance().getLastCompatiblePluginUpdate(pluginId, indicator = indicator)
+    if (descriptor != null) {
+      val downloader = PluginDownloader.createDownloader(descriptor)
+      if (downloader.prepareToInstall(indicator)) {
+        return downloader
+      }
+    }
+    else {
+      val message = SettingsSyncBundle.message("install.plugin.failed.no.compatible.notification.error.message", pluginId )
+      LOG.info(message)
+      if (notifyErrors) {
+        NotificationGroupManager.getInstance().getNotificationGroup(NOTIFICATION_GROUP)
+          .createNotification("", message, NotificationType.ERROR)
+          .notify(null)
+      }
+    }
+    return null
+  }
+
   private fun notifyRestartNeeded(pluginsRequiredRestart: List<String>) {
     if (pluginsRequiredRestart.isEmpty())
       return
@@ -70,7 +108,10 @@ internal class SettingsSyncPluginInstallerImpl(private val notifyErrors: Boolean
   }
 
 
-  private class PrepareInstallationRunnable(val pluginIds: List<PluginId>, val notifyErrors: Boolean) : Runnable {
+  internal class PrepareInstallationRunnable(
+    val pluginIds: List<PluginId>,
+    val dwnldPreparer: (pluginId: PluginId, indicator: ProgressIndicator) -> PluginDownloader?
+  ) : Runnable {
 
     private val collectedInstallers = ArrayList<PluginDownloader>()
 
@@ -85,21 +126,8 @@ internal class SettingsSyncPluginInstallerImpl(private val notifyErrors: Boolean
 
     @RequiresBackgroundThread
     private fun prepareToInstall(pluginId: PluginId, indicator: ProgressIndicator) {
-      val descriptor = MarketplaceRequests.getInstance().getLastCompatiblePluginUpdate(pluginId, indicator = indicator)
-      if (descriptor != null) {
-        val downloader = PluginDownloader.createDownloader(descriptor)
-        if (downloader.prepareToInstall(indicator)) {
-          collectedInstallers.add(downloader)
-        }
-      }
-      else {
-        val message = SettingsSyncBundle.message("install.plugin.failed.no.compatible.notification.error.message", pluginId )
-        LOG.info(message)
-        if (notifyErrors) {
-          NotificationGroupManager.getInstance().getNotificationGroup(NOTIFICATION_GROUP)
-            .createNotification("", message, NotificationType.ERROR)
-            .notify(null)
-        }
+      dwnldPreparer(pluginId, indicator) ?.also {
+          collectedInstallers.add(it)
       }
     }
 

plugins/settings-sync/tests/com/intellij/settingsSync/BasePluginManagerTest.kt

@@ -56,6 +56,7 @@ abstract class BasePluginManagerTest {
   @BeforeEach
   fun setUp() {
     SettingsSyncSettings.getInstance().syncEnabled = true
+    SettingsSyncSettings.getInstance().loadState(SettingsSyncSettings.SettingsSyncSettingsState())
     testPluginManager = TestPluginManager()
     ApplicationManager.getApplication().replaceService(PluginManagerProxy::class.java, testPluginManager, testRootDisposable)
     pluginManager = SettingsSyncPluginManager()

plugins/settings-sync/tests/com/intellij/settingsSync/SettingsSyncPluginManagerTest.kt

@@ -2,6 +2,7 @@ package com.intellij.settingsSync
 
 import com.intellij.idea.TestFor
 import com.intellij.openapi.components.SettingsCategory
+import org.junit.Assert
 import org.junit.jupiter.api.Assertions.*
 import org.junit.jupiter.api.Test
 
@@ -276,4 +277,32 @@ class SettingsSyncPluginManagerTest : BasePluginManagerTest() {
     assertIdeState(pushedState)
     assertPluginManagerState(pushedState)
   }
+
+  @Test
+  @TestFor(issues = ["IDEA-303581"])
+  fun `turn-off sync of plugin that fails to install`() {
+    testPluginManager.addPluginDescriptors(git4idea)
+    pluginManager.updateStateFromIdeOnStart(null)
+
+    testPluginManager.installer.installPluginExceptionThrower = {
+      if (it == quickJump.pluginId)
+        throw RuntimeException("Some arbitrary install exception")
+    }
+
+    val pushedState = state {
+      git4idea(enabled = false)
+      quickJump(enabled = true)
+      typengo(enabled = true)
+    }
+
+    pluginManager.pushChangesToIde(pushedState)
+
+    assertIdeState{
+      git4idea(enabled = false)
+      typengo(enabled = true)
+    }
+    assertPluginManagerState(pushedState)
+    Thread.sleep(100)
+    assertFalse(SettingsSyncSettings.getInstance().isSubcategoryEnabled(SettingsCategory.PLUGINS, quickJump.idString))
+  }
 }

plugins/settings-sync/tests/com/intellij/settingsSync/SettingsSyncPluginManagerTestUtil.kt

@@ -3,23 +3,30 @@ package com.intellij.settingsSync
 import com.intellij.ide.plugins.IdeaPluginDependency
 import com.intellij.ide.plugins.IdeaPluginDescriptor
 import com.intellij.openapi.extensions.PluginId
+import com.intellij.openapi.progress.ProgressIndicator
+import com.intellij.openapi.updateSettings.impl.PluginDownloader
 import com.intellij.settingsSync.plugins.SettingsSyncPluginInstaller
+import com.intellij.settingsSync.plugins.SettingsSyncPluginInstallerImpl
 import java.nio.file.Path
 import java.util.*
 
-class TestPluginInstaller(private val afterInstallPluginCallback: (PluginId) -> Unit) : SettingsSyncPluginInstaller {
+internal class TestPluginInstaller(private val afterInstallPluginCallback: (PluginId) -> Unit) : SettingsSyncPluginInstallerImpl(true) {
   val installedPluginIds = HashSet<PluginId>()
+  var installPluginExceptionThrower: ((PluginId) -> Unit)? = null
 
   // there's no marketplace to find plugin descriptors, so we'll just populate that in advance
 
-  internal fun justInstalledPlugins(): Collection<IdeaPluginDescriptor> =
-    TestPluginDescriptor.ALL.filter { installedPluginIds.contains(it.key) }.values
+  override fun install(installer: PluginDownloader): Boolean {
+    val pluginId = installer.id
+    installPluginExceptionThrower?.invoke(pluginId)
+    installedPluginIds += pluginId
+    afterInstallPluginCallback.invoke(pluginId)
+    return true
+  }
 
-  override fun installPlugins(pluginsToInstall: List<PluginId>) {
-    for (pluginId in pluginsToInstall) {
-      installedPluginIds += pluginId
-      afterInstallPluginCallback.invoke(pluginId)
-    }
+  override fun createDownloader(pluginId: PluginId, indicator: ProgressIndicator): PluginDownloader? {
+    val descriptor = TestPluginDescriptor.ALL[pluginId] ?: return null
+    return PluginDownloader.createDownloader(descriptor)
   }
 }
 

plugins/settings-sync/tests/com/intellij/settingsSync/TestPluginManager.kt

@@ -74,10 +74,7 @@ internal class TestPluginManager : AbstractPluginManagerProxy() {
   }
 
   override fun findPlugin(pluginId: PluginId): IdeaPluginDescriptor? {
-    return if (ownPluginDescriptors.containsKey(pluginId)) {
-      ownPluginDescriptors[pluginId]
-    }
-    else PluginManagerCore.findPlugin(pluginId)
+    return ownPluginDescriptors[pluginId]
   }
 
   override fun createInstaller(notifyErrors: Boolean): SettingsSyncPluginInstaller {