Merge pull request #15539 from bdach/display-string

Fix model download notifications not formatting model correctly

osu.Game.Tests/Models/DisplayStringTest.cs

@@ -0,0 +1,124 @@
+// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
+// See the LICENCE file in the repository root for full licence text.
+
+using Moq;
+using NUnit.Framework;
+using osu.Game.Beatmaps;
+using osu.Game.Extensions;
+using osu.Game.Online.API.Requests.Responses;
+using osu.Game.Rulesets;
+using osu.Game.Scoring;
+using osu.Game.Users;
+
+namespace osu.Game.Tests.Models
+{
+    [TestFixture]
+    public class DisplayStringTest
+    {
+        [Test]
+        public void TestBeatmapSet()
+        {
+            var mock = new Mock<IBeatmapSetInfo>();
+
+            mock.Setup(m => m.Metadata.Artist).Returns("artist");
+            mock.Setup(m => m.Metadata.Title).Returns("title");
+            mock.Setup(m => m.Metadata.Author.Username).Returns("author");
+
+            Assert.That(mock.Object.GetDisplayString(), Is.EqualTo("artist - title (author)"));
+        }
+
+        [Test]
+        public void TestBeatmapSetWithNoAuthor()
+        {
+            var mock = new Mock<IBeatmapSetInfo>();
+
+            mock.Setup(m => m.Metadata.Artist).Returns("artist");
+            mock.Setup(m => m.Metadata.Title).Returns("title");
+            mock.Setup(m => m.Metadata.Author.Username).Returns(string.Empty);
+
+            Assert.That(mock.Object.GetDisplayString(), Is.EqualTo("artist - title"));
+        }
+
+        [Test]
+        public void TestBeatmapSetWithNoMetadata()
+        {
+            var mock = new Mock<IBeatmapSetInfo>();
+
+            mock.Setup(m => m.Metadata).Returns(new BeatmapMetadata());
+
+            Assert.That(mock.Object.GetDisplayString(), Is.EqualTo("unknown artist - unknown title"));
+        }
+
+        [Test]
+        public void TestBeatmap()
+        {
+            var mock = new Mock<IBeatmapInfo>();
+
+            mock.Setup(m => m.Metadata.Artist).Returns("artist");
+            mock.Setup(m => m.Metadata.Title).Returns("title");
+            mock.Setup(m => m.Metadata.Author.Username).Returns("author");
+            mock.Setup(m => m.DifficultyName).Returns("difficulty");
+
+            Assert.That(mock.Object.GetDisplayString(), Is.EqualTo("artist - title (author) [difficulty]"));
+        }
+
+        [Test]
+        public void TestMetadata()
+        {
+            var mock = new Mock<IBeatmapMetadataInfo>();
+
+            mock.Setup(m => m.Artist).Returns("artist");
+            mock.Setup(m => m.Title).Returns("title");
+            mock.Setup(m => m.Author.Username).Returns("author");
+
+            Assert.That(mock.Object.GetDisplayString(), Is.EqualTo("artist - title (author)"));
+        }
+
+        [Test]
+        public void TestScore()
+        {
+            var mock = new Mock<IScoreInfo>();
+
+            mock.Setup(m => m.User).Returns(new APIUser { Username = "user" }); // TODO: temporary.
+            mock.Setup(m => m.Beatmap.Metadata.Artist).Returns("artist");
+            mock.Setup(m => m.Beatmap.Metadata.Title).Returns("title");
+            mock.Setup(m => m.Beatmap.Metadata.Author.Username).Returns("author");
+            mock.Setup(m => m.Beatmap.DifficultyName).Returns("difficulty");
+
+            Assert.That(mock.Object.GetDisplayString(), Is.EqualTo("user playing artist - title (author) [difficulty]"));
+        }
+
+        [Test]
+        public void TestRuleset()
+        {
+            var mock = new Mock<IRulesetInfo>();
+
+            mock.Setup(m => m.Name).Returns("ruleset");
+
+            Assert.That(mock.Object.GetDisplayString(), Is.EqualTo("ruleset"));
+        }
+
+        [Test]
+        public void TestUser()
+        {
+            var mock = new Mock<IUser>();
+
+            mock.Setup(m => m.Username).Returns("user");
+
+            Assert.That(mock.Object.GetDisplayString(), Is.EqualTo("user"));
+        }
+
+        [Test]
+        public void TestFallback()
+        {
+            var fallback = new Fallback();
+
+            Assert.That(fallback.GetDisplayString(), Is.EqualTo("fallback"));
+        }
+
+        private class Fallback
+        {
+            public override string ToString() => "fallback";
+        }
+    }
+}

osu.Game.Tests/Online/TestSceneBeatmapManager.cs

@@ -5,6 +5,7 @@ using NUnit.Framework;
 using osu.Framework.Allocation;
 using osu.Framework.Testing;
 using osu.Game.Beatmaps;
+using osu.Game.Online.API.Requests.Responses;
 using osu.Game.Overlays.Notifications;
 using osu.Game.Tests.Visual;
 
@@ -16,7 +17,30 @@ namespace osu.Game.Tests.Online
         private BeatmapManager beatmaps;
         private ProgressNotification recentNotification;
 
-        private static readonly BeatmapSetInfo test_model = new BeatmapSetInfo { OnlineBeatmapSetID = 1 };
+        private static readonly BeatmapSetInfo test_db_model = new BeatmapSetInfo
+        {
+            OnlineBeatmapSetID = 1,
+            Metadata = new BeatmapMetadata
+            {
+                Artist = "test author",
+                Title = "test title",
+                Author = new APIUser
+                {
+                    Username = "mapper"
+                }
+            }
+        };
+
+        private static readonly APIBeatmapSet test_online_model = new APIBeatmapSet
+        {
+            OnlineID = 2,
+            Artist = "test author",
+            Title = "test title",
+            Author = new APIUser
+            {
+                Username = "mapper"
+            }
+        };
 
         [BackgroundDependencyLoader]
         private void load(BeatmapManager beatmaps)
@@ -26,25 +50,41 @@ namespace osu.Game.Tests.Online
             beatmaps.PostNotification = n => recentNotification = n as ProgressNotification;
         }
 
+        private static readonly object[][] notification_test_cases =
+        {
+            new object[] { test_db_model },
+            new object[] { test_online_model }
+        };
+
+        [TestCaseSource(nameof(notification_test_cases))]
+        public void TestNotificationMessage(IBeatmapSetInfo model)
+        {
+            AddStep("clear recent notification", () => recentNotification = null);
+            AddStep("download beatmap", () => beatmaps.Download(model));
+
+            AddUntilStep("wait for notification", () => recentNotification != null);
+            AddUntilStep("notification text correct", () => recentNotification.Text.ToString() == "Downloading test author - test title (mapper)");
+        }
+
         [Test]
         public void TestCancelDownloadFromRequest()
         {
-            AddStep("download beatmap", () => beatmaps.Download(test_model));
+            AddStep("download beatmap", () => beatmaps.Download(test_db_model));
 
-            AddStep("cancel download from request", () => beatmaps.GetExistingDownload(test_model).Cancel());
+            AddStep("cancel download from request", () => beatmaps.GetExistingDownload(test_db_model).Cancel());
 
-            AddUntilStep("is removed from download list", () => beatmaps.GetExistingDownload(test_model) == null);
+            AddUntilStep("is removed from download list", () => beatmaps.GetExistingDownload(test_db_model) == null);
             AddAssert("is notification cancelled", () => recentNotification.State == ProgressNotificationState.Cancelled);
         }
 
         [Test]
         public void TestCancelDownloadFromNotification()
         {
-            AddStep("download beatmap", () => beatmaps.Download(test_model));
+            AddStep("download beatmap", () => beatmaps.Download(test_db_model));
 
             AddStep("cancel download from notification", () => recentNotification.Close());
 
-            AddUntilStep("is removed from download list", () => beatmaps.GetExistingDownload(test_model) == null);
+            AddUntilStep("is removed from download list", () => beatmaps.GetExistingDownload(test_db_model) == null);
             AddAssert("is notification cancelled", () => recentNotification.State == ProgressNotificationState.Cancelled);
         }
     }

osu.Game.Tests/Visual/Gameplay/TestSceneReplayDownloadButton.cs

@@ -175,7 +175,7 @@ namespace osu.Game.Tests.Visual.Gameplay
                     Id = 39828,
                     Username = @"WubWoofWolf",
                 }
-            }.CreateScoreInfo(rulesets);
+            }.CreateScoreInfo(rulesets, CreateBeatmap(new OsuRuleset().RulesetInfo).BeatmapInfo);
         }
 
         private class TestReplayDownloadButton : ReplayDownloadButton

osu.Game/Beatmaps/BeatmapInfoExtensions.cs

@@ -11,7 +11,7 @@ namespace osu.Game.Beatmaps
         /// <summary>
         /// A user-presentable display title representing this beatmap.
         /// </summary>
-        public static string GetDisplayTitle(this IBeatmapInfo beatmapInfo) => $"{beatmapInfo.Metadata} {getVersionString(beatmapInfo)}".Trim();
+        public static string GetDisplayTitle(this IBeatmapInfo beatmapInfo) => $"{beatmapInfo.Metadata.GetDisplayTitle()} {getVersionString(beatmapInfo)}".Trim();
 
         /// <summary>
         /// A user-presentable display title representing this beatmap, with localisation handling for potentially romanisable fields.

osu.Game/Beatmaps/BeatmapMetadataInfoExtensions.cs

@@ -27,8 +27,12 @@ namespace osu.Game.Beatmaps
         /// </summary>
         public static string GetDisplayTitle(this IBeatmapMetadataInfo metadataInfo)
         {
-            string author = string.IsNullOrEmpty(metadataInfo.Author.Username) ? string.Empty : $"({metadataInfo.Author})";
+            string author = string.IsNullOrEmpty(metadataInfo.Author.Username) ? string.Empty : $" ({metadataInfo.Author.Username})";
-            return $"{metadataInfo.Artist} - {metadataInfo.Title} {author}".Trim();
+
+            string artist = string.IsNullOrEmpty(metadataInfo.Artist) ? "unknown artist" : metadataInfo.Artist;
+            string title = string.IsNullOrEmpty(metadataInfo.Title) ? "unknown title" : metadataInfo.Title;
+
+            return $"{artist} - {title}{author}".Trim();
         }
 
         /// <summary>

osu.Game/Database/ArchiveModelManager.cs

@@ -15,6 +15,7 @@ using osu.Framework.Extensions.IEnumerableExtensions;
 using osu.Framework.Logging;
 using osu.Framework.Platform;
 using osu.Framework.Threading;
+using osu.Game.Extensions;
 using osu.Game.IO;
 using osu.Game.IO.Archives;
 using osu.Game.IPC;
@@ -192,7 +193,7 @@ namespace osu.Game.Database
             else
             {
                 notification.CompletionText = imported.Count == 1
-                    ? $"Imported {imported.First().Value}!"
+                    ? $"Imported {imported.First().Value.GetDisplayString()}!"
                     : $"Imported {imported.Count} {HumanisedModelName}s!";
 
                 if (imported.Count > 0 && PostImport != null)

osu.Game/Database/ModelDownloader.cs

@@ -8,6 +8,7 @@ using System.Threading.Tasks;
 using Humanizer;
 using osu.Framework.Logging;
 using osu.Framework.Platform;
+using osu.Game.Extensions;
 using osu.Game.Online.API;
 using osu.Game.Overlays.Notifications;
 
@@ -50,7 +51,7 @@ namespace osu.Game.Database
 
             DownloadNotification notification = new DownloadNotification
             {
-                Text = $"Downloading {request.Model}",
+                Text = $"Downloading {request.Model.GetDisplayString()}",
             };
 
             request.DownloadProgressed += progress =>

osu.Game/Extensions/ModelExtensions.cs

@@ -0,0 +1,61 @@
+// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
+// See the LICENCE file in the repository root for full licence text.
+
+using osu.Game.Beatmaps;
+using osu.Game.Rulesets;
+using osu.Game.Scoring;
+using osu.Game.Users;
+
+namespace osu.Game.Extensions
+{
+    public static class ModelExtensions
+    {
+        /// <summary>
+        /// Returns a user-facing string representing the <paramref name="model"/>.
+        /// </summary>
+        /// <remarks>
+        /// <para>
+        /// Non-interface types without special handling will fall back to <see cref="object.ToString()"/>.
+        /// </para>
+        /// <para>
+        /// Warning: This method is _purposefully_ not called <c>GetDisplayTitle()</c> like the others, because otherwise
+        /// extension method type inference rules cause this method to call itself and cause a stack overflow.
+        /// </para>
+        /// </remarks>
+        public static string GetDisplayString(this object model)
+        {
+            string result = null;
+
+            switch (model)
+            {
+                case IBeatmapSetInfo beatmapSetInfo:
+                    result = beatmapSetInfo.Metadata?.GetDisplayTitle();
+                    break;
+
+                case IBeatmapInfo beatmapInfo:
+                    result = beatmapInfo.GetDisplayTitle();
+                    break;
+
+                case IBeatmapMetadataInfo metadataInfo:
+                    result = metadataInfo.GetDisplayTitle();
+                    break;
+
+                case IScoreInfo scoreInfo:
+                    result = scoreInfo.GetDisplayTitle();
+                    break;
+
+                case IRulesetInfo rulesetInfo:
+                    result = rulesetInfo.Name;
+                    break;
+
+                case IUser user:
+                    result = user.Username;
+                    break;
+            }
+
+            // fallback in case none of the above happens to match.
+            result ??= model.ToString();
+            return result;
+        }
+    }
+}

osu.Game/Overlays/Notifications/ProgressNotification.cs

@@ -23,9 +23,16 @@ namespace osu.Game.Overlays.Notifications
     {
         private const float loading_spinner_size = 22;
 
+        private LocalisableString text;
+
         public LocalisableString Text
         {
-            set => Schedule(() => textDrawable.Text = value);
+            get => text;
+            set
+            {
+                text = value;
+                Schedule(() => textDrawable.Text = text);
+            }
         }
 
         public string CompletionText { get; set; } = "Task has completed!";

osu.Game/Scoring/ScoreInfo.cs

@@ -225,7 +225,7 @@ namespace osu.Game.Scoring
             return clone;
         }
 
-        public override string ToString() => $"{User} playing {BeatmapInfo}";
+        public override string ToString() => this.GetDisplayTitle();
 
         public bool Equals(ScoreInfo other)
         {

osu.Game/Scoring/ScoreInfoExtensions.cs

@@ -0,0 +1,15 @@
+// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
+// See the LICENCE file in the repository root for full licence text.
+
+using osu.Game.Beatmaps;
+
+namespace osu.Game.Scoring
+{
+    public static class ScoreInfoExtensions
+    {
+        /// <summary>
+        /// A user-presentable display title representing this score.
+        /// </summary>
+        public static string GetDisplayTitle(this IScoreInfo scoreInfo) => $"{scoreInfo.User.Username} playing {scoreInfo.Beatmap.GetDisplayTitle()}";
+    }
+}