From fed0e15cea3f331722ece7b1a0f805eef16a5ad9 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 23 Aug 2021 20:23:46 +0900 Subject: [PATCH 1/4] Fix typo in `ArchiveModelManager` --- osu.Game/Database/ArchiveModelManager.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Database/ArchiveModelManager.cs b/osu.Game/Database/ArchiveModelManager.cs index 87bf54f981..ddd2bc5d1e 100644 --- a/osu.Game/Database/ArchiveModelManager.cs +++ b/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); /// - /// 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 is not overridden. /// /// The existing model. From 7b3f7cc7c1e4534afdd96cf2c97499b418e4a1c4 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 23 Aug 2021 20:24:00 +0900 Subject: [PATCH 2/4] Change skin import to also include directory names in the skin name where appropriate --- osu.Game/Skinning/SkinManager.cs | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/osu.Game/Skinning/SkinManager.cs b/osu.Game/Skinning/SkinManager.cs index 0f805990b9..0aa1c62d04 100644 --- a/osu.Game/Skinning/SkinManager.cs +++ b/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 - if (item.Name?.Contains(".osk", StringComparison.OrdinalIgnoreCase) == true) - populateMetadata(item, GetSkin(item)); + var instance = 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. - // likely, the skin should expose a real version (ie. the version of the skin, not the skin.ini version it's targeting). + // we need to populate early to create a hash based off skin.ini contents + 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); + populateMetadata(model, instance, archive?.Name); 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})"; } /// From 0cbe95d6611c0a6921c6be892f9db02af3735bc4 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 23 Aug 2021 20:25:46 +0900 Subject: [PATCH 3/4] Add test coverage of different folder names with same skin.ini --- osu.Game.Tests/Skins/IO/ImportSkinTest.cs | 36 +++++++++++++++++++---- 1 file changed, 31 insertions(+), 5 deletions(-) diff --git a/osu.Game.Tests/Skins/IO/ImportSkinTest.cs b/osu.Game.Tests/Skins/IO/ImportSkinTest.cs index bab8dfc983..0329ca11e2 100644 --- a/osu.Game.Tests/Skins/IO/ImportSkinTest.cs +++ b/osu.Game.Tests/Skins/IO/ImportSkinTest.cs @@ -138,16 +138,38 @@ 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")); + + var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("name 1", "author 1", false), "my custom skin 2")); + + 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 +177,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(); From a2484692b3a29cc71c383e3f8b43f250d43c6393 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 23 Aug 2021 20:37:19 +0900 Subject: [PATCH 4/4] Change brackets to square --- osu.Game.Tests/Skins/IO/ImportSkinTest.cs | 4 ++++ osu.Game/Skinning/SkinManager.cs | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/osu.Game.Tests/Skins/IO/ImportSkinTest.cs b/osu.Game.Tests/Skins/IO/ImportSkinTest.cs index 0329ca11e2..7a9fc20426 100644 --- a/osu.Game.Tests/Skins/IO/ImportSkinTest.cs +++ b/osu.Game.Tests/Skins/IO/ImportSkinTest.cs @@ -148,8 +148,12 @@ namespace osu.Game.Tests.Skins.IO 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)); } diff --git a/osu.Game/Skinning/SkinManager.cs b/osu.Game/Skinning/SkinManager.cs index 0aa1c62d04..edeb17cbad 100644 --- a/osu.Game/Skinning/SkinManager.cs +++ b/osu.Game/Skinning/SkinManager.cs @@ -181,7 +181,7 @@ namespace osu.Game.Skinning bool isArchiveImport = archiveName?.Contains(".osk", StringComparison.OrdinalIgnoreCase) == true; if (archiveName != null && !isArchiveImport && archiveName != item.Name) - item.Name = $"{item.Name} ({archiveName})"; + item.Name = $"{item.Name} [{archiveName}]"; } ///