Resolve review comments

osu.Game.Tests/Beatmaps/Formats/LegacyBeatmapEncoderTest.cs

@@ -28,25 +28,27 @@ namespace osu.Game.Tests.Beatmaps.Formats
     [TestFixture]
     public class LegacyBeatmapEncoderTest
     {
-        private static readonly DllResourceStore resource_store = TestResources.GetStore();
+        private static readonly DllResourceStore beatmaps_resource_store = TestResources.GetStore();
 
-        private static IEnumerable<string> allBeatmaps = resource_store.GetAvailableResources().Where(res => res.EndsWith(".osu"));
+        private static IEnumerable<string> allBeatmaps = beatmaps_resource_store.GetAvailableResources().Where(res => res.EndsWith(".osu"));
 
         [TestCaseSource(nameof(allBeatmaps))]
         public void TestEncodeDecodeStability(string name)
         {
-            var decoded = decodeFromLegacy(TestResources.GetStore().GetStream(name));
+            var decoded = decodeFromLegacy(TestResources.GetStore().GetStream(name), name);
-            var decodedAfterEncode = decodeFromLegacy(encodeToLegacy(decoded));
+            var decodedAfterEncode = decodeFromLegacy(encodeToLegacy(decoded), name);
 
-            sort(decoded.beatmap);
+            sort(decoded);
-            sort(decodedAfterEncode.beatmap);
+            sort(decodedAfterEncode);
 
             Assert.That(decodedAfterEncode.beatmap.Serialize(), Is.EqualTo(decoded.beatmap.Serialize()));
             Assert.IsTrue(decoded.beatmapSkin.Configuration.Equals(decodedAfterEncode.beatmapSkin.Configuration));
         }
 
-        private void sort(IBeatmap beatmap)
+        private void sort((IBeatmap, LegacyBeatmapSkin) bs)
         {
+            var (beatmap, beatmapSkin) = bs;
+
             // Sort control points to ensure a sane ordering, as they may be parsed in different orders. This works because each group contains only uniquely-typed control points.
             foreach (var g in beatmap.ControlPointInfo.Groups)
             {
@@ -55,17 +57,26 @@ namespace osu.Game.Tests.Beatmaps.Formats
             }
         }
 
-        private (IBeatmap beatmap, LegacySkin beatmapSkin) decodeFromLegacy(Stream stream)
+        private (IBeatmap beatmap, LegacyBeatmapSkin beatmapSkin) decodeFromLegacy(Stream stream, string name)
         {
             using (var reader = new LineBufferedReader(stream))
             {
                 var beatmap = new LegacyBeatmapDecoder { ApplyOffsets = false }.Decode(reader);
-                var beatmapSkin = new LegacyBeatmapSkin(beatmap.BeatmapInfo, resource_store, null);
+                beatmap.BeatmapInfo.BeatmapSet.Files = new List<BeatmapSetFileInfo>
+                {
+                    new BeatmapSetFileInfo
+                    {
+                        Filename = name,
+                        FileInfo = new osu.Game.IO.FileInfo() { Hash = name }
+                    }
+                };
+
+                var beatmapSkin = new LegacyBeatmapSkin(beatmap.BeatmapInfo, beatmaps_resource_store, null);
                 return (convert(beatmap), beatmapSkin);
             }
         }
 
-        private Stream encodeToLegacy((IBeatmap beatmap, LegacySkin beatmapSkin) fullBeatmap)
+        private Stream encodeToLegacy((IBeatmap beatmap, LegacyBeatmapSkin beatmapSkin) fullBeatmap)
         {
             var (beatmap, beatmapSkin) = fullBeatmap;
             var stream = new MemoryStream();

osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs

@@ -730,8 +730,7 @@ namespace osu.Game.Tests.Beatmaps.IO
                     BeatmapSetInfo setToUpdate = manager.GetAllUsableBeatmapSets()[0];
 
                     var beatmapInfo = setToUpdate.Beatmaps.First(b => b.RulesetID == 0);
-                    var workingBeatmap = manager.GetWorkingBeatmap(setToUpdate.Beatmaps.First(b => b.RulesetID == 0));
+                    Beatmap beatmapToUpdate = (Beatmap)manager.GetWorkingBeatmap(setToUpdate.Beatmaps.First(b => b.RulesetID == 0)).Beatmap;
-                    Beatmap beatmapToUpdate = (Beatmap)workingBeatmap.Beatmap;
                     BeatmapSetFileInfo fileToUpdate = setToUpdate.Files.First(f => beatmapToUpdate.BeatmapInfo.Path.Contains(f.Filename));
 
                     string oldMd5Hash = beatmapToUpdate.BeatmapInfo.MD5Hash;
@@ -739,7 +738,7 @@ namespace osu.Game.Tests.Beatmaps.IO
                     beatmapToUpdate.HitObjects.Clear();
                     beatmapToUpdate.HitObjects.Add(new HitCircle { StartTime = 5000 });
 
-                    manager.Save(beatmapInfo, beatmapToUpdate, workingBeatmap.Skin);
+                    manager.Save(beatmapInfo, beatmapToUpdate);
 
                     // Check that the old file reference has been removed
                     Assert.That(manager.QueryBeatmapSet(s => s.ID == setToUpdate.ID).Files.All(f => f.ID != fileToUpdate.ID));

osu.Game/Beatmaps/BeatmapManager.cs

@@ -195,8 +195,8 @@ namespace osu.Game.Beatmaps
         /// </summary>
         /// <param name="info">The <see cref="BeatmapInfo"/> to save the content against. The file referenced by <see cref="BeatmapInfo.Path"/> will be replaced.</param>
         /// <param name="beatmapContent">The <see cref="IBeatmap"/> content to write.</param>
-        /// <param name="skin">Optional beatmap skin for inline skin configuration in beatmap files.</param>
+        /// <param name="beatmapSkin">The beatmap <see cref="ISkin"/> content to write, or null if not to be changed.</param>
-        public void Save(BeatmapInfo info, IBeatmap beatmapContent, ISkin skin)
+        public void Save(BeatmapInfo info, IBeatmap beatmapContent, LegacyBeatmapSkin beatmapSkin = null)
         {
             var setInfo = QueryBeatmapSet(s => s.Beatmaps.Any(b => b.ID == info.ID));
 
@@ -204,7 +204,13 @@ namespace osu.Game.Beatmaps
             {
                 using (var sw = new StreamWriter(stream, Encoding.UTF8, 1024, true))
                 {
-                    new LegacyBeatmapEncoder(beatmapContent, skin).Encode(sw);
+                    if (beatmapSkin == null)
+                    {
+                        var workingBeatmap = GetWorkingBeatmap(info);
+                        beatmapSkin = (workingBeatmap.Skin is LegacyBeatmapSkin legacy) ? legacy : null;
+                    }
+
+                    new LegacyBeatmapEncoder(beatmapContent, beatmapSkin).Encode(sw);
                 }
 
                 stream.Seek(0, SeekOrigin.Begin);

osu.Game/Beatmaps/Formats/IHasCustomColours.cs

@@ -8,6 +8,6 @@ namespace osu.Game.Beatmaps.Formats
 {
     public interface IHasCustomColours
     {
-        Dictionary<string, Color4> CustomColours { get; set; }
+        IDictionary<string, Color4> CustomColours { get; }
     }
 }

osu.Game/Beatmaps/Formats/LegacyBeatmapEncoder.cs

@@ -16,6 +16,7 @@ using osu.Game.Rulesets.Objects.Legacy;
 using osu.Game.Rulesets.Objects.Types;
 using osu.Game.Skinning;
 using osuTK;
+using osuTK.Graphics;
 
 namespace osu.Game.Beatmaps.Formats
 {
@@ -24,11 +25,14 @@ namespace osu.Game.Beatmaps.Formats
         public const int LATEST_VERSION = 128;
 
         private readonly IBeatmap beatmap;
-        private readonly ISkin skin;
+        private readonly LegacyBeatmapSkin skin;
 
-        /// <param name="beatmap">The beatmap to encode</param>
+        /// <summary>
-        /// <param name="skin">An optional skin, for encoding the beatmap's combo colours. This will only work if the parameter is a type of <see cref="LegacyBeatmapSkin"/>.</param>
+        /// Creates a new <see cref="LegacyBeatmapEncoder"/>.
-        public LegacyBeatmapEncoder(IBeatmap beatmap, [CanBeNull] ISkin skin)
+        /// </summary>
+        /// <param name="beatmap">The beatmap to encode.</param>
+        /// <param name="skin">An optional skin, for encoding the beatmap's combo colours.</param>
+        public LegacyBeatmapEncoder(IBeatmap beatmap, [CanBeNull] LegacyBeatmapSkin skin)
         {
             this.beatmap = beatmap;
             this.skin = skin;
@@ -210,7 +214,7 @@ namespace osu.Game.Beatmaps.Formats
             if (!(skin is LegacyBeatmapSkin legacySkin))
                 return;
 
-            var colours = legacySkin?.Configuration.ComboColours;
+            var colours = legacySkin.GetConfig<GlobalSkinColours, IReadOnlyList<Color4>>(GlobalSkinColours.ComboColours)?.Value;
 
             if (colours == null || colours.Count == 0)
                 return;
@@ -221,12 +225,11 @@ namespace osu.Game.Beatmaps.Formats
             {
                 var comboColour = colours[i];
 
-                var r = (byte)(comboColour.R * byte.MaxValue);
+                writer.Write(FormattableString.Invariant($"Combo{i}: "));
-                var g = (byte)(comboColour.G * byte.MaxValue);
+                writer.Write(FormattableString.Invariant($"{(byte)(comboColour.R * byte.MaxValue)},"));
-                var b = (byte)(comboColour.B * byte.MaxValue);
+                writer.Write(FormattableString.Invariant($"{(byte)(comboColour.G * byte.MaxValue)},"));
-                var a = (byte)(comboColour.A * byte.MaxValue);
+                writer.Write(FormattableString.Invariant($"{(byte)(comboColour.B * byte.MaxValue)},"));
-
+                writer.WriteLine(FormattableString.Invariant($"{(byte)(comboColour.A * byte.MaxValue)}"));
-                writer.WriteLine($"Combo{i}: {r},{g},{b},{a}");
             }
         }
 

osu.Game/Screens/Edit/Editor.cs

@@ -65,7 +65,7 @@ namespace osu.Game.Screens.Edit
         private IBeatmap playableBeatmap;
         private EditorBeatmap editorBeatmap;
         private EditorChangeHandler changeHandler;
-        private ISkin beatmapSkin;
+        private LegacyBeatmapSkin beatmapSkin;
 
         private DependencyContainer dependencies;
 
@@ -94,7 +94,6 @@ namespace osu.Game.Screens.Edit
             try
             {
                 playableBeatmap = Beatmap.Value.GetPlayableBeatmap(Beatmap.Value.BeatmapInfo.Ruleset);
-                beatmapSkin = Beatmap.Value.Skin;
             }
             catch (Exception e)
             {
@@ -107,6 +106,7 @@ namespace osu.Game.Screens.Edit
             AddInternal(editorBeatmap = new EditorBeatmap(playableBeatmap));
             dependencies.CacheAs(editorBeatmap);
 
+            beatmapSkin = (Beatmap.Value.Skin is LegacyBeatmapSkin legacy) ? legacy : null;
             changeHandler = new EditorChangeHandler(editorBeatmap, beatmapSkin);
             dependencies.CacheAs<IEditorChangeHandler>(changeHandler);
 

osu.Game/Screens/Edit/EditorChangeHandler.cs

@@ -26,7 +26,7 @@ namespace osu.Game.Screens.Edit
         private int currentState = -1;
 
         private readonly EditorBeatmap editorBeatmap;
-        private readonly ISkin beatmapSkin;
+        private readonly LegacyBeatmapSkin beatmapSkin;
         private int bulkChangesStarted;
         private bool isRestoring;
 
@@ -37,7 +37,7 @@ namespace osu.Game.Screens.Edit
         /// </summary>
         /// <param name="editorBeatmap">The <see cref="EditorBeatmap"/> to track the <see cref="HitObject"/>s of.</param>
         /// <param name="beatmapSkin">The skin to track the inline skin configuration of.</param>
-        public EditorChangeHandler(EditorBeatmap editorBeatmap, ISkin beatmapSkin)
+        public EditorChangeHandler(EditorBeatmap editorBeatmap, LegacyBeatmapSkin beatmapSkin)
         {
             this.editorBeatmap = editorBeatmap;
 

osu.Game/Skinning/LegacyManiaSkinConfiguration.cs

@@ -23,7 +23,7 @@ namespace osu.Game.Skinning
 
         public readonly int Keys;
 
-        public Dictionary<string, Color4> CustomColours { get; set; } = new Dictionary<string, Color4>();
+        public IDictionary<string, Color4> CustomColours { get; set; } = new SortedDictionary<string, Color4>();
 
         public Dictionary<string, string> ImageLookups = new Dictionary<string, string>();
 

osu.Game/Skinning/SkinConfiguration.cs

@@ -12,7 +12,7 @@ namespace osu.Game.Skinning
     /// <summary>
     /// An empty skin configuration.
     /// </summary>
-    public class SkinConfiguration : IHasComboColours, IHasCustomColours, IEquatable<SkinConfiguration>
+    public class SkinConfiguration : IEquatable<SkinConfiguration>, IHasComboColours, IHasCustomColours
     {
         public readonly SkinInfo SkinInfo = new SkinInfo();
 
@@ -47,16 +47,14 @@ namespace osu.Game.Skinning
 
         public void AddComboColours(params Color4[] colours) => comboColours.AddRange(colours);
 
-        public Dictionary<string, Color4> CustomColours { get; set; } = new Dictionary<string, Color4>();
+        public IDictionary<string, Color4> CustomColours { get; set; } = new SortedDictionary<string, Color4>();
 
-        public readonly Dictionary<string, string> ConfigDictionary = new Dictionary<string, string>();
+        public readonly SortedDictionary<string, string> ConfigDictionary = new SortedDictionary<string, string>();
 
         public bool Equals(SkinConfiguration other)
-        {
+            => other != null
-            return other != null &&
+            && ConfigDictionary.SequenceEqual(other.ConfigDictionary)
-                   ConfigDictionary.SequenceEqual(other.ConfigDictionary) &&
+            && ComboColours.SequenceEqual(other.ComboColours)
-                   ComboColours.SequenceEqual(other.ComboColours) &&
+            && CustomColours?.SequenceEqual(other.CustomColours) == true;
-                   CustomColours?.SequenceEqual(other.CustomColours) == true;
-        }
     }
 }