From 789c715f136254e68cdccce5d986bbfd9a05e1d6 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 20 Oct 2021 15:22:47 +0900 Subject: [PATCH 01/20] Add `skin.ini` write support to allow for more correct hashing --- osu.Game.Tests/Skins/IO/ImportSkinTest.cs | 33 +++++- osu.Game/Database/ArchiveModelManager.cs | 43 +++++-- osu.Game/Skinning/SkinManager.cs | 132 +++++++++++++++++----- 3 files changed, 162 insertions(+), 46 deletions(-) diff --git a/osu.Game.Tests/Skins/IO/ImportSkinTest.cs b/osu.Game.Tests/Skins/IO/ImportSkinTest.cs index b2600bb887..67c94413ca 100644 --- a/osu.Game.Tests/Skins/IO/ImportSkinTest.cs +++ b/osu.Game.Tests/Skins/IO/ImportSkinTest.cs @@ -38,7 +38,32 @@ namespace osu.Game.Tests.Skins.IO } [Test] - public async Task TestImportTwiceWithSameMetadata() + public async Task TestImportTwiceWithSameMetadataAndFilename() + { + using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(ImportSkinTest))) + { + try + { + var osu = LoadOsuIntoHost(host); + + var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("test skin", "skinner"), "skin.osk")); + var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("test skin", "skinner"), "skin.osk")); + + Assert.That(imported2.ID, Is.EqualTo(imported.ID)); + Assert.That(osu.Dependencies.Get().GetAllUserSkins(true).Count, Is.EqualTo(1)); + + // the first should be overwritten by the second import. + Assert.That(imported.Files.First().FileInfoID, Is.EqualTo(imported2.Files.First().FileInfoID)); + } + finally + { + host.Exit(); + } + } + } + + [Test] + public async Task TestImportTwiceWithSameMetadataButDifferentFilename() { using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(ImportSkinTest))) { @@ -50,10 +75,10 @@ namespace osu.Game.Tests.Skins.IO var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("test skin", "skinner"), "skin2.osk")); Assert.That(imported2.ID, Is.Not.EqualTo(imported.ID)); - Assert.That(osu.Dependencies.Get().GetAllUserSkins(true).Count, Is.EqualTo(1)); + Assert.That(osu.Dependencies.Get().GetAllUserSkins(true).Count, Is.EqualTo(2)); - // the first should be overwritten by the second import. - Assert.That(osu.Dependencies.Get().GetAllUserSkins(true).First().Files.First().FileInfoID, Is.EqualTo(imported2.Files.First().FileInfoID)); + // skin.ini will be rewritten and therefore not match. + Assert.That(imported.Files.First().FileInfoID, Is.Not.EqualTo(imported2.Files.First().FileInfoID)); } finally { diff --git a/osu.Game/Database/ArchiveModelManager.cs b/osu.Game/Database/ArchiveModelManager.cs index 9c777d324b..e52e5bdfe7 100644 --- a/osu.Game/Database/ArchiveModelManager.cs +++ b/osu.Game/Database/ArchiveModelManager.cs @@ -507,18 +507,26 @@ namespace osu.Game.Database /// The existing file to be deleted. public void DeleteFile(TModel model, TFileModel file) { - using (var usage = ContextFactory.GetForWrite()) + if (model.ID > 0) { - // Dereference the existing file info, since the file model will be removed. - if (file.FileInfo != null) + using (var usage = ContextFactory.GetForWrite()) { - Files.Dereference(file.FileInfo); + // Dereference the existing file info, since the file model will be removed. + if (file.FileInfo != null) + { + Files.Dereference(file.FileInfo); - // This shouldn't be required, but here for safety in case the provided TModel is not being change tracked - // Definitely can be removed once we rework the database backend. - usage.Context.Set().Remove(file); + // This shouldn't be required, but here for safety in case the provided TModel is not being change tracked + // Definitely can be removed once we rework the database backend. + usage.Context.Set().Remove(file); + } + + model.Files.Remove(file); } - + } + else + { + Files.Dereference(file.FileInfo); model.Files.Remove(file); } } @@ -531,15 +539,28 @@ namespace osu.Game.Database /// The filename for the new file. public void AddFile(TModel model, Stream contents, string filename) { - using (ContextFactory.GetForWrite()) + if (model.ID > 0) { + using (ContextFactory.GetForWrite()) + { + model.Files.Add(new TFileModel + { + Filename = filename, + FileInfo = Files.Add(contents) + }); + + Update(model); + } + } + else + { + // This function may be called during the import process. + // Should not exist like this once we have switched to realm. model.Files.Add(new TFileModel { Filename = filename, FileInfo = Files.Add(contents) }); - - Update(model); } } diff --git a/osu.Game/Skinning/SkinManager.cs b/osu.Game/Skinning/SkinManager.cs index 3842acab74..724ca3bc62 100644 --- a/osu.Game/Skinning/SkinManager.cs +++ b/osu.Game/Skinning/SkinManager.cs @@ -14,7 +14,6 @@ using Newtonsoft.Json; using osu.Framework.Audio; using osu.Framework.Audio.Sample; using osu.Framework.Bindables; -using osu.Framework.Extensions; using osu.Framework.Graphics; using osu.Framework.Graphics.OpenGL.Textures; using osu.Framework.Graphics.Textures; @@ -138,52 +137,123 @@ namespace osu.Game.Skinning { var instance = GetSkin(item); - // 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)) - { - // we need to populate early to create a hash based off skin.ini contents - populateMetadata(item, instance, reader?.Name); + // This function can be run on fresh import or save. The logic here ensures a skin.ini file is in a good state for both operations. - return item.ToString().ComputeSHA2Hash(); + // LegacySkin will parse the skin.ini and populate `Skin.Configuration` during construction above. + bool hasSkinIni = string.IsNullOrEmpty(instance.Configuration.SkinInfo.Name); + + if (hasSkinIni) + { + item.Name = instance.Configuration.SkinInfo.Name; + item.Creator = instance.Configuration.SkinInfo.Creator; } + item.Creator ??= unknown_creator_string; + + bool isImport = reader != null; + + if (isImport) + { + // For imports, we want to use the archive or folder name as part of the metadata, in addition to any existing skin.ini metadata. + // In an ideal world, skin.ini would be the only source of metadata, but a lot of skin creators and users don't update it when making modifications. + // In both of these cases, the expectation from the user is that the filename or folder name is displayed somewhere to identify the skin. + + string archiveName = item.Name.Replace(".osk", "", StringComparison.OrdinalIgnoreCase); + + if (archiveName != item.Name) + item.Name = $"{item.Name} [{archiveName}]"; + } + + // By this point, the metadata in SkinInfo will be correct. + // Regardless of whether this is an import or not, let's write the skin.ini if non-existing or non-matching. + // This is (weirdly) done inside ComputeHash to avoid adding a new method to handle this case. After switching to realm it can be moved into another place. + if (instance.Configuration.SkinInfo.Name != item.Name) + updateSkinIniMetadata(item, hasSkinIni); + return base.ComputeHash(item, reader); } + private void updateSkinIniMetadata(SkinInfo item, bool hasSkinIni) + { + string nameLine = $"Name: {item.Name}"; + string authorLine = $"Author: {item.Name}"; + + if (hasSkinIni) + { + List outputLines = new List(); + + bool addedName = false; + bool addedAuthor = false; + + var existingFile = item.Files.First(f => f.Filename == "skin.ini"); + + using (var stream = Files.Storage.GetStream(existingFile.FileInfo.StoragePath)) + using (var sr = new StreamReader(stream)) + { + string line; + + while ((line = sr.ReadLine()) != null) + { + if (line.StartsWith("Name:", StringComparison.Ordinal)) + { + outputLines.Add(nameLine); + addedName = true; + } + else if (line.StartsWith("Author:", StringComparison.Ordinal)) + { + outputLines.Add(authorLine); + addedAuthor = true; + } + else + outputLines.Add(line); + } + } + + if (!addedName || !addedAuthor) + { + outputLines.AddRange(new[] + { + "[General]", + nameLine, + authorLine, + }); + } + + using (Stream stream = new MemoryStream()) + using (var sw = new StreamWriter(stream, Encoding.UTF8, 1024, true)) + { + foreach (string line in outputLines) + sw.WriteLine(line); + + ReplaceFile(item, existingFile, stream); + } + } + else + { + using (Stream stream = new MemoryStream()) + using (var sw = new StreamWriter(stream, Encoding.UTF8, 1024, true)) + { + sw.WriteLine("[General]"); + sw.WriteLine(nameLine); + sw.WriteLine(authorLine); + + AddFile(item, stream, "skin.ini"); + } + } + } + protected override Task Populate(SkinInfo model, ArchiveReader archive, CancellationToken cancellationToken = default) { var instance = GetSkin(model); model.InstantiationInfo ??= instance.GetType().GetInvariantInstantiationInfo(); - populateMetadata(model, instance, archive?.Name); + model.Name = instance.Configuration.SkinInfo.Name; + model.Creator = instance.Configuration.SkinInfo.Creator; return Task.CompletedTask; } - private void populateMetadata(SkinInfo item, Skin instance, string archiveName) - { - if (!string.IsNullOrEmpty(instance.Configuration.SkinInfo.Name)) - { - item.Name = instance.Configuration.SkinInfo.Name; - item.Creator = instance.Configuration.SkinInfo.Creator; - } - else - { - 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}]"; - } - /// /// Retrieve a instance for the provided /// From fa542f254778747c2dfb8e8c0d17fe5f2e995de9 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 20 Oct 2021 14:05:32 +0900 Subject: [PATCH 02/20] Include `json` files in skin hashing This covers the new layout storage we are doing. --- osu.Game/Skinning/SkinManager.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game/Skinning/SkinManager.cs b/osu.Game/Skinning/SkinManager.cs index 724ca3bc62..9c7ea28cc4 100644 --- a/osu.Game/Skinning/SkinManager.cs +++ b/osu.Game/Skinning/SkinManager.cs @@ -50,7 +50,7 @@ namespace osu.Game.Skinning public override IEnumerable HandledExtensions => new[] { ".osk" }; - protected override string[] HashableFileTypes => new[] { ".ini" }; + protected override string[] HashableFileTypes => new[] { ".ini", ".json" }; protected override string ImportFromStablePath => "Skins"; @@ -140,7 +140,7 @@ namespace osu.Game.Skinning // This function can be run on fresh import or save. The logic here ensures a skin.ini file is in a good state for both operations. // LegacySkin will parse the skin.ini and populate `Skin.Configuration` during construction above. - bool hasSkinIni = string.IsNullOrEmpty(instance.Configuration.SkinInfo.Name); + bool hasSkinIni = !string.IsNullOrEmpty(instance.Configuration.SkinInfo.Name); if (hasSkinIni) { From 439e90fce3d3c804fceb80e928af0ed2f87836bb Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 20 Oct 2021 15:51:14 +0900 Subject: [PATCH 03/20] Disallow archive imports with no hashable files --- osu.Game/Database/ArchiveModelManager.cs | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/osu.Game/Database/ArchiveModelManager.cs b/osu.Game/Database/ArchiveModelManager.cs index e52e5bdfe7..5b07bfe9a5 100644 --- a/osu.Game/Database/ArchiveModelManager.cs +++ b/osu.Game/Database/ArchiveModelManager.cs @@ -317,14 +317,18 @@ namespace osu.Game.Database /// protected virtual string ComputeHash(TModel item, ArchiveReader reader = null) { - if (reader != null) - // fast hashing for cases where the item's files may not be populated. - return computeHashFast(reader); + var hashableFiles = item.Files + .Where(f => HashableFileTypes.Any(ext => f.Filename.EndsWith(ext, StringComparison.OrdinalIgnoreCase))) + .OrderBy(f => f.Filename) + .ToArray(); + + if (hashableFiles.Length == 0) + throw new InvalidOperationException("Attempted to hash an archive with no files"); // for now, concatenate all hashable files in the set to create a unique hash. MemoryStream hashable = new MemoryStream(); - foreach (TFileModel file in item.Files.Where(f => HashableFileTypes.Any(ext => f.Filename.EndsWith(ext, StringComparison.OrdinalIgnoreCase))).OrderBy(f => f.Filename)) + foreach (TFileModel file in hashableFiles) { using (Stream s = Files.Store.GetStream(file.FileInfo.StoragePath)) s.CopyTo(hashable); @@ -700,10 +704,10 @@ namespace osu.Game.Database s.CopyTo(hashable); } - if (hashable.Length > 0) - return hashable.ComputeSHA2Hash(); + if (hashable.Length == 0) + throw new InvalidOperationException("Attempted to hash an archive with no files"); - return reader.Name.ComputeSHA2Hash(); + return hashable.ComputeSHA2Hash(); } /// From 19f30177eacb10e733216d3534ec997b1dc9a5a8 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 20 Oct 2021 16:48:32 +0900 Subject: [PATCH 04/20] Rewrite tests completely --- osu.Game.Tests/Skins/IO/ImportSkinTest.cs | 249 ++++++++++------------ 1 file changed, 108 insertions(+), 141 deletions(-) diff --git a/osu.Game.Tests/Skins/IO/ImportSkinTest.cs b/osu.Game.Tests/Skins/IO/ImportSkinTest.cs index 67c94413ca..8449b0bbd3 100644 --- a/osu.Game.Tests/Skins/IO/ImportSkinTest.cs +++ b/osu.Game.Tests/Skins/IO/ImportSkinTest.cs @@ -4,6 +4,7 @@ using System; using System.IO; using System.Linq; +using System.Runtime.CompilerServices; using System.Threading.Tasks; using NUnit.Framework; using osu.Framework.Allocation; @@ -16,180 +17,130 @@ namespace osu.Game.Tests.Skins.IO { public class ImportSkinTest : ImportTest { - [Test] - public async Task TestBasicImport() - { - using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(ImportSkinTest))) - { - try - { - var osu = LoadOsuIntoHost(host); - - var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("test skin", "skinner"), "skin.osk")); - - Assert.That(imported.Name, Is.EqualTo("test skin")); - Assert.That(imported.Creator, Is.EqualTo("skinner")); - } - finally - { - host.Exit(); - } - } - } + #region Testing filename metadata inclusion [Test] - public async Task TestImportTwiceWithSameMetadataAndFilename() + public Task TestSingleImportDifferentFilename() => runSkinTest(async osu => { - using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(ImportSkinTest))) - { - try - { - var osu = LoadOsuIntoHost(host); + var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("test skin", "skinner"), "skin.osk")); - var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("test skin", "skinner"), "skin.osk")); - var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("test skin", "skinner"), "skin.osk")); - - Assert.That(imported2.ID, Is.EqualTo(imported.ID)); - Assert.That(osu.Dependencies.Get().GetAllUserSkins(true).Count, Is.EqualTo(1)); - - // the first should be overwritten by the second import. - Assert.That(imported.Files.First().FileInfoID, Is.EqualTo(imported2.Files.First().FileInfoID)); - } - finally - { - host.Exit(); - } - } - } + // When the import filename doesn't match, it should be appended (and update the skin.ini). + Assert.That(imported.Name, Is.EqualTo("test skin [skin]")); + Assert.That(imported.Creator, Is.EqualTo("skinner")); + }); [Test] - public async Task TestImportTwiceWithSameMetadataButDifferentFilename() + public Task TestSingleImportMatchingFilename() => runSkinTest(async osu => { - using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(ImportSkinTest))) - { - try - { - var osu = LoadOsuIntoHost(host); + var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("test skin", "skinner"), "test skin.osk")); - var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("test skin", "skinner"), "skin.osk")); - var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("test skin", "skinner"), "skin2.osk")); + // When the import filename matches it shouldn't be appended. + Assert.That(imported.Name, Is.EqualTo("test skin")); + Assert.That(imported.Creator, Is.EqualTo("skinner")); + }); - Assert.That(imported2.ID, Is.Not.EqualTo(imported.ID)); - Assert.That(osu.Dependencies.Get().GetAllUserSkins(true).Count, Is.EqualTo(2)); + #endregion - // skin.ini will be rewritten and therefore not match. - Assert.That(imported.Files.First().FileInfoID, Is.Not.EqualTo(imported2.Files.First().FileInfoID)); - } - finally - { - host.Exit(); - } - } - } + #region Cases where imports should match existing [Test] - public async Task TestImportTwiceWithNoMetadata() + public Task TestImportTwiceWithSameMetadataAndFilename() => runSkinTest(async osu => { - using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(ImportSkinTest))) - { - try - { - var osu = LoadOsuIntoHost(host); + var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("test skin", "skinner"), "skin.osk")); + var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("test skin", "skinner"), "skin.osk")); - // if a user downloads two skins that do have skin.ini files but don't have any creator metadata in the skin.ini, they should both import separately just for safety. - var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk(string.Empty, string.Empty), "download.osk")); - var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk(string.Empty, string.Empty), "download.osk")); - - Assert.That(imported2.ID, Is.Not.EqualTo(imported.ID)); - Assert.That(osu.Dependencies.Get().GetAllUserSkins(true).Count, Is.EqualTo(2)); - - Assert.That(osu.Dependencies.Get().GetAllUserSkins(true).First().Files.First().FileInfoID, Is.EqualTo(imported.Files.First().FileInfoID)); - Assert.That(osu.Dependencies.Get().GetAllUserSkins(true).Last().Files.First().FileInfoID, Is.EqualTo(imported2.Files.First().FileInfoID)); - } - finally - { - host.Exit(); - } - } - } + assertImportedOnce(imported, imported2); + }); [Test] - public async Task TestImportTwiceWithDifferentMetadata() + public Task TestImportTwiceWithNoMetadataSameDownloadFilename() => runSkinTest(async osu => { - using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(ImportSkinTest))) - { - try - { - var osu = LoadOsuIntoHost(host); + // if a user downloads two skins that do have skin.ini files but don't have any creator metadata in the skin.ini, they should both import separately just for safety. + var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk(string.Empty, string.Empty), "download.osk")); + var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk(string.Empty, string.Empty), "download.osk")); - var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("test skin v2", "skinner"), "skin.osk")); - var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("test skin v2.1", "skinner"), "skin2.osk")); - - Assert.That(imported2.ID, Is.Not.EqualTo(imported.ID)); - Assert.That(osu.Dependencies.Get().GetAllUserSkins(true).Count, Is.EqualTo(2)); - - Assert.That(osu.Dependencies.Get().GetAllUserSkins(true).First().Files.First().FileInfoID, Is.EqualTo(imported.Files.First().FileInfoID)); - Assert.That(osu.Dependencies.Get().GetAllUserSkins(true).Last().Files.First().FileInfoID, Is.EqualTo(imported2.Files.First().FileInfoID)); - } - finally - { - host.Exit(); - } - } - } + assertImportedOnce(imported, imported2); + }); [Test] - public async Task TestImportUpperCasedOskArchive() + public Task TestImportUpperCasedOskArchive() => runSkinTest(async osu => { - using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(ImportSkinTest))) - { - try - { - var osu = LoadOsuIntoHost(host); + var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("name 1", "author 1"), "name 1.OsK")); + assertCorrectMetadata(imported, "name 1", "author 1"); - var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("name 1", "author 1"), "skin1.OsK")); + var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("name 1", "author 1"), "name 1.oSK")); - Assert.That(imported.Name, Is.EqualTo("name 1")); - Assert.That(imported.Creator, Is.EqualTo("author 1")); + assertImportedOnce(imported, imported2); + }); - var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("name 1", "author 1"), "skin1.oSK")); + #endregion - Assert.That(imported2.Hash, Is.EqualTo(imported.Hash)); - } - finally - { - host.Exit(); - } - } - } + #region Cases where imports should be uniquely imported [Test] - public async Task TestSameMetadataNameDifferentFolderName() + public Task TestImportTwiceWithSameMetadataButDifferentFilename() => runSkinTest(async osu => { - using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(ImportSkinTest))) - { - try - { - var osu = LoadOsuIntoHost(host); + var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("test skin", "skinner"), "skin.osk")); + var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("test skin", "skinner"), "skin2.osk")); - 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")); + assertImportedBoth(imported, imported2); + }); - 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")); + [Test] + public Task TestImportTwiceWithNoMetadataDifferentDownloadFilename() => runSkinTest(async osu => + { + // if a user downloads two skins that do have skin.ini files but don't have any creator metadata in the skin.ini, they should both import separately just for safety. + var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk(string.Empty, string.Empty), "download.osk")); + var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk(string.Empty, string.Empty), "download2.osk")); - Assert.That(imported2.Hash, Is.Not.EqualTo(imported.Hash)); - } - finally - { - host.Exit(); - } - } + assertImportedBoth(imported, imported2); + }); + + [Test] + public Task TestImportTwiceWithSameFilenameDifferentMetadata() => runSkinTest(async osu => + { + var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("test skin v2", "skinner"), "skin.osk")); + var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("test skin v2.1", "skinner"), "skin.osk")); + + assertImportedBoth(imported, imported2); + assertCorrectMetadata(imported, "test skin v2 [skin]", "skinner"); + assertCorrectMetadata(imported2, "test skin v2.1 [skin]", "skinner"); + }); + + [Test] + public Task TestSameMetadataNameDifferentFolderName() => runSkinTest(async osu => + { + var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("name 1", "author 1"), "my custom skin 1")); + var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("name 1", "author 1"), "my custom skin 2")); + + assertImportedBoth(imported, imported2); + assertCorrectMetadata(imported, "name 1 [my custom skin 2]", "author 1"); + assertCorrectMetadata(imported, "name 1 [my custom skin 2]", "author 1"); + }); + + #endregion + + private void assertCorrectMetadata(SkinInfo imported, string name, string creator) + { + Assert.That(imported.Name, Is.EqualTo(name)); + Assert.That(imported.Creator, Is.EqualTo(creator)); } - private MemoryStream createOsk(string name, string author, bool makeUnique = true) + private void assertImportedBoth(SkinInfo imported, SkinInfo imported2) + { + Assert.That(imported2.ID, Is.Not.EqualTo(imported.ID)); + Assert.That(imported2.Hash, Is.Not.EqualTo(imported.Hash)); + Assert.That(imported2.Files.Select(f => f.FileInfoID), Is.Not.EquivalentTo(imported.Files.Select(f => f.FileInfoID))); + } + + private void assertImportedOnce(SkinInfo imported, SkinInfo imported2) + { + Assert.That(imported2.ID, Is.EqualTo(imported.ID)); + Assert.That(imported2.Hash, Is.EqualTo(imported.Hash)); + Assert.That(imported2.Files.Select(f => f.FileInfoID), Is.EquivalentTo(imported.Files.Select(f => f.FileInfoID))); + } + + private MemoryStream createOsk(string name, string author, bool makeUnique = false) { var zipStream = new MemoryStream(); using var zip = ZipArchive.Create(); @@ -218,6 +169,22 @@ namespace osu.Game.Tests.Skins.IO return stream; } + private async Task runSkinTest(Func action, [CallerMemberName] string callingMethodName = @"") + { + using (HeadlessGameHost host = new CleanRunHeadlessGameHost(callingMethodName)) + { + try + { + var osu = LoadOsuIntoHost(host); + await action(osu); + } + finally + { + host.Exit(); + } + } + } + private async Task loadSkinIntoOsu(OsuGameBase osu, ArchiveReader archive = null) { var skinManager = osu.Dependencies.Get(); From a95c754fd3d0e867d7910fe3f2a746a10261329b Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 20 Oct 2021 16:49:58 +0900 Subject: [PATCH 05/20] Fix multiple issues and make metadata fallback process more logical --- osu.Game/Skinning/SkinManager.cs | 49 +++++++++++++++----------------- 1 file changed, 23 insertions(+), 26 deletions(-) diff --git a/osu.Game/Skinning/SkinManager.cs b/osu.Game/Skinning/SkinManager.cs index 9c7ea28cc4..452c081a41 100644 --- a/osu.Game/Skinning/SkinManager.cs +++ b/osu.Game/Skinning/SkinManager.cs @@ -140,26 +140,19 @@ namespace osu.Game.Skinning // This function can be run on fresh import or save. The logic here ensures a skin.ini file is in a good state for both operations. // LegacySkin will parse the skin.ini and populate `Skin.Configuration` during construction above. - bool hasSkinIni = !string.IsNullOrEmpty(instance.Configuration.SkinInfo.Name); - - if (hasSkinIni) - { - item.Name = instance.Configuration.SkinInfo.Name; - item.Creator = instance.Configuration.SkinInfo.Creator; - } - - item.Creator ??= unknown_creator_string; + string skinIniSourcedName = instance.Configuration.SkinInfo.Name; + string archiveName = item.Name.Replace(".osk", "", StringComparison.OrdinalIgnoreCase); bool isImport = reader != null; if (isImport) { + item.Name = !string.IsNullOrEmpty(skinIniSourcedName) ? skinIniSourcedName : archiveName; + item.Creator = instance.Configuration.SkinInfo.Creator ?? unknown_creator_string; + // For imports, we want to use the archive or folder name as part of the metadata, in addition to any existing skin.ini metadata. // In an ideal world, skin.ini would be the only source of metadata, but a lot of skin creators and users don't update it when making modifications. // In both of these cases, the expectation from the user is that the filename or folder name is displayed somewhere to identify the skin. - - string archiveName = item.Name.Replace(".osk", "", StringComparison.OrdinalIgnoreCase); - if (archiveName != item.Name) item.Name = $"{item.Name} [{archiveName}]"; } @@ -167,26 +160,26 @@ namespace osu.Game.Skinning // By this point, the metadata in SkinInfo will be correct. // Regardless of whether this is an import or not, let's write the skin.ini if non-existing or non-matching. // This is (weirdly) done inside ComputeHash to avoid adding a new method to handle this case. After switching to realm it can be moved into another place. - if (instance.Configuration.SkinInfo.Name != item.Name) - updateSkinIniMetadata(item, hasSkinIni); + if (skinIniSourcedName != item.Name) + updateSkinIniMetadata(item); return base.ComputeHash(item, reader); } - private void updateSkinIniMetadata(SkinInfo item, bool hasSkinIni) + private void updateSkinIniMetadata(SkinInfo item) { string nameLine = $"Name: {item.Name}"; - string authorLine = $"Author: {item.Name}"; + string authorLine = $"Author: {item.Creator}"; - if (hasSkinIni) + var existingFile = item.Files.SingleOrDefault(f => f.Filename == "skin.ini"); + + if (existingFile != null) { List outputLines = new List(); bool addedName = false; bool addedAuthor = false; - var existingFile = item.Files.First(f => f.Filename == "skin.ini"); - using (var stream = Files.Storage.GetStream(existingFile.FileInfo.StoragePath)) using (var sr = new StreamReader(stream)) { @@ -220,10 +213,12 @@ namespace osu.Game.Skinning } using (Stream stream = new MemoryStream()) - using (var sw = new StreamWriter(stream, Encoding.UTF8, 1024, true)) { - foreach (string line in outputLines) - sw.WriteLine(line); + using (var sw = new StreamWriter(stream, Encoding.UTF8, 1024, true)) + { + foreach (string line in outputLines) + sw.WriteLine(line); + } ReplaceFile(item, existingFile, stream); } @@ -231,11 +226,13 @@ namespace osu.Game.Skinning else { using (Stream stream = new MemoryStream()) - using (var sw = new StreamWriter(stream, Encoding.UTF8, 1024, true)) { - sw.WriteLine("[General]"); - sw.WriteLine(nameLine); - sw.WriteLine(authorLine); + using (var sw = new StreamWriter(stream, Encoding.UTF8, 1024, true)) + { + sw.WriteLine("[General]"); + sw.WriteLine(nameLine); + sw.WriteLine(authorLine); + } AddFile(item, stream, "skin.ini"); } From 602303e9470225f1a57f647fb648331a1150a10e Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 20 Oct 2021 17:03:30 +0900 Subject: [PATCH 06/20] Add test coverage for `skin.ini` contents --- osu.Game.Tests/Skins/IO/ImportSkinTest.cs | 103 ++++++++++++++++------ osu.Game/Skinning/SkinManager.cs | 4 + 2 files changed, 80 insertions(+), 27 deletions(-) diff --git a/osu.Game.Tests/Skins/IO/ImportSkinTest.cs b/osu.Game.Tests/Skins/IO/ImportSkinTest.cs index 8449b0bbd3..67c5be0c2a 100644 --- a/osu.Game.Tests/Skins/IO/ImportSkinTest.cs +++ b/osu.Game.Tests/Skins/IO/ImportSkinTest.cs @@ -9,6 +9,7 @@ using System.Threading.Tasks; using NUnit.Framework; using osu.Framework.Allocation; using osu.Framework.Platform; +using osu.Game.IO; using osu.Game.IO.Archives; using osu.Game.Skinning; using SharpCompress.Archives.Zip; @@ -22,21 +23,36 @@ namespace osu.Game.Tests.Skins.IO [Test] public Task TestSingleImportDifferentFilename() => runSkinTest(async osu => { - var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("test skin", "skinner"), "skin.osk")); + var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("test skin", "skinner"), "skin.osk")); // When the import filename doesn't match, it should be appended (and update the skin.ini). - Assert.That(imported.Name, Is.EqualTo("test skin [skin]")); - Assert.That(imported.Creator, Is.EqualTo("skinner")); + assertCorrectMetadata(imported, "test skin [skin]", "skinner", osu); }); [Test] public Task TestSingleImportMatchingFilename() => runSkinTest(async osu => { - var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("test skin", "skinner"), "test skin.osk")); + var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("test skin", "skinner"), "test skin.osk")); // When the import filename matches it shouldn't be appended. - Assert.That(imported.Name, Is.EqualTo("test skin")); - Assert.That(imported.Creator, Is.EqualTo("skinner")); + assertCorrectMetadata(imported, "test skin", "skinner", osu); + }); + + [Test] + public Task TestSingleImportNoIniFile() => runSkinTest(async osu => + { + var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithNonIniFile(), "test skin.osk")); + + // When the import filename matches it shouldn't be appended. + assertCorrectMetadata(imported, "test skin", "Unknown", osu); + }); + + [Test] + public Task TestEmptyImportFails() => runSkinTest(osu => + { + Assert.ThrowsAsync(() => loadSkinIntoOsu(osu, new ZipArchiveReader(createEmptyOsk(), "test skin.osk"))); + + return Task.CompletedTask; }); #endregion @@ -46,8 +62,8 @@ namespace osu.Game.Tests.Skins.IO [Test] public Task TestImportTwiceWithSameMetadataAndFilename() => runSkinTest(async osu => { - var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("test skin", "skinner"), "skin.osk")); - var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("test skin", "skinner"), "skin.osk")); + var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("test skin", "skinner"), "skin.osk")); + var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("test skin", "skinner"), "skin.osk")); assertImportedOnce(imported, imported2); }); @@ -56,8 +72,8 @@ namespace osu.Game.Tests.Skins.IO public Task TestImportTwiceWithNoMetadataSameDownloadFilename() => runSkinTest(async osu => { // if a user downloads two skins that do have skin.ini files but don't have any creator metadata in the skin.ini, they should both import separately just for safety. - var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk(string.Empty, string.Empty), "download.osk")); - var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk(string.Empty, string.Empty), "download.osk")); + var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni(string.Empty, string.Empty), "download.osk")); + var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni(string.Empty, string.Empty), "download.osk")); assertImportedOnce(imported, imported2); }); @@ -65,14 +81,24 @@ namespace osu.Game.Tests.Skins.IO [Test] public Task TestImportUpperCasedOskArchive() => runSkinTest(async osu => { - var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("name 1", "author 1"), "name 1.OsK")); - assertCorrectMetadata(imported, "name 1", "author 1"); + var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("name 1", "author 1"), "name 1.OsK")); + assertCorrectMetadata(imported, "name 1", "author 1", osu); - var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("name 1", "author 1"), "name 1.oSK")); + var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("name 1", "author 1"), "name 1.oSK")); assertImportedOnce(imported, imported2); }); + [Test] + public Task TestSameMetadataNameSameFolderName() => runSkinTest(async osu => + { + var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("name 1", "author 1"), "my custom skin 1")); + var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("name 1", "author 1"), "my custom skin 1")); + + assertImportedOnce(imported, imported2); + assertCorrectMetadata(imported, "name 1 [my custom skin 1]", "author 1", osu); + }); + #endregion #region Cases where imports should be uniquely imported @@ -80,8 +106,8 @@ namespace osu.Game.Tests.Skins.IO [Test] public Task TestImportTwiceWithSameMetadataButDifferentFilename() => runSkinTest(async osu => { - var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("test skin", "skinner"), "skin.osk")); - var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("test skin", "skinner"), "skin2.osk")); + var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("test skin", "skinner"), "skin.osk")); + var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("test skin", "skinner"), "skin2.osk")); assertImportedBoth(imported, imported2); }); @@ -90,8 +116,8 @@ namespace osu.Game.Tests.Skins.IO public Task TestImportTwiceWithNoMetadataDifferentDownloadFilename() => runSkinTest(async osu => { // if a user downloads two skins that do have skin.ini files but don't have any creator metadata in the skin.ini, they should both import separately just for safety. - var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk(string.Empty, string.Empty), "download.osk")); - var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk(string.Empty, string.Empty), "download2.osk")); + var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni(string.Empty, string.Empty), "download.osk")); + var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni(string.Empty, string.Empty), "download2.osk")); assertImportedBoth(imported, imported2); }); @@ -99,31 +125,37 @@ namespace osu.Game.Tests.Skins.IO [Test] public Task TestImportTwiceWithSameFilenameDifferentMetadata() => runSkinTest(async osu => { - var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("test skin v2", "skinner"), "skin.osk")); - var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("test skin v2.1", "skinner"), "skin.osk")); + var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("test skin v2", "skinner"), "skin.osk")); + var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("test skin v2.1", "skinner"), "skin.osk")); assertImportedBoth(imported, imported2); - assertCorrectMetadata(imported, "test skin v2 [skin]", "skinner"); - assertCorrectMetadata(imported2, "test skin v2.1 [skin]", "skinner"); + assertCorrectMetadata(imported, "test skin v2 [skin]", "skinner", osu); + assertCorrectMetadata(imported2, "test skin v2.1 [skin]", "skinner", osu); }); [Test] public Task TestSameMetadataNameDifferentFolderName() => runSkinTest(async osu => { - var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("name 1", "author 1"), "my custom skin 1")); - var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOsk("name 1", "author 1"), "my custom skin 2")); + var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("name 1", "author 1"), "my custom skin 1")); + var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("name 1", "author 1"), "my custom skin 2")); assertImportedBoth(imported, imported2); - assertCorrectMetadata(imported, "name 1 [my custom skin 2]", "author 1"); - assertCorrectMetadata(imported, "name 1 [my custom skin 2]", "author 1"); + assertCorrectMetadata(imported, "name 1 [my custom skin 1]", "author 1", osu); + assertCorrectMetadata(imported2, "name 1 [my custom skin 2]", "author 1", osu); }); #endregion - private void assertCorrectMetadata(SkinInfo imported, string name, string creator) + private void assertCorrectMetadata(SkinInfo imported, string name, string creator, OsuGameBase osu) { Assert.That(imported.Name, Is.EqualTo(name)); Assert.That(imported.Creator, Is.EqualTo(creator)); + + // for extra safety let's reconstruct the skin, reading from the skin.ini. + var instance = imported.CreateInstance((IStorageResourceProvider)osu.Dependencies.Get(typeof(SkinManager))); + + Assert.That(instance.Configuration.SkinInfo.Name, Is.EqualTo(name)); + Assert.That(instance.Configuration.SkinInfo.Creator, Is.EqualTo(creator)); } private void assertImportedBoth(SkinInfo imported, SkinInfo imported2) @@ -140,7 +172,24 @@ namespace osu.Game.Tests.Skins.IO Assert.That(imported2.Files.Select(f => f.FileInfoID), Is.EquivalentTo(imported.Files.Select(f => f.FileInfoID))); } - private MemoryStream createOsk(string name, string author, bool makeUnique = false) + private MemoryStream createEmptyOsk() + { + var zipStream = new MemoryStream(); + using var zip = ZipArchive.Create(); + zip.SaveTo(zipStream); + return zipStream; + } + + private MemoryStream createOskWithNonIniFile() + { + var zipStream = new MemoryStream(); + using var zip = ZipArchive.Create(); + zip.AddEntry("hitcircle.png", new MemoryStream(new byte[] { 0, 1, 2, 3 })); + zip.SaveTo(zipStream); + return zipStream; + } + + private MemoryStream createOskWithIni(string name, string author, bool makeUnique = false) { var zipStream = new MemoryStream(); using var zip = ZipArchive.Create(); diff --git a/osu.Game/Skinning/SkinManager.cs b/osu.Game/Skinning/SkinManager.cs index 452c081a41..213c92c103 100644 --- a/osu.Game/Skinning/SkinManager.cs +++ b/osu.Game/Skinning/SkinManager.cs @@ -135,6 +135,10 @@ namespace osu.Game.Skinning protected override string ComputeHash(SkinInfo item, ArchiveReader reader = null) { + // we will be adding a hashable file below, but this is only useful if there are any other files in the skin. + if (item.Files.Count == 0) + throw new InvalidOperationException("Attempted to hash an archive with no files"); + var instance = GetSkin(item); // This function can be run on fresh import or save. The logic here ensures a skin.ini file is in a good state for both operations. From 5f53dd80213c50eccb324c668109429584f94ec6 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 20 Oct 2021 17:12:44 +0900 Subject: [PATCH 07/20] Rename test variable for legibility --- osu.Game.Tests/Skins/IO/ImportSkinTest.cs | 98 +++++++++++------------ 1 file changed, 49 insertions(+), 49 deletions(-) diff --git a/osu.Game.Tests/Skins/IO/ImportSkinTest.cs b/osu.Game.Tests/Skins/IO/ImportSkinTest.cs index 67c5be0c2a..21e4948972 100644 --- a/osu.Game.Tests/Skins/IO/ImportSkinTest.cs +++ b/osu.Game.Tests/Skins/IO/ImportSkinTest.cs @@ -23,28 +23,28 @@ namespace osu.Game.Tests.Skins.IO [Test] public Task TestSingleImportDifferentFilename() => runSkinTest(async osu => { - var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("test skin", "skinner"), "skin.osk")); + var import1 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("test skin", "skinner"), "skin.osk")); // When the import filename doesn't match, it should be appended (and update the skin.ini). - assertCorrectMetadata(imported, "test skin [skin]", "skinner", osu); + assertCorrectMetadata(import1, "test skin [skin]", "skinner", osu); }); [Test] public Task TestSingleImportMatchingFilename() => runSkinTest(async osu => { - var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("test skin", "skinner"), "test skin.osk")); + var import1 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("test skin", "skinner"), "test skin.osk")); // When the import filename matches it shouldn't be appended. - assertCorrectMetadata(imported, "test skin", "skinner", osu); + assertCorrectMetadata(import1, "test skin", "skinner", osu); }); [Test] public Task TestSingleImportNoIniFile() => runSkinTest(async osu => { - var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithNonIniFile(), "test skin.osk")); + var import1 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithNonIniFile(), "test skin.osk")); // When the import filename matches it shouldn't be appended. - assertCorrectMetadata(imported, "test skin", "Unknown", osu); + assertCorrectMetadata(import1, "test skin", "Unknown", osu); }); [Test] @@ -62,114 +62,114 @@ namespace osu.Game.Tests.Skins.IO [Test] public Task TestImportTwiceWithSameMetadataAndFilename() => runSkinTest(async osu => { - var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("test skin", "skinner"), "skin.osk")); - var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("test skin", "skinner"), "skin.osk")); + var import1 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("test skin", "skinner"), "skin.osk")); + var import2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("test skin", "skinner"), "skin.osk")); - assertImportedOnce(imported, imported2); + assertImportedOnce(import1, import2); }); [Test] public Task TestImportTwiceWithNoMetadataSameDownloadFilename() => runSkinTest(async osu => { // if a user downloads two skins that do have skin.ini files but don't have any creator metadata in the skin.ini, they should both import separately just for safety. - var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni(string.Empty, string.Empty), "download.osk")); - var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni(string.Empty, string.Empty), "download.osk")); + var import1 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni(string.Empty, string.Empty), "download.osk")); + var import2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni(string.Empty, string.Empty), "download.osk")); - assertImportedOnce(imported, imported2); + assertImportedOnce(import1, import2); }); [Test] public Task TestImportUpperCasedOskArchive() => runSkinTest(async osu => { - var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("name 1", "author 1"), "name 1.OsK")); - assertCorrectMetadata(imported, "name 1", "author 1", osu); + var import1 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("name 1", "author 1"), "name 1.OsK")); + assertCorrectMetadata(import1, "name 1", "author 1", osu); - var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("name 1", "author 1"), "name 1.oSK")); + var import2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("name 1", "author 1"), "name 1.oSK")); - assertImportedOnce(imported, imported2); + assertImportedOnce(import1, import2); }); [Test] public Task TestSameMetadataNameSameFolderName() => runSkinTest(async osu => { - var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("name 1", "author 1"), "my custom skin 1")); - var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("name 1", "author 1"), "my custom skin 1")); + var import1 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("name 1", "author 1"), "my custom skin 1")); + var import2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("name 1", "author 1"), "my custom skin 1")); - assertImportedOnce(imported, imported2); - assertCorrectMetadata(imported, "name 1 [my custom skin 1]", "author 1", osu); + assertImportedOnce(import1, import2); + assertCorrectMetadata(import1, "name 1 [my custom skin 1]", "author 1", osu); }); #endregion - #region Cases where imports should be uniquely imported + #region Cases where imports should be uniquely import1 [Test] public Task TestImportTwiceWithSameMetadataButDifferentFilename() => runSkinTest(async osu => { - var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("test skin", "skinner"), "skin.osk")); - var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("test skin", "skinner"), "skin2.osk")); + var import1 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("test skin", "skinner"), "skin.osk")); + var import2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("test skin", "skinner"), "skin2.osk")); - assertImportedBoth(imported, imported2); + assertImportedBoth(import1, import2); }); [Test] public Task TestImportTwiceWithNoMetadataDifferentDownloadFilename() => runSkinTest(async osu => { // if a user downloads two skins that do have skin.ini files but don't have any creator metadata in the skin.ini, they should both import separately just for safety. - var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni(string.Empty, string.Empty), "download.osk")); - var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni(string.Empty, string.Empty), "download2.osk")); + var import1 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni(string.Empty, string.Empty), "download.osk")); + var import2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni(string.Empty, string.Empty), "download2.osk")); - assertImportedBoth(imported, imported2); + assertImportedBoth(import1, import2); }); [Test] public Task TestImportTwiceWithSameFilenameDifferentMetadata() => runSkinTest(async osu => { - var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("test skin v2", "skinner"), "skin.osk")); - var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("test skin v2.1", "skinner"), "skin.osk")); + var import1 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("test skin v2", "skinner"), "skin.osk")); + var import2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("test skin v2.1", "skinner"), "skin.osk")); - assertImportedBoth(imported, imported2); - assertCorrectMetadata(imported, "test skin v2 [skin]", "skinner", osu); - assertCorrectMetadata(imported2, "test skin v2.1 [skin]", "skinner", osu); + assertImportedBoth(import1, import2); + assertCorrectMetadata(import1, "test skin v2 [skin]", "skinner", osu); + assertCorrectMetadata(import2, "test skin v2.1 [skin]", "skinner", osu); }); [Test] public Task TestSameMetadataNameDifferentFolderName() => runSkinTest(async osu => { - var imported = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("name 1", "author 1"), "my custom skin 1")); - var imported2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("name 1", "author 1"), "my custom skin 2")); + var import1 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("name 1", "author 1"), "my custom skin 1")); + var import2 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createOskWithIni("name 1", "author 1"), "my custom skin 2")); - assertImportedBoth(imported, imported2); - assertCorrectMetadata(imported, "name 1 [my custom skin 1]", "author 1", osu); - assertCorrectMetadata(imported2, "name 1 [my custom skin 2]", "author 1", osu); + assertImportedBoth(import1, import2); + assertCorrectMetadata(import1, "name 1 [my custom skin 1]", "author 1", osu); + assertCorrectMetadata(import2, "name 1 [my custom skin 2]", "author 1", osu); }); #endregion - private void assertCorrectMetadata(SkinInfo imported, string name, string creator, OsuGameBase osu) + private void assertCorrectMetadata(SkinInfo import1, string name, string creator, OsuGameBase osu) { - Assert.That(imported.Name, Is.EqualTo(name)); - Assert.That(imported.Creator, Is.EqualTo(creator)); + Assert.That(import1.Name, Is.EqualTo(name)); + Assert.That(import1.Creator, Is.EqualTo(creator)); // for extra safety let's reconstruct the skin, reading from the skin.ini. - var instance = imported.CreateInstance((IStorageResourceProvider)osu.Dependencies.Get(typeof(SkinManager))); + var instance = import1.CreateInstance((IStorageResourceProvider)osu.Dependencies.Get(typeof(SkinManager))); Assert.That(instance.Configuration.SkinInfo.Name, Is.EqualTo(name)); Assert.That(instance.Configuration.SkinInfo.Creator, Is.EqualTo(creator)); } - private void assertImportedBoth(SkinInfo imported, SkinInfo imported2) + private void assertImportedBoth(SkinInfo import1, SkinInfo import2) { - Assert.That(imported2.ID, Is.Not.EqualTo(imported.ID)); - Assert.That(imported2.Hash, Is.Not.EqualTo(imported.Hash)); - Assert.That(imported2.Files.Select(f => f.FileInfoID), Is.Not.EquivalentTo(imported.Files.Select(f => f.FileInfoID))); + Assert.That(import2.ID, Is.Not.EqualTo(import1.ID)); + Assert.That(import2.Hash, Is.Not.EqualTo(import1.Hash)); + Assert.That(import2.Files.Select(f => f.FileInfoID), Is.Not.EquivalentTo(import1.Files.Select(f => f.FileInfoID))); } - private void assertImportedOnce(SkinInfo imported, SkinInfo imported2) + private void assertImportedOnce(SkinInfo import1, SkinInfo import2) { - Assert.That(imported2.ID, Is.EqualTo(imported.ID)); - Assert.That(imported2.Hash, Is.EqualTo(imported.Hash)); - Assert.That(imported2.Files.Select(f => f.FileInfoID), Is.EquivalentTo(imported.Files.Select(f => f.FileInfoID))); + Assert.That(import2.ID, Is.EqualTo(import1.ID)); + Assert.That(import2.Hash, Is.EqualTo(import1.Hash)); + Assert.That(import2.Files.Select(f => f.FileInfoID), Is.EquivalentTo(import1.Files.Select(f => f.FileInfoID))); } private MemoryStream createEmptyOsk() From 9e6e41d7c06c09e671c3274ef7f56a75cfd09f1e Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 20 Oct 2021 17:33:36 +0900 Subject: [PATCH 08/20] Add migration to reset and repopulate existing skin hashes --- osu.Game/Database/ArchiveModelManager.cs | 11 +++++---- .../20211020081609_ResetSkinHashes.cs | 23 +++++++++++++++++++ osu.Game/Skinning/SkinManager.cs | 22 ++++++++++++++++++ 3 files changed, 52 insertions(+), 4 deletions(-) create mode 100644 osu.Game/Migrations/20211020081609_ResetSkinHashes.cs diff --git a/osu.Game/Database/ArchiveModelManager.cs b/osu.Game/Database/ArchiveModelManager.cs index 5b07bfe9a5..63a5da79a7 100644 --- a/osu.Game/Database/ArchiveModelManager.cs +++ b/osu.Game/Database/ArchiveModelManager.cs @@ -32,7 +32,7 @@ namespace osu.Game.Database /// The associated file join type. public abstract class ArchiveModelManager : IModelManager, IModelFileManager where TModel : class, IHasFiles, IHasPrimaryKey, ISoftDelete - where TFileModel : class, INamedFileInfo, new() + where TFileModel : class, INamedFileInfo, IHasPrimaryKey, new() { private const int import_queue_request_concurrency = 1; @@ -520,9 +520,12 @@ namespace osu.Game.Database { Files.Dereference(file.FileInfo); - // This shouldn't be required, but here for safety in case the provided TModel is not being change tracked - // Definitely can be removed once we rework the database backend. - usage.Context.Set().Remove(file); + if (file.ID > 0) + { + // This shouldn't be required, but here for safety in case the provided TModel is not being change tracked + // Definitely can be removed once we rework the database backend. + usage.Context.Set().Remove(file); + } } model.Files.Remove(file); diff --git a/osu.Game/Migrations/20211020081609_ResetSkinHashes.cs b/osu.Game/Migrations/20211020081609_ResetSkinHashes.cs new file mode 100644 index 0000000000..6d53c019ec --- /dev/null +++ b/osu.Game/Migrations/20211020081609_ResetSkinHashes.cs @@ -0,0 +1,23 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +using Microsoft.EntityFrameworkCore.Infrastructure; +using Microsoft.EntityFrameworkCore.Migrations; +using osu.Game.Database; + +namespace osu.Game.Migrations +{ + [DbContext(typeof(OsuDbContext))] + [Migration("20211020081609_ResetSkinHashes")] + public partial class ResetSkinHashes : Migration + { + protected override void Up(MigrationBuilder migrationBuilder) + { + migrationBuilder.Sql($"UPDATE SkinInfo SET Hash = null"); + } + + protected override void Down(MigrationBuilder migrationBuilder) + { + } + } +} diff --git a/osu.Game/Skinning/SkinManager.cs b/osu.Game/Skinning/SkinManager.cs index 213c92c103..0b611bfe8f 100644 --- a/osu.Game/Skinning/SkinManager.cs +++ b/osu.Game/Skinning/SkinManager.cs @@ -18,6 +18,7 @@ using osu.Framework.Graphics; using osu.Framework.Graphics.OpenGL.Textures; using osu.Framework.Graphics.Textures; using osu.Framework.IO.Stores; +using osu.Framework.Logging; using osu.Framework.Platform; using osu.Framework.Testing; using osu.Framework.Utils; @@ -84,6 +85,27 @@ namespace osu.Game.Skinning SourceChanged?.Invoke(); }; + + // can be removed 20220420. + populateMissingHashes(); + } + + private void populateMissingHashes() + { + var skinsWithoutHashes = ModelStore.ConsumableItems.Where(i => i.Hash == null).ToArray(); + + foreach (SkinInfo skin in skinsWithoutHashes) + { + try + { + Update(skin); + } + catch (Exception e) + { + Delete(skin); + Logger.Error(e, $"Existing skin {skin} has been deleted during hash recomputation due to being invalid"); + } + } } protected override bool ShouldDeleteArchive(string path) => Path.GetExtension(path)?.ToLowerInvariant() == ".osk"; From 68c01fc20442312cd09a27c29194c2fb3774be35 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 20 Oct 2021 17:34:09 +0900 Subject: [PATCH 09/20] Fix infinite loop on default skin (it can't have a `skin.ini`) --- osu.Game/Skinning/SkinManager.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Skinning/SkinManager.cs b/osu.Game/Skinning/SkinManager.cs index 0b611bfe8f..afa411a15b 100644 --- a/osu.Game/Skinning/SkinManager.cs +++ b/osu.Game/Skinning/SkinManager.cs @@ -186,7 +186,7 @@ namespace osu.Game.Skinning // By this point, the metadata in SkinInfo will be correct. // Regardless of whether this is an import or not, let's write the skin.ini if non-existing or non-matching. // This is (weirdly) done inside ComputeHash to avoid adding a new method to handle this case. After switching to realm it can be moved into another place. - if (skinIniSourcedName != item.Name) + if (instance is LegacySkin && skinIniSourcedName != item.Name) updateSkinIniMetadata(item); return base.ComputeHash(item, reader); From 59b7210efaaf976301deefc9fdbd3c2d756f675f Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 21 Oct 2021 13:35:26 +0900 Subject: [PATCH 10/20] Revert disallowing imports with no files While it is logical that we want this, from a testing perspective this is a bit of a nightmare to fix. Let's revisit at a later point in time. --- osu.Game.Tests/Skins/IO/ImportSkinTest.cs | 7 ++-- osu.Game/Database/ArchiveModelManager.cs | 40 ++++++++++++++--------- osu.Game/Skinning/SkinManager.cs | 4 --- 3 files changed, 28 insertions(+), 23 deletions(-) diff --git a/osu.Game.Tests/Skins/IO/ImportSkinTest.cs b/osu.Game.Tests/Skins/IO/ImportSkinTest.cs index 21e4948972..20bf42345f 100644 --- a/osu.Game.Tests/Skins/IO/ImportSkinTest.cs +++ b/osu.Game.Tests/Skins/IO/ImportSkinTest.cs @@ -48,11 +48,12 @@ namespace osu.Game.Tests.Skins.IO }); [Test] - public Task TestEmptyImportFails() => runSkinTest(osu => + public Task TestEmptyImportImportsWithFilename() => runSkinTest(async osu => { - Assert.ThrowsAsync(() => loadSkinIntoOsu(osu, new ZipArchiveReader(createEmptyOsk(), "test skin.osk"))); + var import1 = await loadSkinIntoOsu(osu, new ZipArchiveReader(createEmptyOsk(), "test skin.osk")); - return Task.CompletedTask; + // When the import filename matches it shouldn't be appended. + assertCorrectMetadata(import1, "test skin", "Unknown", osu); }); #endregion diff --git a/osu.Game/Database/ArchiveModelManager.cs b/osu.Game/Database/ArchiveModelManager.cs index 63a5da79a7..0bc4c40459 100644 --- a/osu.Game/Database/ArchiveModelManager.cs +++ b/osu.Game/Database/ArchiveModelManager.cs @@ -322,22 +322,22 @@ namespace osu.Game.Database .OrderBy(f => f.Filename) .ToArray(); - if (hashableFiles.Length == 0) - throw new InvalidOperationException("Attempted to hash an archive with no files"); - - // for now, concatenate all hashable files in the set to create a unique hash. - MemoryStream hashable = new MemoryStream(); - - foreach (TFileModel file in hashableFiles) + if (hashableFiles.Length > 0) { - using (Stream s = Files.Store.GetStream(file.FileInfo.StoragePath)) - s.CopyTo(hashable); + // for now, concatenate all hashable files in the set to create a unique hash. + MemoryStream hashable = new MemoryStream(); + + foreach (TFileModel file in hashableFiles) + { + using (Stream s = Files.Store.GetStream(file.FileInfo.StoragePath)) + s.CopyTo(hashable); + } + + if (hashable.Length > 0) + return hashable.ComputeSHA2Hash(); } - if (hashable.Length > 0) - return hashable.ComputeSHA2Hash(); - - return item.Hash; + return generateFallbackHash(); } /// @@ -707,10 +707,10 @@ namespace osu.Game.Database s.CopyTo(hashable); } - if (hashable.Length == 0) - throw new InvalidOperationException("Attempted to hash an archive with no files"); + if (hashable.Length > 0) + return hashable.ComputeSHA2Hash(); - return hashable.ComputeSHA2Hash(); + return generateFallbackHash(); } /// @@ -923,6 +923,14 @@ namespace osu.Game.Database #endregion + private static string generateFallbackHash() + { + // if a hash could no be generated from file content, presume a unique / new import. + // therefore, let's use a guaranteed unique hash. + // this doesn't follow the SHA2 hashing schema intentionally, so such entries on the data store can be identified. + return Guid.NewGuid().ToString(); + } + private string getValidFilename(string filename) { foreach (char c in Path.GetInvalidFileNameChars()) diff --git a/osu.Game/Skinning/SkinManager.cs b/osu.Game/Skinning/SkinManager.cs index afa411a15b..304e48e854 100644 --- a/osu.Game/Skinning/SkinManager.cs +++ b/osu.Game/Skinning/SkinManager.cs @@ -157,10 +157,6 @@ namespace osu.Game.Skinning protected override string ComputeHash(SkinInfo item, ArchiveReader reader = null) { - // we will be adding a hashable file below, but this is only useful if there are any other files in the skin. - if (item.Files.Count == 0) - throw new InvalidOperationException("Attempted to hash an archive with no files"); - var instance = GetSkin(item); // This function can be run on fresh import or save. The logic here ensures a skin.ini file is in a good state for both operations. From a5088cac27c2d6955165efd45a86b68ed7952128 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 21 Oct 2021 13:36:04 +0900 Subject: [PATCH 11/20] Fix default metadata propagation when no files are present --- osu.Game/Skinning/SkinInfo.cs | 6 +++--- osu.Game/Skinning/SkinManager.cs | 5 +++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/osu.Game/Skinning/SkinInfo.cs b/osu.Game/Skinning/SkinInfo.cs index 2bf8668ec6..3b34e23d57 100644 --- a/osu.Game/Skinning/SkinInfo.cs +++ b/osu.Game/Skinning/SkinInfo.cs @@ -18,12 +18,12 @@ namespace osu.Game.Skinning public int ID { get; set; } - public string Name { get; set; } + public string Name { get; set; } = string.Empty; + + public string Creator { get; set; } = string.Empty; public string Hash { get; set; } - public string Creator { get; set; } - public string InstantiationInfo { get; set; } public virtual Skin CreateInstance(IStorageResourceProvider resources) diff --git a/osu.Game/Skinning/SkinManager.cs b/osu.Game/Skinning/SkinManager.cs index 304e48e854..fffdf19976 100644 --- a/osu.Game/Skinning/SkinManager.cs +++ b/osu.Game/Skinning/SkinManager.cs @@ -149,7 +149,7 @@ namespace osu.Game.Skinning CurrentSkinInfo.Value = ModelStore.ConsumableItems.Single(i => i.ID == chosen.ID); } - protected override SkinInfo CreateModel(ArchiveReader archive) => new SkinInfo { Name = archive.Name }; + protected override SkinInfo CreateModel(ArchiveReader archive) => new SkinInfo { Name = archive.Name ?? "No name" }; private const string unknown_creator_string = "Unknown"; @@ -163,6 +163,7 @@ namespace osu.Game.Skinning // LegacySkin will parse the skin.ini and populate `Skin.Configuration` during construction above. string skinIniSourcedName = instance.Configuration.SkinInfo.Name; + string skinIniSourcedCreator = instance.Configuration.SkinInfo.Creator; string archiveName = item.Name.Replace(".osk", "", StringComparison.OrdinalIgnoreCase); bool isImport = reader != null; @@ -170,7 +171,7 @@ namespace osu.Game.Skinning if (isImport) { item.Name = !string.IsNullOrEmpty(skinIniSourcedName) ? skinIniSourcedName : archiveName; - item.Creator = instance.Configuration.SkinInfo.Creator ?? unknown_creator_string; + item.Creator = !string.IsNullOrEmpty(skinIniSourcedCreator) ? skinIniSourcedCreator : unknown_creator_string; // For imports, we want to use the archive or folder name as part of the metadata, in addition to any existing skin.ini metadata. // In an ideal world, skin.ini would be the only source of metadata, but a lot of skin creators and users don't update it when making modifications. From 08971ff8f27e1489354f8ee596f89ce186bd6565 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 22 Oct 2021 11:03:28 +0900 Subject: [PATCH 12/20] Fix typo in region spedc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bartłomiej Dach --- osu.Game.Tests/Skins/IO/ImportSkinTest.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game.Tests/Skins/IO/ImportSkinTest.cs b/osu.Game.Tests/Skins/IO/ImportSkinTest.cs index 20bf42345f..b62fb8bd87 100644 --- a/osu.Game.Tests/Skins/IO/ImportSkinTest.cs +++ b/osu.Game.Tests/Skins/IO/ImportSkinTest.cs @@ -102,7 +102,7 @@ namespace osu.Game.Tests.Skins.IO #endregion - #region Cases where imports should be uniquely import1 + #region Cases where imports should be uniquely imported [Test] public Task TestImportTwiceWithSameMetadataButDifferentFilename() => runSkinTest(async osu => From e5b73f25cd40cadf90884fb8f2dc160d6a764ad8 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 22 Oct 2021 12:56:05 +0900 Subject: [PATCH 13/20] Ensure newly created `skin.ini` files are set to latest version --- osu.Game/Skinning/SkinManager.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/osu.Game/Skinning/SkinManager.cs b/osu.Game/Skinning/SkinManager.cs index fffdf19976..16ed25eeb1 100644 --- a/osu.Game/Skinning/SkinManager.cs +++ b/osu.Game/Skinning/SkinManager.cs @@ -255,6 +255,7 @@ namespace osu.Game.Skinning sw.WriteLine("[General]"); sw.WriteLine(nameLine); sw.WriteLine(authorLine); + sw.WriteLine("Version: latest"); } AddFile(item, stream, "skin.ini"); From 9cdc1ba59219ecd31df04c235def14567f4fbee4 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 22 Oct 2021 13:43:45 +0900 Subject: [PATCH 14/20] Fix default legacy skin not being able to read from stored `skin.ini` --- osu.Game/Skinning/DefaultLegacySkin.cs | 9 +++++- osu.Game/Skinning/LegacySkin.cs | 39 ++++++++++++++++---------- 2 files changed, 32 insertions(+), 16 deletions(-) diff --git a/osu.Game/Skinning/DefaultLegacySkin.cs b/osu.Game/Skinning/DefaultLegacySkin.cs index 16ac17546d..cd6dbd9ddd 100644 --- a/osu.Game/Skinning/DefaultLegacySkin.cs +++ b/osu.Game/Skinning/DefaultLegacySkin.cs @@ -19,7 +19,14 @@ namespace osu.Game.Skinning [UsedImplicitly(ImplicitUseKindFlags.InstantiatedWithFixedConstructorSignature)] public DefaultLegacySkin(SkinInfo skin, IStorageResourceProvider resources) - : base(skin, new NamespacedResourceStore(resources.Resources, "Skins/Legacy"), resources, string.Empty) + : base( + skin, + new NamespacedResourceStore(resources.Resources, "Skins/Legacy"), + resources, + // A default legacy skin may still have a skin.ini if it is modified by the user. + // We must specify the stream directly as we are redirecting storage to the osu-resources location for other files. + new LegacySkinResourceStore(skin, resources.Files).GetStream("skin.ini") + ) { Configuration.CustomColours["SliderBall"] = new Color4(2, 170, 255, 255); Configuration.CustomComboColours = new List diff --git a/osu.Game/Skinning/LegacySkin.cs b/osu.Game/Skinning/LegacySkin.cs index b09620411b..5ab3ef84db 100644 --- a/osu.Game/Skinning/LegacySkin.cs +++ b/osu.Game/Skinning/LegacySkin.cs @@ -69,28 +69,37 @@ namespace osu.Game.Skinning /// Access to raw game resources. /// The user-facing filename of the configuration file to be parsed. Can accept an .osu or skin.ini file. protected LegacySkin(SkinInfo skin, [CanBeNull] IResourceStore storage, [CanBeNull] IStorageResourceProvider resources, string configurationFilename) + : this(skin, storage, resources, storage?.GetStream(configurationFilename)) + { + } + + /// + /// Construct a new legacy skin instance. + /// + /// The model for this skin. + /// A storage for looking up files within this skin using user-facing filenames. + /// Access to raw game resources. + /// An optional stream containing the contents of a skin.ini file. + protected LegacySkin(SkinInfo skin, [CanBeNull] IResourceStore storage, [CanBeNull] IStorageResourceProvider resources, [CanBeNull] Stream configurationStream) : base(skin, resources) { - using (var stream = storage?.GetStream(configurationFilename)) + if (configurationStream != null) { - if (stream != null) + using (LineBufferedReader reader = new LineBufferedReader(configurationStream, true)) + Configuration = new LegacySkinDecoder().Decode(reader); + + configurationStream.Seek(0, SeekOrigin.Begin); + + using (LineBufferedReader reader = new LineBufferedReader(configurationStream)) { - using (LineBufferedReader reader = new LineBufferedReader(stream, true)) - Configuration = new LegacySkinDecoder().Decode(reader); + var maniaList = new LegacyManiaSkinDecoder().Decode(reader); - stream.Seek(0, SeekOrigin.Begin); - - using (LineBufferedReader reader = new LineBufferedReader(stream)) - { - var maniaList = new LegacyManiaSkinDecoder().Decode(reader); - - foreach (var config in maniaList) - maniaConfigurations[config.Keys] = config; - } + foreach (var config in maniaList) + maniaConfigurations[config.Keys] = config; } - else - Configuration = new LegacySkinConfiguration(); } + else + Configuration = new LegacySkinConfiguration(); if (storage != null) { From 82d0a6515fbe39536dd86122aede7caec25badd8 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 22 Oct 2021 14:08:12 +0900 Subject: [PATCH 15/20] Fix incorrect conditional for checking whether import (would fail for default skin) --- osu.Game/Skinning/SkinManager.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Skinning/SkinManager.cs b/osu.Game/Skinning/SkinManager.cs index 16ed25eeb1..707afade09 100644 --- a/osu.Game/Skinning/SkinManager.cs +++ b/osu.Game/Skinning/SkinManager.cs @@ -166,7 +166,7 @@ namespace osu.Game.Skinning string skinIniSourcedCreator = instance.Configuration.SkinInfo.Creator; string archiveName = item.Name.Replace(".osk", "", StringComparison.OrdinalIgnoreCase); - bool isImport = reader != null; + bool isImport = item.ID == 0; if (isImport) { From 93482414d6d69fff3e02a0083afb1fb55244db23 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 22 Oct 2021 14:41:59 +0900 Subject: [PATCH 16/20] Remove `SkinConfiguration` subclasses and allow configuration parsing for all skin types --- .../Legacy/CatchLegacySkinTransformer.cs | 2 +- .../Skinning/Legacy/LegacyStageBackground.cs | 2 +- .../Legacy/ManiaLegacySkinTransformer.cs | 2 +- .../Skinning/Legacy/LegacyMainCirclePiece.cs | 3 +- .../Skinning/Legacy/LegacyInputDrum.cs | 2 +- .../Gameplay/TestSceneHitObjectSamples.cs | 2 +- osu.Game.Tests/Skins/LegacySkinDecoderTest.cs | 2 +- .../Skins/TestSceneSkinConfigurationLookup.cs | 8 +-- .../Objects/Legacy/ConvertHitObjectParser.cs | 2 +- osu.Game/Skinning/DefaultSkin.cs | 1 - osu.Game/Skinning/DefaultSkinConfiguration.cs | 12 ---- osu.Game/Skinning/LegacyBeatmapSkin.cs | 2 +- osu.Game/Skinning/LegacySkin.cs | 55 ++++++++----------- osu.Game/Skinning/LegacySkinConfiguration.cs | 28 ---------- osu.Game/Skinning/LegacySkinDecoder.cs | 8 +-- osu.Game/Skinning/LegacySkinExtensions.cs | 2 +- osu.Game/Skinning/LegacySkinTransformer.cs | 2 +- osu.Game/Skinning/Skin.cs | 31 ++++++++++- osu.Game/Skinning/SkinConfiguration.cs | 20 +++++++ osu.Game/Skinning/SkinManager.cs | 2 +- 20 files changed, 92 insertions(+), 96 deletions(-) delete mode 100644 osu.Game/Skinning/DefaultSkinConfiguration.cs delete mode 100644 osu.Game/Skinning/LegacySkinConfiguration.cs diff --git a/osu.Game.Rulesets.Catch/Skinning/Legacy/CatchLegacySkinTransformer.cs b/osu.Game.Rulesets.Catch/Skinning/Legacy/CatchLegacySkinTransformer.cs index 10fc4e78b2..335d78388b 100644 --- a/osu.Game.Rulesets.Catch/Skinning/Legacy/CatchLegacySkinTransformer.cs +++ b/osu.Game.Rulesets.Catch/Skinning/Legacy/CatchLegacySkinTransformer.cs @@ -66,7 +66,7 @@ namespace osu.Game.Rulesets.Catch.Skinning.Legacy return null; case CatchSkinComponents.Catcher: - var version = GetConfig(LegacySkinConfiguration.LegacySetting.Version)?.Value ?? 1; + var version = GetConfig(SkinConfiguration.LegacySetting.Version)?.Value ?? 1; if (version < 2.3m) { diff --git a/osu.Game.Rulesets.Mania/Skinning/Legacy/LegacyStageBackground.cs b/osu.Game.Rulesets.Mania/Skinning/Legacy/LegacyStageBackground.cs index fec3e9493e..952fc7ddd6 100644 --- a/osu.Game.Rulesets.Mania/Skinning/Legacy/LegacyStageBackground.cs +++ b/osu.Game.Rulesets.Mania/Skinning/Legacy/LegacyStageBackground.cs @@ -98,7 +98,7 @@ namespace osu.Game.Rulesets.Mania.Skinning.Legacy float rightLineWidth = skin.GetManiaSkinConfig(LegacyManiaSkinConfigurationLookups.RightLineWidth, columnIndex)?.Value ?? 1; bool hasLeftLine = leftLineWidth > 0; - bool hasRightLine = rightLineWidth > 0 && skin.GetConfig(LegacySkinConfiguration.LegacySetting.Version)?.Value >= 2.4m + bool hasRightLine = rightLineWidth > 0 && skin.GetConfig(SkinConfiguration.LegacySetting.Version)?.Value >= 2.4m || isLastColumn; Color4 lineColour = skin.GetManiaSkinConfig(LegacyManiaSkinConfigurationLookups.ColumnLineColour, columnIndex)?.Value ?? Color4.White; diff --git a/osu.Game.Rulesets.Mania/Skinning/Legacy/ManiaLegacySkinTransformer.cs b/osu.Game.Rulesets.Mania/Skinning/Legacy/ManiaLegacySkinTransformer.cs index 814a737034..0de9a098bc 100644 --- a/osu.Game.Rulesets.Mania/Skinning/Legacy/ManiaLegacySkinTransformer.cs +++ b/osu.Game.Rulesets.Mania/Skinning/Legacy/ManiaLegacySkinTransformer.cs @@ -63,7 +63,7 @@ namespace osu.Game.Rulesets.Mania.Skinning.Legacy { this.beatmap = (ManiaBeatmap)beatmap; - isLegacySkin = new Lazy(() => GetConfig(LegacySkinConfiguration.LegacySetting.Version) != null); + isLegacySkin = new Lazy(() => GetConfig(SkinConfiguration.LegacySetting.Version) != null); hasKeyTexture = new Lazy(() => { var keyImage = this.GetManiaSkinConfig(LegacyManiaSkinConfigurationLookups.KeyImage, 0)?.Value ?? "mania-key1"; diff --git a/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs b/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs index d1c9b1bf92..0e0f4f4291 100644 --- a/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs +++ b/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs @@ -14,7 +14,6 @@ using osu.Game.Rulesets.Osu.Objects.Drawables; using osu.Game.Skinning; using osuTK; using osuTK.Graphics; -using static osu.Game.Skinning.LegacySkinConfiguration; namespace osu.Game.Rulesets.Osu.Skinning.Legacy { @@ -158,7 +157,7 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy if (hasNumber) { - var legacyVersion = skin.GetConfig(LegacySetting.Version)?.Value; + var legacyVersion = skin.GetConfig(SkinConfiguration.LegacySetting.Version)?.Value; if (legacyVersion >= 2.0m) // legacy skins of version 2.0 and newer only apply very short fade out to the number piece. diff --git a/osu.Game.Rulesets.Taiko/Skinning/Legacy/LegacyInputDrum.cs b/osu.Game.Rulesets.Taiko/Skinning/Legacy/LegacyInputDrum.cs index 86be40dea8..43c5c07f80 100644 --- a/osu.Game.Rulesets.Taiko/Skinning/Legacy/LegacyInputDrum.cs +++ b/osu.Game.Rulesets.Taiko/Skinning/Legacy/LegacyInputDrum.cs @@ -69,7 +69,7 @@ namespace osu.Game.Rulesets.Taiko.Skinning.Legacy // because the right half is flipped, we need to position using width - position to get the true "topleft" origin position float negativeScaleAdjust = content.Width / ratio; - if (skin.GetConfig(LegacySkinConfiguration.LegacySetting.Version)?.Value >= 2.1m) + if (skin.GetConfig(SkinConfiguration.LegacySetting.Version)?.Value >= 2.1m) { left.Centre.Position = new Vector2(0, taiko_bar_y) * ratio; right.Centre.Position = new Vector2(negativeScaleAdjust - 56, taiko_bar_y) * ratio; diff --git a/osu.Game.Tests/Gameplay/TestSceneHitObjectSamples.cs b/osu.Game.Tests/Gameplay/TestSceneHitObjectSamples.cs index fc420e22a1..153d5b8e36 100644 --- a/osu.Game.Tests/Gameplay/TestSceneHitObjectSamples.cs +++ b/osu.Game.Tests/Gameplay/TestSceneHitObjectSamples.cs @@ -7,7 +7,7 @@ using osu.Game.Rulesets; using osu.Game.Rulesets.Osu; using osu.Game.Tests.Beatmaps; using osu.Game.Tests.Resources; -using static osu.Game.Skinning.LegacySkinConfiguration; +using static osu.Game.Skinning.SkinConfiguration; namespace osu.Game.Tests.Gameplay { diff --git a/osu.Game.Tests/Skins/LegacySkinDecoderTest.cs b/osu.Game.Tests/Skins/LegacySkinDecoderTest.cs index dcb866c99f..cfc140ce39 100644 --- a/osu.Game.Tests/Skins/LegacySkinDecoderTest.cs +++ b/osu.Game.Tests/Skins/LegacySkinDecoderTest.cs @@ -106,7 +106,7 @@ namespace osu.Game.Tests.Skins var decoder = new LegacySkinDecoder(); using (var resStream = TestResources.OpenResource("skin-latest.ini")) using (var stream = new LineBufferedReader(resStream)) - Assert.AreEqual(LegacySkinConfiguration.LATEST_VERSION, decoder.Decode(stream).LegacyVersion); + Assert.AreEqual(SkinConfiguration.LATEST_VERSION, decoder.Decode(stream).LegacyVersion); } [Test] diff --git a/osu.Game.Tests/Skins/TestSceneSkinConfigurationLookup.cs b/osu.Game.Tests/Skins/TestSceneSkinConfigurationLookup.cs index aadabec100..870d6d8f57 100644 --- a/osu.Game.Tests/Skins/TestSceneSkinConfigurationLookup.cs +++ b/osu.Game.Tests/Skins/TestSceneSkinConfigurationLookup.cs @@ -151,7 +151,7 @@ namespace osu.Game.Tests.Skins { AddStep("Set user skin version 2.3", () => userSource.Configuration.LegacyVersion = 2.3m); AddStep("Set beatmap skin version null", () => beatmapSource.Configuration.LegacyVersion = null); - AddAssert("Check legacy version lookup", () => requester.GetConfig(LegacySkinConfiguration.LegacySetting.Version)?.Value == 2.3m); + AddAssert("Check legacy version lookup", () => requester.GetConfig(SkinConfiguration.LegacySetting.Version)?.Value == 2.3m); } [Test] @@ -160,7 +160,7 @@ namespace osu.Game.Tests.Skins // completely ignoring beatmap versions for simplicity. AddStep("Set user skin version 2.3", () => userSource.Configuration.LegacyVersion = 2.3m); AddStep("Set beatmap skin version null", () => beatmapSource.Configuration.LegacyVersion = 1.7m); - AddAssert("Check legacy version lookup", () => requester.GetConfig(LegacySkinConfiguration.LegacySetting.Version)?.Value == 2.3m); + AddAssert("Check legacy version lookup", () => requester.GetConfig(SkinConfiguration.LegacySetting.Version)?.Value == 2.3m); } [Test] @@ -169,14 +169,14 @@ namespace osu.Game.Tests.Skins AddStep("Set user skin version 2.3", () => userSource.Configuration.LegacyVersion = null); AddStep("Set beatmap skin version null", () => beatmapSource.Configuration.LegacyVersion = null); AddAssert("Check legacy version lookup", - () => requester.GetConfig(LegacySkinConfiguration.LegacySetting.Version)?.Value == LegacySkinConfiguration.LATEST_VERSION); + () => requester.GetConfig(SkinConfiguration.LegacySetting.Version)?.Value == SkinConfiguration.LATEST_VERSION); } [Test] public void TestIniWithNoVersionFallsBackTo1() { AddStep("Parse skin with no version", () => userSource.Configuration = new LegacySkinDecoder().Decode(new LineBufferedReader(new MemoryStream()))); - AddAssert("Check legacy version lookup", () => requester.GetConfig(LegacySkinConfiguration.LegacySetting.Version)?.Value == 1.0m); + AddAssert("Check legacy version lookup", () => requester.GetConfig(SkinConfiguration.LegacySetting.Version)?.Value == 1.0m); } public enum LookupType diff --git a/osu.Game/Rulesets/Objects/Legacy/ConvertHitObjectParser.cs b/osu.Game/Rulesets/Objects/Legacy/ConvertHitObjectParser.cs index 0942a7264d..d902918e83 100644 --- a/osu.Game/Rulesets/Objects/Legacy/ConvertHitObjectParser.cs +++ b/osu.Game/Rulesets/Objects/Legacy/ConvertHitObjectParser.cs @@ -481,7 +481,7 @@ namespace osu.Game.Rulesets.Objects.Legacy /// /// /// Layered hit samples are automatically added in all modes (except osu!mania), but can be disabled - /// using the skin config option. + /// using the skin config option. /// public readonly bool IsLayered; diff --git a/osu.Game/Skinning/DefaultSkin.cs b/osu.Game/Skinning/DefaultSkin.cs index 8e03bddb4d..d972ce3af6 100644 --- a/osu.Game/Skinning/DefaultSkin.cs +++ b/osu.Game/Skinning/DefaultSkin.cs @@ -35,7 +35,6 @@ namespace osu.Game.Skinning : base(skin, resources) { this.resources = resources; - Configuration = new DefaultSkinConfiguration(); } public override Texture GetTexture(string componentName, WrapMode wrapModeS, WrapMode wrapModeT) => null; diff --git a/osu.Game/Skinning/DefaultSkinConfiguration.cs b/osu.Game/Skinning/DefaultSkinConfiguration.cs deleted file mode 100644 index 5842ee82ee..0000000000 --- a/osu.Game/Skinning/DefaultSkinConfiguration.cs +++ /dev/null @@ -1,12 +0,0 @@ -// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. -// See the LICENCE file in the repository root for full licence text. - -namespace osu.Game.Skinning -{ - /// - /// A skin configuration pre-populated with sane defaults. - /// - public class DefaultSkinConfiguration : SkinConfiguration - { - } -} diff --git a/osu.Game/Skinning/LegacyBeatmapSkin.cs b/osu.Game/Skinning/LegacyBeatmapSkin.cs index 2093182dcc..8720a55076 100644 --- a/osu.Game/Skinning/LegacyBeatmapSkin.cs +++ b/osu.Game/Skinning/LegacyBeatmapSkin.cs @@ -50,7 +50,7 @@ namespace osu.Game.Skinning { switch (lookup) { - case LegacySkinConfiguration.LegacySetting s when s == LegacySkinConfiguration.LegacySetting.Version: + case SkinConfiguration.LegacySetting s when s == SkinConfiguration.LegacySetting.Version: // For lookup simplicity, ignore beatmap-level versioning completely. // If it is decided that we need this due to beatmaps somehow using it, the default (1.0 specified in LegacySkinDecoder.CreateTemplateObject) diff --git a/osu.Game/Skinning/LegacySkin.cs b/osu.Game/Skinning/LegacySkin.cs index 5ab3ef84db..6facb6fafd 100644 --- a/osu.Game/Skinning/LegacySkin.cs +++ b/osu.Game/Skinning/LegacySkin.cs @@ -47,12 +47,6 @@ namespace osu.Game.Skinning /// protected virtual bool UseCustomSampleBanks => false; - public new LegacySkinConfiguration Configuration - { - get => base.Configuration as LegacySkinConfiguration; - set => base.Configuration = value; - } - private readonly Dictionary maniaConfigurations = new Dictionary(); [UsedImplicitly(ImplicitUseKindFlags.InstantiatedWithFixedConstructorSignature)] @@ -81,26 +75,8 @@ namespace osu.Game.Skinning /// Access to raw game resources. /// An optional stream containing the contents of a skin.ini file. protected LegacySkin(SkinInfo skin, [CanBeNull] IResourceStore storage, [CanBeNull] IStorageResourceProvider resources, [CanBeNull] Stream configurationStream) - : base(skin, resources) + : base(skin, resources, configurationStream) { - if (configurationStream != null) - { - using (LineBufferedReader reader = new LineBufferedReader(configurationStream, true)) - Configuration = new LegacySkinDecoder().Decode(reader); - - configurationStream.Seek(0, SeekOrigin.Begin); - - using (LineBufferedReader reader = new LineBufferedReader(configurationStream)) - { - var maniaList = new LegacyManiaSkinDecoder().Decode(reader); - - foreach (var config in maniaList) - maniaConfigurations[config.Keys] = config; - } - } - else - Configuration = new LegacySkinConfiguration(); - if (storage != null) { var samples = resources?.AudioManager?.GetSampleStore(storage); @@ -119,6 +95,21 @@ namespace osu.Game.Skinning true) != null); } + protected override void ParseConfigurationStream(Stream stream) + { + base.ParseConfigurationStream(stream); + + stream.Seek(0, SeekOrigin.Begin); + + using (LineBufferedReader reader = new LineBufferedReader(stream)) + { + var maniaList = new LegacyManiaSkinDecoder().Decode(reader); + + foreach (var config in maniaList) + maniaConfigurations[config.Keys] = config; + } + } + public override IBindable GetConfig(TLookup lookup) { switch (lookup) @@ -155,7 +146,7 @@ namespace osu.Game.Skinning break; - case LegacySkinConfiguration.LegacySetting legacy: + case SkinConfiguration.LegacySetting legacy: return legacySettingLookup(legacy); default: @@ -198,7 +189,7 @@ namespace osu.Game.Skinning case LegacyManiaSkinConfigurationLookups.ExplosionScale: Debug.Assert(maniaLookup.TargetColumn != null); - if (GetConfig(LegacySkinConfiguration.LegacySetting.Version)?.Value < 2.5m) + if (GetConfig(SkinConfiguration.LegacySetting.Version)?.Value < 2.5m) return SkinUtils.As(new Bindable(1)); if (existing.ExplosionWidth[maniaLookup.TargetColumn.Value] != 0) @@ -245,7 +236,7 @@ namespace osu.Game.Skinning case LegacyManiaSkinConfigurationLookups.HoldNoteLightScale: Debug.Assert(maniaLookup.TargetColumn != null); - if (GetConfig(LegacySkinConfiguration.LegacySetting.Version)?.Value < 2.5m) + if (GetConfig(SkinConfiguration.LegacySetting.Version)?.Value < 2.5m) return SkinUtils.As(new Bindable(1)); if (existing.HoldNoteLightWidth[maniaLookup.TargetColumn.Value] != 0) @@ -318,15 +309,15 @@ namespace osu.Game.Skinning => source.ImageLookups.TryGetValue(lookup, out var image) ? new Bindable(image) : null; [CanBeNull] - private IBindable legacySettingLookup(LegacySkinConfiguration.LegacySetting legacySetting) + private IBindable legacySettingLookup(SkinConfiguration.LegacySetting legacySetting) { switch (legacySetting) { - case LegacySkinConfiguration.LegacySetting.Version: - return SkinUtils.As(new Bindable(Configuration.LegacyVersion ?? LegacySkinConfiguration.LATEST_VERSION)); + case SkinConfiguration.LegacySetting.Version: + return SkinUtils.As(new Bindable(Configuration.LegacyVersion ?? SkinConfiguration.LATEST_VERSION)); default: - return genericLookup(legacySetting); + return genericLookup(legacySetting); } } diff --git a/osu.Game/Skinning/LegacySkinConfiguration.cs b/osu.Game/Skinning/LegacySkinConfiguration.cs deleted file mode 100644 index 20d1da8aaa..0000000000 --- a/osu.Game/Skinning/LegacySkinConfiguration.cs +++ /dev/null @@ -1,28 +0,0 @@ -// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. -// See the LICENCE file in the repository root for full licence text. - -namespace osu.Game.Skinning -{ - public class LegacySkinConfiguration : SkinConfiguration - { - public const decimal LATEST_VERSION = 2.7m; - - /// - /// Legacy version of this skin. - /// - public decimal? LegacyVersion { get; internal set; } - - public enum LegacySetting - { - Version, - ComboPrefix, - ComboOverlap, - ScorePrefix, - ScoreOverlap, - HitCirclePrefix, - HitCircleOverlap, - AnimationFramerate, - LayeredHitSounds - } - } -} diff --git a/osu.Game/Skinning/LegacySkinDecoder.cs b/osu.Game/Skinning/LegacySkinDecoder.cs index 2700f84815..dd8a9dedb2 100644 --- a/osu.Game/Skinning/LegacySkinDecoder.cs +++ b/osu.Game/Skinning/LegacySkinDecoder.cs @@ -6,14 +6,14 @@ using osu.Game.Beatmaps.Formats; namespace osu.Game.Skinning { - public class LegacySkinDecoder : LegacyDecoder + public class LegacySkinDecoder : LegacyDecoder { public LegacySkinDecoder() : base(1) { } - protected override void ParseLine(LegacySkinConfiguration skin, Section section, string line) + protected override void ParseLine(SkinConfiguration skin, Section section, string line) { if (section != Section.Colours) { @@ -34,7 +34,7 @@ namespace osu.Game.Skinning case @"Version": if (pair.Value == "latest") - skin.LegacyVersion = LegacySkinConfiguration.LATEST_VERSION; + skin.LegacyVersion = SkinConfiguration.LATEST_VERSION; else if (decimal.TryParse(pair.Value, NumberStyles.AllowDecimalPoint, CultureInfo.InvariantCulture, out var version)) skin.LegacyVersion = version; @@ -57,7 +57,7 @@ namespace osu.Game.Skinning base.ParseLine(skin, section, line); } - protected override LegacySkinConfiguration CreateTemplateObject() + protected override SkinConfiguration CreateTemplateObject() { var config = base.CreateTemplateObject(); config.LegacyVersion = 1.0m; diff --git a/osu.Game/Skinning/LegacySkinExtensions.cs b/osu.Game/Skinning/LegacySkinExtensions.cs index fd1f905868..479afabb00 100644 --- a/osu.Game/Skinning/LegacySkinExtensions.cs +++ b/osu.Game/Skinning/LegacySkinExtensions.cs @@ -12,7 +12,7 @@ using osu.Framework.Graphics.Animations; using osu.Framework.Graphics.OpenGL.Textures; using osu.Framework.Graphics.Sprites; using osu.Framework.Graphics.Textures; -using static osu.Game.Skinning.LegacySkinConfiguration; +using static osu.Game.Skinning.SkinConfiguration; namespace osu.Game.Skinning { diff --git a/osu.Game/Skinning/LegacySkinTransformer.cs b/osu.Game/Skinning/LegacySkinTransformer.cs index 92b7a04dee..97084f34e0 100644 --- a/osu.Game/Skinning/LegacySkinTransformer.cs +++ b/osu.Game/Skinning/LegacySkinTransformer.cs @@ -10,7 +10,7 @@ using osu.Framework.Graphics.OpenGL.Textures; using osu.Framework.Graphics.Textures; using osu.Game.Audio; using osu.Game.Rulesets.Objects.Legacy; -using static osu.Game.Skinning.LegacySkinConfiguration; +using static osu.Game.Skinning.SkinConfiguration; namespace osu.Game.Skinning { diff --git a/osu.Game/Skinning/Skin.cs b/osu.Game/Skinning/Skin.cs index 92441f40da..d5f41f01a4 100644 --- a/osu.Game/Skinning/Skin.cs +++ b/osu.Game/Skinning/Skin.cs @@ -3,8 +3,10 @@ using System; using System.Collections.Generic; +using System.IO; using System.Linq; using System.Text; +using JetBrains.Annotations; using Newtonsoft.Json; using osu.Framework.Audio.Sample; using osu.Framework.Bindables; @@ -21,8 +23,9 @@ namespace osu.Game.Skinning public abstract class Skin : IDisposable, ISkin { public readonly SkinInfo SkinInfo; + private readonly IStorageResourceProvider resources; - public SkinConfiguration Configuration { get; protected set; } + public SkinConfiguration Configuration { get; set; } public IDictionary DrawableComponentInfo => drawableComponentInfo; @@ -36,9 +39,17 @@ namespace osu.Game.Skinning public abstract IBindable GetConfig(TLookup lookup); - protected Skin(SkinInfo skin, IStorageResourceProvider resources) + protected Skin(SkinInfo skin, IStorageResourceProvider resources, [CanBeNull] Stream configurationStream = null) { SkinInfo = skin; + this.resources = resources; + + configurationStream ??= getConfigurationStream(); + + if (configurationStream != null) + ParseConfigurationStream(configurationStream); + else + Configuration = new SkinConfiguration(); // we may want to move this to some kind of async operation in the future. foreach (SkinnableTarget skinnableTarget in Enum.GetValues(typeof(SkinnableTarget))) @@ -73,6 +84,22 @@ namespace osu.Game.Skinning } } + protected virtual void ParseConfigurationStream(Stream stream) + { + using (LineBufferedReader reader = new LineBufferedReader(stream, true)) + Configuration = new LegacySkinDecoder().Decode(reader); + } + + private Stream getConfigurationStream() + { + var path = SkinInfo.Files.SingleOrDefault(f => f.Filename == "skin.ini")?.FileInfo.StoragePath; + + if (string.IsNullOrEmpty(path)) + return null; + + return resources?.Files.GetStream(path); + } + /// /// Remove all stored customisations for the provided target. /// diff --git a/osu.Game/Skinning/SkinConfiguration.cs b/osu.Game/Skinning/SkinConfiguration.cs index a18144246f..f71f6811e8 100644 --- a/osu.Game/Skinning/SkinConfiguration.cs +++ b/osu.Game/Skinning/SkinConfiguration.cs @@ -14,11 +14,31 @@ namespace osu.Game.Skinning { public readonly SkinInfo SkinInfo = new SkinInfo(); + public const decimal LATEST_VERSION = 2.7m; + /// /// Whether to allow as a fallback list for when no combo colours are provided. /// internal bool AllowDefaultComboColoursFallback = true; + /// + /// Legacy version of this skin. + /// + public decimal? LegacyVersion { get; internal set; } + + public enum LegacySetting + { + Version, + ComboPrefix, + ComboOverlap, + ScorePrefix, + ScoreOverlap, + HitCirclePrefix, + HitCircleOverlap, + AnimationFramerate, + LayeredHitSounds + } + public static List DefaultComboColours { get; } = new List { new Color4(255, 192, 0, 255), diff --git a/osu.Game/Skinning/SkinManager.cs b/osu.Game/Skinning/SkinManager.cs index 707afade09..177dbaf28f 100644 --- a/osu.Game/Skinning/SkinManager.cs +++ b/osu.Game/Skinning/SkinManager.cs @@ -183,7 +183,7 @@ namespace osu.Game.Skinning // By this point, the metadata in SkinInfo will be correct. // Regardless of whether this is an import or not, let's write the skin.ini if non-existing or non-matching. // This is (weirdly) done inside ComputeHash to avoid adding a new method to handle this case. After switching to realm it can be moved into another place. - if (instance is LegacySkin && skinIniSourcedName != item.Name) + if (skinIniSourcedName != item.Name) updateSkinIniMetadata(item); return base.ComputeHash(item, reader); From eef9949a0a928f3d10df97375119ccc1fbdbbf90 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 22 Oct 2021 14:48:20 +0900 Subject: [PATCH 17/20] Remove unnecessary branching around EF logic --- osu.Game/Database/ArchiveModelManager.cs | 48 +++++++----------------- 1 file changed, 14 insertions(+), 34 deletions(-) diff --git a/osu.Game/Database/ArchiveModelManager.cs b/osu.Game/Database/ArchiveModelManager.cs index 0bc4c40459..ad5bc6d420 100644 --- a/osu.Game/Database/ArchiveModelManager.cs +++ b/osu.Game/Database/ArchiveModelManager.cs @@ -511,29 +511,21 @@ namespace osu.Game.Database /// The existing file to be deleted. public void DeleteFile(TModel model, TFileModel file) { - if (model.ID > 0) + using (var usage = ContextFactory.GetForWrite()) { - using (var usage = ContextFactory.GetForWrite()) + // Dereference the existing file info, since the file model will be removed. + if (file.FileInfo != null) { - // Dereference the existing file info, since the file model will be removed. - if (file.FileInfo != null) + Files.Dereference(file.FileInfo); + + if (file.ID > 0) { - Files.Dereference(file.FileInfo); - - if (file.ID > 0) - { - // This shouldn't be required, but here for safety in case the provided TModel is not being change tracked - // Definitely can be removed once we rework the database backend. - usage.Context.Set().Remove(file); - } + // This shouldn't be required, but here for safety in case the provided TModel is not being change tracked + // Definitely can be removed once we rework the database backend. + usage.Context.Set().Remove(file); } - - model.Files.Remove(file); } - } - else - { - Files.Dereference(file.FileInfo); + model.Files.Remove(file); } } @@ -546,29 +538,17 @@ namespace osu.Game.Database /// The filename for the new file. public void AddFile(TModel model, Stream contents, string filename) { - if (model.ID > 0) + using (ContextFactory.GetForWrite()) { - using (ContextFactory.GetForWrite()) - { - model.Files.Add(new TFileModel - { - Filename = filename, - FileInfo = Files.Add(contents) - }); - - Update(model); - } - } - else - { - // This function may be called during the import process. - // Should not exist like this once we have switched to realm. model.Files.Add(new TFileModel { Filename = filename, FileInfo = Files.Add(contents) }); } + + if (model.ID > 0) + Update(model); } /// From 051cc2cd92972c20027e4ef4dd4973f23cc7ac69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Sun, 24 Oct 2021 12:47:17 +0200 Subject: [PATCH 18/20] Update reference to configuration population process in comment --- osu.Game/Skinning/SkinManager.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Skinning/SkinManager.cs b/osu.Game/Skinning/SkinManager.cs index 177dbaf28f..860d09e011 100644 --- a/osu.Game/Skinning/SkinManager.cs +++ b/osu.Game/Skinning/SkinManager.cs @@ -161,7 +161,7 @@ namespace osu.Game.Skinning // This function can be run on fresh import or save. The logic here ensures a skin.ini file is in a good state for both operations. - // LegacySkin will parse the skin.ini and populate `Skin.Configuration` during construction above. + // `Skin` will parse the skin.ini and populate `Skin.Configuration` during construction above. string skinIniSourcedName = instance.Configuration.SkinInfo.Name; string skinIniSourcedCreator = instance.Configuration.SkinInfo.Creator; string archiveName = item.Name.Replace(".osk", "", StringComparison.OrdinalIgnoreCase); From 5303cae0444064e8696a8db979273ca6730155ee Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Sun, 24 Oct 2021 23:43:37 +0900 Subject: [PATCH 19/20] Add mention of stream close logic --- osu.Game/Skinning/Skin.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/osu.Game/Skinning/Skin.cs b/osu.Game/Skinning/Skin.cs index d5f41f01a4..823a5d5133 100644 --- a/osu.Game/Skinning/Skin.cs +++ b/osu.Game/Skinning/Skin.cs @@ -47,6 +47,7 @@ namespace osu.Game.Skinning configurationStream ??= getConfigurationStream(); if (configurationStream != null) + // stream will be closed after use by LineBufferedReader. ParseConfigurationStream(configurationStream); else Configuration = new SkinConfiguration(); From 26cf5370c35bf977388eaf651503d783746bb34b Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Sun, 24 Oct 2021 23:48:46 +0900 Subject: [PATCH 20/20] Remove unused `reader` parameter --- osu.Game/Database/ArchiveModelManager.cs | 4 ++-- osu.Game/Skinning/SkinManager.cs | 4 ++-- osu.Game/Tests/Visual/EditorTestScene.cs | 3 +-- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/osu.Game/Database/ArchiveModelManager.cs b/osu.Game/Database/ArchiveModelManager.cs index ad5bc6d420..00e1f03b92 100644 --- a/osu.Game/Database/ArchiveModelManager.cs +++ b/osu.Game/Database/ArchiveModelManager.cs @@ -315,7 +315,7 @@ namespace osu.Game.Database /// /// In the case of no matching files, a hash will be generated from the passed archive's . /// - protected virtual string ComputeHash(TModel item, ArchiveReader reader = null) + protected virtual string ComputeHash(TModel item) { var hashableFiles = item.Files .Where(f => HashableFileTypes.Any(ext => f.Filename.EndsWith(ext, StringComparison.OrdinalIgnoreCase))) @@ -397,7 +397,7 @@ namespace osu.Game.Database LogForModel(item, @"Beginning import..."); item.Files = archive != null ? createFileInfos(archive, Files) : new List(); - item.Hash = ComputeHash(item, archive); + item.Hash = ComputeHash(item); await Populate(item, archive, cancellationToken).ConfigureAwait(false); diff --git a/osu.Game/Skinning/SkinManager.cs b/osu.Game/Skinning/SkinManager.cs index 860d09e011..2187d2d875 100644 --- a/osu.Game/Skinning/SkinManager.cs +++ b/osu.Game/Skinning/SkinManager.cs @@ -155,7 +155,7 @@ namespace osu.Game.Skinning protected override bool HasCustomHashFunction => true; - protected override string ComputeHash(SkinInfo item, ArchiveReader reader = null) + protected override string ComputeHash(SkinInfo item) { var instance = GetSkin(item); @@ -186,7 +186,7 @@ namespace osu.Game.Skinning if (skinIniSourcedName != item.Name) updateSkinIniMetadata(item); - return base.ComputeHash(item, reader); + return base.ComputeHash(item); } private void updateSkinIniMetadata(SkinInfo item) diff --git a/osu.Game/Tests/Visual/EditorTestScene.cs b/osu.Game/Tests/Visual/EditorTestScene.cs index 798b0d01ee..4b02306d33 100644 --- a/osu.Game/Tests/Visual/EditorTestScene.cs +++ b/osu.Game/Tests/Visual/EditorTestScene.cs @@ -10,7 +10,6 @@ using osu.Framework.Platform; using osu.Framework.Testing; using osu.Game.Beatmaps; using osu.Game.Database; -using osu.Game.IO.Archives; using osu.Game.Online.API; using osu.Game.Rulesets; using osu.Game.Rulesets.Edit; @@ -154,7 +153,7 @@ namespace osu.Game.Tests.Visual { } - protected override string ComputeHash(BeatmapSetInfo item, ArchiveReader reader = null) + protected override string ComputeHash(BeatmapSetInfo item) => string.Empty; }