IDEA-169025 Preserve run configurations order

java/java-tests/testSrc/com/intellij/execution/impl/RunConfigurableTest.kt

@@ -18,19 +18,24 @@ package com.intellij.execution.impl
 import com.intellij.execution.application.ApplicationConfigurationType
 import com.intellij.execution.impl.RunConfigurable.NodeKind.*
 import com.intellij.execution.junit.JUnitConfigurationType
+import com.intellij.openapi.util.Disposer
 import com.intellij.openapi.util.Trinity
-import com.intellij.testFramework.LightIdeaTestCase
-import com.intellij.testFramework.LightPlatformTestCase
+import com.intellij.testFramework.EdtRule
+import com.intellij.testFramework.ProjectRule
+import com.intellij.testFramework.RunsInEdt
 import com.intellij.testFramework.assertions.Assertions.assertThat
 import com.intellij.ui.RowsDnDSupport
 import com.intellij.ui.RowsDnDSupport.RefinedDropSupport.Position.*
 import com.intellij.ui.treeStructure.Tree
 import com.intellij.util.loadElement
 import org.jdom.Element
+import org.junit.After
+import org.junit.ClassRule
+import org.junit.Rule
+import org.junit.Test
 import java.util.*
 import javax.swing.tree.DefaultMutableTreeNode
 import javax.swing.tree.TreePath
-import kotlin.properties.Delegates
 
 private val ORDER = arrayOf(CONFIGURATION_TYPE, //Application
   FOLDER, //1
@@ -42,41 +47,56 @@ private val ORDER = arrayOf(CONFIGURATION_TYPE, //Application
   CONFIGURATION, CONFIGURATION, TEMPORARY_CONFIGURATION, UNKNOWN//Defaults
 )
 
-private fun createRunManager(element: Element): RunManagerImpl {
-  val runManager = RunManagerImpl(LightPlatformTestCase.getProject())
-  runManager.initializeConfigurationTypes(arrayOf(ApplicationConfigurationType.getInstance(), JUnitConfigurationType.getInstance()))
-  runManager.loadState(element)
-  return runManager
-}
+@RunsInEdt
+class RunConfigurableTest {
+  companion object {
+    @JvmField
+    @ClassRule
+    val projectRule = ProjectRule()
 
-class RunConfigurableTest : LightIdeaTestCase() {
-  private var configurable: MockRunConfigurable? = null
-  private var tree: Tree by Delegates.notNull()
-  private var root: DefaultMutableTreeNode? = null
-  private var model: RunConfigurable.MyTreeModel by Delegates.notNull()
+    private fun createRunManager(element: Element): RunManagerImpl {
+      val runManager = RunManagerImpl(projectRule.project)
+      runManager.initializeConfigurationTypes(arrayOf(ApplicationConfigurationType.getInstance(), JUnitConfigurationType.getInstance()))
+      runManager.loadState(element)
+      return runManager
+    }
 
-  override fun setUp() {
-    super.setUp()
-
-    configurable = MockRunConfigurable(createRunManager(loadElement(RunConfigurableTest::class.java.getResourceAsStream("folders.xml"))))
-    tree = configurable!!.myTree
-    root = configurable!!.myRoot
-    model = configurable!!.myTreeModel
-  }
-
-  override fun tearDown() {
-    try {
-      if (configurable != null) {
-        configurable!!.disposeUIResources()
+    private class MockRunConfigurable(private val testManager: RunManagerImpl) : RunConfigurable(projectRule.project) {
+      init {
+        createComponent()
       }
-      configurable = null
-      root = null
-    }
-    finally {
-      super.tearDown()
+
+      internal override fun getRunManager() = testManager
     }
   }
 
+  @JvmField
+  @Rule
+  val edtRule = EdtRule()
+
+  private val disposable = Disposer.newDisposable()
+
+  private val configurable: RunConfigurable by lazy {
+    val result = MockRunConfigurable(createRunManager(loadElement(RunConfigurableTest::class.java.getResourceAsStream("folders.xml"))))
+    Disposer.register(disposable, result)
+    result
+  }
+
+  private val root: DefaultMutableTreeNode
+    get() = configurable.myRoot
+
+  private val tree: Tree
+    get() = configurable.myTree
+
+  private val model: RunConfigurable.MyTreeModel
+    get() = configurable.myTreeModel
+
+  @After
+  fun tearDown() {
+    Disposer.dispose(disposable)
+  }
+
+  @Test
   fun testDND() {
     doExpand()
     val never = intArrayOf(-1, 0, 14, 22, 23, 999)
@@ -130,7 +150,7 @@ class RunConfigurableTest : LightIdeaTestCase() {
       tree.expandPath(TreePath(node.path))
     }
 
-    assertThat(ORDER.mapIndexed { index, nodeKind ->  RunConfigurable.getKind(tree.getPathForRow(index).lastPathComponent as DefaultMutableTreeNode) }).containsExactly(*ORDER)
+    assertThat(ORDER.mapIndexed { index, nodeKind -> RunConfigurable.getKind(tree.getPathForRow(index).lastPathComponent as DefaultMutableTreeNode) }).containsExactly(*ORDER)
   }
 
   private fun assertCan(oldIndex: Int, newIndex: Int, position: RowsDnDSupport.RefinedDropSupport.Position) {
@@ -155,6 +175,7 @@ class RunConfigurableTest : LightIdeaTestCase() {
     }
   }
 
+  @Test
   fun testMoveUpDown() {
     doExpand()
     checkPositionToMove(0, 1, null)
@@ -181,14 +202,46 @@ class RunConfigurableTest : LightIdeaTestCase() {
 
   private fun checkPositionToMove(selectedRow: Int, direction: Int, expected: Trinity<Int, Int, RowsDnDSupport.RefinedDropSupport.Position>?) {
     tree.setSelectionRow(selectedRow)
-    assertThat(configurable!!.getAvailableDropPosition(direction)).isEqualTo(expected)
-  }
-}
-
-private class MockRunConfigurable(private val testManager: RunManagerImpl) : RunConfigurable(LightPlatformTestCase.getProject()) {
-  init {
-    createComponent()
+    assertThat(configurable.getAvailableDropPosition(direction)).isEqualTo(expected)
   }
 
-  internal override fun getRunManager() = testManager
+  @Test
+  fun testSort() {
+    doExpand()
+    assertThat(configurable.isModified).isFalse()
+    model.drop(2, 0, ABOVE)
+    assertThat(configurable.isModified).isTrue()
+    configurable.apply()
+    assertThat(configurable.runManager.allSettings.map { it.name }).isEqualTo(listOf("Renamer",
+      "UI",
+      "AuTest",
+      "Simples",
+      "OutAndErr",
+      "C148C_TersePrincess",
+      "Periods",
+      "C148E_Porcelain",
+      "ErrAndOut",
+      "All in titled",
+      "All in titled2",
+      "All in titled3",
+      "All in titled4",
+      "All in titled5"))
+    assertThat(configurable.isModified).isFalse()
+    model.drop(4, 8, BELOW)
+    configurable.apply()
+    assertThat(configurable.runManager.allSettings.map { it.name }).isEqualTo(listOf("Renamer",
+      "AuTest",
+      "Simples",
+      "UI",
+      "OutAndErr",
+      "C148C_TersePrincess",
+      "Periods",
+      "C148E_Porcelain",
+      "ErrAndOut",
+      "All in titled",
+      "All in titled2",
+      "All in titled3",
+      "All in titled4",
+      "All in titled5"))
+  }
 }

platform/lang-impl/src/com/intellij/execution/impl/RunConfigurable.java

@@ -20,6 +20,7 @@ import com.intellij.execution.configuration.ConfigurationFactoryEx;
 import com.intellij.execution.configurations.*;
 import com.intellij.icons.AllIcons;
 import com.intellij.ide.DataManager;
+import com.intellij.openapi.Disposable;
 import com.intellij.openapi.actionSystem.*;
 import com.intellij.openapi.application.ApplicationManager;
 import com.intellij.openapi.diagnostic.Logger;
@@ -29,6 +30,7 @@ import com.intellij.openapi.project.Project;
 import com.intellij.openapi.ui.Messages;
 import com.intellij.openapi.ui.popup.ListPopup;
 import com.intellij.openapi.util.Comparing;
+import com.intellij.openapi.util.Disposer;
 import com.intellij.openapi.util.Pair;
 import com.intellij.openapi.util.Trinity;
 import com.intellij.openapi.util.text.StringUtil;
@@ -43,6 +45,7 @@ import com.intellij.util.containers.HashMap;
 import com.intellij.util.ui.*;
 import com.intellij.util.ui.tree.TreeUtil;
 import gnu.trove.THashSet;
+import gnu.trove.TObjectIntHashMap;
 import net.miginfocom.swing.MigLayout;
 import org.jetbrains.annotations.NonNls;
 import org.jetbrains.annotations.NotNull;
@@ -64,7 +67,7 @@ import static com.intellij.openapi.ui.LabeledComponent.create;
 import static com.intellij.openapi.wm.IdeFocusManager.getGlobalInstance;
 import static com.intellij.ui.RowsDnDSupport.RefinedDropSupport.Position.*;
 
-class RunConfigurable extends BaseConfigurable {
+class RunConfigurable extends BaseConfigurable implements Disposable {
   @NonNls private static final Object DEFAULTS = new Object() {
     @Override
     public String toString() {
@@ -642,13 +645,19 @@ class RunConfigurable extends BaseConfigurable {
     try {
       updateActiveConfigurationFromSelected();
 
+      TObjectIntHashMap<RunnerAndConfigurationSettings> settingsToOrder = new TObjectIntHashMap<>();
+      int order = 0;
       Set<RunnerAndConfigurationSettings> toDeleteSettings = new THashSet<>(manager.getAllSettings());
       RunnerAndConfigurationSettings selectedSettings = getSelectedSettings();
       for (int i = 0; i < myRoot.getChildCount(); i++) {
         DefaultMutableTreeNode node = (DefaultMutableTreeNode)myRoot.getChildAt(i);
         Object userObject = node.getUserObject();
         if (userObject instanceof ConfigurationType) {
-          applyByType(node, (ConfigurationType)userObject, selectedSettings, toDeleteSettings);
+          List<RunConfigurationBean> beans = applyByType(node, (ConfigurationType)userObject, selectedSettings);
+          for (RunConfigurationBean bean : beans) {
+            settingsToOrder.put(bean.getSettings(), order++);
+            toDeleteSettings.remove(bean.getSettings());
+          }
         }
       }
       manager.removeConfigurations(toDeleteSettings);
@@ -670,7 +679,7 @@ class RunConfigurable extends BaseConfigurable {
         each.first.apply();
       }
 
-      manager.setOrder(null);
+      manager.setOrder(Comparator.comparingInt(settingsToOrder::get));
     }
     finally {
       manager.fireEndUpdate();
@@ -686,7 +695,7 @@ class RunConfigurable extends BaseConfigurable {
     }
   }
 
-  private void applyByType(@NotNull DefaultMutableTreeNode typeNode, @NotNull ConfigurationType type, @Nullable RunnerAndConfigurationSettings selectedSettings, @NotNull Set<RunnerAndConfigurationSettings> toDeleteSettings) throws ConfigurationException {
+  private List<RunConfigurationBean> applyByType(@NotNull DefaultMutableTreeNode typeNode, @NotNull ConfigurationType type, @Nullable RunnerAndConfigurationSettings selectedSettings) throws ConfigurationException {
     int indexToMove = -1;
 
     final List<RunConfigurationBean> configurationBeans = new ArrayList<>();
@@ -739,7 +748,6 @@ class RunConfigurable extends BaseConfigurable {
     // try to apply all
     for (RunConfigurationBean bean : configurationBeans) {
       applyConfiguration(typeNode, bean.getConfigurable());
-      toDeleteSettings.remove(bean.getSettings());
     }
 
     //Just saved as 'stable' configuration shouldn't stay between temporary ones (here we order model to save)
@@ -750,6 +758,8 @@ class RunConfigurable extends BaseConfigurable {
     if (shift != 0 && indexToMove != -1) {
       configurationBeans.add(indexToMove-shift, configurationBeans.remove(indexToMove));
     }
+
+    return configurationBeans;
   }
 
   static void collectNodesRecursively(DefaultMutableTreeNode parentNode, List<DefaultMutableTreeNode> nodes, NodeKind... allowed) {
@@ -852,6 +862,11 @@ class RunConfigurable extends BaseConfigurable {
 
   @Override
   public void disposeUIResources() {
+    Disposer.dispose(this);
+  }
+
+  @Override
+  public void dispose() {
     isDisposed = true;
     for (Configurable configurable : myStoredComponents.values()) {
       configurable.disposeUIResources();
@@ -1638,6 +1653,7 @@ class RunConfigurable extends BaseConfigurable {
       myShared = configurable.isStoreProjectConfiguration();
     }
 
+    @NotNull
     public RunnerAndConfigurationSettings getSettings() {
       return mySettings;
     }

platform/lang-impl/src/com/intellij/execution/impl/RunManagerImpl.kt

@@ -262,24 +262,23 @@ open class RunManagerImpl(internal val project: Project) : RunManagerEx(), Persi
     lock.write {
       immutableSortedSettingsList = null
 
-      existingId = findExistingConfigurationId(settings)
       // https://youtrack.jetbrains.com/issue/IDEA-112821
       // we should check by instance, not by id (todo is it still relevant?)
+      existingId = if (idToSettings.get(newId) === settings) newId else findExistingConfigurationId(settings)
       existingId?.let {
-        // idToSettings is a LinkedHashMap - we must remove even if existingId equals to newId and in any case we will replace it on put
-        idToSettings.remove(it)
+        if (newId != it) {
+          idToSettings.remove(it)
+        }
       }
 
+      idToSettings.put(newId, settings)
+
       if (selectedConfigurationId != null && selectedConfigurationId == existingId) {
         selectedConfigurationId = newId
       }
-      idToSettings.put(newId, settings)
 
       if (existingId == null) {
         refreshUsagesList(settings)
-      }
-
-      if (existingId == null) {
         settings.schemeManager?.addScheme(settings as RunnerAndConfigurationSettingsImpl)
       }
       else {
@@ -287,9 +286,10 @@ open class RunManagerImpl(internal val project: Project) : RunManagerEx(), Persi
       }
     }
 
-    checkRecentsLimit()
-
     if (existingId == null) {
+      if (settings.isTemporary) {
+        checkRecentsLimit()
+      }
       eventPublisher.runConfigurationAdded(settings)
     }
     else {