Merge pull request #14465 from peppy/improve-stable-skin-import

Improve skin stable import behaviour to better handle similar skins

osu.Game.Tests/Skins/IO/ImportSkinTest.cs

@@ -138,16 +138,42 @@ namespace osu.Game.Tests.Skins.IO
             }
         }
 
-        private MemoryStream createOsk(string name, string author)
+        [Test]
+        public async Task TestSameMetadataNameDifferentFolderName()
+        {
+            using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(ImportSkinTest)))
+            {
+                try
+                {
+                    var osu = LoadOsuIntoHost(host);
+
+                    var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("name 1", "author 1", false), "my custom skin 1"));
+                    Assert.That(imported.Name, Is.EqualTo("name 1 [my custom skin 1]"));
+                    Assert.That(imported.Creator, Is.EqualTo("author 1"));
+
+                    var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("name 1", "author 1", false), "my custom skin 2"));
+                    Assert.That(imported2.Name, Is.EqualTo("name 1 [my custom skin 2]"));
+                    Assert.That(imported2.Creator, Is.EqualTo("author 1"));
+
+                    Assert.That(imported2.Hash, Is.Not.EqualTo(imported.Hash));
+                }
+                finally
+                {
+                    host.Exit();
+                }
+            }
+        }
+
+        private MemoryStream createOsk(string name, string author, bool makeUnique = true)
         {
             var zipStream = new MemoryStream();
             using var zip = ZipArchive.Create();
-            zip.AddEntry("skin.ini", generateSkinIni(name, author));
+            zip.AddEntry("skin.ini", generateSkinIni(name, author, makeUnique));
             zip.SaveTo(zipStream);
             return zipStream;
         }
 
-        private MemoryStream generateSkinIni(string name, string author)
+        private MemoryStream generateSkinIni(string name, string author, bool makeUnique = true)
         {
             var stream = new MemoryStream();
             var writer = new StreamWriter(stream);
@@ -155,8 +181,12 @@ namespace osu.Game.Tests.Skins.IO
             writer.WriteLine("[General]");
             writer.WriteLine($"Name: {name}");
             writer.WriteLine($"Author: {author}");
-            writer.WriteLine();
+
-            writer.WriteLine($"# unique {Guid.NewGuid()}");
+            if (makeUnique)
+            {
+                writer.WriteLine();
+                writer.WriteLine($"# unique {Guid.NewGuid()}");
+            }
 
             writer.Flush();
 

osu.Game/Database/ArchiveModelManager.cs

@@ -806,7 +806,7 @@ namespace osu.Game.Database
         protected TModel CheckForExisting(TModel model) => model.Hash == null ? null : ModelStore.ConsumableItems.FirstOrDefault(b => b.Hash == model.Hash);
 
         /// <summary>
-        /// Whether inport can be skipped after finding an existing import early in the process.
+        /// Whether import can be skipped after finding an existing import early in the process.
         /// Only valid when <see cref="ComputeHash"/> is not overridden.
         /// </summary>
         /// <param name="existing">The existing model.</param>

osu.Game/Skinning/SkinManager.cs

@@ -136,18 +136,19 @@ namespace osu.Game.Skinning
 
         protected override string ComputeHash(SkinInfo item, ArchiveReader reader = null)
         {
-            // we need to populate early to create a hash based off skin.ini contents
+            var instance = GetSkin(item);
-            if (item.Name?.Contains(".osk", StringComparison.OrdinalIgnoreCase) == true)
-                populateMetadata(item, GetSkin(item));
 
-            if (item.Creator != null && item.Creator != unknown_creator_string)
+            // in the case the skin has a skin.ini file, we are going to create a hash based on that.
+            // we don't want to do this in the case we don't have a skin.ini, as it would match only on the filename portion,
+            // causing potentially unique skin imports to be considered as a duplicate.
+            if (!string.IsNullOrEmpty(instance.Configuration.SkinInfo.Name))
             {
-                // this is the optimal way to hash legacy skins, but will need to be reconsidered when we move forward with skin implementation.
+                // we need to populate early to create a hash based off skin.ini contents
-                // likely, the skin should expose a real version (ie. the version of the skin, not the skin.ini version it's targeting).
+                populateMetadata(item, instance, reader?.Name);
+
                 return item.ToString().ComputeSHA2Hash();
             }
 
-            // if there was no creator, the ToString above would give the filename, which alone isn't really enough to base any decisions on.
             return base.ComputeHash(item, reader);
         }
 
@@ -157,13 +158,12 @@ namespace osu.Game.Skinning
 
             model.InstantiationInfo ??= instance.GetType().GetInvariantInstantiationInfo();
 
-            if (model.Name?.Contains(".osk", StringComparison.OrdinalIgnoreCase) == true)
+            populateMetadata(model, instance, archive?.Name);
-                populateMetadata(model, instance);
 
             return Task.CompletedTask;
         }
 
-        private void populateMetadata(SkinInfo item, Skin instance)
+        private void populateMetadata(SkinInfo item, Skin instance, string archiveName)
         {
             if (!string.IsNullOrEmpty(instance.Configuration.SkinInfo.Name))
             {
@@ -175,6 +175,13 @@ namespace osu.Game.Skinning
                 item.Name = item.Name.Replace(".osk", "", StringComparison.OrdinalIgnoreCase);
                 item.Creator ??= unknown_creator_string;
             }
+
+            // generally when importing from a folder, the ".osk" extension will not be present.
+            // if we ever need a more reliable method of determining this, the type of `ArchiveReader` can be checked.
+            bool isArchiveImport = archiveName?.Contains(".osk", StringComparison.OrdinalIgnoreCase) == true;
+
+            if (archiveName != null && !isArchiveImport && archiveName != item.Name)
+                item.Name = $"{item.Name} [{archiveName}]";
         }
 
         /// <summary>