From 789c715f136254e68cdccce5d986bbfd9a05e1d6 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 20 Oct 2021 15:22:47 +0900 Subject: [PATCH] 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 ///