Respect installing modules during dependency resolution

ConsoleUI/DependencyScreen.cs

@@ -7,7 +7,7 @@
 namespace CKAN.ConsoleUI {
 
     /// <summary>
-    /// Screen for letting the user choose additional pacakges to install
+    /// Screen for letting the user choose additional packages to install
     /// based on others they've selected
     /// </summary>
     public class DependencyScreen : ConsoleScreen {

ConsoleUI/InstallScreen.cs

@@ -54,14 +54,14 @@ public override void Run(Action process = null)
                         }
 
                         // FUTURE: BackgroundWorker
-                        
+
                         HashSet<string> possibleConfigOnlyDirs = null;
 
                         RegistryManager regMgr = RegistryManager.Instance(manager.CurrentInstance);
                         ModuleInstaller inst = ModuleInstaller.GetInstance(manager.CurrentInstance, manager.Cache, this);
                         inst.onReportModInstalled = OnModInstalled;
                         if (plan.Remove.Count > 0) {
-                            inst.UninstallList(plan.Remove, ref possibleConfigOnlyDirs, regMgr);
+                            inst.UninstallList(plan.Remove, ref possibleConfigOnlyDirs, regMgr, true, plan.Install);
                             plan.Remove.Clear();
                         }
                         NetAsyncModulesDownloader dl = new NetAsyncModulesDownloader(this, manager.Cache);

Core/ModuleInstaller.cs

@@ -727,7 +727,10 @@ internal static void CopyZipEntry(ZipFile zipfile, ZipEntry entry, string fullPa
         /// This *DOES* save the registry.
         /// Preferred over Uninstall.
         /// </summary>
-        public void UninstallList(IEnumerable<string> mods, ref HashSet<string> possibleConfigOnlyDirs, RegistryManager registry_manager, bool ConfirmPrompt = true, IEnumerable<string> installing = null)
+        public void UninstallList(
+            IEnumerable<string> mods, ref HashSet<string> possibleConfigOnlyDirs,
+            RegistryManager registry_manager, bool ConfirmPrompt = true, IEnumerable<CkanModule> installing = null
+        )
         {
             mods = mods.Memoize();
             // Pre-check, have they even asked for things which are installed?
@@ -740,8 +743,9 @@ public void UninstallList(IEnumerable<string> mods, ref HashSet<string> possible
             // Find all the things which need uninstalling.
             IEnumerable<string> revdep = mods
                 .Union(registry_manager.registry.FindReverseDependencies(
-                    mods.Except(installing ?? new string[] {})))
-                .Memoize();
+                    mods.Except(installing?.Select(m => m.identifier) ?? new string[] {}),
+                    installing
+                )).Memoize();
             IEnumerable<string> goners = revdep.Union(
                     registry_manager.registry.FindRemovableAutoInstalled(
                         registry_manager.registry.InstalledModules.Where(im => !revdep.Contains(im.identifier)))
@@ -796,12 +800,6 @@ public void UninstallList(IEnumerable<string> mods, ref HashSet<string> possible
             User.RaiseMessage("Done!\r\n");
         }
 
-        public void UninstallList(string mod, ref HashSet<string> possibleConfigOnlyDirs, RegistryManager registry_manager)
-        {
-            var list = new List<string> { mod };
-            UninstallList(list, ref possibleConfigOnlyDirs, registry_manager);
-        }
-
         /// <summary>
         /// Uninstall the module provided. For internal use only.
         /// Use UninstallList for user queries, it also does dependency handling.

Core/Registry/IRegistryQuerier.cs

@@ -74,7 +74,9 @@ List<CkanModule> LatestAvailableWithProvides(
         /// <summary>
         /// Finds and returns all modules that could not exist without the listed modules installed, including themselves.
         /// </summary>
-        IEnumerable<string> FindReverseDependencies(IEnumerable<string> modules);
+        IEnumerable<string> FindReverseDependencies(
+            IEnumerable<string> modulesToRemove, IEnumerable<CkanModule> modulesToInstall = null
+        );
 
         /// <summary>
         /// Find auto-installed modules that have no depending modules

Core/Registry/Registry.cs

@@ -1027,45 +1027,65 @@ public List<string> GetSanityErrors()
         /// Finds and returns all modules that could not exist without the listed modules installed, including themselves.
         /// Acts recursively and lazily.
         /// </summary>
+        /// <param name="modulesToRemove">Modules that are about to be removed.</param>
+        /// <param name="modulesToInstall">Optional list of modules that are about to be installed.</param>
+        /// <param name="origInstalled">Modules that are already installed</param>
+        /// <param name="dlls">Installed DLLs</param>
+        /// <param name="dlc">Installed DLCs</param>
+        /// <returns>List of modules whose dependencies are about to be or already removed.</returns>
         internal static IEnumerable<string> FindReverseDependencies(
-            IEnumerable<string> modules_to_remove,
-            IEnumerable<CkanModule> orig_installed,
+            IEnumerable<string> modulesToRemove,
+            IEnumerable<CkanModule> modulesToInstall,
+            IEnumerable<CkanModule> origInstalled,
             IEnumerable<string> dlls,
             IDictionary<string, UnmanagedModuleVersion> dlc
         )
         {
-            modules_to_remove = modules_to_remove.Memoize();
-            orig_installed    = orig_installed.Memoize();
+            modulesToRemove = modulesToRemove.Memoize();
+            origInstalled    = origInstalled.Memoize();
             var dllSet = dlls.ToHashSet();
             // The empty list has no reverse dependencies
             // (Don't remove broken modules if we're only installing)
-            if (modules_to_remove.Any())
+            if (modulesToRemove.Any())
             {
                 // All modules in the input are included in the output
-                foreach (string starter in modules_to_remove)
+                foreach (string starter in modulesToRemove)
                 {
                     yield return starter;
                 }
                 while (true)
                 {
                     // Make our hypothetical install, and remove the listed modules from it.
-                    HashSet<CkanModule> hypothetical = new HashSet<CkanModule>(orig_installed); // Clone because we alter hypothetical.
-                    hypothetical.RemoveWhere(mod => modules_to_remove.Contains(mod.identifier));
+                    HashSet<CkanModule> hypothetical = new HashSet<CkanModule>(origInstalled); // Clone because we alter hypothetical.
+                    if (modulesToInstall != null)
+                    {
+                        // Pretend the mods we are going to install are already installed, so that dependencies that will be
+                        // satisfied by a mod that is going to be installed count as satisfied.
+                        hypothetical = hypothetical.Concat(modulesToInstall).ToHashSet();
+                    }
+                    hypothetical.RemoveWhere(mod => modulesToRemove.Contains(mod.identifier));
 
-                    log.DebugFormat("Started with {0}, removing {1}, and keeping {2}; our dlls are {3}", string.Join(", ", orig_installed), string.Join(", ", modules_to_remove), string.Join(", ", hypothetical), string.Join(", ", dllSet));
+                    log.DebugFormat("Started with {0}, removing {1}, and keeping {2}; our dlls are {3}", string.Join(", ", origInstalled), string.Join(", ", modulesToRemove), string.Join(", ", hypothetical), string.Join(", ", dllSet));
 
                     // Find what would break with this configuration.
                     var broken = SanityChecker.FindUnsatisfiedDepends(hypothetical, dllSet, dlc)
                         .Select(x => x.Key.identifier).ToHashSet();
 
+                    if (modulesToInstall != null)
+                    {
+                        // Make sure to only report modules as broken if they are actually currently installed.
+                        // This is mainly to remove the modulesToInstall again which we added
+                        // earlier to the hypothetical list.
+                        broken.IntersectWith(origInstalled.Select(m => m.identifier));
+                    }
                     // Lazily return each newly found rev dep
-                    foreach (string newFound in broken.Except(modules_to_remove))
+                    foreach (string newFound in broken.Except(modulesToRemove))
                     {
                         yield return newFound;
                     }
 
                     // If nothing else would break, it's just the list of modules we're removing.
-                    HashSet<string> to_remove = new HashSet<string>(modules_to_remove);
+                    HashSet<string> to_remove = new HashSet<string>(modulesToRemove);
 
                     if (to_remove.IsSupersetOf(broken))
                     {
@@ -1075,18 +1095,21 @@ IDictionary<string, UnmanagedModuleVersion> dlc
 
                     // Otherwise, remove our broken modules as well, and recurse.
                     broken.UnionWith(to_remove);
-                    modules_to_remove = broken;
+                    modulesToRemove = broken;
                 }
             }
         }
 
         /// <summary>
         /// Return modules which are dependent on the modules passed in or modules in the return list
         /// </summary>
-        public IEnumerable<string> FindReverseDependencies(IEnumerable<string> modules_to_remove)
+        public IEnumerable<string> FindReverseDependencies(
+            IEnumerable<string> modulesToRemove,
+            IEnumerable<CkanModule> modulesToInstall = null
+        )
         {
             var installed = new HashSet<CkanModule>(installed_modules.Values.Select(x => x.Module));
-            return FindReverseDependencies(modules_to_remove, installed, new HashSet<string>(installed_dlls.Keys), _installedDlcModules);
+            return FindReverseDependencies(modulesToRemove, modulesToInstall, installed, new HashSet<string>(installed_dlls.Keys), _installedDlcModules);
         }
 
         /// <summary>
@@ -1112,7 +1135,7 @@ IDictionary<string, UnmanagedModuleVersion> dlc
             var instCkanMods = installedModules.Select(im => im.Module);
             return autoInstMods.Where(
                 im => autoInstIds.IsSupersetOf(FindReverseDependencies(
-                    new List<string> { im.identifier }, instCkanMods, dlls, dlc)));
+                    new List<string> { im.identifier }, null, instCkanMods, dlls, dlc)));
         }
 
         /// <summary>

Core/Relationships/RelationshipResolver.cs

@@ -311,7 +311,7 @@ private void Resolve(CkanModule module, RelationshipResolverOptions options, IEn
             // is true.
             var sub_options = (RelationshipResolverOptions) options.Clone();
             sub_options.with_suggests = false;
-            
+
             old_stanza = old_stanza?.Memoize();
 
             log.DebugFormat("Resolving dependencies for {0}", module.identifier);
@@ -364,8 +364,7 @@ private void ResolveStanza(IEnumerable<RelationshipDescriptor> stanza, Selection
                 else if (descriptor.ContainsAny(modlist.Keys))
                 {
                     CkanModule module = modlist.Values
-                        .Where(m => descriptor.ContainsAny(new string[] { m.identifier }))
-                        .FirstOrDefault();
+                        .FirstOrDefault(m => descriptor.ContainsAny(new string[] { m.identifier }));
                     if (options.proceed_with_inconsistencies)
                     {
                         conflicts.Add(new KeyValuePair<CkanModule, CkanModule>(module, reason.Parent));
@@ -392,8 +391,7 @@ private void ResolveStanza(IEnumerable<RelationshipDescriptor> stanza, Selection
                 {
                     CkanModule module = registry.InstalledModules
                         .Select(im => im.Module)
-                        .Where(m => descriptor.ContainsAny(new string[] { m.identifier }))
-                        .FirstOrDefault();
+                        .FirstOrDefault(m => descriptor.ContainsAny(new string[] { m.identifier }));
                     if (options.proceed_with_inconsistencies)
                     {
                         conflicts.Add(new KeyValuePair<CkanModule, CkanModule>(module, reason.Parent));
@@ -688,7 +686,7 @@ public override string Reason
                 get { return "  Requested by user.\r\n"; }
             }
         }
-        
+
         public class NoLongerUsed: SelectionReason
         {
             public override string Reason

Core/Relationships/SanityChecker.cs

@@ -1,4 +1,3 @@
-using System;
 using System.Collections.Generic;
 using System.Linq;
 using CKAN.Extensions;

GUI/Main/MainInstall.cs

@@ -177,7 +177,7 @@ out Dictionary<CkanModule, HashSet<string>> supporters
                         processSuccessful = false;
                         if (!installCanceled)
                         {
-                            installer.UninstallList(toUninstall, ref possibleConfigOnlyDirs, registry_manager, false, toInstall.Select(m => m.identifier));
+                            installer.UninstallList(toUninstall, ref possibleConfigOnlyDirs, registry_manager, false, toInstall);
                             processSuccessful = true;
                         }
                     }

GUI/Main/MainModList.cs

@@ -848,19 +848,21 @@ public IEnumerable<ModChange> ComputeChangeSetFromModList(
 
             var installed_modules =
                 registry.InstalledModules.Select(imod => imod.Module).ToDictionary(mod => mod.identifier, mod => mod);
-            foreach (var dependency in registry.FindReverseDependencies(
+
+            foreach (var dependent in registry.FindReverseDependencies(
                 modules_to_remove
                     .Select(mod => mod.identifier)
-                    .Except(modules_to_install.Select(m => m.identifier))
+                    .Except(modules_to_install.Select(m => m.identifier)),
+                modules_to_install
             ))
             {
                 //TODO This would be a good place to have an event that alters the row's graphics to show it will be removed
                 CkanModule depMod;
-                if (installed_modules.TryGetValue(dependency, out depMod))
+                if (installed_modules.TryGetValue(dependent, out depMod))
                 {
                     CkanModule module_by_version = registry.GetModuleByVersion(depMod.identifier,
                     depMod.version)
-                        ?? registry.InstalledModule(dependency).Module;
+                        ?? registry.InstalledModule(dependent).Module;
                     changeSet.Add(new ModChange(module_by_version, GUIModChangeType.Remove, null));
                     modules_to_remove.Add(module_by_version);
                 }

Tests/Core/ModuleInstaller.cs

@@ -440,7 +440,7 @@ public void UninstallModNotFound()
                 {
                     HashSet<string> possibleConfigOnlyDirs = null;
                     // This should throw, as our tidy KSP has no mods installed.
-                    CKAN.ModuleInstaller.GetInstance(manager.CurrentInstance, manager.Cache, nullUser).UninstallList("Foo", ref possibleConfigOnlyDirs, CKAN.RegistryManager.Instance(manager.CurrentInstance));
+                    CKAN.ModuleInstaller.GetInstance(manager.CurrentInstance, manager.Cache, nullUser).UninstallList(new List<string> {"Foo"}, ref possibleConfigOnlyDirs, CKAN.RegistryManager.Instance(manager.CurrentInstance));
                 });
 
                 manager.CurrentInstance = null; // I weep even more.
@@ -520,10 +520,10 @@ public void InstallList_IdentifierEqualsVersionSyntax_InstallsModule()
                 {
                     $"{mod.identifier}={mod.version}"
                 };
-                
+
                 // Act
                 inst.InstallList(modules, new RelationshipResolverOptions(), CKAN.RegistryManager.Instance(manager.CurrentInstance));
-                
+
                 // Assert
                 Assert.IsTrue(File.Exists(mod_file_path));
 

Tests/Core/Relationships/SanityChecker.cs

@@ -340,7 +340,7 @@ private static void TestDepends(
             var dll_count = dlls.Count;
             var mods_count = mods.Count;
 
-            var results = CKAN.Registry.FindReverseDependencies(to_remove, mods, dlls, dlc);
+            var results = CKAN.Registry.FindReverseDependencies(to_remove, null, mods, dlls, dlc);
 
             // Make sure nothing changed.
             Assert.AreEqual(remove_count, to_remove.Count, message + " remove count");