Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use temporary storage for file exports on iOS #31039

Merged
merged 4 commits into from
Dec 11, 2024

Conversation

frenzibyte
Copy link
Member

Usually on iOS the user is explicitly asked where to store every exported file, storing exports permanently will clog up the user's phone storage. I've tried to approach this with as less changes as possible.

@frenzibyte frenzibyte self-assigned this Dec 9, 2024
@peppy peppy self-requested a review December 10, 2024 04:47
Comment on lines 51 to 52
if (storage is OsuStorage osuStorage)
exportStorage = osuStorage.GetExportStorage();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why you've change it this way. Can you just add this at the single usage rather than making Storage nullable? (will allow the failing test to still work).

diff --git a/osu.Game/Database/LegacyExporter.cs b/osu.Game/Database/LegacyExporter.cs
index 193887765d..6f46122e86 100644
--- a/osu.Game/Database/LegacyExporter.cs
+++ b/osu.Game/Database/LegacyExporter.cs
@@ -3,7 +3,6 @@
 
 using System;
 using System.Collections.Generic;
-using System.Diagnostics;
 using System.IO;
 using System.Linq;
 using System.Threading;
@@ -42,14 +41,13 @@ public abstract class LegacyExporter<TModel>
         protected abstract string FileExtension { get; }
 
         protected readonly Storage UserFileStorage;
-        private readonly Storage? exportStorage;
+        private readonly Storage exportStorage;
 
         public Action<Notification>? PostNotification { get; set; }
 
         protected LegacyExporter(Storage storage)
         {
-            if (storage is OsuStorage osuStorage)
-                exportStorage = osuStorage.GetExportStorage();
+            exportStorage = (storage as OsuStorage)?.GetExportStorage() ?? storage.GetStorageForDirectory("exports");
 
             UserFileStorage = storage.GetStorageForDirectory(@"files");
         }
@@ -72,8 +70,6 @@ protected LegacyExporter(Storage storage)
         /// <param name="cancellationToken">A cancellation token.</param>
         public async Task ExportAsync(Live<TModel> model, CancellationToken cancellationToken = default)
         {
-            Debug.Assert(exportStorage != null);
-
             string itemFilename = model.PerformRead(s => GetFilename(s).GetValidFilename());
 
             if (itemFilename.Length > MAX_FILENAME_LENGTH - FileExtension.Length)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid being this lenient will end up having this silently break, that's why I got a little aggressive on the code there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feel free to propose another direction

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I can think of anything else other than to change the constructor signature of every single exporter class to include a unique storage for "exports". The direction that I've went with here avoids that under the assumption that game data storage generally comes in the form of OsuStorage.

We can rename that class to UserDataStorage if it makes this read any better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you're going that far, make the constructors take an OsuStorage and updat all test usages to also provide one.

All seems too much to me for a failure that likely will never happen. I don't know what the silent break you're envisaging is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I was writing about the probability of this breakage, I realised we can quite easily fix it by just publishing a hotfix that cleans up the exports folder at startup and call it a day. I will keep it weakly-typed.

@frenzibyte frenzibyte mentioned this pull request Dec 11, 2024
2 tasks
@peppy peppy merged commit 02c52e4 into ppy:master Dec 11, 2024
8 of 10 checks passed
@frenzibyte frenzibyte deleted the ios-temporary-export-storage branch December 11, 2024 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants