From ae3038ead4afb1329f765eca71dbdfa3dcfc78d0 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 2 Dec 2021 17:17:12 +0900 Subject: [PATCH 1/6] Overwrite existing files if `AddFile` is called with an existing filename --- osu.Game/Database/IModelFileManager.cs | 2 +- osu.Game/Stores/RealmArchiveModelManager.cs | 10 +++++++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/osu.Game/Database/IModelFileManager.cs b/osu.Game/Database/IModelFileManager.cs index 4bc1e2d29b..390be4a69d 100644 --- a/osu.Game/Database/IModelFileManager.cs +++ b/osu.Game/Database/IModelFileManager.cs @@ -25,7 +25,7 @@ namespace osu.Game.Database void DeleteFile(TModel model, TFileModel file); /// - /// Add a new file. + /// Add a new file. If the file already exists, it is overwritten. /// /// The item to operate on. /// The new file contents. diff --git a/osu.Game/Stores/RealmArchiveModelManager.cs b/osu.Game/Stores/RealmArchiveModelManager.cs index a916e2b53a..4efcdd096c 100644 --- a/osu.Game/Stores/RealmArchiveModelManager.cs +++ b/osu.Game/Stores/RealmArchiveModelManager.cs @@ -74,10 +74,18 @@ namespace osu.Game.Stores } /// - /// Add a file from within an ongoing realm transaction. + /// Add a file from within an ongoing realm transaction. If the file already exists, it is overwritten. /// protected void AddFile(TModel item, Stream stream, string filename, Realm realm) { + var existing = item.Files.FirstOrDefault(f => string.Equals(f.Filename, filename, StringComparison.OrdinalIgnoreCase)); + + if (existing != null) + { + ReplaceFile(item, existing, stream, realm); + return; + } + var file = realmFileStore.Add(stream, realm); var namedUsage = new RealmNamedFileUsage(file, filename); From fe99d4e984c7fad82bfb12e54ebcbd03d1835f55 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 2 Dec 2021 17:19:53 +0900 Subject: [PATCH 2/6] Standardise parameter naming across all file IO methods --- osu.Game/Skinning/SkinModelManager.cs | 4 ++-- osu.Game/Stores/RealmArchiveModelManager.cs | 14 +++++++------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/osu.Game/Skinning/SkinModelManager.cs b/osu.Game/Skinning/SkinModelManager.cs index 059345f9bc..40c280c5c2 100644 --- a/osu.Game/Skinning/SkinModelManager.cs +++ b/osu.Game/Skinning/SkinModelManager.cs @@ -128,7 +128,7 @@ namespace osu.Game.Skinning sw.WriteLine(line); } - ReplaceFile(item, existingFile, stream, realm); + ReplaceFile(existingFile, stream, realm); // can be removed 20220502. if (!ensureIniWasUpdated(item)) @@ -214,7 +214,7 @@ namespace osu.Game.Skinning var oldFile = s.Files.FirstOrDefault(f => f.Filename == filename); if (oldFile != null) - ReplaceFile(s, oldFile, streamContent, s.Realm); + ReplaceFile(oldFile, streamContent, s.Realm); else AddFile(s, streamContent, filename, s.Realm); } diff --git a/osu.Game/Stores/RealmArchiveModelManager.cs b/osu.Game/Stores/RealmArchiveModelManager.cs index 4efcdd096c..87a27cbbbc 100644 --- a/osu.Game/Stores/RealmArchiveModelManager.cs +++ b/osu.Game/Stores/RealmArchiveModelManager.cs @@ -52,10 +52,10 @@ namespace osu.Game.Stores item.Realm.Write(() => DeleteFile(item, file, item.Realm)); public void ReplaceFile(TModel item, RealmNamedFileUsage file, Stream contents) - => item.Realm.Write(() => ReplaceFile(item, file, contents, item.Realm)); + => item.Realm.Write(() => ReplaceFile(file, contents, item.Realm)); - public void AddFile(TModel item, Stream stream, string filename) - => item.Realm.Write(() => AddFile(item, stream, filename, item.Realm)); + public void AddFile(TModel item, Stream contents, string filename) + => item.Realm.Write(() => AddFile(item, contents, filename, item.Realm)); /// /// Delete a file from within an ongoing realm transaction. @@ -68,7 +68,7 @@ namespace osu.Game.Stores /// /// Replace a file from within an ongoing realm transaction. /// - protected void ReplaceFile(TModel model, RealmNamedFileUsage file, Stream contents, Realm realm) + protected void ReplaceFile(RealmNamedFileUsage file, Stream contents, Realm realm) { file.File = realmFileStore.Add(contents, realm); } @@ -76,17 +76,17 @@ namespace osu.Game.Stores /// /// Add a file from within an ongoing realm transaction. If the file already exists, it is overwritten. /// - protected void AddFile(TModel item, Stream stream, string filename, Realm realm) + protected void AddFile(TModel item, Stream contents, string filename, Realm realm) { var existing = item.Files.FirstOrDefault(f => string.Equals(f.Filename, filename, StringComparison.OrdinalIgnoreCase)); if (existing != null) { - ReplaceFile(item, existing, stream, realm); + ReplaceFile(existing, contents, realm); return; } - var file = realmFileStore.Add(stream, realm); + var file = realmFileStore.Add(contents, realm); var namedUsage = new RealmNamedFileUsage(file, filename); item.Files.Add(namedUsage); From 0e82e9355bd0072979459e11454fcfdd2f926c88 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 2 Dec 2021 17:42:16 +0900 Subject: [PATCH 3/6] Ensure skin is saved immediately after becoming mutable Without doing this, the JSON content is not written to the file. A user assumption is that as soon as a skin shows up in the skin list as exportable, it should export correctly, so it makes sense that it should be in a sane state even if the user has not made any changes in the skin editor yet. Going forward, we might move more of the json serialisation logic out, and run for consistency as part of the import process. This seems like the simplest way to guarantee things for now, though. --- osu.Game/Skinning/SkinManager.cs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/osu.Game/Skinning/SkinManager.cs b/osu.Game/Skinning/SkinManager.cs index 701bf6d0cf..5134632fb1 100644 --- a/osu.Game/Skinning/SkinManager.cs +++ b/osu.Game/Skinning/SkinManager.cs @@ -156,7 +156,13 @@ namespace osu.Game.Skinning }).Result; if (result != null) + { + // save once to ensure the required json content is populated. + // currently this only happens on save. + result.PerformRead(skin => Save(skin.CreateInstance(this))); + CurrentSkinInfo.Value = result; + } }); } From b9768487742c254866f4460adc808cc580cd6a16 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 2 Dec 2021 18:00:04 +0900 Subject: [PATCH 4/6] Add failing test coverage of exporting default skin importing with incorrect type --- osu.Game.Tests/Skins/IO/ImportSkinTest.cs | 68 +++++++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/osu.Game.Tests/Skins/IO/ImportSkinTest.cs b/osu.Game.Tests/Skins/IO/ImportSkinTest.cs index f3c2cd8ba2..f2ce002650 100644 --- a/osu.Game.Tests/Skins/IO/ImportSkinTest.cs +++ b/osu.Game.Tests/Skins/IO/ImportSkinTest.cs @@ -164,6 +164,74 @@ namespace osu.Game.Tests.Skins.IO assertCorrectMetadata(import2, "name 1 [my custom skin 2]", "author 1", osu); }); + [Test] + public Task TestExportThenImportDefaultSkin() => runSkinTest(osu => + { + var skinManager = osu.Dependencies.Get(); + + skinManager.EnsureMutableSkin(); + + MemoryStream exportStream = new MemoryStream(); + + Guid originalSkinId = skinManager.CurrentSkinInfo.Value.ID; + + skinManager.CurrentSkinInfo.Value.PerformRead(s => + { + Assert.IsFalse(s.Protected); + Assert.AreEqual(typeof(DefaultSkin), s.CreateInstance(skinManager).GetType()); + + new LegacySkinExporter(osu.Dependencies.Get()).ExportModelTo(s, exportStream); + + Assert.Greater(exportStream.Length, 0); + }); + + var imported = skinManager.Import(new ImportTask(exportStream, "exported.osk")); + + imported.Result.PerformRead(s => + { + Assert.IsFalse(s.Protected); + Assert.AreNotEqual(originalSkinId, s.ID); + Assert.AreEqual(typeof(DefaultSkin), s.CreateInstance(skinManager).GetType()); + }); + + return Task.CompletedTask; + }); + + [Test] + public Task TestExportThenImportClassicSkin() => runSkinTest(osu => + { + var skinManager = osu.Dependencies.Get(); + + skinManager.CurrentSkinInfo.Value = skinManager.DefaultLegacySkin.SkinInfo; + + skinManager.EnsureMutableSkin(); + + MemoryStream exportStream = new MemoryStream(); + + Guid originalSkinId = skinManager.CurrentSkinInfo.Value.ID; + + skinManager.CurrentSkinInfo.Value.PerformRead(s => + { + Assert.IsFalse(s.Protected); + Assert.AreEqual(typeof(DefaultLegacySkin), s.CreateInstance(skinManager).GetType()); + + new LegacySkinExporter(osu.Dependencies.Get()).ExportModelTo(s, exportStream); + + Assert.Greater(exportStream.Length, 0); + }); + + var imported = skinManager.Import(new ImportTask(exportStream, "exported.osk")); + + imported.Result.PerformRead(s => + { + Assert.IsFalse(s.Protected); + Assert.AreNotEqual(originalSkinId, s.ID); + Assert.AreEqual(typeof(DefaultLegacySkin), s.CreateInstance(skinManager).GetType()); + }); + + return Task.CompletedTask; + }); + #endregion private void assertCorrectMetadata(ILive import1, string name, string creator, OsuGameBase osu) From cdf2fa99304759414ff11db461508ce32c3f616f Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 2 Dec 2021 17:43:54 +0900 Subject: [PATCH 5/6] Serialise and deserialise `SkinInfo.InstantiationInfo` to allow for more correct imports Until now, skins were always imported using the `LegacySkin` instantiation type. For cases where a user has edited the lazer or classic default (via the new skin editor), which would result in incorrect fallback paths after exporting and importing the edited skin. --- osu.Game/Skinning/SkinModelManager.cs | 43 +++++++++++++++++++++++++-- 1 file changed, 41 insertions(+), 2 deletions(-) diff --git a/osu.Game/Skinning/SkinModelManager.cs b/osu.Game/Skinning/SkinModelManager.cs index 40c280c5c2..822cb8efa0 100644 --- a/osu.Game/Skinning/SkinModelManager.cs +++ b/osu.Game/Skinning/SkinModelManager.cs @@ -24,6 +24,8 @@ namespace osu.Game.Skinning { public class SkinModelManager : RealmArchiveModelManager { + private const string skin_info_file = "skininfo.json"; + private readonly IStorageResourceProvider skinResources; public SkinModelManager(Storage storage, RealmContextFactory contextFactory, GameHost host, IStorageResourceProvider skinResources) @@ -49,8 +51,36 @@ namespace osu.Game.Skinning protected override Task Populate(SkinInfo model, ArchiveReader? archive, Realm realm, CancellationToken cancellationToken = default) { - if (string.IsNullOrEmpty(model.InstantiationInfo)) - model.InstantiationInfo = createInstance(model).GetType().GetInvariantInstantiationInfo(); + var skinInfoFile = model.Files.SingleOrDefault(f => f.Filename == skin_info_file); + + if (skinInfoFile != null) + { + try + { + using (var existingStream = Files.Storage.GetStream(skinInfoFile.File.GetStoragePath())) + using (var reader = new StreamReader(existingStream)) + { + var deserialisedSkinInfo = JsonConvert.DeserializeObject(reader.ReadToEnd()); + + if (deserialisedSkinInfo != null) + { + // for now we only care about the instantiation info. + // eventually we probably want to transfer everything across. + model.InstantiationInfo = deserialisedSkinInfo.InstantiationInfo; + } + } + } + catch (Exception e) + { + LogForModel(model, $"Error during {skin_info_file} parsing, falling back to default", e); + + // Not sure if we should still run the import in the case of failure here, but let's do so for now. + model.InstantiationInfo = string.Empty; + } + } + + // Always rewrite instantiation info (even after parsing in from the skin json) for sanity. + model.InstantiationInfo = createInstance(model).GetType().GetInvariantInstantiationInfo(); checkSkinIniMetadata(model, realm); @@ -203,6 +233,15 @@ namespace osu.Game.Skinning { skin.SkinInfo.PerformWrite(s => { + // Serialise out the SkinInfo itself. + string skinInfoJson = JsonConvert.SerializeObject(s, new JsonSerializerSettings { Formatting = Formatting.Indented }); + + using (var streamContent = new MemoryStream(Encoding.UTF8.GetBytes(skinInfoJson))) + { + AddFile(s, streamContent, skin_info_file, s.Realm); + } + + // Then serialise each of the drawable component groups into respective files. foreach (var drawableInfo in skin.DrawableComponentInfo) { string json = JsonConvert.SerializeObject(drawableInfo.Value, new JsonSerializerSettings { Formatting = Formatting.Indented }); From e4b296e16ee7b144e0529c7890328c14b7eb5389 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 3 Dec 2021 16:36:27 +0900 Subject: [PATCH 6/6] Use `OptIn` serialisation on `SkinInfo` to avoid writing unnecessary information --- osu.Game/Skinning/SkinInfo.cs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/osu.Game/Skinning/SkinInfo.cs b/osu.Game/Skinning/SkinInfo.cs index 9a82964933..20c66a23a6 100644 --- a/osu.Game/Skinning/SkinInfo.cs +++ b/osu.Game/Skinning/SkinInfo.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using Newtonsoft.Json; using osu.Framework.Extensions.ObjectExtensions; using osu.Framework.Testing; using osu.Game.Database; @@ -16,6 +17,7 @@ namespace osu.Game.Skinning { [ExcludeFromDynamicCompile] [MapTo("Skin")] + [JsonObject(MemberSerialization.OptIn)] public class SkinInfo : RealmObject, IHasRealmFiles, IEquatable, IHasGuidPrimaryKey, ISoftDelete, IHasNamedFiles { internal static readonly Guid DEFAULT_SKIN = new Guid("2991CFD8-2140-469A-BCB9-2EC23FBCE4AD"); @@ -23,18 +25,22 @@ namespace osu.Game.Skinning internal static readonly Guid RANDOM_SKIN = new Guid("D39DFEFB-477C-4372-B1EA-2BCEA5FB8908"); [PrimaryKey] + [JsonProperty] public Guid ID { get; set; } = Guid.NewGuid(); + [JsonProperty] public string Name { get; set; } = string.Empty; + [JsonProperty] public string Creator { get; set; } = string.Empty; + [JsonProperty] + public string InstantiationInfo { get; set; } = string.Empty; + public string Hash { get; set; } = string.Empty; public bool Protected { get; set; } - public string InstantiationInfo { get; set; } = string.Empty; - public virtual Skin CreateInstance(IStorageResourceProvider resources) { var type = string.IsNullOrEmpty(InstantiationInfo)