From ee0e04e953aad4fa7bf6616da3e3a36270cf2b81 Mon Sep 17 00:00:00 2001 From: Vladimir Krivosheev Date: Thu, 20 Aug 2015 19:36:17 +0200 Subject: [PATCH] get rid of pathToModule map --- .../testSrc/ModuleStoreRenameTest.kt | 38 +++++----- .../testSrc/ModuleStoreTest.kt | 2 +- .../openapi/module/impl/ModuleImpl.java | 12 +--- .../module/impl/ModuleManagerComponent.java | 2 +- .../openapi/module/ModifiableModuleModel.java | 2 - .../module/impl/ModuleManagerImpl.java | 69 +++++++++---------- .../com/intellij/testFramework/FixtureRule.kt | 24 ++++--- 7 files changed, 71 insertions(+), 78 deletions(-) diff --git a/platform/configuration-store-impl/testSrc/ModuleStoreRenameTest.kt b/platform/configuration-store-impl/testSrc/ModuleStoreRenameTest.kt index 5396a23fcb97..b6c21e7841db 100644 --- a/platform/configuration-store-impl/testSrc/ModuleStoreRenameTest.kt +++ b/platform/configuration-store-impl/testSrc/ModuleStoreRenameTest.kt @@ -10,18 +10,16 @@ import com.intellij.openapi.components.stateStore import com.intellij.openapi.module.ModifiableModuleModel import com.intellij.openapi.module.Module import com.intellij.openapi.module.ModuleManager +import com.intellij.openapi.module.ModuleTypeId import com.intellij.openapi.project.ModuleAdapter import com.intellij.openapi.project.Project import com.intellij.openapi.util.io.systemIndependentPath import com.intellij.openapi.vfs.LocalFileSystem -import com.intellij.testFramework.FixtureRule -import com.intellij.testFramework.RuleChain -import com.intellij.testFramework.builders.EmptyModuleFixtureBuilder -import com.intellij.testFramework.fixtures.ModuleFixture -import com.intellij.testFramework.runInEdtAndWait +import com.intellij.testFramework.* import com.intellij.util.Function import com.intellij.util.SmartList import org.assertj.core.api.Assertions.assertThat +import org.junit.ClassRule import org.junit.Rule import org.junit.Test import org.junit.rules.ExternalResource @@ -30,18 +28,26 @@ import java.util.UUID import kotlin.properties.Delegates class ModuleStoreRenameTest { - var moduleFixture: ModuleFixture by Delegates.notNull() - val module by Delegates.lazy { moduleFixture.getModule() } + companion object { + ClassRule val projectRule = ProjectRule() + } + + var module: Module by Delegates.notNull() // we test fireModuleRenamedByVfsEvent private val oldModuleNames = SmartList() - private val fixtureManager = FixtureRule { - moduleFixture = addModule(javaClass>()).getFixture() - } + private val tempDirManager = TemporaryDirectory() private val ruleChain = RuleChain( + tempDirManager, object : ExternalResource() { + override fun before() { + runInEdtAndWait { + module = runWriteAction { ModuleManager.getInstance(projectRule.project).newModule(tempDirManager.newPath().resolve("m.iml").systemIndependentPath, ModuleTypeId.JAVA_MODULE) } + } + } + // should be invoked after project tearDown override fun after() { (ApplicationManager.getApplication().stateStore.getStateStorageManager() as StateStorageManagerImpl).getVirtualFileTracker()!!.remove { @@ -52,7 +58,7 @@ class ModuleStoreRenameTest { } } }, - fixtureManager, + DisposeModulesRule(projectRule), object : ExternalResource() { override fun before() { module.getMessageBus().connect().subscribe(ProjectTopics.MODULES, object : ModuleAdapter() { @@ -69,7 +75,7 @@ class ModuleStoreRenameTest { fun Module.change(task: ModifiableModuleModel.() -> Unit) { runInEdtAndWait { - val model = ModuleManager.getInstance(fixtureManager.projectFixture.getProject()).getModifiableModel() + val model = ModuleManager.getInstance(projectRule.project).getModifiableModel() runWriteAction { model.task() model.commit() @@ -78,7 +84,7 @@ class ModuleStoreRenameTest { } // project structure - public Test fun `rename module using model`() { + @Test fun `rename module using model`() { runInEdtAndWait { module.saveStore() } val storage = module.stateStore.getStateStorageManager().getStateStorage(StoragePathMacros.MODULE_FILE, RoamingType.PER_USER) as FileBasedStorage val oldFile = storage.file @@ -92,7 +98,7 @@ class ModuleStoreRenameTest { } // project view - public Test fun `rename module using rename virtual file`() { + @Test fun `rename module using rename virtual file`() { runInEdtAndWait { module.saveStore() } var storage = module.stateStore.getStateStorageManager().getStateStorage(StoragePathMacros.MODULE_FILE, RoamingType.PER_USER) as FileBasedStorage val oldFile = storage.file @@ -108,7 +114,7 @@ class ModuleStoreRenameTest { // we cannot test external rename yet, because it is not supported - ModuleImpl doesn't support delete and create events (in case of external change we don't get move event, but get "delete old" and "create new") private fun assertRename(newName: String, oldFile: File) { - val storageManager = moduleFixture.getModule().stateStore.getStateStorageManager() + val storageManager = module.stateStore.getStateStorageManager() val newFile = (storageManager.getStateStorage(StoragePathMacros.MODULE_FILE, RoamingType.PER_USER) as FileBasedStorage).file assertThat(newFile.getName()).isEqualTo("$newName${ModuleFileType.DOT_DEFAULT_EXTENSION}") assertThat(oldFile) @@ -120,7 +126,7 @@ class ModuleStoreRenameTest { assertThat(storageManager.expandMacros(StoragePathMacros.MODULE_FILE)).isEqualTo(newFile.systemIndependentPath) } - public Test fun `rename module parent virtual dir`() { + @Test fun `rename module parent virtual dir`() { runInEdtAndWait { module.saveStore() } val storageManager = module.stateStore.getStateStorageManager() val storage = storageManager.getStateStorage(StoragePathMacros.MODULE_FILE, RoamingType.PER_USER) as FileBasedStorage diff --git a/platform/configuration-store-impl/testSrc/ModuleStoreTest.kt b/platform/configuration-store-impl/testSrc/ModuleStoreTest.kt index bec5d965fa75..96e30f8dfe66 100644 --- a/platform/configuration-store-impl/testSrc/ModuleStoreTest.kt +++ b/platform/configuration-store-impl/testSrc/ModuleStoreTest.kt @@ -41,7 +41,7 @@ class ModuleStoreTest { private fun VirtualFile.loadModule() = runWriteAction { ModuleManager.getInstance(projectRule.project).loadModule(getPath()) } - private fun Path.createModule() = runWriteAction { ModuleManager.getInstance(projectRule.project).newModule(systemIndependentPath, ModuleTypeId.JAVA_MODULE) } + fun Path.createModule() = runWriteAction { ModuleManager.getInstance(projectRule.project).newModule(systemIndependentPath, ModuleTypeId.JAVA_MODULE) } } private val tempDirManager = TemporaryDirectory() diff --git a/platform/lang-impl/src/com/intellij/openapi/module/impl/ModuleImpl.java b/platform/lang-impl/src/com/intellij/openapi/module/impl/ModuleImpl.java index c1e5d1cb232f..23e6adde2b35 100644 --- a/platform/lang-impl/src/com/intellij/openapi/module/impl/ModuleImpl.java +++ b/platform/lang-impl/src/com/intellij/openapi/module/impl/ModuleImpl.java @@ -27,7 +27,6 @@ import com.intellij.openapi.diagnostic.Logger; import com.intellij.openapi.extensions.AreaInstance; import com.intellij.openapi.extensions.ExtensionPointName; import com.intellij.openapi.extensions.Extensions; -import com.intellij.openapi.module.ModifiableModuleModel; import com.intellij.openapi.module.Module; import com.intellij.openapi.module.ModuleComponent; import com.intellij.openapi.module.ModuleServiceManager; @@ -340,18 +339,13 @@ public class ModuleImpl extends PlatformComponentManagerImpl implements ModuleEx String ancestorPath = parentPath + "/" + event.getOldValue(); String moduleFilePath = getModuleFilePath(); if (VfsUtilCore.isAncestor(new File(ancestorPath), new File(moduleFilePath), true)) { - setModuleFilePath(moduleFilePath, parentPath + "/" + event.getNewValue() + "/" + FileUtil.getRelativePath(ancestorPath, moduleFilePath, '/')); + setModuleFilePath(parentPath + "/" + event.getNewValue() + "/" + FileUtil.getRelativePath(ancestorPath, moduleFilePath, '/')); } } } - private void setModuleFilePath(String moduleFilePath, String newFilePath) { + private void setModuleFilePath(String newFilePath) { ClasspathStorage.modulePathChanged(ModuleImpl.this, newFilePath); - - final ModifiableModuleModel modifiableModel = ModuleManagerImpl.getInstanceImpl(getProject()).getModifiableModel(); - modifiableModel.setModuleFilePath(ModuleImpl.this, moduleFilePath, newFilePath); - modifiableModel.commit(); - getMainStorage(ModuleImpl.this).setFile(null, new File(newFilePath)); ComponentsPackage.getStateStore(ModuleImpl.this).setPath(FileUtilRt.toSystemIndependentName(newFilePath)); } @@ -362,7 +356,7 @@ public class ModuleImpl extends PlatformComponentManagerImpl implements ModuleEx String ancestorPath = event.getOldParent().getPath() + "/" + dirName; String moduleFilePath = getModuleFilePath(); if (VfsUtilCore.isAncestor(new File(ancestorPath), new File(moduleFilePath), true)) { - setModuleFilePath(moduleFilePath, event.getNewParent().getPath() + "/" + dirName + "/" + FileUtil.getRelativePath(ancestorPath, moduleFilePath, '/')); + setModuleFilePath(event.getNewParent().getPath() + "/" + dirName + "/" + FileUtil.getRelativePath(ancestorPath, moduleFilePath, '/')); } } } diff --git a/platform/lang-impl/src/com/intellij/openapi/module/impl/ModuleManagerComponent.java b/platform/lang-impl/src/com/intellij/openapi/module/impl/ModuleManagerComponent.java index 235138b03d9d..418aa3cf1df7 100644 --- a/platform/lang-impl/src/com/intellij/openapi/module/impl/ModuleManagerComponent.java +++ b/platform/lang-impl/src/com/intellij/openapi/module/impl/ModuleManagerComponent.java @@ -135,7 +135,7 @@ public class ModuleManagerComponent extends ModuleManagerImpl { Runnable runnableWithProgress = new Runnable() { @Override public void run() { - for (final Module module : myModuleModel.myPathToModule.values()) { + for (final Module module : myModuleModel.myModules) { final Application app = ApplicationManager.getApplication(); final Runnable swingRunnable = new Runnable() { @Override diff --git a/platform/projectModel-api/src/com/intellij/openapi/module/ModifiableModuleModel.java b/platform/projectModel-api/src/com/intellij/openapi/module/ModifiableModuleModel.java index d4c5634801c1..15e8c5dc4324 100644 --- a/platform/projectModel-api/src/com/intellij/openapi/module/ModifiableModuleModel.java +++ b/platform/projectModel-api/src/com/intellij/openapi/module/ModifiableModuleModel.java @@ -138,6 +138,4 @@ public interface ModifiableModuleModel { boolean hasModuleGroups(); void setModuleGroupPath(@NotNull Module module, @Nullable("null means remove") String[] groupPath); - - void setModuleFilePath(@NotNull Module module, String oldPath, String newFilePath); } diff --git a/platform/projectModel-impl/src/com/intellij/openapi/module/impl/ModuleManagerImpl.java b/platform/projectModel-impl/src/com/intellij/openapi/module/impl/ModuleManagerImpl.java index c526047f2aee..b215cc9291f3 100644 --- a/platform/projectModel-impl/src/com/intellij/openapi/module/impl/ModuleManagerImpl.java +++ b/platform/projectModel-impl/src/com/intellij/openapi/module/impl/ModuleManagerImpl.java @@ -36,6 +36,7 @@ import com.intellij.openapi.roots.impl.ModifiableModelCommitter; import com.intellij.openapi.util.Comparing; import com.intellij.openapi.util.Disposer; import com.intellij.openapi.util.Key; +import com.intellij.openapi.util.SystemInfo; import com.intellij.openapi.util.io.FileUtil; import com.intellij.openapi.util.text.StringUtil; import com.intellij.openapi.vfs.StandardFileSystems; @@ -44,17 +45,13 @@ import com.intellij.openapi.vfs.VirtualFileManager; import com.intellij.util.Function; import com.intellij.util.containers.ContainerUtil; import com.intellij.util.containers.HashMap; -import com.intellij.util.containers.HashSet; import com.intellij.util.containers.StringInterner; -import com.intellij.util.containers.hash.EqualityPolicy; -import com.intellij.util.containers.hash.LinkedHashMap; import com.intellij.util.graph.CachingSemiGraph; import com.intellij.util.graph.DFSTBuilder; import com.intellij.util.graph.Graph; import com.intellij.util.graph.GraphGenerator; import com.intellij.util.io.URLUtil; import com.intellij.util.messages.MessageBus; -import com.intellij.util.text.FilePathHashingStrategy; import gnu.trove.THashMap; import gnu.trove.TObjectHashingStrategy; import org.jdom.Element; @@ -331,8 +328,9 @@ public abstract class ModuleManagerImpl extends ModuleManager implements Project myModuleModel.myModulesCache = null; for (ModuleLoadingErrorDescription error : errors) { - final Module module = myModuleModel.myPathToModule.remove(FileUtil.toSystemIndependentName(error.getModulePath().getPath())); + final Module module = myModuleModel.getModuleByFilePath(FileUtil.toSystemIndependentName(error.getModulePath().getPath())); if (module != null) { + myModuleModel.myModules.remove(module); ApplicationManager.getApplication().invokeLater(new Runnable() { @Override public void run() { @@ -586,7 +584,7 @@ public abstract class ModuleManagerImpl extends ModuleManager implements Project } protected void fireModulesAdded() { - for (final Module module : myModuleModel.myPathToModule.values()) { + for (final Module module : myModuleModel.myModules) { fireModuleAddedInWriteAction(module); } } @@ -617,7 +615,7 @@ public abstract class ModuleManagerImpl extends ModuleManager implements Project protected abstract ModuleEx createAndLoadModule(@NotNull String filePath) throws IOException; class ModuleModelImpl implements ModifiableModuleModel { - final Map myPathToModule = new LinkedHashMap(new EqualityPolicy.ByHashingStrategy(FilePathHashingStrategy.create())); + final Set myModules = new LinkedHashSet(); private volatile Module[] myModulesCache; private final List myModulesToDispose = new ArrayList(); @@ -631,7 +629,7 @@ public abstract class ModuleManagerImpl extends ModuleManager implements Project } private ModuleModelImpl(@NotNull ModuleModelImpl that) { - myPathToModule.putAll(that.myPathToModule); + myModules.addAll(that.myModules); final Map groupPath = that.myModuleGroupPath; if (groupPath != null){ myModuleGroupPath = new THashMap(); @@ -648,7 +646,7 @@ public abstract class ModuleManagerImpl extends ModuleManager implements Project @NotNull public Module[] getModules() { if (myModulesCache == null) { - Collection modules = myPathToModule.values(); + Collection modules = myModules; myModulesCache = modules.toArray(new Module[modules.size()]); } return myModulesCache; @@ -740,7 +738,12 @@ public abstract class ModuleManagerImpl extends ModuleManager implements Project @Nullable private ModuleEx getModuleByFilePath(@NotNull String filePath) { - return (ModuleEx)myPathToModule.get(filePath); + for (Module module : myModules) { + if (SystemInfo.isFileSystemCaseSensitive ? module.getModuleFilePath().equals(filePath) : module.getModuleFilePath().equalsIgnoreCase(filePath)) { + return (ModuleEx)module; + } + } + return null; } @Override @@ -770,7 +773,7 @@ public abstract class ModuleManagerImpl extends ModuleManager implements Project if (name.endsWith(IML_EXTENSION)) { final String moduleName = name.substring(0, name.length() - 4); - for (Module module : myPathToModule.values()) { + for (Module module : myModules) { if (module.getName().equals(moduleName)) { throw new ModuleWithNameAlreadyExists(ProjectBundle.message("module.already.exists.error", moduleName), moduleName); } @@ -795,15 +798,14 @@ public abstract class ModuleManagerImpl extends ModuleManager implements Project private void initModule(@NotNull ModuleEx module, @NotNull String path, @Nullable Runnable beforeComponentCreation) { module.init(path, beforeComponentCreation); myModulesCache = null; - myPathToModule.put(path, module); + myModules.add(module); } @Override public void disposeModule(@NotNull Module module) { assertWritable(); myModulesCache = null; - if (myPathToModule.values().contains(module)) { - myPathToModule.remove(module.getModuleFilePath()); + if (myModules.remove(module)) { myModulesToDispose.add(module); } if (myModuleGroupPath != null){ @@ -813,7 +815,7 @@ public abstract class ModuleManagerImpl extends ModuleManager implements Project @Override public Module findModuleByName(@NotNull String name) { - for (Module module : myPathToModule.values()) { + for (Module module : myModules) { if (!module.isDisposed() && module.getName().equals(name)) { return module; } @@ -830,7 +832,7 @@ public abstract class ModuleManagerImpl extends ModuleManager implements Project return GraphGenerator.create(CachingSemiGraph.create(new GraphGenerator.SemiGraph() { @Override public Collection getNodes() { - return myPathToModule.values(); + return myModules; } @Override @@ -843,7 +845,7 @@ public abstract class ModuleManagerImpl extends ModuleManager implements Project @NotNull private List getModuleDependentModules(Module module) { List result = new ArrayList(); - for (Module aModule : myPathToModule.values()) { + for (Module aModule : myModules) { if (isModuleDependent(aModule, module)) { result.add(aModule); } @@ -875,8 +877,8 @@ public abstract class ModuleManagerImpl extends ModuleManager implements Project public void dispose() { assertWritable(); ApplicationManager.getApplication().assertWriteAccessAllowed(); - final Collection list = myModuleModel.myPathToModule.values(); - final Collection thisModules = myPathToModule.values(); + final Collection list = myModuleModel.myModules; + final Collection thisModules = myModules; for (Module thisModule : thisModules) { if (!list.contains(thisModule)) { Disposer.dispose(thisModule); @@ -895,22 +897,20 @@ public abstract class ModuleManagerImpl extends ModuleManager implements Project if (!myIsWritable) { return false; } - Set thisModules = new HashSet(myPathToModule.values()); - Set thatModules = new HashSet(myModuleModel.myPathToModule.values()); - return !thisModules.equals(thatModules) || !Comparing.equal(myModuleModel.myModuleGroupPath, myModuleGroupPath); + return !myModules.equals(myModuleModel.myModules) || !Comparing.equal(myModuleModel.myModuleGroupPath, myModuleGroupPath); } private void disposeModel() { myModulesCache = null; - for (final Module module : myPathToModule.values()) { + for (final Module module : myModules) { Disposer.dispose(module); } - myPathToModule.clear(); + myModules.clear(); myModuleGroupPath = null; } public void projectOpened() { - final Collection collection = myPathToModule.values(); + final Collection collection = myModules; for (final Module aCollection : collection) { ModuleEx module = (ModuleEx)aCollection; module.projectOpened(); @@ -918,8 +918,7 @@ public abstract class ModuleManagerImpl extends ModuleManager implements Project } public void projectClosed() { - final Collection collection = myPathToModule.values(); - for (final Module aCollection : collection) { + for (Module aCollection : myModules) { ModuleEx module = (ModuleEx)aCollection; module.projectClosed(); } @@ -947,20 +946,14 @@ public abstract class ModuleManagerImpl extends ModuleManager implements Project myModuleGroupPath.put(module, groupPath); } } - - @Override - public void setModuleFilePath(@NotNull Module module, String oldPath, String newFilePath) { - myPathToModule.remove(oldPath); - myPathToModule.put(newFilePath, module); - } } private void commitModel(final ModuleModelImpl moduleModel, final Runnable runnable) { myModuleModel.myModulesCache = null; incModificationCount(); ApplicationManager.getApplication().assertWriteAccessAllowed(); - final Collection oldModules = myModuleModel.myPathToModule.values(); - final Collection newModules = moduleModel.myPathToModule.values(); + final Collection oldModules = myModuleModel.myModules; + final Collection newModules = moduleModel.myModules; final List removedModules = new ArrayList(oldModules); removedModules.removeAll(newModules); final List addedModules = new ArrayList(newModules); @@ -975,7 +968,7 @@ public abstract class ModuleManagerImpl extends ModuleManager implements Project } List neverAddedModules = new ArrayList(moduleModel.myModulesToDispose); - neverAddedModules.removeAll(myModuleModel.myPathToModule.values()); + neverAddedModules.removeAll(myModuleModel.myModules); for (final Module neverAddedModule : neverAddedModules) { neverAddedModule.putUserData(DISPOSED_MODULE_NAME, neverAddedModule.getName()); Disposer.dispose(neverAddedModule); @@ -993,10 +986,10 @@ public abstract class ModuleManagerImpl extends ModuleManager implements Project Map oldNames = ContainerUtil.newHashMap(); for (final Module module : modulesToBeRenamed) { oldNames.put(module, module.getName()); - moduleModel.myPathToModule.remove(module.getModuleFilePath()); + moduleModel.myModules.remove(module); modules.add(module); ((ModuleEx)module).rename(modulesToNewNamesMap.get(module)); - moduleModel.myPathToModule.put(module.getModuleFilePath(), module); + moduleModel.myModules.add(module); } moduleModel.myIsWritable = false; diff --git a/platform/testFramework/src/com/intellij/testFramework/FixtureRule.kt b/platform/testFramework/src/com/intellij/testFramework/FixtureRule.kt index eac49ca98e3b..bf2817250882 100644 --- a/platform/testFramework/src/com/intellij/testFramework/FixtureRule.kt +++ b/platform/testFramework/src/com/intellij/testFramework/FixtureRule.kt @@ -277,19 +277,21 @@ public class DisposeModulesRule(private val projectRule: ProjectRule) : External projectRule.projectIfOpened?.let { var errors: MutableList? = null val moduleManager = ModuleManager.getInstance(it) - for (module in moduleManager.getModules()) { - if (module.isDisposed()) { - continue - } + runInEdtAndWait { + for (module in moduleManager.getModules()) { + if (module.isDisposed()) { + continue + } - try { - moduleManager.disposeModule(module) - } - catch(e: Throwable) { - if (errors == null) { - errors = SmartList() + try { + moduleManager.disposeModule(module) + } + catch(e: Throwable) { + if (errors == null) { + errors = SmartList() + } + errors!!.add(e) } - errors.add(e) } } CompoundRuntimeException.doThrow(errors)