diff --git a/osu.Game.Tests/Skins/IO/ImportSkinTest.cs b/osu.Game.Tests/Skins/IO/ImportSkinTest.cs index 8b7fcae1a9..c3c10215a5 100644 --- a/osu.Game.Tests/Skins/IO/ImportSkinTest.cs +++ b/osu.Game.Tests/Skins/IO/ImportSkinTest.cs @@ -83,20 +83,20 @@ namespace osu.Game.Tests.Skins.IO #region Cases where imports should match existing [Test] - public Task TestImportTwiceWithSameMetadataAndFilename() => runSkinTest(async osu => + public Task TestImportTwiceWithSameMetadataAndFilename([Values] bool batchImport) => runSkinTest(async osu => { - var import1 = await loadSkinIntoOsu(osu, new ImportTask(createOskWithIni("test skin", "skinner"), "skin.osk")); - var import2 = await loadSkinIntoOsu(osu, new ImportTask(createOskWithIni("test skin", "skinner"), "skin.osk")); + var import1 = await loadSkinIntoOsu(osu, new ImportTask(createOskWithIni("test skin", "skinner"), "skin.osk"), batchImport); + var import2 = await loadSkinIntoOsu(osu, new ImportTask(createOskWithIni("test skin", "skinner"), "skin.osk"), batchImport); assertImportedOnce(import1, import2); }); [Test] - public Task TestImportTwiceWithNoMetadataSameDownloadFilename() => runSkinTest(async osu => + public Task TestImportTwiceWithNoMetadataSameDownloadFilename([Values] bool batchImport) => 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 import1 = await loadSkinIntoOsu(osu, new ImportTask(createOskWithIni(string.Empty, string.Empty), "download.osk")); - var import2 = await loadSkinIntoOsu(osu, new ImportTask(createOskWithIni(string.Empty, string.Empty), "download.osk")); + var import1 = await loadSkinIntoOsu(osu, new ImportTask(createOskWithIni(string.Empty, string.Empty), "download.osk"), batchImport); + var import2 = await loadSkinIntoOsu(osu, new ImportTask(createOskWithIni(string.Empty, string.Empty), "download.osk"), batchImport); assertImportedOnce(import1, import2); }); @@ -134,10 +134,10 @@ namespace osu.Game.Tests.Skins.IO }); [Test] - public Task TestSameMetadataNameSameFolderName() => runSkinTest(async osu => + public Task TestSameMetadataNameSameFolderName([Values] bool batchImport) => runSkinTest(async osu => { - var import1 = await loadSkinIntoOsu(osu, new ImportTask(createOskWithIni("name 1", "author 1"), "my custom skin 1")); - var import2 = await loadSkinIntoOsu(osu, new ImportTask(createOskWithIni("name 1", "author 1"), "my custom skin 1")); + var import1 = await loadSkinIntoOsu(osu, new ImportTask(createOskWithIni("name 1", "author 1"), "my custom skin 1"), batchImport); + var import2 = await loadSkinIntoOsu(osu, new ImportTask(createOskWithIni("name 1", "author 1"), "my custom skin 1"), batchImport); assertImportedOnce(import1, import2); assertCorrectMetadata(import1, "name 1 [my custom skin 1]", "author 1", osu); @@ -357,10 +357,10 @@ namespace osu.Game.Tests.Skins.IO } } - private async Task> loadSkinIntoOsu(OsuGameBase osu, ImportTask import) + private async Task> loadSkinIntoOsu(OsuGameBase osu, ImportTask import, bool batchImport = false) { var skinManager = osu.Dependencies.Get(); - return await skinManager.Import(import); + return await skinManager.Import(import, batchImport); } } } diff --git a/osu.Game/Database/RealmArchiveModelImporter.cs b/osu.Game/Database/RealmArchiveModelImporter.cs index 27cd565d90..6cea92f1d9 100644 --- a/osu.Game/Database/RealmArchiveModelImporter.cs +++ b/osu.Game/Database/RealmArchiveModelImporter.cs @@ -258,15 +258,13 @@ namespace osu.Game.Database { cancellationToken.ThrowIfCancellationRequested(); - bool checkedExisting = false; - TModel? existing = null; + TModel? existing; if (batchImport && archive != null) { // this is a fast bail condition to improve large import performance. item.Hash = computeHashFast(archive); - checkedExisting = true; existing = CheckForExisting(item, realm); if (existing != null) @@ -311,8 +309,12 @@ namespace osu.Game.Database // TODO: we may want to run this outside of the transaction. Populate(item, archive, realm, cancellationToken); - if (!checkedExisting) - existing = CheckForExisting(item, realm); + // Populate() may have adjusted file content (see SkinImporter.updateSkinIniMetadata), so regardless of whether a fast check was done earlier, let's + // check for existing items a second time. + // + // If this is ever a performance issue, the fast-check hash can be compared and trigger a skip of this second check if it still matches. + // I don't think it is a huge deal doing a second indexed check, though. + existing = CheckForExisting(item, realm); if (existing != null) {