Make one subscription per project in BuildListener

Now all subscriptions merge into one and the collective
subscription lives as long as at least one subscription
is not disposed of.

Bug: N/A
Test: BuildListenerTest
Change-Id: I019ae2f0f54550e199393d949f2fbaf4ac4d2915

GitOrigin-RevId: 75bcbab274e62e4009b6ee41f2f153ab342d5af8

android/src/com/android/tools/idea/projectsystem/BuildListener.kt

@@ -19,29 +19,43 @@ import com.android.annotations.concurrency.GuardedBy
 import com.android.tools.idea.util.listenUntilNextSync
 import com.android.tools.idea.util.runWhenSmartAndSyncedOnEdt
 import com.intellij.openapi.Disposable
+import com.intellij.openapi.application.ApplicationManager
 import com.intellij.openapi.diagnostic.Logger
 import com.intellij.openapi.project.Project
 import com.intellij.openapi.util.Disposer
-import org.jetbrains.annotations.VisibleForTesting
 import java.util.WeakHashMap
 import java.util.concurrent.locks.ReentrantLock
 import kotlin.concurrent.withLock
 
+
+/**
+ * A per-project subscription to the [ProjectSystemBuildManager] (see [createBuildListener]) managing all the client subscriptions made via
+ * [setupBuildListener]. This way we only have at most one subscription to each [Project].
+ */
+private class ProjectSubscription {
+  val listenersMap: WeakHashMap<Disposable, BuildListener> = WeakHashMap()
+  val projectSystemListenerDisposable: Disposable = Disposer.newDisposable()
+}
+
 private val projectSubscriptionsLock = ReentrantLock()
 
 @GuardedBy("projectSubscriptionsLock")
-private val projectSubscriptions = WeakHashMap<Project, WeakHashMap<Disposable, BuildListener>>()
+private val projectSubscriptions = WeakHashMap<Project, ProjectSubscription>()
 
 /**
  * Executes [method] against all the non-disposed subscriptions for the [project]. Removes disposed subscriptions.
  */
 private fun forEachNonDisposedBuildListener(project: Project, method: (BuildListener) -> Unit) {
   projectSubscriptionsLock.withLock {
-    projectSubscriptions[project]?.let { subscriptions ->
+    projectSubscriptions[project]?.let { subscription ->
       // Clear disposed
-      subscriptions.keys.removeIf { Disposer.isDisposed(it) }
+      subscription.listenersMap.keys.removeIf { Disposer.isDisposed(it) }
+      if (subscription.listenersMap.isEmpty()) {
+        Disposer.dispose(subscription.projectSystemListenerDisposable)
+        projectSubscriptions.remove(project)
+      }
 
-      ArrayList<BuildListener>(subscriptions.values)
+      ArrayList<BuildListener>(subscription.listenersMap.values)
     } ?: emptyList()
   }.forEach(method)
 }
@@ -71,94 +85,85 @@ interface BuildListener {
   }
 }
 
+private fun Project.createBuildListener() = object : ProjectSystemBuildManager.BuildListener {
+  // We do not have to check isDisposed inside the callbacks since they won't get called if parentDisposable is disposed
+  override fun buildStarted(mode: ProjectSystemBuildManager.BuildMode) {
+    if (mode == ProjectSystemBuildManager.BuildMode.CLEAN) return
+
+    forEachNonDisposedBuildListener(this@createBuildListener,
+                                    BuildListener::buildStarted)
+  }
+
+  override fun buildCompleted(result: ProjectSystemBuildManager.BuildResult) {
+    // We do not call refresh if the build was not successful or if it was simply a clean build.
+    val isCleanBuild = result.mode == ProjectSystemBuildManager.BuildMode.CLEAN
+    if (result.status == ProjectSystemBuildManager.BuildStatus.SUCCESS && !isCleanBuild) {
+      forEachNonDisposedBuildListener(
+        this@createBuildListener,
+        BuildListener::buildSucceeded)
+    }
+    else {
+      if (isCleanBuild) {
+        forEachNonDisposedBuildListener(
+          this@createBuildListener,
+          BuildListener::buildCleaned)
+      }
+      else {
+        forEachNonDisposedBuildListener(
+          this@createBuildListener,
+          BuildListener::buildFailed)
+      }
+    }
+  }
+}
+
 /**
  * This sets up a listener that receives updates every time a build starts or finishes. On successful build, it calls
  * [BuildListener.buildSucceeded] method of the passed [BuildListener]. If the build fails, [BuildListener.buildFailed] will be called
- * instead.
- * This class ignores "clean" target builds and will not notify the listener when a clean happens since most listeners will not need to
- * listen for changes on "clean" target builds. If you need to listen for "clean" target builds, use [ProjectSystemBuildManager] directly.
+ * instead. If the successful build is "clean" build [BuildListener.buildCleaned] will be called.
  *
  * This is intended to be used by [com.intellij.openapi.fileEditor.FileEditor]'s. The editor should recreate/amend the model to reflect
  * build changes. This set up should be called in the constructor the last, so that all other members are initialized as it could call
  * [BuildListener.buildSucceeded] method straight away.
  */
-fun setupBuildListener(project: Project,
-                       buildable: BuildListener,
-                       parentDisposable: Disposable,
-                       allowMultipleSubscriptionsPerProject: Boolean = false) =
-  setupBuildListener(project, buildable, parentDisposable, allowMultipleSubscriptionsPerProject,
-                     ProjectSystemService.getInstance(project).projectSystem.getBuildManager())
-
-@VisibleForTesting
 fun setupBuildListener(
   project: Project,
   buildable: BuildListener,
   parentDisposable: Disposable,
-  allowMultipleSubscriptionsPerProject: Boolean,
   buildManager: ProjectSystemBuildManager = ProjectSystemService.getInstance(project).projectSystem.getBuildManager(),
 ) {
   if (Disposer.isDisposed(parentDisposable)) {
     Logger.getInstance("com.android.tools.idea.common.util.ChangeManager")
       .warn("calling setupBuildListener for a disposed component $parentDisposable")
     return
   }
-  // If we are not yet subscribed to this project, we should subscribe. If allowMultipleSubscriptionsPerProject is set to true, a build
-  // listener is added anyway, so it's up to the caller to avoid setting up redundant build listeners.
-  val projectNotSubscribed = projectSubscriptionsLock.withLock {
-    val notSubscribed = projectSubscriptions[project]?.isEmpty() != false
-    projectSubscriptions.computeIfAbsent(project) { WeakHashMap() }
-    notSubscribed
-  }
-  // Double check that listener is properly disposed. Add test for it.
-
-  if (projectNotSubscribed || allowMultipleSubscriptionsPerProject) {
-    buildManager.addBuildListener(
-      parentDisposable,
-      object : ProjectSystemBuildManager.BuildListener {
-        // We do not have to check isDisposed inside the callbacks since they won't get called if parentDisposable is disposed
-        override fun buildStarted(mode: ProjectSystemBuildManager.BuildMode) {
-          if (mode == ProjectSystemBuildManager.BuildMode.CLEAN) return
-
-          forEachNonDisposedBuildListener(project,
-                                          BuildListener::buildStarted)
-        }
 
-        override fun buildCompleted(result: ProjectSystemBuildManager.BuildResult) {
-          // We do not call refresh if the build was not successful or if it was simply a clean build.
-          val isCleanBuild = result.mode == ProjectSystemBuildManager.BuildMode.CLEAN
-          if (result.status == ProjectSystemBuildManager.BuildStatus.SUCCESS && !isCleanBuild) {
-            forEachNonDisposedBuildListener(
-              project,
-              BuildListener::buildSucceeded)
-          }
-          else {
-            if (isCleanBuild) {
-              forEachNonDisposedBuildListener(
-                project,
-                BuildListener::buildCleaned)
-            }
-            else {
-              forEachNonDisposedBuildListener(
-                project,
-                BuildListener::buildFailed)
-            }
-          }
-        }
-      })
-  }
   /**
-   * Sets up the listener once all conditions are met. This method can only be called once the project has synced and is smart.
+   * Sets up the listener once all conditions are met. This method can only be called once the project has synced and is smart on a
+   * dispatcher thread.
    */
   fun setupListenerWhenSmartAndSynced(buildManager: ProjectSystemBuildManager) {
+    ApplicationManager.getApplication().assertIsDispatchThread() // To verify parentDisposable is not disposed during the method execution
     if (Disposer.isDisposed(parentDisposable)) return
 
     projectSubscriptionsLock.withLock {
-      projectSubscriptions[project]!!.let {
-        it[parentDisposable] = buildable
-        Disposer.register(parentDisposable) {
-          projectSubscriptionsLock.withLock disposable@{
-            val subscriptions = projectSubscriptions[project] ?: return@disposable
-            subscriptions.remove(parentDisposable)
+      val subscription = projectSubscriptions.computeIfAbsent(project) {
+        val projectSubscription = ProjectSubscription()
+        // If we are not yet subscribed to this project, we should subscribe.
+        buildManager.addBuildListener(
+          projectSubscription.projectSystemListenerDisposable,
+          project.createBuildListener()
+        )
+        projectSubscription
+      }
+      subscription.listenersMap[parentDisposable] = buildable
+      Disposer.register(parentDisposable) {
+        projectSubscriptionsLock.withLock disposable@{
+          val disposingSubscription = projectSubscriptions[project] ?: return@disposable
+          disposingSubscription.listenersMap.remove(parentDisposable)
+          if (disposingSubscription.listenersMap.isEmpty()) {
+            Disposer.dispose(disposingSubscription.projectSystemListenerDisposable)
+            projectSubscriptions.remove(project)
           }
         }
       }

android/testSrc/com/android/tools/idea/projectsystem/BuildListenerTest.kt

@@ -63,8 +63,7 @@ class BuildListenerTest {
     // Make sure there are no pending call before setting up the listener
     processEvents()
     val listener = TestBuildListener()
-    setupBuildListener(project, listener, projectRule.fixture.testRootDisposable, buildManager = testBuildManager,
-                       allowMultipleSubscriptionsPerProject = false)
+    setupBuildListener(project, listener, projectRule.fixture.testRootDisposable, buildManager = testBuildManager)
     return Triple(testBuildManager,
                   buildMode,
                   listener)
@@ -139,16 +138,35 @@ class BuildListenerTest {
   }
 
   @Test
-  fun testAllowMultipleSubscriptionsPerProject() {
+  fun testCalledOnSubscriptionWhenPreviousBuildIsSuccessful() {
+    testBuildManager.buildStarted(ProjectSystemBuildManager.BuildMode.ASSEMBLE)
+    testBuildManager.buildCompleted(ProjectSystemBuildManager.BuildStatus.SUCCESS)
+    processEvents()
+
+    var listenerCalls = 0
+    val listener = object : BuildListener {
+      override fun buildStarted() {
+        listenerCalls++
+      }
+    }
+    val disposable = Disposer.newDisposable()
+    setupBuildListener(project, listener, disposable, buildManager = testBuildManager)
+
+    assertThat(listenerCalls).isEqualTo(1)
+
+    Disposer.dispose(disposable)
+  }
+
+  @Test
+  fun testOnlyOneSubscriptionPerProject() {
     var firstListenerCalls = 0
     val firstListener = object : BuildListener {
       override fun buildStarted() {
         firstListenerCalls++
       }
     }
     val firstDisposable = Disposer.newDisposable()
-    setupBuildListener(project, firstListener, firstDisposable, buildManager = testBuildManager,
-                       allowMultipleSubscriptionsPerProject = false)
+    setupBuildListener(project, firstListener, firstDisposable, buildManager = testBuildManager)
 
     var secondListenerCalls = 0
     val secondListener = object : BuildListener {
@@ -157,8 +175,7 @@ class BuildListenerTest {
       }
     }
     val secondDisposable = Disposer.newDisposable()
-    setupBuildListener(project, secondListener, secondDisposable, buildManager = testBuildManager,
-                       allowMultipleSubscriptionsPerProject = false)
+    setupBuildListener(project, secondListener, secondDisposable, buildManager = testBuildManager)
 
     testBuildManager.buildStarted(ProjectSystemBuildManager.BuildMode.ASSEMBLE)
     testBuildManager.buildCompleted(ProjectSystemBuildManager.BuildStatus.SUCCESS)
@@ -174,9 +191,7 @@ class BuildListenerTest {
     processEvents()
 
     assertThat(firstListenerCalls).isEqualTo(1)
-    // The second disposable was not disposed, but the second listener was not called since we didn't allow multiple listener subscriptions
-    // per project. Therefore, after the first subscription was disposed, further subscriptions were ignored.
-    assertThat(secondListenerCalls).isEqualTo(1)
+    assertThat(secondListenerCalls).isEqualTo(2)
 
     var thirdListenerCalls = 0
     val thirdListener = object : BuildListener {
@@ -185,20 +200,48 @@ class BuildListenerTest {
       }
     }
     val thirdDisposable = Disposer.newDisposable()
-    setupBuildListener(project, thirdListener, thirdDisposable, buildManager = testBuildManager,
-                       allowMultipleSubscriptionsPerProject = true)
+    setupBuildListener(project, thirdListener, thirdDisposable, buildManager = testBuildManager)
+
+    assertThat(thirdListenerCalls).isEqualTo(1)
 
     testBuildManager.buildStarted(ProjectSystemBuildManager.BuildMode.COMPILE)
     testBuildManager.buildCompleted(ProjectSystemBuildManager.BuildStatus.SUCCESS)
     processEvents()
 
-
-    assertThat(thirdListenerCalls).isEqualTo(2) //
-    // The second disposable was not disposed, and now we're allowing multiple listener subscriptions per project. Therefore, the second
-    // listener will also be called.
-    assertThat(secondListenerCalls).isEqualTo(2)
+    assertThat(secondListenerCalls).isEqualTo(3)
+    assertThat(thirdListenerCalls).isEqualTo(2)
 
     Disposer.dispose(secondDisposable)
     Disposer.dispose(thirdDisposable)
+
+    testBuildManager.buildStarted(ProjectSystemBuildManager.BuildMode.COMPILE)
+    testBuildManager.buildCompleted(ProjectSystemBuildManager.BuildStatus.SUCCESS)
+    processEvents()
+
+    assertThat(firstListenerCalls).isEqualTo(1)
+    assertThat(secondListenerCalls).isEqualTo(3)
+    assertThat(thirdListenerCalls).isEqualTo(2)
+
+    var fourthListenerCalls = 0
+    val fourthListener = object : BuildListener {
+      override fun buildStarted() {
+        fourthListenerCalls++
+      }
+    }
+    val fourthDisposable = Disposer.newDisposable()
+    setupBuildListener(project, fourthListener, fourthDisposable, buildManager = testBuildManager)
+
+    assertThat(fourthListenerCalls).isEqualTo(1)
+
+    testBuildManager.buildStarted(ProjectSystemBuildManager.BuildMode.COMPILE)
+    testBuildManager.buildCompleted(ProjectSystemBuildManager.BuildStatus.SUCCESS)
+    processEvents()
+
+    assertThat(firstListenerCalls).isEqualTo(1)
+    assertThat(secondListenerCalls).isEqualTo(3)
+    assertThat(thirdListenerCalls).isEqualTo(2)
+    assertThat(fourthListenerCalls).isEqualTo(2)
+
+    Disposer.dispose(fourthDisposable)
   }
 }

compose-designer/src/com/android/tools/idea/compose/preview/Preview.kt

@@ -670,7 +670,7 @@ class ComposePreviewRepresentation(psiFile: PsiFile,
         composeWorkBench.updateProgress(message("panel.building"))
         afterBuildStarted()
       }
-    }, this, allowMultipleSubscriptionsPerProject = true)
+    }, this)
 
     // When the preview is opened we must trigger an initial refresh. We wait for the project to be smart and synched to do it.
     project.runWhenSmartAndSyncedOnEdt(this, {

compose-designer/src/com/android/tools/idea/compose/preview/util/CodeOutOfDateTracker.kt

@@ -152,7 +152,7 @@ private class CodeOutOfDateTrackerImpl constructor(module: Module,
         invalidateSavedBuildStatus()
         buildFailed()
       }
-    }, parentDisposable = buildDisposable, allowMultipleSubscriptionsPerProject = true)
+    }, parentDisposable = buildDisposable)
 
     module.androidFacet?.let { facet ->
       val listener: ((MutableSet<ResourceNotificationManager.Reason>) -> Unit) = { reasons ->

compose-designer/testSrc/com/android/tools/idea/common/actions/LayoutlibSceneManagerRefreshIconActionTest.kt

@@ -51,7 +51,7 @@ internal class LayoutlibSceneManagerRefreshIconActionTest {
     val buildListener = mutableListOf<BuildListener>()
     val action = LayoutlibSceneManagerRefreshIconAction.forTesting(project,
                                                                    { },
-                                                                   { _, listener, _, _ -> buildListener.add(listener) },
+                                                                   { _, listener, _ -> buildListener.add(listener) },
                                                                    projectRule.fixture.testRootDisposable)
     action.updateAndRun {
       assertFalse(it.presentation.isVisible)
@@ -82,7 +82,7 @@ internal class LayoutlibSceneManagerRefreshIconActionTest {
     val renderListeners = mutableListOf<RenderListener>()
     val action = LayoutlibSceneManagerRefreshIconAction.forTesting(project,
                                                                    { renderListeners.add(it) },
-                                                                   { _, _, _, _ -> },
+                                                                   { _, _, _ -> },
                                                                    projectRule.fixture.testRootDisposable)
     action.updateAndRun {
       assertFalse(it.presentation.isVisible)

designer/src/com/android/tools/idea/common/actions/LayoutlibSceneManagerRefreshIconAction.kt

@@ -34,12 +34,12 @@ import com.intellij.ui.AnimatedIcon
 class LayoutlibSceneManagerRefreshIconAction private constructor(
   project: Project,
   addRenderListener: (RenderListener) -> Unit,
-  setupBuildListener: (Project, BuildListener, Disposable, Boolean) -> Unit,
+  setupBuildListener: (Project, BuildListener, Disposable) -> Unit,
   parentDisposable: Disposable): AnAction() {
   companion object {
     fun forTesting(project: Project,
                    addRenderListener: (RenderListener) -> Unit,
-                   setupBuildListener: (Project, BuildListener, Disposable, Boolean) -> Unit,
+                   setupBuildListener: (Project, BuildListener, Disposable) -> Unit,
                    parentDisposable: Disposable): LayoutlibSceneManagerRefreshIconAction =
       LayoutlibSceneManagerRefreshIconAction(project, addRenderListener, setupBuildListener, parentDisposable)
 
@@ -56,7 +56,7 @@ class LayoutlibSceneManagerRefreshIconAction private constructor(
      * builds.
      */
     fun forRefreshOnly(sceneManager: LayoutlibSceneManager) =
-      LayoutlibSceneManagerRefreshIconAction(sceneManager.model.project, sceneManager::addRenderListener, {_, _, _, _ ->}, sceneManager)
+      LayoutlibSceneManagerRefreshIconAction(sceneManager.model.project, sceneManager::addRenderListener, {_, _, _ ->}, sceneManager)
   }
 
   private var isRendering = false
@@ -93,7 +93,7 @@ class LayoutlibSceneManagerRefreshIconAction private constructor(
   init {
     templatePresentation.disabledIcon = AnimatedIcon.Default()
 
-    setupBuildListener(project, buildListener, parentDisposable, false)
+    setupBuildListener(project, buildListener, parentDisposable)
     addRenderListener(object : RenderListener {
       override fun onInflateStarted() {
         isRendering = true