From 518e5a2245b60e0429f86229382015fafb5a58d4 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 18 Oct 2017 10:07:20 +0900 Subject: [PATCH 1/5] Make ProgressNotification's status and progress thread-safe Quite regularly a task will hold a reference to a progress notification and udpate it as progress is made. Therefore these operations should be thread-safe. --- .../Notifications/ProgressNotification.cs | 72 +++++++++---------- 1 file changed, 36 insertions(+), 36 deletions(-) diff --git a/osu.Game/Overlays/Notifications/ProgressNotification.cs b/osu.Game/Overlays/Notifications/ProgressNotification.cs index a31291e1b8..58aff16de0 100644 --- a/osu.Game/Overlays/Notifications/ProgressNotification.cs +++ b/osu.Game/Overlays/Notifications/ProgressNotification.cs @@ -25,10 +25,7 @@ namespace osu.Game.Overlays.Notifications public float Progress { get { return progressBar.Progress; } - set - { - progressBar.Progress = value; - } + set { Schedule(() => progressBar.Progress = value); } } protected override void LoadComplete() @@ -44,41 +41,44 @@ namespace osu.Game.Overlays.Notifications get { return state; } set { - bool stateChanged = state != value; - state = value; - - if (IsLoaded) + Schedule(() => { - switch (state) - { - case ProgressNotificationState.Queued: - Light.Colour = colourQueued; - Light.Pulsate = false; - progressBar.Active = false; - break; - case ProgressNotificationState.Active: - Light.Colour = colourActive; - Light.Pulsate = true; - progressBar.Active = true; - break; - case ProgressNotificationState.Cancelled: - Light.Colour = colourCancelled; - Light.Pulsate = false; - progressBar.Active = false; - break; - } - } + bool stateChanged = state != value; + state = value; - if (stateChanged) - { - switch (state) + if (IsLoaded) { - case ProgressNotificationState.Completed: - NotificationContent.MoveToY(-DrawSize.Y / 2, 200, Easing.OutQuint); - this.FadeOut(200).Finally(d => Completed()); - break; + switch (state) + { + case ProgressNotificationState.Queued: + Light.Colour = colourQueued; + Light.Pulsate = false; + progressBar.Active = false; + break; + case ProgressNotificationState.Active: + Light.Colour = colourActive; + Light.Pulsate = true; + progressBar.Active = true; + break; + case ProgressNotificationState.Cancelled: + Light.Colour = colourCancelled; + Light.Pulsate = false; + progressBar.Active = false; + break; + } } - } + + if (stateChanged) + { + switch (state) + { + case ProgressNotificationState.Completed: + NotificationContent.MoveToY(-DrawSize.Y / 2, 200, Easing.OutQuint); + this.FadeOut(200).Finally(d => Completed()); + break; + } + } + }); } } @@ -232,4 +232,4 @@ namespace osu.Game.Overlays.Notifications Completed, Cancelled } -} \ No newline at end of file +} From 7f83cf6780b5fb84e0da6ea6690894438d89d4d9 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 20 Oct 2017 10:05:54 +0900 Subject: [PATCH 2/5] Fix hiding not always working Because we are not sharing a single context, we need to use Update to attach the entity to the local context. --- osu.Game/Beatmaps/BeatmapStore.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/osu.Game/Beatmaps/BeatmapStore.cs b/osu.Game/Beatmaps/BeatmapStore.cs index 8eac35a667..ad4c657619 100644 --- a/osu.Game/Beatmaps/BeatmapStore.cs +++ b/osu.Game/Beatmaps/BeatmapStore.cs @@ -102,6 +102,7 @@ namespace osu.Game.Beatmaps if (beatmap.Hidden) return false; beatmap.Hidden = true; + context.Update(beatmap); context.SaveChanges(); BeatmapHidden?.Invoke(beatmap); @@ -120,6 +121,7 @@ namespace osu.Game.Beatmaps if (!beatmap.Hidden) return false; beatmap.Hidden = false; + context.Update(beatmap); context.SaveChanges(); BeatmapRestored?.Invoke(beatmap); From f69fa0cf1f9ef4a70f8525be5ec05103262df683 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 20 Oct 2017 10:50:00 +0900 Subject: [PATCH 3/5] Fix selection after hiding all difficulties in a set --- osu.Game/Screens/Select/BeatmapCarousel.cs | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs index b8cc9782ca..582eeebd48 100644 --- a/osu.Game/Screens/Select/BeatmapCarousel.cs +++ b/osu.Game/Screens/Select/BeatmapCarousel.cs @@ -52,7 +52,7 @@ namespace osu.Game.Screens.Select Schedule(() => { foreach (var g in newGroups) - addGroup(g); + if (g != null) addGroup(g); computeYPositions(); BeatmapsChanged?.Invoke(); @@ -101,6 +101,9 @@ namespace osu.Game.Screens.Select { var group = createGroup(beatmapSet); + if (group == null) + return; + addGroup(group); computeYPositions(); if (selectedGroup == null) @@ -124,19 +127,23 @@ namespace osu.Game.Screens.Select if (group == null) return; - var newGroup = createGroup(set); - int i = groups.IndexOf(group); groups.RemoveAt(i); - groups.Insert(i, newGroup); - if (selectedGroup == group && newGroup.BeatmapPanels.Count == 0) + var newGroup = createGroup(set); + + if (newGroup != null) + groups.Insert(i, newGroup); + + bool hadSelection = selectedGroup == group; + + if (hadSelection && newGroup == null) selectedGroup = null; Filter(null, false); //check if we can/need to maintain our current selection. - if (selectedGroup == group && newGroup.BeatmapPanels.Count > 0) + if (hadSelection && newGroup != null) { var newSelection = newGroup.BeatmapPanels.Find(p => p.Beatmap.ID == selectedPanel?.Beatmap.ID) ?? @@ -335,6 +342,9 @@ namespace osu.Game.Screens.Select private BeatmapGroup createGroup(BeatmapSetInfo beatmapSet) { + if (beatmapSet.Beatmaps.All(b => b.Hidden)) + return null; + foreach (var b in beatmapSet.Beatmaps) { if (b.Metadata == null) From 93b2fc6dc51ff9ed665e0a4a2ad09a9421b47e0d Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 20 Oct 2017 11:14:45 +0900 Subject: [PATCH 4/5] Fix issues with deletion Main fix is avoiding nullrefs being thrown when metadata isn't present on a beatmap (quite a common scenario). --- osu.Game/Beatmaps/BeatmapStore.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/osu.Game/Beatmaps/BeatmapStore.cs b/osu.Game/Beatmaps/BeatmapStore.cs index 8eac35a667..6451862824 100644 --- a/osu.Game/Beatmaps/BeatmapStore.cs +++ b/osu.Game/Beatmaps/BeatmapStore.cs @@ -66,6 +66,7 @@ namespace osu.Game.Beatmaps if (beatmapSet.DeletePending) return false; beatmapSet.DeletePending = true; + context.Update(beatmapSet); context.SaveChanges(); BeatmapSetRemoved?.Invoke(beatmapSet); @@ -84,6 +85,7 @@ namespace osu.Game.Beatmaps if (!beatmapSet.DeletePending) return false; beatmapSet.DeletePending = false; + context.Update(beatmapSet); context.SaveChanges(); BeatmapSetAdded?.Invoke(beatmapSet); @@ -137,7 +139,7 @@ namespace osu.Game.Beatmaps // metadata is M-N so we can't rely on cascades context.BeatmapMetadata.RemoveRange(purgeable.Select(s => s.Metadata)); - context.BeatmapMetadata.RemoveRange(purgeable.SelectMany(s => s.Beatmaps.Select(b => b.Metadata))); + context.BeatmapMetadata.RemoveRange(purgeable.SelectMany(s => s.Beatmaps.Select(b => b.Metadata).Where(m => m != null))); // todo: we can probably make cascades work here with a FK in BeatmapDifficulty. just make to make it work correctly. context.BeatmapDifficulty.RemoveRange(purgeable.SelectMany(s => s.Beatmaps.Select(b => b.BaseDifficulty))); From f9d5eadd05857d5c5587d93a6924fb219d23803c Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 20 Oct 2017 12:21:18 +0900 Subject: [PATCH 5/5] Fix TestCase failing in an infinite loop --- osu.Game/Tests/Visual/TestCaseNotificationOverlay.cs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/osu.Game/Tests/Visual/TestCaseNotificationOverlay.cs b/osu.Game/Tests/Visual/TestCaseNotificationOverlay.cs index fead5c8b24..ed331076b2 100644 --- a/osu.Game/Tests/Visual/TestCaseNotificationOverlay.cs +++ b/osu.Game/Tests/Visual/TestCaseNotificationOverlay.cs @@ -66,13 +66,11 @@ namespace osu.Game.Tests.Visual progressingNotifications.RemoveAll(n => n.State == ProgressNotificationState.Completed); - while (progressingNotifications.Count(n => n.State == ProgressNotificationState.Active) < 3) + if (progressingNotifications.Count(n => n.State == ProgressNotificationState.Active) < 3) { var p = progressingNotifications.FirstOrDefault(n => n.IsAlive && n.State == ProgressNotificationState.Queued); - if (p == null) - break; - - p.State = ProgressNotificationState.Active; + if (p != null) + p.State = ProgressNotificationState.Active; } foreach (var n in progressingNotifications.FindAll(n => n.State == ProgressNotificationState.Active))