From b0b52146eafd10c442c6809d1f4d9f6cb0e1ee56 Mon Sep 17 00:00:00 2001 From: Andrei Zavatski Date: Mon, 24 Feb 2020 05:53:33 +0300 Subject: [PATCH 1/4] Fix crash when clicking on ShowMore button --- osu.Game/Overlays/Comments/CommentsPage.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/osu.Game/Overlays/Comments/CommentsPage.cs b/osu.Game/Overlays/Comments/CommentsPage.cs index 4f82ee5a19..591400f741 100644 --- a/osu.Game/Overlays/Comments/CommentsPage.cs +++ b/osu.Game/Overlays/Comments/CommentsPage.cs @@ -96,12 +96,17 @@ namespace osu.Game.Overlays.Comments { var orphaned = new List(); + // Exclude possible duplicated comments. foreach (var topLevel in bundle.Comments) + { + if (commentDictionary.ContainsKey(topLevel.Id)) + continue; + addNewComment(topLevel); + } foreach (var child in bundle.IncludedComments) { - // Included comments can contain the parent comment, which already exists in the hierarchy. if (commentDictionary.ContainsKey(child.Id)) continue; From fe1f2858c19e8e5bf7726abe121b61c8fc9d13ca Mon Sep 17 00:00:00 2001 From: Andrei Zavatski Date: Mon, 24 Feb 2020 23:10:37 +0300 Subject: [PATCH 2/4] Refactor to avoid duplicated code --- osu.Game/Overlays/Comments/CommentsPage.cs | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/osu.Game/Overlays/Comments/CommentsPage.cs b/osu.Game/Overlays/Comments/CommentsPage.cs index 591400f741..1fb6032924 100644 --- a/osu.Game/Overlays/Comments/CommentsPage.cs +++ b/osu.Game/Overlays/Comments/CommentsPage.cs @@ -97,20 +97,12 @@ namespace osu.Game.Overlays.Comments var orphaned = new List(); // Exclude possible duplicated comments. - foreach (var topLevel in bundle.Comments) + foreach (var comment in bundle.Comments.Concat(bundle.IncludedComments)) { - if (commentDictionary.ContainsKey(topLevel.Id)) + if (commentDictionary.ContainsKey(comment.Id)) continue; - addNewComment(topLevel); - } - - foreach (var child in bundle.IncludedComments) - { - if (commentDictionary.ContainsKey(child.Id)) - continue; - - addNewComment(child); + addNewComment(comment); } // Comments whose parents were seen later than themselves can now be added. From 4cbb2a2f591f07f6a748aa57434bf78554a9c4ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Mon, 24 Feb 2020 21:30:27 +0100 Subject: [PATCH 3/4] Move comment to more pertinent place --- osu.Game/Overlays/Comments/CommentsPage.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Overlays/Comments/CommentsPage.cs b/osu.Game/Overlays/Comments/CommentsPage.cs index 1fb6032924..f3a774908c 100644 --- a/osu.Game/Overlays/Comments/CommentsPage.cs +++ b/osu.Game/Overlays/Comments/CommentsPage.cs @@ -96,9 +96,9 @@ namespace osu.Game.Overlays.Comments { var orphaned = new List(); - // Exclude possible duplicated comments. foreach (var comment in bundle.Comments.Concat(bundle.IncludedComments)) { + // Exclude possible duplicated comments. if (commentDictionary.ContainsKey(comment.Id)) continue; From c1455be85592c3ccf8c0ab95b27c23830927ca53 Mon Sep 17 00:00:00 2001 From: Andrei Zavatski Date: Tue, 25 Feb 2020 10:29:03 +0300 Subject: [PATCH 4/4] Add tests --- .../Visual/Online/TestSceneCommentsPage.cs | 52 +++++++++++++++++-- osu.Game/Overlays/Comments/CommentsPage.cs | 16 +++--- 2 files changed, 57 insertions(+), 11 deletions(-) diff --git a/osu.Game.Tests/Visual/Online/TestSceneCommentsPage.cs b/osu.Game.Tests/Visual/Online/TestSceneCommentsPage.cs index 0ed8864860..a28a0107a1 100644 --- a/osu.Game.Tests/Visual/Online/TestSceneCommentsPage.cs +++ b/osu.Game.Tests/Visual/Online/TestSceneCommentsPage.cs @@ -13,6 +13,8 @@ using osu.Framework.Bindables; using osu.Framework.Graphics.Containers; using osu.Framework.Graphics; using osuTK; +using JetBrains.Annotations; +using NUnit.Framework; namespace osu.Game.Tests.Visual.Online { @@ -30,6 +32,8 @@ namespace osu.Game.Tests.Visual.Online private readonly BindableBool showDeleted = new BindableBool(); private readonly Container content; + private TestCommentsPage commentsPage; + public TestSceneCommentsPage() { Add(new FillFlowContainer @@ -57,15 +61,29 @@ namespace osu.Game.Tests.Visual.Online } } }); + } - AddStep("load comments", () => createPage(getCommentBundle())); - AddStep("load empty comments", () => createPage(getEmptyCommentBundle())); + [Test] + public void TestAppendDuplicatedComment() + { + AddStep("Create page", () => createPage(getCommentBundle())); + AddAssert("Dictionary length is 10", () => commentsPage?.DictionaryLength == 10); + AddStep("Append existing comment", () => commentsPage?.AppendComments(getCommentSubBundle())); + AddAssert("Dictionary length is 10", () => commentsPage?.DictionaryLength == 10); + } + + [Test] + public void TestEmptyBundle() + { + AddStep("Create page", () => createPage(getEmptyCommentBundle())); + AddAssert("Dictionary length is 0", () => commentsPage?.DictionaryLength == 0); } private void createPage(CommentBundle commentBundle) { + commentsPage = null; content.Clear(); - content.Add(new CommentsPage(commentBundle) + content.Add(commentsPage = new TestCommentsPage(commentBundle) { ShowDeleted = { BindTarget = showDeleted } }); @@ -182,5 +200,33 @@ namespace osu.Game.Tests.Visual.Online } }, }; + + private CommentBundle getCommentSubBundle() => new CommentBundle + { + Comments = new List + { + new Comment + { + Id = 1, + Message = "Simple test comment", + LegacyName = "TestUser1", + CreatedAt = DateTimeOffset.Now, + VotesCount = 5 + }, + }, + IncludedComments = new List(), + }; + + private class TestCommentsPage : CommentsPage + { + public TestCommentsPage(CommentBundle commentBundle) + : base(commentBundle) + { + } + + public new void AppendComments([NotNull] CommentBundle bundle) => base.AppendComments(bundle); + + public int DictionaryLength => CommentDictionary.Count; + } } } diff --git a/osu.Game/Overlays/Comments/CommentsPage.cs b/osu.Game/Overlays/Comments/CommentsPage.cs index f3a774908c..9b146b0a7d 100644 --- a/osu.Game/Overlays/Comments/CommentsPage.cs +++ b/osu.Game/Overlays/Comments/CommentsPage.cs @@ -61,15 +61,15 @@ namespace osu.Game.Overlays.Comments return; } - appendComments(commentBundle); + AppendComments(commentBundle); } private DrawableComment getDrawableComment(Comment comment) { - if (commentDictionary.TryGetValue(comment.Id, out var existing)) + if (CommentDictionary.TryGetValue(comment.Id, out var existing)) return existing; - return commentDictionary[comment.Id] = new DrawableComment(comment) + return CommentDictionary[comment.Id] = new DrawableComment(comment) { ShowDeleted = { BindTarget = ShowDeleted }, Sort = { BindTarget = Sort }, @@ -81,25 +81,25 @@ namespace osu.Game.Overlays.Comments { var request = new GetCommentsRequest(CommentableId.Value, Type.Value, Sort.Value, page, drawableComment.Comment.Id); - request.Success += response => Schedule(() => appendComments(response)); + request.Success += response => Schedule(() => AppendComments(response)); api.PerformAsync(request); } - private readonly Dictionary commentDictionary = new Dictionary(); + protected readonly Dictionary CommentDictionary = new Dictionary(); /// /// Appends retrieved comments to the subtree rooted of comments in this page. /// /// The bundle of comments to add. - private void appendComments([NotNull] CommentBundle bundle) + protected void AppendComments([NotNull] CommentBundle bundle) { var orphaned = new List(); foreach (var comment in bundle.Comments.Concat(bundle.IncludedComments)) { // Exclude possible duplicated comments. - if (commentDictionary.ContainsKey(comment.Id)) + if (CommentDictionary.ContainsKey(comment.Id)) continue; addNewComment(comment); @@ -118,7 +118,7 @@ namespace osu.Game.Overlays.Comments // Comments that have no parent are added as top-level comments to the flow. flow.Add(drawableComment); } - else if (commentDictionary.TryGetValue(comment.ParentId.Value, out var parentDrawable)) + else if (CommentDictionary.TryGetValue(comment.ParentId.Value, out var parentDrawable)) { // The comment's parent has already been seen, so the parent<-> child links can be added. comment.ParentComment = parentDrawable.Comment;